Re: [PATCH] usb: gadget: zero: fix INT endpoint assignment
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/0450
|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
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 > 25
Re: [PATCH] usb: gadget: zero: fix INT endpoint assignment
On 11/26/2014 10:40 PM, Alan Stern wrote: > On Wed, 26 Nov 2014, Sebastian Andrzej Siewior wrote: > >> On 11/26/2014 04:24 PM, Alan Stern wrote: 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 The maximum allowable interrupt data payload size is 64 bytes or less for full-speed. High-speed endpoints are allowed maximum data payload sizes up to 1024 bytes. >>> >>> The real problem is that we are assuming endpoints can be allocated in >>> a single way that will work correctly for all possible connection >>> speeds. (I suspect it was done this way out of laziness and a desire >>> to minimize the code size.) This assumption is obviously incorrect >>> when the hardware has an interrupt endpoint that supports packet sizes >>> of 64 but no larger. >> >> The code will check properly if you pass 1024 and check the size >> accordingly. You have to "downsize" your FS descriptor later. This >> function will only fail to do this if your gadget isn't dual speed. In >> that case it expects 64 as the upper limit for INT (if I recall >> correctly). > > It will also fail in situations where you use up a lot of endpoints. > For example, let's say the UDC only supports 4 endpoints, one of which > must have a maxpacket value <= 64. If you want to have four interrupt > endpoints, you can't run at high speed -- but you can run at full > speed. However, the approach you outlined above won't allow it. fair enough. So we could try to look for one with 1024 and it fails we could re-try with 512 and then 64. If all three fail we would continue without INT support. >> Ah. And endpoints are never returned to the allocator. Some gadgets set >> ->private to NULL, other just ignore it and let the core do it. So >> re-doing the endpoint allocator is probably the right thing to do. And > > The allocator doesn't need to be changed. We already have a > usb_ep_autoconfig_reset() function. yes, that one that frees them all. >> then force every gadget to allocate an endpoint for FS/HS/SS and give >> it back _and_ please edit the copy of the descriptor and not the global >> "original". > > Yes. > >> But the work will not be done before we have the next release is out >> and as of now it breaks g_zero on musb, net2280 and maybe others. > > Felipe will have to decide how to handle this. > > Alan Stern > Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: zero: fix INT endpoint assignment
On 11/25/2014 08:39 PM, Paul Zimmerman wrote:
>> 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),
>
> This seems strange. You are setting the max packet size in the FS Intr
> endpoint descriptor to a value that is illegal for FS. Won't that cause
> usb_ep_autoconfig() to fail if the UDC only has a FS Intr endpoint?
Funny at it is, tests have shown to work as expected. Only if UDC is FS
only then it won't pass. This could be fixed…
> Maybe you should set wMaxPacketSize to 0 instead? The ep_matches()
> function in epautoconf.c has this code:
> /*
>* If the protocol driver hasn't yet decided on wMaxPacketSize
>* and wants to know the maximum possible, provide the info.
>*/
> if (desc->wMaxPacketSize == 0)
> desc->wMaxPacketSize = cpu_to_le16(ep->maxpacket_limit);
>
This means we get most likely the smallest possible endpoint and won't
be able to perform transfers @1024 even if we would have such an
endpoint.
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: zero: fix INT endpoint assignment
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/0450
>> |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
>> ---
>> 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.bE
Re: [PATCH] usb: gadget: zero: fix INT endpoint assignment
On Wed, 26 Nov 2014, Sebastian Andrzej Siewior wrote: > On 11/26/2014 04:24 PM, Alan Stern wrote: > >> 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 > >> > >>The maximum allowable interrupt data payload size is 64 bytes > >>or less for full-speed. High-speed endpoints are allowed > >>maximum data payload sizes up to 1024 bytes. > > > > The real problem is that we are assuming endpoints can be allocated in > > a single way that will work correctly for all possible connection > > speeds. (I suspect it was done this way out of laziness and a desire > > to minimize the code size.) This assumption is obviously incorrect > > when the hardware has an interrupt endpoint that supports packet sizes > > of 64 but no larger. > > The code will check properly if you pass 1024 and check the size > accordingly. You have to "downsize" your FS descriptor later. This > function will only fail to do this if your gadget isn't dual speed. In > that case it expects 64 as the upper limit for INT (if I recall > correctly). It will also fail in situations where you use up a lot of endpoints. For example, let's say the UDC only supports 4 endpoints, one of which must have a maxpacket value <= 64. If you want to have four interrupt endpoints, you can't run at high speed -- but you can run at full speed. However, the approach you outlined above won't allow it. > But what you can't do (and this is done by ISOC and INT endpoint) is to > upgrade your capabilities by increasing the max packet size after you > received an endpoint. This "works" for ISOC because on FS you can use > up to 1023 bytes and the matching FIFO has usually 1024 bytes and not > 1023. It isn't correct but it works. > > This was done out of laziness and "simplicity" as far as I recall. > Usually the only difference between FS and HS is 0 so the only thing > you do is to update the bEndpointAddress member in HS/SS descriptors. > Nothing more. Most INT don't care for 1024 transfer size or anything > like that and the first gadgets in kernel were not ISOC and even these > days their MPS is < 200. > > So it was easy just to update the endpoint address for the HS > descriptor, you used the same endpoint as for FS. Most in tree gadgets > don't do more. > > > The right way to fix the problem is to avoid allocating endpoints until > > after the connection speed is known. Then we can call > > usb_ep_autoconfig() with the appropriate set of descriptors, and > > nothing will need to be fixed up. > > > > However, I don't know if this approach is compatible with the composite > > framework in its current form. > > I've been looking at this mess by the time I started the configfs. I > tried a few times and decided not now and there was no later. > You need all descriptors prior you connect / at bind time. At this time > you need also your endpoints allocated. A small change to this breaks > things in multiple places therefore the created configfs gadget is > "bound" once it is complete. I tried even to have "one descriptor" and > let the code create HS/SS descriptors out of it (since in 99% they are > the same) but a few gadgets did more and I dropped that idea again. > > So this is what is expected in most parts of the code as of now. Then > you have USB_DT_OTHER_SPEED_CONFIG where you may ask for HS descriptors > on a FS link. So you need those. You're right. It looks like we need to set up separate allocations of endpoints for all possible connection speeds, during initialization. If one of the allocations fails then the gadget must not be allowed to connect at that speed. (Unfortunately the gadget framework doesn't include any method for telling the hardware not to connect at a certain speed...) > All in all I tried to minimize the effects by leaving the descriptors > "untouched" and creating a copy after bind. I didn't manage to redo the > endpoint allocation. > Part of the problem is that you have no clear cut currently between > descriptor preparation and endpoint allocation. The endpoint allocator > does not know about HS/FS/SS. It knows that there is an endpoint, it > can do all speeds and has a special special upper limit (it may not be > able to INT or BULK or ISOC so it considers that as well). > Once it finds a match, it writes the endpoint address into the > descriptor and returns the pointer to the endpoint. So technically you > return two values here. The endpoint is considered taken once > ep->private is set (so it is a little racy). > Based on this you can't get the same endpoint on HS because it is gone. > You could but then you get another one. it will work but is a waste of > resources. > > Ah. And endpoints a
Re: [PATCH] usb: gadget: zero: fix INT endpoint assignment
On 11/26/2014 04:24 PM, Alan Stern wrote: >> 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 >> >> The maximum allowable interrupt data payload size is 64 bytes >> or less for full-speed. High-speed endpoints are allowed >> maximum data payload sizes up to 1024 bytes. > > The real problem is that we are assuming endpoints can be allocated in > a single way that will work correctly for all possible connection > speeds. (I suspect it was done this way out of laziness and a desire > to minimize the code size.) This assumption is obviously incorrect > when the hardware has an interrupt endpoint that supports packet sizes > of 64 but no larger. The code will check properly if you pass 1024 and check the size accordingly. You have to "downsize" your FS descriptor later. This function will only fail to do this if your gadget isn't dual speed. In that case it expects 64 as the upper limit for INT (if I recall correctly). But what you can't do (and this is done by ISOC and INT endpoint) is to upgrade your capabilities by increasing the max packet size after you received an endpoint. This "works" for ISOC because on FS you can use up to 1023 bytes and the matching FIFO has usually 1024 bytes and not 1023. It isn't correct but it works. This was done out of laziness and "simplicity" as far as I recall. Usually the only difference between FS and HS is 0 so the only thing you do is to update the bEndpointAddress member in HS/SS descriptors. Nothing more. Most INT don't care for 1024 transfer size or anything like that and the first gadgets in kernel were not ISOC and even these days their MPS is < 200. So it was easy just to update the endpoint address for the HS descriptor, you used the same endpoint as for FS. Most in tree gadgets don't do more. > The right way to fix the problem is to avoid allocating endpoints until > after the connection speed is known. Then we can call > usb_ep_autoconfig() with the appropriate set of descriptors, and > nothing will need to be fixed up. > > However, I don't know if this approach is compatible with the composite > framework in its current form. I've been looking at this mess by the time I started the configfs. I tried a few times and decided not now and there was no later. You need all descriptors prior you connect / at bind time. At this time you need also your endpoints allocated. A small change to this breaks things in multiple places therefore the created configfs gadget is "bound" once it is complete. I tried even to have "one descriptor" and let the code create HS/SS descriptors out of it (since in 99% they are the same) but a few gadgets did more and I dropped that idea again. So this is what is expected in most parts of the code as of now. Then you have USB_DT_OTHER_SPEED_CONFIG where you may ask for HS descriptors on a FS link. So you need those. All in all I tried to minimize the effects by leaving the descriptors "untouched" and creating a copy after bind. I didn't manage to redo the endpoint allocation. Part of the problem is that you have no clear cut currently between descriptor preparation and endpoint allocation. The endpoint allocator does not know about HS/FS/SS. It knows that there is an endpoint, it can do all speeds and has a special special upper limit (it may not be able to INT or BULK or ISOC so it considers that as well). Once it finds a match, it writes the endpoint address into the descriptor and returns the pointer to the endpoint. So technically you return two values here. The endpoint is considered taken once ep->private is set (so it is a little racy). Based on this you can't get the same endpoint on HS because it is gone. You could but then you get another one. it will work but is a waste of resources. Ah. And endpoints are never returned to the allocator. Some gadgets set ->private to NULL, other just ignore it and let the core do it. So re-doing the endpoint allocator is probably the right thing to do. And then force every gadget to allocate an endpoint for FS/HS/SS and give it back _and_ please edit the copy of the descriptor and not the global "original". But the work will not be done before we have the next release is out and as of now it breaks g_zero on musb, net2280 and maybe others. > Alan Stern Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: zero: fix INT endpoint assignment
On 11/26/2014 02:21 PM, Amit Virdi wrote:
>> - if ISOC is not available, we won't have INT as well.
>
> I didn't understand this. The original patch added a new alternate
> setting (2) that has two interrupt endpoints. ISOC EP is available
> through alternate setting 1.
The ISOC code does this:
no_iso:
/*
* We still want to work even if the UDC doesn't have isoc
* endpoints, so null out the alt interface that contains
* them and continue.
*/
fs_source_sink_descs[FS_ALT_IFC_1_OFFSET] = NULL;
hs_source_sink_descs[HS_ALT_IFC_1_OFFSET] = NULL;
ss_source_sink_descs[SS_ALT_IFC_1_OFFSET] = NULL;
That means the descriptor is terminated early:
static struct usb_descriptor_header *fs_source_sink_descs[] = {
(struct usb_descriptor_header *) &source_sink_intf_alt0,
(struct usb_descriptor_header *) &fs_sink_desc,
(struct usb_descriptor_header *) &fs_source_desc,
=> (struct usb_descriptor_header *) &source_sink_intf_alt1,
#define FS_ALT_IFC_1_OFFSET 3
That means "no ISOC" will NULL the fourth descriptor and there is no
alt1 including your alt2. Those descs won't be passed to the host. It
was "okay" while it was ISOC only but now "disabling" ISOC means no INT
either.
>> - wMaxPacketSize is supposed to be LE. The assignments within the code
>>does not use cpu_to_le16().
>
> Can you please point to the code where it should have been and is missing?
You look for each assignment to wMaxPacketSize and you will notice that
the cpu_to_le16 isn't used:
/* fill in the FS isoc descriptors from the module parameters */
fs_iso_source_desc.wMaxPacketSize = isoc_maxpacket > 1023 ?
1023 : isoc_maxpacket;
this one example, the very same is true for your copy/pasted INT
handling.
> Regards
> Amit Virdi
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: zero: fix INT endpoint assignment
On Wed, 26 Nov 2014, Amit Virdi wrote: > Dear Sebastian, > > 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 > > The maximum allowable interrupt data payload size is 64 bytes > or less for full-speed. High-speed endpoints are allowed > maximum data payload sizes up to 1024 bytes. The real problem is that we are assuming endpoints can be allocated in a single way that will work correctly for all possible connection speeds. (I suspect it was done this way out of laziness and a desire to minimize the code size.) This assumption is obviously incorrect when the hardware has an interrupt endpoint that supports packet sizes of 64 but no larger. The right way to fix the problem is to avoid allocating endpoints until after the connection speed is known. Then we can call usb_ep_autoconfig() with the appropriate set of descriptors, and nothing will need to be fixed up. However, I don't know if this approach is compatible with the composite framework in its current form. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: zero: fix INT endpoint assignment
Dear Sebastian, On 11/25/2014 10:56 PM, Sebastian Andrzej Siewior wrote: This fixes the test case mentioned for musb which is a regression. Other things that I noticed: - if ISOC is not available, we won't have INT as well. I didn't understand this. The original patch added a new alternate setting (2) that has two interrupt endpoints. ISOC EP is available through alternate setting 1. - wMaxPacketSize is supposed to be LE. The assignments within the code does not use cpu_to_le16(). Can you please point to the code where it should have been and is missing? - the module Parameter for INT says max packet size is 0…1023 for FS. Yes, I'll send a patch to rectify this. This is clearly a copy/paste bug. Amit could you please look at this and fix it? Sebastian Regards Amit Virdi -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: zero: fix INT endpoint assignment
Dear Sebastian,
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
The maximum allowable interrupt data payload size is 64 bytes
or less for full-speed. High-speed endpoints are allowed
maximum data payload sizes up to 1024 bytes.
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?
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?
|~# testusb -a -t 9 -c 2
|unknown speed /dev/bus/usb/002/0450
|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
---
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.
@@ -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.
Looking into the patch I feel it shall introduce other reg
RE: [PATCH] usb: gadget: zero: fix INT endpoint assignment
> From: Sebastian Andrzej Siewior [mailto:bige...@linutronix.de]
> Sent: Tuesday, November 25, 2014 9:22 AM
>
> 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 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
> 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:
>
> |~# testusb -a -t 9 -c 2
> |unknown speed /dev/bus/usb/002/0450
> |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
> ---
> 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),
This seems strange. You are setting the max packet size in the FS Intr
endpoint descriptor to a value that is illegal for FS. Won't that cause
usb_ep_autoconfig() to fail if the UDC only has a FS Intr endpoint?
Maybe you should set wMaxPacketSize to 0 instead? The ep_matches()
function in epautoconf.c has this code:
/*
* If the protocol driver hasn't yet decided on wMaxPacketSize
* and wants to know the maximum possible, provide the info.
*/
if (desc->wMaxPacketSize == 0)
desc->wMaxPacketSize = cpu_to_le16(ep->maxpacket_limit);
--
Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: zero: fix INT endpoint assignment
This fixes the test case mentioned for musb which is a regression. Other things that I noticed: - if ISOC is not available, we won't have INT as well. - wMaxPacketSize is supposed to be LE. The assignments within the code does not use cpu_to_le16(). - the module Parameter for INT says max packet size is 0…1023 for FS. This is clearly a copy/paste bug. Amit could you please look at this and fix it? Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
