Hi,
Jassi Brar <jassisinghbrar@xxxxxxxxx> writes:
> On Wed, Oct 14, 2015 at 10:40 PM, Felipe Balbi <balbi@xxxxxx> wrote:
>>
>> Hi,
>>
>> Jassi Brar <jassisinghbrar@xxxxxxxxx> writes:
>>> On Wed, Oct 14, 2015 at 9:18 PM, Felipe Balbi <balbi@xxxxxx> wrote:
>>>>
>>>> Hi,
>>>>
>>>> Jassi Brar <jassisinghbrar@xxxxxxxxx> writes:
>>>>> On Wed, Oct 14, 2015 at 8:42 PM, Felipe Balbi <balbi@xxxxxx> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> jaswinder.singh@xxxxxxxxxx writes:
>>>>>>> From: Jassi Brar <jaswinder.singh@xxxxxxxxxx>
>>>>>>>
>>>>>>> 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:
signature.asc
Description: PGP signature