[PATCH 0/4] usb: dwc3: gadget: More TRB handling cleanup

[PATCH 0/4] usb: dwc3: gadget: More TRB handling cleanup

[Thinh Nguyen]

This short series further improves the TRB handling logic and resolves some
more corner cases. The changes are verfied with the USB IF compliance
verification (running Chp.9, MSC, and UASP CV) and our verification
environment.


Thinh Nguyen (4):
  usb: dwc3: gadget: Look ahead when setting IOC
  usb: dwc3: gadget: Revise setting IOC when no TRB left
  usb: dwc3: gadget: Keep TRBs in request order
  usb: dwc3: gadget: Return early if no TRB update

 drivers/usb/dwc3/gadget.c | 93 +++++++++++++++++++++++----------------
 1 file changed, 55 insertions(+), 38 deletions(-)


base-commit: 8e9f3908b995a33443821dc3a977277f69a4adc3
-- 
2.28.0

[PATCH 1/4] usb: dwc3: gadget: Look ahead when setting IOC

[Thinh Nguyen]

Previously if we run out of TRBs for the last SG entry that requires
an extra TRB, we set interrupt-on-completion (IOC) bit to an already
prepared TRB (i.e. HWO=1). This logic is not clean, and it's not a
typical way to prepare TRB. Also, this prevents showing IOC setup in
tracepoint when __dwc3_prepare_one_trb() is executed. Instead, let's
look ahead when preparing TRB to know whether to set the IOC bit before
the last SG entry. This requires adding a new parameter "must_interrupt"
to dwc3_prepare_one_trb().

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/dwc3/gadget.c | 72 +++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 33 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 76c383eecfd3..0387b94b8f94 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -947,7 +947,7 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
 		dma_addr_t dma, unsigned int length, unsigned int chain,
 		unsigned int node, unsigned int stream_id,
 		unsigned int short_not_ok, unsigned int no_interrupt,
-		unsigned int is_last)
+		unsigned int is_last, bool must_interrupt)
 {
 	struct dwc3		*dwc = dep->dwc;
 	struct usb_gadget	*gadget = dwc->gadget;
@@ -1034,7 +1034,7 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
 			trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI;
 	}
 
-	if ((!no_interrupt && !chain) ||
+	if ((!no_interrupt && !chain) || must_interrupt ||
 			(dwc3_calc_trbs_left(dep) == 1))
 		trb->ctrl |= DWC3_TRB_CTRL_IOC;
 
@@ -1061,10 +1061,12 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
  * @chain: should this TRB be chained to the next?
  * @node: only for isochronous endpoints. First TRB needs different type.
  * @use_bounce_buffer: set to use bounce buffer
+ * @must_interrupt: set to interrupt on TRB completion
  */
 static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
 		struct dwc3_request *req, unsigned int trb_length,
-		unsigned int chain, unsigned int node, bool use_bounce_buffer)
+		unsigned int chain, unsigned int node, bool use_bounce_buffer,
+		bool must_interrupt)
 {
 	struct dwc3_trb		*trb;
 	dma_addr_t		dma;
@@ -1091,7 +1093,21 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
 	req->num_trbs++;
 
 	__dwc3_prepare_one_trb(dep, trb, dma, trb_length, chain, node,
-			stream_id, short_not_ok, no_interrupt, is_last);
+			stream_id, short_not_ok, no_interrupt, is_last,
+			must_interrupt);
+}
+
+static bool dwc3_needs_extra_trb(struct dwc3_ep *dep, struct dwc3_request *req)
+{
+	unsigned int maxp = usb_endpoint_maxp(dep->endpoint.desc);
+	unsigned int rem = req->request.length % maxp;
+
+	if ((req->request.length && req->request.zero && !rem &&
+			!usb_endpoint_xfer_isoc(dep->endpoint.desc)) ||
+			(!req->direction && rem))
+		return true;
+
+	return false;
 }
 
 /**
@@ -1111,9 +1127,7 @@ static int dwc3_prepare_last_sg(struct dwc3_ep *dep,
 	unsigned int rem = req->request.length % maxp;
 	unsigned int num_trbs = 1;
 
-	if ((req->request.length && req->request.zero && !rem &&
-			!usb_endpoint_xfer_isoc(dep->endpoint.desc)) ||
-			(!req->direction && rem))
+	if (dwc3_needs_extra_trb(dep, req))
 		num_trbs++;
 
 	if (dwc3_calc_trbs_left(dep) < num_trbs)
@@ -1124,13 +1138,13 @@ static int dwc3_prepare_last_sg(struct dwc3_ep *dep,
 	/* Prepare a normal TRB */
 	if (req->direction || req->request.length)
 		dwc3_prepare_one_trb(dep, req, entry_length,
-				req->needs_extra_trb, node, false);
+				req->needs_extra_trb, node, false, false);
 
 	/* Prepare extra TRBs for ZLP and MPS OUT transfer alignment */
 	if ((!req->direction && !req->request.length) || req->needs_extra_trb)
 		dwc3_prepare_one_trb(dep, req,
 				req->direction ? 0 : maxp - rem,
-				false, 1, true);
+				false, 1, true, false);
 
 	return num_trbs;
 }
@@ -1145,6 +1159,7 @@ static int dwc3_prepare_trbs_sg(struct dwc3_ep *dep,
 	unsigned int remaining = req->request.num_mapped_sgs
 		- req->num_queued_sgs;
 	unsigned int num_trbs = req->num_trbs;
+	bool needs_extra_trb = dwc3_needs_extra_trb(dep, req);
 
 	/*
 	 * If we resume preparing the request, then get the remaining length of
@@ -1155,6 +1170,7 @@ static int dwc3_prepare_trbs_sg(struct dwc3_ep *dep,
 
 	for_each_sg(sg, s, remaining, i) {
 		unsigned int trb_length;
+		bool must_interrupt = false;
 		bool last_sg = false;
 
 		trb_length = min_t(unsigned int, length, sg_dma_len(s));
@@ -1176,9 +1192,20 @@ static int dwc3_prepare_trbs_sg(struct dwc3_ep *dep,
 
 		if (last_sg) {
 			if (!dwc3_prepare_last_sg(dep, req, trb_length, i))
-				goto out;
+				break;
 		} else {
-			dwc3_prepare_one_trb(dep, req, trb_length, 1, i, false);
+			/*
+			 * Look ahead to check if we have enough TRBs for the
+			 * last SG entry. If not, set interrupt on this TRB to
+			 * resume preparing the last SG entry when more TRBs are
+			 * free.
+			 */
+			if (needs_extra_trb && dwc3_calc_trbs_left(dep) <= 2 &&
+					sg_dma_len(sg_next(s)) >= length)
+				must_interrupt = true;
+
+			dwc3_prepare_one_trb(dep, req, trb_length, 1, i, false,
+					must_interrupt);
 		}
 
 		/*
@@ -1203,31 +1230,10 @@ static int dwc3_prepare_trbs_sg(struct dwc3_ep *dep,
 			break;
 		}
 
-		if (!dwc3_calc_trbs_left(dep))
+		if (!dwc3_calc_trbs_left(dep) || must_interrupt)
 			break;
 	}
 
-	return req->num_trbs - num_trbs;
-
-out:
-	/*
-	 * If we run out of TRBs for MPS alignment setup, then set IOC on the
-	 * previous TRB to get notified for TRB completion to resume when more
-	 * TRBs are available.
-	 *
-	 * Note: normally we shouldn't update the TRB after the HWO bit is set.
-	 * However, the controller doesn't update its internal cache to handle
-	 * the newly prepared TRBs until UPDATE_TRANSFER or START_TRANSFER
-	 * command is executed. At this point, it doesn't happen yet, so we
-	 * should be fine modifying it here.
-	 */
-	if (i) {
-		struct dwc3_trb	*trb;
-
-		trb = dwc3_ep_prev_trb(dep, dep->trb_enqueue);
-		trb->ctrl |= DWC3_TRB_CTRL_IOC;
-	}
-
 	return req->num_trbs - num_trbs;
 }
 
-- 
2.28.0

[PATCH 2/4] usb: dwc3: gadget: Revise setting IOC when no TRB left

[Thinh Nguyen]

To keep the setting of interrupt-on-completion (IOC) when out of TRBs
consistent and easier to read, the caller of dwc3_prepare_one_trb()
will determine if the TRB must have IOC bit set. This also reduces the
number of times we need to call dwc3_calc_trbs_left(). Note that we only
care about setting IOC from insufficient number of TRBs for SG and not
linear requests (because we don't need to split linear requests).

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/dwc3/gadget.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 0387b94b8f94..327cd556e8db 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1034,8 +1034,7 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
 			trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI;
 	}
 
-	if ((!no_interrupt && !chain) || must_interrupt ||
-			(dwc3_calc_trbs_left(dep) == 1))
+	if ((!no_interrupt && !chain) || must_interrupt)
 		trb->ctrl |= DWC3_TRB_CTRL_IOC;
 
 	if (chain)
@@ -1169,6 +1168,7 @@ static int dwc3_prepare_trbs_sg(struct dwc3_ep *dep,
 		length -= sg_dma_len(s);
 
 	for_each_sg(sg, s, remaining, i) {
+		unsigned int num_trbs_left = dwc3_calc_trbs_left(dep);
 		unsigned int trb_length;
 		bool must_interrupt = false;
 		bool last_sg = false;
@@ -1187,7 +1187,7 @@ static int dwc3_prepare_trbs_sg(struct dwc3_ep *dep,
 		if ((i == remaining - 1) || !length)
 			last_sg = true;
 
-		if (!dwc3_calc_trbs_left(dep))
+		if (!num_trbs_left)
 			break;
 
 		if (last_sg) {
@@ -1196,12 +1196,13 @@ static int dwc3_prepare_trbs_sg(struct dwc3_ep *dep,
 		} else {
 			/*
 			 * Look ahead to check if we have enough TRBs for the
-			 * last SG entry. If not, set interrupt on this TRB to
-			 * resume preparing the last SG entry when more TRBs are
+			 * next SG entry. If not, set interrupt on this TRB to
+			 * resume preparing the next SG entry when more TRBs are
 			 * free.
 			 */
-			if (needs_extra_trb && dwc3_calc_trbs_left(dep) <= 2 &&
-					sg_dma_len(sg_next(s)) >= length)
+			if (num_trbs_left == 1 || (needs_extra_trb &&
+					num_trbs_left <= 2 &&
+					sg_dma_len(sg_next(s)) >= length))
 				must_interrupt = true;
 
 			dwc3_prepare_one_trb(dep, req, trb_length, 1, i, false,
@@ -1230,7 +1231,7 @@ static int dwc3_prepare_trbs_sg(struct dwc3_ep *dep,
 			break;
 		}
 
-		if (!dwc3_calc_trbs_left(dep) || must_interrupt)
+		if (must_interrupt)
 			break;
 	}
 
-- 
2.28.0

[PATCH 3/4] usb: dwc3: gadget: Keep TRBs in request order

[Thinh Nguyen]

If we couldn't finish preparing all the TRBs of a request, don't prepare
the next request. Otherwise, the TRBs order will be mixed up and the
controller will process the wrong TRB. This is a corner case where
there's not enough TRBs for a request that needs the extra TRB but
there's still 1 available TRB in the pool.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/dwc3/gadget.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 327cd556e8db..ff924656f690 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1274,7 +1274,7 @@ static int dwc3_prepare_trbs(struct dwc3_ep *dep)
 	list_for_each_entry(req, &dep->started_list, list) {
 		if (req->num_pending_sgs > 0) {
 			ret = dwc3_prepare_trbs_sg(dep, req);
-			if (!ret)
+			if (!ret || req->num_pending_sgs)
 				return ret;
 		}
 
@@ -1303,10 +1303,13 @@ static int dwc3_prepare_trbs(struct dwc3_ep *dep)
 		req->num_queued_sgs	= 0;
 		req->num_pending_sgs	= req->request.num_mapped_sgs;
 
-		if (req->num_pending_sgs > 0)
+		if (req->num_pending_sgs > 0) {
 			ret = dwc3_prepare_trbs_sg(dep, req);
-		else
+			if (req->num_pending_sgs)
+				return ret;
+		} else {
 			ret = dwc3_prepare_trbs_linear(dep, req);
+		}
 
 		if (!ret || !dwc3_calc_trbs_left(dep))
 			return ret;
-- 
2.28.0

[PATCH 4/4] usb: dwc3: gadget: Return early if no TRB update

[Thinh Nguyen]

If the transfer had already started and there's no TRB to update, then
there's no need to go through __dwc3_gadget_kick_transfer(). There is
no problem reissuing UPDATE_TRANSFER command. This change just saves
the driver from doing a few operations. This happens when we run out of
TRB and function driver still queues for more requests.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/dwc3/gadget.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index ff924656f690..da1f2ad2ad90 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1347,6 +1347,13 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
 
 	starting = !(dep->flags & DWC3_EP_TRANSFER_STARTED);
 
+	/*
+	 * If there's no new TRB prepared and we don't need to restart a
+	 * transfer, there's no need to update the transfer.
+	 */
+	if (!ret && !starting)
+		return ret;
+
 	req = next_request(&dep->started_list);
 	if (!req) {
 		dep->flags |= DWC3_EP_PENDING_REQUEST;
-- 
2.28.0