zd1211rw (2.6.26 sparc64): unaligned access (zd_mac_rx)

Quoted 7 lines · * Shaddy Baddah | 2008-10-25 19:54:19 [+1100]:

[  282.300681] Kernel unaligned access at TPC[10129b68] 
zd_mac_rx+0x144/0x32c [zd1211rw]
periodically is reported even after the ifconfig command completes (seemingly successfully).
Thanks in advance for your assistance,

Could you try this patch:

Quoted 1 line

From a46436536ab13926ff3f592cfa3c333e06a99a0a Mon Sep 17 00:00:00 2001

From: Sebastian Andrzej Siewior <[email protected]>
Date: Sat, 25 Oct 2008 13:15:14 +0200
Subject: [PATCH] wireless/zd1211rw: use get_unaligned_le16 helper

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

---
 drivers/net/wireless/zd1211rw/zd_mac.c |    5 +++--
 drivers/net/wireless/zd1211rw/zd_usb.c |    2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

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

index 4d7b98b..bf4b4a4 100644
@@ -25,6 +25,7 @@
 #include <linux/usb.h>
 #include <linux/jiffies.h>
 #include <net/ieee80211_radiotap.h>
+#include <asm/unaligned.h>
 
 #include "zd_def.h"
 #include "zd_chip.h"

@@ -662,7 +663,7 @@ int zd_mac_rx(struct ieee80211_hw *hw, const u8 *buffer, unsigned int length)
 			&& !mac->pass_ctrl)
 		return 0;
 
-	fc = *(__le16 *)buffer;
+	fc = get_unaligned_le32(buffer);
 	need_padding = ieee80211_is_data_qos(fc) ^ ieee80211_has_a4(fc);
 
 	skb = dev_alloc_skb(length + (need_padding ? 2 : 0));

@@ -758,7 +759,7 @@ void zd_process_intr(struct work_struct *work)
 	u16 int_status;
 	struct zd_mac *mac = container_of(work, struct zd_mac, process_intr);
 
-	int_status = le16_to_cpu(*(__le16 *)(mac->intr_buffer+4));
+	int_status = get_unaligned_le16(mac->intr_buffer+4);
 	if (int_status & INT_CFG_NEXT_BCN) {
 		if (net_ratelimit())
 			dev_dbg_f(zd_mac_dev(mac), "INT_CFG_NEXT_BCN\n");

drivers/net/wireless/zd1211rw/zd_usb.c +1 -1

index a60ae86..0b27778 100644
@@ -355,7 +355,7 @@ static inline void handle_regs_int(struct urb *urb)
 	ZD_ASSERT(in_interrupt());
 	spin_lock(&intr->lock);
 
-	int_num = le16_to_cpu(*(__le16 *)(urb->transfer_buffer+2));
+	int_num = get_unaligned_le16(urb->transfer_buffer+2);
 	if (int_num == CR_INTERRUPT) {
 		struct zd_mac *mac = zd_hw_mac(zd_usb_to_hw(urb->context));
 		memcpy(&mac->intr_buffer, urb->transfer_buffer,

Signature · 1 line

1.6.0.2

Sebastian Andrzej Siewior

2008-10-25 11:25 UTC #2 lore raw

Quoted 8 lines · * Johannes Berg | 2008-10-25 11:17:21 [+0200]:

Quoted 1 line
[  277.190494] Kernel unaligned access at TPC[10129b68] zd_mac_rx+0x144/0x32c [zd1211rw]
my best bet is this:
       fc = *(__le16 *)buffer;
could you try something with get_unaligned? Though if this is it, then ... We really need disassembly of the exact spot.

A few lines before that:
| buffer += ZD_PLCP_HEADER_SIZE;

and ZD_PLCP_HEADER_SIZE is 5 so we end up unaligned, don't we?

Quoted 2 lines

johannes

Sebastian

Michael Buesch

2008-10-25 11:25 UTC #7 lore raw

drivers/net/wireless/zd1211rw/zd_mac.c int zd_mac_rx(struct ieee80211_hw *hw, const u8 *buffer, unsigned int length) · line 666

Quoted 2 lines · On Saturday 25 October 2008 13:21:47 Sebastian Andrzej Siewior wrote:

drivers/net/wireless/zd1211rw/zd_mac.c +1 -1
-	fc = *(__le16 *)buffer;
+	fc = get_unaligned_le32(buffer);

I'd say this semantically changes the code.

Quoted 3 lines

drivers/net/wireless/zd1211rw/zd_mac.c +0 -0

 	need_padding = ieee80211_is_data_qos(fc) ^ ieee80211_has_a4(fc);
 
 	skb = dev_alloc_skb(length + (need_padding ? 2 : 0));

Signature · 1 line

Greetings Michael.

Sebastian Andrzej Siewior

2008-10-25 11:28 UTC #8 lore raw

Quoted 5 lines · * Michael Buesch | 2008-10-25 13:25:07 [+0200]:

Review
Quoted 2 lines · On Saturday 25 October 2008 13:21:47 Sebastian Andrzej Siewior wrote:
(quoted diff) +1 -1
-	fc = *(__le16 *)buffer;
+	fc = get_unaligned_le32(buffer);
I'd say this semantically changes the code.

argh,
sorry,

Subject: [PATCH] wireless/zd1211rw: use get_unaligned_le16 helper (v2)

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

---
 drivers/net/wireless/zd1211rw/zd_mac.c |    5 +++--
 drivers/net/wireless/zd1211rw/zd_usb.c |    2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

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

index 4d7b98b..bf4b4a4 100644
@@ -25,6 +25,7 @@
 #include <linux/usb.h>
 #include <linux/jiffies.h>
 #include <net/ieee80211_radiotap.h>
+#include <asm/unaligned.h>
 
 #include "zd_def.h"
 #include "zd_chip.h"

@@ -662,7 +663,7 @@ int zd_mac_rx(struct ieee80211_hw *hw, const u8 *buffer, unsigned int length)
 			&& !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));

@@ -758,7 +759,7 @@ void zd_process_intr(struct work_struct *work)
 	u16 int_status;
 	struct zd_mac *mac = container_of(work, struct zd_mac, process_intr);
 
-	int_status = le16_to_cpu(*(__le16 *)(mac->intr_buffer+4));
+	int_status = get_unaligned_le16(mac->intr_buffer+4);
 	if (int_status & INT_CFG_NEXT_BCN) {
 		if (net_ratelimit())
 			dev_dbg_f(zd_mac_dev(mac), "INT_CFG_NEXT_BCN\n");

drivers/net/wireless/zd1211rw/zd_usb.c +1 -1

index a60ae86..0b27778 100644
@@ -355,7 +355,7 @@ static inline void handle_regs_int(struct urb *urb)
 	ZD_ASSERT(in_interrupt());
 	spin_lock(&intr->lock);
 
-	int_num = le16_to_cpu(*(__le16 *)(urb->transfer_buffer+2));
+	int_num = get_unaligned_le16(urb->transfer_buffer+2);
 	if (int_num == CR_INTERRUPT) {
 		struct zd_mac *mac = zd_hw_mac(zd_usb_to_hw(urb->context));
 		memcpy(&mac->intr_buffer, urb->transfer_buffer,

Signature · 1 line

1.6.0.2

Michael Buesch

2008-10-25 11:31 UTC #9 lore raw

drivers/net/wireless/zd1211rw/zd_mac.c int zd_mac_rx(struct ieee80211_hw *hw, const u8 *buffer, unsigned int length) · line 666

Quoted 6 lines · On Saturday 25 October 2008 13:28:13 Sebastian Andrzej Siewior wrote:

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

@@ -662,7 +663,7 @@ int zd_mac_rx(struct ieee80211_hw *hw, const u8 *buffer, unsigned int length)
 			&& !mac->pass_ctrl)
 		return 0;
 
-	fc = *(__le16 *)buffer;
+	fc = get_unaligned_le16(buffer);

This still does semantically change the code.
fc is supposed to be LE and is used as LE in the following code.

Quoted 3 lines

drivers/net/wireless/zd1211rw/zd_mac.c +0 -0

 	need_padding = ieee80211_is_data_qos(fc) ^ ieee80211_has_a4(fc);
 
 	skb = dev_alloc_skb(length + (need_padding ? 2 : 0));

Signature · 1 line

Greetings Michael.

Sebastian Andrzej Siewior

2008-10-25 11:38 UTC #10 lore raw

Quoted 10 lines · * Michael Buesch | 2008-10-25 13:31:24 [+0200]:

drivers/net/wireless/zd1211rw/zd_mac.c int zd_mac_rx(struct ieee80211_hw *hw, const u8 *buffer, unsigned int length) · line 666
Quoted 6 lines · On Saturday 25 October 2008 13:28:13 Sebastian Andrzej Siewior wrote:
drivers/net/wireless/zd1211rw/zd_mac.c +1 -1
@@ -662,7 +663,7 @@ int zd_mac_rx(struct ieee80211_hw *hw, const u8 *buffer, unsigned int length)
 			&& !mac->pass_ctrl)
 		return 0;
 
-	fc = *(__le16 *)buffer;
+	fc = get_unaligned_le16(buffer);
This still does semantically change the code.
fc is supposed to be LE and is used as LE in the following code.

So you say we need a get_unaligned() here. Okay.
A few lines before that, in filter_ack() we dereferance the same position. filter_ack() is used once and probably inlined so I guess that's the address where the unaligned access occurs.

Quoted 8 lines

Quoted 3 lines

drivers/net/wireless/zd1211rw/zd_mac.c +0 -0

 	need_padding = ieee80211_is_data_qos(fc) ^ ieee80211_has_a4(fc);
 
 	skb = dev_alloc_skb(length + (need_padding ? 2 : 0));

Signature · 1 line

Greetings Michael.

Sebastian

Johannes Berg

maintainer

2008-10-25 12:05 UTC #3 lore raw

Quoted 6 lines · Sebastian Andrzej Siewior

Quoted 4 lines · * Johannes Berg | 2008-10-25 11:17:21 [+0200]:
Quoted 1 line
[  277.190494] Kernel unaligned access at TPC[10129b68] zd_mac_rx+0x144/0x32c [zd1211rw]
my best bet is this:

Quoted 8 lines

Quoted 4 lines
       fc = *(__le16 *)buffer;
could you try something with get_unaligned? Though if this is it, then ... We really need disassembly of the exact spot.
A few lines before that:
| buffer += ZD_PLCP_HEADER_SIZE;
and ZD_PLCP_HEADER_SIZE is 5 so we end up unaligned, don't we?

Depends where the buffer was before, and thought has gone into this alignment stuff here so it shouldn't. Might well be a new bug though.

johannes

Shaddy Baddah

author

2008-10-25 15:13 UTC #11 lore raw

Hi Sebastian,

Quoted 6 lines · Sebastian Andrzej Siewior

Quoted 4 lines · * Michael Buesch | 2008-10-25 13:25:07 [+0200]:
Review
Quoted 2 lines · On Saturday 25 October 2008 13:21:47 Sebastian Andrzej Siewior wrote:
(quoted diff) +1 -1
-	fc = *(__le16 *)buffer;
+	fc = get_unaligned_le32(buffer);
I'd say this semantically changes the code.

Quoted 6 lines

argh,
sorry,
Subject: [PATCH] wireless/zd1211rw: use get_unaligned_le16 helper (v2)
Signed-off-by Sebastian Andrzej Siewior <[email protected]>

After applying this patch, things seem better. I can associate to an open access point, get an IP address and even ping.

However on association, I am still seeing:

[  179.249516] zd1211rw 4-3:1.0: zd_usb_rfwrite() value 0x01e6666 bits 24
[  179.254524] wlan0: Initial auth_alg=0
[  179.254549] wlan0: authenticate with AP XX:XX:XX:XX:XX:XX
[  179.256215] Kernel unaligned access at TPC[10129b94] zd_mac_rx+0x174/0x320 [zd1211rw]
[  179.349135] Kernel unaligned access at TPC[10129b9c] zd_mac_rx+0x17c/0x320 [zd1211rw]
[  179.442852] Kernel unaligned access at TPC[10129ba0] zd_mac_rx+0x180/0x320 [zd1211rw]
[  179.536579] Kernel unaligned access at TPC[10129ba4] zd_mac_rx+0x184/0x320 [zd1211rw]
[  179.630306] Kernel unaligned access at TPC[10129ba8] zd_mac_rx+0x188/0x320 [zd1211rw]
[  179.727277] wlan0: RX authentication from XX:XX:XX:XX:XX:XX (alg=0 transaction=2 status=0)
[  179.727302] wlan0: authenticated
[  179.727318] wlan0: associate with AP XX:XX:XX:XX:XX:XX
[  179.727440] wlan0: associate with AP XX:XX:XX:XX:XX:XX
[  179.736350] wlan0: RX AssocResp from XX:XX:XX:XX:XX:XX (capab=0x421 status=0 aid=12)
[  179.736380] wlan0: associated
[  179.736421] wlan0: CTS protection enabled (BSSID=XX:XX:XX:XX:XX:XX)
[  179.736440] wlan0: switched to short barker preamble (BSSID=XX:XX:XX:XX:XX:XX)
[  179.736521] zd1211rw 4-3:1.0: zd_op_bss_info_changed() changes: 7
[  179.736666] wlan0: association frame received from XX:XX:XX:XX:XX:XX, but not in associate state - ignored
[  179.737404] ADDRCONF(NETDEV_CHANGE): wlan0: link becomes ready
[  179.807280] zd1211rw 4-3:1.0: zd_chip_set_rts_cts_rate_locked() preamble=1

and when I start pinging the AP, this starts chiming in:

[  240.139093] Kernel unaligned access at TPC[100f7f44] sta_info_get+0x24/0x68 [mac80211]
[  240.233255] Kernel unaligned access at TPC[100f7f48] sta_info_get+0x28/0x68 [mac80211]
[  240.328015] Kernel unaligned access at TPC[100f7f50] sta_info_get+0x30/0x68 [mac80211]
[  240.422771] Kernel unaligned access at TPC[100f7f44] sta_info_get+0x24/0x68 [mac80211]
[  240.517554] Kernel unaligned access at TPC[100f7f48] sta_info_get+0x28/0x68 [mac80211]

I also cannot get a full scan with:

iwlist wlan0 scanning

I get about 4 APs (I know there's more) then a bus error. This may be related to a similar problem that I was assisted with in my first round of posts last year, with regards to the wireless-tools. Perhaps the patches still haven't made it downstream?

Thanking you for help,
Shaddy

Sebastian Andrzej Siewior

2008-10-26 22:00 UTC #4 lore raw

Quoted 11 lines · * Johannes Berg | 2008-10-25 14:05:11 [+0200]:

Quoted 10 lines · Sebastian Andrzej Siewior
Quoted 8 lines · * Johannes Berg | 2008-10-25 11:17:21 [+0200]:
Quoted 1 line
[  277.190494] Kernel unaligned access at TPC[10129b68] zd_mac_rx+0x144/0x32c [zd1211rw]
my best bet is this:
       fc = *(__le16 *)buffer;
could you try something with get_unaligned? Though if this is it, then ... We really need disassembly of the exact spot.

Quoted 7 lines

Quoted 4 lines
A few lines before that:
| buffer += ZD_PLCP_HEADER_SIZE;
and ZD_PLCP_HEADER_SIZE is 5 so we end up unaligned, don't we?
Depends where the buffer was before, and thought has gone into this alignment stuff here so it shouldn't. Might well be a new bug though.

On my x86_64 box the address of the buffer pointer right after the add of ZD_PLCP_HEADER_SIZE is:

[   28.462273] buffer: ffff88003e310005

so it looks to me like it was perfectly aligned before the add. Is it possible that it is not a new bug and we need a get_unaligend helper here?

Quoted 2 lines

johannes

Sebastian

Johannes Berg

maintainer

2008-10-27 07:00 UTC #5 lore raw

Quoted 7 lines · Sebastian Andrzej Siewior

On my x86_64 box the address of the buffer pointer right after the add of ZD_PLCP_HEADER_SIZE is:
[   28.462273] buffer: ffff88003e310005
so it looks to me like it was perfectly aligned before the add. Is it possible that it is not a new bug and we need a get_unaligend helper here?

Possible, yeah, maybe before we only saw problems with the IP alignment and somehow this shows up now. Not sure. Anyway, if that fixes it, obviously we want such a patch :)

johannes

Shaddy Baddah

author

2008-11-08 12:51 UTC #12 lore raw

Hi again,

Quoted 5 lines · Shaddy Baddah

After applying this patch, things seem better. I can associate to an open access point, get an IP address and even ping.
However on association, I am still seeing:

Quoted 8 lines

[  179.249516] zd1211rw 4-3:1.0: zd_usb_rfwrite() value 0x01e6666 bits 24
[  179.254524] wlan0: Initial auth_alg=0
[  179.254549] wlan0: authenticate with AP XX:XX:XX:XX:XX:XX
[  179.256215] Kernel unaligned access at TPC[10129b94] zd_mac_rx+0x174/0x320 [zd1211rw]
[  179.349135] Kernel unaligned access at TPC[10129b9c] zd_mac_rx+0x17c/0x320 [zd1211rw]
[  179.442852] Kernel unaligned access at TPC[10129ba0] zd_mac_rx+0x180/0x320 [zd1211rw]
[  179.536579] Kernel unaligned access at TPC[10129ba4] zd_mac_rx+0x184/0x320 [zd1211rw]
[  179.630306] Kernel unaligned access at TPC[10129ba8] zd_mac_rx+0x188/0x320 [zd1211rw]

I'm keen on troubleshooting this myself. Could you please tell me how I can get a disassembly intermixed with source. An objdump -S ./zd1211rw.o is not doing the job. I'd rather not re-compile the whole kernel if possible, because it takes long. Should I rm the file, do a build, take the command line and substitute -g for any -O option?

TIA,
Shaddy

Johannes Berg

maintainer

2008-11-08 13:11 UTC #14 lore raw

Quoted 14 lines · Shaddy Baddah

Quoted 12 lines

[  179.249516] zd1211rw 4-3:1.0: zd_usb_rfwrite() value 0x01e6666 bits 24
[  179.254524] wlan0: Initial auth_alg=0
[  179.254549] wlan0: authenticate with AP XX:XX:XX:XX:XX:XX
[  179.256215] Kernel unaligned access at TPC[10129b94] 
zd_mac_rx+0x174/0x320 [zd1211rw]
[  179.349135] Kernel unaligned access at TPC[10129b9c] 
zd_mac_rx+0x17c/0x320 [zd1211rw]
[  179.442852] Kernel unaligned access at TPC[10129ba0] 
zd_mac_rx+0x180/0x320 [zd1211rw]
[  179.536579] Kernel unaligned access at TPC[10129ba4] 
zd_mac_rx+0x184/0x320 [zd1211rw]
[  179.630306] Kernel unaligned access at TPC[10129ba8]

Quoted 1 line

zd_mac_rx+0x188/0x320 [zd1211rw]

Quoted 5 lines

I'm keen on troubleshooting this myself. Could you please tell me how I can get a disassembly intermixed with source. An objdump -S ./zd1211rw.o is not doing the job. I'd rather not re-compile the whole kernel if possible, because it takes long. Should I rm the file, do a build, take the command line and substitute -g for any -O option?

I usually just _add_ -g3 to the command line I get with make V=1 M=...

johannes

Shaddy Baddah

author

2008-11-09 03:06 UTC #15 lore raw

Hi,

Quoted 10 lines · Johannes Berg

Quoted 8 lines · Shaddy Baddah

Quoted 8 lines

[  179.249516] zd1211rw 4-3:1.0: zd_usb_rfwrite() value 0x01e6666 bits 24
[  179.254524] wlan0: Initial auth_alg=0
[  179.254549] wlan0: authenticate with AP XX:XX:XX:XX:XX:XX
[  179.256215] Kernel unaligned access at TPC[10129b94] zd_mac_rx+0x174/0x320 [zd1211rw]
[  179.349135] Kernel unaligned access at TPC[10129b9c] zd_mac_rx+0x17c/0x320 [zd1211rw]
[  179.442852] Kernel unaligned access at TPC[10129ba0] zd_mac_rx+0x180/0x320 [zd1211rw]
[  179.536579] Kernel unaligned access at TPC[10129ba4] zd_mac_rx+0x184/0x320 [zd1211rw]
[  179.630306] Kernel unaligned access at TPC[10129ba8] zd_mac_rx+0x188/0x320 [zd1211rw]

Quoted 3 lines

Quoted 1 line
I'm keen on troubleshooting this myself. Could you please tell me how I can get a disassembly intermixed with source. An objdump -S ./zd1211rw.o is not doing the job. I'd rather not re-compile the whole kernel if possible, because it takes long. Should I rm the file, do a build, take the command line and substitute -g for any -O option?
I usually just _add_ -g3 to the command line I get with make V=1 M=...

Johannes, thank you for the tip. It's helped me no end.

So, looking into this problem, I was able to come up with a patch (attached. Note, it also includes remnants of the patch recommended by Sebastian Andrzej Siewior in http://marc.info/?l=linux-wireless&m=122493409906326&w=2). The problem occurs within compare_ether_addr() and this immediately reminded me that the patches recommended to me way back last year (http://marc.info/?l=linux-wireless&m=119543627712471&w=2) also dealt with this problem.

Looking into how it was solved then, all the compare_ether_addr() calls were replaced with memcmp() calls. This is what I have done with my patch as well... but this approach did not fill me with confidence. It seemed to me like a quick fix that didn't directly address the alignment problem. And IMO my subsequent findings confirm this.

I now hit the following kernel errors:

Quoted 10 lines

[  240.139093] Kernel unaligned access at TPC[100f7f44]
sta_info_get+0x24/0x68 [mac80211]
[  240.233255] Kernel unaligned access at TPC[100f7f48]
sta_info_get+0x28/0x68 [mac80211]
[  240.328015] Kernel unaligned access at TPC[100f7f50]
sta_info_get+0x30/0x68 [mac80211]
[  240.422771] Kernel unaligned access at TPC[100f7f44]
sta_info_get+0x24/0x68 [mac80211]
[  240.517554] Kernel unaligned access at TPC[100f7f48]

sta_info_get+0x28/0x68 [mac80211]

Looking into this, and the underlying problem is again compare_ether_addr(). Now, I'm sure that replacing this with a memcmp() is now treading on more toes than just zd1211rw users. And for this reason, I think a better solution is going to be required.

Does that sound right? I will persist in trying to understand the code to try and come up with a fix. But obviously, I hope that I can get help with this to make the former an education exercise, not a kernel maintenance exercise.

TIA,
Shaddy

Shaddy Baddah

author

2008-11-09 03:09 UTC #16 lore raw

Quoted 1 line · Shaddy Baddah

(attached. Note, it also includes remnants of the patch recommended by Sebastian Andrzej Siewior in http://marc.info/?l=linux-wireless&m=122493409906326&w=2). The problem occurs within compare_ether_addr() and this immediately reminded me that

This time, really attached. Sorry about that.

Regards,
Shaddy

linux-source-2.6.26/drivers/net/wireless/zd1211rw/zd_mac.c renamed from linux-source-2.6.26.orig/drivers/net/wireless/zd1211rw/zd_mac.c +4 -3

diff -urN -x '*.o' -x '*.ko' ./linux-source-2.6.26.orig/drivers/net/wireless/zd1211rw/zd_mac.c ./linux-source-2.6.26/drivers/net/wireless/zd1211rw/zd_mac.c
@@ -25,6 +25,7 @@
 #include <linux/usb.h>
 #include <linux/jiffies.h>
 #include <net/ieee80211_radiotap.h>
+#include <asm/unaligned.h>
 
 #include "zd_def.h"
 #include "zd_chip.h"

@@ -632,7 +633,7 @@
 		struct ieee80211_hdr *tx_hdr;
 
 		tx_hdr = (struct ieee80211_hdr *)skb->data;
-		if (likely(!compare_ether_addr(tx_hdr->addr2, rx_hdr->addr1)))
+		if (likely(!memcmp(tx_hdr->addr2, rx_hdr->addr1, ETH_ALEN)))
 		{
 			struct ieee80211_tx_status status;

@@ -716,7 +717,7 @@
 			&& !mac->pass_ctrl)
 		return 0;
 
-	fc = le16_to_cpu(*((__le16 *) buffer));
+	fc = get_unaligned_le16(buffer);
 
 	is_qos = ((fc & IEEE80211_FCTL_FTYPE) == IEEE80211_FTYPE_DATA) &&
 		 (fc & IEEE80211_STYPE_QOS_DATA);

@@ -806,7 +807,7 @@
 	u16 int_status;
 	struct zd_mac *mac = container_of(work, struct zd_mac, process_intr);
 
-	int_status = le16_to_cpu(*(__le16 *)(mac->intr_buffer+4));
+	int_status = get_unaligned_le16(mac->intr_buffer+4);
 	if (int_status & INT_CFG_NEXT_BCN) {
 		if (net_ratelimit())
 			dev_dbg_f(zd_mac_dev(mac), "INT_CFG_NEXT_BCN\n");

Johannes Berg

maintainer

2008-11-09 08:56 UTC #19 lore raw

Hi,

Quoted 8 lines

So, looking into this problem, I was able to come up with a patch (attached. Note, it also includes remnants of the patch recommended by Sebastian Andrzej Siewior in
http://marc.info/?l=linux-wireless&m=122493409906326&w=2). The problem occurs within compare_ether_addr() and this immediately reminded me that the patches recommended to me way back last year
(http://marc.info/?l=linux-wireless&m=119543627712471&w=2) also dealt with this problem.

I think you forgot to attach the patch.

Quoted 24 lines

Looking into how it was solved then, all the compare_ether_addr() calls were replaced with memcmp() calls. This is what I have done with my patch as well... but this approach did not fill me with confidence. It seemed to me like a quick fix that didn't directly address the alignment problem. And IMO my subsequent findings confirm this.
I now hit the following kernel errors:
Quoted 10 lines
[  240.139093] Kernel unaligned access at TPC[100f7f44]
sta_info_get+0x24/0x68 [mac80211]
[  240.233255] Kernel unaligned access at TPC[100f7f48]
sta_info_get+0x28/0x68 [mac80211]
[  240.328015] Kernel unaligned access at TPC[100f7f50]
sta_info_get+0x30/0x68 [mac80211]
[  240.422771] Kernel unaligned access at TPC[100f7f44]
sta_info_get+0x24/0x68 [mac80211]
[  240.517554] Kernel unaligned access at TPC[100f7f48]
sta_info_get+0x28/0x68 [mac80211]
Looking into this, and the underlying problem is again compare_ether_addr(). Now, I'm sure that replacing this with a memcmp() is now treading on more toes than just zd1211rw users. And for this reason, I think a better solution is going to be required.

Quoted 4 lines

Does that sound right? I will persist in trying to understand the code to try and come up with a fix. But obviously, I hope that I can get help with this to make the former an education exercise, not a kernel maintenance exercise.

:)
Yes, I think you're right, it appears that the 802.11 header isn't aligned on a 2-byte boundary, which is extremely strange. Can you, with your patch applied, do something like

	printk(KERN_DEBUG "%p\n", skb->data);

before this code in zd_mac.c:

        memcpy(skb_put(skb, length), buffer, length);

        ieee80211_rx_irqsafe(hw, skb, &stats);

johannes

Johannes Berg

maintainer

2008-11-09 09:03 UTC #20 lore raw

Quoted 1 line · Johannes Berg

I think you forgot to attach the patch.

Sorry, I missed your second mail. (btw, if you generate patches with -p in diff arguments it helps a bit reading them)

Ok so it looks like the buffer you're getting is unaligned. This is strange, but I'm too unfamiliar with the code. It seems that there's the PLCP 5 byte header in front. Well, try the printk I asked for, because at that point zd1211 should avoid "leaking" the alignment problem.

johannes

Michael Buesch

2008-11-09 12:00 UTC #17 lore raw

Quoted 4 lines · On Sunday 09 November 2008 04:06:14 Shaddy Baddah wrote:

Looking into this, and the underlying problem is again compare_ether_addr(). Now, I'm sure that replacing this with a memcmp() is now treading on more toes than just zd1211rw users. And for this reason, I think a better solution is going to be required.

I think this requires a comment in the code why we do not use compare_ether_addr() at this place. Something like:

/* We use memcmp() instead of compare_ether_addr(), because the
* packet might not be 4-byte aligned, yet. This is done later. */

Signature · 1 line

Greetings Michael.

Michael Buesch

2008-11-09 12:02 UTC #21 lore raw

Quoted 11 lines · On Sunday 09 November 2008 10:03:58 Johannes Berg wrote:

Quoted 1 line · Johannes Berg
I think you forgot to attach the patch.
Sorry, I missed your second mail. (btw, if you generate patches with -p in diff arguments it helps a bit reading them)
Ok so it looks like the buffer you're getting is unaligned. This is strange, but I'm too unfamiliar with the code. It seems that there's the PLCP 5 byte header in front. Well, try the printk I asked for, because at that point zd1211 should avoid "leaking" the alignment problem.

zd1211rw has some code to align the packet to a 4-byte boundary. But the compare_ether_addr() and the other le16 accesses are done before that realignment.
This these blow up if (ieee80211_is_data_qos(fc) ^ ieee80211_has_a4(fc))

Signature · 1 line

Greetings Michael.

Shaddy Baddah

author

2008-11-09 14:21 UTC #23 lore raw

In reply to Michael Buesch

Hi,

Quoted 11 lines · Johannes Berg

Yes, I think you're right, it appears that the 802.11 header isn't aligned on a 2-byte boundary, which is extremely strange. Can you, with your patch applied, do something like
	printk(KERN_DEBUG "%p\n", skb->data);
before this code in zd_mac.c:
        memcpy(skb_put(skb, length), buffer, length);

        ieee80211_rx_irqsafe(hw, skb, &stats);

I have done this... but I think that there is too much output to place here. What I can tell you with confidence is that every value is 4-byte aligned.

However, I modify filter_ack() in zd_mac.c so that the code looks like this:

        for (skb = q->next; skb != (struct sk_buff *)q; skb = skb->next) {
                struct ieee80211_hdr *tx_hdr;

                tx_hdr = (struct ieee80211_hdr *)skb->data;
        printk(KERN_DEBUG "%s:%u: skb %p skb->data %p tx_hdr->addr2 %p 
rx_hdr->addr1 %p\n", __FILE__, __LINE__, skb, skb->data, tx_hdr->addr2, rx_hdr->addr1);
                //if (likely(!memcmp(tx_hdr->addr2, rx_hdr->addr1, 
ETH_ALEN)))
                if (likely(!compare_ether_addr(tx_hdr->addr2, 
rx_hdr->addr1)))
                {

(note, I re-instated the compare_ether_addr() so that I could be sure that the unaligned access corresponded with whatever values I was seeing in the print).

Here is the output of the first print from this location that I see in syslog:

Nov 10 00:47:17 trad kernel: [ 8239.534950] drivers/net/wireless/zd1211rw/zd_mac.c:639: skb fffff8003d0d8fc0 skb->data fffff8003d874cdb tx_hdr->addr2 fffff8003d874ce5 rx_hdr->addr1 fffff8003f3e0009
Nov 10 00:47:17 trad kernel: [ 8239.534986] Kernel unaligned access at TPC[100f5bd0] zd_mac_rx+0x1b0/0x398 [zd1211rw]
Nov 10 00:47:17 trad kernel: [ 8239.637697] Kernel unaligned access at TPC[100f5bd4] zd_mac_rx+0x1b4/0x398 [zd1211rw]
Nov 10 00:47:17 trad kernel: [ 8239.740208] Kernel unaligned access at TPC[100f5bd8] zd_mac_rx+0x1b8/0x398 [zd1211rw]
Nov 10 00:47:17 trad kernel: [ 8239.842417] Kernel unaligned access at TPC[100f5bdc] zd_mac_rx+0x1bc/0x398 [zd1211rw]
Nov 10 00:47:17 trad kernel: [ 8239.944583] Kernel unaligned access at TPC[100f5be0] zd_mac_rx+0x1c0/0x398 [zd1211rw]

So you can see that skb->data is not even 2-byte aligned. And my debug leads me to believe that the problem is this line in zd_mac_tx_to_dev():

                 skb_pull(skb, sizeof(struct zd_ctrlset));

A before and after of this line gives me this output:

Nov 10 00:47:17 trad kernel: [ 8239.534065] drivers/net/wireless/zd1211rw/zd_mac.c:376: skb fffff8003d0d8fc0 skb->data fffff8003d874cd0
Nov 10 00:47:17 trad kernel: [ 8239.534092] drivers/net/wireless/zd1211rw/zd_mac.c:390: skb fffff8003d0d8fc0 skb->data fffff8003d874cdb

Perhaps there needs to be some padding there?

Also, I've just had a look at the git version of zd_mac.c, and I notice it is all change there again. I would feel uncomfortable having studied the 2.6.26 code to have to start again. Is it expected that I use the latest available via git?

Thanks in advance,
Shaddy

Johannes Berg

maintainer

2008-11-09 18:31 UTC #22 lore raw

Quoted 13 lines · Michael Buesch

Quoted 11 lines · On Sunday 09 November 2008 10:03:58 Johannes Berg wrote:
Quoted 1 line · Johannes Berg
I think you forgot to attach the patch.
Sorry, I missed your second mail. (btw, if you generate patches with -p in diff arguments it helps a bit reading them)
Ok so it looks like the buffer you're getting is unaligned. This is strange, but I'm too unfamiliar with the code. It seems that there's the PLCP 5 byte header in front. Well, try the printk I asked for, because at that point zd1211 should avoid "leaking" the alignment problem.

Quoted 4 lines

zd1211rw has some code to align the packet to a 4-byte boundary. But the compare_ether_addr() and the other le16 accesses are done before that realignment.
This these blow up if (ieee80211_is_data_qos(fc) ^ ieee80211_has_a4(fc))

Well, yes, however, then his patch should have fixed the whole thing, which it didn't.

johannes

Shaddy Baddah

author

2008-11-10 15:13 UTC #25 lore raw

Hi again,

On 10/11/08 01:20, Shaddy Baddah wrote:
]> So you can see that skb->data is not even 2-byte aligned. And my debug

Quoted 10 lines

leads me to believe that the problem is this line in zd_mac_tx_to_dev():
                 skb_pull(skb, sizeof(struct zd_ctrlset));
A before and after of this line gives me this output:
Nov 10 00:47:17 trad kernel: [ 8239.534065] drivers/net/wireless/zd1211rw/zd_mac.c:376: skb fffff8003d0d8fc0 skb->data fffff8003d874cd0
Nov 10 00:47:17 trad kernel: [ 8239.534092] drivers/net/wireless/zd1211rw/zd_mac.c:390: skb fffff8003d0d8fc0 skb->data fffff8003d874cdb
Perhaps there needs to be some padding there?

Ok, after a long hard slog at trying to understand all this, I'm going to have to pause the effort. I am beginning to understand the mechanisms in place... however, I am not totally sure about anything. My first impression though is that before control is even passed to the driver via zd_op_tx(), alignment issues have already hit. As far as I could tell, at that stage skb->data can already be on an odd byte boundary.

Now, all this is very confusing to me... largely because I don't fully understand what an SKB is. However, it would seem to me that the sunlance driver would use this mechanism as well, and AFAICT, that driver (even if it isn't a wireless driver) does not suffer from alignment issues.

I have much digging left... but I would greatly appreciate the offering of opinions as to whether this problem lies deeper in the kernel than the zd1211rw driver itself. I get a sense that I'm being a little disengaged... but that's totally cool. I understand the onus of at least trying to contribute a fix myself.

Quoted 4 lines

Also, I've just had a look at the git version of zd_mac.c, and I notice it is all change there again. I would feel uncomfortable having studied the 2.6.26 code to have to start again. Is it expected that I use the latest available via git?

Ignore that. I was looking at older code (via web git). Thanks in advance,
Shaddy

Shaddy Baddah

author

2008-11-28 05:34 UTC #26 lore raw

Hi,

Quoted 1 line · Shaddy Baddah

Ok, after a long hard slog at trying to understand all this, I'm going to have to pause the effort. I am beginning to understand the mechanisms in place... however, I am not totally sure about anything. My first

Devoting a little bit of time here and there to understanding the unaligned accesses, I think I understand a whole lot better now.

First off, I only came to realise that the reason the module seemed to continue operations despite reporting the unaligned access error was because the exception handler actually emulates the intended operation (albeit much slower). Good to know.

I then came to understand that we cannot expect alignment on RX in the zd1211rw module. In fact, we might expect that 80211 packets are always unaligned, because the 5 byte PLCP prefix in the SKB buffer will always push it into an odd address (assuming the SKB buffer is allocated aligned). In which case, as detailed in the Documentation/unaligned-memory-access.txt, replacing compare_ether_addr() with memcmp() is perfectly fine.

After solving that, I was still getting an unaligned access from a compare_ether_addr() call in sta_info_get() from mac80211 module. My dilemma was whether that module would expect alignment always, in which case the same simple function replacement would not be OK. But I saw several places where memcmp() is preferred to compare_ether_addr(), which I assume indicates that mac80211 does not expect alignment of 80211 packets passed to it.

Applying Sebastian Andrzej Siewior wireless/zd1211rw: use get_unaligned_le16 helper (v2) patch and my two changes quietens down the kernel completely. I will submit these two patches in follow up emails.

It is interesting (at least to me) to note that a change between kernel versions 2.6.26 and 2.6.28-rc6 fixed something that allowed WPA2 to work on sparc64. At least according to my careful verification. Perhaps one day, I'll narrow that change down.

Regards,
Shaddy

Shaddy Baddah

author Tested-by

2008-11-28 05:45 UTC #33 lore raw

Quoted 1 line · Sebastian Andrzej Siewior

Subject: [PATCH] wireless/zd1211rw: use get_unaligned_le16 helper (v2)

I'm not too sure about the protocol for updating a patch to claim that you've verified it, but I hope to get this patch applied to git by verifying it worked for me on an sparc64 machine.

Subject: [PATCH] wireless/zd1211rw: use get_unaligned_le16 helper (v2)

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

Tested-by Shaddy Baddah <[email protected]>

---
 drivers/net/wireless/zd1211rw/zd_mac.c |    5 +++--
 drivers/net/wireless/zd1211rw/zd_usb.c |    2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

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

index 4d7b98b..bf4b4a4 100644
@@ -25,6 +25,7 @@
 #include <linux/usb.h>
 #include <linux/jiffies.h>
 #include <net/ieee80211_radiotap.h>
+#include <asm/unaligned.h>
 
 #include "zd_def.h"
 #include "zd_chip.h"

@@ -662,7 +663,7 @@ int zd_mac_rx(struct ieee80211_hw *hw, const u8 *buffer, unsigned int length)
 			&& !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));

@@ -758,7 +759,7 @@ void zd_process_intr(struct work_struct *work)
 	u16 int_status;
 	struct zd_mac *mac = container_of(work, struct zd_mac, process_intr);
 
-	int_status = le16_to_cpu(*(__le16 *)(mac->intr_buffer+4));
+	int_status = get_unaligned_le16(mac->intr_buffer+4);
 	if (int_status & INT_CFG_NEXT_BCN) {
 		if (net_ratelimit())
 			dev_dbg_f(zd_mac_dev(mac), "INT_CFG_NEXT_BCN\n");

drivers/net/wireless/zd1211rw/zd_usb.c +1 -1

index a60ae86..0b27778 100644
@@ -355,7 +355,7 @@ static inline void handle_regs_int(struct urb *urb)
 	ZD_ASSERT(in_interrupt());
 	spin_lock(&intr->lock);
 
-	int_num = le16_to_cpu(*(__le16 *)(urb->transfer_buffer+2));
+	int_num = get_unaligned_le16(urb->transfer_buffer+2);
 	if (int_num == CR_INTERRUPT) {
 		struct zd_mac *mac = zd_hw_mac(zd_usb_to_hw(urb->context));
 		memcpy(&mac->intr_buffer, urb->transfer_buffer,

-- 1.6.0.2

Harvey Harrison

2008-11-28 06:47 UTC #35 lore raw

Quoted 30 lines · Shaddy Baddah

Quoted 1 line · Sebastian Andrzej Siewior
Subject: [PATCH] wireless/zd1211rw: use get_unaligned_le16 helper (v2)
I'm not too sure about the protocol for updating a patch to claim that you've verified it, but I hope to get this patch applied to git by verifying it worked for me on an sparc64 machine.
Subject: [PATCH] wireless/zd1211rw: use get_unaligned_le16 helper (v2)
Signed-off-by Sebastian Andrzej Siewior <[email protected]>
Tested-by Shaddy Baddah <[email protected]>
---
  drivers/net/wireless/zd1211rw/zd_mac.c |    5 +++--
  drivers/net/wireless/zd1211rw/zd_usb.c |    2 +-
  2 files changed, 4 insertions(+), 3 deletions(-)
drivers/net/wireless/zd1211rw/zd_mac.c +1 -0
index 4d7b98b..bf4b4a4 100644
@@ -25,6 +25,7 @@
  #include <linux/usb.h>
  #include <linux/jiffies.h>
  #include <net/ieee80211_radiotap.h>
+#include <asm/unaligned.h>
 
  #include "zd_def.h"
  #include "zd_chip.h"

drivers/net/wireless/zd1211rw/zd_mac.c int zd_mac_rx(struct ieee80211_hw *hw, const u8 · line 669

Quoted 8 lines

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

@@ -662,7 +663,7 @@ int zd_mac_rx(struct ieee80211_hw *hw, const u8 
 *buffer, unsigned int length)
  			&& !mac->pass_ctrl)
  		return 0;
 
-	fc = *(__le16 *)buffer;
+	fc = get_unaligned_le16(buffer);
  	need_padding = ieee80211_is_data_qos(fc) ^ ieee80211_has_a4(fc);

I think this should be:

	fc = get_unaligned((__le16 *)buffer);

As ieee8011_is_data_qos wants a le16 and your use above will swap to cpu-order.

drivers/net/wireless/zd1211rw/zd_mac.c void zd_process_intr(struct work_struct *work) · line 762

Quoted 8 lines

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

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

@@ -758,7 +759,7 @@ void zd_process_intr(struct work_struct *work)
  	u16 int_status;
  	struct zd_mac *mac = container_of(work, struct zd_mac, process_intr);
 
-	int_status = le16_to_cpu(*(__le16 *)(mac->intr_buffer+4));
+	int_status = get_unaligned_le16(mac->intr_buffer+4);

OK.

Quoted 9 lines

drivers/net/wireless/zd1211rw/zd_mac.c +0 -0

  	if (int_status & INT_CFG_NEXT_BCN) {
  		if (net_ratelimit())
  			dev_dbg_f(zd_mac_dev(mac), "INT_CFG_NEXT_BCN\n");

drivers/net/wireless/zd1211rw/zd_usb.c +0 -0

index a60ae86..0b27778 100644

drivers/net/wireless/zd1211rw/zd_usb.c static inline void handle_regs_int(struct urb *urb) · line 361

Quoted 8 lines

drivers/net/wireless/zd1211rw/zd_usb.c +1 -1

@@ -355,7 +355,7 @@ static inline void handle_regs_int(struct urb *urb)
  	ZD_ASSERT(in_interrupt());
  	spin_lock(&intr->lock);
 
-	int_num = le16_to_cpu(*(__le16 *)(urb->transfer_buffer+2));
+	int_num = get_unaligned_le16(urb->transfer_buffer+2);
  	if (int_num == CR_INTERRUPT) {
  		struct zd_mac *mac = zd_hw_mac(zd_usb_to_hw(urb->context));
  		memcpy(&mac->intr_buffer, urb->transfer_buffer,

OK.

So the first hunk needs changing, but other than that, looks OK.

Harvey

David Miller

2008-11-28 07:10 UTC #27 lore raw

From: Shaddy Baddah <[email protected]>
Date: Fri, 28 Nov 2008 16:34:34 +1100

Quoted 4 lines

It is interesting (at least to me) to note that a change between kernel versions 2.6.26 and 2.6.28-rc6 fixed something that allowed WPA2 to work on sparc64. At least according to my careful verification. Perhaps one day, I'll narrow that change down.

That would be the following set of changes:

commit 0f5cabba49021d36e9f76bd97d7fa0f4a408063f

Author: David S. Miller <[email protected]>
Date:   Tue Jun 3 07:39:16 2008 -0700

wext: Create IW_REQUEST_FLAG_COMPAT and set it as needed.

Now low-level WEXT ioctl handlers can do compat handling when necessary.

commit 169a3ec492ddb6b0a8203fccba2ddff077154e26

Author: David S. Miller <[email protected]>
Date:   Fri Dec 21 03:55:13 2007 -0800

wext: Remove compat handling from fs/compat_ioctl.c

No longer used.

commit 87de87d5e47f94b4ea647a5bd1bc8dc1f7930db4

Author: David S. Miller <[email protected]>
Date:   Tue Jun 3 09:14:03 2008 -0700

wext: Dispatch and handle compat ioctls entirely in net/wireless/wext.c

Next we can kill the hacks in fs/compat_ioctl.c and also dispatch compat ioctls down into the driver and 80211 protocol helper layers in order to handle iw_point objects embedded in stream replies which need to be translated.

commit a67fa76d8be4e24e2d61cd76438a893d4c2886f7

Author: David S. Miller <[email protected]>
Date:   Tue Jun 3 07:36:30 2008 -0700

wext: Pull top-level ioctl dispatch logic into helper function.

commit d2911255590d9ca561a481b9dbebcfcbbf38fa4e

Author: David S. Miller <[email protected]>
Date:   Fri Dec 21 03:46:01 2007 -0800

wext: Pass iwreq pointer down into standard/private handlers.

They have no need to see the object as an ifreq.

commit ca1e8bb8e4e89e2769e2b39eb29fdcfc5c19cf89

Author: David S. Miller <[email protected]>
Date:   Fri Dec 21 03:41:45 2007 -0800

wext: Parameterize the standard/private handlers.

The WEXT standard and private handlers to use are now arguments to wireless_process_ioctl().

commit 67dd7608078b17f63f29ff2108fc5bf2407ddcec

Author: David S. Miller <[email protected]>
Date:   Fri Dec 21 03:36:31 2007 -0800

wext: Pull ioctl permission checking out into helper function.

commit d88174e4d295f0880e5f9cb6d42f26b0367c8fd9

Author: David S. Miller <[email protected]>
Date:   Fri Dec 21 03:33:46 2007 -0800

wext: Extract private call iw_point handling into seperate functions.

commit 84149b0fca08f9ec554dfc28dabc39839fdf8a06

Author: David S. Miller <[email protected]>
Date:   Fri Dec 21 03:27:17 2007 -0800

wext: Extract standard call iw_point handling into seperate function.

commit 208887d4cc5a5c1eeb68bd170e21e32b1129cd94

Author: David S. Miller <[email protected]>
Date:   Fri Dec 21 03:24:24 2007 -0800

wext: Make adjust_priv_size() take a "struct iw_point *".

commit 25519a2a769d42fc2733a8f119682272d99b1304

Author: David S. Miller <[email protected]>
Date:   Fri Dec 21 03:22:38 2007 -0800

wext: Remove inline from get_priv_size() and adjust_priv_size().

The compiler inlines when appropriate.

Michael Buesch

2008-11-28 22:45 UTC #28 lore raw

Quoted 4 lines · On Friday 28 November 2008 06:34:34 Shaddy Baddah wrote:

case the same simple function replacement would not be OK. But I saw several places where memcmp() is preferred to compare_ether_addr(), which I assume indicates that mac80211 does not expect alignment of 80211 packets passed to it.

The whole networking stack expects 4byte alignment and the driver _must_ make sure it is this way. Either by padding a constant number of bytes at the front of each allocated SKB, or (if the alignment differs) use dynamic checks for the lowest few bits of the pointer passed to mac80211.

Signature · 1 line

Greetings Michael.

Shaddy Baddah

author

2008-11-29 08:59 UTC #29 lore raw

Quoted 7 lines · Michael Buesch

Quoted 1 line · On Friday 28 November 2008 06:34:34 Shaddy Baddah wrote:
case the same simple function replacement would not be OK. But I saw several places where memcmp() is preferred to compare_ether_addr(), which I assume indicates that mac80211 does not expect alignment of 80211 packets passed to it.
The whole networking stack expects 4byte alignment and the driver _must_ make sure it is this way. Either by padding a constant number of bytes at the front of each allocated SKB, or (if the alignment differs) use dynamic checks for the lowest few bits of the pointer passed to mac80211.

Right. But I'm not sure that in this case that the packet is being passed to mac80211 by the driver. My recall of the stack traces I saw were that the unalignment I was seeing in sta_info_get() were a consequence of transmission attempts.

I must admit though, I could not source where the unaligned packet came from, as I do not fully understand the different queuing mechanisms in use and where and how enqueues occur. My limited efforts to understand this gave me the impression that network stack itself, and not the driver, was constructing these unaligned packets, for transmission.

The memcmp() that really persuaded me to believe that the mac80211 had no alignment requirements was this one:

net/mac80211/tx.c:1304: if (memcmp(odev->dev_addr, hdr->addr4, ETH_ALEN) != 0)

I would have thought it would use compare_ether_addr() if we could safely assume alignment. In any case, say that alignment was always the intention... then can't we just use memcmp() as a hack (not being derogatory. Just that it would turn into a hack in deference to using compare_ether_addr()) consistent with hacks like the above? Even in the interim until a program to iron out alignment is set in place?

Regards,
Shaddy

Johannes Berg

maintainer

2008-11-29 09:50 UTC #31 lore raw