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

Groups > linux.kernel > #1558868 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase Baolin Wang <baolin.wang@linaro.org> - 2017-01-14 09:50 +0100
    Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase Felipe Balbi <balbi@kernel.org> - 2017-01-16 12:00 +0100
      Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for  handling delay STATUS phase Baolin Wang <baolin.wang@linaro.org> - 2017-01-16 12:30 +0100
        Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase Felipe Balbi <balbi@kernel.org> - 2017-01-16 12:40 +0100
          Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for  handling delay STATUS phase Baolin Wang <baolin.wang@linaro.org> - 2017-01-16 13:10 +0100
            Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase Felipe Balbi <balbi@kernel.org> - 2017-01-16 13:10 +0100
              Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for  handling delay STATUS phase Alan Stern <stern@rowland.harvard.edu> - 2017-01-16 19:00 +0100

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

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

[toc] | [next] | [standalone]

#1559633

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

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

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

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

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

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

#1559657

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

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

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

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

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

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

#1559674

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

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

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

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web