Skip to contentSkip to conversation

: use get/put_unaligned_* helpers to fix more potential unaligned issues.

PATCH In review Bryan Wu 7 participants 18 messages May 15, 2008 – May 17, 2008

Subsystems (3)
lore
Share & tools
lore

Bryan Wu

author
#0 lore raw

PATCH 1/1 : use get/put_unaligned_* helpers to fix more potential unaligned issues.

---
 drivers/usb/host/isp116x-hcd.c |    4 ++--
 drivers/usb/host/uhci-hub.c    |    6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)
drivers/usb/host/isp116x-hcd.c +2 -2
index 20b9a0d..97ac6ff 100644
@@ -1024,7 +1024,7 @@ static int isp116x_hub_control(struct usb_hcd *hcd,
 		break;
 	case GetHubStatus:
 		DBG("GetHubStatus\n");
-		*(__le32 *) buf = 0;
+		put_unaligned_le32(0, buf);
 		break;
 	case GetPortStatus:
 		DBG("GetPortStatus\n");
@@ -1033,7 +1033,7 @@ static int isp116x_hub_control(struct usb_hcd *hcd,
 		spin_lock_irqsave(&isp116x->lock, flags);
 		tmp = isp116x_read_reg32(isp116x, (--wIndex) ? HCRHPORT2 : HCRHPORT1);
 		spin_unlock_irqrestore(&isp116x->lock, flags);
-		*(__le32 *) buf = cpu_to_le32(tmp);
+		put_unaligned_le32(tmp, buf);
 		DBG("GetPortStatus: port[%d]  %08x\n", wIndex + 1, tmp);
 		break;
 	case ClearPortFeature:
drivers/usb/host/uhci-hub.c +3 -3
index 8e4427a..801fa99 100644
@@ -253,7 +253,7 @@ static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 	switch (typeReq) {
 
 	case GetHubStatus:
-		*(__le32 *)buf = cpu_to_le32(0);
+		put_unaligned_le32(0, buf);
 		OK(4);		/* hub power */
 	case GetPortStatus:
 		if (port >= uhci->rh_numports)
@@ -306,8 +306,8 @@ static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 			dev_dbg(uhci_dev(uhci), "port %d portsc %04x,%02x\n",
 					wIndex, status, lstatus);
 
-		*(__le16 *)buf = cpu_to_le16(wPortStatus);
-		*(__le16 *)(buf + 2) = cpu_to_le16(wPortChange);
+		put_unaligned_le16(wPortStatus, buf);
+		put_unaligned_le16(wPortChange, buf + 2);
 		OK(4);
 	case SetHubFeature:		/* We don't implement these */
 	case ClearHubFeature:
Signature · 1 line
1.5.5
#1 lore raw

Am Donnerstag 15 Mai 2008 08:19:24 schrieb Bryan Wu:> --- a/drivers/usb/host/uhci-hub.c> +++ b/drivers/usb/host/uhci-hub.c> @@ -253,7 +253,7 @@ static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,>         switch (typeReq) {>  >         case GetHubStatus:> -               *(__le32 *)buf = cpu_to_le32(0);> +               put_unaligned_le32(0, buf); What is supposed to make all these changes a good idea?
Regards Oliver
���{.n�+�������+%�����ݶ��w��{.n�+����{��G�����{ay�ʇڙ�,j��f���h��������z_�(�階�ݢj"���m������G������?����&���~��iO��z��v�^�m���� ������?�I�

#2 lore raw
Quoted 6 lines · Oliver Neukum

Am Donnerstag 15 Mai 2008 08:19:24 schrieb Bryan Wu:

Quoted 5 lines
drivers/usb/host/uhci-hub.c +0 -0
@@ -253,7 +253,7 @@ static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
        switch (typeReq) {
 
Quoted 6 lines

drivers/usb/host/uhci-hub.c static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, · line 256

Quoted 3 lines
drivers/usb/host/uhci-hub.c +1 -1
        case GetHubStatus:
-               *(__le32 *)buf = cpu_to_le32(0);
+               put_unaligned_le32(0, buf);

What is supposed to make all these changes a good idea?

Since buf might not be 4-byte aligned.

Jie

author
#3 lore raw
Quoted 15 lines · Jie Zhang
Quoted 14 lines · Oliver Neukum

Am Donnerstag 15 Mai 2008 08:19:24 schrieb Bryan Wu:

drivers/usb/host/uhci-hub.c static int uhci_hub_control(struct usb_hcd *hcd, u16 · line 256

Quoted 9 lines
drivers/usb/host/uhci-hub.c +1 -1
@@ -253,7 +253,7 @@ static int uhci_hub_control(struct usb_hcd *hcd, u16
 typeReq, u16 wValue,
       switch (typeReq) {
         case GetHubStatus:
-               *(__le32 *)buf = cpu_to_le32(0);
+               put_unaligned_le32(0, buf);

What is supposed to make all these changes a good idea?

Quoted 2 lines

Since buf might not be 4-byte aligned.

And this change follows Harvey Harrison's patch's style:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=a5abdeafedf722b0f3f357f4a23089a686b1b80d

-Bryan

#4 lore raw
In reply to Jie Zhang

Am Donnerstag 15 Mai 2008 10:03:35 schrieb Jie Zhang:

Quoted 7 lines
Quoted 6 lines · Oliver Neukum

Am Donnerstag 15 Mai 2008 08:19:24 schrieb Bryan Wu:

Quoted 5 lines
drivers/usb/host/uhci-hub.c +0 -0
@@ -253,7 +253,7 @@ static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
        switch (typeReq) {
 
Quoted 7 lines
Quoted 6 lines

drivers/usb/host/uhci-hub.c static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, · line 256

Quoted 3 lines
drivers/usb/host/uhci-hub.c +1 -1
        case GetHubStatus:
-               *(__le32 *)buf = cpu_to_le32(0);
+               put_unaligned_le32(0, buf);

What is supposed to make all these changes a good idea?

Since buf might not be 4-byte aligned.

It is. Please analyze the code before you use these access methods.

Regards
Oliver

#5 lore raw
Quoted 8 lines · Oliver Neukum

Am Donnerstag 15 Mai 2008 10:03:35 schrieb Jie Zhang:

Quoted 7 lines
Quoted 6 lines · Oliver Neukum

Am Donnerstag 15 Mai 2008 08:19:24 schrieb Bryan Wu:

Quoted 5 lines
drivers/usb/host/uhci-hub.c +0 -0
@@ -253,7 +253,7 @@ static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
        switch (typeReq) {
 
Quoted 9 lines
Quoted 6 lines
Quoted 5 lines

drivers/usb/host/uhci-hub.c static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, · line 256

Quoted 3 lines
drivers/usb/host/uhci-hub.c +1 -1
        case GetHubStatus:
-               *(__le32 *)buf = cpu_to_le32(0);
+               put_unaligned_le32(0, buf);

What is supposed to make all these changes a good idea?

Since buf might not be 4-byte aligned.

It is. Please analyze the code before you use these access methods.

You are right. buf has been 4-byte aligned since 2.6.19. My patch was written two years ago. Sorry for the noise I caused.

Jie

author
#6 lore raw
Quoted 19 lines · Jie Zhang
Quoted 18 lines · Oliver Neukum

Am Donnerstag 15 Mai 2008 10:03:35 schrieb Jie Zhang:

Quoted 16 lines
Quoted 14 lines · Oliver Neukum

Am Donnerstag 15 Mai 2008 08:19:24 schrieb Bryan Wu:

drivers/usb/host/uhci-hub.c static int uhci_hub_control(struct usb_hcd *hcd, · line 256

Quoted 9 lines
drivers/usb/host/uhci-hub.c +1 -1
@@ -253,7 +253,7 @@ static int uhci_hub_control(struct usb_hcd *hcd,
 u16 typeReq, u16 wValue,
       switch (typeReq) {
         case GetHubStatus:
-               *(__le32 *)buf = cpu_to_le32(0);
+               put_unaligned_le32(0, buf);

What is supposed to make all these changes a good idea?

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

Since buf might not be 4-byte aligned.

It is. Please analyze the code before you use these access methods.

You are right. buf has been 4-byte aligned since 2.6.19. My patch was written two years ago. Sorry for the noise I caused.

I has keeping this patch for a long time. Jie fixed this patch at 2006/09/20 in our svn:
http://blackfin.uclinux.org/git/?p=readonly-mirrors/linux-kernel.git;a=commit;h=cb3da1243f84b37b53486c7e86da34565b4c5d92
http://blackfin.uclinux.org/git/?p=readonly-mirrors/linux-kernel.git;a=commit;h=2559298f0dca2cffc0b87390b92a484004f0d85e

And a similar patch from David Miller was accept in ohci-hub.c
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=92164c5dd1ade33f4e90b72e407910de6694de49

Also because of the same issue, which was fixed by:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=54bee6e1b455573658972510a76119f279db32b7

If other functions not only rh_call_control() call this hub_control() pointer and the buf is not 4-byte aligned,
this bug will fire again without the unaligned API. This patch is safer for the caller, although not efficient.

-Bryan

#7 lore raw

Am Donnerstag 15 Mai 2008 11:54:51 schrieb Bryan Wu:

Quoted 36 lines · Jie Zhang
Quoted 19 lines
Quoted 18 lines · Oliver Neukum

Am Donnerstag 15 Mai 2008 10:03:35 schrieb Jie Zhang:

Quoted 16 lines
Quoted 14 lines · Oliver Neukum

Am Donnerstag 15 Mai 2008 08:19:24 schrieb Bryan Wu:

drivers/usb/host/uhci-hub.c static int uhci_hub_control(struct usb_hcd *hcd, · line 256

Quoted 9 lines
drivers/usb/host/uhci-hub.c +1 -1
@@ -253,7 +253,7 @@ static int uhci_hub_control(struct usb_hcd *hcd,
 u16 typeReq, u16 wValue,
       switch (typeReq) {
         case GetHubStatus:
-               *(__le32 *)buf = cpu_to_le32(0);
+               put_unaligned_le32(0, buf);

What is supposed to make all these changes a good idea?

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

Since buf might not be 4-byte aligned.

It is. Please analyze the code before you use these access methods.

You are right. buf has been 4-byte aligned since 2.6.19. My patch was written two years ago. Sorry for the noise I caused.

I has keeping this patch for a long time. Jie fixed this patch at 2006/09/20 in our svn:
http://blackfin.uclinux.org/git/?p=readonly-mirrors/linux-kernel.git;a=commit;h=cb3da1243f84b37b53486c7e86da34565b4c5d92
http://blackfin.uclinux.org/git/?p=readonly-mirrors/linux-kernel.git;a=commit;h=2559298f0dca2cffc0b87390b92a484004f0d85e

And a similar patch from David Miller was accept in ohci-hub.c
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=92164c5dd1ade33f4e90b72e407910de6694de49

Quoted 7 lines

Also because of the same issue, which was fixed by:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=54bee6e1b455573658972510a76119f279db32b7

If other functions not only rh_call_control() call this hub_control() pointer and the buf is not 4-byte aligned,
this bug will fire again without the unaligned API. This patch is safer for the caller, although not efficient.

This is not very nice. If we pass around unaligned pointers we should mark those not catch errors later on.

Regards
Oliver

#8 lore raw
Quoted 2 lines · Oliver Neukum

This is not very nice. If we pass around unaligned pointers we should mark those not catch errors later on.

ISTR that GCC doesn't have a way to annotate pointers that way.

#9 lore raw

Am Donnerstag 15 Mai 2008 15:03:20 schrieben Sie:

Quoted 5 lines · Oliver Neukum
Quoted 2 lines

This is not very nice. If we pass around unaligned pointers we should mark those not catch errors later on.

ISTR that GCC doesn't have a way to annotate pointers that way.

But we can do the _user annotation, can't we?

Regards
Oliver

maintainer
#10 lore raw
In reply to Bryan Wu
Quoted 18 lines · Bryan Wu
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
Quoted 2 lines

What is supposed to make all these changes a good idea?

Since buf might not be 4-byte aligned.

It is. Please analyze the code before you use these access methods.

You are right. buf has been 4-byte aligned since 2.6.19. My patch was written two years ago. Sorry for the noise I caused.

I has keeping this patch for a long time. Jie fixed this patch at 2006/09/20 in our svn:
http://blackfin.uclinux.org/git/?p=readonly-mirrors/linux-kernel.git;a=commit;h=cb3da1243f84b37b53486c7e86da34565b4c5d92
http://blackfin.uclinux.org/git/?p=readonly-mirrors/linux-kernel.git;a=commit;h=2559298f0dca2cffc0b87390b92a484004f0d85e

And a similar patch from David Miller was accept in ohci-hub.c
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=92164c5dd1ade33f4e90b72e407910de6694de49

Quoted 7 lines

Also because of the same issue, which was fixed by:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=54bee6e1b455573658972510a76119f279db32b7

If other functions not only rh_call_control() call this hub_control() pointer and the buf is not 4-byte aligned,
this bug will fire again without the unaligned API. This patch is safer for the caller, although not efficient.

But the hub_control routines are almost never called by anything other than rh_call_control(). The only exception is in ehci_hub.c, and there the buf pointer is NULL.

So I don't see any need for this patch.

On the other hand, Jie could make a bunch of useful changes to drivers/usb/gadget/file_storage.c, which has its own private routines for unaligned little-endian access.

Alan Stern

#11 lore raw

PATCH 1/1 usb: file-storage.c use unaligned access helpers

Replace the put_be16/32 and get_be16/32 helpers with the common unaligned access routines. Note that these put_ helpers had the pointer/value parameter in the opposite order from the common version.

---
 drivers/usb/gadget/file_storage.c |   79 ++++++++++++-------------------------
 1 files changed, 25 insertions(+), 54 deletions(-)
drivers/usb/gadget/file_storage.c +25 -54
index 47bb9f0..9405961 100644
@@ -242,6 +242,8 @@
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 
+#include <asm/unaligned.h>
+
 #include "gadget_chips.h"
 
 
@@ -763,37 +765,6 @@ static int fsg_set_halt(struct fsg_dev *fsg, struct usb_ep *ep)
 	return usb_ep_set_halt(ep);
 }
 
-
-/*-------------------------------------------------------------------------*/
-
-/* Routines for unaligned data access */
-
-static u16 get_be16(u8 *buf)
-{
-	return ((u16) buf[0] << 8) | ((u16) buf[1]);
-}
-
-static u32 get_be32(u8 *buf)
-{
-	return ((u32) buf[0] << 24) | ((u32) buf[1] << 16) |
-			((u32) buf[2] << 8) | ((u32) buf[3]);
-}
-
-static void put_be16(u8 *buf, u16 val)
-{
-	buf[0] = val >> 8;
-	buf[1] = val;
-}
-
-static void put_be32(u8 *buf, u32 val)
-{
-	buf[0] = val >> 24;
-	buf[1] = val >> 16;
-	buf[2] = val >> 8;
-	buf[3] = val & 0xff;
-}
-
-
 /*-------------------------------------------------------------------------*/
 
 /*
@@ -1551,9 +1522,9 @@ static int do_read(struct fsg_dev *fsg)
 	/* Get the starting Logical Block Address and check that it's
 	 * not too big */
 	if (fsg->cmnd[0] == SC_READ_6)
-		lba = (fsg->cmnd[1] << 16) | get_be16(&fsg->cmnd[2]);
+		lba = get_unaligned_be32(fsg->cmnd) & 0x00ffffff;
 	else {
-		lba = get_be32(&fsg->cmnd[2]);
+		lba = get_unaligned_be32(&fsg->cmnd[2]);
 
 		/* We allow DPO (Disable Page Out = don't save data in the
 		 * cache) and FUA (Force Unit Access = don't read from the
@@ -1684,9 +1655,9 @@ static int do_write(struct fsg_dev *fsg)
 	/* Get the starting Logical Block Address and check that it's
 	 * not too big */
 	if (fsg->cmnd[0] == SC_WRITE_6)
-		lba = (fsg->cmnd[1] << 16) | get_be16(&fsg->cmnd[2]);
+		lba = get_unaligned_be32(fsg->cmnd) & 0x00ffffff;
 	else {
-		lba = get_be32(&fsg->cmnd[2]);
+		lba = get_unaligned_be32(&fsg->cmnd[2]);
 
 		/* We allow DPO (Disable Page Out = don't save data in the
 		 * cache) and FUA (Force Unit Access = write directly to the
@@ -1920,7 +1891,7 @@ static int do_verify(struct fsg_dev *fsg)
 
 	/* Get the starting Logical Block Address and check that it's
 	 * not too big */
-	lba = get_be32(&fsg->cmnd[2]);
+	lba = get_unaligned_be32(&fsg->cmnd[2]);
 	if (lba >= curlun->num_sectors) {
 		curlun->sense_data = SS_LOGICAL_BLOCK_ADDRESS_OUT_OF_RANGE;
 		return -EINVAL;
@@ -1933,7 +1904,7 @@ static int do_verify(struct fsg_dev *fsg)
 		return -EINVAL;
 	}
 
-	verification_length = get_be16(&fsg->cmnd[7]);
+	verification_length = get_unaligned_be16(&fsg->cmnd[7]);
 	if (unlikely(verification_length == 0))
 		return -EIO;		// No default reply
 
@@ -2078,7 +2049,7 @@ static int do_request_sense(struct fsg_dev *fsg, struct fsg_buffhd *bh)
 	memset(buf, 0, 18);
 	buf[0] = valid | 0x70;			// Valid, current error
 	buf[2] = SK(sd);
-	put_be32(&buf[3], sdinfo);		// Sense information
+	put_unaligned_be32(sdinfo, &buf[3]);		// Sense information
 	buf[7] = 18 - 8;			// Additional sense length
 	buf[12] = ASC(sd);
 	buf[13] = ASCQ(sd);
@@ -2089,7 +2060,7 @@ static int do_request_sense(struct fsg_dev *fsg, struct fsg_buffhd *bh)
 static int do_read_capacity(struct fsg_dev *fsg, struct fsg_buffhd *bh)
 {
 	struct lun	*curlun = fsg->curlun;
-	u32		lba = get_be32(&fsg->cmnd[2]);
+	u32		lba = get_unaligned_be32(&fsg->cmnd[2]);
 	int		pmi = fsg->cmnd[8];
 	u8		*buf = (u8 *) bh->buf;
 
@@ -2099,8 +2070,8 @@ static int do_read_capacity(struct fsg_dev *fsg, struct fsg_buffhd *bh)
 		return -EINVAL;
 	}
 
-	put_be32(&buf[0], curlun->num_sectors - 1);	// Max logical block
-	put_be32(&buf[4], 512);				// Block length
+	put_unaligned_be32(curlun->num_sectors - 1, &buf[0]);	// Max logical block
+	put_unaligned_be32(512, &buf[4]);				// Block length
 	return 8;
 }
 
@@ -2158,10 +2129,10 @@ static int do_mode_sense(struct fsg_dev *fsg, struct fsg_buffhd *bh)
 			buf[2] = 0x04;	// Write cache enable,
 					// Read cache not disabled
 					// No cache retention priorities
-			put_be16(&buf[4], 0xffff);  // Don't disable prefetch
+			put_unaligned_be16(0xffff, &buf[4]);  // Don't disable prefetch
 					// Minimum prefetch = 0
-			put_be16(&buf[8], 0xffff);  // Maximum prefetch
-			put_be16(&buf[10], 0xffff); // Maximum prefetch ceiling
+			put_unaligned_be16(0xffff, &buf[8]);  // Maximum prefetch
+			put_unaligned_be16(0xffff, &buf[10]); // Maximum prefetch ceiling
 		}
 		buf += 12;
 	}
@@ -2178,7 +2149,7 @@ static int do_mode_sense(struct fsg_dev *fsg, struct fsg_buffhd *bh)
 	if (mscmnd == SC_MODE_SENSE_6)
 		buf0[0] = len - 1;
 	else
-		put_be16(buf0, len - 2);
+		put_unaligned_be16(len - 2, buf0);
 	return len;
 }
 
@@ -2266,8 +2237,8 @@ static int do_read_format_capacities(struct fsg_dev *fsg,
 	buf[3] = 8;		// Only the Current/Maximum Capacity Descriptor
 	buf += 4;
 
-	put_be32(&buf[0], curlun->num_sectors);		// Number of blocks
-	put_be32(&buf[4], 512);				// Block length
+	put_unaligned_be32(curlun->num_sectors, &buf[0]);	// Number of blocks
+	put_unaligned_be32(512, &buf[4]);			// Block length
 	buf[4] = 0x02;					// Current capacity
 	return 12;
 }
@@ -2775,7 +2746,7 @@ static int do_scsi_command(struct fsg_dev *fsg)
 		break;
 
 	case SC_MODE_SELECT_10:
-		fsg->data_size_from_cmnd = get_be16(&fsg->cmnd[7]);
+		fsg->data_size_from_cmnd = get_unaligned_be16(&fsg->cmnd[7]);
 		if ((reply = check_command(fsg, 10, DATA_DIR_FROM_HOST,
 				(1<<1) | (3<<7), 0,
 				"MODE SELECT(10)")) == 0)
@@ -2791,7 +2762,7 @@ static int do_scsi_command(struct fsg_dev *fsg)
 		break;
 
 	case SC_MODE_SENSE_10:
-		fsg->data_size_from_cmnd = get_be16(&fsg->cmnd[7]);
+		fsg->data_size_from_cmnd = get_unaligned_be16(&fsg->cmnd[7]);
 		if ((reply = check_command(fsg, 10, DATA_DIR_TO_HOST,
 				(1<<1) | (1<<2) | (3<<7), 0,
 				"MODE SENSE(10)")) == 0)
@@ -2816,7 +2787,7 @@ static int do_scsi_command(struct fsg_dev *fsg)
 		break;
 
 	case SC_READ_10:
-		fsg->data_size_from_cmnd = get_be16(&fsg->cmnd[7]) << 9;
+		fsg->data_size_from_cmnd = get_unaligned_be16(&fsg->cmnd[7]) << 9;
 		if ((reply = check_command(fsg, 10, DATA_DIR_TO_HOST,
 				(1<<1) | (0xf<<2) | (3<<7), 1,
 				"READ(10)")) == 0)
@@ -2824,7 +2795,7 @@ static int do_scsi_command(struct fsg_dev *fsg)
 		break;
 
 	case SC_READ_12:
-		fsg->data_size_from_cmnd = get_be32(&fsg->cmnd[6]) << 9;
+		fsg->data_size_from_cmnd = get_unaligned_be32(&fsg->cmnd[6]) << 9;
 		if ((reply = check_command(fsg, 12, DATA_DIR_TO_HOST,
 				(1<<1) | (0xf<<2) | (0xf<<6), 1,
 				"READ(12)")) == 0)
@@ -2840,7 +2811,7 @@ static int do_scsi_command(struct fsg_dev *fsg)
 		break;
 
 	case SC_READ_FORMAT_CAPACITIES:
-		fsg->data_size_from_cmnd = get_be16(&fsg->cmnd[7]);
+		fsg->data_size_from_cmnd = get_unaligned_be16(&fsg->cmnd[7]);
 		if ((reply = check_command(fsg, 10, DATA_DIR_TO_HOST,
 				(3<<7), 1,
 				"READ FORMAT CAPACITIES")) == 0)
@@ -2898,7 +2869,7 @@ static int do_scsi_command(struct fsg_dev *fsg)
 		break;
 
 	case SC_WRITE_10:
-		fsg->data_size_from_cmnd = get_be16(&fsg->cmnd[7]) << 9;
+		fsg->data_size_from_cmnd = get_unaligned_be16(&fsg->cmnd[7]) << 9;
 		if ((reply = check_command(fsg, 10, DATA_DIR_FROM_HOST,
 				(1<<1) | (0xf<<2) | (3<<7), 1,
 				"WRITE(10)")) == 0)
@@ -2906,7 +2877,7 @@ static int do_scsi_command(struct fsg_dev *fsg)
 		break;
 
 	case SC_WRITE_12:
-		fsg->data_size_from_cmnd = get_be32(&fsg->cmnd[6]) << 9;
+		fsg->data_size_from_cmnd = get_unaligned_be32(&fsg->cmnd[6]) << 9;
 		if ((reply = check_command(fsg, 12, DATA_DIR_FROM_HOST,
 				(1<<1) | (0xf<<2) | (0xf<<6), 1,
 				"WRITE(12)")) == 0)
Signature · 1 line
1.5.5.1.482.g0f174
maintainer
#12 lore raw

Re: [PATCH] usb: file-storage.c use unaligned access helpers

Quoted 4 lines · Harvey Harrison

Replace the put_be16/32 and get_be16/32 helpers with the common unaligned access routines. Note that these put_ helpers had the pointer/value parameter in the opposite order from the common version.

This is fine and it's what I requested. Still, it's not really the best solution.

In many of the places where these helpers are used, we _know_ that the buffers are in fact aligned properly. For example, in code like this:

Review

Quoted 2 lines
(quoted diff) +1 -1
-			put_be16(&buf[4], 0xffff);
+			put_unaligned_be16(0xffff, &buf[4]);

we know that buf is aligned. It would be great if there were helpers which would allow us to do

			put_be16(&buf[4], 0xffff);

instead of

* (__be16 *) &buf[4] = cpu_to_be16(0xffff);

They could also be used to clean up the disputed HCD code. Is this a reasonable thing to ask for? A few simple, arch-independent macros would be sufficient.

Alan Stern

#13 lore raw

[RFC-PATCH] lib: add byteorder helpers for the aligned case

Some users know the pointer they are writing to are aligned, rather than doing *(__le16 *)ptr = cpu_to_le16(val) add helpers wrapping this up that have the same convention as put_unaligned_le/be.

---
Alan, as requested, I'm looking around a bit to see if there are actual users for this. But it does make a nice complement to the unaligned versions.

 include/linux/byteorder/generic.h |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)
include/linux/byteorder/generic.h +30 -0
index 0846e6b..38ff3e6 100644
@@ -119,6 +119,36 @@
 #define cpu_to_be16s __cpu_to_be16s
 #define be16_to_cpus __be16_to_cpus
 
+static inline void put_le16(u16 val, void *ptr)
+{
+	*(__le16 *)ptr = cpu_to_le16(val);
+}
+
+static inline void put_le32(u32 val, void *ptr)
+{
+	*(__le32 *)ptr = cpu_to_le32(val);
+}
+
+static inline void put_le64(u64 val, void *ptr)
+{
+	*(__le64 *)ptr = cpu_to_le64(val);
+}
+
+static inline void put_be16(u16 val, void *ptr)
+{
+	*(__be16 *)ptr = cpu_to_be16(val);
+}
+
+static inline void put_be32(u32 val, void *ptr)
+{
+	*(__be32 *)ptr = cpu_to_be32(val);
+}
+
+static inline void put_be64(u64 val, void *ptr)
+{
+	*(__be64 *)ptr = cpu_to_be64(val);
+}
+
 /*
  * They have to be macros in order to do the constant folding
  * correctly - if the argument passed into a inline function
Signature · 1 line
1.5.5.1.570.g26b5e
maintainer
#14 lore raw

Re: [RFC-PATCH] lib: add byteorder helpers for the aligned case

Quoted 9 lines · Harvey Harrison

Some users know the pointer they are writing to are aligned, rather than doing *(__le16 *)ptr = cpu_to_le16(val) add helpers wrapping this up that have the same convention as put_unaligned_le/be.

---
Alan, as requested, I'm looking around a bit to see if there are actual users for this. But it does make a nice complement to the unaligned versions.

This is great -- thanks! I've wanted this sort of thing for a long time. There's a good chance that the SCSI core could make use of it. Try looking at the prep functions in sd.c and sr.c.

include/linux/byteorder/generic.h line 125

Quoted 4 lines
include/linux/byteorder/generic.h +4 -0
+static inline void put_le16(u16 val, void *ptr)
+{
+	*(__le16 *)ptr = cpu_to_le16(val);
+}

Is this able to do the byte rearrangement at compile time if val is a compile-time constant? I imagine it would.

Alan Stern

#15 lore raw

Re: [RFC-PATCH] lib: add byteorder helpers for the aligned case

Quoted 10 lines · Alan Stern

include/linux/byteorder/generic.h line 125

Quoted 4 lines · Harvey Harrison
include/linux/byteorder/generic.h +4 -0
+static inline void put_le16(u16 val, void *ptr)
+{
+	*(__le16 *)ptr = cpu_to_le16(val);
+}

Is this able to do the byte rearrangement at compile time if val is a compile-time constant? I imagine it would.

Yes, the cpu_to_le/be functions to get optimized if they are passed a compile time constant.

Harvey

Reviewed-by on v1 1/1
#16 lore raw
In reply to Bryan Wu
Quoted 1 line · Bryan Wu
Cc Harvey Harrison <[email protected]>

Looks fine.

Quoted 53 lines
---
 drivers/usb/host/isp116x-hcd.c |    4 ++--
 drivers/usb/host/uhci-hub.c    |    6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)
drivers/usb/host/isp116x-hcd.c +2 -2
index 20b9a0d..97ac6ff 100644
@@ -1024,7 +1024,7 @@ static int isp116x_hub_control(struct usb_hcd *hcd,
 		break;
 	case GetHubStatus:
 		DBG("GetHubStatus\n");
-		*(__le32 *) buf = 0;
+		put_unaligned_le32(0, buf);
 		break;
 	case GetPortStatus:
 		DBG("GetPortStatus\n");
@@ -1033,7 +1033,7 @@ static int isp116x_hub_control(struct usb_hcd *hcd,
 		spin_lock_irqsave(&isp116x->lock, flags);
 		tmp = isp116x_read_reg32(isp116x, (--wIndex) ? HCRHPORT2 : HCRHPORT1);
 		spin_unlock_irqrestore(&isp116x->lock, flags);
-		*(__le32 *) buf = cpu_to_le32(tmp);
+		put_unaligned_le32(tmp, buf);
 		DBG("GetPortStatus: port[%d]  %08x\n", wIndex + 1, tmp);
 		break;
 	case ClearPortFeature:
drivers/usb/host/uhci-hub.c +3 -3
index 8e4427a..801fa99 100644
@@ -253,7 +253,7 @@ static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 	switch (typeReq) {
 
 	case GetHubStatus:
-		*(__le32 *)buf = cpu_to_le32(0);
+		put_unaligned_le32(0, buf);
 		OK(4);		/* hub power */
 	case GetPortStatus:
 		if (port >= uhci->rh_numports)
@@ -306,8 +306,8 @@ static int uhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 			dev_dbg(uhci_dev(uhci), "port %d portsc %04x,%02x\n",
 					wIndex, status, lstatus);
 
-		*(__le16 *)buf = cpu_to_le16(wPortStatus);
-		*(__le16 *)(buf + 2) = cpu_to_le16(wPortChange);
+		put_unaligned_le16(wPortStatus, buf);
+		put_unaligned_le16(wPortChange, buf + 2);
 		OK(4);
 	case SetHubFeature:		/* We don't implement these */
 	case ClearHubFeature:
#17 lore raw
In reply to Bryan Wu
Quoted 1 line · On Thu, 15 May 2008 14:19:24 +0800 Bryan Wu <[email protected]> wrote:

Subject: [PATCH 1/1] [usb/host]: use get/put_unaligned_* helpers to fix more potential unaligned issues.

Please don't put the subsystem identifier ("usb/host") inside square brackets. Because text in square brackets is considered "not part of the patch title" and gets thrown away. It contains text such as "patch", "BUG", "RFC", "2.6.26-rc2", etc.

That should be in Documentation/SubmittingPatches but I can't find it. hrm.