On 11/26/2014 02:08 PM, Amit Virdi wrote:
> Dear Sebastian,
Hi Amit,
> On 11/25/2014 10:52 PM, Sebastian Andrzej Siewior wrote:
>> The max packet size within the FS descriptor has to be highest possible
>> value and _not_ the one that is (or will be) used on FS.
>
> The current code sets wMaxPacketSize of FS interrupt endpoint descriptor
> as 64, which is in accordance with the usb 2.0 specification, section 5.7.3
I know about the specification. The "1024" is only used initially while
allocating endpoints. It is never passed to the host at FS.
>> The way the code works (since day 1) is that usb_ep_autoconfig() is
>> invoked _only_ on the FS endpoint and then the endpoint address is
>> copies over to HS/SS endpoints. If the any of the "critical" options are
>> different between FS and HS then we have to pass the HS value and
>> correct later.
>>
>> What now happens is that we look for an INT-OUT endpoint of 64bytes. The
>> code will return an endpoint matching this category. Further the
>> sourcesink will take this endpoint and increase the MPS to 1024. On
>
> This is valid only for HS and SS interrupt endpoints. Right?
Correct *but* the chosen endpoint may not be capable of MPS > what you
were looking for.
>> net2280 for instance the code tries to be clever to return an endpoint
>> which can only do 64 MPS. The same happens on musb where we mostlike get
>> an endpoint which can only do 512. The result is then on the host side:
>>
>
> What is the speed at which your device is operating?
HS.
>> |~# testusb -a -t 9 -c 2
>> |unknown speed /dev/bus/usb/002/045 0
>> |usbtest 2-1:3.0: TEST 9: ch9 (subset) control tests, 2 times
>> |usbtest 2-1:3.0: can't set_interface = 2, -32
>> |usbtest 2-1:3.0: ch9 subset failed, iterations left 1
>> |/dev/bus/usb/002/045 test 9 --> 32 (Broken pipe)
>>
>> because the on the gadget side we can't enable the endpoint because
>> desc->size > ep->max_size.
>>
>> Fixes: ef11982dd7a6 ("usb: gadget: zero: Add support for interrupt EP")
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
>> ---
>> drivers/usb/gadget/function/f_sourcesink.c | 24
>> ++++++++++++------------
>> 1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_sourcesink.c
>> b/drivers/usb/gadget/function/f_sourcesink.c
>> index 80be25b32cd7..7d8f0216e1a6 100644
>> --- a/drivers/usb/gadget/function/f_sourcesink.c
>> +++ b/drivers/usb/gadget/function/f_sourcesink.c
>> @@ -161,7 +161,7 @@ static struct usb_endpoint_descriptor
>> fs_int_source_desc = {
>>
>> .bEndpointAddress = USB_DIR_IN,
>> .bmAttributes = USB_ENDPOINT_XFER_INT,
>> - .wMaxPacketSize = cpu_to_le16(64),
>> + .wMaxPacketSize = cpu_to_le16(1024),
>> .bInterval = GZERO_INT_INTERVAL,
>> };
>>
>> @@ -171,7 +171,7 @@ static struct usb_endpoint_descriptor
>> fs_int_sink_desc = {
>>
>> .bEndpointAddress = USB_DIR_OUT,
>> .bmAttributes = USB_ENDPOINT_XFER_INT,
>> - .wMaxPacketSize = cpu_to_le16(64),
>> + .wMaxPacketSize = cpu_to_le16(1024),
>> .bInterval = GZERO_INT_INTERVAL,
>> };
>>
>
> This change in wMAxPacketSize of FS interrupt descriptors is violation
> of the specs.
It is changed back before the descriptor is sent to the host.
>> @@ -556,16 +556,6 @@ sourcesink_bind(struct usb_configuration *c,
>> struct usb_function *f)
>> if (int_maxburst > 15)
>> int_maxburst = 15;
>>
>> - /* fill in the FS interrupt descriptors from the module
>> parameters */
>> - fs_int_source_desc.wMaxPacketSize = int_maxpacket > 64 ?
>> - 64 : int_maxpacket;
>> - fs_int_source_desc.bInterval = int_interval > 255 ?
>> - 255 : int_interval;
>> - fs_int_sink_desc.wMaxPacketSize = int_maxpacket > 64 ?
>> - 64 : int_maxpacket;
>> - fs_int_sink_desc.bInterval = int_interval > 255 ?
>> - 255 : int_interval;
>> -
>> /* allocate int endpoints */
>> ss->int_in_ep = usb_ep_autoconfig(cdev->gadget,
>> &fs_int_source_desc);
>> if (!ss->int_in_ep)
>> @@ -587,6 +577,16 @@ sourcesink_bind(struct usb_configuration *c,
>> struct usb_function *f)
>> if (int_maxpacket > 1024)
>> int_maxpacket = 1024;
>>
>> + /* fill in the FS interrupt descriptors from the module
>> parameters */
>> + fs_int_source_desc.wMaxPacketSize = int_maxpacket > 64 ?
>> + 64 : int_maxpacket;
>> + fs_int_source_desc.bInterval = int_interval > 255 ?
>> + 255 : int_interval;
>> + fs_int_sink_desc.wMaxPacketSize = int_maxpacket > 64 ?
>> + 64 : int_maxpacket;
>> + fs_int_sink_desc.bInterval = int_interval > 255 ?
>> + 255 : int_interval;
>> +
>> /* support high speed hardware */
>> hs_source_desc.bEndpointAddress = fs_source_desc.bEndpointAddress;
>> hs_sink_desc.bEndpointAddress = fs_sink_desc.bEndpointAddress;
>>
>
> Things might be working for you but this is not the correct fix, IMO.
Do you have something better?
> Looking into the patch I feel it shall introduce other regressions.
For instance?
> Did you try inserting g_zero with module parameters for musb?
If I force MPP to 64 (as Paul suggested more or less) then it works.
But this means I know about the upcoming problem.
>
> Regards
> Amit Virdi
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
