Re: [PATCH] usb: dwc3: EP clear halt leading to incorrect submission of delayed_status

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxx>
---
  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



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux