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

[Michael Grzeschik <mgr@pengutronix.de>]

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

On Tue, Apr 27, 2021 at 03:18:57AM +0000, Thinh Nguyen wrote:
>Hi Michael,
>
>Thinh Nguyen wrote:
>> Felipe Balbi wrote:
>>>
>>> HI,
>>>
>>> Michael Grzeschik <mgr@pengutronix.de> writes:
>>>
>>> <big snip>
>>>
>
>
><bigger snip>
>
>
>> I think I see the issue that Michael reported.
>>
>> The problem is that we're using num_pending_sgs to track both pending SG
>> entries and queued SG entries. num_pending_sgs doesn't get updated until
>> TRB completion interrupt (ie XferInProgress). Before the driver queues
>> more SG requests, it will check if there's any pending SG in the started
>> request list before it prepares more. Since the num_pending_sgs doesn't
>> get updated until the request is completed, the driver doesn't process
>> more until the request is completed.
>>
>> I need to review more on Michael's patches next week, but I think what
>> he suggested makes sense (in term of properly usage of queued sgs vs
>> pending sgs). BTW, please correct me if I'm wrong, but we do modify
>> num_queued_sgs.
>>
>
>There's still some issue with your patch. I think this should cover it.
>Let me know if it works for you.

This works for me! Will you spin a proper patch from that?

>Note: this however probably needs more "Tested-by" and reviews
>to make sure I'm not missing anything. I only ran some basic tests,
>and will need to run more.

You may already have mine:

Tested-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

>Let me know if this makes sense.

From what I understand about the issue and the purpose of all
variables this makes total sense to me. So thanks for taking over
and make a proper solution.

Thanks,
Michael

>BR,
>Thinh
>
>
>diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>index dd80e5ca8c78..040cc67b3361 100644
>--- a/drivers/usb/dwc3/gadget.c
>+++ b/drivers/usb/dwc3/gadget.c
>@@ -1244,6 +1244,7 @@ static int dwc3_prepare_trbs_sg(struct dwc3_ep *dep,
>                        req->start_sg = sg_next(s);
>
>                req->num_queued_sgs++;
>+               req->num_pending_sgs--;
>
>                /*
>                 * The number of pending SG entries may not correspond to the
>@@ -1251,7 +1252,7 @@ static int dwc3_prepare_trbs_sg(struct dwc3_ep *dep,
>                 * don't include unused SG entries.
>                 */
>                if (length == 0) {
>-                       req->num_pending_sgs -= req->request.num_mapped_sgs - req->num_queued_sgs;
>+                       req->num_pending_sgs = 0;
>                        break;
>                }
>
>@@ -2867,15 +2868,15 @@ 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 num_queued = req->num_queued_sgs;
>        unsigned int i;
>        int ret = 0;
>
>-       for_each_sg(sg, s, pending, i) {
>+       for_each_sg(sg, s, num_queued, i) {
>                trb = &dep->trb_pool[dep->trb_dequeue];
>
>                req->sg = sg_next(s);
>-               req->num_pending_sgs--;
>+               req->num_queued_sgs--;
>
>                ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
>                                trb, event, status, true);
>@@ -2898,7 +2899,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_pending_sgs == 0 && req->num_queued_sgs == 0;
> }
>
> static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
>@@ -2907,7 +2908,7 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
> {
>        int ret;
>
>-       if (req->num_pending_sgs)
>+       if (req->request.num_mapped_sgs)
>                ret = dwc3_gadget_ep_reclaim_trb_sg(dep, req, event,
>                                status);
>        else

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