[PATCH v2] usb: gadget: udc: core: Offload usb_udc_vbus_handler processing

From: Badhri Jagan Sridharan
Date: Fri May 19 2023 - 00:30:59 EST


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>
---
Changes since v1:
- Address Alan Stern's comment on usb_udc_vbus_handler invocation from
atomic context:
* vbus_events_lock is now a spinlock and allocations in
* usb_udc_vbus_handler are atomic now.
---
drivers/usb/gadget/udc/core.c | 63 +++++++++++++++++++++++++++++++----
1 file changed, 57 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 69041cca5d24..ee612387b39c 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;
+ spinlock_t 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,30 @@ 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);
+ unsigned long flags;
+
+ spin_lock_irqsave(&udc->vbus_events_lock, flags);
+ list_for_each_entry_safe(event, n, &udc->vbus_events, node) {
+ list_del(&event->node);
+ /* OK to drop the lock here as it suffice to syncrhronize udc->vbus_events node
+ * retrieval and deletion against usb_udc_vbus_handler. usb_udc_vbus_handler does
+ * list_add_tail so n would be the same even if the lock is dropped.
+ */
+ spin_unlock_irqrestore(&udc->vbus_events_lock, flags);
+ mutex_lock(&udc->connect_lock);
+ udc->vbus = event->vbus_on;
+ usb_udc_connect_control_locked(udc);
+ kfree(event);
+ mutex_unlock(&udc->connect_lock);
+ spin_lock_irqsave(&udc->vbus_events_lock, flags);
+ }
+ spin_unlock_irqrestore(&udc->vbus_events_lock, flags);
+}
+
/**
* usb_udc_vbus_handler - updates the udc core vbus status, and try to
* connect or disconnect gadget
@@ -1146,13 +1186,21 @@ 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;
+ unsigned long flags;

- 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_ATOMIC);
+ if (!vbus_event)
+ return;
+
+ spin_lock_irqsave(&udc->vbus_events_lock, flags);
+ vbus_event->vbus_on = status;
+ list_add_tail(&vbus_event->node, &udc->vbus_events);
+ spin_unlock_irqrestore(&udc->vbus_events_lock, flags);
+ schedule_work(&udc->vbus_work);
}
EXPORT_SYMBOL_GPL(usb_udc_vbus_handler);

@@ -1379,6 +1427,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);
+ spin_lock_init(&udc->vbus_events_lock);
+ INIT_WORK(&udc->vbus_work, vbus_event_work);

udc->started = false;


base-commit: a4422ff221429c600c3dc5d0394fb3738b89d040
--
2.40.1.698.g37aff9b760-goog