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

[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@xxxxxxxxxxxxxxxxxxx> 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