Re: Disconnect race in Gadget core

[Felipe Balbi <balbi@kernel.org>]

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


Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
> On Wed, May 12, 2021 at 10:00:41AM +0300, Felipe Balbi wrote:
>> 
>> > So in dwc3, for example: At what point do you abort all outstanding 
>> > requests with -ESHUTDOWN status?  We don't want to do this before 
>> 
>> we do this as part of dwc3_remove_requests(). So, it's done either when
>> the relevant endpoint is disabled or as part of
>> dwc3_stop_active_transfers() which in turn is called from a (bus) reset
>> interrupt or when disconnecting pullups.
>
> I wish these sorts of questions had been answered in the original design 
> of the gadget subsystem.  For example, does it even make sense for the 
> UDC driver to disable endpoints on its own initiative?  Oh well, too 
> late now...

heh, right. IMHO the UDC shouldn't do anything unless it's explicitly
requested. I would go as far as not even automatically starting SETUP
stage of ctrl transfer, but that's me.

>> >> I'm not against adding new udc methods to gadget ops, but it seems
>> >> fitting that udc_start/udc_stop would fit your description while some
>> >> new members could be given the task of priming the default control pipe
>> >> to receive the first SETUP packet.
>> >> 
>> >> What do you think?
>> >
>> > Starting things up when a new gadget driver binds doesn't seem to be so 
>> > much of a problem.  After all, the new driver isn't going to do anything 
>> > before the first SETUP packet arrives, since the gadget will be 
>> 
>> it could be an impact in power consumption, albeit minimal
>
> All right.  But at least it isn't an issue of correctness.

nope, it's not.

>> > unconfigured.  Unbinding and shutting down are the hard parts.
>> >
>> > I guess the ideal approach would be:
>> >
>> > 	First, the UDC driver basically turns off the UDC hardware.
>> > 	This means no more IRQs will be generated.  But pending requests
>> > 	remain pending until they are explicitly cancelled.
>> 
>> right, that, I argue, is the responsibility of ->udc_stop()
>> 
>> > 	Second, the gadget driver's unbind callback runs.  It should
>> > 	cancel any outstanding requests and generally release resources.
>> 
>> correct. But that means we would require the gadget driver to initiate
>> cancelling of outstanding requests
>
> Or even better, disabling all endpoints.  That's a reasonable 
> requirement.  Function drivers are expected to do that anyway whenever 
> the composite core switches to a different configuration, aren't they?
>
> In some ways, unbinding is similar to switching to configuration 0 (not 
> configured).

I agree. We have too many special cases in the gadget framework anyway.

>> > 	Third, the UDC driver WARNs about any requests that still exist
>> > 	and automatically releases them without doing any completion
>> > 	callbacks.  It also forgets about the gadget driver (this can't
>> > 	happen until after the gadget driver has cancelled its 
>> > 	requests).
>> >
>> > Right now we are doing the first two steps in the opposite order.  That 
>> > would be okay, provided we could guarantee there are no more 
>> > asynchronous callbacks once unbind is called (sort of like what Peter 
>> > has done for configfs).  But it would be better to do the steps in the 
>> > order shown above.  This does correspond to calling udc_stop first, as 
>> > you suggest.
>> 
>> right
>> 
>> > But it also would mean splitting out the third step as something 
>> > separate from udc_stop.  Not to mention some potentially serious 
>> > updating of some UDC drivers.
>> 
>> yeah, it would take quite a bit of effort.
>> 
>> > On the other hand, there is something to be said for leaving the UDC 
>> > operational until after the unbind callback.  If the gadget driver 
>> > happened to be installing a new alternate setting at that time, say in a 
>> > workqueue thread, calls to activate new endpoints wouldn't suddenly get 
>> > unexpected errors.
>> 
>> 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.

>> What
>> you describe can also happen today depending on how far into the future
>> the kthread is scheduled, no? So, how does storage gadget behave with
>> that today?
>
> I'm not clear on the details any more, but in essence the unbind routine 
> takes great care to wait until any queued worker threads have completed 
> or been successfully cancelled before it returns.

okay :-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]