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 ...
[ message continues ]

QW0gRG9ubmVyc3RhZyAxNSBNYWkgMjAwOCAwODoxOToyNCBzY2hyaWViIEJyeWFuIFd1Ogo+IC0t
LSBhL2RyaXZlcnMvdXNiL2hvc3QvdWhjaS1odWIuYwo+ICsrKyBiL2RyaXZlcnMvdXNiL2hvc3Qv
dWhjaS1odWIuYwo+IEBAIC0yNTMsNyArMjUzLDcgQEAgc3RhdGljIGludCB1aGNpX2h1Yl9jb250
cm9sKHN0cnVjdCB1c2JfaGNkICpoY2QsIHUxNiB0eXBlUmVxLCB1MTYgd1ZhbHVlLAo+IMKgwqDC
oMKgwqDCoMKgwqBzd2l0Y2ggKHR5cGVSZXEpIHsKPiDCoAo+IMKgwqDCoMKgwqDCoMKgwqBjYXNl
IEdldEh1YlN0YXR1czoKPiAtwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgKihfX2xlMzIg
KilidWYgPSBjcHVfdG9fbGUzMigwKTsKPiArwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
cHV0X3VuYWxpZ25lZF9sZTMyKDAsIGJ1Zik7CgpXaGF0IGlzIHN1cHBvc2VkIHRvIG1ha2UgYWxs
IHRoZXNlIGNoYW5nZXMgYSBnb29kIGlkZWE/CgoJUmVnYXJkcwoJCU9saXZlcgoK
--

[ message continues ]

Since buf might not be 4-byte aligned.


Jie
--

[ message continues ]

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=a5abde...

-Bryan
--

[ message continues ]

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

	Regards
		Oliver
--

[ message continues ]

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

[ message continues ]

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=cb3da1...
http://blackfin.uclinux.org/git/?p=readonly-mirrors/linux-kernel.git;a=commit;h=255929...

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=92164c...

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=54bee6...

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

[ message continues ]

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

	Regards
		Oliver
--

[ message continues ]

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

--

[ message continues ]

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

	Regards
		Oliver
--

[ message continues ]

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

--

[ message continues ]

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 ...
[ message continues ]

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

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

--

[ message continues ]

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



--

[ message continues ]

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

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

Alan Stern

--

[ message continues ]

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


Harvey

--

[ message continues ]

Looks fine.


--

[ message continues ]

On Thu, 15 May 2008 14:19:24 +0800

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

[ message continues ]