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

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

[Bryan Wu]

Cc: Harvey Harrison <harvey.harrison@gmail.com>
Signed-off-by: Jie Zhang <jie.zhang@analog.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 drivers/usb/host/isp116x-hcd.c |    4 ++--
 drivers/usb/host/uhci-hub.c    |    6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/isp116x-hcd.c b/drivers/usb/host/isp116x-hcd.c
index 20b9a0d..97ac6ff 100644
--- a/drivers/usb/host/isp116x-hcd.c
+++ b/drivers/usb/host/isp116x-hcd.c
@@ -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:
diff --git a/drivers/usb/host/uhci-hub.c b/drivers/usb/host/uhci-hub.c
index 8e4427a..801fa99 100644
--- 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);
 		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:
-- 
1.5.5

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

[Oliver Neukum]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 652 bytes --]

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Ç+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Jie Zhang]

Oliver Neukum wrote:
> 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?
> 
Since buf might not be 4-byte aligned.


Jie

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

[Bryan Wu]

On Thu, May 15, 2008 at 4:03 PM, Jie Zhang <jie.zhang@analog.com> wrote:
> Oliver Neukum wrote:
>>
>> 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?
>>
> 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

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

[Oliver Neukum]

Am Donnerstag 15 Mai 2008 10:03:35 schrieb Jie Zhang:
> Oliver Neukum wrote:
> > 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?
> > 
> Since buf might not be 4-byte aligned.

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

	Regards
		Oliver

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

[Jie Zhang]

Oliver Neukum wrote:
> Am Donnerstag 15 Mai 2008 10:03:35 schrieb Jie Zhang:
>> Oliver Neukum wrote:
>>> 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?
>>>
>> 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

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

[Bryan Wu]

On Thu, May 15, 2008 at 5:15 PM, Jie Zhang <jie.zhang@analog.com> wrote:
> Oliver Neukum wrote:
>>
>> Am Donnerstag 15 Mai 2008 10:03:35 schrieb Jie Zhang:
>>>
>>> Oliver Neukum wrote:
>>>>
>>>> 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?
>>>>
>>> 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

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

[Oliver Neukum]

Am Donnerstag 15 Mai 2008 11:54:51 schrieb Bryan Wu:
> On Thu, May 15, 2008 at 5:15 PM, Jie Zhang <jie.zhang@analog.com> wrote:
> > Oliver Neukum wrote:
> >>
> >> Am Donnerstag 15 Mai 2008 10:03:35 schrieb Jie Zhang:
> >>>
> >>> Oliver Neukum wrote:
> >>>>
> >>>> 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?
> >>>>
> >>> 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.

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

	Regards
		Oliver

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

[David Brownell]

On Thursday 15 May 2008, Oliver Neukum wrote:
> 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.

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

[Oliver Neukum]

Am Donnerstag 15 Mai 2008 15:03:20 schrieben Sie:
> On Thursday 15 May 2008, Oliver Neukum wrote:
> > 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

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

[Alan Stern]

On Thu, 15 May 2008, Bryan Wu wrote:

> >>>> 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
> 
> 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

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

[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.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 drivers/usb/gadget/file_storage.c |   79 ++++++++++++-------------------------
 1 files changed, 25 insertions(+), 54 deletions(-)

diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c
index 47bb9f0..9405961 100644
--- a/drivers/usb/gadget/file_storage.c
+++ b/drivers/usb/gadget/file_storage.c
@@ -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)
-- 
1.5.5.1.482.g0f174

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

[Alan Stern]

On Thu, 15 May 2008, Harvey Harrison wrote:

> 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:

> -			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

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

[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.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
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(-)

diff --git a/include/linux/byteorder/generic.h b/include/linux/byteorder/generic.h
index 0846e6b..38ff3e6 100644
--- a/include/linux/byteorder/generic.h
+++ b/include/linux/byteorder/generic.h
@@ -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
-- 
1.5.5.1.570.g26b5e

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

[Alan Stern]

On Fri, 16 May 2008, Harvey Harrison wrote:

> 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.
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> ---
> 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.

> +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

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

[Harvey Harrison]

On Fri, 2008-05-16 at 20:35 -0400, Alan Stern wrote:
> On Fri, 16 May 2008, Harvey Harrison wrote:
> 
> > +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

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

[Harvey Harrison]

On Thu, 2008-05-15 at 14:19 +0800, Bryan Wu wrote:
> Cc: Harvey Harrison <harvey.harrison@gmail.com>

Looks fine.

Reviewed-by: Harvey Harrison <harvey.harrison@gmail.com>

> Signed-off-by: Jie Zhang <jie.zhang@analog.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>
> ---
>  drivers/usb/host/isp116x-hcd.c |    4 ++--
>  drivers/usb/host/uhci-hub.c    |    6 +++---
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/host/isp116x-hcd.c b/drivers/usb/host/isp116x-hcd.c
> index 20b9a0d..97ac6ff 100644
> --- a/drivers/usb/host/isp116x-hcd.c
> +++ b/drivers/usb/host/isp116x-hcd.c
> @@ -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:
> diff --git a/drivers/usb/host/uhci-hub.c b/drivers/usb/host/uhci-hub.c
> index 8e4427a..801fa99 100644
> --- 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);
>  		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:

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

[Andrew Morton]

On Thu, 15 May 2008 14:19:24 +0800
Bryan Wu <cooloney@kernel.org> 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.