Skip to contentSkip to conversation

usb: dwc3: gadget: Don't skip updating remaining data

PATCH New Felipe Balbi 1 participant 1 message Jul 31, 2018 USB

lore
Share & tools
lore
#0 lore raw

PATCH 2/3 usb: dwc3: gadget: Don't skip updating remaining data

Quoted 58 lines · Thinh Nguyen

Hi Felipe,

Quoted 39 lines · Felipe Balbi
Quoted 25 lines · Thinh Nguyen

DWC3 must check for the BUFSIZ and update the req->remaining regardless of transfer alignment. Returning early from transfer OUT unalignment will skip updating the req->remaining.

Fixes c6267a51639b ("usb: dwc3: gadget: align transfers to wMaxPacketSize")
---
 drivers/usb/dwc3/gadget.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/usb/dwc3/gadget.c +3 -0
index 032ea7d709ba..a5b8387a37ba 100644
@@ -2246,6 +2246,9 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
 	if (chain && (trb->ctrl & DWC3_TRB_CTRL_HWO))
 		trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
 
+	count = trb->size & DWC3_TRB_SIZE_MASK;
+	req->remaining += count;
+
 	/*
 	 * If we're dealing with unaligned size OUT transfer, we will be left
 	 * with one TRB pending in the ring. We need to manually clear HWO bit

drivers/usb/dwc3/gadget.c static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, · line 2261

Quoted 6 lines
drivers/usb/dwc3/gadget.c +0 -3
@@ -2256,9 +2259,6 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
 		return 1;
 	}
 
-	count = trb->size & DWC3_TRB_SIZE_MASK;
-	req->remaining += count;
-

this shouldn't be necessary, actually. The remaining TRB which was added to fix alignment issues, is simply because DWC3 doesn't like OUT transfers whose size are not aligned to wMaxPacketSize. We *know* we have e.g. 511 bytes to receive from host, but dwc3 wants me to write 1024 bytes on the TRB (for superspeed), so I need to add this chained TRB for the remaining 513 bytes.

Quoted 3 lines

I don't expect host to send us more than 511 bytes, if it does, the host is faulty and I don't want to operate on those bytes anyway.

Right. But that's not the issue here. I realized that I did not describe the issue fully...

The issue here is the reporting of the actual number of bytes transferred. For isoc OUT transfers, the last TRB of the Buffer Descriptor will be retired with HWO = 0 (dwc_usb3 and dwc_usb31 programming guide 4.3.7). So, the check for unaligned && HWO will be false and will not return early. As a result, req->remaining will still be updated with the BUFSIZ count, and the remaining will be 513+ in your example. The 'actual' bytes written calculation will be wrong (e.g 511 - 513 = -2).

Quoted 4 lines

We can't rely on the current check to determine whether it's the last TRB. My current solution is to update req->remaining for all TRB completions. However, we have to also update the 'actual' bytes calculation for OUT transfers.

I think the bug here is another one. Look at the TRB types:

     irq/16-dwc3-2463  [003] d...  3589.933478: dwc3_complete_trb: ep1out: trb 000000007d3fbc0d buf 00000000b8ed3800 size 0 ctrl 2e46446c (hlCS:Sc:isoc-first)
     irq/16-dwc3-2463  [003] d...  3589.933492: dwc3_complete_trb: ep1out: trb 00000000d1fcb0c4 buf 000000003787e000 size 1023 ctrl 2e464c68 (hlcS:SC:isoc-first)

Shouldn't the one be of type ISOC instead of ISOC_FIRST? Can you test patch below and see if things behave better? Please also capture tracepoints :)

modified drivers/usb/dwc3/gadget.c
@@ -1073,7 +1073,7 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
/* Now prepare one extra TRB to align transfer size */ trb = &dep->trb_pool[dep->trb_enqueue]; __dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr,
- maxp - rem, false, 0,
+ maxp - rem, false, 1,
req->request.stream_id, req->request.short_not_ok, req->request.no_interrupt);
@@ -1117,7 +1117,7 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep *dep,
/* Now prepare one extra TRB to align transfer size */ trb = &dep->trb_pool[dep->trb_enqueue]; __dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, maxp - rem,
- false, 0, req->request.stream_id,
+ false, 1, req->request.stream_id,
req->request.short_not_ok, req->request.no_interrupt);
} else if (req->request.zero && req->request.length &&