Hi, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes: > On Fri, May 14, 2021 at 10:35:59AM +0300, Felipe Balbi wrote: >> Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes: >> > On Wed, May 12, 2021 at 10:00:41AM +0300, Felipe Balbi wrote: >> >> Hmm, IIRC only the storage gadget defers work to another thread. >> > >> > Well, the composite core is built into almost every gadget, and doesn't >> > it handle Set-Configuration and Set-Interface requests in a separate >> > thread? Or doesn't it expect function drivers to do so? >> >> composite.c doesn't defer anything to a separate thread by itself. The >> gadget driver, if it finds it necessary, should handle it >> internally. Most gadget drivers handle all of this in interrupt context. >> >> > After all, the gadget first learns about config or altsetting changes >> > when it receives a Set-Configuration or Set-Interface request, which >> > happens in interrupt context. But changing configs or altsettings >> > requires disabling/enabling endpoints, which needs a process context >> > (see the kerneldoc for usb_ep_enable and usb_ep_disable). >> >> Ouch, I don't think any driver is guaranteeing that other than the >> storage gadget. > > A quick search shows that USB_GADGET_DELAYED_STATUS is used in f_fs.c, > f_tcm.c, and f_uvc.c in addition to f_mass_storage.c. So the situation > isn't as bad as you thought, although it should be better. right, that's not what the documentation means, though. We're still enabling/disabling endpoints in interrupt context, just delaying the status stage until a later time. It looks like delaying status like this is enough for the current UDC drivers so, perhaps, we should make delayed_status mandatory and update the documentation accordingly. > Anyway, getting back to the main point... > > To minimize disruption, suppose we add a new callback to usb_gadget_ops: > > void (*udc_async_callbacks)(struct usb_gadget *, int enable); > > The UDC core can call this at the appropriate times, if it is not NULL. > It allows the core to tell a UDC driver whether or not to issue > callbacks for setup, disconnect, reset, suspend, and resume. It doesn't > affect request completion callbacks. > > So for removing a driver, the proper sequence will be: > > usb_gadget_disconnect() > if (gadget->ops->udc_async_callbacks) > gadget->ops->udc_async_callbacks(gadget, 0); > synchronize_irq() > udc->driver->unbind() > usb_gadget_udc_stop() > > Or maybe the last two steps should be reversed. In udc_bind_to_driver, > the opposite sequence is: > > bind > udc_start > enable async callbacks > connect (turn on pull-up) > > How does this sound? Sounds reasonable and, probably, minimizes the amount of code that needs to be changed. This will also enable us to fix each UDC in isolation. -- balbi
Attachment:
signature.asc
Description: PGP signature
