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. 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? Alan Stern
