| Message ID | 1444834590-25759-1-git-send-email-jassisinghbrar@gmail.com |
|---|---|
| State | New |
| Headers | show |
Hi, jaswinder.singh@linaro.org writes: > From: Jassi Brar <jaswinder.singh@linaro.org> > > We must return 0 for any OUT setup request, otherwise > protocol error may occur. have you seen any such errors ? composite.c treats >=0 as success: if (value >= 0 && value != USB_GADGET_DELAYED_STATUS) { req->length = value; req->context = cdev; req->zero = value < w_length; value = composite_ep0_queue(cdev, req, GFP_ATOMIC); if (value < 0) { DBG(cdev, "ep_queue --> %d\n", value); req->status = 0; composite_setup_complete(gadget->ep0, req); } } else if .... We actually NEED to return w_length to cope with the need for ZLPs. Care to describe what problems you have seen and which UDC you were using, what's the exact scenario. How have you tested this ?
On Wed, Oct 14, 2015 at 8:42 PM, Felipe Balbi <balbi@ti.com> wrote: > > Hi, > > jaswinder.singh@linaro.org writes: >> From: Jassi Brar <jaswinder.singh@linaro.org> >> >> We must return 0 for any OUT setup request, otherwise >> protocol error may occur. > > have you seen any such errors ? > Yes. While testing my new udc driver. > Care to describe what problems you have seen and which UDC you were using, > what's the exact scenario. How have you tested this ? > The udc I am writing the driver for is not yet public. To test my driver at lowest level possible, I use 'usbmon' to capture and analyze traffic since I don't have any hardware probe. I see the following on gadget side ------- configfs-gadget gadget: non-core control req21.20 v0000 i0001 l7 configfs-gadget gadget: acm ttyGS0 req21.20 v0000 i0001 l7 configfs-gadget gadget: acm ttyGS0 completion, err -71 ------- and the following on host side usbmon capture ------- ffff88041ed01f00 540296433 S Co:3:023:0 s 21 20 0000 0001 0007 7 = 80250000 000008 ffff88041ed01f00 540296790 C Co:3:023:0 -71 0 ------- Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Jassi Brar <jassisinghbrar@gmail.com> writes: > On Wed, Oct 14, 2015 at 8:42 PM, Felipe Balbi <balbi@ti.com> wrote: >> >> Hi, >> >> jaswinder.singh@linaro.org writes: >>> From: Jassi Brar <jaswinder.singh@linaro.org> >>> >>> We must return 0 for any OUT setup request, otherwise >>> protocol error may occur. >> >> have you seen any such errors ? >> > Yes. While testing my new udc driver. > > >> Care to describe what problems you have seen and which UDC you were using, >> what's the exact scenario. How have you tested this ? >> > The udc I am writing the driver for is not yet public. To test my > driver at lowest level possible, I use 'usbmon' to capture and analyze > traffic since I don't have any hardware probe. > > I see the following on gadget side > ------- > configfs-gadget gadget: non-core control req21.20 v0000 i0001 l7 > configfs-gadget gadget: acm ttyGS0 req21.20 v0000 i0001 l7 > configfs-gadget gadget: acm ttyGS0 completion, err -71 > ------- > > and the following on host side usbmon capture > ------- > ffff88041ed01f00 540296433 S Co:3:023:0 s 21 20 0000 0001 0007 7 = > 80250000 000008 > ffff88041ed01f00 540296790 C Co:3:023:0 -71 0 > ------- and you did you even consider this could be a bug in your new UDC driver at all ? This works fine with other UDCs. How far are you in developing this new UDC driver ? Did you run USBCV at all ? Are you sure you're implementing EP0 handling correctly ? What sort of tests have you done with this UDC ? Is it working with testusb against a known good host (EHCI and XHCI should be good for that) ?
On Wed, Oct 14, 2015 at 9:18 PM, Felipe Balbi <balbi@ti.com> wrote: > > Hi, > > Jassi Brar <jassisinghbrar@gmail.com> writes: >> On Wed, Oct 14, 2015 at 8:42 PM, Felipe Balbi <balbi@ti.com> wrote: >>> >>> Hi, >>> >>> jaswinder.singh@linaro.org writes: >>>> From: Jassi Brar <jaswinder.singh@linaro.org> >>>> >>>> We must return 0 for any OUT setup request, otherwise >>>> protocol error may occur. >>> >>> have you seen any such errors ? >>> >> Yes. While testing my new udc driver. >> >> >>> Care to describe what problems you have seen and which UDC you were using, >>> what's the exact scenario. How have you tested this ? >>> >> The udc I am writing the driver for is not yet public. To test my >> driver at lowest level possible, I use 'usbmon' to capture and analyze >> traffic since I don't have any hardware probe. >> >> I see the following on gadget side >> ------- >> configfs-gadget gadget: non-core control req21.20 v0000 i0001 l7 >> configfs-gadget gadget: acm ttyGS0 req21.20 v0000 i0001 l7 >> configfs-gadget gadget: acm ttyGS0 completion, err -71 >> ------- >> >> and the following on host side usbmon capture >> ------- >> ffff88041ed01f00 540296433 S Co:3:023:0 s 21 20 0000 0001 0007 7 = >> 80250000 000008 >> ffff88041ed01f00 540296790 C Co:3:023:0 -71 0 >> ------- > > and you did you even consider this could be a bug in your new UDC driver > at all ? This works fine with other UDCs. > I was happy too until I decided to look closely at the annoying print on gadget side. This is a non-fatal error and visible only when debugging is enabled, so every udc seems 'fine' > How far are you in developing this new UDC driver ? > Its done and tested for fsg+acm composite and each alone. > Did you run USBCV at all ? > No USBCV not yet. I borrowed Windows machine to test FSG but forgot USBCV since it's been years I wrote last udc driver. Will give it a try tomorrow but I don't think it could emulate the sequence I hit with f_acm. > Are you sure you're implementing EP0 handling correctly ? What > sort of tests have you done with this UDC ? Is it working with testusb > against a known good host (EHCI and XHCI should be good for that) ? > Well as I said I have been looking at low level transfers and everything is good. BTW, should the gadget stack ever queue a Non-ZLP as reply to some setup request that has USB_DIR_IN not set? -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Jassi Brar <jassisinghbrar@gmail.com> writes: > On Wed, Oct 14, 2015 at 9:18 PM, Felipe Balbi <balbi@ti.com> wrote: >> >> Hi, >> >> Jassi Brar <jassisinghbrar@gmail.com> writes: >>> On Wed, Oct 14, 2015 at 8:42 PM, Felipe Balbi <balbi@ti.com> wrote: >>>> >>>> Hi, >>>> >>>> jaswinder.singh@linaro.org writes: >>>>> From: Jassi Brar <jaswinder.singh@linaro.org> >>>>> >>>>> We must return 0 for any OUT setup request, otherwise >>>>> protocol error may occur. >>>> >>>> have you seen any such errors ? >>>> >>> Yes. While testing my new udc driver. >>> >>> >>>> Care to describe what problems you have seen and which UDC you were using, >>>n> what's the exact scenario. How have you tested this ? >>>> >>> The udc I am writing the driver for is not yet public. To test my >>> driver at lowest level possible, I use 'usbmon' to capture and analyze >>> traffic since I don't have any hardware probe. >>> >>> I see the following on gadget side >>> ------- >>> configfs-gadget gadget: non-core control req21.20 v0000 i0001 l7 >>> configfs-gadget gadget: acm ttyGS0 req21.20 v0000 i0001 l7 >>> configfs-gadget gadget: acm ttyGS0 completion, err -71 >>> ------- >>> >>> and the following on host side usbmon capture >>> ------- >>> ffff88041ed01f00 540296433 S Co:3:023:0 s 21 20 0000 0001 0007 7 = >>> 80250000 000008 >>> ffff88041ed01f00 540296790 C Co:3:023:0 -71 0 >>> ------- >> >> and you did you even consider this could be a bug in your new UDC driver >> at all ? This works fine with other UDCs. >> > I was happy too until I decided to look closely at the annoying print > on gadget side. This is a non-fatal error and visible only when > debugging is enabled, so every udc seems 'fine' I tried with my sniffer and saw no stalls, nothing. My setup request to set line coding to 9600 baud worked just fine. >> How far are you in developing this new UDC driver ? >> > Its done and tested for fsg+acm composite and each alone. stress tests ? Meaning, did you leave it running for a couple of weeks at least ? >> Did you run USBCV at all ? >> > No USBCV not yet. I borrowed Windows machine to test FSG but forgot > USBCV since it's been years I wrote last udc driver. Will give it a > try tomorrow but I don't think it could emulate the sequence I hit > with f_acm. USBCV might already have some ACM test cases and it should exercise all mandatory setup requests. >> Are you sure you're implementing EP0 handling correctly ? What >> sort of tests have you done with this UDC ? Is it working with testusb >> against a known good host (EHCI and XHCI should be good for that) ? >> > Well as I said I have been looking at low level transfers and > everything is good. still, run testusb with the acompanying shell script and keep it running for at least 2 weeks. > BTW, should the gadget stack ever queue a Non-ZLP as reply to some > setup request that has USB_DIR_IN not set? What phase of the setup are we talking about ? If it has a DATA phase, then data can have non-ZLP transfers and it can also require a ZLP to end the data phase itself (if wLength % wMaxPacketSize == 0). Status phase is always zlp.
On Wed, 14 Oct 2015, Jassi Brar wrote: > BTW, should the gadget stack ever queue a Non-ZLP as reply to some > setup request that has USB_DIR_IN not set? Yes. If USB_DIR_IN is not set then the control transfer is OUT, so the gadget needs to queue a request to receive some data from the host. That request will obviously need to be a non-ZLP. In fact, it's hard to think of a situation where a gadget would ever want to submit a zero-length OUT request. Isn't the UDC driver supposed to handle the status stage of a control-IN transfer automatically? Could this cause the problem you're seeing? The host tries to send more data than the gadget is ready to receive? (Although then the error code on the gadget side should be -75, not -71.) Alan Stern -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Alan Stern <stern@rowland.harvard.edu> writes: > On Wed, 14 Oct 2015, Jassi Brar wrote: > >> BTW, should the gadget stack ever queue a Non-ZLP as reply to some >> setup request that has USB_DIR_IN not set? > > Yes. If USB_DIR_IN is not set then the control transfer is OUT, so the > gadget needs to queue a request to receive some data from the host. > That request will obviously need to be a non-ZLP. In fact, it's hard > to think of a situation where a gadget would ever want to submit a > zero-length OUT request. Isn't the UDC driver supposed to handle the > status stage of a control-IN transfer automatically? yes and no. :-) If USB_GADGET_DELAYED_STATUS is returned, we need to wait for the gadget driver to queue a request.
On Wed, 14 Oct 2015, Felipe Balbi wrote: > Hi, > > Alan Stern <stern@rowland.harvard.edu> writes: > > On Wed, 14 Oct 2015, Jassi Brar wrote: > > > >> BTW, should the gadget stack ever queue a Non-ZLP as reply to some > >> setup request that has USB_DIR_IN not set? > > > > Yes. If USB_DIR_IN is not set then the control transfer is OUT, so the > > gadget needs to queue a request to receive some data from the host. > > That request will obviously need to be a non-ZLP. In fact, it's hard > > to think of a situation where a gadget would ever want to submit a > > zero-length OUT request. Isn't the UDC driver supposed to handle the > > status stage of a control-IN transfer automatically? > > yes and no. :-) If USB_GADGET_DELAYED_STATUS is returned, we need to > wait for the gadget driver to queue a request. USB_GADGET_DELAYED_STATUS is used with control-OUT transfers that don't have a data stage. I was speaking of control-IN, because the status stage for a control-IN transfer is a zero-length OUT transaction. But speaking of DELAYED_STATUS, there is a long-standing weakness in the Gadget API. When a gadget receives data from the host in a control-OUT transfer, the API doesn't provide any way for the gadget to delay the status stage until its processing is complete. USB_GADGET_DELAYED_STATUS only works as a return value from the setup routine; once the data stage has started there's no way to delay the status stage. It's an unfortunate oversight in the original design. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 14, 2015 at 10:40 PM, Felipe Balbi <balbi@ti.com> wrote: > > Hi, > > Jassi Brar <jassisinghbrar@gmail.com> writes: >> On Wed, Oct 14, 2015 at 9:18 PM, Felipe Balbi <balbi@ti.com> wrote: >>> >>> Hi, >>> >>> Jassi Brar <jassisinghbrar@gmail.com> writes: >>>> On Wed, Oct 14, 2015 at 8:42 PM, Felipe Balbi <balbi@ti.com> wrote: >>>>> >>>>> Hi, >>>>> >>>>> jaswinder.singh@linaro.org writes: >>>>>> From: Jassi Brar <jaswinder.singh@linaro.org> >>>>>> >>>>>> We must return 0 for any OUT setup request, otherwise >>>>>> protocol error may occur. >>>>> >>>>> have you seen any such errors ? >>>>> >>>> Yes. While testing my new udc driver. >>>> >>>> >>>>> Care to describe what problems you have seen and which UDC you were using, >>>>n> what's the exact scenario. How have you tested this ? >>>>> >>>> The udc I am writing the driver for is not yet public. To test my >>>> driver at lowest level possible, I use 'usbmon' to capture and analyze >>>> traffic since I don't have any hardware probe. >>>> >>>> I see the following on gadget side >>>> ------- >>>> configfs-gadget gadget: non-core control req21.20 v0000 i0001 l7 >>>> configfs-gadget gadget: acm ttyGS0 req21.20 v0000 i0001 l7 >>>> configfs-gadget gadget: acm ttyGS0 completion, err -71 >>>> ------- >>>> >>>> and the following on host side usbmon capture >>>> ------- >>>> ffff88041ed01f00 540296433 S Co:3:023:0 s 21 20 0000 0001 0007 7 = >>>> 80250000 000008 >>>> ffff88041ed01f00 540296790 C Co:3:023:0 -71 0 >>>> ------- >>> >>> and you did you even consider this could be a bug in your new UDC driver >>> at all ? This works fine with other UDCs. >>> >> I was happy too until I decided to look closely at the annoying print >> on gadget side. This is a non-fatal error and visible only when >> debugging is enabled, so every udc seems 'fine' > > I tried with my sniffer and saw no stalls, nothing. My setup request to > set line coding to 9600 baud worked just fine. > I am sure your udc driver will (need to) track stages of a transfer. If you put some print in usb_ep_ops.queue() you will notice the stack queues 7byte transfer but the udc driver silently drops it and send a zlp here. I can move the change into my driver as well, but the point is gadget should never try to _send_ non-zlp as reply to control-OUT command. >>> How far are you in developing this new UDC driver ? >>> >> Its done and tested for fsg+acm composite and each alone. > > stress tests ? Meaning, did you leave it running for a couple of weeks > at least ? > I know I can't stress test enough, but this bug is 100% hit and staring in the eye at protocol level. >> BTW, should the gadget stack ever queue a Non-ZLP as reply to some >> setup request that has USB_DIR_IN not set? > > What phase of the setup are we talking about ? > I said "_reply_ to setup request" so I meant status phase. UDC driver receives the SETUP command as '21 20 00 00 01 00 07 00', passes it onto composite, which hands it over to acm and which pretends we need to return a 7byte packet to host (value = w_length = 7). -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 14, 2015 at 11:18 PM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Wed, 14 Oct 2015, Jassi Brar wrote: > >> BTW, should the gadget stack ever queue a Non-ZLP as reply to some >> setup request that has USB_DIR_IN not set? > > Yes. If USB_DIR_IN is not set then the control transfer is OUT, so the > gadget needs to queue a request to receive some data from the host. > That request will obviously need to be a non-ZLP. > By 'reply' I meant after reading out and parsing the setup(control-out) request. I am sure we need to send a ZLP. > Could this cause the problem you're seeing? The host tries to send > more data than the gadget is ready to receive? (Although then the > error code on the gadget side should be -75, not -71.) > Thanks, but as you said my problem is not that (I get protocol error -71). My problem is my udc driver actually tries to send a 7 byte response to a control-out command. -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 15 Oct 2015, Jassi Brar wrote: > On Wed, Oct 14, 2015 at 11:18 PM, Alan Stern <stern@rowland.harvard.edu> wrote: > > On Wed, 14 Oct 2015, Jassi Brar wrote: > > > >> BTW, should the gadget stack ever queue a Non-ZLP as reply to some > >> setup request that has USB_DIR_IN not set? > > > > Yes. If USB_DIR_IN is not set then the control transfer is OUT, so the > > gadget needs to queue a request to receive some data from the host. > > That request will obviously need to be a non-ZLP. > > > By 'reply' I meant after reading out and parsing the > setup(control-out) request. I am sure we need to send a ZLP. You're wrong. Consider what happens when the host wants to send 7 bytes of data to the gadget using a control-OUT transfer: The gadget receives a SETUP packet. The USB_DIR_IN bit is clear because this is an OUT transfer, and wLength is set to 7. Which is what you got, right? Next, the host will send the 7-byte data packet. The gadget has to prepare to receive it, and it does so by submitting a 7-byte OUT request to ep0. This happens within the setup handler. The data packet is sent and the gadget receives it. The status stage for this transfer consists of a 0-length IN transaction, which the UDC driver automatically queues as soon as the completion routine for the data packet returns. The gadget driver isn't involved in the status stage (unfortunately). > > Could this cause the problem you're seeing? The host tries to send > > more data than the gadget is ready to receive? (Although then the > > error code on the gadget side should be -75, not -71.) > > > Thanks, but as you said my problem is not that (I get protocol error > -71). My problem is my udc driver actually tries to send a 7 byte > response to a control-out command. It shouldn't try to _send_ a 7-byte response; it should try to _receive_ a 7-byte data packet. This is, after all, an OUT transfer: host -> gadget. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 15, 2015 at 7:59 PM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Thu, 15 Oct 2015, Jassi Brar wrote: > >> On Wed, Oct 14, 2015 at 11:18 PM, Alan Stern <stern@rowland.harvard.edu> wrote: >> > On Wed, 14 Oct 2015, Jassi Brar wrote: >> > >> >> BTW, should the gadget stack ever queue a Non-ZLP as reply to some >> >> setup request that has USB_DIR_IN not set? >> > >> > Yes. If USB_DIR_IN is not set then the control transfer is OUT, so the >> > gadget needs to queue a request to receive some data from the host. >> > That request will obviously need to be a non-ZLP. >> > >> By 'reply' I meant after reading out and parsing the >> setup(control-out) request. I am sure we need to send a ZLP. > > You're wrong. Consider what happens when the host wants to send 7 > bytes of data to the gadget using a control-OUT transfer: > > The gadget receives a SETUP packet. The USB_DIR_IN bit is > clear because this is an OUT transfer, and wLength is set to 7. > Which is what you got, right? > > Next, the host will send the 7-byte data packet. The gadget > has to prepare to receive it, and it does so by submitting a > 7-byte OUT request to ep0. This happens within the setup > handler. > > The data packet is sent and the gadget receives it. The status > stage for this transfer consists of a 0-length IN transaction, > I have been referring to this "0-length IN transaction" when I said "need to send a zlp". > which the UDC driver automatically queues as soon as the > completion routine for the data packet returns. The gadget > driver isn't involved in the status stage (unfortunately). > OK, so that is a known 'feature', not a bug in gadget drivers as I have been calling it. I was trying to make the f_acm.c send that "0-length IN transaction" by making 'value=0' Thanks -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 15 Oct 2015, Jassi Brar wrote: > On Thu, Oct 15, 2015 at 7:59 PM, Alan Stern <stern@rowland.harvard.edu> wrote: > > On Thu, 15 Oct 2015, Jassi Brar wrote: > > > >> On Wed, Oct 14, 2015 at 11:18 PM, Alan Stern <stern@rowland.harvard.edu> wrote: > >> > On Wed, 14 Oct 2015, Jassi Brar wrote: > >> > > >> >> BTW, should the gadget stack ever queue a Non-ZLP as reply to some > >> >> setup request that has USB_DIR_IN not set? > >> > > >> > Yes. If USB_DIR_IN is not set then the control transfer is OUT, so the > >> > gadget needs to queue a request to receive some data from the host. > >> > That request will obviously need to be a non-ZLP. > >> > > >> By 'reply' I meant after reading out and parsing the > >> setup(control-out) request. I am sure we need to send a ZLP. > > > > You're wrong. Consider what happens when the host wants to send 7 > > bytes of data to the gadget using a control-OUT transfer: > > > > The gadget receives a SETUP packet. The USB_DIR_IN bit is > > clear because this is an OUT transfer, and wLength is set to 7. > > Which is what you got, right? > > > > Next, the host will send the 7-byte data packet. The gadget > > has to prepare to receive it, and it does so by submitting a > > 7-byte OUT request to ep0. This happens within the setup > > handler. > > > > The data packet is sent and the gadget receives it. The status > > stage for this transfer consists of a 0-length IN transaction, > > > I have been referring to this "0-length IN transaction" when I said > "need to send a zlp". You're thinking of what happens when the host does a 0-length control-OUT transfer. In that case there is no data stage, so after the setup stage you go directly to the status stage -- which consists of a 0-length IN transaction that the gadget driver has to submit. It sounds like you were getting confused over the difference between a 0-length and a more-than-0-length control-OUT transfer. They don't get handled the same way. > > which the UDC driver automatically queues as soon as the > > completion routine for the data packet returns. The gadget > > driver isn't involved in the status stage (unfortunately). > > > OK, so that is a known 'feature', not a bug in gadget drivers as I > have been calling it. I was trying to make the f_acm.c send that > "0-length IN transaction" by making 'value=0' > > Thanks You're welcome. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Jassi Brar <jassisinghbrar@gmail.com> writes: > On Wed, Oct 14, 2015 at 10:40 PM, Felipe Balbi <balbi@ti.com> wrote: >> >> Hi, >> >> Jassi Brar <jassisinghbrar@gmail.com> writes: >>> On Wed, Oct 14, 2015 at 9:18 PM, Felipe Balbi <balbi@ti.com> wrote: >>>> >>>> Hi, >>>> >>>> Jassi Brar <jassisinghbrar@gmail.com> writes: >>>>> On Wed, Oct 14, 2015 at 8:42 PM, Felipe Balbi <balbi@ti.com> wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> jaswinder.singh@linaro.org writes: >>>>>>> From: Jassi Brar <jaswinder.singh@linaro.org> >>>>>>> >>>>>>> We must return 0 for any OUT setup request, otherwise >>>>>>> protocol error may occur. >>>>>> >>>>>> have you seen any such errors ? >>>>>> >>>>> Yes. While testing my new udc driver. >>>>> >>>>> >>>>>> Care to describe what problems you have seen and which UDC you were using, >>>>>n> what's the exact scenario. How have you tested this ? >>>>>> >>>>> The udc I am writing the driver for is not yet public. To test my >>>>> driver at lowest level possible, I use 'usbmon' to capture and analyze >>>>> traffic since I don't have any hardware probe. >>>>> >>>>> I see the following on gadget side >>>>> ------- >>>>> configfs-gadget gadget: non-core control req21.20 v0000 i0001 l7 >>>>> configfs-gadget gadget: acm ttyGS0 req21.20 v0000 i0001 l7 >>>>> configfs-gadget gadget: acm ttyGS0 completion, err -71 >>>>> ------- >>>>> >>>>> and the following on host side usbmon capture >>>>> ------- >>>>> ffff88041ed01f00 540296433 S Co:3:023:0 s 21 20 0000 0001 0007 7 = >>>>> 80250000 000008 >>>>> ffff88041ed01f00 540296790 C Co:3:023:0 -71 0 >>>>> ------- >>>> >>>> and you did you even consider this could be a bug in your new UDC driver >>>> at all ? This works fine with other UDCs. >>>> >>> I was happy too until I decided to look closely at the annoying print >>> on gadget side. This is a non-fatal error and visible only when >>> debugging is enabled, so every udc seems 'fine' >> >> I tried with my sniffer and saw no stalls, nothing. My setup request to >> set line coding to 9600 baud worked just fine. right, because you worked around a bug in your UDC driver. > I am sure your udc driver will (need to) track stages of a transfer. > If you put some print in usb_ep_ops.queue() you will notice the stack > queues 7byte transfer but the udc driver silently drops it and send a > zlp here. no it doesn't. It starts the 7-byte Data phase and later starts a ZLP for the status phase. But only a SINGLE request was ever queued by the gadget driver. > I can move the change into my driver as well, but the point is > gadget should never try to _send_ non-zlp as reply to control-OUT > command. no, you shouldn't do that. You need to receive these 7 bytes from host which is the data phase. What you're missing is to handle the status phase yourself, without waiting for the gadget driver. This is a bug in your UDC driver. Here's what happens with DWC3: irq/181-dwc3-358 [000] d... 60.705735: dwc3_event: event 0000c040 irq/181-dwc3-358 [000] d... 60.705740: dwc3_ep0: Transfer Complete while ep0out in state 'Setup Phase' irq/181-dwc3-358 [000] d... 60.705745: dwc3_ep0: Setup Phase irq/181-dwc3-358 [000] d... 60.705747: dwc3_ctrl_req: bRequestType 21 bRequest 20 wValue 0000 wIndex 0000 wLength 7 This is what we received from host. This is a 3-stage Control request. Data phase has 7 bytes in the OUT direction (device side needs to receive these 7 bytes). irq/181-dwc3-358 [000] d... 60.705781: dwc3_ep0: queueing request ed05e0c0 to ep0out-control-cont length 7 state 'Setup Phase' So gadget driver queues a 7-byte request to the UDC. irq/181-dwc3-358 [000] d... 60.705789: dwc3_prepare_trb: ep0out-control-cont: trb f32d5000 bph 00000000 bpl bf0c6000 size 00000040 ctrl 00000c53 irq/181-dwc3-358 [000] d... 60.705793: dwc3_gadget_ep_cmd: ep0out-control-cont: cmd 'Start Transfer' [6] params 00000000 bf0c5000 00000000 irq/181-dwc3-358 [000] d... 60.705812: dwc3_gadget: Command Complete --> 0 The transfer gets started and we wait for an IRQ. (note that size is 0x40, or 64-bytes. That's due to a bug in dwc3. OUT *MUST* be aligned to wMaxPacketSize) irq/181-dwc3-358 [000] d... 60.705822: dwc3_event: event 000010c0 irq/181-dwc3-358 [000] d... 60.705825: dwc3_ep0: Transfer Not Ready while ep0out in state 'Data Phase' irq/181-dwc3-358 [000] d... 60.705830: dwc3_ep0: Control Data irq/181-dwc3-358 [000] d... 60.705912: dwc3_event: event 0000e040 irq/181-dwc3-358 [000] d... 60.705914: dwc3_ep0: Transfer Complete while ep0out in state 'Data Phase' Here's the IRQ. irq/181-dwc3-358 [000] d... 60.705919: dwc3_ep0: Data Phase irq/181-dwc3-358 [000] d... 60.705921: dwc3_complete_trb: ep0out-control-cont: trb f32d5000 bph 00000000 bpl bf0c6000 size 00000039 ctrl 00000c52 irq/181-dwc3-358 [000] d... 60.705927: dwc3_gadget_giveback: ep0out-control-cont: req ed05e0c0 length 7/7 ==> 0 The request is given back to the gadget driver. Now we need to wait for another interrupt. This will be for the Status phase. irq/181-dwc3-358 [000] d... 60.705937: dwc3_event: event 000020c2 irq/181-dwc3-358 [000] d... 60.705939: dwc3_ep0: Transfer Not Ready while ep0in in state 'Data Phase' irq/181-dwc3-358 [000] d... 60.705942: dwc3_ep0: Control Status Note that we got this new IRQ but... irq/181-dwc3-358 [000] d... 60.705944: dwc3_prepare_trb: ep0in-control-contr: trb f32d5000 bph 00000000 bpl bf0c4000 size 00000000 ctrl 00000c43 irq/181-dwc3-358 [000] d... 60.705948: dwc3_gadget_ep_cmd: ep0in-control-contr: cmd 'Start Transfer' [6] params 00000000 bf0c5000 00000000 irq/181-dwc3-358 [000] d... 60.705966: dwc3_gadget: Command Complete --> 0 We started the transfer right away. We did NOT wait for the gadget driver. This is the ZLP. You should not wait for the gadget driver and just handle it at the UDC level. There's no masking of anything, there's no lying to gadget driver or UDC or host side. We DID receive the 7 bytes we had to receive. irq/181-dwc3-358 [000] d... 60.706010: dwc3_ep0: Transfer Complete while ep0in in state 'Status Phase' irq/181-dwc3-358 [000] d... 60.706014: dwc3_ep0: Status Phase irq/181-dwc3-358 [000] d... 60.706016: dwc3_complete_trb: ep0out-control-cont: trb f32d5000 bph 00000000 bpl bf0c4000 size 00000000 ctrl 00000c42 And here we finished the Status Phase and the Control Request is finally complete. Here's a screenshot of my sniffer: http://imgur.com/iEqaVKV >>>> How far are you in developing this new UDC driver ? >>>> >>> Its done and tested for fsg+acm composite and each alone. >> >> stress tests ? Meaning, did you leave it running for a couple of weeks >> at least ? >> > I know I can't stress test enough, but this bug is 100% hit and > staring in the eye at protocol level. > >>> BTW, should the gadget stack ever queue a Non-ZLP as reply to some >>> setup request that has USB_DIR_IN not set? >> >> What phase of the setup are we talking about ? >> > I said "_reply_ to setup request" so I meant status phase. right, there's never an OUT data phase, is there ? Except when there is. Have a look at SetDescriptor. I think DFU class also uses control requests to send firmware to USB device, so another OUT data phase. > UDC driver receives the SETUP command as '21 20 00 00 01 00 07 00', > passes it onto composite, which hands it over to acm and which > pretends we need to return a 7byte packet to host (value = w_length = > 7). It doesn't pretend, it needs to return the 7 bytes because it's telling you that it needs to receive a 7-byte packet for the Data phase.
diff --git a/drivers/usb/gadget/function/f_acm.c b/drivers/usb/gadget/function/f_acm.c index aad8165..14d9e28 100644 --- a/drivers/usb/gadget/function/f_acm.c +++ b/drivers/usb/gadget/function/f_acm.c @@ -364,7 +364,7 @@ static int acm_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl) || w_index != acm->ctrl_id) goto invalid; - value = w_length; + value = 0; cdev->gadget->ep0->driver_data = acm; req->complete = acm_complete_set_line_coding; break;