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

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

[Baolin Wang]

When handing the SETUP packet by composite_setup(), we will release the
dwc->lock. If we get the 'USB_GADGET_DELAYED_STATUS' result from setup
function, which means we need to delay handling the STATUS phase.

But during the lock release period, maybe the request for handling delay
STATUS phase has been queued into list before we set 'dwc->delayed_status'
flag or entering 'EP0_STATUS_PHASE' phase, then we will miss the chance
to handle the STATUS phase. Thus we should check if the request for delay
STATUS phase has been enqueued when entering 'EP0_STATUS_PHASE' phase in
dwc3_ep0_xfernotready(), if so, we should handle it.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/usb/dwc3/ep0.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 9bb1f85..e689ced 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -1123,7 +1123,21 @@ static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
 		dwc->ep0state = EP0_STATUS_PHASE;
 
 		if (dwc->delayed_status) {
+			struct dwc3_ep *dep = dwc->eps[0];
+
 			WARN_ON_ONCE(event->endpoint_number != 1);
+			/*
+			 * We should handle the delay STATUS phase here if the
+			 * request for handling delay STATUS has been queued
+			 * into the list.
+			 */
+			if (!list_empty(&dep->pending_list)) {
+				dwc->delayed_status = false;
+				usb_gadget_set_state(&dwc->gadget,
+						     USB_STATE_CONFIGURED);
+				dwc3_ep0_do_control_status(dwc, event);
+			}
+
 			return;
 		}
 
-- 
1.7.9.5

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

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
> When handing the SETUP packet by composite_setup(), we will release the
> dwc->lock. If we get the 'USB_GADGET_DELAYED_STATUS' result from setup
> function, which means we need to delay handling the STATUS phase.

this sentence needs a little work. Seems like it's missing some
information.

anyway, I get that we release the lock but...

> But during the lock release period, maybe the request for handling delay

execution of ->setup() itself should be locked. I can see that it's only
locked for set_config() which is rather peculiar.

What exact request you had when you triggered this? (Hint: dwc3
tracepoints print out ctrl request bytes). IIRC, only set_config() or
f->set_alt() can actually return USB_GADGET_DELAYED_STATUS.

Which gadget driver were you using when you triggered this?

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.

The reason I'm saying this is that other UDC drivers might have similar
races already but they just haven't triggered.

> STATUS phase has been queued into list before we set 'dwc->delayed_status'
> flag or entering 'EP0_STATUS_PHASE' phase, then we will miss the chance
> to handle the STATUS phase. Thus we should check if the request for delay
> STATUS phase has been enqueued when entering 'EP0_STATUS_PHASE' phase in
> dwc3_ep0_xfernotready(), if so, we should handle it.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  drivers/usb/dwc3/ep0.c |   14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 9bb1f85..e689ced 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -1123,7 +1123,21 @@ static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
>  		dwc->ep0state = EP0_STATUS_PHASE;
>  
>  		if (dwc->delayed_status) {
> +			struct dwc3_ep *dep = dwc->eps[0];
> +
>  			WARN_ON_ONCE(event->endpoint_number != 1);
> +			/*
> +			 * We should handle the delay STATUS phase here if the
> +			 * request for handling delay STATUS has been queued
> +			 * into the list.
> +			 */
> +			if (!list_empty(&dep->pending_list)) {
> +				dwc->delayed_status = false;
> +				usb_gadget_set_state(&dwc->gadget,
> +						     USB_STATE_CONFIGURED);

Isn't this patch also changing the normal case when usb_ep_queue() comes
later? I guess list_empty() protects against that...

-- 
balbi

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

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

[Baolin Wang]

Hi,

On 16 January 2017 at 18:56, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>> When handing the SETUP packet by composite_setup(), we will release the
>> dwc->lock. If we get the 'USB_GADGET_DELAYED_STATUS' result from setup
>> function, which means we need to delay handling the STATUS phase.
>
> this sentence needs a little work. Seems like it's missing some
> information.
>
> anyway, I get that we release the lock but...
>
>> But during the lock release period, maybe the request for handling delay
>
> execution of ->setup() itself should be locked. I can see that it's only
> locked for set_config() which is rather peculiar.
>
> What exact request you had when you triggered this? (Hint: dwc3
> tracepoints print out ctrl request bytes). IIRC, only set_config() or
> f->set_alt() can actually return USB_GADGET_DELAYED_STATUS.

Yes, when host set configuration for mass storage driver, it can
produce this issue.

>
> Which gadget driver were you using when you triggered this?

mass storage driver. When host issues the setting config request, we
will get USB_GADGET_DELAYED_STATUS result from
set_config()--->fsg_set_alt(). Then the mass storage driver will issue
one thread to complete the status stage by ep0_queue() (this thread
may be running on another core), then if the thread issues ep0_queue()
too fast before we get the dwc->lock in dwc3_ep0_delegate_req() or
before we get into the STATUS stage, then we can not handle this
request for the delay STATUS stage in dwc3_gadget_ep0_queue().

>
> 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.
>
> The reason I'm saying this is that other UDC drivers might have similar
> races already but they just haven't triggered.

Yes, maybe.

>
>> STATUS phase has been queued into list before we set 'dwc->delayed_status'
>> flag or entering 'EP0_STATUS_PHASE' phase, then we will miss the chance
>> to handle the STATUS phase. Thus we should check if the request for delay
>> STATUS phase has been enqueued when entering 'EP0_STATUS_PHASE' phase in
>> dwc3_ep0_xfernotready(), if so, we should handle it.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>>  drivers/usb/dwc3/ep0.c |   14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>> index 9bb1f85..e689ced 100644
>> --- a/drivers/usb/dwc3/ep0.c
>> +++ b/drivers/usb/dwc3/ep0.c
>> @@ -1123,7 +1123,21 @@ static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
>>               dwc->ep0state = EP0_STATUS_PHASE;
>>
>>               if (dwc->delayed_status) {
>> +                     struct dwc3_ep *dep = dwc->eps[0];
>> +
>>                       WARN_ON_ONCE(event->endpoint_number != 1);
>> +                     /*
>> +                      * We should handle the delay STATUS phase here if the
>> +                      * request for handling delay STATUS has been queued
>> +                      * into the list.
>> +                      */
>> +                     if (!list_empty(&dep->pending_list)) {
>> +                             dwc->delayed_status = false;
>> +                             usb_gadget_set_state(&dwc->gadget,
>> +                                                  USB_STATE_CONFIGURED);
>
> Isn't this patch also changing the normal case when usb_ep_queue() comes
> later? I guess list_empty() protects against that...

I think it will not change other cases, we only handle the delayed
status and I've tested it for a while and I did not find any problem.

-- 
Baolin.wang
Best Regards

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

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
> Hi,
>
> On 16 January 2017 at 18:56, Felipe Balbi <balbi@kernel.org> wrote:
>>
>> Hi,
>>
>> Baolin Wang <baolin.wang@linaro.org> writes:
>>> When handing the SETUP packet by composite_setup(), we will release the
>>> dwc->lock. If we get the 'USB_GADGET_DELAYED_STATUS' result from setup
>>> function, which means we need to delay handling the STATUS phase.
>>
>> this sentence needs a little work. Seems like it's missing some
>> information.
>>
>> anyway, I get that we release the lock but...
>>
>>> But during the lock release period, maybe the request for handling delay
>>
>> execution of ->setup() itself should be locked. I can see that it's only
>> locked for set_config() which is rather peculiar.
>>
>> What exact request you had when you triggered this? (Hint: dwc3
>> tracepoints print out ctrl request bytes). IIRC, only set_config() or
>> f->set_alt() can actually return USB_GADGET_DELAYED_STATUS.
>
> Yes, when host set configuration for mass storage driver, it can
> produce this issue.
>
>>
>> Which gadget driver were you using when you triggered this?
>
> mass storage driver. When host issues the setting config request, we
> will get USB_GADGET_DELAYED_STATUS result from
> set_config()--->fsg_set_alt(). Then the mass storage driver will issue
> one thread to complete the status stage by ep0_queue() (this thread
> may be running on another core), then if the thread issues ep0_queue()
> too fast before we get the dwc->lock in dwc3_ep0_delegate_req() or
> before we get into the STATUS stage, then we can not handle this
> request for the delay STATUS stage in dwc3_gadget_ep0_queue().
>
>>
>> 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.
>>
>> The reason I'm saying this is that other UDC drivers might have similar
>> races already but they just haven't triggered.
>
> Yes, maybe.
>
>>
>>> STATUS phase has been queued into list before we set 'dwc->delayed_status'
>>> flag or entering 'EP0_STATUS_PHASE' phase, then we will miss the chance
>>> to handle the STATUS phase. Thus we should check if the request for delay
>>> STATUS phase has been enqueued when entering 'EP0_STATUS_PHASE' phase in
>>> dwc3_ep0_xfernotready(), if so, we should handle it.
>>>
>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>> ---
>>>  drivers/usb/dwc3/ep0.c |   14 ++++++++++++++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>> index 9bb1f85..e689ced 100644
>>> --- a/drivers/usb/dwc3/ep0.c
>>> +++ b/drivers/usb/dwc3/ep0.c
>>> @@ -1123,7 +1123,21 @@ static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
>>>               dwc->ep0state = EP0_STATUS_PHASE;
>>>
>>>               if (dwc->delayed_status) {
>>> +                     struct dwc3_ep *dep = dwc->eps[0];
>>> +
>>>                       WARN_ON_ONCE(event->endpoint_number != 1);
>>> +                     /*
>>> +                      * We should handle the delay STATUS phase here if the
>>> +                      * request for handling delay STATUS has been queued
>>> +                      * into the list.
>>> +                      */
>>> +                     if (!list_empty(&dep->pending_list)) {
>>> +                             dwc->delayed_status = false;
>>> +                             usb_gadget_set_state(&dwc->gadget,
>>> +                                                  USB_STATE_CONFIGURED);
>>
>> Isn't this patch also changing the normal case when usb_ep_queue() comes
>> later? I guess list_empty() protects against that...
>
> I think it will not change other cases, we only handle the delayed
> status and I've tested it for a while and I did not find any problem.

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.

-- 
balbi

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

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

[Baolin Wang]

Hi,

On 16 January 2017 at 19:29, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>> Hi,
>>
>> On 16 January 2017 at 18:56, Felipe Balbi <balbi@kernel.org> wrote:
>>>
>>> Hi,
>>>
>>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>> When handing the SETUP packet by composite_setup(), we will release the
>>>> dwc->lock. If we get the 'USB_GADGET_DELAYED_STATUS' result from setup
>>>> function, which means we need to delay handling the STATUS phase.
>>>
>>> this sentence needs a little work. Seems like it's missing some
>>> information.
>>>
>>> anyway, I get that we release the lock but...
>>>
>>>> But during the lock release period, maybe the request for handling delay
>>>
>>> execution of ->setup() itself should be locked. I can see that it's only
>>> locked for set_config() which is rather peculiar.
>>>
>>> What exact request you had when you triggered this? (Hint: dwc3
>>> tracepoints print out ctrl request bytes). IIRC, only set_config() or
>>> f->set_alt() can actually return USB_GADGET_DELAYED_STATUS.
>>
>> Yes, when host set configuration for mass storage driver, it can
>> produce this issue.
>>
>>>
>>> Which gadget driver were you using when you triggered this?
>>
>> mass storage driver. When host issues the setting config request, we
>> will get USB_GADGET_DELAYED_STATUS result from
>> set_config()--->fsg_set_alt(). Then the mass storage driver will issue
>> one thread to complete the status stage by ep0_queue() (this thread
>> may be running on another core), then if the thread issues ep0_queue()
>> too fast before we get the dwc->lock in dwc3_ep0_delegate_req() or
>> before we get into the STATUS stage, then we can not handle this
>> request for the delay STATUS stage in dwc3_gadget_ep0_queue().
>>
>>>
>>> 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.
>>>
>>> The reason I'm saying this is that other UDC drivers might have similar
>>> races already but they just haven't triggered.
>>
>> Yes, maybe.
>>
>>>
>>>> STATUS phase has been queued into list before we set 'dwc->delayed_status'
>>>> flag or entering 'EP0_STATUS_PHASE' phase, then we will miss the chance
>>>> to handle the STATUS phase. Thus we should check if the request for delay
>>>> STATUS phase has been enqueued when entering 'EP0_STATUS_PHASE' phase in
>>>> dwc3_ep0_xfernotready(), if so, we should handle it.
>>>>
>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>> ---
>>>>  drivers/usb/dwc3/ep0.c |   14 ++++++++++++++
>>>>  1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>>> index 9bb1f85..e689ced 100644
>>>> --- a/drivers/usb/dwc3/ep0.c
>>>> +++ b/drivers/usb/dwc3/ep0.c
>>>> @@ -1123,7 +1123,21 @@ static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
>>>>               dwc->ep0state = EP0_STATUS_PHASE;
>>>>
>>>>               if (dwc->delayed_status) {
>>>> +                     struct dwc3_ep *dep = dwc->eps[0];
>>>> +
>>>>                       WARN_ON_ONCE(event->endpoint_number != 1);
>>>> +                     /*
>>>> +                      * We should handle the delay STATUS phase here if the
>>>> +                      * request for handling delay STATUS has been queued
>>>> +                      * into the list.
>>>> +                      */
>>>> +                     if (!list_empty(&dep->pending_list)) {
>>>> +                             dwc->delayed_status = false;
>>>> +                             usb_gadget_set_state(&dwc->gadget,
>>>> +                                                  USB_STATE_CONFIGURED);
>>>
>>> Isn't this patch also changing the normal case when usb_ep_queue() comes
>>> later? I guess list_empty() protects against that...
>>
>> I think it will not change other cases, we only handle the delayed
>> status and I've tested it for a while and I did not find any problem.
>
> 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.

-- 
Baolin.wang
Best Regards

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

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
> Hi,
>
> On 16 January 2017 at 19:29, Felipe Balbi <balbi@kernel.org> wrote:
>>
>> Hi,
>>
>> Baolin Wang <baolin.wang@linaro.org> writes:
>>> Hi,
>>>
>>> On 16 January 2017 at 18:56, Felipe Balbi <balbi@kernel.org> wrote:
>>>>
>>>> Hi,
>>>>
>>>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>> When handing the SETUP packet by composite_setup(), we will release the
>>>>> dwc->lock. If we get the 'USB_GADGET_DELAYED_STATUS' result from setup
>>>>> function, which means we need to delay handling the STATUS phase.
>>>>
>>>> this sentence needs a little work. Seems like it's missing some
>>>> information.
>>>>
>>>> anyway, I get that we release the lock but...
>>>>
>>>>> But during the lock release period, maybe the request for handling delay
>>>>
>>>> execution of ->setup() itself should be locked. I can see that it's only
>>>> locked for set_config() which is rather peculiar.
>>>>
>>>> What exact request you had when you triggered this? (Hint: dwc3
>>>> tracepoints print out ctrl request bytes). IIRC, only set_config() or
>>>> f->set_alt() can actually return USB_GADGET_DELAYED_STATUS.
>>>
>>> Yes, when host set configuration for mass storage driver, it can
>>> produce this issue.
>>>
>>>>
>>>> Which gadget driver were you using when you triggered this?
>>>
>>> mass storage driver. When host issues the setting config request, we
>>> will get USB_GADGET_DELAYED_STATUS result from
>>> set_config()--->fsg_set_alt(). Then the mass storage driver will issue
>>> one thread to complete the status stage by ep0_queue() (this thread
>>> may be running on another core), then if the thread issues ep0_queue()
>>> too fast before we get the dwc->lock in dwc3_ep0_delegate_req() or
>>> before we get into the STATUS stage, then we can not handle this
>>> request for the delay STATUS stage in dwc3_gadget_ep0_queue().
>>>
>>>>
>>>> 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.
>>>>
>>>> The reason I'm saying this is that other UDC drivers might have similar
>>>> races already but they just haven't triggered.
>>>
>>> Yes, maybe.
>>>
>>>>
>>>>> STATUS phase has been queued into list before we set 'dwc->delayed_status'
>>>>> flag or entering 'EP0_STATUS_PHASE' phase, then we will miss the chance
>>>>> to handle the STATUS phase. Thus we should check if the request for delay
>>>>> STATUS phase has been enqueued when entering 'EP0_STATUS_PHASE' phase in
>>>>> dwc3_ep0_xfernotready(), if so, we should handle it.
>>>>>
>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>> ---
>>>>>  drivers/usb/dwc3/ep0.c |   14 ++++++++++++++
>>>>>  1 file changed, 14 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>>>> index 9bb1f85..e689ced 100644
>>>>> --- a/drivers/usb/dwc3/ep0.c
>>>>> +++ b/drivers/usb/dwc3/ep0.c
>>>>> @@ -1123,7 +1123,21 @@ static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
>>>>>               dwc->ep0state = EP0_STATUS_PHASE;
>>>>>
>>>>>               if (dwc->delayed_status) {
>>>>> +                     struct dwc3_ep *dep = dwc->eps[0];
>>>>> +
>>>>>                       WARN_ON_ONCE(event->endpoint_number != 1);
>>>>> +                     /*
>>>>> +                      * We should handle the delay STATUS phase here if the
>>>>> +                      * request for handling delay STATUS has been queued
>>>>> +                      * into the list.
>>>>> +                      */
>>>>> +                     if (!list_empty(&dep->pending_list)) {
>>>>> +                             dwc->delayed_status = false;
>>>>> +                             usb_gadget_set_state(&dwc->gadget,
>>>>> +                                                  USB_STATE_CONFIGURED);
>>>>
>>>> Isn't this patch also changing the normal case when usb_ep_queue() comes
>>>> later? I guess list_empty() protects against that...
>>>
>>> I think it will not change other cases, we only handle the delayed
>>> status and I've tested it for a while and I did not find any problem.
>>
>> 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.

-- 
balbi

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

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

[Alan Stern]

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.

> >>>>
> >>>> 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.

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.)

    (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.)

    (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).

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.

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.

Also, it won't fix the race that Baolin Wang found.  The setup routine
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.  It seems that this was the real problem 
Baolin wanted to fix.

Alan Stern

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

[Felipe Balbi]

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

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

[Baolin Wang]

Hi,

On 16 January 2017 at 20:06, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>> Hi,
>>
>> On 16 January 2017 at 19:29, Felipe Balbi <balbi@kernel.org> wrote:
>>>
>>> Hi,
>>>
>>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>> Hi,
>>>>
>>>> On 16 January 2017 at 18:56, Felipe Balbi <balbi@kernel.org> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>>> When handing the SETUP packet by composite_setup(), we will release the
>>>>>> dwc->lock. If we get the 'USB_GADGET_DELAYED_STATUS' result from setup
>>>>>> function, which means we need to delay handling the STATUS phase.
>>>>>
>>>>> this sentence needs a little work. Seems like it's missing some
>>>>> information.
>>>>>
>>>>> anyway, I get that we release the lock but...
>>>>>
>>>>>> But during the lock release period, maybe the request for handling delay
>>>>>
>>>>> execution of ->setup() itself should be locked. I can see that it's only
>>>>> locked for set_config() which is rather peculiar.
>>>>>
>>>>> What exact request you had when you triggered this? (Hint: dwc3
>>>>> tracepoints print out ctrl request bytes). IIRC, only set_config() or
>>>>> f->set_alt() can actually return USB_GADGET_DELAYED_STATUS.
>>>>
>>>> Yes, when host set configuration for mass storage driver, it can
>>>> produce this issue.
>>>>
>>>>>
>>>>> Which gadget driver were you using when you triggered this?
>>>>
>>>> mass storage driver. When host issues the setting config request, we
>>>> will get USB_GADGET_DELAYED_STATUS result from
>>>> set_config()--->fsg_set_alt(). Then the mass storage driver will issue
>>>> one thread to complete the status stage by ep0_queue() (this thread
>>>> may be running on another core), then if the thread issues ep0_queue()
>>>> too fast before we get the dwc->lock in dwc3_ep0_delegate_req() or
>>>> before we get into the STATUS stage, then we can not handle this
>>>> request for the delay STATUS stage in dwc3_gadget_ep0_queue().
>>>>
>>>>>
>>>>> 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.
>>>>>
>>>>> The reason I'm saying this is that other UDC drivers might have similar
>>>>> races already but they just haven't triggered.
>>>>
>>>> Yes, maybe.
>>>>
>>>>>
>>>>>> STATUS phase has been queued into list before we set 'dwc->delayed_status'
>>>>>> flag or entering 'EP0_STATUS_PHASE' phase, then we will miss the chance
>>>>>> to handle the STATUS phase. Thus we should check if the request for delay
>>>>>> STATUS phase has been enqueued when entering 'EP0_STATUS_PHASE' phase in
>>>>>> dwc3_ep0_xfernotready(), if so, we should handle it.
>>>>>>
>>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>>> ---
>>>>>>  drivers/usb/dwc3/ep0.c |   14 ++++++++++++++
>>>>>>  1 file changed, 14 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>>>>> index 9bb1f85..e689ced 100644
>>>>>> --- a/drivers/usb/dwc3/ep0.c
>>>>>> +++ b/drivers/usb/dwc3/ep0.c
>>>>>> @@ -1123,7 +1123,21 @@ static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
>>>>>>               dwc->ep0state = EP0_STATUS_PHASE;
>>>>>>
>>>>>>               if (dwc->delayed_status) {
>>>>>> +                     struct dwc3_ep *dep = dwc->eps[0];
>>>>>> +
>>>>>>                       WARN_ON_ONCE(event->endpoint_number != 1);
>>>>>> +                     /*
>>>>>> +                      * We should handle the delay STATUS phase here if the
>>>>>> +                      * request for handling delay STATUS has been queued
>>>>>> +                      * into the list.
>>>>>> +                      */
>>>>>> +                     if (!list_empty(&dep->pending_list)) {
>>>>>> +                             dwc->delayed_status = false;
>>>>>> +                             usb_gadget_set_state(&dwc->gadget,
>>>>>> +                                                  USB_STATE_CONFIGURED);
>>>>>
>>>>> Isn't this patch also changing the normal case when usb_ep_queue() comes
>>>>> later? I guess list_empty() protects against that...
>>>>
>>>> I think it will not change other cases, we only handle the delayed
>>>> status and I've tested it for a while and I did not find any problem.
>>>
>>> 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.

Thanks to explain, but seems it need lots of work to convert these
drivers, and I saw Alan had some concern about that. So I am not sure
how to convert these drivers which can meet your requirements, maybe
from adding one "wants_explicit_ctrl_phases"  flag in struct
usb_gadget as you suggested to start?

-- 
Baolin.wang
Best Regards

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

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
>>>>>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>>>> When handing the SETUP packet by composite_setup(), we will release the
>>>>>>> dwc->lock. If we get the 'USB_GADGET_DELAYED_STATUS' result from setup
>>>>>>> function, which means we need to delay handling the STATUS phase.
>>>>>>
>>>>>> this sentence needs a little work. Seems like it's missing some
>>>>>> information.
>>>>>>
>>>>>> anyway, I get that we release the lock but...
>>>>>>
>>>>>>> But during the lock release period, maybe the request for handling delay
>>>>>>
>>>>>> execution of ->setup() itself should be locked. I can see that it's only
>>>>>> locked for set_config() which is rather peculiar.
>>>>>>
>>>>>> What exact request you had when you triggered this? (Hint: dwc3
>>>>>> tracepoints print out ctrl request bytes). IIRC, only set_config() or
>>>>>> f->set_alt() can actually return USB_GADGET_DELAYED_STATUS.
>>>>>
>>>>> Yes, when host set configuration for mass storage driver, it can
>>>>> produce this issue.
>>>>>
>>>>>>
>>>>>> Which gadget driver were you using when you triggered this?
>>>>>
>>>>> mass storage driver. When host issues the setting config request, we
>>>>> will get USB_GADGET_DELAYED_STATUS result from
>>>>> set_config()--->fsg_set_alt(). Then the mass storage driver will issue
>>>>> one thread to complete the status stage by ep0_queue() (this thread
>>>>> may be running on another core), then if the thread issues ep0_queue()
>>>>> too fast before we get the dwc->lock in dwc3_ep0_delegate_req() or
>>>>> before we get into the STATUS stage, then we can not handle this
>>>>> request for the delay STATUS stage in dwc3_gadget_ep0_queue().
>>>>>
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> The reason I'm saying this is that other UDC drivers might have similar
>>>>>> races already but they just haven't triggered.
>>>>>
>>>>> Yes, maybe.
>>>>>
>>>>>>
>>>>>>> STATUS phase has been queued into list before we set 'dwc->delayed_status'
>>>>>>> flag or entering 'EP0_STATUS_PHASE' phase, then we will miss the chance
>>>>>>> to handle the STATUS phase. Thus we should check if the request for delay
>>>>>>> STATUS phase has been enqueued when entering 'EP0_STATUS_PHASE' phase in
>>>>>>> dwc3_ep0_xfernotready(), if so, we should handle it.
>>>>>>>
>>>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>>>> ---
>>>>>>>  drivers/usb/dwc3/ep0.c |   14 ++++++++++++++
>>>>>>>  1 file changed, 14 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>>>>>> index 9bb1f85..e689ced 100644
>>>>>>> --- a/drivers/usb/dwc3/ep0.c
>>>>>>> +++ b/drivers/usb/dwc3/ep0.c
>>>>>>> @@ -1123,7 +1123,21 @@ static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
>>>>>>>               dwc->ep0state = EP0_STATUS_PHASE;
>>>>>>>
>>>>>>>               if (dwc->delayed_status) {
>>>>>>> +                     struct dwc3_ep *dep = dwc->eps[0];
>>>>>>> +
>>>>>>>                       WARN_ON_ONCE(event->endpoint_number != 1);
>>>>>>> +                     /*
>>>>>>> +                      * We should handle the delay STATUS phase here if the
>>>>>>> +                      * request for handling delay STATUS has been queued
>>>>>>> +                      * into the list.
>>>>>>> +                      */
>>>>>>> +                     if (!list_empty(&dep->pending_list)) {
>>>>>>> +                             dwc->delayed_status = false;
>>>>>>> +                             usb_gadget_set_state(&dwc->gadget,
>>>>>>> +                                                  USB_STATE_CONFIGURED);
>>>>>>
>>>>>> Isn't this patch also changing the normal case when usb_ep_queue() comes
>>>>>> later? I guess list_empty() protects against that...
>>>>>
>>>>> I think it will not change other cases, we only handle the delayed
>>>>> status and I've tested it for a while and I did not find any problem.
>>>>
>>>> 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.
>
> Thanks to explain, but seems it need lots of work to convert these
> drivers, and I saw Alan had some concern about that. So I am not sure
> how to convert these drivers which can meet your requirements, maybe
> from adding one "wants_explicit_ctrl_phases"  flag in struct
> usb_gadget as you suggested to start?

yeah. Something like this:

patch 1: add the new flag and document it
patch 2: implement usb_ep_queue() for data and status phases conditional
	to that flag being set
patch 3: (e.g.) change dwc3 to rely on that behavior and pass the flag
	to usb_gadget.

this will be enough to actually test that the idea and implementation
works out. After that, just for_each_UDC_driver() port_patch_3(); Then
you add:

patch N - 1: remove legacy code and force behavior of
	wants_explicit_ctrl_phases
patch N: remove wants_explicit_ctrl_phases

something along these lines.

-- 
balbi

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

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

[Baolin Wang]

Hi,

On 17 January 2017 at 18:39, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>>>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>>>>> When handing the SETUP packet by composite_setup(), we will release the
>>>>>>>> dwc->lock. If we get the 'USB_GADGET_DELAYED_STATUS' result from setup
>>>>>>>> function, which means we need to delay handling the STATUS phase.
>>>>>>>
>>>>>>> this sentence needs a little work. Seems like it's missing some
>>>>>>> information.
>>>>>>>
>>>>>>> anyway, I get that we release the lock but...
>>>>>>>
>>>>>>>> But during the lock release period, maybe the request for handling delay
>>>>>>>
>>>>>>> execution of ->setup() itself should be locked. I can see that it's only
>>>>>>> locked for set_config() which is rather peculiar.
>>>>>>>
>>>>>>> What exact request you had when you triggered this? (Hint: dwc3
>>>>>>> tracepoints print out ctrl request bytes). IIRC, only set_config() or
>>>>>>> f->set_alt() can actually return USB_GADGET_DELAYED_STATUS.
>>>>>>
>>>>>> Yes, when host set configuration for mass storage driver, it can
>>>>>> produce this issue.
>>>>>>
>>>>>>>
>>>>>>> Which gadget driver were you using when you triggered this?
>>>>>>
>>>>>> mass storage driver. When host issues the setting config request, we
>>>>>> will get USB_GADGET_DELAYED_STATUS result from
>>>>>> set_config()--->fsg_set_alt(). Then the mass storage driver will issue
>>>>>> one thread to complete the status stage by ep0_queue() (this thread
>>>>>> may be running on another core), then if the thread issues ep0_queue()
>>>>>> too fast before we get the dwc->lock in dwc3_ep0_delegate_req() or
>>>>>> before we get into the STATUS stage, then we can not handle this
>>>>>> request for the delay STATUS stage in dwc3_gadget_ep0_queue().
>>>>>>
>>>>>>>
>>>>>>> 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.
>>>>>>>
>>>>>>> The reason I'm saying this is that other UDC drivers might have similar
>>>>>>> races already but they just haven't triggered.
>>>>>>
>>>>>> Yes, maybe.
>>>>>>
>>>>>>>
>>>>>>>> STATUS phase has been queued into list before we set 'dwc->delayed_status'
>>>>>>>> flag or entering 'EP0_STATUS_PHASE' phase, then we will miss the chance
>>>>>>>> to handle the STATUS phase. Thus we should check if the request for delay
>>>>>>>> STATUS phase has been enqueued when entering 'EP0_STATUS_PHASE' phase in
>>>>>>>> dwc3_ep0_xfernotready(), if so, we should handle it.
>>>>>>>>
>>>>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>>>>> ---
>>>>>>>>  drivers/usb/dwc3/ep0.c |   14 ++++++++++++++
>>>>>>>>  1 file changed, 14 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>>>>>>> index 9bb1f85..e689ced 100644
>>>>>>>> --- a/drivers/usb/dwc3/ep0.c
>>>>>>>> +++ b/drivers/usb/dwc3/ep0.c
>>>>>>>> @@ -1123,7 +1123,21 @@ static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
>>>>>>>>               dwc->ep0state = EP0_STATUS_PHASE;
>>>>>>>>
>>>>>>>>               if (dwc->delayed_status) {
>>>>>>>> +                     struct dwc3_ep *dep = dwc->eps[0];
>>>>>>>> +
>>>>>>>>                       WARN_ON_ONCE(event->endpoint_number != 1);
>>>>>>>> +                     /*
>>>>>>>> +                      * We should handle the delay STATUS phase here if the
>>>>>>>> +                      * request for handling delay STATUS has been queued
>>>>>>>> +                      * into the list.
>>>>>>>> +                      */
>>>>>>>> +                     if (!list_empty(&dep->pending_list)) {
>>>>>>>> +                             dwc->delayed_status = false;
>>>>>>>> +                             usb_gadget_set_state(&dwc->gadget,
>>>>>>>> +                                                  USB_STATE_CONFIGURED);
>>>>>>>
>>>>>>> Isn't this patch also changing the normal case when usb_ep_queue() comes
>>>>>>> later? I guess list_empty() protects against that...
>>>>>>
>>>>>> I think it will not change other cases, we only handle the delayed
>>>>>> status and I've tested it for a while and I did not find any problem.
>>>>>
>>>>> 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.
>>
>> Thanks to explain, but seems it need lots of work to convert these
>> drivers, and I saw Alan had some concern about that. So I am not sure
>> how to convert these drivers which can meet your requirements, maybe
>> from adding one "wants_explicit_ctrl_phases"  flag in struct
>> usb_gadget as you suggested to start?
>
> yeah. Something like this:
>
> patch 1: add the new flag and document it
> patch 2: implement usb_ep_queue() for data and status phases conditional
>         to that flag being set
> patch 3: (e.g.) change dwc3 to rely on that behavior and pass the flag
>         to usb_gadget.
>
> this will be enough to actually test that the idea and implementation
> works out. After that, just for_each_UDC_driver() port_patch_3(); Then
> you add:
>
> patch N - 1: remove legacy code and force behavior of
>         wants_explicit_ctrl_phases
> patch N: remove wants_explicit_ctrl_phases
>
> something along these lines.

Okay, I will try to do that. Thanks.

-- 
Baolin.wang
Best Regards

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

[Alan Stern]

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

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

[Felipe Balbi]

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


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

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

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

[Baolin Wang]

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

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

[Felipe Balbi]

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


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

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

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

[Baolin Wang]

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

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

[Baolin Wang]

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

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

[Alan Stern]

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

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

[Felipe Balbi]

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

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

[Alan Stern]

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

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

[Baolin Wang]

Hi,

On 28 February 2017 at 06:11, Alan Stern <stern@rowland.harvard.edu> wrote:
> 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.

Correct. What I mean is it will not queue one request for status phase
by usb_ep_queue() call, UDC driver will do that.

>
>> (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()
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.
(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
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().
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.

Your concern about an IN request or an OUT request with a data stage,
I agree with Felipe and we can identify. Thanks.
-- 
Baolin.wang
Best Regards

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

[Felipe Balbi]

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


Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
>> 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.

yeah, that's a given and was also mentioned in this thread somewhere.

>> 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.

right, we could use ->complete() for that.

> 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

we should also have code for this race since it would happen with
USB_GADGET_DELAYED_STATUS.

> queued but before the UDC driver has completed it.  (Of course, that's 
> already true now for the data request...)

right

-- 
balbi

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

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

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
>>> > 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.
>
> 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

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