Hi,
Let's look at the relevant code:
fei.yang@intel.com writes:
From: Fei Yang fei.yang@intel.com
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@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().
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.