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

Groups > linux.kernel > #1583105 > 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 Baolin Wang <baolin.wang@linaro.org> - 2017-02-17 06:50 +0100
    Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase Felipe Balbi <balbi@kernel.org> - 2017-02-17 09:10 +0100
      Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for  handling delay STATUS phase Baolin Wang <baolin.wang@linaro.org> - 2017-02-20 03:30 +0100
      Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for  handling delay STATUS phase Baolin Wang <baolin.wang@linaro.org> - 2017-02-21 10:20 +0100
        Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for  handling delay STATUS phase Alan Stern <stern@rowland.harvard.edu> - 2017-02-27 23:20 +0100
          Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase Felipe Balbi <balbi@kernel.org> - 2017-02-28 13:10 +0100
            Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for  handling delay STATUS phase Alan Stern <stern@rowland.harvard.edu> - 2017-02-28 20:00 +0100

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

On 23 January 2017 at 19:57, Felipe Balbi <balbi@kernel.org> wrote:
>
> 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.

(Sorry for late reply due to my holiday)
I also met the problem pointed by Alan, from my test, I still want to
need one return value to indicate if it wants to submit an explicit
status request. Think about the Control-IN with a data stage, we can
not get the STATUS phase request from usb_ep_queue() call, and we need
to handle this STATUS phase request in dwc3_ep0_xfernotready(). But
Control-OUT will get one 0-length IN request for the status stage from
usb_ep_queue(), so we need one return value from setup routine to
distinguish these in dwc3_ep0_xfernotready(), or we can not handle
status request correctly. Maybe I missed something else.

>
>> On the other hand, I am very doubtful about requiring explicit setup
>> requests.
>
> right, me too ;-)

So do you suggest me continue to try to do this? Thanks.

-- 
Baolin.wang
Best Regards

[toc] | [next] | [standalone]

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

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

Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
>>> (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.
>
> (Sorry for late reply due to my holiday)
> I also met the problem pointed by Alan, from my test, I still want to
> need one return value to indicate if it wants to submit an explicit
> status request. Think about the Control-IN with a data stage, we can
> not get the STATUS phase request from usb_ep_queue() call, and we need

why not? wLength tells you that this is a 3-stage transfer. Gadget
driver should be able to figure out that it needs to usb_ep_queue()
another request for status stage.

> to handle this STATUS phase request in dwc3_ep0_xfernotready(). But
> Control-OUT will get one 0-length IN request for the status stage from
> usb_ep_queue(), so we need one return value from setup routine to

no we don't :-)

> distinguish these in dwc3_ep0_xfernotready(), or we can not handle
> status request correctly. Maybe I missed something else.
>>
>>> On the other hand, I am very doubtful about requiring explicit setup
>>> requests.
>>
>> right, me too ;-)
>
> So do you suggest me continue to try to do this? Thanks.

explicit setup? no
explicit status? yes

If you don't wanna do it, it's fine :-) I'll just add to my TODO
list. It just depends on how much other tasks you have on your end ;-)

-- 
balbi

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

#1584310

On 17 February 2017 at 16:04, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>>>> (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.
>>
>> (Sorry for late reply due to my holiday)
>> I also met the problem pointed by Alan, from my test, I still want to
>> need one return value to indicate if it wants to submit an explicit
>> status request. Think about the Control-IN with a data stage, we can
>> not get the STATUS phase request from usb_ep_queue() call, and we need
>
> why not? wLength tells you that this is a 3-stage transfer. Gadget
> driver should be able to figure out that it needs to usb_ep_queue()
> another request for status stage.
>
>> to handle this STATUS phase request in dwc3_ep0_xfernotready(). But
>> Control-OUT will get one 0-length IN request for the status stage from
>> usb_ep_queue(), so we need one return value from setup routine to
>
> no we don't :-)
>
>> distinguish these in dwc3_ep0_xfernotready(), or we can not handle
>> status request correctly. Maybe I missed something else.
>>>
>>>> On the other hand, I am very doubtful about requiring explicit setup
>>>> requests.
>>>
>>> right, me too ;-)
>>
>> So do you suggest me continue to try to do this? Thanks.
>
> explicit setup? no
> explicit status? yes
>
> If you don't wanna do it, it's fine :-) I'll just add to my TODO
> list. It just depends on how much other tasks you have on your end ;-)

OK, I will take some time to check and test again. It will be better
if I send out one RFC patch to review.

-- 
Baolin.wang
Best Regards

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

#1585153

On 17 February 2017 at 16:04, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>>>> (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.
>>
>> (Sorry for late reply due to my holiday)
>> I also met the problem pointed by Alan, from my test, I still want to
>> need one return value to indicate if it wants to submit an explicit
>> status request. Think about the Control-IN with a data stage, we can
>> not get the STATUS phase request from usb_ep_queue() call, and we need
>
> why not? wLength tells you that this is a 3-stage transfer. Gadget
> driver should be able to figure out that it needs to usb_ep_queue()
> another request for status stage.

I tried again, but still can not work. Suppose the no-data control:
(1) SET_ADDRESS request: function driver will not queue one request
for status phase by usb_ep_queue() call.
(2) SET_CONFIGURATION request: function driver will queue one 0-length
request for status phase by usb_ep_queue() call, especially for
mass_storage driver, it will queue one request  for status phase
later.

So I am not sure how the Gadget driver can figure out that it needs to
usb_ep_queue() another request for status stage when handling the
no-data control?

>> to handle this STATUS phase request in dwc3_ep0_xfernotready(). But
>> Control-OUT will get one 0-length IN request for the status stage from
>> usb_ep_queue(), so we need one return value from setup routine to
>
> no we don't :-)
>
>> distinguish these in dwc3_ep0_xfernotready(), or we can not handle
>> status request correctly. Maybe I missed something else.
>>>
>>>> On the other hand, I am very doubtful about requiring explicit setup
>>>> requests.
>>>
>>> right, me too ;-)
>>
>> So do you suggest me continue to try to do this? Thanks.
>
> explicit setup? no
> explicit status? yes
>
> If you don't wanna do it, it's fine :-) I'll just add to my TODO
> list. It just depends on how much other tasks you have on your end ;-)
>
> --
> balbi



-- 
Baolin.wang
Best Regards

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

#1588973

On Tue, 21 Feb 2017, Baolin Wang wrote:

> On 17 February 2017 at 16:04, Felipe Balbi <balbi@kernel.org> wrote:
> >
> > Hi,
> >
> > Baolin Wang <baolin.wang@linaro.org> writes:
> >>>> (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.
> >>
> >> (Sorry for late reply due to my holiday)
> >> I also met the problem pointed by Alan, from my test, I still want to
> >> need one return value to indicate if it wants to submit an explicit
> >> status request. Think about the Control-IN with a data stage, we can
> >> not get the STATUS phase request from usb_ep_queue() call, and we need
> >
> > why not? wLength tells you that this is a 3-stage transfer. Gadget
> > driver should be able to figure out that it needs to usb_ep_queue()
> > another request for status stage.
> 
> I tried again, but still can not work. Suppose the no-data control:
> (1) SET_ADDRESS request: function driver will not queue one request
> for status phase by usb_ep_queue() call.

Function drivers do not handle Set-Address requests at all.  The UDC
driver handles these requests without telling the gadget driver about 
them.

> (2) SET_CONFIGURATION request: function driver will queue one 0-length
> request for status phase by usb_ep_queue() call, especially for
> mass_storage driver, it will queue one request  for status phase
> later.
> 
> So I am not sure how the Gadget driver can figure out that it needs to
> usb_ep_queue() another request for status stage when handling the
> no-data control?

Gadget drivers already queue status-stage requests for no-data
control-OUT requests.  The difficulty comes when you want to handle an
IN request or an OUT request with a data stage.

Alan Stern

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

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

Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
>> So I am not sure how the Gadget driver can figure out that it needs to
>> usb_ep_queue() another request for status stage when handling the
>> no-data control?
>
> Gadget drivers already queue status-stage requests for no-data
> control-OUT requests.  The difficulty comes when you want to handle an
> IN request or an OUT request with a data stage.

I don't see a difficulty there. Gadget driver will see wLength and
notice it needs both data and status stages, then it does:

usb_ep_queue(ep0, data_req, GFP_KERNEL);
usb_ep_queue(ep0, status_req, GFP_KERNEL);

Just needs to prepare both requests and queue them both ahead of
time. UDC drivers should hold both requests in their own private list
and process one at a time.

-- 
balbi

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

#1589704

On Tue, 28 Feb 2017, Felipe Balbi wrote:

> 
> Hi,
> 
> Alan Stern <stern@rowland.harvard.edu> writes:
> >> So I am not sure how the Gadget driver can figure out that it needs to
> >> usb_ep_queue() another request for status stage when handling the
> >> no-data control?
> >
> > Gadget drivers already queue status-stage requests for no-data
> > control-OUT requests.  The difficulty comes when you want to handle an
> > IN request or an OUT request with a data stage.
> 
> I don't see a difficulty there. Gadget driver will see wLength and
> notice it needs both data and status stages, then it does:
> 
> usb_ep_queue(ep0, data_req, GFP_KERNEL);
> usb_ep_queue(ep0, status_req, GFP_KERNEL);

The main difficulty is that all the gadget/function drivers will have 
to be audited to add the status requests.

> Just needs to prepare both requests and queue them both ahead of
> time. UDC drivers should hold both requests in their own private list
> and process one at a time.

Or the gadget driver should queue the status request after the 
data stage has been fully processed, in the case of an OUT transfer.

There is still a possible race.  The host might send another SETUP
packet before the status request has been queued, or after it has been
queued but before the UDC driver has completed it.  (Of course, that's 
already true now for the data request...)

Alan Stern

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web