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@xxxxxxxxxxx>
> ---
>
> 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
--
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
