Re: [U-Boot] [PATCH] USB: Use (get|put)_unaligned_le16 for accessing wMaxPacketSize

[Stefan Kristiansson]

Hi Tom,

after taking a second look at this a couple of things came to mind

On Wed, Dec 14, 2011 at 03:20:03PM -0700, Tom Rini wrote:
> In 9792987721c7980453fe6447c3fa6593b44f8458 Stefan describes a usecase
> where the previous behavior of leaving wMaxPacketSize be unaligned
> caused fatal problems.  The initial fix for this problem was incomplete
> however as it showed another cases of non-aligned access that previously
> worked implicitly.  This switches to making sure that all access of
> wMaxPacketSize are done via (get|put)_unaligned_le16.
> 

Why the _le16? Shouldn't it just be (get|put)_unaligned?

> - le16_to_cpus(&(dev->config.if_desc[ifno].ep_desc[epno].\
> -wMaxPacketSize));
> + ep_wMaxPacketSize = get_unaligned_le16(&dev->config.\
> + if_desc[ifno].\
> + ep_desc[epno].\
> + wMaxPacketSize);
> + le16_to_cpus(&ep_wMaxPacketSize);
>   USB_PRINTF("if %d, ep %d\n", ifno, epno);
>   break;
>   default:

Since this code is changing the wMaxPacketSize, it should probably be:

ep_wMaxPacketSize = get_unaligned(&dev->config.\
if_desc[ifno].\
ep_desc[epno].\
wMaxPacketSize);
put_unaligned(le16_to_cpu(ep_wMaxPacketSize),
&dev->config.\
if_desc[ifno].\
ep_desc[epno].\
wMaxPacketSize);

Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] USB: Use (get|put)_unaligned_le16 for accessing wMaxPacketSize

[Tom Rini]

In 9792987721c7980453fe6447c3fa6593b44f8458 Stefan describes a usecase
where the previous behavior of leaving wMaxPacketSize be unaligned
caused fatal problems.  The initial fix for this problem was incomplete
however as it showed another cases of non-aligned access that previously
worked implicitly.  This switches to making sure that all access of
wMaxPacketSize are done via (get|put)_unaligned_le16.

In order to maintain a level of readability to the code in some cases
we now use a variable for the value of wMaxPacketSize and in others, a
macro.

Cc: Minkyu Kang 
Cc: Remy Bohmer 

OpenRISC:
Tested-by: Stefan Kristiansson 

Beagleboard xM, Pandaboard run-tested, s5p_goni build-tested.
Signed-off-by: Tom Rini 
---
 common/cmd_usb.c |3 ++-
 common/usb.c |   23 +++
 drivers/serial/usbtty.c  |   10 ++
 drivers/usb/gadget/epautoconf.c  |8 +---
 drivers/usb/gadget/s3c_udc_otg.c |   10 ++
 include/usbdescriptors.h |2 +-
 6 files changed, 35 insertions(+), 21 deletions(-)

diff --git a/common/cmd_usb.c b/common/cmd_usb.c
index d4ec2a2..52ecb43 100644
--- a/common/cmd_usb.c
+++ b/common/cmd_usb.c
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -240,7 +241,7 @@ void usb_display_ep_desc(struct usb_endpoint_descriptor 
*epdesc)
printf("Interrupt");
break;
}
-   printf(" MaxPacket %d", epdesc->wMaxPacketSize);
+   printf(" MaxPacket %d", get_unaligned_le16(&epdesc->wMaxPacketSize));
if ((epdesc->bmAttributes & 0x03) == 0x3)
printf(" Interval %dms", epdesc->bInterval);
printf("\n");
diff --git a/common/usb.c b/common/usb.c
index 4418c70..6e35f7d 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -49,6 +49,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #ifdef CONFIG_4xx
@@ -279,30 +280,32 @@ usb_set_maxpacket_ep(struct usb_device *dev, int if_idx, 
int ep_idx)
 {
int b;
struct usb_endpoint_descriptor *ep;
+   u16 ep_wMaxPacketSize;
 
ep = &dev->config.if_desc[if_idx].ep_desc[ep_idx];
 
b = ep->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
+   ep_wMaxPacketSize = get_unaligned_le16(&ep->wMaxPacketSize);
 
if ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) ==
USB_ENDPOINT_XFER_CONTROL) {
/* Control => bidirectional */
-   dev->epmaxpacketout[b] = ep->wMaxPacketSize;
-   dev->epmaxpacketin[b] = ep->wMaxPacketSize;
+   dev->epmaxpacketout[b] = ep_wMaxPacketSize;
+   dev->epmaxpacketin[b] = ep_wMaxPacketSize;
USB_PRINTF("##Control EP epmaxpacketout/in[%d] = %d\n",
   b, dev->epmaxpacketin[b]);
} else {
if ((ep->bEndpointAddress & 0x80) == 0) {
/* OUT Endpoint */
-   if (ep->wMaxPacketSize > dev->epmaxpacketout[b]) {
-   dev->epmaxpacketout[b] = ep->wMaxPacketSize;
+   if (ep_wMaxPacketSize > dev->epmaxpacketout[b]) {
+   dev->epmaxpacketout[b] = ep_wMaxPacketSize;
USB_PRINTF("##EP epmaxpacketout[%d] = %d\n",
   b, dev->epmaxpacketout[b]);
}
} else {
/* IN Endpoint */
-   if (ep->wMaxPacketSize > dev->epmaxpacketin[b]) {
-   dev->epmaxpacketin[b] = ep->wMaxPacketSize;
+   if (ep_wMaxPacketSize > dev->epmaxpacketin[b]) {
+   dev->epmaxpacketin[b] = ep_wMaxPacketSize;
USB_PRINTF("##EP epmaxpacketin[%d] = %d\n",
   b, dev->epmaxpacketin[b]);
}
@@ -333,6 +336,7 @@ int usb_parse_config(struct usb_device *dev, unsigned char 
*buffer, int cfgno)
struct usb_descriptor_header *head;
int index, ifno, epno, curr_if_num;
int i;
+   u16 ep_wMaxPacketSize;
 
ifno = -1;
epno = -1;
@@ -378,8 +382,11 @@ int usb_parse_config(struct usb_device *dev, unsigned char 
*buffer, int cfgno)
dev->config.if_desc[ifno].no_of_ep++;
memcpy(&dev->config.if_desc[ifno].ep_desc[epno],
&buffer[index], buffer[index]);
-   le16_to_cpus(&(dev->config.if_desc[ifno].ep_desc[epno].\
-  wMaxPacketSize));
+   ep_wMaxPacketSize = get_unaligned_le16(&dev->config.\
+   if_desc[ifno].\
+   ep_desc[epno].\
+