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

  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>
---
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(-)


base-commit: a4422ff221429c600c3dc5d0394fb3738b89d040
  

On Fri, May 19, 2023 at 04:30:41AM +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.

Look, this is way overkill.

usb_udc_vbus_handler() has only two jobs to do: set udc->vbus and call 
usb_udc_connect_control().  Furthermore, it gets called from only two 
drivers: chipidea and max3420.

Why not have the callers set udc->vbus themselves and then call 
usb_gadget_{dis}connect() directly?  Then we could eliminate 
usb_udc_vbus_handler() entirely.  And the unnecessary calls -- the ones 
causing deadlocks -- from within udc_start() and udc_stop() handlers can 
be removed with no further consequence.

This approach simplifies and removes code.  Whereas your approach 
complicates and adds code for no good reason.

Alan Stern
  

On Fri, May 19, 2023 at 10:49:49AM -0400, Alan Stern wrote:
> On Fri, May 19, 2023 at 04:30:41AM +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.
> 
> Look, this is way overkill.
> 
> usb_udc_vbus_handler() has only two jobs to do: set udc->vbus and call 
> usb_udc_connect_control().  Furthermore, it gets called from only two 
> drivers: chipidea and max3420.
> 
> Why not have the callers set udc->vbus themselves and then call 
> usb_gadget_{dis}connect() directly?  Then we could eliminate 
> usb_udc_vbus_handler() entirely.  And the unnecessary calls -- the ones 
> causing deadlocks -- from within udc_start() and udc_stop() handlers can 
> be removed with no further consequence.
> 
> This approach simplifies and removes code.  Whereas your approach 
> complicates and adds code for no good reason.

I changed my mind.

After looking more closely, I found the comment in gadget.h about 
->disconnect() callbacks happening in interrupt context.  This means we 
cannot use a mutex to protect the associated state, and therefore the 
connect_lock _must_ be a spinlock, not a mutex.

This also probably means that udc_start and udc_stop callbacks should 
not be invoked with the lock held.  In fact, you might want to avoid 
using the lock at all with gadget_bind_driver() and 
gadget_unbind_driver() -- use it only in the functions that these 
routines call.

So it appears the whole connect_lock thing needs to be redesigned with 
these ideas in mind.  However, it's still true that the UDC drivers 
shouldn't try to set the connection state from within their udc_start 
and udc_stop callbacks, because the core takes care of this 
automatically.

Alan Stern
  

On Fri, May 19, 2023 at 8:07 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Fri, May 19, 2023 at 10:49:49AM -0400, Alan Stern wrote:
> > On Fri, May 19, 2023 at 04:30:41AM +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.
> >
> > Look, this is way overkill.
> >
> > usb_udc_vbus_handler() has only two jobs to do: set udc->vbus and call
> > usb_udc_connect_control().  Furthermore, it gets called from only two
> > drivers: chipidea and max3420.
> >
> > Why not have the callers set udc->vbus themselves and then call
> > usb_gadget_{dis}connect() directly?  Then we could eliminate
> > usb_udc_vbus_handler() entirely.  And the unnecessary calls -- the ones
> > causing deadlocks -- from within udc_start() and udc_stop() handlers can
> > be removed with no further consequence.
> >
> > This approach simplifies and removes code.  Whereas your approach
> > complicates and adds code for no good reason.
>
> I changed my mind.
>
> After looking more closely, I found the comment in gadget.h about
> ->disconnect() callbacks happening in interrupt context.  This means we
> cannot use a mutex to protect the associated state, and therefore the
> connect_lock _must_ be a spinlock, not a mutex.

Quick observation so that I don't misunderstand.
I already see gadget->udc->driver->disconnect(gadget) being called with
udc_lock being held.

               mutex_lock(&udc_lock);
               if (gadget->udc->driver)
                       gadget->udc->driver->disconnect(gadget);
               mutex_unlock(&udc_lock);

The below patch seems to have introduced it:
1016fc0c096c USB: gadget: Fix obscure lockdep violation for udc_mutex

Are you referring to some other ->disconnect() callback ? If so, can you point
me to which one ?

>
> This also probably means that udc_start and udc_stop callbacks should
> not be invoked with the lock held.  In fact, you might want to avoid
> using the lock at all with gadget_bind_driver() and
> gadget_unbind_driver() -- use it only in the functions that these
> routines call.
>
> So it appears the whole connect_lock thing needs to be redesigned with
> these ideas in mind.  However, it's still true that the UDC drivers
> shouldn't try to set the connection state from within their udc_start
> and udc_stop callbacks, because the core takes care of this
> automatically.
>
> Alan Stern

Thanks for your inputs !
Badhri
  

On Fri, May 19, 2023 at 08:44:57AM -0700, Badhri Jagan Sridharan wrote:
> On Fri, May 19, 2023 at 8:07 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Fri, May 19, 2023 at 10:49:49AM -0400, Alan Stern wrote:
> > > On Fri, May 19, 2023 at 04:30:41AM +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.
> > >
> > > Look, this is way overkill.
> > >
> > > usb_udc_vbus_handler() has only two jobs to do: set udc->vbus and call
> > > usb_udc_connect_control().  Furthermore, it gets called from only two
> > > drivers: chipidea and max3420.
> > >
> > > Why not have the callers set udc->vbus themselves and then call
> > > usb_gadget_{dis}connect() directly?  Then we could eliminate
> > > usb_udc_vbus_handler() entirely.  And the unnecessary calls -- the ones
> > > causing deadlocks -- from within udc_start() and udc_stop() handlers can
> > > be removed with no further consequence.
> > >
> > > This approach simplifies and removes code.  Whereas your approach
> > > complicates and adds code for no good reason.
> >
> > I changed my mind.
> >
> > After looking more closely, I found the comment in gadget.h about
> > ->disconnect() callbacks happening in interrupt context.  This means we
> > cannot use a mutex to protect the associated state, and therefore the
> > connect_lock _must_ be a spinlock, not a mutex.
> 
> Quick observation so that I don't misunderstand.
> I already see gadget->udc->driver->disconnect(gadget) being called with
> udc_lock being held.
> 
>                mutex_lock(&udc_lock);
>                if (gadget->udc->driver)
>                        gadget->udc->driver->disconnect(gadget);
>                mutex_unlock(&udc_lock);
> 
> The below patch seems to have introduced it:
> 1016fc0c096c USB: gadget: Fix obscure lockdep violation for udc_mutex

Hmmm...  You're right about this.  A big problem with the USB gadget 
framework is that it does not clearly state which routines have to run 
in process context and which have to run in interrupt/atomic context.  
People therefore don't think about it and frequently get it wrong.

So now the problem is that the UDC or transceiver driver may detect 
(typically in an interrupt handler) that VBUS power has appeared or 
disappeared, and it wants to tell the core to adjust the D+/D- pullup 
signals appropriately.  The core notifies the UDC driver about this, and 
then in the case of a disconnection, it has to notify the gadget driver.  
But notifying the gadget driver requires process context for the 
udc_lock mutex, the ultimate reason being that disconnect notifications 
can race with gadget driver binding and unbinding.

If we could prevent those races in some other way then we wouldn't need 
to hold udc_lock in usb_gadget_disconnect().  This seems like a sensible 
thing to do in any case; the UDC core should never allow a connection to 
occur before a gadget driver is bound or after it is unbound.

The first approach that occurs to me is to add a boolean allow_connect 
flag to struct usb_udc, together with a global spinlock to synchronize 
access to it.  Then usb_gadget_disconnect() could check the flag before 
calling driver->disconnect(), gadget_bind_driver() could set the flag 
before calling usb_udc_connect_control(), and gadget_unbind_driver() 
could clear the flag before calling usb_gadget_disconnect().

(Another possible approach would be to change gadget->deactivated into a 
counter.  It would still need to be synchronized by a spinlock, 
however.)

This will simplify matters considerably.  udc_lock can remain a mutex 
and the deadlock problem should go away.

Do you want to try adding allow_connect as described here or would you 
prefer that I do it?

(And in any case, we should prevent the udc_start and udc_stop callbacks 
in the chipidea and max3420 drivers from trying to update the connection 
status.)

Alan Stern
  

Hi Alan,

Thanks for taking the time out to share more details !
+1 on your comment: " A big problem with the USB gadget
framework is that it does not clearly state which routines have to run
in process context and which have to run in interrupt/atomic context."


I started to work on allow_connect and other suggestions that you had made.
In one of the previous comments you had mentioned that the
connect_lock should be a spinlock and not a mutex.
Right now there are four conditions that seem to be deciding whether
pullup needs to be enabled or disabled through gadget->ops->pullup().
1. Gadget not deactivated through usb_gadget_deactivate()
2. Gadget has to be started through usb_gadget_udc_start().
soft_connect_store() can start/stop gadget.
3. usb_gadget has been connected through usb_gadget_connect(). This is
assuming we are getting rid of usb_udc_vbus_handler.
4. allow_connect is true

I have so far identified two constraints here:
a. gadget->ops->pullup() can sleep in some implementations.
For instance:
BUG: scheduling while atomic: init/1/0x00000002
..
[   26.990631][    T1] Call trace:
[   26.993759][    T1]  dump_backtrace+0x104/0x128
[   26.998281][    T1]  show_stack+0x20/0x30
[   27.002279][    T1]  dump_stack_lvl+0x6c/0x9c
[   27.006627][    T1]  __schedule_bug+0x84/0xb4
[   27.010973][    T1]  __schedule+0x6f0/0xaec
[   27.015147][    T1]  schedule+0xc8/0x134
[   27.019059][    T1]  schedule_timeout+0x98/0x134
[   27.023666][    T1]  msleep+0x34/0x4c
[   27.027317][    T1]  dwc3_core_soft_reset+0xf0/0x354
[   27.032273][    T1]  dwc3_gadget_pullup+0xec/0x1d8
[   27.037055][    T1]  usb_gadget_pullup_update_locked+0xa0/0x1e0
[   27.042967][    T1]  udc_bind_to_driver+0x1e4/0x30c
[   27.047835][    T1]  usb_gadget_probe_driver+0xd0/0x178
[   27.053051][    T1]  gadget_dev_desc_UDC_store+0xf0/0x13c
[   27.058442][    T1]  configfs_write_iter+0x100/0x178
[   27.063399][    T1]  vfs_write+0x278/0x3c4
[   27.067483][    T1]  ksys_write+0x80/0xf4

b. gadget->ops->udc_start can also sleep in some implementations.
For example:
[   28.024255][    T1] BUG: scheduling while atomic: init/1/0x00000002
....
[   28.324996][    T1] Call trace:
[   28.328126][    T1]  dump_backtrace+0x104/0x128
[   28.332647][    T1]  show_stack+0x20/0x30
[   28.336645][    T1]  dump_stack_lvl+0x6c/0x9c
[   28.340993][    T1]  __schedule_bug+0x84/0xb4
[   28.345340][    T1]  __schedule+0x6f0/0xaec
[   28.349513][    T1]  schedule+0xc8/0x134
[   28.353425][    T1]  schedule_timeout+0x4c/0x134
[   28.358033][    T1]  wait_for_common+0xac/0x13c
[   28.362554][    T1]  wait_for_completion_killable+0x20/0x3c
[   28.368118][    T1]  __kthread_create_on_node+0xe4/0x1ec
[   28.373422][    T1]  kthread_create_on_node+0x54/0x80
[   28.378464][    T1]  setup_irq_thread+0x50/0x108
[   28.383072][    T1]  __setup_irq+0x90/0x87c
[   28.387245][    T1]  request_threaded_irq+0x144/0x180
[   28.392287][    T1]  dwc3_gadget_start+0x50/0xac
[   28.396866][    T1]  udc_bind_to_driver+0x14c/0x31c
[   28.401763][    T1]  usb_gadget_probe_driver+0xd0/0x178
[   28.406980][    T1]  gadget_dev_desc_UDC_store+0xf0/0x13c
[   28.412370][    T1]  configfs_write_iter+0x100/0x178
[   28.417325][    T1]  vfs_write+0x278/0x3c4
[   28.421411][    T1]  ksys_write+0x80/0xf4

static int dwc3_gadget_start(struct usb_gadget *g,
                struct usb_gadget_driver *driver)
{
        struct dwc3             *dwc = gadget_to_dwc(g);
...
        irq = dwc->irq_gadget;
        ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
                        IRQF_SHARED, "dwc3", dwc->ev_buf);

Given that "1016fc0c096c USB: gadget: Fix obscure lockdep violation
for udc_mutex" has been there for a while and no one has reported
issues so far, perhaps ->disconnect() callback is no longer being
invoked in atomic context and the documentation is what that needs to
be updated ?

Thanks,
Badhri

On Fri, May 19, 2023 at 10:27 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Fri, May 19, 2023 at 08:44:57AM -0700, Badhri Jagan Sridharan wrote:
> > On Fri, May 19, 2023 at 8:07 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> > >
> > > On Fri, May 19, 2023 at 10:49:49AM -0400, Alan Stern wrote:
> > > > On Fri, May 19, 2023 at 04:30:41AM +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.
> > > >
> > > > Look, this is way overkill.
> > > >
> > > > usb_udc_vbus_handler() has only two jobs to do: set udc->vbus and call
> > > > usb_udc_connect_control().  Furthermore, it gets called from only two
> > > > drivers: chipidea and max3420.
> > > >
> > > > Why not have the callers set udc->vbus themselves and then call
> > > > usb_gadget_{dis}connect() directly?  Then we could eliminate
> > > > usb_udc_vbus_handler() entirely.  And the unnecessary calls -- the ones
> > > > causing deadlocks -- from within udc_start() and udc_stop() handlers can
> > > > be removed with no further consequence.
> > > >
> > > > This approach simplifies and removes code.  Whereas your approach
> > > > complicates and adds code for no good reason.
> > >
> > > I changed my mind.
> > >
> > > After looking more closely, I found the comment in gadget.h about
> > > ->disconnect() callbacks happening in interrupt context.  This means we
> > > cannot use a mutex to protect the associated state, and therefore the
> > > connect_lock _must_ be a spinlock, not a mutex.
> >
> > Quick observation so that I don't misunderstand.
> > I already see gadget->udc->driver->disconnect(gadget) being called with
> > udc_lock being held.
> >
> >                mutex_lock(&udc_lock);
> >                if (gadget->udc->driver)
> >                        gadget->udc->driver->disconnect(gadget);
> >                mutex_unlock(&udc_lock);
> >
> > The below patch seems to have introduced it:
> > 1016fc0c096c USB: gadget: Fix obscure lockdep violation for udc_mutex
>
> Hmmm...  You're right about this.  A big problem with the USB gadget
> framework is that it does not clearly state which routines have to run
> in process context and which have to run in interrupt/atomic context.
> People therefore don't think about it and frequently get it wrong.
>
> So now the problem is that the UDC or transceiver driver may detect
> (typically in an interrupt handler) that VBUS power has appeared or
> disappeared, and it wants to tell the core to adjust the D+/D- pullup
> signals appropriately.  The core notifies the UDC driver about this, and
> then in the case of a disconnection, it has to notify the gadget driver.
> But notifying the gadget driver requires process context for the
> udc_lock mutex, the ultimate reason being that disconnect notifications
> can race with gadget driver binding and unbinding.
>
> If we could prevent those races in some other way then we wouldn't need
> to hold udc_lock in usb_gadget_disconnect().  This seems like a sensible
> thing to do in any case; the UDC core should never allow a connection to
> occur before a gadget driver is bound or after it is unbound.
>
> The first approach that occurs to me is to add a boolean allow_connect
> flag to struct usb_udc, together with a global spinlock to synchronize
> access to it.  Then usb_gadget_disconnect() could check the flag before
> calling driver->disconnect(), gadget_bind_driver() could set the flag
> before calling usb_udc_connect_control(), and gadget_unbind_driver()
> could clear the flag before calling usb_gadget_disconnect().
>
> (Another possible approach would be to change gadget->deactivated into a
> counter.  It would still need to be synchronized by a spinlock,
> however.)
>
> This will simplify matters considerably.  udc_lock can remain a mutex
> and the deadlock problem should go away.
>
> Do you want to try adding allow_connect as described here or would you
> prefer that I do it?
>
> (And in any case, we should prevent the udc_start and udc_stop callbacks
> in the chipidea and max3420 drivers from trying to update the connection
> status.)
>
> Alan Stern
  

On Mon, May 22, 2023 at 12:48 AM Badhri Jagan Sridharan
<badhri@google.com> wrote:
>
> Hi Alan,
>
> Thanks for taking the time out to share more details !
> +1 on your comment: " A big problem with the USB gadget
> framework is that it does not clearly state which routines have to run
> in process context and which have to run in interrupt/atomic context."
>
>
> I started to work on allow_connect and other suggestions that you had made.
> In one of the previous comments you had mentioned that the
> connect_lock should be a spinlock and not a mutex.
> Right now there are four conditions that seem to be deciding whether
> pullup needs to be enabled or disabled through gadget->ops->pullup().
> 1. Gadget not deactivated through usb_gadget_deactivate()
> 2. Gadget has to be started through usb_gadget_udc_start().
> soft_connect_store() can start/stop gadget.
> 3. usb_gadget has been connected through usb_gadget_connect(). This is
> assuming we are getting rid of usb_udc_vbus_handler.
> 4. allow_connect is true
>
> I have so far identified two constraints here:
> a. gadget->ops->pullup() can sleep in some implementations.
> For instance:
> BUG: scheduling while atomic: init/1/0x00000002
> ..
> [   26.990631][    T1] Call trace:
> [   26.993759][    T1]  dump_backtrace+0x104/0x128
> [   26.998281][    T1]  show_stack+0x20/0x30
> [   27.002279][    T1]  dump_stack_lvl+0x6c/0x9c
> [   27.006627][    T1]  __schedule_bug+0x84/0xb4
> [   27.010973][    T1]  __schedule+0x6f0/0xaec
> [   27.015147][    T1]  schedule+0xc8/0x134
> [   27.019059][    T1]  schedule_timeout+0x98/0x134
> [   27.023666][    T1]  msleep+0x34/0x4c

Adding more context to make sure that I am more articulate.
I am aware that alternatives such as mdelay can be used to work around
in this specific instance. However, my concern is more around whether
gadget->ops->pullup() of other implementations were designed as
atomic. I only have dwc3 based hardware so can't test other udc
implementations. Hence the concern.

Thanks,
Badhri

> [   27.027317][    T1]  dwc3_core_soft_reset+0xf0/0x354
> [   27.032273][    T1]  dwc3_gadget_pullup+0xec/0x1d8
> [   27.037055][    T1]  usb_gadget_pullup_update_locked+0xa0/0x1e0
> [   27.042967][    T1]  udc_bind_to_driver+0x1e4/0x30c
> [   27.047835][    T1]  usb_gadget_probe_driver+0xd0/0x178
> [   27.053051][    T1]  gadget_dev_desc_UDC_store+0xf0/0x13c
> [   27.058442][    T1]  configfs_write_iter+0x100/0x178
> [   27.063399][    T1]  vfs_write+0x278/0x3c4
> [   27.067483][    T1]  ksys_write+0x80/0xf4
>
> b. gadget->ops->udc_start can also sleep in some implementations.
> For example:
> [   28.024255][    T1] BUG: scheduling while atomic: init/1/0x00000002
> ....
> [   28.324996][    T1] Call trace:
> [   28.328126][    T1]  dump_backtrace+0x104/0x128
> [   28.332647][    T1]  show_stack+0x20/0x30
> [   28.336645][    T1]  dump_stack_lvl+0x6c/0x9c
> [   28.340993][    T1]  __schedule_bug+0x84/0xb4
> [   28.345340][    T1]  __schedule+0x6f0/0xaec
> [   28.349513][    T1]  schedule+0xc8/0x134
> [   28.353425][    T1]  schedule_timeout+0x4c/0x134
> [   28.358033][    T1]  wait_for_common+0xac/0x13c
> [   28.362554][    T1]  wait_for_completion_killable+0x20/0x3c
> [   28.368118][    T1]  __kthread_create_on_node+0xe4/0x1ec
> [   28.373422][    T1]  kthread_create_on_node+0x54/0x80
> [   28.378464][    T1]  setup_irq_thread+0x50/0x108
> [   28.383072][    T1]  __setup_irq+0x90/0x87c
> [   28.387245][    T1]  request_threaded_irq+0x144/0x180
> [   28.392287][    T1]  dwc3_gadget_start+0x50/0xac
> [   28.396866][    T1]  udc_bind_to_driver+0x14c/0x31c
> [   28.401763][    T1]  usb_gadget_probe_driver+0xd0/0x178
> [   28.406980][    T1]  gadget_dev_desc_UDC_store+0xf0/0x13c
> [   28.412370][    T1]  configfs_write_iter+0x100/0x178
> [   28.417325][    T1]  vfs_write+0x278/0x3c4
> [   28.421411][    T1]  ksys_write+0x80/0xf4
>
> static int dwc3_gadget_start(struct usb_gadget *g,
>                 struct usb_gadget_driver *driver)
> {
>         struct dwc3             *dwc = gadget_to_dwc(g);
> ...
>         irq = dwc->irq_gadget;
>         ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
>                         IRQF_SHARED, "dwc3", dwc->ev_buf);
>
> Given that "1016fc0c096c USB: gadget: Fix obscure lockdep violation
> for udc_mutex" has been there for a while and no one has reported
> issues so far, perhaps ->disconnect() callback is no longer being
> invoked in atomic context and the documentation is what that needs to
> be updated ?
>
> Thanks,
> Badhri
>
> On Fri, May 19, 2023 at 10:27 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Fri, May 19, 2023 at 08:44:57AM -0700, Badhri Jagan Sridharan wrote:
> > > On Fri, May 19, 2023 at 8:07 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> > > >
> > > > On Fri, May 19, 2023 at 10:49:49AM -0400, Alan Stern wrote:
> > > > > On Fri, May 19, 2023 at 04:30:41AM +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.
> > > > >
> > > > > Look, this is way overkill.
> > > > >
> > > > > usb_udc_vbus_handler() has only two jobs to do: set udc->vbus and call
> > > > > usb_udc_connect_control().  Furthermore, it gets called from only two
> > > > > drivers: chipidea and max3420.
> > > > >
> > > > > Why not have the callers set udc->vbus themselves and then call
> > > > > usb_gadget_{dis}connect() directly?  Then we could eliminate
> > > > > usb_udc_vbus_handler() entirely.  And the unnecessary calls -- the ones
> > > > > causing deadlocks -- from within udc_start() and udc_stop() handlers can
> > > > > be removed with no further consequence.
> > > > >
> > > > > This approach simplifies and removes code.  Whereas your approach
> > > > > complicates and adds code for no good reason.
> > > >
> > > > I changed my mind.
> > > >
> > > > After looking more closely, I found the comment in gadget.h about
> > > > ->disconnect() callbacks happening in interrupt context.  This means we
> > > > cannot use a mutex to protect the associated state, and therefore the
> > > > connect_lock _must_ be a spinlock, not a mutex.
> > >
> > > Quick observation so that I don't misunderstand.
> > > I already see gadget->udc->driver->disconnect(gadget) being called with
> > > udc_lock being held.
> > >
> > >                mutex_lock(&udc_lock);
> > >                if (gadget->udc->driver)
> > >                        gadget->udc->driver->disconnect(gadget);
> > >                mutex_unlock(&udc_lock);
> > >
> > > The below patch seems to have introduced it:
> > > 1016fc0c096c USB: gadget: Fix obscure lockdep violation for udc_mutex
> >
> > Hmmm...  You're right about this.  A big problem with the USB gadget
> > framework is that it does not clearly state which routines have to run
> > in process context and which have to run in interrupt/atomic context.
> > People therefore don't think about it and frequently get it wrong.
> >
> > So now the problem is that the UDC or transceiver driver may detect
> > (typically in an interrupt handler) that VBUS power has appeared or
> > disappeared, and it wants to tell the core to adjust the D+/D- pullup
> > signals appropriately.  The core notifies the UDC driver about this, and
> > then in the case of a disconnection, it has to notify the gadget driver.
> > But notifying the gadget driver requires process context for the
> > udc_lock mutex, the ultimate reason being that disconnect notifications
> > can race with gadget driver binding and unbinding.
> >
> > If we could prevent those races in some other way then we wouldn't need
> > to hold udc_lock in usb_gadget_disconnect().  This seems like a sensible
> > thing to do in any case; the UDC core should never allow a connection to
> > occur before a gadget driver is bound or after it is unbound.
> >
> > The first approach that occurs to me is to add a boolean allow_connect
> > flag to struct usb_udc, together with a global spinlock to synchronize
> > access to it.  Then usb_gadget_disconnect() could check the flag before
> > calling driver->disconnect(), gadget_bind_driver() could set the flag
> > before calling usb_udc_connect_control(), and gadget_unbind_driver()
> > could clear the flag before calling usb_gadget_disconnect().
> >
> > (Another possible approach would be to change gadget->deactivated into a
> > counter.  It would still need to be synchronized by a spinlock,
> > however.)
> >
> > This will simplify matters considerably.  udc_lock can remain a mutex
> > and the deadlock problem should go away.
> >
> > Do you want to try adding allow_connect as described here or would you
> > prefer that I do it?
> >
> > (And in any case, we should prevent the udc_start and udc_stop callbacks
> > in the chipidea and max3420 drivers from trying to update the connection
> > status.)
> >
> > Alan Stern
  

On Mon, May 22, 2023 at 12:48:39AM -0700, Badhri Jagan Sridharan wrote:
> Hi Alan,
> 
> Thanks for taking the time out to share more details !
> +1 on your comment: " A big problem with the USB gadget
> framework is that it does not clearly state which routines have to run
> in process context and which have to run in interrupt/atomic context."
> 
> 
> I started to work on allow_connect and other suggestions that you had made.
> In one of the previous comments you had mentioned that the
> connect_lock should be a spinlock and not a mutex.

Yeah, I changed my mind about that.

> Right now there are four conditions that seem to be deciding whether
> pullup needs to be enabled or disabled through gadget->ops->pullup().
> 1. Gadget not deactivated through usb_gadget_deactivate()
> 2. Gadget has to be started through usb_gadget_udc_start().
> soft_connect_store() can start/stop gadget.
> 3. usb_gadget has been connected through usb_gadget_connect(). This is
> assuming we are getting rid of usb_udc_vbus_handler.
> 4. allow_connect is true
> 
> I have so far identified two constraints here:
> a. gadget->ops->pullup() can sleep in some implementations.
> For instance:
> BUG: scheduling while atomic: init/1/0x00000002
> ..
> [   26.990631][    T1] Call trace:
> [   26.993759][    T1]  dump_backtrace+0x104/0x128
> [   26.998281][    T1]  show_stack+0x20/0x30
> [   27.002279][    T1]  dump_stack_lvl+0x6c/0x9c
> [   27.006627][    T1]  __schedule_bug+0x84/0xb4
> [   27.010973][    T1]  __schedule+0x6f0/0xaec
> [   27.015147][    T1]  schedule+0xc8/0x134
> [   27.019059][    T1]  schedule_timeout+0x98/0x134
> [   27.023666][    T1]  msleep+0x34/0x4c
> [   27.027317][    T1]  dwc3_core_soft_reset+0xf0/0x354
> [   27.032273][    T1]  dwc3_gadget_pullup+0xec/0x1d8
> [   27.037055][    T1]  usb_gadget_pullup_update_locked+0xa0/0x1e0
> [   27.042967][    T1]  udc_bind_to_driver+0x1e4/0x30c
> [   27.047835][    T1]  usb_gadget_probe_driver+0xd0/0x178
> [   27.053051][    T1]  gadget_dev_desc_UDC_store+0xf0/0x13c
> [   27.058442][    T1]  configfs_write_iter+0x100/0x178
> [   27.063399][    T1]  vfs_write+0x278/0x3c4
> [   27.067483][    T1]  ksys_write+0x80/0xf4

What kernel was this trace made with?  I don't see udc_bind_to_driver 
appearing anywhere in 6.4-rc3.

> b. gadget->ops->udc_start can also sleep in some implementations.
> For example:
> [   28.024255][    T1] BUG: scheduling while atomic: init/1/0x00000002
> ....
> [   28.324996][    T1] Call trace:
> [   28.328126][    T1]  dump_backtrace+0x104/0x128
> [   28.332647][    T1]  show_stack+0x20/0x30
> [   28.336645][    T1]  dump_stack_lvl+0x6c/0x9c
> [   28.340993][    T1]  __schedule_bug+0x84/0xb4
> [   28.345340][    T1]  __schedule+0x6f0/0xaec
> [   28.349513][    T1]  schedule+0xc8/0x134
> [   28.353425][    T1]  schedule_timeout+0x4c/0x134
> [   28.358033][    T1]  wait_for_common+0xac/0x13c
> [   28.362554][    T1]  wait_for_completion_killable+0x20/0x3c
> [   28.368118][    T1]  __kthread_create_on_node+0xe4/0x1ec
> [   28.373422][    T1]  kthread_create_on_node+0x54/0x80
> [   28.378464][    T1]  setup_irq_thread+0x50/0x108
> [   28.383072][    T1]  __setup_irq+0x90/0x87c
> [   28.387245][    T1]  request_threaded_irq+0x144/0x180
> [   28.392287][    T1]  dwc3_gadget_start+0x50/0xac
> [   28.396866][    T1]  udc_bind_to_driver+0x14c/0x31c
> [   28.401763][    T1]  usb_gadget_probe_driver+0xd0/0x178
> [   28.406980][    T1]  gadget_dev_desc_UDC_store+0xf0/0x13c
> [   28.412370][    T1]  configfs_write_iter+0x100/0x178
> [   28.417325][    T1]  vfs_write+0x278/0x3c4
> [   28.421411][    T1]  ksys_write+0x80/0xf4
> 
> static int dwc3_gadget_start(struct usb_gadget *g,
>                 struct usb_gadget_driver *driver)
> {
>         struct dwc3             *dwc = gadget_to_dwc(g);
> ...
>         irq = dwc->irq_gadget;
>         ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
>                         IRQF_SHARED, "dwc3", dwc->ev_buf);
> 
> Given that "1016fc0c096c USB: gadget: Fix obscure lockdep violation
> for udc_mutex" has been there for a while and no one has reported
> issues so far, perhaps ->disconnect() callback is no longer being
> invoked in atomic context and the documentation is what that needs to
> be updated ?

That's part of what I'm trying to figure out.  However, some UDC drivers 
call ->disconnect() directly when they detect loss of VBUS power, 
instead of going through the core.  So disconnect handlers will have 
remain capable of running in interrupt context until those UDC drivers 
are changed.

Getting back to your first point, it looks like we need to assume any 
routine that needs to communicate with the UDC hardware (such as the 
->pullup callback used in usb_gadget_{dis}connect()) must always be 
called in process context.  This means that usb_udc_connect_control() 
always has to run in process context, since it will do either a connect 
or a disconnect.

On the other hand, some routines -- in particular, 
usb_udc_vbus_handler() -- may be called by a UDC driver's interrupt 
handler and therefore may run in interrupt context.  (This fact should 
be noted in that routine's kerneldoc, by the way.)

So here's the problem: usb_udc_vbus_handler() running in interrupt 
context calls usb_udc_connect_control(), which has to run in process 
context.  And this is not just a simple issue caused by the 
->disconnect() callback or use of mutexes; it's more fundamental.

I'm led to conclude that you were right to offload part of 
usb_udc_vbus_handler()'s job to a workqueue.  It's an awkward thing to 
do, because you have to make sure to cancel the work item at times when 
it's no longer needed.  But there doesn't seem to be any other choice.

Here's two related problems for you to think about:

    1.	Once gadget_unbind_driver() has called usb_gadget_disconnect(),
	we don't want a VBUS change to cause usb_udc_vbus_handler()'s 
	work routine to turn the pullup back on.  How can we prevent 
	this?

    2.	More generally, suppose usb_udc_vbus_handler() gets called at 
	exactly the same time that some other pathway (either 
	gadget_bind_driver() or soft_connect_store()) tries to do a
	connect or disconnect.  What should happen then?

Alan Stern
  

Thanks again Alan !

On Mon, May 22, 2023 at 8:55 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Mon, May 22, 2023 at 12:48:39AM -0700, Badhri Jagan Sridharan wrote:
> > Hi Alan,
> >
> > Thanks for taking the time out to share more details !
> > +1 on your comment: " A big problem with the USB gadget
> > framework is that it does not clearly state which routines have to run
> > in process context and which have to run in interrupt/atomic context."
> >
> >
> > I started to work on allow_connect and other suggestions that you had made.
> > In one of the previous comments you had mentioned that the
> > connect_lock should be a spinlock and not a mutex.
>
> Yeah, I changed my mind about that.
>
> > Right now there are four conditions that seem to be deciding whether
> > pullup needs to be enabled or disabled through gadget->ops->pullup().
> > 1. Gadget not deactivated through usb_gadget_deactivate()
> > 2. Gadget has to be started through usb_gadget_udc_start().
> > soft_connect_store() can start/stop gadget.
> > 3. usb_gadget has been connected through usb_gadget_connect(). This is
> > assuming we are getting rid of usb_udc_vbus_handler.
> > 4. allow_connect is true
> >
> > I have so far identified two constraints here:
> > a. gadget->ops->pullup() can sleep in some implementations.
> > For instance:
> > BUG: scheduling while atomic: init/1/0x00000002
> > ..
> > [   26.990631][    T1] Call trace:
> > [   26.993759][    T1]  dump_backtrace+0x104/0x128
> > [   26.998281][    T1]  show_stack+0x20/0x30
> > [   27.002279][    T1]  dump_stack_lvl+0x6c/0x9c
> > [   27.006627][    T1]  __schedule_bug+0x84/0xb4
> > [   27.010973][    T1]  __schedule+0x6f0/0xaec
> > [   27.015147][    T1]  schedule+0xc8/0x134
> > [   27.019059][    T1]  schedule_timeout+0x98/0x134
> > [   27.023666][    T1]  msleep+0x34/0x4c
> > [   27.027317][    T1]  dwc3_core_soft_reset+0xf0/0x354
> > [   27.032273][    T1]  dwc3_gadget_pullup+0xec/0x1d8
> > [   27.037055][    T1]  usb_gadget_pullup_update_locked+0xa0/0x1e0
> > [   27.042967][    T1]  udc_bind_to_driver+0x1e4/0x30c
> > [   27.047835][    T1]  usb_gadget_probe_driver+0xd0/0x178
> > [   27.053051][    T1]  gadget_dev_desc_UDC_store+0xf0/0x13c
> > [   27.058442][    T1]  configfs_write_iter+0x100/0x178
> > [   27.063399][    T1]  vfs_write+0x278/0x3c4
> > [   27.067483][    T1]  ksys_write+0x80/0xf4
>
> What kernel was this trace made with?  I don't see udc_bind_to_driver
> appearing anywhere in 6.4-rc3.


Sorry, I was switching between devices running different kernel
versions, with the latest one running 6.1, and posted trace from an
older one by mistake.
>
>
> > b. gadget->ops->udc_start can also sleep in some implementations.
> > For example:
> > [   28.024255][    T1] BUG: scheduling while atomic: init/1/0x00000002
> > ....
> > [   28.324996][    T1] Call trace:
> > [   28.328126][    T1]  dump_backtrace+0x104/0x128
> > [   28.332647][    T1]  show_stack+0x20/0x30
> > [   28.336645][    T1]  dump_stack_lvl+0x6c/0x9c
> > [   28.340993][    T1]  __schedule_bug+0x84/0xb4
> > [   28.345340][    T1]  __schedule+0x6f0/0xaec
> > [   28.349513][    T1]  schedule+0xc8/0x134
> > [   28.353425][    T1]  schedule_timeout+0x4c/0x134
> > [   28.358033][    T1]  wait_for_common+0xac/0x13c
> > [   28.362554][    T1]  wait_for_completion_killable+0x20/0x3c
> > [   28.368118][    T1]  __kthread_create_on_node+0xe4/0x1ec
> > [   28.373422][    T1]  kthread_create_on_node+0x54/0x80
> > [   28.378464][    T1]  setup_irq_thread+0x50/0x108
> > [   28.383072][    T1]  __setup_irq+0x90/0x87c
> > [   28.387245][    T1]  request_threaded_irq+0x144/0x180
> > [   28.392287][    T1]  dwc3_gadget_start+0x50/0xac
> > [   28.396866][    T1]  udc_bind_to_driver+0x14c/0x31c
> > [   28.401763][    T1]  usb_gadget_probe_driver+0xd0/0x178
> > [   28.406980][    T1]  gadget_dev_desc_UDC_store+0xf0/0x13c
> > [   28.412370][    T1]  configfs_write_iter+0x100/0x178
> > [   28.417325][    T1]  vfs_write+0x278/0x3c4
> > [   28.421411][    T1]  ksys_write+0x80/0xf4
> >
> > static int dwc3_gadget_start(struct usb_gadget *g,
> >                 struct usb_gadget_driver *driver)
> > {
> >         struct dwc3             *dwc = gadget_to_dwc(g);
> > ...
> >         irq = dwc->irq_gadget;
> >         ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
> >                         IRQF_SHARED, "dwc3", dwc->ev_buf);
> >
> > Given that "1016fc0c096c USB: gadget: Fix obscure lockdep violation
> > for udc_mutex" has been there for a while and no one has reported
> > issues so far, perhaps ->disconnect() callback is no longer being
> > invoked in atomic context and the documentation is what that needs to
> > be updated ?
>
> That's part of what I'm trying to figure out.  However, some UDC drivers
> call ->disconnect() directly when they detect loss of VBUS power,
> instead of going through the core.  So disconnect handlers will have
> remain capable of running in interrupt context until those UDC drivers
> are changed.
>
> Getting back to your first point, it looks like we need to assume any
> routine that needs to communicate with the UDC hardware (such as the
> ->pullup callback used in usb_gadget_{dis}connect()) must always be
> called in process context.  This means that usb_udc_connect_control()
> always has to run in process context, since it will do either a connect
> or a disconnect.
>
> On the other hand, some routines -- in particular,
> usb_udc_vbus_handler() -- may be called by a UDC driver's interrupt
> handler and therefore may run in interrupt context.  (This fact should
> be noted in that routine's kerneldoc, by the way.)
>
> So here's the problem: usb_udc_vbus_handler() running in interrupt
> context calls usb_udc_connect_control(), which has to run in process
> context.  And this is not just a simple issue caused by the
> ->disconnect() callback or use of mutexes; it's more fundamental.
>
> I'm led to conclude that you were right to offload part of
> usb_udc_vbus_handler()'s job to a workqueue.  It's an awkward thing to
> do, because you have to make sure to cancel the work item at times when
> it's no longer needed.  But there doesn't seem to be any other choice.
>
> Here's two related problems for you to think about:
>
>     1.  Once gadget_unbind_driver() has called usb_gadget_disconnect(),
>         we don't want a VBUS change to cause usb_udc_vbus_handler()'s
>         work routine to turn the pullup back on.  How can we prevent
>         this?
>
>     2.  More generally, suppose usb_udc_vbus_handler() gets called at
>         exactly the same time that some other pathway (either
>         gadget_bind_driver() or soft_connect_store()) tries to do a
>         connect or disconnect.  What should happen then?


I believe I can solve the above races by protecting the flags set by
each of them with connect_lock and not pulling up unless all of them
are true.
The caller will hold connect_lock, update the respective flag and
invoke the below usb_gadget_pullup_update_locked function(shown
below).

Code stub:
/* Internal version of usb_gadget_connect needs to be called with
connect_lock held. */
static int usb_gadget_pullup_update_locked(struct usb_gadget *gadget)
        __must_hold(&gadget->udc->connect_lock)
{
        int ret = 0;
        bool connect = !gadget->deactivated && gadget->udc->started &&
gadget->udc->vbus &&
                             gadget->udc->allow_connect;

        if (!gadget->ops->pullup) {
                ret = -EOPNOTSUPP;
                goto out;
        }

        if (connect != gadget->connected) {
                ret = gadget->ops->pullup(gadget, connect);
                if (!ret)
                        gadget->connected = connect;
                if (!connect) {
                        mutex_lock(&udc_lock);
                        if (gadget->udc->driver)
                                gadget->udc->driver->disconnect(gadget);
                        mutex_unlock(&udc_lock);
        }

out:
        trace_usb_gadget_connect(gadget, ret);

        return ret;
}

However, while auditing the code again, I noticed another potential
race as well:
Looks like usb_del_gadget() can potentially race against
usb_udc_vbus_handler() and call device_unregister.
This implies usb_udc can be freed while usb_udc_vbus_handler() or the
work item is executing.

void usb_del_gadget(struct usb_gadget *gadget)
{
        struct usb_udc *udc = gadget->udc;

..
...
        device_unregister(&udc->dev);
}
EXPORT_SYMBOL_GPL(usb_del_gadget);

Does this look like a valid concern to you or am I misunderstanding this ?
If so, I am afraid that the only way to solve this is by synchronizing
usb_udc_vbus_handler() against usb_del_gadget() through a mutex as
device_unregister() can sleep.
So offloading usb_udc_vbus_handler() cannot work either.

usb_udc_vbus_hander() seems to be invoked from the following drivers:

1. drivers/usb/chipidea/udc.c:
usb_udc_vbus_hander()  is called from a non-atomic context.

2. drivers/usb/gadget/udc/max3420_udc.c
usb_udc_vbus_hander()  is called from the interrupt handler.
However, all the events are processed from max3420_thread kthread.
So I am thinking of making them threaded irq handlers instead.

3. drivers/usb/gadget/udc/renesas_usbf.c
This one looks more invasive. However, still attempting to move them
to threaded irq handlers.

As always, I'm looking forward to your feedback !

Thanks,
Badhri

>
> Alan Stern
  

On Fri, May 26, 2023 at 07:42:39PM -0700, Badhri Jagan Sridharan wrote:
> Thanks again Alan !
> 
> On Mon, May 22, 2023 at 8:55 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> > Getting back to your first point, it looks like we need to assume any
> > routine that needs to communicate with the UDC hardware (such as the
> > ->pullup callback used in usb_gadget_{dis}connect()) must always be
> > called in process context.  This means that usb_udc_connect_control()
> > always has to run in process context, since it will do either a connect
> > or a disconnect.
> >
> > On the other hand, some routines -- in particular,
> > usb_udc_vbus_handler() -- may be called by a UDC driver's interrupt
> > handler and therefore may run in interrupt context.  (This fact should
> > be noted in that routine's kerneldoc, by the way.)
> >
> > So here's the problem: usb_udc_vbus_handler() running in interrupt
> > context calls usb_udc_connect_control(), which has to run in process
> > context.  And this is not just a simple issue caused by the
> > ->disconnect() callback or use of mutexes; it's more fundamental.
> >
> > I'm led to conclude that you were right to offload part of
> > usb_udc_vbus_handler()'s job to a workqueue.  It's an awkward thing to
> > do, because you have to make sure to cancel the work item at times when
> > it's no longer needed.  But there doesn't seem to be any other choice.
> >
> > Here's two related problems for you to think about:
> >
> >     1.  Once gadget_unbind_driver() has called usb_gadget_disconnect(),
> >         we don't want a VBUS change to cause usb_udc_vbus_handler()'s
> >         work routine to turn the pullup back on.  How can we prevent
> >         this?
> >
> >     2.  More generally, suppose usb_udc_vbus_handler() gets called at
> >         exactly the same time that some other pathway (either
> >         gadget_bind_driver() or soft_connect_store()) tries to do a
> >         connect or disconnect.  What should happen then?
> 
> 
> I believe I can solve the above races by protecting the flags set by
> each of them with connect_lock and not pulling up unless all of them
> are true.
> 
> The caller will hold connect_lock, update the respective flag and
> invoke the below usb_gadget_pullup_update_locked function(shown
> below).

Are you certain this can be done without causing any deadlocks?

> Code stub:
> /* Internal version of usb_gadget_connect needs to be called with
> connect_lock held. */
> static int usb_gadget_pullup_update_locked(struct usb_gadget *gadget)
>         __must_hold(&gadget->udc->connect_lock)
> {
>         int ret = 0;
>         bool connect = !gadget->deactivated && gadget->udc->started &&
> gadget->udc->vbus &&
>                              gadget->udc->allow_connect;

On further thought, I decided "allow_connect" is a dumb name.  Let's 
call it "unbinding" instead, since it gets set only when a gadget driver 
is about to be unbound (which is when we want to prevent new 
connections).

>         if (!gadget->ops->pullup) {
>                 ret = -EOPNOTSUPP;
>                 goto out;
>         }
> 
>         if (connect != gadget->connected) {

You need to be more careful here.  It's possible to have 
gadget->connected set at the same time as gadget->deactivated -- it 
means that when the gadget gets re-activated, it will immediately try to 
connect again.

In fact, this logic doesn't look right at all.  For example, suppose the 
gadget driver wants to disconnect.  This routine will compute connect = 
true and will see that gadget->connected is set, so it won't do 
anything!

I think it would be better just to merge the new material into 
usb_gadget_connect() and usb_gadget_disconnect().

>                 ret = gadget->ops->pullup(gadget, connect);
>                 if (!ret)
>                         gadget->connected = connect;
>                 if (!connect) {
>                         mutex_lock(&udc_lock);
>                         if (gadget->udc->driver)
>                                 gadget->udc->driver->disconnect(gadget);
>                         mutex_unlock(&udc_lock);
>         }
> 
> out:
>         trace_usb_gadget_connect(gadget, ret);
> 
>         return ret;
> }
> 
> However, while auditing the code again, I noticed another potential
> race as well:
> Looks like usb_del_gadget() can potentially race against
> usb_udc_vbus_handler() and call device_unregister.
> This implies usb_udc can be freed while usb_udc_vbus_handler() or the
> work item is executing.
> 
> void usb_del_gadget(struct usb_gadget *gadget)
> {
>         struct usb_udc *udc = gadget->udc;
> 
> ..
> ...
>         device_unregister(&udc->dev);
> }
> EXPORT_SYMBOL_GPL(usb_del_gadget);
> 
> Does this look like a valid concern to you or am I misunderstanding this ?

You're missing an important point.  Before doing device_unregister(), 
this routine calls device_del(&gadget->dev).  That will unbind the 
gadget driver, which (among other things) will stop the UDC, preventing 
it from calling usb_udc_vbus_handler().  However, you're right that the 
work item will need to be cancelled at some point before the usb_udc is 
unregistered.

> If so, I am afraid that the only way to solve this is by synchronizing
> usb_udc_vbus_handler() against usb_del_gadget() through a mutex as
> device_unregister() can sleep.
> So offloading usb_udc_vbus_handler() cannot work either.
> 
> usb_udc_vbus_hander() seems to be invoked from the following drivers:
> 
> 1. drivers/usb/chipidea/udc.c:
> usb_udc_vbus_hander()  is called from a non-atomic context.
> 
> 2. drivers/usb/gadget/udc/max3420_udc.c
> usb_udc_vbus_hander()  is called from the interrupt handler.
> However, all the events are processed from max3420_thread kthread.
> So I am thinking of making them threaded irq handlers instead.
> 
> 3. drivers/usb/gadget/udc/renesas_usbf.c
> This one looks more invasive. However, still attempting to move them
> to threaded irq handlers.
> 
> As always, I'm looking forward to your feedback !

Moving those things to threaded IRQ handlers is a separate job.  Let's 
get this issue fixed first.

Alan Stern
  

On Sat, May 27, 2023 at 9:36 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Fri, May 26, 2023 at 07:42:39PM -0700, Badhri Jagan Sridharan wrote:
> > Thanks again Alan !
> >
> > On Mon, May 22, 2023 at 8:55 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> > > Getting back to your first point, it looks like we need to assume any
> > > routine that needs to communicate with the UDC hardware (such as the
> > > ->pullup callback used in usb_gadget_{dis}connect()) must always be
> > > called in process context.  This means that usb_udc_connect_control()
> > > always has to run in process context, since it will do either a connect
> > > or a disconnect.
> > >
> > > On the other hand, some routines -- in particular,
> > > usb_udc_vbus_handler() -- may be called by a UDC driver's interrupt
> > > handler and therefore may run in interrupt context.  (This fact should
> > > be noted in that routine's kerneldoc, by the way.)
> > >
> > > So here's the problem: usb_udc_vbus_handler() running in interrupt
> > > context calls usb_udc_connect_control(), which has to run in process
> > > context.  And this is not just a simple issue caused by the
> > > ->disconnect() callback or use of mutexes; it's more fundamental.
> > >
> > > I'm led to conclude that you were right to offload part of
> > > usb_udc_vbus_handler()'s job to a workqueue.  It's an awkward thing to
> > > do, because you have to make sure to cancel the work item at times when
> > > it's no longer needed.  But there doesn't seem to be any other choice.
> > >
> > > Here's two related problems for you to think about:
> > >
> > >     1.  Once gadget_unbind_driver() has called usb_gadget_disconnect(),
> > >         we don't want a VBUS change to cause usb_udc_vbus_handler()'s
> > >         work routine to turn the pullup back on.  How can we prevent
> > >         this?
> > >
> > >     2.  More generally, suppose usb_udc_vbus_handler() gets called at
> > >         exactly the same time that some other pathway (either
> > >         gadget_bind_driver() or soft_connect_store()) tries to do a
> > >         connect or disconnect.  What should happen then?
> >
> >
> > I believe I can solve the above races by protecting the flags set by
> > each of them with connect_lock and not pulling up unless all of them
> > are true.
> >
> > The caller will hold connect_lock, update the respective flag and
> > invoke the below usb_gadget_pullup_update_locked function(shown
> > below).
>
> Are you certain this can be done without causing any deadlocks?
>
> > Code stub:
> > /* Internal version of usb_gadget_connect needs to be called with
> > connect_lock held. */
> > static int usb_gadget_pullup_update_locked(struct usb_gadget *gadget)
> >         __must_hold(&gadget->udc->connect_lock)
> > {
> >         int ret = 0;
> >         bool connect = !gadget->deactivated && gadget->udc->started &&
> > gadget->udc->vbus &&
> >                              gadget->udc->allow_connect;
>
> On further thought, I decided "allow_connect" is a dumb name.  Let's
> call it "unbinding" instead, since it gets set only when a gadget driver
> is about to be unbound (which is when we want to prevent new
> connections).

Sure, fixing it in v3.

>
> >         if (!gadget->ops->pullup) {
> >                 ret = -EOPNOTSUPP;
> >                 goto out;
> >         }
> >
> >         if (connect != gadget->connected) {
>
> You need to be more careful here.  It's possible to have
> gadget->connected set at the same time as gadget->deactivated -- it
> means that when the gadget gets re-activated, it will immediately try to
> connect again.
>
> In fact, this logic doesn't look right at all.  For example, suppose the
> gadget driver wants to disconnect.  This routine will compute connect =
> true and will see that gadget->connected is set, so it won't do
> anything!
>
> I think it would be better just to merge the new material into
> usb_gadget_connect() and usb_gadget_disconnect().

I ended up merging them into usb_gadget_pullup_update_locked() so that
each of the individual helper function can call
usb_gadget_pullup_update_locked() while holding the connect_lock. I
actually had usb_gadget_(dis)connect() set udc->vbus. It appears to me
that both usb_gadget_(dis)connect() and usb_udc_vbus_handler() are
meant to be called based on vbus presence and hence seem to be
redundant. Wondering if we could get rid of usb_gadget_(dis)connect()
given that drivers/power/supply/isp1704_charger.c is only call it and
instead make it call usb_udc_vbus_handler() instead ?

>
> >                 ret = gadget->ops->pullup(gadget, connect);
> >                 if (!ret)
> >                         gadget->connected = connect;
> >                 if (!connect) {
> >                         mutex_lock(&udc_lock);
> >                         if (gadget->udc->driver)
> >                                 gadget->udc->driver->disconnect(gadget);
> >                         mutex_unlock(&udc_lock);
> >         }
> >
> > out:
> >         trace_usb_gadget_connect(gadget, ret);
> >
> >         return ret;
> > }
> >
> > However, while auditing the code again, I noticed another potential
> > race as well:
> > Looks like usb_del_gadget() can potentially race against
> > usb_udc_vbus_handler() and call device_unregister.
> > This implies usb_udc can be freed while usb_udc_vbus_handler() or the
> > work item is executing.
> >
> > void usb_del_gadget(struct usb_gadget *gadget)
> > {
> >         struct usb_udc *udc = gadget->udc;
> >
> > ..
> > ...
> >         device_unregister(&udc->dev);
> > }
> > EXPORT_SYMBOL_GPL(usb_del_gadget);
> >
> > Does this look like a valid concern to you or am I misunderstanding this ?
>
> You're missing an important point.  Before doing device_unregister(),
> this routine calls device_del(&gadget->dev).  That will unbind the
> gadget driver, which (among other things) will stop the UDC, preventing
> it from calling usb_udc_vbus_handler().  However, you're right that the
> work item will need to be cancelled at some point before the usb_udc is
> unregistered.
>

Sure,  thought gadget_unbind_driver() might be a good place to cancel
the work item. So, cancelling it there in V3.

> > If so, I am afraid that the only way to solve this is by synchronizing
> > usb_udc_vbus_handler() against usb_del_gadget() through a mutex as
> > device_unregister() can sleep.
> > So offloading usb_udc_vbus_handler() cannot work either.
> >
> > usb_udc_vbus_hander() seems to be invoked from the following drivers:
> >
> > 1. drivers/usb/chipidea/udc.c:
> > usb_udc_vbus_hander()  is called from a non-atomic context.
> >
> > 2. drivers/usb/gadget/udc/max3420_udc.c
> > usb_udc_vbus_hander()  is called from the interrupt handler.
> > However, all the events are processed from max3420_thread kthread.
> > So I am thinking of making them threaded irq handlers instead.
> >
> > 3. drivers/usb/gadget/udc/renesas_usbf.c
> > This one looks more invasive. However, still attempting to move them
> > to threaded irq handlers.
> >
> > As always, I'm looking forward to your feedback !
>
> Moving those things to threaded IRQ handlers is a separate job.  Let's
> get this issue fixed first.

Sounds good !

Thanks,
Badhri

>
> Alan Stern
  

On Mon, May 29, 2023 at 04:32:29PM -0700, Badhri Jagan Sridharan wrote:
> On Sat, May 27, 2023 at 9:36 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > I think it would be better just to merge the new material into
> > usb_gadget_connect() and usb_gadget_disconnect().
> 
> I ended up merging them into usb_gadget_pullup_update_locked() so that
> each of the individual helper function can call
> usb_gadget_pullup_update_locked() while holding the connect_lock. I
> actually had usb_gadget_(dis)connect() set udc->vbus.

What?  No, that's not right.  They are two completely separate concepts.  
The host controls VBUS and the gadget controls the pullup.

>  It appears to me
> that both usb_gadget_(dis)connect() and usb_udc_vbus_handler() are
> meant to be called based on vbus presence and hence seem to be
> redundant.

They are not.  We need to support turning off the pullup while VBUS is 
on.

>  Wondering if we could get rid of usb_gadget_(dis)connect()
> given that drivers/power/supply/isp1704_charger.c is only call it and
> instead make it call usb_udc_vbus_handler() instead ?

In short, no.

Alan Stern