| Message ID | 20230517115955.1078339-1-badhri@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] usb: gadget: udc: core: Offload usb_udc_vbus_handler processing | expand |
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@google.com> 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@vger.kernel.org > Reported-by: Stephan Gerhold <stephan@gerhold.net> > Closes: https://lore.kernel.org/all/ZF4bMptC3Lf2Hnee@gerhold.net/ > Reported-by: Francesco Dolcini <francesco.dolcini@toradex.com> > Closes: https://lore.kernel.org/all/ZF4BvgsOyoKxdPFF@francesco-nb.int.toradex.com/ > Reported-by: Alistair <alistair@alistair23.me> > Closes: https://lore.kernel.org/lkml/0cf8c588b701d7cf25ffe1a9217b81716e6a5c51.camel@alistair23.me/ > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> > --- > 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 >
On Wed, May 17, 2023 at 11:59:55AM +0000, Badhri Jagan Sridharan 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. Surely that is the wrong approach. The real problem here is that usb_udc_vbus_handler() gets called from within a udc_start routine. But this is totally unnecessary, because the UDC core will call usb_udc_connect_control_locked() itself, later on during gadget_bind_driver(). So a proper solution would be simply to remove the unnecessary usb_udc_vbus_handler() call from the chipidea driver (and similarly for the max3420_udc driver). Similar changes may be needed in these drivers' udc_stop routines. Alan Stern
On Wed, May 17, 2023 at 7:44 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Wed, May 17, 2023 at 11:59:55AM +0000, Badhri Jagan Sridharan 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. > > Surely that is the wrong approach. > > The real problem here is that usb_udc_vbus_handler() gets called from > within a udc_start routine. But this is totally unnecessary, because > the UDC core will call usb_udc_connect_control_locked() itself, later on > during gadget_bind_driver(). Hi Alan, usb_udc_vbus_handler sets the udc->vbus flag as well apart from calling usb_udc_connect_control_locked(). So, removing usb_udc_vbus_handler from chip specific start callback might prevent the controller from starting. void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status) { struct usb_udc *udc = gadget->udc; mutex_lock(&udc->connect_lock); if (udc) { udc->vbus = status; usb_udc_connect_control_locked(udc); Thanks, Badhri > > So a proper solution would be simply to remove the unnecessary > usb_udc_vbus_handler() call from the chipidea driver (and similarly for > the max3420_udc driver). > > Similar changes may be needed in these drivers' udc_stop routines. > > Alan Stern
On Wed, May 17, 2023 at 10:19:25AM -0700, Badhri Jagan Sridharan wrote: > On Wed, May 17, 2023 at 7:44 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > On Wed, May 17, 2023 at 11:59:55AM +0000, Badhri Jagan Sridharan 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. > > > > Surely that is the wrong approach. > > > > The real problem here is that usb_udc_vbus_handler() gets called from > > within a udc_start routine. But this is totally unnecessary, because > > the UDC core will call usb_udc_connect_control_locked() itself, later on > > during gadget_bind_driver(). > > Hi Alan, > > usb_udc_vbus_handler sets the udc->vbus flag as well apart from > calling usb_udc_connect_control_locked(). So, removing usb_udc_vbus_handler > from chip specific start callback might prevent the controller from > starting. > > void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status) > { > struct usb_udc *udc = gadget->udc; > > mutex_lock(&udc->connect_lock); > if (udc) { > udc->vbus = status; > usb_udc_connect_control_locked(udc); Then add "udc->vbus = true;" at the appropriate spot in gadget_bind_driver(). Alan Stern PS: I just noticed that in max3420_udc.c, the max_3420_vbus_handler() function calls usb_udc_vbus_handler() from within an interrupt handler. This won't work, since interrupt handlers aren't allowed to sleep and therefore can't lock mutexes.
On Wed, May 17, 2023 at 1:01 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Wed, May 17, 2023 at 10:19:25AM -0700, Badhri Jagan Sridharan wrote: > > On Wed, May 17, 2023 at 7:44 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > On Wed, May 17, 2023 at 11:59:55AM +0000, Badhri Jagan Sridharan 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. > > > > > > Surely that is the wrong approach. > > > > > > The real problem here is that usb_udc_vbus_handler() gets called from > > > within a udc_start routine. But this is totally unnecessary, because > > > the UDC core will call usb_udc_connect_control_locked() itself, later on > > > during gadget_bind_driver(). > > > > Hi Alan, > > > > usb_udc_vbus_handler sets the udc->vbus flag as well apart from > > calling usb_udc_connect_control_locked(). So, removing usb_udc_vbus_handler > > from chip specific start callback might prevent the controller from > > starting. > > > > void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status) > > { > > struct usb_udc *udc = gadget->udc; > > > > mutex_lock(&udc->connect_lock); > > if (udc) { > > udc->vbus = status; > > usb_udc_connect_control_locked(udc); > > Then add "udc->vbus = true;" at the appropriate spot in > gadget_bind_driver(). Not sure if I am misunderstanding something. "udc->vbus = true" is set by usb_udc_vbus_handler based on invocation from the chip level gadget driver and gadget_bind_driver() does not seem to have the context for udc->vbus. Do you still think it makes sense to add "udc->vbus = true;" to gadget_bind_driver() ? > > > Alan Stern > > PS: I just noticed that in max3420_udc.c, the max_3420_vbus_handler() > function calls usb_udc_vbus_handler() from within an interrupt handler. > This won't work, since interrupt handlers aren't allowed to sleep and > therefore can't lock mutexes. Good point ! I didn't notice that usb_udc_vbus_handler() is invoked from interrupt context as well. I was looking at turning connect_lock into a spin lock. But looks like udc_lock which is acquired in usb_gadget_disconnect_locked is a mutex, So keeping connect_lock as mutex and changing vbus_events_lock into spin_lock is what that seems to be possible. Sending out V2 of this patch with these changes so that it's easier to see what I am referring to. Eager to know your thoughts ! Thanks, Badhri
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;
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@vger.kernel.org Reported-by: Stephan Gerhold <stephan@gerhold.net> Closes: https://lore.kernel.org/all/ZF4bMptC3Lf2Hnee@gerhold.net/ Reported-by: Francesco Dolcini <francesco.dolcini@toradex.com> Closes: https://lore.kernel.org/all/ZF4BvgsOyoKxdPFF@francesco-nb.int.toradex.com/ Reported-by: Alistair <alistair@alistair23.me> Closes: https://lore.kernel.org/lkml/0cf8c588b701d7cf25ffe1a9217b81716e6a5c51.camel@alistair23.me/ Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> --- drivers/usb/gadget/udc/core.c | 55 +++++++++++++++++++++++++++++++---- 1 file changed, 49 insertions(+), 6 deletions(-) base-commit: 6bae03b0484b54f699d69339fbec5658e885c224