Re: [PATCH v3] usb: dwc3: gadget: trb_dequeue is not updated properly

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

Hi,

fei.yang@intel.com wrote:
> From: Fei Yang <fei.yang@intel.com>
>
> If scatter-gather operation is allowed, a large USB request is split into
> multiple TRBs. These TRBs are chained up by setting DWC3_TRB_CTRL_CHN bit
> except the last one which has DWC3_TRB_CTRL_IOC bit set instead.
> Since only the last TRB has IOC set for the whole USB request, the
> dwc3_gadget_ep_reclaim_trb_sg() gets called only once after the last TRB
> completes and all the TRBs allocated for this request are supposed to be
> reclaimed. However that is not what the current code does.
>
> dwc3_gadget_ep_reclaim_trb_sg() is trying to reclaim all the TRBs in the
> following for-loop,
> 	for_each_sg(sg, s, pending, i) {
> 		trb = &dep->trb_pool[dep->trb_dequeue];
>
>                 if (trb->ctrl & DWC3_TRB_CTRL_HWO)
>                         break;
>
>                 req->sg = sg_next(s);
>                 req->num_pending_sgs--;
>
>                 ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
>                                 trb, event, status, chain);
>                 if (ret)
>                         break;
>         }
> but since the interrupt comes only after the last TRB completes, the
> event->status has DEPEVT_STATUS_IOC bit set, so that the for-loop ends for
> the first TRB due to dwc3_gadget_ep_reclaim_completed_trb() returns 1.
> 	if (event->status & DEPEVT_STATUS_IOC)
> 		return 1;
>
> This patch addresses the issue by checking each TRB in function
> dwc3_gadget_ep_reclaim_trb_sg() and maing sure the chained ones are properly
> reclaimed. dwc3_gadget_ep_reclaim_completed_trb() will return 1 Only for the
> last TRB.
>
> Signed-off-by: Fei Yang <fei.yang@intel.com>
> Cc: stable <stable@vger.kernel.org>
> ---
> v2: Better solution is to reclaim chained TRBs in dwc3_gadget_ep_reclaim_trb_sg()
>     and leave the last TRB to the dwc3_gadget_ep_reclaim_completed_trb().
> v3: Checking DWC3_TRB_CTRL_CHN bit for each TRB instead, and making sure that
>     dwc3_gadget_ep_reclaim_completed_trb() returns 1 only for the last TRB.
> ---
>  drivers/usb/dwc3/gadget.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 173f532..88eed49 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2394,7 +2394,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>  	if (event->status & DEPEVT_STATUS_SHORT && !chain)
>  		return 1;
>  
> -	if (event->status & DEPEVT_STATUS_IOC)
> +	if (event->status & DEPEVT_STATUS_IOC && !chain)
>  		return 1;
>  
>  	return 0;
> @@ -2404,11 +2404,12 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>  		struct dwc3_request *req, const struct dwc3_event_depevt *event,
>  		int status)
>  {
> -	struct dwc3_trb *trb = &dep->trb_pool[dep->trb_dequeue];
> +	struct dwc3_trb *trb;
>  	struct scatterlist *sg = req->sg;
>  	struct scatterlist *s;
>  	unsigned int pending = req->num_pending_sgs;
>  	unsigned int i;
> +	int chain = false;
>  	int ret = 0;
>  
>  	for_each_sg(sg, s, pending, i) {
> @@ -2419,9 +2420,13 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>  
>  		req->sg = sg_next(s);
>  		req->num_pending_sgs--;
> +		if (trb->ctrl & DWC3_TRB_CTRL_CHN)
> +			chain = true;
> +		else
> +			chain = false;
>  
>  		ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
> -				trb, event, status, true);
> +				trb, event, status, chain);
>  		if (ret)
>  			break;
>  	}

There was already a fix a long time ago by Anurag. But it never made it
to the kernel mainline. You can check this out:
https://patchwork.kernel.org/patch/10640137/

Hi Felipe,

Maybe you can review and cherry-pick that patch?

Thanks,
Thinh