| Message ID | 20220630140216.185919-1-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc3: gadget: fix high-speed multiplier setting | expand |
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
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
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
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
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)
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(-)