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: [email protected]
Reported-by: Stephan Gerhold <[email protected]>
Closes: https://lore.kernel.org/all/[email protected]/
Reported-by: Francesco Dolcini <[email protected]>
Closes: https://lore.kernel.org/all/[email protected]/
Reported-by: Alistair <[email protected]>
Closes: https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Badhri Jagan Sridharan <[email protected]>
---
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
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
<[email protected]> 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: [email protected]
> Reported-by: Stephan Gerhold <[email protected]>
> Closes: https://lore.kernel.org/all/[email protected]/
> Reported-by: Francesco Dolcini <[email protected]>
> Closes: https://lore.kernel.org/all/[email protected]/
> Reported-by: Alistair <[email protected]>
> Closes: https://lore.kernel.org/lkml/[email protected]/
> Signed-off-by: Badhri Jagan Sridharan <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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