Hi Rob, Rob Weber <rob@xxxxxxxxxxx> writes: >> > * We still saw the drop from SS to HS on a Windows laptop. Linux and Mac >> > seemed to be fine, but there's definitely a chance we did not test >> > with enough samples to confidently say this quirk is stable with those >> > hosts. >> >> okay, so still not good enough. Let's try to prevent the device side >> from ever initiating U1/U2 entry and from ever accepting LGO_U1 and >> LGO_U2 from the host. Here's a (hack) patch for that >> >> modified drivers/usb/dwc3/ep0.c >> @@ -382,7 +382,7 @@ static int dwc3_ep0_handle_u1(struct dwc3 *dwc, enum usb_device_state state, >> >> reg = dwc3_readl(dwc->regs, DWC3_DCTL); >> if (set) >> - reg |= DWC3_DCTL_INITU1ENA; >> + return -EINVAL; >> else >> reg &= ~DWC3_DCTL_INITU1ENA; >> dwc3_writel(dwc->regs, DWC3_DCTL, reg); >> @@ -404,7 +404,7 @@ static int dwc3_ep0_handle_u2(struct dwc3 *dwc, enum usb_device_state state, >> >> reg = dwc3_readl(dwc->regs, DWC3_DCTL); >> if (set) >> - reg |= DWC3_DCTL_INITU2ENA; >> + return -EINVAL; >> else >> reg &= ~DWC3_DCTL_INITU2ENA; >> dwc3_writel(dwc->regs, DWC3_DCTL, reg); >> @@ -625,9 +625,9 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl) >> * Enable transition to U1/U2 state when >> * nothing is pending from application. >> */ >> - reg = dwc3_readl(dwc->regs, DWC3_DCTL); >> - reg |= (DWC3_DCTL_ACCEPTU1ENA | DWC3_DCTL_ACCEPTU2ENA); >> - dwc3_writel(dwc->regs, DWC3_DCTL, reg); >> + /* reg = dwc3_readl(dwc->regs, DWC3_DCTL); */ >> + /* reg |= (DWC3_DCTL_ACCEPTU1ENA | DWC3_DCTL_ACCEPTU2ENA); */ >> + /* dwc3_writel(dwc->regs, DWC3_DCTL, reg); */ >> } >> break; >> >> >> I don't think it will apply cleanly to your kernel, let me know if you >> need help porting it. > > Thank you for the offer. It wasn't too bad after reading the ep0.c file > for both 4.9.115 and master. I've included the patch in the attached > tarball if you would like to see how I ported it. Thank you. BTW, it may very well be that you only need to disable U2 and leave U1 enabled. That may help save some power, but I don't know whether you have requirements for this :-) >> > So tomorrow I hope to gather more concrete data to back up these >> > findings. Our USB protocol analyzer is also out-of-order at the moment, >> > so we might not be able to get analyzer data for a day or two either. >> > >> > Are there some other quirks you would like us to try out? >> >> See above, not a quirk flag, but just a complete removal of U1/U2 entry >> from the device side. > > Thank you for providing the above patch. It seems quite stable for a > variety of hosts! We want to continue testing this on several samples > with a bunch of different host computers and cables to make sure we're > in the clear, but this patch is feeling pretty good. Good to hear, please update me once you're happy with the results. Also, if you notice this problem happening again in the future, don't hesitate to contact me. > I've attached a tarball containing the dmesg output, regdump, and > tracepoints of a succesful session with the patch applied. I've included > the patch in the tarball if you would like to see how I ported it. > Could you please double-check the traces to make sure they meet your > expectations for disabling the U1/U2_ENABLE features? I looked through > the traces and could not find any link state changes to U1 or U2, so it > seems like it's working. The host, from that run, didn't enable device-initiated U1/U2 entry. So only host-initiated entry is avaiable. Since we have removed ACCEPTU1/ACCEPTU2 bits from DCTL, dwc3 is denying LGO_U1 and LGO_U2. You can confirm that only with a sniffer. > I have a couple of follow-up questions about this patch: > > * I added some tracepoints in the patch I'm applying but they don't seem > to be included in the trace output. This patch applies cleanly and I > haven't seen any compiler warnings about it. Is the > dwc3_ep0_handle_feature function not called during it's called from SetFeature() command. If SetFeature() is not sent by the host, then you won't see your tracepoints. FYI, SetFeature would have a bRequest of 3. Here we can all ctrl requests: 192: irq/23-dwc3-1765 [000] d..1 213.007591: dwc3_ctrl_req: bRequestType 00 bRequest 05 wValue 0005 wIndex 0000 wLength 0 208: irq/23-dwc3-1765 [000] d..1 213.010457: dwc3_ctrl_req: bRequestType 80 bRequest 06 wValue 0100 wIndex 0000 wLength 18 237: irq/23-dwc3-1765 [000] d..1 213.010871: dwc3_ctrl_req: bRequestType 80 bRequest 06 wValue 0304 wIndex 0409 wLength 2 266: irq/23-dwc3-1765 [000] d..1 213.011330: dwc3_ctrl_req: bRequestType 80 bRequest 06 wValue 0304 wIndex 0409 wLength 40 295: irq/23-dwc3-1765 [000] d..1 213.011707: dwc3_ctrl_req: bRequestType 80 bRequest 06 wValue 0303 wIndex 0409 wLength 2 324: irq/23-dwc3-1765 [000] d..1 213.012137: dwc3_ctrl_req: bRequestType 80 bRequest 06 wValue 0303 wIndex 0409 wLength 92 353: irq/23-dwc3-1765 [000] d..1 213.012548: dwc3_ctrl_req: bRequestType 80 bRequest 06 wValue 0305 wIndex 0409 wLength 2 382: irq/23-dwc3-1765 [000] d..1 213.012934: dwc3_ctrl_req: bRequestType 80 bRequest 06 wValue 0305 wIndex 0409 wLength 12 411: irq/23-dwc3-1765 [000] d..1 213.013331: dwc3_ctrl_req: bRequestType 80 bRequest 06 wValue 0f00 wIndex 0000 wLength 5 440: irq/23-dwc3-1765 [000] d..1 213.013748: dwc3_ctrl_req: bRequestType 80 bRequest 06 wValue 0f00 wIndex 0000 wLength 22 469: irq/23-dwc3-1765 [000] d..1 213.016006: dwc3_ctrl_req: bRequestType 80 bRequest 06 wValue 0200 wIndex 0000 wLength 9 498: irq/23-dwc3-1765 [000] d..1 213.016543: dwc3_ctrl_req: bRequestType 80 bRequest 06 wValue 0200 wIndex 0000 wLength 44 527: irq/23-dwc3-1765 [000] d..1 213.016951: dwc3_ctrl_req: bRequestType 00 bRequest 09 wValue 0001 wIndex 0000 wLength 0 568: irq/23-dwc3-1765 [000] d..1 213.017569: dwc3_ctrl_req: bRequestType 80 bRequest 06 wValue 0301 wIndex 0409 wLength 2 597: irq/23-dwc3-1765 [000] d..1 213.017926: dwc3_ctrl_req: bRequestType 80 bRequest 06 wValue 0301 wIndex 0409 wLength 26 626: irq/23-dwc3-1765 [000] d..1 213.018367: dwc3_ctrl_req: bRequestType 81 bRequest 00 wValue 0000 wIndex 0000 wLength 2 653: irq/23-dwc3-1765 [000] d..1 213.026404: dwc3_ctrl_req: bRequestType a1 bRequest fe wValue 0000 wIndex 0000 wLength 1 929: irq/23-dwc3-1765 [000] d..1 214.138385: dwc3_ctrl_req: bRequestType 82 bRequest 00 wValue 0000 wIndex 0081 wLength 2 956: irq/23-dwc3-1765 [000] d..1 214.138791: dwc3_ctrl_req: bRequestType 02 bRequest 01 wValue 0000 wIndex 0081 wLength 0 1031: irq/23-dwc3-1765 [000] d..1 214.140214: dwc3_ctrl_req: bRequestType 82 bRequest 00 wValue 0000 wIndex 0081 wLength 2 1058: irq/23-dwc3-1765 [000] d..1 214.140618: dwc3_ctrl_req: bRequestType 02 bRequest 01 wValue 0000 wIndex 0081 wLength 0 1133: irq/23-dwc3-1765 [000] d..1 214.142208: dwc3_ctrl_req: bRequestType 82 bRequest 00 wValue 0000 wIndex 0081 wLength 2 1160: irq/23-dwc3-1765 [000] d..1 214.142587: dwc3_ctrl_req: bRequestType 02 bRequest 01 wValue 0000 wIndex 0081 wLength 0 Note that we don't have any "bRequest 03" substring. > * I looked through the USB 3.0 Spec to find out more information about > this solution you've suggested and the relevant section is 9.4.9. This > section documents the "set feature" request. It specifies that a > device should return a STALL transaction packet when the request > references a feature that cannot be set. So does returning -EINVAL > in the dwc3_ep0_handle_feature function translate to a STALL packet > that gets sent to the host? And it's totally within spec for a device > to not accept U1/U2_ENABLE requests, right? That's correct. A Device can STALL a request for device-iniated U1/U2 entry. >> > I've attached the traces and dmesg output in an archive titled >> > "gnarbox-ep0out-failure.tar.xz". Please let me know if you need more >> > information about this particular issue. >> >> I'll go over the traces, this could be something that has been fixed in >> the past. > > Did you get a chance to take a look at the tracepoints for this error? > No worries if you haven't gotten a chance yet. I did, nothing screaming wrong with them. That "can't enable ep0out" still puzzles me a bit. What we see on tracepoints is a time out: modprobe-1281 [002] d..1 246.504043: dwc3_gadget_ep_cmd: ep0out: cmd 'Set Endpoint Transfer Resource' [2] params 00000001 00000000 00000000 --> status: Timed Out But I have no idea why that happens. It could be that this was fixed long ago, but we won't know unless we run a recent kernel on your setup. I, certainly, haven't seen that :-) -- balbi
Attachment:
signature.asc
Description: PGP signature
