diff mbox

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

Message ID 1323901203-9274-2-git-send-email-trini@ti.com
State Accepted
Delegated to: Remy Bohmer
Headers show

Commit Message

Tom Rini Dec. 14, 2011, 10:20 p.m. UTC 
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 <mk7.kang@samsung.com>
Cc: Remy Bohmer <linux@bohmer.net>

OpenRISC:
Tested-by: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>

Beagleboard xM, Pandaboard run-tested, s5p_goni build-tested.
Signed-off-by: Tom Rini <trini@ti.com>
---
 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(-)

Comments

Stefan Kristiansson Dec. 15, 2011, 6:05 a.m. UTC | #1 
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
diff mbox

Patch

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 <common.h>
 #include <command.h>
 #include <asm/byteorder.h>
+#include <asm/unaligned.h>
 #include <part.h>
 #include <usb.h>
 
@@ -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 <asm/processor.h>
 #include <linux/ctype.h>
 #include <asm/byteorder.h>
+#include <asm/unaligned.h>
 
 #include <usb.h>
 #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].\
+							wMaxPacketSize);
+			le16_to_cpus(&ep_wMaxPacketSize);
 			USB_PRINTF("if %d, ep %d\n", ifno, epno);
 			break;
 		default:
diff --git a/drivers/serial/usbtty.c b/drivers/serial/usbtty.c
index e2e87fe..0061937 100644
--- a/drivers/serial/usbtty.c
+++ b/drivers/serial/usbtty.c
@@ -25,6 +25,7 @@ 
 #include <config.h>
 #include <circbuf.h>
 #include <stdio_dev.h>
+#include <asm/unaligned.h>
 #include "usbtty.h"
 #include "usb_cdc_acm.h"
 #include "usbdescriptors.h"
@@ -626,6 +627,9 @@  static void usbtty_init_strings (void)
 	usb_strings = usbtty_string_table;
 }
 
+#define init_wMaxPacketSize(x)	le16_to_cpu(get_unaligned_le16(\
+			&ep_descriptor_ptrs[(x) - 1]->wMaxPacketSize));
+
 static void usbtty_init_instances (void)
 {
 	int i;
@@ -688,14 +692,12 @@  static void usbtty_init_instances (void)
 		endpoint_instance[i].rcv_attributes =
 			ep_descriptor_ptrs[i - 1]->bmAttributes;
 
-		endpoint_instance[i].rcv_packetSize =
-			le16_to_cpu(ep_descriptor_ptrs[i - 1]->wMaxPacketSize);
+		endpoint_instance[i].rcv_packetSize = init_wMaxPacketSize(i);
 
 		endpoint_instance[i].tx_attributes =
 			ep_descriptor_ptrs[i - 1]->bmAttributes;
 
-		endpoint_instance[i].tx_packetSize =
-			le16_to_cpu(ep_descriptor_ptrs[i - 1]->wMaxPacketSize);
+		endpoint_instance[i].tx_packetSize = init_wMaxPacketSize(i);
 
 		endpoint_instance[i].tx_attributes =
 			ep_descriptor_ptrs[i - 1]->bmAttributes;
diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
index 1896489..97a07ad 100644
--- a/drivers/usb/gadget/epautoconf.c
+++ b/drivers/usb/gadget/epautoconf.c
@@ -25,6 +25,7 @@ 
 #include <linux/usb/ch9.h>
 #include <asm/errno.h>
 #include <linux/usb/gadget.h>
+#include <asm/unaligned.h>
 #include "gadget_chips.h"
 
 #define isdigit(c)      ('0' <= (c) && (c) <= '9')
@@ -127,7 +128,7 @@  static int ep_matches(
 	 * where it's an output parameter representing the full speed limit.
 	 * the usb spec fixes high speed bulk maxpacket at 512 bytes.
 	 */
-	max = 0x7ff & le16_to_cpu(desc->wMaxPacketSize);
+	max = 0x7ff & le16_to_cpu(get_unaligned_le16(&desc->wMaxPacketSize));
 	switch (type) {
 	case USB_ENDPOINT_XFER_INT:
 		/* INT:  limit 64 bytes full speed, 1024 high speed */
@@ -143,7 +144,8 @@  static int ep_matches(
 			return 0;
 
 		/* BOTH:  "high bandwidth" works only at high speed */
-		if ((desc->wMaxPacketSize & __constant_cpu_to_le16(3<<11))) {
+		if ((get_unaligned_le16(&desc->wMaxPacketSize) &
+					__constant_cpu_to_le16(3<<11))) {
 			if (!gadget->is_dualspeed)
 				return 0;
 			/* configure your hardware with enough buffering!! */
@@ -176,7 +178,7 @@  static int ep_matches(
 		/* min() doesn't work on bitfields with gcc-3.5 */
 		if (size > 64)
 			size = 64;
-		desc->wMaxPacketSize = cpu_to_le16(size);
+		put_unaligned_le16(cpu_to_le16(size), &desc->wMaxPacketSize);
 	}
 	return 1;
 }
diff --git a/drivers/usb/gadget/s3c_udc_otg.c b/drivers/usb/gadget/s3c_udc_otg.c
index 5a3ac78..e743509 100644
--- a/drivers/usb/gadget/s3c_udc_otg.c
+++ b/drivers/usb/gadget/s3c_udc_otg.c
@@ -40,6 +40,7 @@ 
 #include <linux/usb/gadget.h>
 
 #include <asm/byteorder.h>
+#include <asm/unaligned.h>
 #include <asm/io.h>
 
 #include <asm/mach-types.h>
@@ -586,7 +587,8 @@  static int s3c_ep_enable(struct usb_ep *_ep,
 	if (!_ep || !desc || ep->desc || _ep->name == ep0name
 	    || desc->bDescriptorType != USB_DT_ENDPOINT
 	    || ep->bEndpointAddress != desc->bEndpointAddress
-	    || ep_maxpacket(ep) < le16_to_cpu(desc->wMaxPacketSize)) {
+	    || ep_maxpacket(ep) <
+	    le16_to_cpu(get_unaligned_le16(&desc->wMaxPacketSize))) {
 
 		DEBUG("%s: bad ep or descriptor\n", __func__);
 		return -EINVAL;
@@ -603,8 +605,8 @@  static int s3c_ep_enable(struct usb_ep *_ep,
 
 	/* hardware _could_ do smaller, but driver doesn't */
 	if ((desc->bmAttributes == USB_ENDPOINT_XFER_BULK
-	     && le16_to_cpu(desc->wMaxPacketSize) != ep_maxpacket(ep))
-	    || !desc->wMaxPacketSize) {
+	     && le16_to_cpu(get_unaligned_le16(&desc->wMaxPacketSize)) !=
+	     ep_maxpacket(ep)) || !get_unaligned_le16(&desc->wMaxPacketSize)) {
 
 		DEBUG("%s: bad %s maxpacket\n", __func__, _ep->name);
 		return -ERANGE;
@@ -620,7 +622,7 @@  static int s3c_ep_enable(struct usb_ep *_ep,
 	ep->stopped = 0;
 	ep->desc = desc;
 	ep->pio_irqs = 0;
-	ep->ep.maxpacket = le16_to_cpu(desc->wMaxPacketSize);
+	ep->ep.maxpacket = le16_to_cpu(get_unaligned_le16(&desc->wMaxPacketSize));
 
 	/* Reset halt state */
 	s3c_udc_set_nak(ep);
diff --git a/include/usbdescriptors.h b/include/usbdescriptors.h
index 392fcf5..2dec3b9 100644
--- a/include/usbdescriptors.h
+++ b/include/usbdescriptors.h
@@ -199,7 +199,7 @@  struct usb_endpoint_descriptor {
 	u8 bmAttributes;
 	u16 wMaxPacketSize;
 	u8 bInterval;
-} __attribute__ ((packed)) __attribute__ ((aligned(2)));
+} __attribute__ ((packed));
 
 struct usb_interface_descriptor {
 	u8 bLength;