* [PATCH] usb: dwc3: EP clear halt leading to incorrect submission of delayed_status
@ 2022-04-07 1:53 Wesley Cheng
2022-04-11 15:48 ` Pavan Kondeti
0 siblings, 1 reply; 3+ messages in thread
From: Wesley Cheng @ 2022-04-07 1:53 UTC (permalink / raw)
To: balbi, gregkh; +Cc: linux-usb, linux-kernel, Wesley Cheng
The usb_ep_clear_halt() API can be called from the function driver, and
translates to dwc3_gadget_ep_set_halt(). This routine is shared with when
the host issues a clear feature ENDPOINT_HALT, and is differentiated by the
protocol argument. If the following sequence occurs, there can be a
situation where the delayed_status flag is improperly cleared for the wrong
SETUP transaction:
1. Vendor specific control transfer returns USB_GADGET_DELAYED_STATUS.
2. DWC3 gadget sets dwc->delayed_status to '1'.
3. Another function driver issues a usb_ep_clear_halt() call.
4. DWC3 gadget issues dwc3_stop_active_transfer() and sets
DWC3_EP_PENDING_CLEAR_STALL.
5. EP command complete interrupt triggers for the end transfer, and
dwc3_ep0_send_delayed_status() is allowed to run, as delayed_status
is '1' due to step#1.
6. STATUS phase is sent, and delayed_status is cleared.
7. Vendor specific control transfer is finished being handled, and issues
usb_composite_setup_continue(). This results in queuing of a data
phase.
Cache the protocol flag so that DWC3 gadget is aware of when the clear halt
is due to a SETUP request from the host versus when it is sourced from a
function driver. This allows for the EP command complete interrupt to know
if it needs to issue a delayed status phase.
Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
drivers/usb/dwc3/core.h | 1 +
drivers/usb/dwc3/ep0.c | 1 +
drivers/usb/dwc3/gadget.c | 3 ++-
3 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 5c9d467195a6..55f98485c54c 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1272,6 +1272,7 @@ struct dwc3 {
unsigned connected:1;
unsigned softconnect:1;
unsigned delayed_status:1;
+ unsigned clear_stall_protocol:1;
unsigned ep0_bounced:1;
unsigned ep0_expect_in:1;
unsigned has_hibernation:1;
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 1064be5518f6..aa8476da222d 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -1080,6 +1080,7 @@ void dwc3_ep0_send_delayed_status(struct dwc3 *dwc)
unsigned int direction = !dwc->ep0_expect_in;
dwc->delayed_status = false;
+ dwc->clear_stall_protocol = 0;
if (dwc->ep0state != EP0_STATUS_PHASE)
return;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index ab725d2262d6..c427ddae016f 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2152,6 +2152,7 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
if (dep->flags & DWC3_EP_END_TRANSFER_PENDING ||
(dep->flags & DWC3_EP_DELAY_STOP)) {
dep->flags |= DWC3_EP_PENDING_CLEAR_STALL;
+ dwc->clear_stall_protocol = protocol;
return 0;
}
@@ -3483,7 +3484,7 @@ static void dwc3_gadget_endpoint_command_complete(struct dwc3_ep *dep,
}
dep->flags &= ~(DWC3_EP_STALL | DWC3_EP_WEDGE);
- if (dwc->delayed_status)
+ if (dwc->clear_stall_protocol)
dwc3_ep0_send_delayed_status(dwc);
}
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] usb: dwc3: EP clear halt leading to incorrect submission of delayed_status
2022-04-07 1:53 [PATCH] usb: dwc3: EP clear halt leading to incorrect submission of delayed_status Wesley Cheng
@ 2022-04-11 15:48 ` Pavan Kondeti
2022-04-14 21:44 ` Wesley Cheng
0 siblings, 1 reply; 3+ messages in thread
From: Pavan Kondeti @ 2022-04-11 15:48 UTC (permalink / raw)
To: Wesley Cheng; +Cc: balbi, gregkh, linux-usb, linux-kernel
Hi Wesley,
On Wed, Apr 06, 2022 at 06:53:36PM -0700, Wesley Cheng wrote:
> The usb_ep_clear_halt() API can be called from the function driver, and
> translates to dwc3_gadget_ep_set_halt(). This routine is shared with when
> the host issues a clear feature ENDPOINT_HALT, and is differentiated by the
> protocol argument. If the following sequence occurs, there can be a
> situation where the delayed_status flag is improperly cleared for the wrong
> SETUP transaction:
>
> 1. Vendor specific control transfer returns USB_GADGET_DELAYED_STATUS.
> 2. DWC3 gadget sets dwc->delayed_status to '1'.
> 3. Another function driver issues a usb_ep_clear_halt() call.
> 4. DWC3 gadget issues dwc3_stop_active_transfer() and sets
> DWC3_EP_PENDING_CLEAR_STALL.
> 5. EP command complete interrupt triggers for the end transfer, and
> dwc3_ep0_send_delayed_status() is allowed to run, as delayed_status
> is '1' due to step#1.
> 6. STATUS phase is sent, and delayed_status is cleared.
> 7. Vendor specific control transfer is finished being handled, and issues
> usb_composite_setup_continue(). This results in queuing of a data
> phase.
>
> Cache the protocol flag so that DWC3 gadget is aware of when the clear halt
> is due to a SETUP request from the host versus when it is sourced from a
> function driver. This allows for the EP command complete interrupt to know
> if it needs to issue a delayed status phase.
>
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
> drivers/usb/dwc3/core.h | 1 +
> drivers/usb/dwc3/ep0.c | 1 +
> drivers/usb/dwc3/gadget.c | 3 ++-
> 3 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 5c9d467195a6..55f98485c54c 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1272,6 +1272,7 @@ struct dwc3 {
> unsigned connected:1;
> unsigned softconnect:1;
> unsigned delayed_status:1;
> + unsigned clear_stall_protocol:1;
> unsigned ep0_bounced:1;
> unsigned ep0_expect_in:1;
> unsigned has_hibernation:1;
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 1064be5518f6..aa8476da222d 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -1080,6 +1080,7 @@ void dwc3_ep0_send_delayed_status(struct dwc3 *dwc)
> unsigned int direction = !dwc->ep0_expect_in;
>
> dwc->delayed_status = false;
> + dwc->clear_stall_protocol = 0;
>
> if (dwc->ep0state != EP0_STATUS_PHASE)
> return;
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index ab725d2262d6..c427ddae016f 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2152,6 +2152,7 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
> if (dep->flags & DWC3_EP_END_TRANSFER_PENDING ||
> (dep->flags & DWC3_EP_DELAY_STOP)) {
> dep->flags |= DWC3_EP_PENDING_CLEAR_STALL;
> + dwc->clear_stall_protocol = protocol;
> return 0;
> }
>
> @@ -3483,7 +3484,7 @@ static void dwc3_gadget_endpoint_command_complete(struct dwc3_ep *dep,
> }
>
> dep->flags &= ~(DWC3_EP_STALL | DWC3_EP_WEDGE);
> - if (dwc->delayed_status)
> + if (dwc->clear_stall_protocol)
> dwc3_ep0_send_delayed_status(dwc);
> }
>
Is it safe to maintain clear_stall_protocol per dwc3 instance? What if
CLEAR_FEATURE(halt_endpoint) and usb_ep_clear_halt() are interleaved and
We come here as part of usb_ep_clear_halt()'s endpoint command complete.
We may simply send the delayed status corresponding to the protocol clear
stall.
We can still maintain a global flag if we cache endpoint number in it so
that we can cross check against the endpoint for which completion received.
Thanks,
Pavan
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] usb: dwc3: EP clear halt leading to incorrect submission of delayed_status
2022-04-11 15:48 ` Pavan Kondeti
@ 2022-04-14 21:44 ` Wesley Cheng
0 siblings, 0 replies; 3+ messages in thread
From: Wesley Cheng @ 2022-04-14 21:44 UTC (permalink / raw)
To: Pavan Kondeti; +Cc: balbi, gregkh, linux-usb, linux-kernel
Hi Pavan,
On 4/11/2022 8:48 AM, Pavan Kondeti wrote:
> Hi Wesley,
>
> On Wed, Apr 06, 2022 at 06:53:36PM -0700, Wesley Cheng wrote:
>> The usb_ep_clear_halt() API can be called from the function driver, and
>> translates to dwc3_gadget_ep_set_halt(). This routine is shared with when
>> the host issues a clear feature ENDPOINT_HALT, and is differentiated by the
>> protocol argument. If the following sequence occurs, there can be a
>> situation where the delayed_status flag is improperly cleared for the wrong
>> SETUP transaction:
>>
>> 1. Vendor specific control transfer returns USB_GADGET_DELAYED_STATUS.
>> 2. DWC3 gadget sets dwc->delayed_status to '1'.
>> 3. Another function driver issues a usb_ep_clear_halt() call.
>> 4. DWC3 gadget issues dwc3_stop_active_transfer() and sets
>> DWC3_EP_PENDING_CLEAR_STALL.
>> 5. EP command complete interrupt triggers for the end transfer, and
>> dwc3_ep0_send_delayed_status() is allowed to run, as delayed_status
>> is '1' due to step#1.
>> 6. STATUS phase is sent, and delayed_status is cleared.
>> 7. Vendor specific control transfer is finished being handled, and issues
>> usb_composite_setup_continue(). This results in queuing of a data
>> phase.
>>
>> Cache the protocol flag so that DWC3 gadget is aware of when the clear halt
>> is due to a SETUP request from the host versus when it is sourced from a
>> function driver. This allows for the EP command complete interrupt to know
>> if it needs to issue a delayed status phase.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>> drivers/usb/dwc3/core.h | 1 +
>> drivers/usb/dwc3/ep0.c | 1 +
>> drivers/usb/dwc3/gadget.c | 3 ++-
>> 3 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 5c9d467195a6..55f98485c54c 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -1272,6 +1272,7 @@ struct dwc3 {
>> unsigned connected:1;
>> unsigned softconnect:1;
>> unsigned delayed_status:1;
>> + unsigned clear_stall_protocol:1;
>> unsigned ep0_bounced:1;
>> unsigned ep0_expect_in:1;
>> unsigned has_hibernation:1;
>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>> index 1064be5518f6..aa8476da222d 100644
>> --- a/drivers/usb/dwc3/ep0.c
>> +++ b/drivers/usb/dwc3/ep0.c
>> @@ -1080,6 +1080,7 @@ void dwc3_ep0_send_delayed_status(struct dwc3 *dwc)
>> unsigned int direction = !dwc->ep0_expect_in;
>>
>> dwc->delayed_status = false;
>> + dwc->clear_stall_protocol = 0;
>>
>> if (dwc->ep0state != EP0_STATUS_PHASE)
>> return;
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index ab725d2262d6..c427ddae016f 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2152,6 +2152,7 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
>> if (dep->flags & DWC3_EP_END_TRANSFER_PENDING ||
>> (dep->flags & DWC3_EP_DELAY_STOP)) {
>> dep->flags |= DWC3_EP_PENDING_CLEAR_STALL;
>> + dwc->clear_stall_protocol = protocol;
>> return 0;
>> }
>>
>> @@ -3483,7 +3484,7 @@ static void dwc3_gadget_endpoint_command_complete(struct dwc3_ep *dep,
>> }
>>
>> dep->flags &= ~(DWC3_EP_STALL | DWC3_EP_WEDGE);
>> - if (dwc->delayed_status)
>> + if (dwc->clear_stall_protocol)
>> dwc3_ep0_send_delayed_status(dwc);
>> }
>>
>
> Is it safe to maintain clear_stall_protocol per dwc3 instance? What if
> CLEAR_FEATURE(halt_endpoint) and usb_ep_clear_halt() are interleaved and
> We come here as part of usb_ep_clear_halt()'s endpoint command complete.
> We may simply send the delayed status corresponding to the protocol clear
> stall.
>
> We can still maintain a global flag if we cache endpoint number in it so
> that we can cross check against the endpoint for which completion received.
>
> Thanks,
> Pavan
>
Thanks for the comments/feedback. I agree with what you mentioned, and
will fix that potential condition. Will resubmit a new rev, after some
testing.
Thanks
Wesley Cheng
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-04-14 21:45 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-07 1:53 [PATCH] usb: dwc3: EP clear halt leading to incorrect submission of delayed_status Wesley Cheng
2022-04-11 15:48 ` Pavan Kondeti
2022-04-14 21:44 ` Wesley Cheng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.