Hi,
Let's look at the relevant code:
fei.yang@xxxxxxxxx writes:
> From: Fei Yang <fei.yang@xxxxxxxxx>
>
> If scatter-gather operation is allowed, a large USB request would be 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, dwc3_gadget_ep_reclaim_completed_trb()
> would be called only once for the whole USB request, thus all the TRBs need
> to be reclaimed within this single call. However that is not what the current
> code does.
>
> This patch addresses the issue by checking each TRB in function
> dwc3_gadget_ep_reclaim_trb_sg() and reclaiming the chained ones right there.
> Only the last TRB gets passed to dwc3_gadget_ep_reclaim_completed_trb(). This
> would guarantee all TRBs are reclaimed and trb_dequeue/num_trbs are updated
> properly.
>
> 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().
>
> ---
>
> drivers/usb/dwc3/gadget.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 173f532..c0662c2 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2404,7 +2404,7 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
> struct dwc3_request *req, const struct dwc3_event_depevt *event,
> int status)
Here's the full function:
| 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 scatterlist *sg = req->sg;
| struct scatterlist *s;
| unsigned int pending = req->num_pending_sgs;
| unsigned int i;
| int ret = 0;
|
| for_each_sg(sg, s, pending, i) {
iterate over each scatterlist member for the current request...
| 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, true);
... and reclaim its TRB.
Now, looking dwc3_gadget_ep_reclaim_compmleted_trb() we have:
| static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
| struct dwc3_request *req, struct dwc3_trb *trb,
| const struct dwc3_event_depevt *event, int status, int chain)
| {
| unsigned int count;
|
| dwc3_ep_inc_deq(dep);
unconditionally increment the dequeue pointer. What Are we missing here?
[...]
| return 0;
| }
Now, looking at what your patch does we will have:
> {
> - struct dwc3_trb *trb = &dep->trb_pool[dep->trb_dequeue];
> + struct dwc3_trb *trb;
small cleanup, should be part of its own patch.
> struct scatterlist *sg = req->sg;
> struct scatterlist *s;
> unsigned int pending = req->num_pending_sgs;
> @@ -2419,7 +2419,15 @@ 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_IOC)) {
> + /* reclaim the TRB without calling
> + * dwc3_gadget_ep_reclaim_completed_trb */
why do you have to skip dwc3_gadget_ep_reclaim_completed_trb()? Also,
your patch description claims that we're NOT incrementing the TRBs,
which is wrong. I fail to see what problem you're trying to solve here,
really.
Could it be that we're, simply. returning 1 when we should return 0 for
the previous SG list members? If that's the case, then that's the bug
that should be fixed. Still, you shouldn't avoid calling
dwc3_gadget_ep_reclaim_completed_trb() and should, instead, fix the bug
it contains.
Looking at the cases where dwc3_gadget_ep_reclaim_completed_trb()
returns 1, I can't see how that would be the case either:
| if (chain && (trb->ctrl & DWC3_TRB_CTRL_HWO))
| trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
if CHN bit it set and HWO is bit, clear HWO
| if (req->needs_extra_trb && !(trb->ctrl & DWC3_TRB_CTRL_CHN)) {
| trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
| return 1;
| }
if *not* CHN and needs_extra_trb, return 1. This can only be true for
the last TRB in the SG list.
| if ((trb->ctrl & DWC3_TRB_CTRL_HWO) && status != -ESHUTDOWN)
| return 1;
This can't be true because we cleared HWO up above
| if (event->status & DEPEVT_STATUS_SHORT && !chain)
| return 1;
can only be true for last TRB
| if (event->status & DEPEVT_STATUS_IOC)
| return 1;
If we have a short packet, then we may fall here. Is that the case?
Please share dwc3 tracepoints of the problem happening so I can verify
what's going on.
--
balbi
