RE: [PATCH] USB/ISP1760: Fix for unaligned exceptions

[Hennerich, Michael]

Sebastian,

>The link [1] you sent me reports an unaligned access which occurred
here.
>So I thing the access to *src should be either a get_unaligned helper
or a
>byte read loop like I did it in the read path.

It's not just that single spot.
I've seen unaligned pointers with count > 3 coming from various drivers.

Here just two examples:

1) The generic Bluetooth USB driver: CONFIG_BT_HCIUSB  
Bluez-utils: hcitool scan:

priv_write_copy: src = 00efaa09, dst = 203c1200, len = 13

Full trace attached.

2) RTL8150 based USB Ethernet adapter: CONFIG_USB_RTL8150
dhcpcd:

priv_read_copy: src = 00ea4812, dst = 203d8000, len = 64

This trace was taken with the unaligned workaround for lengths < 4.

I wonder if it's only us (NOMMU) seeing these odd aligned buffers?

-Michael



>-----Original Message-----
>From: Sebastian Andrzej Siewior [mailto:bigeasy@xxxxxxxxxxxxx]
>Sent: Tuesday, November 18, 2008 11:52 AM
>To: Bryan Wu
>Cc: linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Michael
>Hennerich
>Subject: Re: [PATCH] USB/ISP1760: Fix for unaligned exceptions
>
>Bryan Wu wrote:
>> From: Michael Hennerich <michael.hennerich@xxxxxxxxxx>
>> Signed-off-by: Michael Hennerich <michael.hennerich@xxxxxxxxxx>
>> Signed-off-by: Bryan Wu <cooloney@xxxxxxxxxx>
>> ---
>>  drivers/usb/host/isp1760-hcd.c |   67
++++++++++++++++++++++++++++------
>-----
>>  1 files changed, 48 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/usb/host/isp1760-hcd.c
b/drivers/usb/host/isp1760-
>hcd.c
>> index 8017f1c..00bece2 100644
>> --- a/drivers/usb/host/isp1760-hcd.c
>> +++ b/drivers/usb/host/isp1760-hcd.c
>> @@ -136,12 +136,21 @@ static void priv_read_copy(struct isp1760_hcd
>*priv, u32 *src,
>>  		return;
>>  	}
>>
>> -	while (len >= 4) {
>> -		*src = __raw_readl(dst);
>> -		len -= 4;
>> -		src++;
>> -		dst++;
>> -	}
>> +	if (unlikely((u32)src & 0x3)) {
>> +		while (len >= 4) {
>> +			put_unaligned(__raw_readl(dst), src);
>> +			len -= 4;
>> +			src++;
>> +			dst++;
>> +		}
>> +	} else {
>> +		while (len >= 4) {
>> +			*src = __raw_readl(dst);
>> +			len -= 4;
>> +			src++;
>> +			dst++;
>> +		}
>> +	}
>>
>>  	if (!len)
>>  		return;
>> @@ -159,25 +168,45 @@ static void priv_read_copy(struct isp1760_hcd
>*priv, u32 *src,
>>  		len--;
>>  		buff8++;
>>  	}
>> +
>>  }
>>
>>  static void priv_write_copy(const struct isp1760_hcd *priv, const
u32
>*src,
>>  		__u32 __iomem *dst, u32 len)
>>  {
>> -	while (len >= 4) {
>> -		__raw_writel(*src, dst);
>> -		len -= 4;
>> -		src++;
>> -		dst++;
>> -	}
>>
>> -	if (!len)
>> -		return;
>> -	/* in case we have 3, 2 or 1 by left. The buffer is allocated
and the
>> -	 * extra bytes should not be read by the HW
>> -	 */
>> -
>> -	__raw_writel(*src, dst);
>The link [1] you sent me reports an unaligned access which occurred
here.
>So I thing the access to *src should be either a get_unaligned helper
or a
>byte read loop like I did it in the read path.
>The r8a66597 is doing the same thing (as you suggest) for one SH
machine.
>However, I'm not convinced to fix it that way: The buffer should be
>properly aligned by the driver. Unless there is HW which requires
>unaligned data, I would prefer just to fix this unaligned access.
>According to the thread in [1] a similar patch fixed it for the user
until
>he dropped into another bug.
>
>> +	if (unlikely((u32)src & 0x3)) {
>> +		while (len >= 4) {
>> +			__raw_writel(get_unaligned(src), dst);
>> +			len -= 4;
>> +			src++;
>> +			dst++;
>> +		}
>> +
>> +		if (!len)
>> +			return;
>> +		/* in case we have 3, 2 or 1 by left. The buffer is
allocated
>and the
>> +		 * extra bytes should not be read by the HW
>> +		 */
>> +
>> +		__raw_writel(get_unaligned(src), dst);
>> +
>> +	} else{
>> +		while (len >= 4) {
>> +			__raw_writel(*src, dst);
>> +			len -= 4;
>> +			src++;
>> +			dst++;
>> +		}
>> +
>> +		if (!len)
>> +			return;
>> +		/* in case we have 3, 2 or 1 by left. The buffer is
allocated
>and the
>> +		 * extra bytes should not be read by the HW
>> +		 */
>> +
>> +		__raw_writel(*src, dst);
>> +	}
>>  }
>>
>>  /* memory management of the 60kb on the chip from 0x1000 to 0xffff
*/
>
>[1]
>http://blackfin.uclinux.org/gf/project/uclinux-
>dist/forum/?action=ForumBrowse&_forum_action=MessageReply&message_id=64
889
>
>Sebastian

Attachment: dumps.tar.bz2
Description: dumps.tar.bz2