Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1560001 > unrolled thread

Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase Felipe Balbi <balbi@kernel.org> - 2017-01-16 20:30 +0100
    Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for  handling delay STATUS phase Alan Stern <stern@rowland.harvard.edu> - 2017-01-17 17:10 +0100
      Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase Felipe Balbi <balbi@kernel.org> - 2017-01-23 13:00 +0100

#1560001 — Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase

Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
> On Mon, 16 Jan 2017, Felipe Balbi wrote:
>
>> >>>> Another point here is that the really robust way of fixing this is to
>> >>>> get rid of USB_GADGET_DELAYED_STATUS altogether and just make sure
>> >>>> gadget drivers know how to queue requests for all three phases of a
>> >>>> Control Transfer.
>> >>>>
>> >>>> A lot of code will be removed from all gadget drivers and UDC drivers
>> >>>> while combining all of it in a single place in composite.c.
>
> Don't forget the legacy drivers.

right. I think EHCI Debug gadget is the only one not using composite.c
though. All others under drivers/usb/gadget/legacy are static
configurations of existing function drivers and all use composite.c

>> >>>> The reason I'm saying this is that other UDC drivers might have similar
>> >>>> races already but they just haven't triggered.
>
>> >> Alright, it's important enough to fix this bug. Can you also have a look
>> >> into dropping USB_GADGET_DELAYED_STATUS altogether? If you're too busy,
>> >> no issues. It'll stay in my queue.
>> >
>> > Okay, I will have a look at f_mass_storage driver to see if we can
>> > drop USB_GADGET_DELAYED_STATUS. Thanks.
>> 
>> not only mass storage. It needs to be done for all drivers. The way to
>> do that is to teach functions that control transfers are composed of two
>> or three phases. If you look at UDC drivers today, they all have
>> peculiarities about control transfers to handle stuff that *maybe*
>> gadget drivers won't handle.
>> 
>> What we should do here is make sure that *all* 3 phases always have a
>> matching usb_ep_queue() coming from the upper layers. Whether
>> composite.c or f_*.c handles it, that's an implementation detail. But
>> just to illustrate the problem, we should be able to get rid of
>> dwc3_ep0_out_start() and assume that the upper layer will call
>> usb_ep_queue() when it wants to receive a new SETUP packet.
>> 
>> Likewise, we should be able to assume that STATUS phase will only start
>> based on a usb_ep_queue() call. That way we can remove
>> USB_GADGET_DELAYED_STATUS altogether, because that will *always* be the
>> case. There will be no races to handle apart from the normal case where
>> XferNotReady can come before or after usb_ep_queue(), but we already
>> have proper handling for that too.
>
> It sounds like you're talking about a major change in the way the 
> gadget subsystem handles control requests.

yeah, not the first time :-)

> We can distinguish three cases.  In the existing implementation, they 
> work like this:
>
>     (1) Control-OUT with no data stage.  The gadget driver's setup
> 	routine either queues a request on ep0, which the UDC driver 
> 	uses for the status stage transfer (so it should be a length-0 
> 	IN transfer) and returns 0, or else returns an error, in which
> 	case the UDC driver sends a protocol STALL for the status 
> 	stage.
>
> 	(What the UDC driver should do if the setup routine queues a
> 	request on ep0 and then returns an error is undefined.)

correct

>     (2) Control-OUT with a data stage.  The gadget driver's setup 
> 	routine either queues an OUT request on ep0, which the UDC
> 	driver uses for the data stage transfer, or else returns an
> 	error, in which case the UDC driver sends a protocol STALL for
> 	the data stage.  In the first case, the UDC driver 
> 	automatically queues a 0-length IN request for the status 
> 	stage; the gadget driver does not get any chance to fail the
> 	transfer after the host's data has been successfully received.
> 	(IMO this is a bug in the design of the gadget subsystem.)

exactly, what I'm proposing here would let us fix this detail, too.

>     (3) Control-IN with a data stage.  The gadget driver's setup 
> 	routine either queues an IN request on ep0, which the UDC
> 	driver uses for the data stage transfer, or else returns an
> 	error, in which case the UDC driver sends a protocol STALL for
> 	the data stage.  In the first case, the UDC driver 
> 	automatically queues a 0-length OUT request for the status 
> 	stage; the gadget driver does not get any chance to fail the
> 	transfer after its data has been successfully sent (and I can't 
> 	think of any reason for doing this).

right

> In the delayed-status or delayed-data case, the setup routine does not
> queue a request on ep0 before returning 0; instead the gadget driver
> queues this request at a later time in a separate thread.
>
> The gadget driver never calls usb_ep_queue in order to receive the next
> SETUP packet; the UDC driver takes care of SETUP handling
> automatically.

yeah, that's another thing I'd like to change. Currently, we have no
means to either try to implement device-initiated LPM without adding a
ton of hacks to UDC drivers. If we require upper layers (composite.c,
most of the time) to usb_ep_queue() separate requests for all 3 phases
of a ctrl transfer, we can actually rely on the fact that a new SETUP
phase hasn't been queued yet to trigger U3 entry.

Another detail that this helps is that PM (overall) becomes simpler as,
most likely, we won't need to mess with transfer cancellation, for
example.

> You are suggesting that status stage requests should not be queued 
> automatically by UDC drivers but instead queued explicitly by gadget 
> drivers.  This would mean changing every UDC driver and every gadget 
> driver.

yes, a bit of work but has been done before. One example that comes to
mind is when I added ->udc_start() and ->udc_stop(). It's totally
doable. We can, for instance, add a temporary
"wants_explicit_ctrl_phases"  flag to struct usb_gadget which, if set,
will tell composite.c (or whatever) that the UDC wants explicitly queued
ctrl phases.

Then add support for that to each UDC and set the flag. Once all are
converted, add one extra patch to remove the flag and the legacy
code. This has, of course, the draw back of increasing complexity until
everything is converted over; but if it's all done in a single series, I
can't see any problems with that.

> Also, it won't fix the race that Baolin Wang found.  The setup routine

well, it will help... see below.

> is always called in interrupt context, so it can't sleep.  Doing
> anything non-trivial will require a separate task, and it's possible
> that this task will try to enqueue the data-stage or status-stage
> request before the UDC driver is ready to handle it (for example, 
> before or shortly after the setup routine returns).
>
> To work properly, the UDC driver must be able to accept a request for 
> ep0 any time after it invokes the setup callback -- either before the 
> callback returns or after.

Right, all UDCs are *already* required to support this case anyway
because of USB_GADGET_DELAYED_STATUS. There was a bug in DWC3, sure, but
it was already required to support this case.

By removing USB_GADGET_DELAYED_STATUS altogether and making phases more
explict, we enforce this requirement and it'll be much easier to test
for it IMO.

>It seems that this was the real problem Baolin wanted to fix.

yup

-- 
balbi

[toc] | [next] | [standalone]

#1560740 — Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase

On Mon, 16 Jan 2017, Felipe Balbi wrote:

> > The gadget driver never calls usb_ep_queue in order to receive the next
> > SETUP packet; the UDC driver takes care of SETUP handling
> > automatically.
> 
> yeah, that's another thing I'd like to change. Currently, we have no
> means to either try to implement device-initiated LPM without adding a
> ton of hacks to UDC drivers. If we require upper layers (composite.c,
> most of the time) to usb_ep_queue() separate requests for all 3 phases
> of a ctrl transfer, we can actually rely on the fact that a new SETUP
> phase hasn't been queued yet to trigger U3 entry.

I haven't given any thought to LPM.

However, requiring gadget drivers to request SETUP packets seems rather
questionable.  It flies against the USB spec, which requires
peripherals to accept SETUP packets at any time -- a device is not
allowed to NAK or STALL a SETUP packet (see 8.4.6.4 in the USB-2 spec).  
In fact, the hardware in UDCs probably isn't capable of doing it.

This means that to do what you want, the UDC driver would have to
accept SETUP packets at any time, and store the most recent packet
contents.  Then, when the gadget driver submits a request, the UDC
driver would give it this stored data.  It would also have to detect
and prevent a nasty race where the gadget driver tries to queue a
request on ep0 that is a response to an old SETUP, one that has already
been overwritten.  I'm not even sure preventing this race would be
possible in your scheme.

The advantage to invoking the gadget driver's setup callback directly
from the UDC driver's interrupt handler is that the gadget driver will
know immediately when an old SETUP has become stale.  (That's what
ep0_req_tag is for in f_mass_storage.)  It also provides a concurrency
guarantee, because the driver does not re-enable UDC SETUP interrupts 
until the handler is finished.

> Another detail that this helps is that PM (overall) becomes simpler as,
> most likely, we won't need to mess with transfer cancellation, for
> example.

System PM on a gadget is always troublesome.  Even if the USB 
connection is a wakeup source, it may not be possible to guarantee that 
the gadget can wake up quickly enough to handle an incoming packet.

> > You are suggesting that status stage requests should not be queued 
> > automatically by UDC drivers but instead queued explicitly by gadget 
> > drivers.  This would mean changing every UDC driver and every gadget 
> > driver.
> 
> yes, a bit of work but has been done before. One example that comes to
> mind is when I added ->udc_start() and ->udc_stop(). It's totally
> doable. We can, for instance, add a temporary
> "wants_explicit_ctrl_phases"  flag to struct usb_gadget which, if set,
> will tell composite.c (or whatever) that the UDC wants explicitly queued
> ctrl phases.

The term used in the USB spec is "stage", not "phase".  "Phase" refers
to the packets making up a single transaction: token, data, and
handshake.

Also, data stages are already explicit.  So your temporary flag might 
better be called "wants_explicit_status_stages".

> Then add support for that to each UDC and set the flag. Once all are
> converted, add one extra patch to remove the flag and the legacy
> code. This has, of course, the draw back of increasing complexity until
> everything is converted over; but if it's all done in a single series, I
> can't see any problems with that.
> 
> > Also, it won't fix the race that Baolin Wang found.  The setup routine
> 
> well, it will help... see below.
> 
> > is always called in interrupt context, so it can't sleep.  Doing
> > anything non-trivial will require a separate task, and it's possible
> > that this task will try to enqueue the data-stage or status-stage
> > request before the UDC driver is ready to handle it (for example, 
> > before or shortly after the setup routine returns).
> >
> > To work properly, the UDC driver must be able to accept a request for 
> > ep0 any time after it invokes the setup callback -- either before the 
> > callback returns or after.
> 
> Right, all UDCs are *already* required to support this case anyway
> because of USB_GADGET_DELAYED_STATUS. There was a bug in DWC3, sure, but
> it was already required to support this case.
> 
> By removing USB_GADGET_DELAYED_STATUS altogether and making phases more
> explict, we enforce this requirement and it'll be much easier to test
> for it IMO.

Okay, I can see the point of requiring explicit status requests.  
Implementing it will be a little tricky, because right now some status 
requests already are explicit (those for length-0 OUT transfers) while 
others are implicit.

(One possible approach would be to have the setup routine return 
different values for explicit and implicit status stages -- for 
example, return 1 if it wants to submit an explicit status request.  
That wouldn't be very different from the current 
USB_GADGET_DELAYED_STATUS approach.)

On the other hand, I am very doubtful about requiring explicit setup 
requests.

Alan Stern

[toc] | [prev] | [next] | [standalone]

#1564921

[Multipart message — attachments visible in raw view] — view raw

Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
> On Mon, 16 Jan 2017, Felipe Balbi wrote:
>
>> > The gadget driver never calls usb_ep_queue in order to receive the next
>> > SETUP packet; the UDC driver takes care of SETUP handling
>> > automatically.
>> 
>> yeah, that's another thing I'd like to change. Currently, we have no
>> means to either try to implement device-initiated LPM without adding a
>> ton of hacks to UDC drivers. If we require upper layers (composite.c,
>> most of the time) to usb_ep_queue() separate requests for all 3 phases
>> of a ctrl transfer, we can actually rely on the fact that a new SETUP
>> phase hasn't been queued yet to trigger U3 entry.
>
> I haven't given any thought to LPM.

okay

> However, requiring gadget drivers to request SETUP packets seems rather
> questionable.  It flies against the USB spec, which requires

right, maybe SETUP is a bit of an overkill. DATA and STATUS, however,
should be doable.

> peripherals to accept SETUP packets at any time -- a device is not
> allowed to NAK or STALL a SETUP packet (see 8.4.6.4 in the USB-2 spec).  
> In fact, the hardware in UDCs probably isn't capable of doing it.
>
> This means that to do what you want, the UDC driver would have to
> accept SETUP packets at any time, and store the most recent packet
> contents.  Then, when the gadget driver submits a request, the UDC
> driver would give it this stored data.  It would also have to detect

that's right, I missed that part.

> and prevent a nasty race where the gadget driver tries to queue a
> request on ep0 that is a response to an old SETUP, one that has already
> been overwritten.  I'm not even sure preventing this race would be
> possible in your scheme.
>
> The advantage to invoking the gadget driver's setup callback directly
> from the UDC driver's interrupt handler is that the gadget driver will
> know immediately when an old SETUP has become stale.  (That's what
> ep0_req_tag is for in f_mass_storage.)  It also provides a concurrency
> guarantee, because the driver does not re-enable UDC SETUP interrupts 
> until the handler is finished.
>
>> Another detail that this helps is that PM (overall) becomes simpler as,
>> most likely, we won't need to mess with transfer cancellation, for
>> example.
>
> System PM on a gadget is always troublesome.  Even if the USB 
> connection is a wakeup source, it may not be possible to guarantee that 
> the gadget can wake up quickly enough to handle an incoming packet.

that's true.

>> > You are suggesting that status stage requests should not be queued 
>> > automatically by UDC drivers but instead queued explicitly by gadget 
>> > drivers.  This would mean changing every UDC driver and every gadget 
>> > driver.
>> 
>> yes, a bit of work but has been done before. One example that comes to
>> mind is when I added ->udc_start() and ->udc_stop(). It's totally
>> doable. We can, for instance, add a temporary
>> "wants_explicit_ctrl_phases"  flag to struct usb_gadget which, if set,
>> will tell composite.c (or whatever) that the UDC wants explicitly queued
>> ctrl phases.
>
> The term used in the USB spec is "stage", not "phase".  "Phase" refers
> to the packets making up a single transaction: token, data, and
> handshake.
>
> Also, data stages are already explicit.  So your temporary flag might 
> better be called "wants_explicit_status_stages".

I stand corrected ;-)

>> Then add support for that to each UDC and set the flag. Once all are
>> converted, add one extra patch to remove the flag and the legacy
>> code. This has, of course, the draw back of increasing complexity until
>> everything is converted over; but if it's all done in a single series, I
>> can't see any problems with that.
>> 
>> > Also, it won't fix the race that Baolin Wang found.  The setup routine
>> 
>> well, it will help... see below.
>> 
>> > is always called in interrupt context, so it can't sleep.  Doing
>> > anything non-trivial will require a separate task, and it's possible
>> > that this task will try to enqueue the data-stage or status-stage
>> > request before the UDC driver is ready to handle it (for example, 
>> > before or shortly after the setup routine returns).
>> >
>> > To work properly, the UDC driver must be able to accept a request for 
>> > ep0 any time after it invokes the setup callback -- either before the 
>> > callback returns or after.
>> 
>> Right, all UDCs are *already* required to support this case anyway
>> because of USB_GADGET_DELAYED_STATUS. There was a bug in DWC3, sure, but
>> it was already required to support this case.
>> 
>> By removing USB_GADGET_DELAYED_STATUS altogether and making phases more
>> explict, we enforce this requirement and it'll be much easier to test
>> for it IMO.
>
> Okay, I can see the point of requiring explicit status requests.  
> Implementing it will be a little tricky, because right now some status 
> requests already are explicit (those for length-0 OUT transfers) while 
> others are implicit.

exactly. And that's source of issues for every new UDC driver we get.

> (One possible approach would be to have the setup routine return 
> different values for explicit and implicit status stages -- for 
> example, return 1 if it wants to submit an explicit status request.  
> That wouldn't be very different from the current 
> USB_GADGET_DELAYED_STATUS approach.)

not really, no. The idea was for composite.c and/or functions to support
both methods (temporarily) and use "gadget->wants_explicit_stages" to
explicitly queue DATA and STATUS. That would mean that f_mass_storage
wouldn't have to return DELAYED_STATUS if
(gadget->wants_explicit_stages).

After all UDCs are converted over and set wants_explicit_stages (which
should all be done in a single series), then we get rid of the flag and
the older method of DELAYED_STATUS.

> On the other hand, I am very doubtful about requiring explicit setup 
> requests.

right, me too ;-)

-- 
balbi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web