[U-Boot] [PATCH 1/1] USB: Fix strict aliasing in ohci-hcd

[U-Boot] [PATCH 1/1] USB: Fix strict aliasing in ohci-hcd

[Troy Kisky]

commit 5f6aa03fda2a0a79940765865c1e4266be8a75f8
    USB: Fix complaints about strict aliasing in OHCI-HCD

tried to fix this, but gcc4.4 still complains. So, this
patch basically reverts the above and does a simpler fix.

also, the above commit incorrectly changed
	/* corresponds to data_buf[4-7] */
	datab [1] = 0;
to

	/* corresponds to databuf.u8[4-7] */
	databuf.u8[1] = 0;

This patch also fixes that.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 drivers/usb/host/ohci-hcd.c |   70 +++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 39 deletions(-)

diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index d24f2f1..9f47351 100644
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -1261,19 +1261,11 @@ static int ohci_submit_rh_msg(struct usb_device *dev, unsigned long pipe,
 	int leni = transfer_len;
 	int len = 0;
 	int stat = 0;
-	__u32 datab[4];
-	union {
-		void *ptr;
-		__u8 *u8;
-		__u16 *u16;
-		__u32 *u32;
-	} databuf;
 	__u16 bmRType_bReq;
 	__u16 wValue;
 	__u16 wIndex;
 	__u16 wLength;
-
-	databuf.u32 = (__u32 *)datab;
+	ALLOC_ALIGN_BUFFER(__u8, databuf, 16, sizeof(u32));
 
 #ifdef DEBUG
 pkt_print(NULL, dev, pipe, buffer, transfer_len,
@@ -1304,20 +1296,20 @@ pkt_print(NULL, dev, pipe, buffer, transfer_len,
 	*/
 
 	case RH_GET_STATUS:
-		databuf.u16[0] = cpu_to_le16(1);
+		*(u16 *)databuf = cpu_to_le16(1);
 		OK(2);
 	case RH_GET_STATUS | RH_INTERFACE:
-		databuf.u16[0] = cpu_to_le16(0);
+		*(u16 *)databuf = cpu_to_le16(0);
 		OK(2);
 	case RH_GET_STATUS | RH_ENDPOINT:
-		databuf.u16[0] = cpu_to_le16(0);
+		*(u16 *)databuf = cpu_to_le16(0);
 		OK(2);
 	case RH_GET_STATUS | RH_CLASS:
-		databuf.u32[0] = cpu_to_le32(
+		*(u32 *)databuf = cpu_to_le32(
 				RD_RH_STAT & ~(RH_HS_CRWE | RH_HS_DRWE));
 		OK(4);
 	case RH_GET_STATUS | RH_OTHER | RH_CLASS:
-		databuf.u32[0] = cpu_to_le32(RD_RH_PORTSTAT);
+		*(u32 *)databuf = cpu_to_le32(RD_RH_PORTSTAT);
 		OK(4);
 
 	case RH_CLEAR_FEATURE | RH_ENDPOINT:
@@ -1381,14 +1373,14 @@ pkt_print(NULL, dev, pipe, buffer, transfer_len,
 					min_t(unsigned int,
 					sizeof(root_hub_dev_des),
 					wLength));
-			databuf.ptr = root_hub_dev_des; OK(len);
+			databuf = root_hub_dev_des; OK(len);
 		case (0x02): /* configuration descriptor */
 			len = min_t(unsigned int,
 					leni,
 					min_t(unsigned int,
 					sizeof(root_hub_config_des),
 					wLength));
-			databuf.ptr = root_hub_config_des; OK(len);
+			databuf = root_hub_config_des; OK(len);
 		case (0x03): /* string descriptors */
 			if (wValue == 0x0300) {
 				len = min_t(unsigned int,
@@ -1396,7 +1388,7 @@ pkt_print(NULL, dev, pipe, buffer, transfer_len,
 						min_t(unsigned int,
 						sizeof(root_hub_str_index0),
 						wLength));
-				databuf.ptr = root_hub_str_index0;
+				databuf = root_hub_str_index0;
 				OK(len);
 			}
 			if (wValue == 0x0301) {
@@ -1405,7 +1397,7 @@ pkt_print(NULL, dev, pipe, buffer, transfer_len,
 						min_t(unsigned int,
 						sizeof(root_hub_str_index1),
 						wLength));
-				databuf.ptr = root_hub_str_index1;
+				databuf = root_hub_str_index1;
 				OK(len);
 		}
 		default:
@@ -1417,40 +1409,40 @@ pkt_print(NULL, dev, pipe, buffer, transfer_len,
 	{
 		__u32 temp = roothub_a(&gohci);
 
-		databuf.u8[0] = 9;		/* min length; */
-		databuf.u8[1] = 0x29;
-		databuf.u8[2] = temp & RH_A_NDP;
+		databuf[0] = 9;		/* min length; */
+		databuf[1] = 0x29;
+		databuf[2] = temp & RH_A_NDP;
 #ifdef CONFIG_AT91C_PQFP_UHPBUG
-		databuf.u8[2] = (databuf.u8[2] == 2) ? 1 : 0;
+		databuf[2] = (databuf[2] == 2) ? 1 : 0;
 #endif
-		databuf.u8[3] = 0;
+		databuf[3] = 0;
 		if (temp & RH_A_PSM)	/* per-port power switching? */
-			databuf.u8[3] |= 0x1;
+			databuf[3] |= 0x1;
 		if (temp & RH_A_NOCP)	/* no overcurrent reporting? */
-			databuf.u8[3] |= 0x10;
+			databuf[3] |= 0x10;
 		else if (temp & RH_A_OCPM)/* per-port overcurrent reporting? */
-			databuf.u8[3] |= 0x8;
+			databuf[3] |= 0x8;
 
-		/* corresponds to databuf.u8[4-7] */
-		databuf.u8[1] = 0;
-		databuf.u8[5] = (temp & RH_A_POTPGT) >> 24;
+		databuf[4] = 0;
+		databuf[5] = (temp & RH_A_POTPGT) >> 24;
+		databuf[6] = 0;
 		temp = roothub_b(&gohci);
-		databuf.u8[7] = temp & RH_B_DR;
-		if (databuf.u8[2] < 7) {
-			databuf.u8[8] = 0xff;
+		databuf[7] = temp & RH_B_DR;
+		if (databuf[2] < 7) {
+			databuf[8] = 0xff;
 		} else {
-			databuf.u8[0] += 2;
-			databuf.u8[8] = (temp & RH_B_DR) >> 8;
-			databuf.u8[10] = databuf.u8[9] = 0xff;
+			databuf[0] += 2;
+			databuf[8] = (temp & RH_B_DR) >> 8;
+			databuf[10] = databuf[9] = 0xff;
 		}
 
 		len = min_t(unsigned int, leni,
-			    min_t(unsigned int, databuf.u8[0], wLength));
+			    min_t(unsigned int, databuf[0], wLength));
 		OK(len);
 	}
 
 	case RH_GET_CONFIGURATION:
-		databuf.u8[0] = 0x01;
+		databuf[0] = 0x01;
 		OK(1);
 
 	case RH_SET_CONFIGURATION:
@@ -1469,8 +1461,8 @@ pkt_print(NULL, dev, pipe, buffer, transfer_len,
 #endif
 
 	len = min_t(int, len, leni);
-	if (data != databuf.ptr)
-		memcpy(data, databuf.ptr, len);
+	if (data != databuf)
+		memcpy(data, databuf, len);
 	dev->act_len = len;
 	dev->status = stat;
 
-- 
1.7.9.5

[U-Boot] [PATCH 1/1] USB: Fix strict aliasing in ohci-hcd

[Marek Vasut]

Dear Troy Kisky,

> commit 5f6aa03fda2a0a79940765865c1e4266be8a75f8
>     USB: Fix complaints about strict aliasing in OHCI-HCD
> 
> tried to fix this, but gcc4.4 still complains. So, this
> patch basically reverts the above and does a simpler fix.
> 
> also, the above commit incorrectly changed
> 	/* corresponds to data_buf[4-7] */
> 	datab [1] = 0;
> to
> 
> 	/* corresponds to databuf.u8[4-7] */
> 	databuf.u8[1] = 0;
> 
> This patch also fixes that.
[...]

AGAIN?! :-C

If I were to run git log on that file, I'd get like five such "fixes" :-(

I'll review this properly tomorrow, this will be certainly a very pleasant 
reviewing time ;-)

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/1] USB: Fix strict aliasing in ohci-hcd

[Marek Vasut]

Dear Troy Kisky,

> commit 5f6aa03fda2a0a79940765865c1e4266be8a75f8
>     USB: Fix complaints about strict aliasing in OHCI-HCD
> 
> tried to fix this, but gcc4.4 still complains. So, this
> patch basically reverts the above and does a simpler fix.
> 
> also, the above commit incorrectly changed
> 	/* corresponds to data_buf[4-7] */
> 	datab [1] = 0;
> to
> 
> 	/* corresponds to databuf.u8[4-7] */
> 	databuf.u8[1] = 0;
> 
> This patch also fixes that.
> 
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> ---
>  drivers/usb/host/ohci-hcd.c |   70
> +++++++++++++++++++------------------------ 1 file changed, 31
> insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
> index d24f2f1..9f47351 100644
> --- a/drivers/usb/host/ohci-hcd.c
> +++ b/drivers/usb/host/ohci-hcd.c
> @@ -1261,19 +1261,11 @@ static int ohci_submit_rh_msg(struct usb_device
> *dev, unsigned long pipe, int leni = transfer_len;
>  	int len = 0;
>  	int stat = 0;
> -	__u32 datab[4];
> -	union {
> -		void *ptr;
> -		__u8 *u8;
> -		__u16 *u16;
> -		__u32 *u32;
> -	} databuf;
>  	__u16 bmRType_bReq;
>  	__u16 wValue;
>  	__u16 wIndex;
>  	__u16 wLength;
> -
> -	databuf.u32 = (__u32 *)datab;
> +	ALLOC_ALIGN_BUFFER(__u8, databuf, 16, sizeof(u32));

Ok, pardon my sloppiness in reviews ... but why not alloc this cache aligned? 
That way we'd also circumvent the issues with caches we'd eventually hit anyway.

[...]

The rest is OK.

[U-Boot] [PATCH 1/1] USB: Fix strict aliasing in ohci-hcd

[Troy Kisky]

On 8/14/2012 10:33 AM, Marek Vasut wrote:
> Dear Troy Kisky,
>
>> commit 5f6aa03fda2a0a79940765865c1e4266be8a75f8
>>      USB: Fix complaints about strict aliasing in OHCI-HCD
>>
>> tried to fix this, but gcc4.4 still complains. So, this
>> patch basically reverts the above and does a simpler fix.
>>
>> also, the above commit incorrectly changed
>> 	/* corresponds to data_buf[4-7] */
>> 	datab [1] = 0;
>> to
>>
>> 	/* corresponds to databuf.u8[4-7] */
>> 	databuf.u8[1] = 0;
>>
>> This patch also fixes that.
>>
>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>> ---
>>   drivers/usb/host/ohci-hcd.c |   70
>> +++++++++++++++++++------------------------ 1 file changed, 31
>> insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
>> index d24f2f1..9f47351 100644
>> --- a/drivers/usb/host/ohci-hcd.c
>> +++ b/drivers/usb/host/ohci-hcd.c
>> @@ -1261,19 +1261,11 @@ static int ohci_submit_rh_msg(struct usb_device
>> *dev, unsigned long pipe, int leni = transfer_len;
>>   	int len = 0;
>>   	int stat = 0;
>> -	__u32 datab[4];
>> -	union {
>> -		void *ptr;
>> -		__u8 *u8;
>> -		__u16 *u16;
>> -		__u32 *u32;
>> -	} databuf;
>>   	__u16 bmRType_bReq;
>>   	__u16 wValue;
>>   	__u16 wIndex;
>>   	__u16 wLength;
>> -
>> -	databuf.u32 = (__u32 *)datab;
>> +	ALLOC_ALIGN_BUFFER(__u8, databuf, 16, sizeof(u32));
> Ok, pardon my sloppiness in reviews ... but why not alloc this cache aligned?
> That way we'd also circumvent the issues with caches we'd eventually hit anyway.
>
> [...]
>
> The rest is OK.
>


Line 1464 has

     if (data != databuf)
         memcpy(data, databuf, len);

So as far as I can tell, databuf is always copied and there are no cache 
issues to circumvent.
But I have no complaint if you desire to cacheline aligned anyway...


Troy

[U-Boot] [PATCH 1/1] USB: Fix strict aliasing in ohci-hcd

[Marek Vasut]

Dear Troy Kisky,

> On 8/14/2012 10:33 AM, Marek Vasut wrote:
> > Dear Troy Kisky,
> > 
> >> commit 5f6aa03fda2a0a79940765865c1e4266be8a75f8
> >> 
> >>      USB: Fix complaints about strict aliasing in OHCI-HCD
> >> 
> >> tried to fix this, but gcc4.4 still complains. So, this
> >> patch basically reverts the above and does a simpler fix.
> >> 
> >> also, the above commit incorrectly changed
> >> 
> >> 	/* corresponds to data_buf[4-7] */
> >> 	datab [1] = 0;
> >> 
> >> to
> >> 
> >> 	/* corresponds to databuf.u8[4-7] */
> >> 	databuf.u8[1] = 0;
> >> 
> >> This patch also fixes that.
> >> 
> >> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> >> ---
> >> 
> >>   drivers/usb/host/ohci-hcd.c |   70
> >> 
> >> +++++++++++++++++++------------------------ 1 file changed, 31
> >> insertions(+), 39 deletions(-)
> >> 
> >> diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
> >> index d24f2f1..9f47351 100644
> >> --- a/drivers/usb/host/ohci-hcd.c
> >> +++ b/drivers/usb/host/ohci-hcd.c
> >> @@ -1261,19 +1261,11 @@ static int ohci_submit_rh_msg(struct usb_device
> >> *dev, unsigned long pipe, int leni = transfer_len;
> >> 
> >>   	int len = 0;
> >>   	int stat = 0;
> >> 
> >> -	__u32 datab[4];
> >> -	union {
> >> -		void *ptr;
> >> -		__u8 *u8;
> >> -		__u16 *u16;
> >> -		__u32 *u32;
> >> -	} databuf;
> >> 
> >>   	__u16 bmRType_bReq;
> >>   	__u16 wValue;
> >>   	__u16 wIndex;
> >>   	__u16 wLength;
> >> 
> >> -
> >> -	databuf.u32 = (__u32 *)datab;
> >> +	ALLOC_ALIGN_BUFFER(__u8, databuf, 16, sizeof(u32));
> > 
> > Ok, pardon my sloppiness in reviews ... but why not alloc this cache
> > aligned? That way we'd also circumvent the issues with caches we'd
> > eventually hit anyway.
> > 
> > [...]
> > 
> > The rest is OK.
> 
> Line 1464 has
> 
>      if (data != databuf)
>          memcpy(data, databuf, len);
> 
> So as far as I can tell, databuf is always copied and there are no cache
> issues to circumvent.
> But I have no complaint if you desire to cacheline aligned anyway...

Oh ok ... this is worse than I thought ... ok, I'll apply as-is

> Troy

Best regards,
Marek Vasut