Skip to contentSkip to conversation

: USB: Fix ARM CONFIG_USB_TTY boards

PATCH New Tom Rini 2 participants 3 messages Dec 14, 2011 – Dec 15, 2011 USB

lore
Share & tools
lore

Tom Rini

author
#0 lore raw

cover : USB: Fix ARM CONFIG_USB_TTY boards

Hey all,

As has been reported in a few threads now, and thanks to Aneesh V's analysis we know why Pandaboard/Beagleboard (and probably a number of other ARM boards) currently hang on boot. The following patch is based on Stefan's list of possible solutions to the problem he first encountered on OpenRISC which lead to the first patch. This has been tested on Panda and Beagle and OpenRISC and I think we must have this for v2011.12 release. Thanks all!

Signature · 1 line
Tom
author
#1 lore raw

PATCH 1/1 USB: Use (get|put)_unaligned_le16 for accessing wMaxPacketSize

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 <[email protected]>
Cc Remy Bohmer <[email protected]>

OpenRISC:

Beagleboard xM, Pandaboard run-tested, s5p_goni build-tested.

---
 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(-)
common/cmd_usb.c +2 -1
index d4ec2a2..52ecb43 100644
@@ -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");
common/usb.c +15 -8
index 4418c70..6e35f7d 100644
@@ -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:
drivers/serial/usbtty.c +6 -4
index e2e87fe..0061937 100644
@@ -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;
drivers/usb/gadget/epautoconf.c +5 -3
index 1896489..97a07ad 100644
@@ -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@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;
 }
drivers/usb/gadget/s3c_udc_otg.c +6 -4
index 5a3ac78..e743509 100644
@@ -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);
include/usbdescriptors.h +1 -1
index 392fcf5..2dec3b9 100644
@@ -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;
Signature · 1 line
1.7.0.4
#2 lore raw

PATCH 1/1 USB: Use (get|put)_unaligned_le16 for accessing wMaxPacketSize

Hi Tom,

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

Quoted 7 lines · 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.

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

common/usb.c int usb_parse_config(struct usb_device *dev, unsigned char *buffer, int cfgno) · line 392

Quoted 10 lines
common/usb.c +5 -2
-			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