linux-kernel - Re: [PATCH] usb: dwc3: gadget: Add 100uS delay after end transfer command without IOC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230228021925.j5bquwnwuvog3hx6@synopsys.com>
Date:   Tue, 28 Feb 2023 02:19:39 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     Wesley Cheng <quic_wcheng@...cinc.com>
CC:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "quic_jackp@...cinc.com" <quic_jackp@...cinc.com>
Subject: Re: [PATCH] usb: dwc3: gadget: Add 100uS delay after end transfer
 command without IOC

On Mon, Feb 27, 2023, Wesley Cheng wrote:
> Previously, there was a 100uS delay inserted after issuing an end transfer
> command for specific controller revisions.  This was due to the fact that
> there was a GUCTL2 bit field which enabled synchronous completion of the
> end transfer command once the CMDACT bit was cleared in the DEPCMD
> register.  Since this bit does not exist for all controller revisions, add
> the delay back in.
> 
> An issue was seen where the USB request buffer was unmapped while the DWC3
> controller was still accessing the TRB.  However, it was confirmed that the
> end transfer command was successfully submitted. (no end transfer timeout)

Currently we only check for command active, not completion on teardown.

> In situations, such as dwc3_gadget_soft_disconnect() and
> __dwc3_gadget_ep_disable(), the dwc3_remove_request() is utilized, which
> will issue the end transfer command, and follow up with
> dwc3_gadget_giveback().  At least for the USB ep disable path, it is
> required for any pending and started requests to be completed and returned
> to the function driver in the same context of the disable call.  Without
> the GUCTL2 bit, it is not ensured that the end transfer is completed before
> the buffers are unmapped.
> 
> Signed-off-by: Wesley Cheng <quic_wcheng@...cinc.com>

This is expected. We're supposed to make sure the End Transfer command
complete before accessing the request. Usually on device/endpoint
teardown, the gadget drivers don't access the stale/incomplete requests
with -ESHUTDOWN status. There will be problems if we do, and we haven't
fixed that.

Adding 100uS may not apply for every device, and we don't need to do
that for every End Transfer command. Can you try this untested diff
instead:


diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 30408bafe64e..5ae5ff4c8858 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1962,6 +1962,34 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
 	return DWC3_DSTS_SOFFN(reg);
 }
 
+static int dwc3_poll_ep_completion(struct dwc3_ep *dep)
+{
+	if (!list_empty(&dep->started_list)) {
+		struct dwc3_request *req;
+		int timeout = 500;
+
+		req = next_request(&dep->started_list);
+		while(--timeout) {
+			/*
+			 * Note: don't check the last enqueued TRB in case
+			 * of short transfer. Check first TRB of a started
+			 * request instead.
+			 */
+			if (!(req->trb->ctrl & DWC3_TRB_CTRL_HWO))
+				break;
+
+			udelay(2);
+		}
+		if (!timeout) {
+			dev_warn(dep->dwc->dev,
+				 "%s is still in-progress\n", dep->name);
+			return -ETIMEDOUT;
+		}
+	}
+
+	return 0;
+}
+
 /**
  * __dwc3_stop_active_transfer - stop the current active transfer
  * @dep: isoc endpoint
@@ -2003,10 +2031,12 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int
 	WARN_ON_ONCE(ret);
 	dep->resource_index = 0;
 
-	if (!interrupt)
+	if (!interrupt) {
+		ret = dwc3_poll_ep_completion(dep);
 		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
-	else if (!ret)
+	} else if (!ret) {
 		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
+	}
 
 	dep->flags &= ~DWC3_EP_DELAY_STOP;
 	return ret;


Thanks,
Thinh