Hi,
fei.yang@xxxxxxxxx wrote:
> From: Fei Yang <fei.yang@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Cc: stable <stable@xxxxxxxxxxxxxxx>
> ---
> 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
