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
I think you meant " then we have to pass the *FS* value ", right??
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
SourceSink does this unconditionally, assuming the corresponding EP
supports MPS of 1024. This assumption is incorrect, atleast for net2280
and musb, causing regressions.
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>
What I understood is as an effect of this patch, for musb and net2280
ep_autoconfig would provide EPs only in case it is able to find having
MPS of 1024 bytes. In case it doesn't find a free EP, the device won't
have interrupt support at all.
---
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.
That is correct, I agree... but still it is a hack :)
@@ -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?
Can't suggest anything right now. :(
Looking into the patch I feel it shall introduce other regressions.
For instance?
As you yourself correctly figured out in an another email, this shall
break for all the FS devices wishing to use interrupt EP, since max
would be 1024 as a result of this patch.
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.
What's the upcoming problem here? Do you mean the code isn't generic
enough in case you have to use module_params?
Regards
Amit Virdi
--
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
