* [PATCH v4] USB: gadget: epautoconf: fix ep maxpacket check
@ 2013-10-02 10:09 Robert Baldyga
2013-10-02 15:48 ` Alan Stern
0 siblings, 1 reply; 4+ messages in thread
From: Robert Baldyga @ 2013-10-02 10:09 UTC (permalink / raw)
To: balbi
Cc: gregkh, linux-usb, linux-kernel, b.zolnierkie, m.szyprowski,
andrzej.p, Robert Baldyga
This patch fix validation of maxpacket value given in endpoint descriptor.
Add check of maxpacket for bulk endpoints. If maxpacket is not set in
descriptor, it's set to maximum value for given type on endpoint in used
speed.
Correct maxpacket value is:
FULL-SPEED HIGH-SPEED SUPER-SPEED
BULK 8, 16, 32, 64 512 1024
INTERRUPT 1..64 1..1024 1..1024
ISOCHRONOUS 1..1023 1..1024 1..1024
Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
Hello,
This is fourth version of my patch. From last version I have removed
code reporting full speed bulk maxpacket because it's not needed since
maxpacket check for all speeds is performed before.
Best regards
Robert Baldyga
Samsung R&D Institute Poland
Changelog:
v4:
- removed unneded code reporting full speed bulk maxpacket
v3: https://lkml.org/lkml/2013/10/1/38
- fixed maxpacket check for bulk endpoints
- improved handling of ep descriptors which has not set wMaxPaketSize value
v2: https://lkml.org/lkml/2013/9/30/79
- arrange code for clearity
- fix support for super speed devices
v1: https://lkml.org/lkml/2013/9/27/127
drivers/usb/gadget/epautoconf.c | 107 +++++++++++++++++++++++++++------------
1 file changed, 75 insertions(+), 32 deletions(-)
diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
index a777f7b..7245e6d 100644
--- a/drivers/usb/gadget/epautoconf.c
+++ b/drivers/usb/gadget/epautoconf.c
@@ -124,37 +124,90 @@ ep_matches (
}
+ max = 0x7ff & usb_endpoint_maxp(desc);
+
/*
- * If the protocol driver hasn't yet decided on wMaxPacketSize
- * and wants to know the maximum possible, provide the info.
+ * Test if maxpacket given in descriptor isn't greater than maximum
+ * packet size for this endpoint
*/
- if (desc->wMaxPacketSize == 0)
- desc->wMaxPacketSize = cpu_to_le16(ep->maxpacket);
+ if (ep->maxpacket < max)
+ return 0;
- /* endpoint maxpacket size is an input parameter, except for bulk
- * where it's an output parameter representing the full speed limit.
- * the usb spec fixes high speed bulk maxpacket at 512 bytes.
+ /*
+ * Test if ep supports maxpacket size set in descriptor.
+ * If the protocol driver hasn't yet decided on wMaxPacketSize
+ * (when wMaxPacketSize == 0) and wants to know the maximum possible,
+ * provide the info.
*/
- max = 0x7ff & usb_endpoint_maxp(desc);
switch (type) {
+ case USB_ENDPOINT_XFER_BULK:
+ /*
+ * LIMITS:
+ * full speed: 64 bytes
+ * high speed: 512 bytes
+ * super speed: 1024 bytes
+ */
+ if (max == 0) {
+ if (gadget_is_superspeed(gadget))
+ desc->wMaxPacketSize = cpu_to_le16(1024);
+ else if (gadget_is_dualspeed(gadget))
+ desc->wMaxPacketSize = cpu_to_le16(512);
+ else
+ desc->wMaxPacketSize = cpu_to_le16(64);
+ } else {
+ if (max > 1024)
+ return 0;
+ if (!gadget_is_superspeed(gadget) && max > 512)
+ return 0;
+ if (!gadget_is_dualspeed(gadget) && max > 64)
+ return 0;
+ }
+ break;
+
case USB_ENDPOINT_XFER_INT:
- /* INT: limit 64 bytes full speed, 1024 high/super speed */
- if (!gadget_is_dualspeed(gadget) && max > 64)
- return 0;
- /* FALLTHROUGH */
+ /*
+ * LIMITS:
+ * full speed: 64 bytes
+ * high/super speed: 1024 bytes
+ * multiple transactions per microframe only for super speed
+ */
+ if (max == 0) {
+ if (gadget_is_dualspeed(gadget))
+ desc->wMaxPacketSize = cpu_to_le16(1024);
+ else
+ desc->wMaxPacketSize = cpu_to_le16(64);
+ } else {
+ if (max > 1024)
+ return 0;
+ if (!gadget_is_superspeed(gadget))
+ if ((desc->wMaxPacketSize & cpu_to_le16(3<<11)))
+ return 0;
+ if (!gadget_is_dualspeed(gadget) && max > 64)
+ return 0;
+ }
+ break;
case USB_ENDPOINT_XFER_ISOC:
- /* ISO: limit 1023 bytes full speed, 1024 high/super speed */
- if (ep->maxpacket < max)
- return 0;
- if (!gadget_is_dualspeed(gadget) && max > 1023)
- return 0;
-
- /* BOTH: "high bandwidth" works only at high speed */
- if ((desc->wMaxPacketSize & cpu_to_le16(3<<11))) {
- if (!gadget_is_dualspeed(gadget))
+ /*
+ * LIMITS:
+ * full speed: 1023 bytes
+ * high/super speed: 1024 bytes
+ * multiple transactions per microframe for high/super speed
+ */
+ if (max == 0) {
+ if (gadget_is_dualspeed(gadget))
+ desc->wMaxPacketSize = cpu_to_le16(1024);
+ else
+ desc->wMaxPacketSize = cpu_to_le16(1023);
+ } else {
+ if (max > 1024)
return 0;
- /* configure your hardware with enough buffering!! */
+ if (!gadget_is_dualspeed(gadget)) {
+ if (max > 1023)
+ return 0;
+ if ((desc->wMaxPacketSize & cpu_to_le16(3<<11)))
+ return 0;
+ }
}
break;
}
@@ -175,16 +228,6 @@ ep_matches (
return 0;
desc->bEndpointAddress |= gadget->out_epnum;
}
-
- /* report (variable) full speed bulk maxpacket */
- if ((USB_ENDPOINT_XFER_BULK == type) && !ep_comp) {
- int size = ep->maxpacket;
-
- /* min() doesn't work on bitfields with gcc-3.5 */
- if (size > 64)
- size = 64;
- desc->wMaxPacketSize = cpu_to_le16(size);
- }
ep->address = desc->bEndpointAddress;
return 1;
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v4] USB: gadget: epautoconf: fix ep maxpacket check
2013-10-02 10:09 [PATCH v4] USB: gadget: epautoconf: fix ep maxpacket check Robert Baldyga
@ 2013-10-02 15:48 ` Alan Stern
2013-10-03 10:36 ` Robert Baldyga
0 siblings, 1 reply; 4+ messages in thread
From: Alan Stern @ 2013-10-02 15:48 UTC (permalink / raw)
To: Robert Baldyga
Cc: balbi, gregkh, linux-usb, linux-kernel, b.zolnierkie,
m.szyprowski, andrzej.p
On Wed, 2 Oct 2013, Robert Baldyga wrote:
> This patch fix validation of maxpacket value given in endpoint descriptor.
> Add check of maxpacket for bulk endpoints. If maxpacket is not set in
> descriptor, it's set to maximum value for given type on endpoint in used
> speed.
>
> Correct maxpacket value is:
>
> FULL-SPEED HIGH-SPEED SUPER-SPEED
> BULK 8, 16, 32, 64 512 1024
> INTERRUPT 1..64 1..1024 1..1024
> ISOCHRONOUS 1..1023 1..1024 1..1024
>
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> ---
>
> Hello,
>
> This is fourth version of my patch. From last version I have removed
> code reporting full speed bulk maxpacket because it's not needed since
> maxpacket check for all speeds is performed before.
It seems that this patch does a lot of things wrong. Comments below.
> @@ -124,37 +124,90 @@ ep_matches (
>
> }
>
> + max = 0x7ff & usb_endpoint_maxp(desc);
> +
> /*
> - * If the protocol driver hasn't yet decided on wMaxPacketSize
> - * and wants to know the maximum possible, provide the info.
> + * Test if maxpacket given in descriptor isn't greater than maximum
> + * packet size for this endpoint
> */
> - if (desc->wMaxPacketSize == 0)
> - desc->wMaxPacketSize = cpu_to_le16(ep->maxpacket);
> + if (ep->maxpacket < max)
> + return 0;
>
> - /* endpoint maxpacket size is an input parameter, except for bulk
> - * where it's an output parameter representing the full speed limit.
> - * the usb spec fixes high speed bulk maxpacket at 512 bytes.
> + /*
> + * Test if ep supports maxpacket size set in descriptor.
> + * If the protocol driver hasn't yet decided on wMaxPacketSize
> + * (when wMaxPacketSize == 0) and wants to know the maximum possible,
> + * provide the info.
This disagrees with the kerneldoc for usb_ep_autoconfig(). For bulk
endpoints, wMaxPacket is always supposed to be set to the full-speed
value, regardless of what the protocol driver specifies.
> */
> - max = 0x7ff & usb_endpoint_maxp(desc);
> switch (type) {
> + case USB_ENDPOINT_XFER_BULK:
> + /*
> + * LIMITS:
> + * full speed: 64 bytes
> + * high speed: 512 bytes
> + * super speed: 1024 bytes
> + */
> + if (max == 0) {
> + if (gadget_is_superspeed(gadget))
> + desc->wMaxPacketSize = cpu_to_le16(1024);
> + else if (gadget_is_dualspeed(gadget))
> + desc->wMaxPacketSize = cpu_to_le16(512);
> + else
> + desc->wMaxPacketSize = cpu_to_le16(64);
So these lines are wrong. Also, how do you know that 64 is correct for
full speed? The hardware might only support 32.
> + } else {
> + if (max > 1024)
> + return 0;
> + if (!gadget_is_superspeed(gadget) && max > 512)
> + return 0;
> + if (!gadget_is_dualspeed(gadget) && max > 64)
> + return 0;
> + }
For bulk endpoints, you should ignore the original value in the
descriptor. All that matters is ep->maxpacket; it will override the
value in the descriptor.
> + break;
> +
> case USB_ENDPOINT_XFER_INT:
> - /* INT: limit 64 bytes full speed, 1024 high/super speed */
> - if (!gadget_is_dualspeed(gadget) && max > 64)
> - return 0;
> - /* FALLTHROUGH */
> + /*
> + * LIMITS:
> + * full speed: 64 bytes
> + * high/super speed: 1024 bytes
> + * multiple transactions per microframe only for super speed
The last comment is wrong. High speed also allows multiple interrupt
transactions in a microframe.
Also, why bother to spell out the limits in the comment? You're not
going to use those values; you're going to use the limit in
ep->maxpacket.
> + */
> + if (max == 0) {
> + if (gadget_is_dualspeed(gadget))
> + desc->wMaxPacketSize = cpu_to_le16(1024);
> + else
> + desc->wMaxPacketSize = cpu_to_le16(64);
These values should be taken from ep->maxpacket, not from the spec.
> + } else {
> + if (max > 1024)
> + return 0;
> + if (!gadget_is_superspeed(gadget))
> + if ((desc->wMaxPacketSize & cpu_to_le16(3<<11)))
> + return 0;
> + if (!gadget_is_dualspeed(gadget) && max > 64)
> + return 0;
The first and third tests are unnecessary, because you have already
checked that max <= ep->maxpacket.
Similar issues apply to the Isochronous case.
Alan Stern
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v4] USB: gadget: epautoconf: fix ep maxpacket check
2013-10-02 15:48 ` Alan Stern
@ 2013-10-03 10:36 ` Robert Baldyga
2013-10-03 14:20 ` Alan Stern
0 siblings, 1 reply; 4+ messages in thread
From: Robert Baldyga @ 2013-10-03 10:36 UTC (permalink / raw)
To: Alan Stern
Cc: balbi, gregkh, linux-usb, linux-kernel, b.zolnierkie,
m.szyprowski, andrzej.p
Hello,
On 10/02/2013 05:48 PM, Alan Stern wrote:
> On Wed, 2 Oct 2013, Robert Baldyga wrote:
>
>> This patch fix validation of maxpacket value given in endpoint descriptor.
>> Add check of maxpacket for bulk endpoints. If maxpacket is not set in
>> descriptor, it's set to maximum value for given type on endpoint in used
>> speed.
>>
>> Correct maxpacket value is:
>>
>> FULL-SPEED HIGH-SPEED SUPER-SPEED
>> BULK 8, 16, 32, 64 512 1024
>> INTERRUPT 1..64 1..1024 1..1024
>> ISOCHRONOUS 1..1023 1..1024 1..1024
>>
>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>> ---
>>
>> Hello,
>>
>> This is fourth version of my patch. From last version I have removed
>> code reporting full speed bulk maxpacket because it's not needed since
>> maxpacket check for all speeds is performed before.
>
> It seems that this patch does a lot of things wrong. Comments below.
>
>> @@ -124,37 +124,90 @@ ep_matches (
>>
>> }
>>
>> + max = 0x7ff & usb_endpoint_maxp(desc);
>> +
>> /*
>> - * If the protocol driver hasn't yet decided on wMaxPacketSize
>> - * and wants to know the maximum possible, provide the info.
>> + * Test if maxpacket given in descriptor isn't greater than maximum
>> + * packet size for this endpoint
>> */
>> - if (desc->wMaxPacketSize == 0)
>> - desc->wMaxPacketSize = cpu_to_le16(ep->maxpacket);
>> + if (ep->maxpacket < max)
>> + return 0;
>>
>> - /* endpoint maxpacket size is an input parameter, except for bulk
>> - * where it's an output parameter representing the full speed limit.
>> - * the usb spec fixes high speed bulk maxpacket at 512 bytes.
>> + /*
>> + * Test if ep supports maxpacket size set in descriptor.
>> + * If the protocol driver hasn't yet decided on wMaxPacketSize
>> + * (when wMaxPacketSize == 0) and wants to know the maximum possible,
>> + * provide the info.
>
> This disagrees with the kerneldoc for usb_ep_autoconfig(). For bulk
> endpoints, wMaxPacket is always supposed to be set to the full-speed
> value, regardless of what the protocol driver specifies.
Hmm, it looks like all gadgets calls usb_ep_autoconfig() for full speed
descriptors and after it they uses usb_assign_descriptors() function to
set descriptors proper for device speed. And it works until gadget sets
full speed descriptors. But what if gadget supports only high speed and
don't want to set full speed descriptors? If it will use
usb_ep_autoconfig() function for high speed descriptor, value of
wMaxPacketSize field will change to 64. Is there any good solution for
this problem or all gadgets have to support full speed?
>
>> */
>> - max = 0x7ff & usb_endpoint_maxp(desc);
>> switch (type) {
>> + case USB_ENDPOINT_XFER_BULK:
>> + /*
>> + * LIMITS:
>> + * full speed: 64 bytes
>> + * high speed: 512 bytes
>> + * super speed: 1024 bytes
>> + */
>> + if (max == 0) {
>> + if (gadget_is_superspeed(gadget))
>> + desc->wMaxPacketSize = cpu_to_le16(1024);
>> + else if (gadget_is_dualspeed(gadget))
>> + desc->wMaxPacketSize = cpu_to_le16(512);
>> + else
>> + desc->wMaxPacketSize = cpu_to_le16(64);
>
> So these lines are wrong. Also, how do you know that 64 is correct for
> full speed? The hardware might only support 32.
>
>> + } else {
>> + if (max > 1024)
>> + return 0;
>> + if (!gadget_is_superspeed(gadget) && max > 512)
>> + return 0;
>> + if (!gadget_is_dualspeed(gadget) && max > 64)
>> + return 0;
>> + }
>
> For bulk endpoints, you should ignore the original value in the
> descriptor. All that matters is ep->maxpacket; it will override the
> value in the descriptor.
>
>> + break;
>> +
>> case USB_ENDPOINT_XFER_INT:
>> - /* INT: limit 64 bytes full speed, 1024 high/super speed */
>> - if (!gadget_is_dualspeed(gadget) && max > 64)
>> - return 0;
>> - /* FALLTHROUGH */
>> + /*
>> + * LIMITS:
>> + * full speed: 64 bytes
>> + * high/super speed: 1024 bytes
>> + * multiple transactions per microframe only for super speed
>
> The last comment is wrong. High speed also allows multiple interrupt
> transactions in a microframe.
>
> Also, why bother to spell out the limits in the comment? You're not
> going to use those values; you're going to use the limit in
> ep->maxpacket.
>
>> + */
>> + if (max == 0) {
>> + if (gadget_is_dualspeed(gadget))
>> + desc->wMaxPacketSize = cpu_to_le16(1024);
>> + else
>> + desc->wMaxPacketSize = cpu_to_le16(64);
>
> These values should be taken from ep->maxpacket, not from the spec.
>
>> + } else {
>> + if (max > 1024)
>> + return 0;
>> + if (!gadget_is_superspeed(gadget))
>> + if ((desc->wMaxPacketSize & cpu_to_le16(3<<11)))
>> + return 0;
>> + if (!gadget_is_dualspeed(gadget) && max > 64)
>> + return 0;
>
> The first and third tests are unnecessary, because you have already
> checked that max <= ep->maxpacket.
>
> Similar issues apply to the Isochronous case.
>
Best regards
Robert Baldyga
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v4] USB: gadget: epautoconf: fix ep maxpacket check
2013-10-03 10:36 ` Robert Baldyga
@ 2013-10-03 14:20 ` Alan Stern
0 siblings, 0 replies; 4+ messages in thread
From: Alan Stern @ 2013-10-03 14:20 UTC (permalink / raw)
To: Robert Baldyga
Cc: balbi, gregkh, linux-usb, linux-kernel, b.zolnierkie,
m.szyprowski, andrzej.p
On Thu, 3 Oct 2013, Robert Baldyga wrote:
> > This disagrees with the kerneldoc for usb_ep_autoconfig(). For bulk
> > endpoints, wMaxPacket is always supposed to be set to the full-speed
> > value, regardless of what the protocol driver specifies.
>
> Hmm, it looks like all gadgets calls usb_ep_autoconfig() for full speed
> descriptors and after it they uses usb_assign_descriptors() function to
> set descriptors proper for device speed.
Also, the gadgets have the wMaxPacketSize value hard-coded for the HS
and SS descriptors.
> And it works until gadget sets
> full speed descriptors. But what if gadget supports only high speed and
> don't want to set full speed descriptors? If it will use
> usb_ep_autoconfig() function for high speed descriptor, value of
> wMaxPacketSize field will change to 64. Is there any good solution for
> this problem or all gadgets have to support full speed?
The gadget driver can change wMaxPacketSize back to the correct value
after calling usb_ep_autoconfig().
Or you can change the definition of usb_ep_autoconfig(), and have the
driver pass the speed value as an additional argument.
Alan Stern
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-10-03 14:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-02 10:09 [PATCH v4] USB: gadget: epautoconf: fix ep maxpacket check Robert Baldyga
2013-10-02 15:48 ` Alan Stern
2013-10-03 10:36 ` Robert Baldyga
2013-10-03 14:20 ` Alan Stern
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox