Re: [PATCH] usb: dwc3: gadget: skip Set/Clear Halt when invalid
Hi, John Younwrites: >> Felipe Balbi writes: >>> At least macOS seems to be sending >>> ClearFeature(ENDPOINT_HALT) to endpoints which >>> aren't Halted. This makes DWC3's CLEARSTALL command >>> time out which causes several issues for the driver. >>> >>> Instead, let's just return 0 and bail out early. >>> >>> Cc: >>> Signed-off-by: Felipe Balbi >>> --- >>> >>> this falls into "has never worked before" category, so I'll be sending >>> it together with other patches for v4.11 merge window. Still, it's a >>> valid bug that's likely needed for stable trees. >>> >>> drivers/usb/dwc3/gadget.c | 5 + >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>> index 6faf484e5dfc..0a664d8eba3f 100644 >>> --- a/drivers/usb/dwc3/gadget.c >>> +++ b/drivers/usb/dwc3/gadget.c >>> @@ -1379,6 +1379,9 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, >>> int value, int protocol) >>> unsigned transfer_in_flight; >>> unsigned started; >>> >>> + if (dep->flags & DWC3_EP_STALL) >>> + return 0; >>> + >>> if (dep->number > 1) >>> trb = dwc3_ep_prev_trb(dep, dep->trb_enqueue); >>> else >>> @@ -1400,6 +1403,8 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, >>> int value, int protocol) >>> else >>> dep->flags |= DWC3_EP_STALL; >>> } else { >>> + if (!(dep->flags & DWC3_EP_STALL)) >>> + return 0; >>> >>> ret = dwc3_send_clear_stall_ep_cmd(dep); >>> if (ret) >> >> Reviving this old thread here. While $subject allowed dwc3 to work when >> attached to macOS Host, I fear that we might have more issues than not >> in the future. The reason is that USB20 spec allows hosts to use >> ClearFeature(ENDPOINT_HALT) as a "Reset Data Toggle/SeqN" hint. >> >> With this, we're basically blocking that possibility. Still, without >> $subject, ClearStall commands were timing out. I'll try to do a local >> revert here and check what happens in this case, but would you have any >> idea why ClearStall would time out like that? >> > > Hi Felipe, > > I think Thinh Nguyen e-mailed you about this before saying it caused a > regression for us. But he has not had time to look into it further and > follow-up yet. > > No idea about the timing out on Mac though. We can try to reproduce > this when we have some time and take a look. Do you have a USB trace? not anymore, but as soon I have some free time, I can revert the patch locally and try to reproduce. I'll also implement a little libusb-based test to issue that request hundreds of times in a row and see if I can force the problem to happen ;-) Thanks for responding so quickly :-) -- balbi -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: gadget: skip Set/Clear Halt when invalid
On 04/04/2017 01:09 AM, Felipe Balbi wrote: > > Hi John, > > Felipe Balbiwrites: >> At least macOS seems to be sending >> ClearFeature(ENDPOINT_HALT) to endpoints which >> aren't Halted. This makes DWC3's CLEARSTALL command >> time out which causes several issues for the driver. >> >> Instead, let's just return 0 and bail out early. >> >> Cc: >> Signed-off-by: Felipe Balbi >> --- >> >> this falls into "has never worked before" category, so I'll be sending >> it together with other patches for v4.11 merge window. Still, it's a >> valid bug that's likely needed for stable trees. >> >> drivers/usb/dwc3/gadget.c | 5 + >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 6faf484e5dfc..0a664d8eba3f 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -1379,6 +1379,9 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int >> value, int protocol) >> unsigned transfer_in_flight; >> unsigned started; >> >> +if (dep->flags & DWC3_EP_STALL) >> +return 0; >> + >> if (dep->number > 1) >> trb = dwc3_ep_prev_trb(dep, dep->trb_enqueue); >> else >> @@ -1400,6 +1403,8 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int >> value, int protocol) >> else >> dep->flags |= DWC3_EP_STALL; >> } else { >> +if (!(dep->flags & DWC3_EP_STALL)) >> +return 0; >> >> ret = dwc3_send_clear_stall_ep_cmd(dep); >> if (ret) > > Reviving this old thread here. While $subject allowed dwc3 to work when > attached to macOS Host, I fear that we might have more issues than not > in the future. The reason is that USB20 spec allows hosts to use > ClearFeature(ENDPOINT_HALT) as a "Reset Data Toggle/SeqN" hint. > > With this, we're basically blocking that possibility. Still, without > $subject, ClearStall commands were timing out. I'll try to do a local > revert here and check what happens in this case, but would you have any > idea why ClearStall would time out like that? > Hi Felipe, I think Thinh Nguyen e-mailed you about this before saying it caused a regression for us. But he has not had time to look into it further and follow-up yet. No idea about the timing out on Mac though. We can try to reproduce this when we have some time and take a look. Do you have a USB trace? Regards, John -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: gadget: skip Set/Clear Halt when invalid
Hi John, Felipe Balbiwrites: > At least macOS seems to be sending > ClearFeature(ENDPOINT_HALT) to endpoints which > aren't Halted. This makes DWC3's CLEARSTALL command > time out which causes several issues for the driver. > > Instead, let's just return 0 and bail out early. > > Cc: > Signed-off-by: Felipe Balbi > --- > > this falls into "has never worked before" category, so I'll be sending > it together with other patches for v4.11 merge window. Still, it's a > valid bug that's likely needed for stable trees. > > drivers/usb/dwc3/gadget.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 6faf484e5dfc..0a664d8eba3f 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -1379,6 +1379,9 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int > value, int protocol) > unsigned transfer_in_flight; > unsigned started; > > + if (dep->flags & DWC3_EP_STALL) > + return 0; > + > if (dep->number > 1) > trb = dwc3_ep_prev_trb(dep, dep->trb_enqueue); > else > @@ -1400,6 +1403,8 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int > value, int protocol) > else > dep->flags |= DWC3_EP_STALL; > } else { > + if (!(dep->flags & DWC3_EP_STALL)) > + return 0; > > ret = dwc3_send_clear_stall_ep_cmd(dep); > if (ret) Reviving this old thread here. While $subject allowed dwc3 to work when attached to macOS Host, I fear that we might have more issues than not in the future. The reason is that USB20 spec allows hosts to use ClearFeature(ENDPOINT_HALT) as a "Reset Data Toggle/SeqN" hint. With this, we're basically blocking that possibility. Still, without $subject, ClearStall commands were timing out. I'll try to do a local revert here and check what happens in this case, but would you have any idea why ClearStall would time out like that? -- balbi signature.asc Description: PGP signature
RE: [PATCH] usb: dwc3: gadget: skip Set/Clear Halt when invalid
Hi, Felipe Balbiwrites: >>> I've been looking at this and based on sniffer and dwc3 tracepoints, it >>> seems like dwc3 is behaving properly. The real issue seems to be that >>> g_mass_storage isn't queueing a new request to IN endpoint. >>> >>> I'll continue debugging this and try to find a solution that doesn't >>> involve reverting $subject. >> >> oh no, wait. ep2out misses XferInProgress: >> >> file-storage-1592 [000] d..1 152.809922: dwc3_ep_queue: ep2out: req >> 88003cd6ee40 length 0/512 zsI ==> -115 >> file-storage-1592 [000] d..1 152.809931: dwc3_prepare_trb: ep2out: >> 3/8 trb 88003a196050 buf 2d5e4000 size 512 ctrl 0819 >> (HlcS:sC:normal) >> file-storage-1592 [000] d..1 152.809942: dwc3_gadget_ep_cmd: ep2out: >> cmd 'Update Transfer' [262151] params --> status: >> Successful >> file-storage-1592 [000] 152.809951: usb_ep_queue: ep2out: length >> 0/512 sgs 0/0 stream 0 zsI status -115 --> 0 >> irq/34-dwc3-1593 [001] d..1 152.810212: dwc3_event: event >> (c040): ep0out: Transfer Complete [Setup Phase] >> irq/34-dwc3-1593 [001] d..1 152.810218: dwc3_ctrl_req: bRequestType >> 02 bRequest 01 wValue wIndex 0002 wLength 0 >> irq/34-dwc3-1593 [001] d..1 152.810228: __dwc3_gadget_ep_set_halt: >> ep2out: NOT stalled >> >> Sniffer shows me this completing, but I don't see IRQ for this. > > BTW, I just tested without $subject and it fails the same way. This is > caused by something else. Can you rerun your bisect while I look at the > problem here? Okay, found it. This is caused by the ep_dequeue bug that I already fixed. see [1] for that [1] https://marc.info/?i=20170217105759.24356-1-felipe.ba...@linux.intel.com -- balbi signature.asc Description: PGP signature
RE: [PATCH] usb: dwc3: gadget: skip Set/Clear Halt when invalid
Hi, Felipe Balbiwrites: >> Thinh Nguyen writes: drivers/usb/dwc3/gadget.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 6faf484e5dfc..0a664d8eba3f 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1379,6 +1379,9 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol) unsigned transfer_in_flight; unsigned started; + if (dep->flags & DWC3_EP_STALL) + return 0; + if (dep->number > 1) trb = dwc3_ep_prev_trb(dep, dep->trb_enqueue); else @@ -1400,6 +1403,8 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol) else dep->flags |= DWC3_EP_STALL; } else { + if (!(dep->flags & DWC3_EP_STALL)) + return 0; ret = dwc3_send_clear_stall_ep_cmd(dep); if (ret) -- 2.11.0.295.gd7dffce1ce -- >>> >>> I encounter an issue when I test mainline with USB 3 CV MSC test and >>> bisected to this patch. >>> >>> Tester: USB 3 CV test (v2.1.3.0). >>> Function Driver: f_mass_storage >>> Reproducibility: always >>> >>> The failure occurs under MSC Error Recovery Test (TD 1.4). >>> >>> Failure from the test sequence as follow (MSC compliance test spec) : >>> 1. Place the device in the desired starting state. >>> 2. Issue a Case 9 CBW (see Table 2) but with invalid signature of >>> 0xDEADBEEF. >>> 3. Issue several In requests to the Bulk-Only Data Interface Bulk In >>> endpoint. Verify a >>> STALL handshake is returned. If STALL handshake is not returned, skip to >>> step 11. >>> 4. Issue a Get_Status(endpoint) request targeting the Bulk-Only Data >>> Interface bulk In >>> endpoint. Verify that it completes normally, reports endpoint halt status. >>> >>> ***After this point the device idles for 10 seconds and resets. *** >>> >>> Test fails. >> >> I've been looking at this and based on sniffer and dwc3 tracepoints, it >> seems like dwc3 is behaving properly. The real issue seems to be that >> g_mass_storage isn't queueing a new request to IN endpoint. >> >> I'll continue debugging this and try to find a solution that doesn't >> involve reverting $subject. > > oh no, wait. ep2out misses XferInProgress: > > file-storage-1592 [000] d..1 152.809922: dwc3_ep_queue: ep2out: req > 88003cd6ee40 length 0/512 zsI ==> -115 > file-storage-1592 [000] d..1 152.809931: dwc3_prepare_trb: ep2out: 3/8 > trb 88003a196050 buf 2d5e4000 size 512 ctrl 0819 > (HlcS:sC:normal) > file-storage-1592 [000] d..1 152.809942: dwc3_gadget_ep_cmd: ep2out: > cmd 'Update Transfer' [262151] params --> status: > Successful > file-storage-1592 [000] 152.809951: usb_ep_queue: ep2out: length > 0/512 sgs 0/0 stream 0 zsI status -115 --> 0 > irq/34-dwc3-1593 [001] d..1 152.810212: dwc3_event: event (c040): > ep0out: Transfer Complete [Setup Phase] > irq/34-dwc3-1593 [001] d..1 152.810218: dwc3_ctrl_req: bRequestType > 02 bRequest 01 wValue wIndex 0002 wLength 0 > irq/34-dwc3-1593 [001] d..1 152.810228: __dwc3_gadget_ep_set_halt: > ep2out: NOT stalled > > Sniffer shows me this completing, but I don't see IRQ for this. BTW, I just tested without $subject and it fails the same way. This is caused by something else. Can you rerun your bisect while I look at the problem here? -- balbi signature.asc Description: PGP signature
RE: [PATCH] usb: dwc3: gadget: skip Set/Clear Halt when invalid
Hi Thinh, Thinh Nguyenwrites: >> drivers/usb/dwc3/gadget.c | 5 + >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 6faf484e5dfc..0a664d8eba3f 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -1379,6 +1379,9 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep >> *dep, int value, int protocol) >> unsigned transfer_in_flight; >> unsigned started; >> >> +if (dep->flags & DWC3_EP_STALL) >> +return 0; >> + >> if (dep->number > 1) >> trb = dwc3_ep_prev_trb(dep, dep->trb_enqueue); >> else >> @@ -1400,6 +1403,8 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep >> *dep, int value, int protocol) >> else >> dep->flags |= DWC3_EP_STALL; >> } else { >> +if (!(dep->flags & DWC3_EP_STALL)) >> +return 0; >> >> ret = dwc3_send_clear_stall_ep_cmd(dep); >> if (ret) >> -- >> 2.11.0.295.gd7dffce1ce >> >> -- > > I encounter an issue when I test mainline with USB 3 CV MSC test and > bisected to this patch. > > Tester: USB 3 CV test (v2.1.3.0). > Function Driver: f_mass_storage > Reproducibility: always > > The failure occurs under MSC Error Recovery Test (TD 1.4). > > Failure from the test sequence as follow (MSC compliance test spec) : > 1. Place the device in the desired starting state. > 2. Issue a Case 9 CBW (see Table 2) but with invalid signature of 0xDEADBEEF. > 3. Issue several In requests to the Bulk-Only Data Interface Bulk In > endpoint. Verify a > STALL handshake is returned. If STALL handshake is not returned, skip to step > 11. > 4. Issue a Get_Status(endpoint) request targeting the Bulk-Only Data > Interface bulk In > endpoint. Verify that it completes normally, reports endpoint halt status. > > ***After this point the device idles for 10 seconds and resets. *** > > Test fails. I've been looking at this and based on sniffer and dwc3 tracepoints, it seems like dwc3 is behaving properly. The real issue seems to be that g_mass_storage isn't queueing a new request to IN endpoint. I'll continue debugging this and try to find a solution that doesn't involve reverting $subject. -- balbi signature.asc Description: PGP signature
RE: [PATCH] usb: dwc3: gadget: skip Set/Clear Halt when invalid
Felipe Balbiwrites: > Hi Thinh, > > Thinh Nguyen writes: >>> drivers/usb/dwc3/gadget.c | 5 + >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>> index 6faf484e5dfc..0a664d8eba3f 100644 >>> --- a/drivers/usb/dwc3/gadget.c >>> +++ b/drivers/usb/dwc3/gadget.c >>> @@ -1379,6 +1379,9 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep >>> *dep, int value, int protocol) >>> unsigned transfer_in_flight; >>> unsigned started; >>> >>> + if (dep->flags & DWC3_EP_STALL) >>> + return 0; >>> + >>> if (dep->number > 1) >>> trb = dwc3_ep_prev_trb(dep, dep->trb_enqueue); >>> else >>> @@ -1400,6 +1403,8 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep >>> *dep, int value, int protocol) >>> else >>> dep->flags |= DWC3_EP_STALL; >>> } else { >>> + if (!(dep->flags & DWC3_EP_STALL)) >>> + return 0; >>> >>> ret = dwc3_send_clear_stall_ep_cmd(dep); >>> if (ret) >>> -- >>> 2.11.0.295.gd7dffce1ce >>> >>> -- >> >> I encounter an issue when I test mainline with USB 3 CV MSC test and >> bisected to this patch. >> >> Tester: USB 3 CV test (v2.1.3.0). >> Function Driver: f_mass_storage >> Reproducibility: always >> >> The failure occurs under MSC Error Recovery Test (TD 1.4). >> >> Failure from the test sequence as follow (MSC compliance test spec) : >> 1. Place the device in the desired starting state. >> 2. Issue a Case 9 CBW (see Table 2) but with invalid signature of 0xDEADBEEF. >> 3. Issue several In requests to the Bulk-Only Data Interface Bulk In >> endpoint. Verify a >> STALL handshake is returned. If STALL handshake is not returned, skip to >> step 11. >> 4. Issue a Get_Status(endpoint) request targeting the Bulk-Only Data >> Interface bulk In >> endpoint. Verify that it completes normally, reports endpoint halt status. >> >> ***After this point the device idles for 10 seconds and resets. *** >> >> Test fails. > > I've been looking at this and based on sniffer and dwc3 tracepoints, it > seems like dwc3 is behaving properly. The real issue seems to be that > g_mass_storage isn't queueing a new request to IN endpoint. > > I'll continue debugging this and try to find a solution that doesn't > involve reverting $subject. oh no, wait. ep2out misses XferInProgress: file-storage-1592 [000] d..1 152.809922: dwc3_ep_queue: ep2out: req 88003cd6ee40 length 0/512 zsI ==> -115 file-storage-1592 [000] d..1 152.809931: dwc3_prepare_trb: ep2out: 3/8 trb 88003a196050 buf 2d5e4000 size 512 ctrl 0819 (HlcS:sC:normal) file-storage-1592 [000] d..1 152.809942: dwc3_gadget_ep_cmd: ep2out: cmd 'Update Transfer' [262151] params --> status: Successful file-storage-1592 [000] 152.809951: usb_ep_queue: ep2out: length 0/512 sgs 0/0 stream 0 zsI status -115 --> 0 irq/34-dwc3-1593 [001] d..1 152.810212: dwc3_event: event (c040): ep0out: Transfer Complete [Setup Phase] irq/34-dwc3-1593 [001] d..1 152.810218: dwc3_ctrl_req: bRequestType 02 bRequest 01 wValue wIndex 0002 wLength 0 irq/34-dwc3-1593 [001] d..1 152.810228: __dwc3_gadget_ep_set_halt: ep2out: NOT stalled Sniffer shows me this completing, but I don't see IRQ for this. -- balbi signature.asc Description: PGP signature
[PATCH] usb: dwc3: gadget: skip Set/Clear Halt when invalid
At least macOS seems to be sending ClearFeature(ENDPOINT_HALT) to endpoints which aren't Halted. This makes DWC3's CLEARSTALL command time out which causes several issues for the driver. Instead, let's just return 0 and bail out early. Cc:Signed-off-by: Felipe Balbi --- this falls into "has never worked before" category, so I'll be sending it together with other patches for v4.11 merge window. Still, it's a valid bug that's likely needed for stable trees. drivers/usb/dwc3/gadget.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 6faf484e5dfc..0a664d8eba3f 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1379,6 +1379,9 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol) unsigned transfer_in_flight; unsigned started; + if (dep->flags & DWC3_EP_STALL) + return 0; + if (dep->number > 1) trb = dwc3_ep_prev_trb(dep, dep->trb_enqueue); else @@ -1400,6 +1403,8 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol) else dep->flags |= DWC3_EP_STALL; } else { + if (!(dep->flags & DWC3_EP_STALL)) + return 0; ret = dwc3_send_clear_stall_ep_cmd(dep); if (ret) -- 2.11.0.295.gd7dffce1ce -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html