Re: [PATCH 2/2] dwc3: gadget: fix tracking of used sgs in request

[Felipe Balbi <balbi@kernel.org>]

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


Hi,

(subject format as well)

Michael Grzeschik <m.grzeschik@pengutronix.de> writes:
> The variable pending_sgs was used to keep track of handled
> sgs in one request. But instead queued_sgs is being decremented

no, it wasn't. If the total number of entries in the scatter list is 'x'
and we have transferred (completed) 'y' entries, then pending_sgs should
be (x - y).

> on every handled sg. This patch fixes the usage of the variable
> to use queued_sgs instead as intended.
>
> 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 118b5bcc565d6..2d7d861b13b31 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2856,7 +2856,7 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>  	struct dwc3_trb *trb = &dep->trb_pool[dep->trb_dequeue];
>  	struct scatterlist *sg = req->sg;
>  	struct scatterlist *s;
> -	unsigned int pending = req->num_pending_sgs;
> +	unsigned int pending = req->num_queued_sgs;
>  	unsigned int i;
>  	int ret = 0;
>  
> @@ -2864,7 +2864,7 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>  		trb = &dep->trb_pool[dep->trb_dequeue];
>  
>  		req->sg = sg_next(s);
> -		req->num_pending_sgs--;
> +		req->num_queued_sgs--;

no, this is wrong. queued shouldn't be modified as it comes straight
from the gadget driver. This is the number of entries in the request
that the gadget driver gave us. We don't want to modify it.

>  
>  		ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
>  				trb, event, status, true);
> @@ -2887,7 +2887,7 @@ static int dwc3_gadget_ep_reclaim_trb_linear(struct dwc3_ep *dep,
>  
>  static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req)
>  {
> -	return req->num_pending_sgs == 0;
> +	return req->num_queued_sgs == 0;

nope, request is, indeed, completed when there no more pending entries
to be consumed.

What sort of problem are you dealing with? Got any way of reproducing
it? Got some trace output showing the issue?

-- 
balbi

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