: atk5k: fix FCS corruption for ACKs
RFC 1/1 : atk5k: fix FCS corruption for ACKs
When running a monitor interface with ath5k, the FCS of ACK frames is two bytes short. The reason is that unlike the payload, the FCS doesn't seem to aligned to a 4 byte boundary by the chip, the attempt to remove the padding corrupts it.
This patch checks whether there actually is any payload after the header (besides the FCS) before removing the padding.
This might not be fully correct or not handle other cases where this can occur, but it doesn't seem too hackish and fixes the problem for me :)
http://bugzilla.kernel.org/show_bug.cgi?id=12101
commit 8c1d598338077d397aed25fe4840e4d4b51ba79c
Author: Patrick McHardy <[email protected]> Date: Tue Dec 9 21:52:00 2008 +0100
atk5k: fix FCS corruption for ACKs
The trailing FCS value is not aliged when no payload is present. Don't try to remove padding to avoid corrupting it.
Kernel Bugzilla #12101
drivers/net/wireless/ath5k/base.c +1 -1
@@ -1752,7 +1752,7 @@ accept:* not multiples of 4 - remove it */ hdrlen =ieee80211_get_hdrlen_from_skb (skb); -if (hdrlen &3 ) { +if (hdrlen &3 && hdrlen != rs.rs_datalen -FCS_LEN ) { pad = hdrlen %4 ;memmove (skb->data + pad, skb->data, hdrlen);skb_pull (skb, pad);
drivers/net/wireless/ath5k/base.c accept: · line 1758
Quoted 9 lines · Patrick McHardy
This might not be fully correct or not handle other cases where this can occur, but it doesn't seem too hackish and fixes the problem for me :)
drivers/net/wireless/ath5k/base.c +1
-1-if (hdrlen &3 ) { +if (hdrlen &3 && hdrlen != rs.rs_datalen -FCS_LEN ) { pad = hdrlen %4 ;memmove (skb->data + pad, skb->data, hdrlen);skb_pull (skb, pad);
It seems very plausible to me. Though, why doesn't ath9k also have this problem? Luis, it looks like in that case ath9k could trim two extra bytes if ath9k hw behaves the same.
main.c:
951 /* see if any padding is done by the hw and remove it */
952 if (hdrlen & 3) {
953 padsize = hdrlen % 4;
954 memmove(skb->data + padsize, skb->data, hdrlen);
955 skb_pull(skb, padsize);
956 } 957 /* remove FCS before passing up to protocol stack */
958 skb_trim(skb, (skb->len - FCS_LEN));
Signature · 1 line
Bob Copeland %% www.bobcopeland.com
Johannes Berg
maintainerQuoted 13 lines · Bob Copeland
It seems very plausible to me. Though, why doesn't ath9k also have this problem? Luis, it looks like in that case ath9k could trim two extra bytes if ath9k hw behaves the same.
main.c:951 /* see if any padding is done by the hw and remove it */ 952 if (hdrlen & 3) { 953 padsize = hdrlen % 4; 954 memmove(skb->data + padsize, skb->data, hdrlen); 955 skb_pull(skb, padsize); 956 }
Quoted 2 lines
957 /* remove FCS before passing up to protocol stack */
958skb_trim(skb, (skb->len - FCS_LEN));
It also shouldn't remove the FCS, sometimes you want to see that on wireshark and mac80211 handles removing the FCS itself just fine, just set the HW flag.
johannes
Quoted 10 lines · Bob Copeland
951 /* see if any padding is done by the hw and remove it */ 952 if (hdrlen & 3) { 953 padsize = hdrlen % 4; 954 memmove(skb->data + padsize, skb->data, hdrlen); 955 skb_pull(skb, padsize); 956 }957 /* remove FCS before passing up to protocol stack */
958skb_trim(skb, (skb->len - FCS_LEN));
I remember removing this piece of code...
Sujith
Quoted 5 lines · Sujith
Quoted 3 lines
957 /* remove FCS before passing up to protocol stack */
958skb_trim(skb, (skb->len - FCS_LEN));I remember removing this piece of code...
Hmm, you're right. I must've been looking at 2.6.27. Sorry for the noise.
The padding still gets removed regardless of whether there is a payload; does that truncate the FCS for ACK/CTS frames on ath9k?
Quoted 2 lines
Quoted 2 lines
952 if (hdrlen & 3) { 953 padsize = hdrlen % 4;
While we're at it, (for ath5k also) -- hdrlen % 4 can just be hdrlen & 3 again.
Signature · 1 line
Bob Copeland %% www.bobcopeland.com
Quoted 5 lines · Bob Copeland
Quoted 3 lines · Patrick McHardy
This might not be fully correct or not handle other cases where this can occur, but it doesn't seem too hackish and fixes the problem for me :)
Quoted 8 lines
drivers/net/wireless/ath5k/base.c accept: · line 1758
Quoted 5 lines
drivers/net/wireless/ath5k/base.c +1
-1-if (hdrlen &3 ) { +if (hdrlen &3 && hdrlen != rs.rs_datalen -FCS_LEN ) { pad = hdrlen %4 ;memmove (skb->data + pad, skb->data, hdrlen);skb_pull (skb, pad);It seems very plausible to me. Though, why doesn't ath9k also have this problem?
So, I guess ath9k did too. This patch gets my ack, though we should probably do the hdrlen check the same way as Jouni's patch for consistency, and switch to hdrlen & 3 when computing pad.
Signature · 1 line
Bob Copeland %% www.bobcopeland.com