[PATCH v3 0/4] usb: dwc3: gadget: Fix TRB preparation

[PATCH v3 0/4] usb: dwc3: gadget: Fix TRB preparation

[Thinh Nguyen]

There are a few issues in DWC3 driver when preparing for TRB.
The driver needs to account the following:

* MPS alignment for ZLP OUT direction
* Extra TRBs when checking for available TRBs
* SG entries size > request length

Along with these fixes, there are some cleanup/refactoring patches in this
series.

Changes in v3:
- Rebase on Felipe's testing/next branch with the remaining patches of this
  series

Changes in v2:
- Update remaining length when resume request
- Remove unused variables
- Add a missing "return 0" for dwc3_prepare_trbs()
- Update doc indicating dwc3_prepare_trbs() can return other -errno


Thinh Nguyen (4):
  usb: dwc3: gadget: Refactor preparing TRBs
  usb: dwc3: gadget: Account for extra TRB
  usb: dwc3: gadget: Rename misleading function names
  usb: dwc3: ep0: Skip ZLP setup for OUT

 drivers/usb/dwc3/ep0.c    |   2 +-
 drivers/usb/dwc3/gadget.c | 246 +++++++++++++++++---------------------
 2 files changed, 112 insertions(+), 136 deletions(-)


base-commit: 8bc2602e7a402664131d79251d59b262b896ca37
-- 
2.28.0

[PATCH v3 1/4] usb: dwc3: gadget: Refactor preparing TRBs

[Thinh Nguyen]

There are a lot of common codes for preparing SG and linear TRBs. Let's
refactor them for easier read. No functional change here.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
Changes in v3:
- None
Changes in v2:
- Remove unused variables rem and maxp

 drivers/usb/dwc3/gadget.c | 178 ++++++++++++++------------------------
 1 file changed, 67 insertions(+), 111 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 13a187efb5b9..09810ca537bc 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1091,6 +1091,65 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
 			stream_id, short_not_ok, no_interrupt, is_last);
 }
 
+/**
+ * dwc3_prepare_last_sg - prepare TRBs for the last SG entry
+ * @dep: The endpoint that the request belongs to
+ * @req: The request to prepare
+ * @entry_length: The last SG entry size
+ * @node: Indicates whether this is not the first entry (for isoc only)
+ */
+static void dwc3_prepare_last_sg(struct dwc3_ep *dep,
+				 struct dwc3_request *req,
+				 unsigned int entry_length,
+				 unsigned int node)
+{
+	unsigned int maxp = usb_endpoint_maxp(dep->endpoint.desc);
+	unsigned int rem = req->request.length % maxp;
+	unsigned int num_extra_trbs = 0;
+	unsigned int i;
+	bool do_zlp = false;
+
+	if (!usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
+	    req->request.zero && req->request.length && !rem) {
+		num_extra_trbs++;
+		do_zlp = true;
+	}
+
+	if (!req->direction && (!req->request.length || rem || do_zlp))
+		num_extra_trbs++;
+
+	if (num_extra_trbs > 0)
+		req->needs_extra_trb = true;
+
+	/* Prepare a normal TRB */
+	dwc3_prepare_one_trb(dep, req, entry_length, req->needs_extra_trb, node);
+
+	/* Prepare extra TRBs for ZLP and MPS OUT transfer alignment */
+	for (i = 0; i < num_extra_trbs; i++) {
+		struct dwc3 *dwc = dep->dwc;
+		struct dwc3_trb	*trb;
+		unsigned int extra_trb_length;
+		bool chain = true;
+
+		if (do_zlp && !i)
+			extra_trb_length = 0;
+		else
+			extra_trb_length = maxp - rem;
+
+		if (i == num_extra_trbs - 1)
+			chain = false;
+
+		trb = &dep->trb_pool[dep->trb_enqueue];
+		req->num_trbs++;
+		__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr,
+				       extra_trb_length, chain, 1,
+				       req->request.stream_id,
+				       req->request.short_not_ok,
+				       req->request.no_interrupt,
+				       req->request.is_last);
+	}
+}
+
 static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
 		struct dwc3_request *req)
 {
@@ -1109,10 +1168,8 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
 		length -= sg_dma_len(s);
 
 	for_each_sg(sg, s, remaining, i) {
-		unsigned int maxp = usb_endpoint_maxp(dep->endpoint.desc);
-		unsigned int rem = length % maxp;
 		unsigned int trb_length;
-		unsigned int chain = true;
+		bool last_sg = false;
 
 		trb_length = min_t(unsigned int, length, sg_dma_len(s));
 
@@ -1126,60 +1183,12 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
 		 * mapped sg.
 		 */
 		if ((i == remaining - 1) || !length)
-			chain = false;
+			last_sg = true;
 
-		if (rem && usb_endpoint_dir_out(dep->endpoint.desc) && !chain) {
-			struct dwc3	*dwc = dep->dwc;
-			struct dwc3_trb	*trb;
-
-			req->needs_extra_trb = true;
-
-			/* prepare normal TRB */
-			dwc3_prepare_one_trb(dep, req, trb_length, true, i);
-
-			/* Now prepare one extra TRB to align transfer size */
-			trb = &dep->trb_pool[dep->trb_enqueue];
-			req->num_trbs++;
-			__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr,
-					maxp - rem, false, 1,
-					req->request.stream_id,
-					req->request.short_not_ok,
-					req->request.no_interrupt,
-					req->request.is_last);
-		} else if (req->request.zero && req->request.length &&
-			   !usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
-			   !rem && !chain) {
-			struct dwc3	*dwc = dep->dwc;
-			struct dwc3_trb	*trb;
-
-			req->needs_extra_trb = true;
-
-			/* Prepare normal TRB */
-			dwc3_prepare_one_trb(dep, req, trb_length, true, i);
-
-			/* Prepare one extra TRB to handle ZLP */
-			trb = &dep->trb_pool[dep->trb_enqueue];
-			req->num_trbs++;
-			__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, 0,
-					       !req->direction, 1,
-					       req->request.stream_id,
-					       req->request.short_not_ok,
-					       req->request.no_interrupt,
-					       req->request.is_last);
-
-			/* Prepare one more TRB to handle MPS alignment */
-			if (!req->direction) {
-				trb = &dep->trb_pool[dep->trb_enqueue];
-				req->num_trbs++;
-				__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, maxp,
-						       false, 1, req->request.stream_id,
-						       req->request.short_not_ok,
-						       req->request.no_interrupt,
-						       req->request.is_last);
-			}
-		} else {
-			dwc3_prepare_one_trb(dep, req, trb_length, chain, i);
-		}
+		if (last_sg)
+			dwc3_prepare_last_sg(dep, req, trb_length, i);
+		else
+			dwc3_prepare_one_trb(dep, req, trb_length, 1, i);
 
 		/*
 		 * There can be a situation where all sgs in sglist are not
@@ -1188,7 +1197,7 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
 		 * we have free trbs we can continue queuing from where we
 		 * previously stopped
 		 */
-		if (chain)
+		if (!last_sg)
 			req->start_sg = sg_next(s);
 
 		req->num_queued_sgs++;
@@ -1211,60 +1220,7 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
 static void dwc3_prepare_one_trb_linear(struct dwc3_ep *dep,
 		struct dwc3_request *req)
 {
-	unsigned int length = req->request.length;
-	unsigned int maxp = usb_endpoint_maxp(dep->endpoint.desc);
-	unsigned int rem = length % maxp;
-
-	if ((!length || rem) && usb_endpoint_dir_out(dep->endpoint.desc)) {
-		struct dwc3	*dwc = dep->dwc;
-		struct dwc3_trb	*trb;
-
-		req->needs_extra_trb = true;
-
-		/* prepare normal TRB */
-		dwc3_prepare_one_trb(dep, req, length, true, 0);
-
-		/* Now prepare one extra TRB to align transfer size */
-		trb = &dep->trb_pool[dep->trb_enqueue];
-		req->num_trbs++;
-		__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, maxp - rem,
-				false, 1, req->request.stream_id,
-				req->request.short_not_ok,
-				req->request.no_interrupt,
-				req->request.is_last);
-	} else if (req->request.zero && req->request.length &&
-		   !usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
-		   (IS_ALIGNED(req->request.length, maxp))) {
-		struct dwc3	*dwc = dep->dwc;
-		struct dwc3_trb	*trb;
-
-		req->needs_extra_trb = true;
-
-		/* prepare normal TRB */
-		dwc3_prepare_one_trb(dep, req, length, true, 0);
-
-		/* Prepare one extra TRB to handle ZLP */
-		trb = &dep->trb_pool[dep->trb_enqueue];
-		req->num_trbs++;
-		__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, 0,
-				!req->direction, 1, req->request.stream_id,
-				req->request.short_not_ok,
-				req->request.no_interrupt,
-				req->request.is_last);
-
-		/* Prepare one more TRB to handle MPS alignment for OUT */
-		if (!req->direction) {
-			trb = &dep->trb_pool[dep->trb_enqueue];
-			req->num_trbs++;
-			__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, maxp,
-					       false, 1, req->request.stream_id,
-					       req->request.short_not_ok,
-					       req->request.no_interrupt,
-					       req->request.is_last);
-		}
-	} else {
-		dwc3_prepare_one_trb(dep, req, length, false, 0);
-	}
+	dwc3_prepare_last_sg(dep, req, req->request.length, 0);
 }
 
 /*
-- 
2.28.0

Re: [PATCH v3 1/4] usb: dwc3: gadget: Refactor preparing TRBs

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1402 bytes --]

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:

> There are a lot of common codes for preparing SG and linear TRBs. Let's
> refactor them for easier read. No functional change here.
>
> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>

email here doesn't match author's ;-)

> @@ -1091,6 +1091,65 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
>  			stream_id, short_not_ok, no_interrupt, is_last);
>  }
>  
> +/**
> + * dwc3_prepare_last_sg - prepare TRBs for the last SG entry
> + * @dep: The endpoint that the request belongs to
> + * @req: The request to prepare
> + * @entry_length: The last SG entry size
> + * @node: Indicates whether this is not the first entry (for isoc only)
> + */
> +static void dwc3_prepare_last_sg(struct dwc3_ep *dep,
> +				 struct dwc3_request *req,
> +				 unsigned int entry_length,
> +				 unsigned int node)

I think some of these can be combined into a single line. Also, why so
far to the right on the line breaks? Could you keep the existing style?

> +{
> +	unsigned int maxp = usb_endpoint_maxp(dep->endpoint.desc);
> +	unsigned int rem = req->request.length % maxp;
> +	unsigned int num_extra_trbs = 0;
> +	unsigned int i;
> +	bool do_zlp = false;
> +
> +	if (!usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
> +	    req->request.zero && req->request.length && !rem) {

spaces for indentation?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH v3 1/4] usb: dwc3: gadget: Refactor preparing TRBs

[Thinh Nguyen]

Felipe Balbi wrote:
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>
>> There are a lot of common codes for preparing SG and linear TRBs. Let's
>> refactor them for easier read. No functional change here.
>>
>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> email here doesn't match author's ;-)

Whenever I send email outside of our organization, the "From" header
will be modified to an alias email address. I'm working with our IT to
see if we can do something about it. This may take some time. I hope
that this doesn't cause too much issue accepting my patches in the
meanwhile.

>
>> @@ -1091,6 +1091,65 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
>>  			stream_id, short_not_ok, no_interrupt, is_last);
>>  }
>>  
>> +/**
>> + * dwc3_prepare_last_sg - prepare TRBs for the last SG entry
>> + * @dep: The endpoint that the request belongs to
>> + * @req: The request to prepare
>> + * @entry_length: The last SG entry size
>> + * @node: Indicates whether this is not the first entry (for isoc only)
>> + */
>> +static void dwc3_prepare_last_sg(struct dwc3_ep *dep,
>> +				 struct dwc3_request *req,
>> +				 unsigned int entry_length,
>> +				 unsigned int node)
> I think some of these can be combined into a single line. Also, why so
> far to the right on the line breaks? Could you keep the existing style?


It's because of open parenthesis descendant alignment style. I'll change
to not do that.

>> +{
>> +	unsigned int maxp = usb_endpoint_maxp(dep->endpoint.desc);
>> +	unsigned int rem = req->request.length % maxp;
>> +	unsigned int num_extra_trbs = 0;
>> +	unsigned int i;
>> +	bool do_zlp = false;
>> +
>> +	if (!usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
>> +	    req->request.zero && req->request.length && !rem) {
> spaces for indentation?
>

Same comment as above.

Thanks,
Thinh

[PATCH v3 2/4] usb: dwc3: gadget: Account for extra TRB

[Thinh Nguyen]

When checking for how many TRB remaining, make sure to account for extra
TRBs for ZLP or MPS alignment transfers. Since the dwc3_prepare_trb*
functions should know if we need the extra TRBs, make those functions
return a status code -EAGAIN if there isn't enough TRB. Check against
those status when preparing TRB instead.

Fixes: c6267a51639b ("usb: dwc3: gadget: align transfers to wMaxPacketSize")
Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
Changes in v3:
- None
Changes in v2:
- Add a missing "return 0" for dwc3_prepare_trbs()
- Update doc indicate dwc3_prepare_trbs() can return other -errno

 drivers/usb/dwc3/gadget.c | 82 ++++++++++++++++++++++++---------------
 1 file changed, 51 insertions(+), 31 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 09810ca537bc..7c95ac3186be 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1060,8 +1060,10 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
  * @trb_length: buffer size of the TRB
  * @chain: should this TRB be chained to the next?
  * @node: only for isochronous endpoints. First TRB needs different type.
+ *
+ * Return 0 on success or -EAGAIN if there is not enough TRBs.
  */
-static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
+static int dwc3_prepare_one_trb(struct dwc3_ep *dep,
 		struct dwc3_request *req, unsigned int trb_length,
 		unsigned int chain, unsigned int node)
 {
@@ -1072,6 +1074,9 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
 	unsigned int		no_interrupt = req->request.no_interrupt;
 	unsigned int		is_last = req->request.is_last;
 
+	if (!dwc3_calc_trbs_left(dep))
+		return -EAGAIN;
+
 	if (req->request.num_sgs > 0)
 		dma = sg_dma_address(req->start_sg);
 	else
@@ -1089,6 +1094,8 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
 
 	__dwc3_prepare_one_trb(dep, trb, dma, trb_length, chain, node,
 			stream_id, short_not_ok, no_interrupt, is_last);
+
+	return 0;
 }
 
 /**
@@ -1097,11 +1104,13 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
  * @req: The request to prepare
  * @entry_length: The last SG entry size
  * @node: Indicates whether this is not the first entry (for isoc only)
+ *
+ * Returns 0 on success or -EAGAIN if there is not enough TRBs.
  */
-static void dwc3_prepare_last_sg(struct dwc3_ep *dep,
-				 struct dwc3_request *req,
-				 unsigned int entry_length,
-				 unsigned int node)
+static int dwc3_prepare_last_sg(struct dwc3_ep *dep,
+				struct dwc3_request *req,
+				unsigned int entry_length,
+				unsigned int node)
 {
 	unsigned int maxp = usb_endpoint_maxp(dep->endpoint.desc);
 	unsigned int rem = req->request.length % maxp;
@@ -1121,6 +1130,9 @@ static void dwc3_prepare_last_sg(struct dwc3_ep *dep,
 	if (num_extra_trbs > 0)
 		req->needs_extra_trb = true;
 
+	if (dwc3_calc_trbs_left(dep) < num_extra_trbs + 1)
+		return -EAGAIN;
+
 	/* Prepare a normal TRB */
 	dwc3_prepare_one_trb(dep, req, entry_length, req->needs_extra_trb, node);
 
@@ -1148,9 +1160,11 @@ static void dwc3_prepare_last_sg(struct dwc3_ep *dep,
 				       req->request.no_interrupt,
 				       req->request.is_last);
 	}
+
+	return 0;
 }
 
-static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
+static int dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
 		struct dwc3_request *req)
 {
 	struct scatterlist *sg = req->start_sg;
@@ -1170,6 +1184,7 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
 	for_each_sg(sg, s, remaining, i) {
 		unsigned int trb_length;
 		bool last_sg = false;
+		int ret;
 
 		trb_length = min_t(unsigned int, length, sg_dma_len(s));
 
@@ -1186,9 +1201,13 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
 			last_sg = true;
 
 		if (last_sg)
-			dwc3_prepare_last_sg(dep, req, trb_length, i);
+			ret = dwc3_prepare_last_sg(dep, req, trb_length, i);
 		else
-			dwc3_prepare_one_trb(dep, req, trb_length, 1, i);
+			ret = dwc3_prepare_one_trb(dep, req, trb_length, 1, i);
+
+		/* Ran out of TRBs */
+		if (ret)
+			return ret;
 
 		/*
 		 * There can be a situation where all sgs in sglist are not
@@ -1211,16 +1230,14 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
 			req->num_pending_sgs -= req->request.num_mapped_sgs - req->num_queued_sgs;
 			break;
 		}
-
-		if (!dwc3_calc_trbs_left(dep))
-			break;
 	}
+	return 0;
 }
 
-static void dwc3_prepare_one_trb_linear(struct dwc3_ep *dep,
+static int dwc3_prepare_one_trb_linear(struct dwc3_ep *dep,
 		struct dwc3_request *req)
 {
-	dwc3_prepare_last_sg(dep, req, req->request.length, 0);
+	return dwc3_prepare_last_sg(dep, req, req->request.length, 0);
 }
 
 /*
@@ -1230,10 +1247,13 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep *dep,
  * The function goes through the requests list and sets up TRBs for the
  * transfers. The function returns once there are no more TRBs available or
  * it runs out of requests.
+ *
+ * Returns 0 on success or negative errno.
  */
-static void dwc3_prepare_trbs(struct dwc3_ep *dep)
+static int dwc3_prepare_trbs(struct dwc3_ep *dep)
 {
 	struct dwc3_request	*req, *n;
+	int ret = 0;
 
 	BUILD_BUG_ON_NOT_POWER_OF_2(DWC3_TRB_NUM);
 
@@ -1248,11 +1268,11 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
 	 * break things.
 	 */
 	list_for_each_entry(req, &dep->started_list, list) {
-		if (req->num_pending_sgs > 0)
-			dwc3_prepare_one_trb_sg(dep, req);
-
-		if (!dwc3_calc_trbs_left(dep))
-			return;
+		if (req->num_pending_sgs > 0) {
+			ret = dwc3_prepare_one_trb_sg(dep, req);
+			if (ret)
+				return ret;
+		}
 
 		/*
 		 * Don't prepare beyond a transfer. In DWC_usb32, its transfer
@@ -1260,17 +1280,16 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
 		 * active transfer instead of stopping.
 		 */
 		if (dep->stream_capable && req->request.is_last)
-			return;
+			return 0;
 	}
 
 	list_for_each_entry_safe(req, n, &dep->pending_list, list) {
 		struct dwc3	*dwc = dep->dwc;
-		int		ret;
 
 		ret = usb_gadget_map_request_by_dev(dwc->sysdev, &req->request,
 						    dep->direction);
 		if (ret)
-			return;
+			return ret;
 
 		req->sg			= req->request.sg;
 		req->start_sg		= req->sg;
@@ -1278,12 +1297,12 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
 		req->num_pending_sgs	= req->request.num_mapped_sgs;
 
 		if (req->num_pending_sgs > 0)
-			dwc3_prepare_one_trb_sg(dep, req);
+			ret = dwc3_prepare_one_trb_sg(dep, req);
 		else
-			dwc3_prepare_one_trb_linear(dep, req);
+			ret = dwc3_prepare_one_trb_linear(dep, req);
 
-		if (!dwc3_calc_trbs_left(dep))
-			return;
+		if (ret)
+			return ret;
 
 		/*
 		 * Don't prepare beyond a transfer. In DWC_usb32, its transfer
@@ -1291,8 +1310,9 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
 		 * active transfer instead of stopping.
 		 */
 		if (dep->stream_capable && req->request.is_last)
-			return;
+			return 0;
 	}
+	return 0;
 }
 
 static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep);
@@ -1305,12 +1325,12 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
 	int				ret;
 	u32				cmd;
 
-	if (!dwc3_calc_trbs_left(dep))
-		return 0;
-
 	starting = !(dep->flags & DWC3_EP_TRANSFER_STARTED);
 
-	dwc3_prepare_trbs(dep);
+	ret = dwc3_prepare_trbs(dep);
+	if (ret)
+		return 0;
+
 	req = next_request(&dep->started_list);
 	if (!req) {
 		dep->flags |= DWC3_EP_PENDING_REQUEST;
-- 
2.28.0

Re: [PATCH v3 2/4] usb: dwc3: gadget: Account for extra TRB

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 716 bytes --]


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> When checking for how many TRB remaining, make sure to account for extra
> TRBs for ZLP or MPS alignment transfers. Since the dwc3_prepare_trb*
> functions should know if we need the extra TRBs, make those functions
> return a status code -EAGAIN if there isn't enough TRB. Check against
> those status when preparing TRB instead.
>
> Fixes: c6267a51639b ("usb: dwc3: gadget: align transfers to wMaxPacketSize")
> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>

this is quite extensive for a fix and it also depends on the previous
refactoring from what I can tell. In that case, you can break this down
into smaller patches.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH v3 2/4] usb: dwc3: gadget: Account for extra TRB

[Thinh Nguyen]

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> When checking for how many TRB remaining, make sure to account for extra
>> TRBs for ZLP or MPS alignment transfers. Since the dwc3_prepare_trb*
>> functions should know if we need the extra TRBs, make those functions
>> return a status code -EAGAIN if there isn't enough TRB. Check against
>> those status when preparing TRB instead.
>>
>> Fixes: c6267a51639b ("usb: dwc3: gadget: align transfers to wMaxPacketSize")
>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> this is quite extensive for a fix and it also depends on the previous
> refactoring from what I can tell. In that case, you can break this down
> into smaller patches.
>

Sure. I'll break it down more and revise it so there's no dependency on
the previous cleanup patch.

Thanks,
Thinh

[PATCH v3 3/4] usb: dwc3: gadget: Rename misleading function names

[Thinh Nguyen]

The functions dwc3_prepare_one_trb_sg and dwc3_prepare_one_trb_linear
are not necessarily preparing "one" TRB, it can prepare multiple TRBs.
Rename these functions as follow:

dwc3_prepare_one_trb_sg -> dwc3_prepare_trbs_sg
dwc3_prepare_one_trb_linear -> dwc3_prepare_trbs_linear

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 Changes in v3:
 - None
 Changes in v2:
 - None

 drivers/usb/dwc3/gadget.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 7c95ac3186be..692b12367d8a 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1164,7 +1164,7 @@ static int dwc3_prepare_last_sg(struct dwc3_ep *dep,
 	return 0;
 }
 
-static int dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
+static int dwc3_prepare_trbs_sg(struct dwc3_ep *dep,
 		struct dwc3_request *req)
 {
 	struct scatterlist *sg = req->start_sg;
@@ -1234,7 +1234,7 @@ static int dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
 	return 0;
 }
 
-static int dwc3_prepare_one_trb_linear(struct dwc3_ep *dep,
+static int dwc3_prepare_trbs_linear(struct dwc3_ep *dep,
 		struct dwc3_request *req)
 {
 	return dwc3_prepare_last_sg(dep, req, req->request.length, 0);
@@ -1269,7 +1269,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_one_trb_sg(dep, req);
+			ret = dwc3_prepare_trbs_sg(dep, req);
 			if (ret)
 				return ret;
 		}
@@ -1297,9 +1297,9 @@ static int dwc3_prepare_trbs(struct dwc3_ep *dep)
 		req->num_pending_sgs	= req->request.num_mapped_sgs;
 
 		if (req->num_pending_sgs > 0)
-			ret = dwc3_prepare_one_trb_sg(dep, req);
+			ret = dwc3_prepare_trbs_sg(dep, req);
 		else
-			ret = dwc3_prepare_one_trb_linear(dep, req);
+			ret = dwc3_prepare_trbs_linear(dep, req);
 
 		if (ret)
 			return ret;
-- 
2.28.0

[PATCH v3 4/4] usb: dwc3: ep0: Skip ZLP setup for OUT

[Thinh Nguyen]

The current implementation for ZLP handling of usb_request->zero for ep0
is only for control IN requests. For OUT direction, DWC3 needs to check
and set up for MPS boundary alignment, and it doesn't do that at the
moment.

Usually, control OUT requests can indicate its transfer size via the
wLength field of the control message. So usb_request->zero is usually
not needed for OUT direction. To handle ZLP OUT for control endpoint,
we'd need to allocate at least 3 TRBs for control requests (we have 2 at
the moment). For now, let's just make sure the current ZLP setup is only
for IN direction.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 Changes in v3:
 - None
 Changes in v2:
 - None

 drivers/usb/dwc3/ep0.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 5bb4327ae237..b531f63d19de 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -979,7 +979,7 @@ static void __dwc3_ep0_do_control_data(struct dwc3 *dwc,
 					 false);
 		ret = dwc3_ep0_start_trans(dep);
 	} else if (IS_ALIGNED(req->request.length, dep->endpoint.maxpacket) &&
-		   req->request.length && req->request.zero) {
+		   req->request.length && req->request.zero && req->direction) {
 
 		ret = usb_gadget_map_request_by_dev(dwc->sysdev,
 				&req->request, dep->number);
-- 
2.28.0