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@... 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