Fix SPARC64 unaligned access in zd_mac_rx

drivers/net/wireless/zd1211rw/zd_mac.c 2009-07-30 23:58:19.000000000 -0600 line 698

Quoted 7 lines

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.

drivers/net/wireless/zd1211rw/zd_mac.c 2009-07-30 23:58:19.000000000 -0600 renamed from drivers/net/wireless/zd1211rw/zd_mac.c 2009-07-30 23:23:50.000000000 -0600 +1 -1

-       fc = *(__le16 *)buffer;
+       memcpy(&fc,buffer,sizeof(__le16));
        need_padding = ieee80211_is_data_qos(fc) ^ ieee80211_has_a4(fc);

Please use get_unaligned instead.

From: Johannes Berg <[email protected]>
Date: Fri, 31 Jul 2009 10:39:43 +0200

Quoted 5 lines · Patrick Simmons

Quoted 4 lines

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.

Quoted 5 lines

drivers/net/wireless/zd1211rw/zd_mac.c 2009-07-30 23:58:19.000000000 -0600 line 698

Quoted 3 lines

drivers/net/wireless/zd1211rw/zd_mac.c 2009-07-30 23:58:19.000000000 -0600 renamed from drivers/net/wireless/zd1211rw/zd_mac.c 2009-07-30 23:23:50.000000000 -0600 +1 -1

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

Quoted 12 lines

From: Johannes Berg <[email protected]>
Date: Fri, 31 Jul 2009 10:39:43 +0200

Quoted 9 lines · Patrick Simmons

Quoted 7 lines

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.

drivers/net/wireless/zd1211rw/zd_mac.c 2009-07-30 23:58:19.000000000 -0600 renamed from drivers/net/wireless/zd1211rw/zd_mac.c 2009-07-30 23:23:50.000000000 -0600 +1 -1

-       fc = *(__le16 *)buffer;
+       memcpy(&fc,buffer,sizeof(__le16));
        need_padding = ieee80211_is_data_qos(fc) ^ ieee80211_has_a4(fc);

Quoted 3 lines

Quoted 1 line

Please use get_unaligned instead.

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

No. fc is supposed to be LE.

From: Michael Buesch <[email protected]>
Date: Sat, 1 Aug 2009 12:40:16 +0200

Quoted 7 lines · On Saturday 01 August 2009 07:23:50 David Miller wrote:

Quoted 6 lines

From: Johannes Berg <[email protected]>
Date: Fri, 31 Jul 2009 10:39:43 +0200

Quoted 3 lines · Patrick Simmons

Quoted 2 lines

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.

Quoted 6 lines

Quoted 5 lines

Quoted 4 lines

Review

Quoted 3 lines

(quoted diff) +1 -1

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

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 <[email protected]>

drivers/net/wireless/zd1211rw/zd_mac.c +1 -2

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

Quoted 2 lines · Patrick Simmons

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.

Quoted 5 lines

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