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
