Re: Disconnect race in Gadget core

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2890 bytes --]


Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
> On Fri, May 14, 2021 at 10:35:59AM +0300, Felipe Balbi wrote:
>> Alan Stern <stern@rowland.harvard.edu> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]