There are a few issues in DWC3 driver when preparing for TRB. The driver needs to account the following:
* MPS alignment for ZLP OUT direction * Extra TRBs when checking for available TRBs * SG entries size > request length
Along with these fixes, there are some cleanup/refactoring patches in this series .
Thinh Nguyen (7): usb: dwc3: gadget: Don't setup more than requested usb: dwc3: gadget: Fix handling ZLP usb: dwc3: gadget: Handle ZLP for sg requests usb: dwc3: gadget: Refactor preparing TRBs usb: dwc3: gadget: Account for extra TRB usb: dwc3: gadget: Rename misleading function names usb: dwc3: ep0: Skip ZLP setup for OUT
drivers/usb/dwc3/ep0.c | 2 +- drivers/usb/dwc3/gadget.c | 232 ++++++++++++++++++++++---------------- 2 files changed, 137 insertions(+), 97 deletions(-)
base-commit: e3ee0e740c3887d2293e8d54a8707218d70d86ca
The SG list may be set up with entry size more than the requested length. Check the usb_request->length and make sure that we don't setup the TRBs to send/receive more than requested. This case may occur when the SG entry is allocated up to a certain minimum size, but the request length is less than that. It can also occur when the request is reused for a different request length.
Cc: stable@vger.kernel.org Fixes: a31e63b608ff ("usb: dwc3: gadget: Correct handling of scattergather lists") Signed-off-by: Thinh Nguyen thinhn@synopsys.com --- drivers/usb/dwc3/gadget.c | 41 ++++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index e44bfc3b5096..657616077502 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1054,27 +1054,25 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb, * dwc3_prepare_one_trb - setup one TRB from one request * @dep: endpoint for which this request is prepared * @req: dwc3_request pointer + * @trb_length: buffer size of the TRB * @chain: should this TRB be chained to the next? * @node: only for isochronous endpoints. First TRB needs different type. */ static void dwc3_prepare_one_trb(struct dwc3_ep *dep, - struct dwc3_request *req, unsigned chain, unsigned node) + struct dwc3_request *req, unsigned int trb_length, + unsigned chain, unsigned node) { struct dwc3_trb *trb; - unsigned int length; dma_addr_t dma; unsigned stream_id = req->request.stream_id; unsigned short_not_ok = req->request.short_not_ok; unsigned no_interrupt = req->request.no_interrupt; unsigned is_last = req->request.is_last;
- if (req->request.num_sgs > 0) { - length = sg_dma_len(req->start_sg); + if (req->request.num_sgs > 0) dma = sg_dma_address(req->start_sg); - } else { - length = req->request.length; + else dma = req->request.dma; - }
trb = &dep->trb_pool[dep->trb_enqueue];
@@ -1086,7 +1084,7 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
req->num_trbs++;
- __dwc3_prepare_one_trb(dep, trb, dma, length, chain, node, + __dwc3_prepare_one_trb(dep, trb, dma, trb_length, chain, node, stream_id, short_not_ok, no_interrupt, is_last); }
@@ -1104,8 +1102,13 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep, unsigned int length = req->request.length; unsigned int maxp = usb_endpoint_maxp(dep->endpoint.desc); unsigned int rem = length % maxp; + unsigned int trb_length; unsigned chain = true;
+ trb_length = min_t(unsigned int, length, sg_dma_len(req->start_sg)); + + length -= trb_length; + /* * IOMMU driver is coalescing the list of sgs which shares a * page boundary into one and giving it to USB driver. With @@ -1113,7 +1116,7 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep, * sgs passed. So mark the chain bit to false if it isthe last * mapped sg. */ - if (i == remaining - 1) + if ((i == remaining - 1) || !length) chain = false;
if (rem && usb_endpoint_dir_out(dep->endpoint.desc) && !chain) { @@ -1123,7 +1126,7 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep, req->needs_extra_trb = true;
/* prepare normal TRB */ - dwc3_prepare_one_trb(dep, req, true, i); + dwc3_prepare_one_trb(dep, req, trb_length, true, i);
/* Now prepare one extra TRB to align transfer size */ trb = &dep->trb_pool[dep->trb_enqueue]; @@ -1135,7 +1138,7 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep, req->request.no_interrupt, req->request.is_last); } else { - dwc3_prepare_one_trb(dep, req, chain, i); + dwc3_prepare_one_trb(dep, req, trb_length, chain, i); }
/* @@ -1150,6 +1153,16 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
req->num_queued_sgs++;
+ /* + * The number of pending SG entries may not correspond to the + * number of mapped SG entries. If all the data are queued, then + * don't include unused SG entries. + */ + if (length == 0) { + req->num_pending_sgs -= req->request.num_mapped_sgs - req->num_queued_sgs; + break; + } + if (!dwc3_calc_trbs_left(dep)) break; } @@ -1169,7 +1182,7 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep *dep, req->needs_extra_trb = true;
/* prepare normal TRB */ - dwc3_prepare_one_trb(dep, req, true, 0); + dwc3_prepare_one_trb(dep, req, length, true, 0);
/* Now prepare one extra TRB to align transfer size */ trb = &dep->trb_pool[dep->trb_enqueue]; @@ -1187,7 +1200,7 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep *dep, req->needs_extra_trb = true;
/* prepare normal TRB */ - dwc3_prepare_one_trb(dep, req, true, 0); + dwc3_prepare_one_trb(dep, req, length, true, 0);
/* Now prepare one extra TRB to handle ZLP */ trb = &dep->trb_pool[dep->trb_enqueue]; @@ -1198,7 +1211,7 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep *dep, req->request.no_interrupt, req->request.is_last); } else { - dwc3_prepare_one_trb(dep, req, false, 0); + dwc3_prepare_one_trb(dep, req, length, false, 0); } }
Thinh Nguyen wrote:
The SG list may be set up with entry size more than the requested length. Check the usb_request->length and make sure that we don't setup the TRBs to send/receive more than requested. This case may occur when the SG entry is allocated up to a certain minimum size, but the request length is less than that. It can also occur when the request is reused for a different request length.
Cc: stable@vger.kernel.org Fixes: a31e63b608ff ("usb: dwc3: gadget: Correct handling of scattergather lists") Signed-off-by: Thinh Nguyen thinhn@synopsys.com
drivers/usb/dwc3/gadget.c | 41 ++++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 14 deletions(-)
Got a little trigger happy with this patch. Too many obvious issues. Please ignore this patch for now. Will revise and resubmit.
Thanks, Thinh
The usb_request->zero doesn't apply for isoc. Also, if we prepare a 0-length (ZLP) TRB for the OUT direction, we need to prepare an extra TRB to pad up to the MPS alignment. Use the same bounce buffer for the ZLP TRB and the extra pad TRB.
Cc: stable@vger.kernel.org Fixes: d6e5a549cc4d ("usb: dwc3: simplify ZLP handling") Fixes: 04c03d10e507 ("usb: dwc3: gadget: handle request->zero") Signed-off-by: Thinh Nguyen thinhn@synopsys.com --- drivers/usb/dwc3/gadget.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 657616077502..c0175dff194e 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1193,6 +1193,7 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep *dep, req->request.no_interrupt, req->request.is_last); } else if (req->request.zero && req->request.length && + !usb_endpoint_xfer_isoc(dep->endpoint.desc) && (IS_ALIGNED(req->request.length, maxp))) { struct dwc3 *dwc = dep->dwc; struct dwc3_trb *trb; @@ -1202,14 +1203,25 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep *dep, /* prepare normal TRB */ dwc3_prepare_one_trb(dep, req, length, true, 0);
- /* Now prepare one extra TRB to handle ZLP */ + /* Prepare one extra TRB to handle ZLP */ trb = &dep->trb_pool[dep->trb_enqueue]; req->num_trbs++; __dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, 0, - false, 1, req->request.stream_id, + !req->direction, 1, req->request.stream_id, req->request.short_not_ok, req->request.no_interrupt, req->request.is_last); + + /* Prepare one more TRB to handle MPS alignment for OUT */ + if (!req->direction) { + trb = &dep->trb_pool[dep->trb_enqueue]; + req->num_trbs++; + __dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, maxp, + false, 1, req->request.stream_id, + req->request.short_not_ok, + req->request.no_interrupt, + req->request.is_last); + } } else { dwc3_prepare_one_trb(dep, req, length, false, 0); } @@ -2684,8 +2696,17 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep, status);
if (req->needs_extra_trb) { + unsigned int maxp = usb_endpoint_maxp(dep->endpoint.desc); + ret = dwc3_gadget_ep_reclaim_trb_linear(dep, req, event, status); + + /* Reclaim MPS padding TRB for ZLP */ + if (!req->direction && req->request.zero && req->request.length && + !usb_endpoint_xfer_isoc(dep->endpoint.desc) && + (IS_ALIGNED(req->request.length, maxp))) + ret = dwc3_gadget_ep_reclaim_trb_linear(dep, req, event, status); + req->needs_extra_trb = false; }
Currently dwc3 doesn't handle usb_request->zero for SG requests. This change checks and prepares extra TRBs for the ZLP for SG requests.
Cc: stable@vger.kernel.org Fixes: 04c03d10e507 ("usb: dwc3: gadget: handle request->zero") Signed-off-by: Thinh Nguyen thinhn@synopsys.com --- drivers/usb/dwc3/gadget.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index c0175dff194e..ced229aeccec 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1137,6 +1137,37 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep, req->request.short_not_ok, req->request.no_interrupt, req->request.is_last); + } else if (req->request.zero && req->request.length && + !usb_endpoint_xfer_isoc(dep->endpoint.desc) && + !rem && !chain) { + struct dwc3 *dwc = dep->dwc; + struct dwc3_trb *trb; + + req->needs_extra_trb = true; + + /* Prepare normal TRB */ + dwc3_prepare_one_trb(dep, req, trb_length, true, i); + + /* Prepare one extra TRB to handle ZLP */ + trb = &dep->trb_pool[dep->trb_enqueue]; + req->num_trbs++; + __dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, 0, + !req->direction, 1, + req->request.stream_id, + req->request.short_not_ok, + req->request.no_interrupt, + req->request.is_last); + + /* Prepare one more TRB to handle MPS alignment */ + if (!req->direction) { + trb = &dep->trb_pool[dep->trb_enqueue]; + req->num_trbs++; + __dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, maxp, + false, 1, req->request.stream_id, + req->request.short_not_ok, + req->request.no_interrupt, + req->request.is_last); + } } else { dwc3_prepare_one_trb(dep, req, trb_length, chain, i); }
linux-stable-mirror@lists.linaro.org