[PATCH] usb: dwc3: gadget: fix high-speed multiplier setting

[PATCH] usb: dwc3: gadget: fix high-speed multiplier setting

[Michael Grzeschik]

For high-speed transfers the __dwc3_prepare_one_trb function is
calculating the multiplier setting for the trb based on the length of
the trb currently prepared. This assumption is wrong. For trbs with a
sglist the length of the actual request has to be taken instead.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/dwc3/gadget.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 8716bece107208..0fc3ecfaa48baf 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1186,7 +1186,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, bool must_interrupt)
+		unsigned int is_last, bool must_interrupt, int req_length)
 {
 	struct dwc3		*dwc = dep->dwc;
 	struct usb_gadget	*gadget = dwc->gadget;
@@ -1232,10 +1232,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 (req_length <= (2 * maxp))
 					mult--;
 
-				if (length <= maxp)
+				if (req_length <= maxp)
 					mult--;
 
 				trb->size |= DWC3_TRB_SIZE_PCM1(mult);
@@ -1350,7 +1350,7 @@ 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,
-			must_interrupt);
+			must_interrupt, req->request.length);
 }
 
 static bool dwc3_needs_extra_trb(struct dwc3_ep *dep, struct dwc3_request *req)
-- 
2.30.2

Re: [PATCH] usb: dwc3: gadget: fix high-speed multiplier setting

[Greg KH]

On Thu, Jun 30, 2022 at 04:02:16PM +0200, Michael Grzeschik wrote:
> For high-speed transfers the __dwc3_prepare_one_trb function is
> calculating the multiplier setting for the trb based on the length of
> the trb currently prepared. This assumption is wrong. For trbs with a
> sglist the length of the actual request has to be taken instead.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---

What commit id does this fix?

thanks,

greg k-h

Re: [PATCH] usb: dwc3: gadget: fix high-speed multiplier setting

[Michael Grzeschik]

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

On Fri, Jul 01, 2022 at 10:01:10AM +0200, Greg KH wrote:
>On Thu, Jun 30, 2022 at 04:02:16PM +0200, Michael Grzeschik wrote:
>> For high-speed transfers the __dwc3_prepare_one_trb function is
>> calculating the multiplier setting for the trb based on the length of
>> the trb currently prepared. This assumption is wrong. For trbs with a
>> sglist the length of the actual request has to be taken instead.
>>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> ---
>
>What commit id does this fix?

It would be commitish 40d829fb2ec636b6b4b0cc95e2546ab9aca04cc9 .

Thanks,
Michael

-- 
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] usb: dwc3: gadget: fix high-speed multiplier setting

[Greg KH]

On Thu, Jun 30, 2022 at 04:02:16PM +0200, Michael Grzeschik wrote:
> For high-speed transfers the __dwc3_prepare_one_trb function is
> calculating the multiplier setting for the trb based on the length of
> the trb currently prepared. This assumption is wrong. For trbs with a
> sglist the length of the actual request has to be taken instead.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/usb/dwc3/gadget.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 8716bece107208..0fc3ecfaa48baf 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1186,7 +1186,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, bool must_interrupt)
> +		unsigned int is_last, bool must_interrupt, int req_length)

Why not put this right next to 'length'?

And wow, that's a crazy number of options for a function.  Why is this
even a separate function at all?  Why can't it just be in
dwc3_prepare_one_trb() as that's the only place this is called?

thanks,

greg k-h

Re: [PATCH] usb: dwc3: gadget: fix high-speed multiplier setting

[Michael Grzeschik]

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

On Fri, Jul 01, 2022 at 10:02:56AM +0200, Greg KH wrote:
>On Thu, Jun 30, 2022 at 04:02:16PM +0200, Michael Grzeschik wrote:
>> For high-speed transfers the __dwc3_prepare_one_trb function is
>> calculating the multiplier setting for the trb based on the length of
>> the trb currently prepared. This assumption is wrong. For trbs with a
>> sglist the length of the actual request has to be taken instead.
>>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> ---
>>  drivers/usb/dwc3/gadget.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 8716bece107208..0fc3ecfaa48baf 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -1186,7 +1186,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, bool must_interrupt)
>> +		unsigned int is_last, bool must_interrupt, int req_length)
>
>Why not put this right next to 'length'?

Yes, would probably make more sense, but:

>And wow, that's a crazy number of options for a function.  Why is this
>even a separate function at all?  Why can't it just be in
>dwc3_prepare_one_trb() as that's the only place this is called?

This is an absolute legitimate question. I will send a v2 including a
patch refactoring this to one function beforehand.

Thanks,
Michael

-- 
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 --]