Message-ID: <87fuiwt0nk.fsf@linux.intel.com>
Date:   Thu, 02 Mar 2017 12:48:15 +0200
From:   Felipe Balbi <balbi@...nel.org>
To:     Baolin Wang <baolin.wang@...aro.org>,
        Alan Stern <stern@...land.harvard.edu>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        USB <linux-usb@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Linaro Kernel Mailman List <linaro-kernel@...ts.linaro.org>,
        Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase


Hi,

Baolin Wang <baolin.wang@...aro.org> writes:
>>> > Baolin Wang <baolin.wang@...aro.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.
>
> Correct. What I mean is it will not queue one request for status phase
> by usb_ep_queue() call, UDC driver will do that.

how the UDC driver handles this case, is up to the UDC driver. In DWC3 I
chose to rely on the same ep_queue mechanism; but that's an arbitrary
choice.

>>> (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.
>>
>
> I try to explain that explicitly, In dwc3 driver, we can handle the
> STATUS phase request in 2 places: (1) in usb_ep_queue(), (2) in
> dwc3_ep0_xfernotready()

this is the very detail that what I proposed will change. After what I
proposed is implemented, status stage will *always* be done in response
to a usb_ep_queue().

> For no-data control-OUT requests:
> (1) SET_ADDRESS request: no request queued for status phase by
> usb_ep_queue(), dwc3 driver need handle the STATUS phase request when
> one not-ready-event comes in dwc3_ep0_xfernotready() function.

or we change dwc3 to prepare an internal request and queue it to its own
enpdoint.

> (2) SET_CONFIGURATION request: function driver will queue one 0-length
> request for status phase by usb_ep_queue(), but we can handle this
> request in usb_ep_queue() or dwc3_ep0_xfernotready(). When the

for DWC3, status stage *must* be done after XFER_NOT_READY event. That's
required by the databook. What you're claiming is not correct.

The only situation where we start status stage from usb_ep_queue() is
for the case when XFER_NOT_READY already triggered and we set
PENDING_REQUEST flag for the endpoint.

> function driver queued one 0-length request for status phase before
> the not-ready-event comes, we need handle this request in
> dwc3_ep0_xfernotready() when the not-ready-event comes. When  the
> function driver queued one 0-length request for status phase after the
> not-ready-event comes, we can handle this request in usb_ep_queue().

already implemented. Nothing will change for this case.

> So in dwc3_ep0_xfernotready(), we need to check if the request for
> status phase has been queued into pending request list, but if the
> pending request list is NULL, which means the function driver have not
> queued one 0-length request until now (then we can handle it in
> usb_ep_queue()), or situation (1) (no request queued for status
> phase), then I can not identify this 2 situations to determine where I
> can handle the status request. Hope I make it clear.

this is already implemented. There's nothing new coming to this case.

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)

Powered by blists - more mailing lists