[PATCH] usb: gadget: function: acm: return zlp for OUT setup

[PATCH] usb: gadget: function: acm: return zlp for OUT setup

[jaswinder.singh]

From: Jassi Brar <jaswinder.singh@linaro.org>

We must return 0 for any OUT setup request, otherwise
protocol error may occur.

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 drivers/usb/gadget/function/f_acm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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;
-- 
1.9.1

Re: [PATCH] usb: gadget: function: acm: return zlp for OUT setup

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 819 bytes --]


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 ?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] usb: gadget: function: acm: return zlp for OUT setup

[Jassi Brar]

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.

Re: [PATCH] usb: gadget: function: acm: return zlp for OUT setup

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1636 bytes --]


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) ?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] usb: gadget: function: acm: return zlp for OUT setup

[Jassi Brar]

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?

Re: [PATCH] usb: gadget: function: acm: return zlp for OUT setup

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3220 bytes --]


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.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] usb: gadget: function: acm: return zlp for OUT setup

[Jassi Brar]

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).

Re: [PATCH] usb: gadget: function: acm: return zlp for OUT setup

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 7901 bytes --]


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.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] usb: gadget: function: acm: return zlp for OUT setup

[Alan Stern]

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

Re: [PATCH] usb: gadget: function: acm: return zlp for OUT setup

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 777 bytes --]


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.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] usb: gadget: function: acm: return zlp for OUT setup

[Alan Stern]

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

Re: [PATCH] usb: gadget: function: acm: return zlp for OUT setup

[Jassi Brar]

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.

Re: [PATCH] usb: gadget: function: acm: return zlp for OUT setup

[Alan Stern]

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

Re: [PATCH] usb: gadget: function: acm: return zlp for OUT setup

[Jassi Brar]

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

Re: [PATCH] usb: gadget: function: acm: return zlp for OUT setup

[Alan Stern]

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