[PATCH] Fix SPARC64 unaligned access in zd_mac_rx

[PATCH] Fix SPARC64 unaligned access in zd_mac_rx

[Patrick Simmons]

I'm running zd1211rw on SPARC64 and was getting a lot of "unaligned 
access" messages in dmesg.  I tracked the problem down to this line, and 
changing the cast to a memcpy fixes the issue.

Please CC me with any comments as I am not subscribed to the list.

Signed-off-by: Patrick Simmons <linuxrocks123@netscape.net>

--- a/drivers/net/wireless/zd1211rw/zd_mac.c    2009-07-30 
23:23:50.000000000 -0600
+++ b/drivers/net/wireless/zd1211rw/zd_mac.c    2009-07-30 
23:58:19.000000000 -0600
@@ -694,7 +694,7 @@
                        && !mac->pass_ctrl)
                return 0;
 
-       fc = *(__le16 *)buffer;
+       memcpy(&fc,buffer,sizeof(__le16));
        need_padding = ieee80211_is_data_qos(fc) ^ ieee80211_has_a4(fc);
 
        skb = dev_alloc_skb(length + (need_padding ? 2 : 0));

Re: [PATCH] Fix SPARC64 unaligned access in zd_mac_rx

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 469 bytes --]

On Fri, 2009-07-31 at 01:59 -0600, Patrick Simmons wrote:
> I'm running zd1211rw on SPARC64 and was getting a lot of "unaligned 
> access" messages in dmesg.  I tracked the problem down to this line, and 
> changing the cast to a memcpy fixes the issue.
>  
> -       fc = *(__le16 *)buffer;
> +       memcpy(&fc,buffer,sizeof(__le16));
>         need_padding = ieee80211_is_data_qos(fc) ^ ieee80211_has_a4(fc);

Please use get_unaligned instead.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] Fix SPARC64 unaligned access in zd_mac_rx

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Fri, 31 Jul 2009 10:39:43 +0200

> On Fri, 2009-07-31 at 01:59 -0600, Patrick Simmons wrote:
>> I'm running zd1211rw on SPARC64 and was getting a lot of "unaligned 
>> access" messages in dmesg.  I tracked the problem down to this line, and 
>> changing the cast to a memcpy fixes the issue.
>>  
>> -       fc = *(__le16 *)buffer;
>> +       memcpy(&fc,buffer,sizeof(__le16));
>>         need_padding = ieee80211_is_data_qos(fc) ^ ieee80211_has_a4(fc);
> 
> Please use get_unaligned instead.

And, more specifically, one of the endian get_unaligned variants.

Re: [PATCH] Fix SPARC64 unaligned access in zd_mac_rx

[Michael Buesch]

On Saturday 01 August 2009 07:23:50 David Miller wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Fri, 31 Jul 2009 10:39:43 +0200
> 
> > On Fri, 2009-07-31 at 01:59 -0600, Patrick Simmons wrote:
> >> I'm running zd1211rw on SPARC64 and was getting a lot of "unaligned 
> >> access" messages in dmesg.  I tracked the problem down to this line, and 
> >> changing the cast to a memcpy fixes the issue.
> >>  
> >> -       fc = *(__le16 *)buffer;
> >> +       memcpy(&fc,buffer,sizeof(__le16));
> >>         need_padding = ieee80211_is_data_qos(fc) ^ ieee80211_has_a4(fc);
> > 
> > Please use get_unaligned instead.
> 
> And, more specifically, one of the endian get_unaligned variants.

No. fc is supposed to be LE.

-- 
Greetings, Michael.

Re: [PATCH] Fix SPARC64 unaligned access in zd_mac_rx

[David Miller]

From: Michael Buesch <mb@bu3sch.de>
Date: Sat, 1 Aug 2009 12:40:16 +0200

> On Saturday 01 August 2009 07:23:50 David Miller wrote:
>> From: Johannes Berg <johannes@sipsolutions.net>
>> Date: Fri, 31 Jul 2009 10:39:43 +0200
>> 
>> > On Fri, 2009-07-31 at 01:59 -0600, Patrick Simmons wrote:
>> >> I'm running zd1211rw on SPARC64 and was getting a lot of "unaligned 
>> >> access" messages in dmesg.  I tracked the problem down to this line, and 
>> >> changing the cast to a memcpy fixes the issue.
>> >>  
>> >> -       fc = *(__le16 *)buffer;
>> >> +       memcpy(&fc,buffer,sizeof(__le16));
>> >>         need_padding = ieee80211_is_data_qos(fc) ^ ieee80211_has_a4(fc);
>> > 
>> > Please use get_unaligned instead.
>> 
>> And, more specifically, one of the endian get_unaligned variants.
> 
> No. fc is supposed to be LE.

Ok, I see.

Re: [PATCH] Fix SPARC64 unaligned access in zd_mac_rx

[Patrick Simmons]

David Miller wrote:
> From: Michael Buesch <mb@bu3sch.de>
> Date: Sat, 1 Aug 2009 12:40:16 +0200
> 
>> On Saturday 01 August 2009 07:23:50 David Miller wrote:
>>> From: Johannes Berg <johannes@sipsolutions.net>
>>> Date: Fri, 31 Jul 2009 10:39:43 +0200
>>>
>>>> On Fri, 2009-07-31 at 01:59 -0600, Patrick Simmons wrote:
>>>>> I'm running zd1211rw on SPARC64 and was getting a lot of "unaligned 
>>>>> access" messages in dmesg.  I tracked the problem down to this line, and 
>>>>> changing the cast to a memcpy fixes the issue.
>>>>>  
>>>>> -       fc = *(__le16 *)buffer;
>>>>> +       memcpy(&fc,buffer,sizeof(__le16));
>>>>>         need_padding = ieee80211_is_data_qos(fc) ^ ieee80211_has_a4(fc);
>>>> Please use get_unaligned instead.
>>> And, more specifically, one of the endian get_unaligned variants.
>> No. fc is supposed to be LE.
> 
> Ok, I see.

Revised patch below.

Signed-off-by: Patrick Simmons <linuxrocks123@netscape.net>

--- a/drivers/net/wireless/zd1211rw/zd_mac.c
+++ b/drivers/net/wireless/zd1211rw/zd_mac.c
@@ -694,7 +694,7 @@
                         && !mac->pass_ctrl)
                 return 0;

-       fc = *(__le16 *)buffer;
+       fc = get_unaligned(buffer);
         need_padding = ieee80211_is_data_qos(fc) ^ ieee80211_has_a4(fc);

         skb = dev_alloc_skb(length + (need_padding ? 2 : 0));


-- 
If I'm not here, I've gone out to find myself.  If I get back before I 
return, please keep me here.

Re: [PATCH] Fix SPARC64 unaligned access in zd_mac_rx

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 573 bytes --]

On Sat, 2009-08-01 at 19:41 -0600, Patrick Simmons wrote:

> --- a/drivers/net/wireless/zd1211rw/zd_mac.c
> +++ b/drivers/net/wireless/zd1211rw/zd_mac.c
> @@ -694,7 +694,7 @@
>                          && !mac->pass_ctrl)
>                  return 0;
> 
> -       fc = *(__le16 *)buffer;
> +       fc = get_unaligned(buffer);

Now the code is completely incorrect -- you're now loading the lower 16
bits of the 'buffer' _pointer_ into the variable as an unaligned load.

It really needs to be

fc = get_unaligned((__le16 *)buffer);

I think.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] Fix SPARC64 unaligned access in zd_mac_rx

[Patrick Simmons]

Johannes Berg wrote:
> Now the code is completely incorrect -- you're now loading the lower 16
> bits of the 'buffer' _pointer_ into the variable as an unaligned load.
>
> It really needs to be
>
> fc = get_unaligned((__le16 *)buffer);
>
> I think.
>
> johannes
>   

It does look wrong, now that I look at it further, but I'm pretty sure 
that what was happening was that only the lower 8 bits of the 
dereferenced value were being accessed.  What's odd is that it works on 
my machine, though.  Whatever.  Your version is clearer, in any case.

What follows is hopefully the last version of this patch.

Signed-off-by: Patrick Simmons <linuxrocks123@netscape.net>

--- a/drivers/net/wireless/zd1211rw/zd_mac.c
+++ b/drivers/net/wireless/zd1211rw/zd_mac.c
@@ -694,7 +694,7 @@
                        && !mac->pass_ctrl)
                return 0;
 
-       fc = *(__le16 *)buffer;
+       fc = get_unaligned((__le16*)buffer);
        need_padding = ieee80211_is_data_qos(fc) ^ ieee80211_has_a4(fc);
 
        skb = dev_alloc_skb(length + (need_padding ? 2 : 0));

--Patrick

Re: [PATCH] Fix SPARC64 unaligned access in zd_mac_rx

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

On Sun, 2009-08-02 at 02:24 -0600, Patrick Simmons wrote:

> It does look wrong, now that I look at it further, but I'm pretty sure 
> that what was happening was that only the lower 8 bits of the 
> dereferenced value were being accessed. 

Yes, you're right, because buffer is a "u8 *", I initially thought that
it didn't take a pointer, but that wouldn't work, of course.

>  What's odd is that it works on 
> my machine, though.  Whatever.  Your version is clearer, in any case.
> 
> What follows is hopefully the last version of this patch.

Looks good, but I think you should resubmit with
 * a changed subject (e.g. "zd1211rw: fix unaligned access in zd_mac_rx")
 * a standalone patch description so John doesn't have to pick it out of
   your first patch etc.


Thanks!
johannes

> Signed-off-by: Patrick Simmons <linuxrocks123@netscape.net>
> 
> --- a/drivers/net/wireless/zd1211rw/zd_mac.c
> +++ b/drivers/net/wireless/zd1211rw/zd_mac.c
> @@ -694,7 +694,7 @@
>                         && !mac->pass_ctrl)
>                 return 0;
>  
> -       fc = *(__le16 *)buffer;
> +       fc = get_unaligned((__le16*)buffer);
>         need_padding = ieee80211_is_data_qos(fc) ^ ieee80211_has_a4(fc);
>  
>         skb = dev_alloc_skb(length + (need_padding ? 2 : 0));
> 
> --Patrick
> 
> 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] Fix SPARC64 unaligned access in zd_mac_rx

[Patrick Simmons]

Johannes Berg wrote:
> On Sun, 2009-08-02 at 02:24 -0600, Patrick Simmons wrote:
>
>   
>> It does look wrong, now that I look at it further, but I'm pretty sure 
>> that what was happening was that only the lower 8 bits of the 
>> dereferenced value were being accessed. 
>>     
>
> Yes, you're right, because buffer is a "u8 *", I initially thought that
> it didn't take a pointer, but that wouldn't work, of course.
>
>   
>>  What's odd is that it works on 
>> my machine, though.  Whatever.  Your version is clearer, in any case.
>>
>> What follows is hopefully the last version of this patch.
>>     
>
> Looks good, but I think you should resubmit with
>  * a changed subject (e.g. "zd1211rw: fix unaligned access in zd_mac_rx")
>  * a standalone patch description so John doesn't have to pick it out of
>    your first patch etc.
>
>
> Thanks!
> johannes
>
>   
Sure thing; here it comes.  Thanks for the review.

--Patrick

[PATCH] zd1211rw: fix unaligned access in zd_mac_rx

[Patrick Simmons]

Fix an unaligned memory access in the zd_mac_rx function of zd1211rw 
that causes problems on SPARC64.

Signed-off-by: Patrick Simmons <linuxrocks123@netscape.net>

--- a/drivers/net/wireless/zd1211rw/zd_mac.c
+++ b/drivers/net/wireless/zd1211rw/zd_mac.c
@@ -694,7 +694,7 @@
                        && !mac->pass_ctrl)
                return 0;

-       fc = *(__le16 *)buffer;
+       fc = get_unaligned((__le16*)buffer);
        need_padding = ieee80211_is_data_qos(fc) ^ ieee80211_has_a4(fc);

        skb = dev_alloc_skb(length + (need_padding ? 2 : 0));