Re: [PATCH v1] usb: gadget: udc: core: Offload usb_udc_vbus_handler processing

[Badhri Jagan Sridharan]

Hi Francesco, Alistair, Stephan, Bagas, Lucas,

Thanks for sharing the stack traces !
Requesting your help on validating the patch as I dont have the same
hardware with me which reproduces the issue.
Let me know if you are still seeing the crash.

Thanks,
Badhri

On Wed, May 17, 2023 at 5:00 AM Badhri Jagan Sridharan
<badhri@xxxxxxxxxx> wrote:
>
> chipidea udc calls usb_udc_vbus_handler from udc_start gadget
> ops causing a deadlock. Avoid this by offloading usb_udc_vbus_handler
> processing.
>
> ============================================
> WARNING: possible recursive locking detected
> 640-rc1-000-devel-00005-gcda3c69ebc14 #1 Not tainted
> -------------------------------------------
>
> CPU0
> ----
> lock(&udc->connect_lock);
> lock(&udc->connect_lock);
>
>  DEADLOCK
>
> stack backtrace:
>   CPU: 1 PID: 566 Comm: echo Not tainted 640-rc1-000-devel-00005-gcda3c69ebc14 #1
>   Hardware name: Freescale iMX7 Dual (Device Tree)
>   unwind_backtrace from show_stack+0x10/0x14
>   show_stack from dump_stack_lvl+0x70/0xb0
>   dump_stack_lvl from __lock_acquire+0x924/0x22c4
>   __lock_acquire from lock_acquire+0x100/0x370
>   lock_acquire from __mutex_lock+0xa8/0xfb4
>   __mutex_lock from mutex_lock_nested+0x1c/0x24
>   mutex_lock_nested from usb_udc_vbus_handler+0x1c/0x60
>   usb_udc_vbus_handler from ci_udc_start+0x74/0x9c
>   ci_udc_start from gadget_bind_driver+0x130/0x230
>   gadget_bind_driver from really_probe+0xd8/0x3fc
>   really_probe from __driver_probe_device+0x94/0x1f0
>   __driver_probe_device from driver_probe_device+0x2c/0xc4
>   driver_probe_device from __driver_attach+0x114/0x1cc
>   __driver_attach from bus_for_each_dev+0x7c/0xcc
>   bus_for_each_dev from bus_add_driver+0xd4/0x200
>   bus_add_driver from driver_register+0x7c/0x114
>   driver_register from usb_gadget_register_driver_owner+0x40/0xe0
>   usb_gadget_register_driver_owner from gadget_dev_desc_UDC_store+0xd4/0x110
>   gadget_dev_desc_UDC_store from configfs_write_iter+0xac/0x118
>   configfs_write_iter from vfs_write+0x1b4/0x40c
>   vfs_write from ksys_write+0x70/0xf8
>   ksys_write from ret_fast_syscall+0x0/0x1c
>
> Fixes: 0db213ea8eed ("usb: gadget: udc: core: Invoke usb_gadget_connect only when started")
> Cc: stable@xxxxxxxxxxxxxxx
> Reported-by: Stephan Gerhold <stephan@xxxxxxxxxxx>
> Closes: https://lore.kernel.org/all/ZF4bMptC3Lf2Hnee@xxxxxxxxxxx/
> Reported-by: Francesco Dolcini <francesco.dolcini@xxxxxxxxxxx>
> Closes: https://lore.kernel.org/all/ZF4BvgsOyoKxdPFF@xxxxxxxxxxxxxxxxxxxxxxxxxxxx/
> Reported-by: Alistair <alistair@xxxxxxxxxxxxx>
> Closes: https://lore.kernel.org/lkml/0cf8c588b701d7cf25ffe1a9217b81716e6a5c51.camel@xxxxxxxxxxxxx/
> Signed-off-by: Badhri Jagan Sridharan <badhri@xxxxxxxxxx>
> ---
>  drivers/usb/gadget/udc/core.c | 55 +++++++++++++++++++++++++++++++----
>  1 file changed, 49 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index 69041cca5d24..bbcfab50f657 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -41,6 +41,9 @@ static const struct bus_type gadget_bus_type;
>   * functions. usb_gadget_connect_locked, usb_gadget_disconnect_locked,
>   * usb_udc_connect_control_locked, usb_gadget_udc_start_locked, usb_gadget_udc_stop_locked are
>   * called with this lock held.
> + * @vbus_events: list head for processing vbus updates on usb_udc_vbus_handler.
> + * @vbus_events_lock: protects vbus_events list
> + * @vbus_work: work item that invokes usb_udc_connect_control_locked.
>   *
>   * This represents the internal data structure which is used by the UDC-class
>   * to hold information about udc driver and gadget together.
> @@ -53,6 +56,19 @@ struct usb_udc {
>         bool                            vbus;
>         bool                            started;
>         struct mutex                    connect_lock;
> +       struct list_head                vbus_events;
> +       struct mutex                    vbus_events_lock;
> +       struct work_struct              vbus_work;
> +};
> +
> +/**
> + * struct vbus_event - used to notify vbus updates posted through usb_udc_vbus_handler.
> + * @vbus_on: true when vbus is on. false other wise.
> + * @node: list node for maintaining a list of pending updates to be processed.
> + */
> +struct vbus_event {
> +       bool vbus_on;
> +       struct list_head node;
>  };
>
>  static struct class *udc_class;
> @@ -1134,6 +1150,23 @@ static int usb_udc_connect_control_locked(struct usb_udc *udc) __must_hold(&udc-
>         return ret;
>  }
>
> +static void vbus_event_work(struct work_struct *work)
> +{
> +       struct vbus_event *event, *n;
> +       struct usb_udc *udc = container_of(work, struct usb_udc, vbus_work);
> +
> +       mutex_lock(&udc->vbus_events_lock);
> +       list_for_each_entry_safe(event, n, &udc->vbus_events, node) {
> +               mutex_lock(&udc->connect_lock);
> +               udc->vbus = event->vbus_on;
> +               usb_udc_connect_control_locked(udc);
> +               list_del(&event->node);
> +               kfree(event);
> +               mutex_unlock(&udc->connect_lock);
> +       }
> +       mutex_unlock(&udc->vbus_events_lock);
> +}
> +
>  /**
>   * usb_udc_vbus_handler - updates the udc core vbus status, and try to
>   * connect or disconnect gadget
> @@ -1146,13 +1179,20 @@ static int usb_udc_connect_control_locked(struct usb_udc *udc) __must_hold(&udc-
>  void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status)
>  {
>         struct usb_udc *udc = gadget->udc;
> +       struct vbus_event *vbus_event;
>
> -       mutex_lock(&udc->connect_lock);
> -       if (udc) {
> -               udc->vbus = status;
> -               usb_udc_connect_control_locked(udc);
> -       }
> -       mutex_unlock(&udc->connect_lock);
> +       if (!udc)
> +               return;
> +
> +       vbus_event = kzalloc(sizeof(*vbus_event), GFP_KERNEL);
> +       if (!vbus_event)
> +               return;
> +
> +       mutex_lock(&udc->vbus_events_lock);
> +       vbus_event->vbus_on = status;
> +       list_add_tail(&vbus_event->node, &udc->vbus_events);
> +       mutex_unlock(&udc->vbus_events_lock);
> +       schedule_work(&udc->vbus_work);
>  }
>  EXPORT_SYMBOL_GPL(usb_udc_vbus_handler);
>
> @@ -1379,6 +1419,9 @@ int usb_add_gadget(struct usb_gadget *gadget)
>         udc->gadget = gadget;
>         gadget->udc = udc;
>         mutex_init(&udc->connect_lock);
> +       INIT_LIST_HEAD(&udc->vbus_events);
> +       mutex_init(&udc->vbus_events_lock);
> +       INIT_WORK(&udc->vbus_work, vbus_event_work);
>
>         udc->started = false;
>
>
> base-commit: 6bae03b0484b54f699d69339fbec5658e885c224
> --
> 2.40.1.606.ga4b1b128d6-goog
>