[PATCH] pegasus usb-net: Fix endianness bugs

[PATCH] pegasus usb-net: Fix endianness bugs

[Michael Buesch]

This fixes various endianness bugs. Some harmless and some real ones.
This is tested on a PowerPC-64 machine.

Signed-off-by: Michael Buesch <mb-fseUSCV1ubazQB+pC5nmwQ@public.gmane.org>
Cc: Stable <stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

---
 drivers/net/usb/pegasus.c |   29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

--- linux-2.6.30.orig/drivers/net/usb/pegasus.c
+++ linux-2.6.30/drivers/net/usb/pegasus.c
@@ -297,7 +297,7 @@ static int update_eth_regs_async(pegasus
 
 	pegasus->dr.bRequestType = PEGASUS_REQT_WRITE;
 	pegasus->dr.bRequest = PEGASUS_REQ_SET_REGS;
-	pegasus->dr.wValue = 0;
+	pegasus->dr.wValue = cpu_to_le16(0);
 	pegasus->dr.wIndex = cpu_to_le16(EthCtrl0);
 	pegasus->dr.wLength = cpu_to_le16(3);
 	pegasus->ctrl_urb->transfer_buffer_length = 3;
@@ -446,11 +446,12 @@ static int write_eprom_word(pegasus_t * 
 	int i;
 	__u8 tmp, d[4] = { 0x3f, 0, 0, EPROM_WRITE };
 	int ret;
+	__le16 le_data = cpu_to_le16(data);
 
 	set_registers(pegasus, EpromOffset, 4, d);
 	enable_eprom_write(pegasus);
 	set_register(pegasus, EpromOffset, index);
-	set_registers(pegasus, EpromData, 2, &data);
+	set_registers(pegasus, EpromData, 2, &le_data);
 	set_register(pegasus, EpromCtrl, EPROM_WRITE);
 
 	for (i = 0; i < REG_TIMEOUT; i++) {
@@ -923,29 +924,32 @@ static struct net_device_stats *pegasus_
 
 static inline void disable_net_traffic(pegasus_t * pegasus)
 {
-	int tmp = 0;
+	__le16 tmp = cpu_to_le16(0);
 
-	set_registers(pegasus, EthCtrl0, 2, &tmp);
+	set_registers(pegasus, EthCtrl0, sizeof(tmp), &tmp);
 }
 
 static inline void get_interrupt_interval(pegasus_t * pegasus)
 {
-	__u8 data[2];
+	u16 data;
+	u8 interval;
 
-	read_eprom_word(pegasus, 4, (__u16 *) data);
+	read_eprom_word(pegasus, 4, &data);
+	interval = data >> 8;
 	if (pegasus->usb->speed != USB_SPEED_HIGH) {
-		if (data[1] < 0x80) {
+		if (interval < 0x80) {
 			if (netif_msg_timer(pegasus))
 				dev_info(&pegasus->intf->dev, "intr interval "
 					"changed from %ums to %ums\n",
-					data[1], 0x80);
-			data[1] = 0x80;
+					interval, 0x80);
+			interval = 0x80;
+			data = (data & 0x00FF) | ((u16)interval << 8);
 #ifdef PEGASUS_WRITE_EEPROM
-			write_eprom_word(pegasus, 4, *(__u16 *) data);
+			write_eprom_word(pegasus, 4, data);
 #endif
 		}
 	}
-	pegasus->intr_interval = data[1];
+	pegasus->intr_interval = interval;
 }
 
 static void set_carrier(struct net_device *net)
@@ -1299,7 +1303,8 @@ static int pegasus_blacklisted(struct us
 	/* Special quirk to keep the driver from handling the Belkin Bluetooth
 	 * dongle which happens to have the same ID.
 	 */
-	if ((udd->idVendor == VENDOR_BELKIN && udd->idProduct == 0x0121) &&
+	if ((udd->idVendor == cpu_to_le16(VENDOR_BELKIN)) &&
+	    (udd->idProduct == cpu_to_le16(0x0121)) &&
 	    (udd->bDeviceClass == USB_CLASS_WIRELESS_CONTROLLER) &&
 	    (udd->bDeviceProtocol == 1))
 		return 1;


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

Re: [PATCH] pegasus usb-net: Fix endianness bugs

[Jiri Pirko]

Thu, Jun 18, 2009 at 07:03:47PM CEST, mb-fseUSCV1ubazQB+pC5nmwQ@public.gmane.org wrote:
>This fixes various endianness bugs. Some harmless and some real ones.
>This is tested on a PowerPC-64 machine.
>
>Signed-off-by: Michael Buesch <mb-fseUSCV1ubazQB+pC5nmwQ@public.gmane.org>
>Cc: Stable <stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>
>---
> drivers/net/usb/pegasus.c |   29 +++++++++++++++++------------
> 1 file changed, 17 insertions(+), 12 deletions(-)
>
>--- linux-2.6.30.orig/drivers/net/usb/pegasus.c
>+++ linux-2.6.30/drivers/net/usb/pegasus.c
>@@ -297,7 +297,7 @@ static int update_eth_regs_async(pegasus
> 
> 	pegasus->dr.bRequestType = PEGASUS_REQT_WRITE;
> 	pegasus->dr.bRequest = PEGASUS_REQ_SET_REGS;
>-	pegasus->dr.wValue = 0;
>+	pegasus->dr.wValue = cpu_to_le16(0);
Is this necessary? I mean zero is still zero :-)

> 	pegasus->dr.wIndex = cpu_to_le16(EthCtrl0);
> 	pegasus->dr.wLength = cpu_to_le16(3);
> 	pegasus->ctrl_urb->transfer_buffer_length = 3;
>@@ -446,11 +446,12 @@ static int write_eprom_word(pegasus_t * 
> 	int i;
> 	__u8 tmp, d[4] = { 0x3f, 0, 0, EPROM_WRITE };
> 	int ret;
>+	__le16 le_data = cpu_to_le16(data);
> 
> 	set_registers(pegasus, EpromOffset, 4, d);
> 	enable_eprom_write(pegasus);
> 	set_register(pegasus, EpromOffset, index);
>-	set_registers(pegasus, EpromData, 2, &data);
>+	set_registers(pegasus, EpromData, 2, &le_data);
> 	set_register(pegasus, EpromCtrl, EPROM_WRITE);
> 
> 	for (i = 0; i < REG_TIMEOUT; i++) {
>@@ -923,29 +924,32 @@ static struct net_device_stats *pegasus_
> 
> static inline void disable_net_traffic(pegasus_t * pegasus)
> {
>-	int tmp = 0;
>+	__le16 tmp = cpu_to_le16(0);
> 
>-	set_registers(pegasus, EthCtrl0, 2, &tmp);
>+	set_registers(pegasus, EthCtrl0, sizeof(tmp), &tmp);
> }
> 
> static inline void get_interrupt_interval(pegasus_t * pegasus)
> {
>-	__u8 data[2];
>+	u16 data;
>+	u8 interval;
> 
>-	read_eprom_word(pegasus, 4, (__u16 *) data);
>+	read_eprom_word(pegasus, 4, &data);
>+	interval = data >> 8;
> 	if (pegasus->usb->speed != USB_SPEED_HIGH) {
>-		if (data[1] < 0x80) {
>+		if (interval < 0x80) {
> 			if (netif_msg_timer(pegasus))
> 				dev_info(&pegasus->intf->dev, "intr interval "
> 					"changed from %ums to %ums\n",
>-					data[1], 0x80);
>-			data[1] = 0x80;
>+					interval, 0x80);
>+			interval = 0x80;
>+			data = (data & 0x00FF) | ((u16)interval << 8);
                                                  ^^^^^ you do not need this
							  typecast
> #ifdef PEGASUS_WRITE_EEPROM
>-			write_eprom_word(pegasus, 4, *(__u16 *) data);
>+			write_eprom_word(pegasus, 4, data);
> #endif
> 		}
> 	}
>-	pegasus->intr_interval = data[1];
>+	pegasus->intr_interval = interval;
> }
> 
> static void set_carrier(struct net_device *net)
>@@ -1299,7 +1303,8 @@ static int pegasus_blacklisted(struct us
> 	/* Special quirk to keep the driver from handling the Belkin Bluetooth
> 	 * dongle which happens to have the same ID.
> 	 */
>-	if ((udd->idVendor == VENDOR_BELKIN && udd->idProduct == 0x0121) &&
>+	if ((udd->idVendor == cpu_to_le16(VENDOR_BELKIN)) &&
>+	    (udd->idProduct == cpu_to_le16(0x0121)) &&
> 	    (udd->bDeviceClass == USB_CLASS_WIRELESS_CONTROLLER) &&
> 	    (udd->bDeviceProtocol == 1))
> 		return 1;
>
>
>-- 
>Greetings, Michael.
>--
>To unsubscribe from this list: send the line "unsubscribe netdev" in
>the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pegasus usb-net: Fix endianness bugs

[Michael Buesch]

On Thursday 18 June 2009 19:46:22 Jiri Pirko wrote:
> >--- linux-2.6.30.orig/drivers/net/usb/pegasus.c
> >+++ linux-2.6.30/drivers/net/usb/pegasus.c
> >@@ -297,7 +297,7 @@ static int update_eth_regs_async(pegasus
> > 
> > 	pegasus->dr.bRequestType = PEGASUS_REQT_WRITE;
> > 	pegasus->dr.bRequest = PEGASUS_REQ_SET_REGS;
> >-	pegasus->dr.wValue = 0;
> >+	pegasus->dr.wValue = cpu_to_le16(0);
> Is this necessary? I mean zero is still zero :-)

Well, yes. However wValue is a little endian variable and 0 is CPU endian.
The fact that 0 is represented with the same bits in LE and BE doesn't really
matter. It documents the fact that we really require this to be a LE value.
GCC will recognize it and optimize it away, so it's not a runtime issue.
And  wValue = cpu_to_le16(0)  is used in the rest of the driver, too. This is the
only place that doesn't use it. So let's fix it. If only for consistency.

> >+	u8 interval;
> > 
> >-	read_eprom_word(pegasus, 4, (__u16 *) data);
> >+	read_eprom_word(pegasus, 4, &data);
> >+	interval = data >> 8;
> > 	if (pegasus->usb->speed != USB_SPEED_HIGH) {
> >-		if (data[1] < 0x80) {
> >+		if (interval < 0x80) {
> > 			if (netif_msg_timer(pegasus))
> > 				dev_info(&pegasus->intf->dev, "intr interval "
> > 					"changed from %ums to %ums\n",
> >-					data[1], 0x80);
> >-			data[1] = 0x80;
> >+					interval, 0x80);
> >+			interval = 0x80;
> >+			data = (data & 0x00FF) | ((u16)interval << 8);
>                                                   ^^^^^ you do not need this
> 							  typecast

Hm, well. The cast is there because "interval" is an 8bit variable.
I think the behavior of C is machine dependent in such a situation.
But as Linux doesn't run on anything with less than 32bit registers, it probably
doesn't matter.

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

Re: [PATCH] pegasus usb-net: Fix endianness bugs

[David Miller]

From: Michael Buesch <mb-fseUSCV1ubazQB+pC5nmwQ@public.gmane.org>
Date: Thu, 18 Jun 2009 19:03:47 +0200

> This fixes various endianness bugs. Some harmless and some real ones.
> This is tested on a PowerPC-64 machine.
> 
> Signed-off-by: Michael Buesch <mb-fseUSCV1ubazQB+pC5nmwQ@public.gmane.org>
> Cc: Stable <stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Applied and queued up for -stable, thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html