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

[Alan Stern <stern@rowland.harvard.edu>]

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