[PATCH v2 0/2] fix trb handling for high speed isoc transfers

[PATCH v2 0/2] fix trb handling for high speed isoc transfers

[Michael Grzeschik]

Michael Grzeschik (2):
  usb: dwc3: gadget: refactor dwc3_repare_one_trb
  usb: dwc3: gadget: fix high speed multiplier setting

 drivers/usb/dwc3/gadget.c | 92 +++++++++++++++++----------------------
 1 file changed, 40 insertions(+), 52 deletions(-)

-- 
2.30.2

[PATCH v2 1/2] usb: dwc3: gadget: refactor dwc3_repare_one_trb

[Michael Grzeschik]

The function __dwc3_prepare_one_trb has many parameters. Since it is
only used in dwc3_prepare_one_trb there is no point in keeping the
function. We merge both functions and get rid of the big list of
parameters.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
v1 -> v2: - new patch

 drivers/usb/dwc3/gadget.c | 92 +++++++++++++++++----------------------
 1 file changed, 40 insertions(+), 52 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index a944c7a6c83a28..dcd8fc209ccd6a 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1183,17 +1183,49 @@ static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep)
 	return trbs_left;
 }
 
-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, bool must_interrupt)
+/**
+ * dwc3_prepare_one_trb - setup one TRB from one request
+ * @dep: endpoint for which this request is prepared
+ * @req: dwc3_request pointer
+ * @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.
+ * @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,
+		bool must_interrupt)
 {
+	struct dwc3_trb		*trb;
+	dma_addr_t		dma;
+	unsigned int		stream_id = req->request.stream_id;
+	unsigned int		short_not_ok = req->request.short_not_ok;
+	unsigned int		no_interrupt = req->request.no_interrupt;
+	unsigned int		is_last = req->request.is_last;
 	struct dwc3		*dwc = dep->dwc;
 	struct usb_gadget	*gadget = dwc->gadget;
 	enum usb_device_speed	speed = gadget->speed;
 
-	trb->size = DWC3_TRB_SIZE_LENGTH(length);
+	if (use_bounce_buffer)
+		dma = dep->dwc->bounce_addr;
+	else if (req->request.num_sgs > 0)
+		dma = sg_dma_address(req->start_sg);
+	else
+		dma = req->request.dma;
+
+	trb = &dep->trb_pool[dep->trb_enqueue];
+
+	if (!req->trb) {
+		dwc3_gadget_move_started_request(req);
+		req->trb = trb;
+		req->trb_dma = dwc3_trb_dma_offset(dep, trb);
+	}
+
+	req->num_trbs++;
+
+	trb->size = DWC3_TRB_SIZE_LENGTH(trb_length);
 	trb->bpl = lower_32_bits(dma);
 	trb->bph = upper_32_bits(dma);
 
@@ -1233,10 +1265,10 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
 				unsigned int mult = 2;
 				unsigned int maxp = usb_endpoint_maxp(ep->desc);
 
-				if (length <= (2 * maxp))
+				if (trb_length <= (2 * maxp))
 					mult--;
 
-				if (length <= maxp)
+				if (trb_length <= maxp)
 					mult--;
 
 				trb->size |= DWC3_TRB_SIZE_PCM1(mult);
@@ -1310,50 +1342,6 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
 	trace_dwc3_prepare_trb(dep, trb);
 }
 
-/**
- * dwc3_prepare_one_trb - setup one TRB from one request
- * @dep: endpoint for which this request is prepared
- * @req: dwc3_request pointer
- * @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.
- * @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,
-		bool must_interrupt)
-{
-	struct dwc3_trb		*trb;
-	dma_addr_t		dma;
-	unsigned int		stream_id = req->request.stream_id;
-	unsigned int		short_not_ok = req->request.short_not_ok;
-	unsigned int		no_interrupt = req->request.no_interrupt;
-	unsigned int		is_last = req->request.is_last;
-
-	if (use_bounce_buffer)
-		dma = dep->dwc->bounce_addr;
-	else if (req->request.num_sgs > 0)
-		dma = sg_dma_address(req->start_sg);
-	else
-		dma = req->request.dma;
-
-	trb = &dep->trb_pool[dep->trb_enqueue];
-
-	if (!req->trb) {
-		dwc3_gadget_move_started_request(req);
-		req->trb = trb;
-		req->trb_dma = dwc3_trb_dma_offset(dep, trb);
-	}
-
-	req->num_trbs++;
-
-	__dwc3_prepare_one_trb(dep, trb, dma, trb_length, chain, node,
-			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);
-- 
2.30.2

Re: [PATCH v2 1/2] usb: dwc3: gadget: refactor dwc3_repare_one_trb

[Greg KH]

On Mon, Jul 04, 2022 at 04:18:11PM +0200, Michael Grzeschik wrote:
> The function __dwc3_prepare_one_trb has many parameters. Since it is
> only used in dwc3_prepare_one_trb there is no point in keeping the
> function. We merge both functions and get rid of the big list of
> parameters.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
> v1 -> v2: - new patch
> 
>  drivers/usb/dwc3/gadget.c | 92 +++++++++++++++++----------------------
>  1 file changed, 40 insertions(+), 52 deletions(-)

Much nicer, thanks!

[PATCH v2 2/2] usb: dwc3: gadget: fix high speed multiplier setting

[Michael Grzeschik]

For High-Speed Transfers the prepare_one_trb function is calculating the
multiplier setting for the trb based on the length parameter of the trb
currently prepared. This assumption is wrong. For trbs with a sg list,
the length of the actual request has to be taken instead.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
v1 -> v2: - added refactor patch before this patch
          - using req->request.length as condition value

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

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index dcd8fc209ccd6a..4366c45c28cf6d 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1265,10 +1265,10 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
 				unsigned int mult = 2;
 				unsigned int maxp = usb_endpoint_maxp(ep->desc);
 
-				if (trb_length <= (2 * maxp))
+				if (req->request.length <= (2 * maxp))
 					mult--;
 
-				if (trb_length <= maxp)
+				if (req->request.length <= maxp)
 					mult--;
 
 				trb->size |= DWC3_TRB_SIZE_PCM1(mult);
-- 
2.30.2

Re: [PATCH v2 2/2] usb: dwc3: gadget: fix high speed multiplier setting

[Greg KH]

On Mon, Jul 04, 2022 at 04:18:12PM +0200, Michael Grzeschik wrote:
> For High-Speed Transfers the prepare_one_trb function is calculating the
> multiplier setting for the trb based on the length parameter of the trb
> currently prepared. This assumption is wrong. For trbs with a sg list,
> the length of the actual request has to be taken instead.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
> v1 -> v2: - added refactor patch before this patch
>           - using req->request.length as condition value

Does this need to be backported to older kernels or is it ok for
5.20-rc1?

thanks,

greg k-h

Re: [PATCH v2 2/2] usb: dwc3: gadget: fix high speed multiplier setting

[Michael Grzeschik]

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

Cc: Thinh.Nguyen@synopsys.com

On Fri, Jul 08, 2022 at 02:45:30PM +0200, Greg KH wrote:
>On Mon, Jul 04, 2022 at 04:18:12PM +0200, Michael Grzeschik wrote:
>> For High-Speed Transfers the prepare_one_trb function is calculating the
>> multiplier setting for the trb based on the length parameter of the trb
>> currently prepared. This assumption is wrong. For trbs with a sg list,
>> the length of the actual request has to be taken instead.
>>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> ---
>> v1 -> v2: - added refactor patch before this patch
>>           - using req->request.length as condition value
>
>Does this need to be backported to older kernels or is it ok for
>5.20-rc1?

We can add this line to the patch.

Fixes: 40d829fb2ec6 ("usb: dwc3: gadget: Correct ISOC DATA PIDs for short packets")

I would say this could be backported.

Thinh?

>thanks,
>
>greg k-h
>

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

Re: [PATCH v2 2/2] usb: dwc3: gadget: fix high speed multiplier setting

[Thinh Nguyen]

On 7/8/2022, Michael Grzeschik wrote:
> Cc: Thinh.Nguyen@synopsys.com
>
> On Fri, Jul 08, 2022 at 02:45:30PM +0200, Greg KH wrote:
>> On Mon, Jul 04, 2022 at 04:18:12PM +0200, Michael Grzeschik wrote:
>>> For High-Speed Transfers the prepare_one_trb function is calculating 
>>> the
>>> multiplier setting for the trb based on the length parameter of the trb
>>> currently prepared. This assumption is wrong. For trbs with a sg list,
>>> the length of the actual request has to be taken instead.
>>>
>>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>> ---
>>> v1 -> v2: - added refactor patch before this patch
>>>           - using req->request.length as condition value
>>
>> Does this need to be backported to older kernels or is it ok for
>> 5.20-rc1?
>
> We can add this line to the patch.
>
> Fixes: 40d829fb2ec6 ("usb: dwc3: gadget: Correct ISOC DATA PIDs for 
> short packets")
>
> I would say this could be backported.
>
> Thinh?

Hi Michael,

Since you have the refactoring patch prior to this, it's a bit difficult 
to backport cleanly without rewriting the patch.

It would be nice to be able to backport fixes. But for this particular 
fix, I don't think will affect too many users. Not many application 
would use SG for simply 3KB or less requests. (Note that periodic ep can 
only transfer up to 3KB/interval in HS). Typically a page size is 4KB, 
and I don't think SG is needed.

Anyway, I think Greg already picked up your patches. Thanks for the fix!

BR,
Thinh