[PATCH] usb/composite: fix bMaxPacketSize for SuperSpeed

[Sebastian Andrzej Siewior]

For bMaxPacketSize0 we usually take what is specified in ep0->maxpacket.
This is fine in most cases, however on SuperSpeed bMaxPacketSize0
specifies the exponent instead of the actual size in bytes. The only
valid value on SS is 9 which denotes 512 bytes.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy-***@public.gmane.org>
---
drivers/usb/gadget/composite.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 5ef8779..77dfda5 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1079,9 +1079,10 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
cdev->desc.bMaxPacketSize0 =
cdev->gadget->ep0->maxpacket;
if (gadget_is_superspeed(gadget)) {
- if (gadget->speed >= USB_SPEED_SUPER)
+ if (gadget->speed >= USB_SPEED_SUPER) {
cdev->desc.bcdUSB = cpu_to_le16(0x0300);
- else
+ cdev->desc.bMaxPacketSize0 = 9;
+ } else
cdev->desc.bcdUSB = cpu_to_le16(0x0210);
}

[Felipe Balbi]

Hi,

Post by Sebastian Andrzej Siewior
For bMaxPacketSize0 we usually take what is specified in ep0->maxpacket.
This is fine in most cases, however on SuperSpeed bMaxPacketSize0
specifies the exponent instead of the actual size in bytes. The only
valid value on SS is 9 which denotes 512 bytes.
---
drivers/usb/gadget/composite.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 5ef8779..77dfda5 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1079,9 +1079,10 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
cdev->desc.bMaxPacketSize0 =
cdev->gadget->ep0->maxpacket;
if (gadget_is_superspeed(gadget)) {
- if (gadget->speed >= USB_SPEED_SUPER)
+ if (gadget->speed >= USB_SPEED_SUPER) {
cdev->desc.bcdUSB = cpu_to_le16(0x0300);
- else
+ cdev->desc.bMaxPacketSize0 = 9;
+ } else

coding style asks you to put braces on the else branch too ;-)

[Sebastian Andrzej Siewior]

For bMaxPacketSize0 we usually take what is specified in ep0->maxpacket.
This is fine in most cases, however on SuperSpeed bMaxPacketSize0
specifies the exponent instead of the actual size in bytes. The only
valid value on SS is 9 which denotes 512 bytes.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy-***@public.gmane.org>
---
v1..v2:
- adding braces on the else path

drivers/usb/gadget/composite.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 5ef8779..aef4741 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1079,10 +1079,12 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
cdev->desc.bMaxPacketSize0 =
cdev->gadget->ep0->maxpacket;
if (gadget_is_superspeed(gadget)) {
- if (gadget->speed >= USB_SPEED_SUPER)
+ if (gadget->speed >= USB_SPEED_SUPER) {
cdev->desc.bcdUSB = cpu_to_le16(0x0300);
- else
+ cdev->desc.bMaxPacketSize0 = 9;
+ } else {
cdev->desc.bcdUSB = cpu_to_le16(0x0210);
+ }
}

value = min(w_length, (u16) sizeof cdev->desc);

[Felipe Balbi]

Post by Sebastian Andrzej Siewior
For bMaxPacketSize0 we usually take what is specified in ep0->maxpacket.
This is fine in most cases, however on SuperSpeed bMaxPacketSize0
specifies the exponent instead of the actual size in bytes. The only
valid value on SS is 9 which denotes 512 bytes.

applied, thanks

[Tanya Brokhman]

Hi Sebastian,

Post by Sebastian Andrzej Siewior
For bMaxPacketSize0 we usually take what is specified in ep0-

Post by Sebastian Andrzej Siewior
maxpacket.

This is fine in most cases, however on SuperSpeed bMaxPacketSize0
specifies the exponent instead of the actual size in bytes. The only
valid value on SS is 9 which denotes 512 bytes.

You're right but something in this patch still bothers me: according to your
patch it is possible that cdev->gadget->ep0->maxpacket (that is set by the
DCD) won't match the bMaxPacketSize0 we report to the host in the device
descriptor.
cdev->gadget->ep0->maxpacket should be determined by the DCD otherwise it
won't function correctly. So, you're right that it should be 9 for SS but it
seems to me that it should be the DCDs responsibility to set the correct
value.
In the current code the device descriptor is updated according to the value
of bMaxPacketSize0 set by the DCD:

cdev->desc.bMaxPacketSize0 = cdev->gadget->ep0->maxpacket;

I think this should remain as is. For If the DCD declared that it supports
SS but neglected to set cdev->gadget->ep0->maxpacket to the correct value it
doesn't seem right to me to update the device descriptor.
I would add a warning message (or even ERROR) if the speed is SS but
cdev->gadget->ep0->maxpacket != 9.


Thanks,
Tanya Brokhman
---
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.






--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Sebastian Andrzej Siewior]

Post by Tanya Brokhman
Hi Sebastian,

Hi Tanya,

Post by Tanya Brokhman

Post by Sebastian Andrzej Siewior
For bMaxPacketSize0 we usually take what is specified in ep0-

Post by Sebastian Andrzej Siewior
maxpacket.

This is fine in most cases, however on SuperSpeed bMaxPacketSize0
specifies the exponent instead of the actual size in bytes. The only
valid value on SS is 9 which denotes 512 bytes.

=20
You're right but something in this patch still bothers me: according =

to your

Post by Tanya Brokhman
patch it is possible that cdev->gadget->ep0->maxpacket (that is set b=

y the

Post by Tanya Brokhman
DCD) won't match the bMaxPacketSize0 we report to the host in the dev=

ice

Post by Tanya Brokhman
descriptor.
cdev->gadget->ep0->maxpacket should be determined by the DCD otherwis=

e it

Post by Tanya Brokhman
won't function correctly. So, you're right that it should be 9 for SS=

but it

Post by Tanya Brokhman
seems to me that it should be the DCDs responsibility to set the corr=

ect

Post by Tanya Brokhman
value.
In the current code the device descriptor is updated according to the=

value

Post by Tanya Brokhman
=20
cdev->desc.bMaxPacketSize0 =3D cdev->gadget->ep0->maxpacket;
=20
I think this should remain as is. For If the DCD declared that it sup=

ports

Post by Tanya Brokhman
SS but neglected to set cdev->gadget->ep0->maxpacket to the correct v=

alue it

Post by Tanya Brokhman
doesn't seem right to me to update the device descriptor.
I would add a warning message (or even ERROR) if the speed is SS but
cdev->gadget->ep0->maxpacket !=3D 9.

I've been thinking about this and decided against it:
maxpacket size is u16 and used in various for comparison for instance i=
n
the choose_ep code. It can be even 1024 on bulk SS eps. So if we change
the meaning of the field from bytes to exponent than it makes the code
more complicated as it has to check speed each time.

We don't "update" any device descriptor. ->ep0 is setup by the gadget=20
driver. There are some UDC which don't support 64 as max packet size fo=
r=20
ep0 and set maxpacket to something less. This value is taken for the
bMaxPacketSize0 descriptor field which is not set by the udc.

The same kind of is required for all other gadgets which do not use the
composite framework like the printer gadget. However they don't support=
SS
and they may be more hiding so I postponed them.

Post by Tanya Brokhman
Thanks,
Tanya Brokhman

Sebastian
--=20
Phone: [phone]; Fax.: [phone]

=46irmensitz: 88690 Uhldingen, Auf dem Berg 3
Registergericht: Amtsgericht Freiburg i. Br., HRB 700 806;
StNr. 87007/07777; Ust-Id Nr.: DE252739476
Gesch=E4ftsf=FChrer: Heinz Egger, Thomas Gleixner
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Felipe Balbi]

Hi,

Post by Sebastian Andrzej Siewior

Post by Tanya Brokhman
Hi Sebastian,

Hi Tanya,

Post by Tanya Brokhman

Post by Sebastian Andrzej Siewior
For bMaxPacketSize0 we usually take what is specified in ep0-

Post by Sebastian Andrzej Siewior
maxpacket.

This is fine in most cases, however on SuperSpeed bMaxPacketSize0
specifies the exponent instead of the actual size in bytes. The only
valid value on SS is 9 which denotes 512 bytes.

You're right but something in this patch still bothers me: according to your
patch it is possible that cdev->gadget->ep0->maxpacket (that is set by the
DCD) won't match the bMaxPacketSize0 we report to the host in the device
descriptor.
cdev->gadget->ep0->maxpacket should be determined by the DCD otherwise it
won't function correctly. So, you're right that it should be 9 for SS but it
seems to me that it should be the DCDs responsibility to set the correct
value.
In the current code the device descriptor is updated according to the value
cdev->desc.bMaxPacketSize0 = cdev->gadget->ep0->maxpacket;
I think this should remain as is. For If the DCD declared that it supports
SS but neglected to set cdev->gadget->ep0->maxpacket to the correct value it
doesn't seem right to me to update the device descriptor.
I would add a warning message (or even ERROR) if the speed is SS but
cdev->gadget->ep0->maxpacket != 9.

maxpacket size is u16 and used in various for comparison for instance in
the choose_ep code. It can be even 1024 on bulk SS eps. So if we change
the meaning of the field from bytes to exponent than it makes the code
more complicated as it has to check speed each time.

moreover ep*->maxpacket is a copy of wMaxPacketSize, not
bMaxPacketSize0 ;-)

wMaxPacketSize is 512 for SS ep0.