From: Benoit PAPILLAULT <[email protected]>
This patch is close to the original code except that
ieee80211_get_hdrlen_from_skb() has been replaced by
ath5k_hw_get_hdrlen_from_skb() which is specific to Atheros hardware. The same
probably applies to ath9k as well.
Sign-off-by: Benoit Papillault <[email protected]>
---
drivers/net/wireless/ath5k/base.c | 33 ++++++++++++++++-----------------
drivers/net/wireless/ath5k/base.h | 34 ++++++++++++++++++++++++++++++++++
2 files changed, 50 insertions(+), 17 deletions(-)
diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
index 9d2c597..ac17960 100644
--- a/drivers/net/wireless/ath5k/base.c
+++ b/drivers/net/wireless/ath5k/base.c
@@ -1192,7 +1192,7 @@ ath5k_txbuf_setup(struct ath5k_softc *sc, struct ath5k_buf *bf)
pktlen += info->control.hw_key->icv_len;
}
ret = ah->ah_setup_tx_desc(ah, ds, pktlen,
- ieee80211_get_hdrlen_from_skb(skb), AR5K_PKT_TYPE_NORMAL,
+ ath5k_hw_get_hdrlen_from_skb(skb), AR5K_PKT_TYPE_NORMAL,
(sc->power_level * 2),
ieee80211_get_tx_rate(sc->hw, info)->hw_value,
info->control.rates[0].count, keyidx, 0, flags, 0, 0);
@@ -1667,7 +1667,7 @@ ath5k_tasklet_rx(unsigned long data)
struct ath5k_desc *ds;
int ret;
int hdrlen;
- int padsize;
+ int pad;
spin_lock(&sc->rxbuflock);
if (list_empty(&sc->rxbuf)) {
@@ -1760,11 +1760,11 @@ accept:
* bytes and we can optimize this a bit. In addition, we must
* not try to remove padding from short control frames that do
* not have payload. */
- hdrlen = ieee80211_get_hdrlen_from_skb(skb);
- padsize = hdrlen & 3;
- if (padsize && hdrlen >= 24) {
- memmove(skb->data + padsize, skb->data, hdrlen);
- skb_pull(skb, padsize);
+ hdrlen = ath5k_hw_get_hdrlen_from_skb(skb);
+ if (hdrlen & 3) {
+ pad = hdrlen & 3;
+ memmove(skb->data + pad, skb->data, hdrlen);
+ skb_pull(skb, pad);
}
/*
@@ -1965,7 +1965,7 @@ ath5k_beacon_setup(struct ath5k_softc *sc, struct ath5k_buf *bf)
ds->ds_data = bf->skbaddr;
ret = ah->ah_setup_tx_desc(ah, ds, skb->len,
- ieee80211_get_hdrlen_from_skb(skb),
+ ath5k_hw_get_hdrlen_from_skb(skb),
AR5K_PKT_TYPE_BEACON, (sc->power_level * 2),
ieee80211_get_tx_rate(sc->hw, info)->hw_value,
1, AR5K_TXKEYIX_INVALID,
@@ -2625,7 +2625,7 @@ ath5k_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
struct ath5k_buf *bf;
unsigned long flags;
int hdrlen;
- int padsize;
+ int pad;
ath5k_debug_dump_skb(sc, skb, "TX ", 1);
@@ -2636,17 +2636,16 @@ ath5k_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
* the hardware expects the header padded to 4 byte boundaries
* if this is not the case we add the padding after the header
*/
- hdrlen = ieee80211_get_hdrlen_from_skb(skb);
- padsize = hdrlen & 3;
- if (padsize && hdrlen >= 24) {
-
- if (skb_headroom(skb) < padsize) {
+ hdrlen = ath5k_hw_get_hdrlen_from_skb(skb);
+ if (hdrlen & 3) {
+ pad = hdrlen & 3;
+ if (skb_headroom(skb) < pad) {
ATH5K_ERR(sc, "tx hdrlen not %%4: %d not enough"
- " headroom to pad %d\n", hdrlen, padsize);
+ " headroom to pad %d\n", hdrlen, pad);
return -1;
}
- skb_push(skb, padsize);
- memmove(skb->data, skb->data+padsize, hdrlen);
+ skb_push(skb, pad);
+ memmove(skb->data, skb->data+pad, hdrlen);
}
spin_lock_irqsave(&sc->txbuflock, flags);
diff --git a/drivers/net/wireless/ath5k/base.h b/drivers/net/wireless/ath5k/base.h
index facc60d..85ff036 100644
--- a/drivers/net/wireless/ath5k/base.h
+++ b/drivers/net/wireless/ath5k/base.h
@@ -187,4 +187,38 @@ struct ath5k_softc {
#define ath5k_hw_hasveol(_ah) \
(ath5k_hw_get_capability(_ah, AR5K_CAP_VEOL, 0, NULL) == 0)
+/* This formula returns the header length and is specific to Atheros hardware
+ * and differs from 802.11 standards. It has been tested using an AR5212
+ * hardware. */
+
+static inline unsigned int ath5k_hw_get_hdrlen_from_skb(
+ const struct sk_buff *skb)
+{
+ const struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
+ unsigned int hdrlen;
+
+ /* Since we need to read Frame Control field, we first check that the
+ * frames contains at least 2 bytes */
+ if (unlikely(skb->len < 2))
+ return 0;
+
+ hdrlen = 24;
+
+ /* ToDS=1 FromDS=1 frames have an additionnal Address4 field */
+ if (ieee80211_has_a4(hdr->frame_control)) {
+ hdrlen += 6; /* IEEE80211_ADDR_LEN */
+ }
+
+ /* QoS data frames have an additionnal QoS Control field */
+ if (ieee80211_is_data_qos(hdr->frame_control)) {
+ hdrlen += IEEE80211_QOS_CTL_LEN;
+ }
+
+ /* check that skb already contains at least hdrlen bytes */
+ if (unlikely(hdrlen > skb->len))
+ return 0;
+
+ return hdrlen;
+}
+
#endif
--
1.5.6.5
On Wed, Dec 17, 2008 at 12:40:56PM -0500, Bob Copeland wrote:
> On Wed, Dec 17, 2008 at 10:55 AM, John W. Linville
> <[email protected]> wrote:
> > On Tue, Dec 16, 2008 at 10:22:39AM -0500, Bob Copeland wrote:
> >> Well, anyway John already picked up your earlier (better, IMHO) patch.
> >> Now we just need to fix the tx descriptors :)
> >
> > Based on that comment, I'm dropping this version of the patch.
> > Feel free to submit additional patches to implement whatever might
> > be missing now.
>
> Okay. I think the one you took before, plus my diff should be sufficient.
> Do you want me to submit a proper patch that just fixes the TX, or should
> we regroup and combine both up into a single patch on our side?
>
> Either way I'll give it a little testing tonight to make sure we didn't
> miss anything.
Just your new patch will be sufficient I think...?
Thanks,
John
--
John W. Linville Linux should be at the core
[email protected] of your literate lifestyle.
On Mon, Dec 15, 2008 at 9:34 AM, Benoit Papillault
<[email protected]> wrote:
> From: Benoit PAPILLAULT <[email protected]>
>
> This patch is close to the original code except that
> ieee80211_get_hdrlen_from_skb() has been replaced by
> ath5k_hw_get_hdrlen_from_skb() which is specific to Atheros hardware. The same
> probably applies to ath9k as well.
I for one would rather still use ieee80211_get_hdrlen_from_skb(), and the
ath5k_pad_size() helper to calculate the pad size (=0 for small control
frames). Was there any problem with the '> 24' check?
After all we're not computing the _header_ length, just the existence
of the pad (contents of which is meaningless so can't be counted as part
of the header).
--
Bob Copeland %% http://www.bobcopeland.com
On Mon, Dec 15, 2008 at 4:06 PM, Benoit PAPILLAULT
<[email protected]> wrote:
> frames. To me, the bug we have seen when receiving a ACK frame in
> monitor mode was not because "ACK is a small control frame", it was
> because the header length we computed was smaller than what the hardware
> considered.
It's because data frames have a payload and ACKs do not.
> To reply to Bob as well, we need to compute the number of padded bytes +
> the position at which the padding occurs, so proper "header" length is
> required.
Well, anyway John already picked up your earlier (better, IMHO) patch.
Now we just need to fix the tx descriptors :)
--
Bob Copeland %% http://www.bobcopeland.com
On Tue, Dec 16, 2008 at 10:22:39AM -0500, Bob Copeland wrote:
> On Mon, Dec 15, 2008 at 4:06 PM, Benoit PAPILLAULT
> <[email protected]> wrote:
>
> > frames. To me, the bug we have seen when receiving a ACK frame in
> > monitor mode was not because "ACK is a small control frame", it was
> > because the header length we computed was smaller than what the hardware
> > considered.
>
> It's because data frames have a payload and ACKs do not.
>
> > To reply to Bob as well, we need to compute the number of padded bytes +
> > the position at which the padding occurs, so proper "header" length is
> > required.
>
> Well, anyway John already picked up your earlier (better, IMHO) patch.
> Now we just need to fix the tx descriptors :)
Based on that comment, I'm dropping this version of the patch.
Feel free to submit additional patches to implement whatever might
be missing now.
John
--
John W. Linville Linux should be at the core
[email protected] of your literate lifestyle.
On Wed, 17 Dec 2008 13:35:27 -0500, John W. Linville wrote
> > Okay. I think the one you took before, plus my diff should be sufficient.
> > Do you want me to submit a proper patch that just fixes the TX, or should
> > we regroup and combine both up into a single patch on our side?
>
> Just your new patch will be sufficient I think...?
Ok here you go. In fact we won't trigger this case very often since
those packets are usually sent by hardware, but anyway we should be
consistent everywhere. This is on top of 28bd3684fb5.
>From 853eff110f7aefaf150b189ac8a87b77fa9447c5 Mon Sep 17 00:00:00 2001
From: Bob Copeland <[email protected]>
Date: Thu, 18 Dec 2008 23:23:05 -0500
Subject: [PATCH] ath5k: correct packet length in tx descriptors
Packet length calculation (which includes frame check sequence)
should take into account whether we add a pad field or not.
Extract the calculation into a helper and use it in both places.
Changes to desc.c
Changes-licensed-under: ISC
Changes to ath5k.h, base.c
Changes-licensed-under: 3-Clause-BSD
Signed-off-by: Bob Copeland <[email protected]>
---
drivers/net/wireless/ath5k/ath5k.h | 5 +++++
drivers/net/wireless/ath5k/base.c | 8 ++++----
drivers/net/wireless/ath5k/desc.c | 4 ++--
3 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/ath5k/ath5k.h
b/drivers/net/wireless/ath5k/ath5k.h
index 13df119..183ffc8 100644
--- a/drivers/net/wireless/ath5k/ath5k.h
+++ b/drivers/net/wireless/ath5k/ath5k.h
@@ -1350,4 +1350,9 @@ static inline u32 ath5k_hw_bitswap(u32 val, unsigned int
bits)
return retval;
}
+static inline int ath5k_pad_size(int hdrlen)
+{
+ return (hdrlen < 24) ? 0 : hdrlen & 3;
+}
+
#endif
diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
index 9d2c597..40c0155 100644
--- a/drivers/net/wireless/ath5k/base.c
+++ b/drivers/net/wireless/ath5k/base.c
@@ -1761,8 +1761,8 @@ accept:
* not try to remove padding from short control frames that do
* not have payload. */
hdrlen = ieee80211_get_hdrlen_from_skb(skb);
- padsize = hdrlen & 3;
- if (padsize && hdrlen >= 24) {
+ padsize = ath5k_pad_size(hdrlen);
+ if (padsize) {
memmove(skb->data + padsize, skb->data, hdrlen);
skb_pull(skb, padsize);
}
@@ -2637,8 +2637,8 @@ ath5k_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
* if this is not the case we add the padding after the header
*/
hdrlen = ieee80211_get_hdrlen_from_skb(skb);
- padsize = hdrlen & 3;
- if (padsize && hdrlen >= 24) {
+ padsize = ath5k_pad_size(hdrlen);
+ if (padsize) {
if (skb_headroom(skb) < padsize) {
ATH5K_ERR(sc, "tx hdrlen not %%4: %d not enough"
diff --git a/drivers/net/wireless/ath5k/desc.c b/drivers/net/wireless/ath5k/desc.c
index 5e362a7..b40a928 100644
--- a/drivers/net/wireless/ath5k/desc.c
+++ b/drivers/net/wireless/ath5k/desc.c
@@ -71,7 +71,7 @@ ath5k_hw_setup_2word_tx_desc(struct ath5k_hw *ah, struct
ath5k_desc *desc,
/* Verify and set frame length */
/* remove padding we might have added before */
- frame_len = pkt_len - (hdr_len & 3) + FCS_LEN;
+ frame_len = pkt_len - ath5k_pad_size(hdr_len) + FCS_LEN;
if (frame_len & ~AR5K_2W_TX_DESC_CTL0_FRAME_LEN)
return -EINVAL;
@@ -202,7 +202,7 @@ static int ath5k_hw_setup_4word_tx_desc(struct ath5k_hw *ah,
/* Verify and set frame length */
/* remove padding we might have added before */
- frame_len = pkt_len - (hdr_len & 3) + FCS_LEN;
+ frame_len = pkt_len - ath5k_pad_size(hdr_len) + FCS_LEN;
if (frame_len & ~AR5K_4W_TX_DESC_CTL0_FRAME_LEN)
return -EINVAL;
--
1.6.0.4
--
Bob Copeland %% http://www.bobcopeland.com
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Jouni Malinen a =E9crit :
> On Mon, Dec 15, 2008 at 03:34:58PM +0100, Benoit Papillault wrote:
>> This patch is close to the original code except that
>> ieee80211_get_hdrlen_from_skb() has been replaced by
>> ath5k_hw_get_hdrlen_from_skb() which is specific to Atheros hardware=
=2E The same
>> probably applies to ath9k as well.
>=20
> Most of this is changes on how unspecified frames are handled (e.g.,
> frames that are not used in IEEE 802.11). What is the use case that
> justifies this type of extra complexity to be added into the driver?
> Please note that there are no guarantees that all hardware revisions
> behave the same as far as undocumented functionality is concerned. Th=
e
> only clear case when padding is required is data frames with non-empt=
y
> frame body.
>=20
Agreed. In fact, I wanted to know how the hardware behaves in the
general case to better understand it in the particular case of 802.11
frames. To me, the bug we have seen when receiving a ACK frame in
monitor mode was not because "ACK is a small control frame", it was
because the header length we computed was smaller than what the hardwar=
e
considered.
Another point is that we have to deal with anything on the RX side, be
it later classified as invalid or not.
As far as the hardware is concerned, I'm not working for Atheros, but I
bet that hardware guys did the same thing for all their chipsets. I
could test it on AR5416 and AR9160 if needed.
To reply to Bob as well, we need to compute the number of padded bytes =
+
the position at which the padding occurs, so proper "header" length is
required.
Just my 2 cents.
Benoit
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFJRsbZOR6EySwP7oIRArWVAJ0fjqY01Xi3qTH7DIJBE9D+c099pQCgp/u5
pHHZiKOzsU83CEecamnuZpY=3D
=3Dw10J
-----END PGP SIGNATURE-----
On Wed, Dec 17, 2008 at 10:55 AM, John W. Linville
<[email protected]> wrote:
> On Tue, Dec 16, 2008 at 10:22:39AM -0500, Bob Copeland wrote:
>> Well, anyway John already picked up your earlier (better, IMHO) patch.
>> Now we just need to fix the tx descriptors :)
>
> Based on that comment, I'm dropping this version of the patch.
> Feel free to submit additional patches to implement whatever might
> be missing now.
Okay. I think the one you took before, plus my diff should be sufficient.
Do you want me to submit a proper patch that just fixes the TX, or should
we regroup and combine both up into a single patch on our side?
Either way I'll give it a little testing tonight to make sure we didn't
miss anything.
--
Bob Copeland %% http://www.bobcopeland.com
On Mon, Dec 15, 2008 at 03:34:58PM +0100, Benoit Papillault wrote:
> This patch is close to the original code except that
> ieee80211_get_hdrlen_from_skb() has been replaced by
> ath5k_hw_get_hdrlen_from_skb() which is specific to Atheros hardware. The same
> probably applies to ath9k as well.
Most of this is changes on how unspecified frames are handled (e.g.,
frames that are not used in IEEE 802.11). What is the use case that
justifies this type of extra complexity to be added into the driver?
Please note that there are no guarantees that all hardware revisions
behave the same as far as undocumented functionality is concerned. The
only clear case when padding is required is data frames with non-empty
frame body.
--
Jouni Malinen PGP id EFC895FA
On Tue, Dec 16, 2008 at 10:22 AM, Bob Copeland <[email protected]> wrote:
> On Mon, Dec 15, 2008 at 4:06 PM, Benoit PAPILLAULT
> <[email protected]> wrote:
> Well, anyway John already picked up your earlier (better, IMHO) patch.
> Now we just need to fix the tx descriptors :)
i.e. something like the following (attached as well because gmail will
destroy it). I only compile-tested it.
diff --git a/drivers/net/wireless/ath5k/ath5k.h
b/drivers/net/wireless/ath5k/ath5k.h
index 13df119..183ffc8 100644
--- a/drivers/net/wireless/ath5k/ath5k.h
+++ b/drivers/net/wireless/ath5k/ath5k.h
@@ -1350,4 +1350,9 @@ static inline u32 ath5k_hw_bitswap(u32 val,
unsigned int bits)
return retval;
}
+static inline int ath5k_pad_size(int hdrlen)
+{
+ return (hdrlen < 24) ? 0 : hdrlen & 3;
+}
+
#endif
diff --git a/drivers/net/wireless/ath5k/base.c
b/drivers/net/wireless/ath5k/base.c
index f222b4a..0c3e186 100644
--- a/drivers/net/wireless/ath5k/base.c
+++ b/drivers/net/wireless/ath5k/base.c
@@ -1754,8 +1754,8 @@ accept:
* not try to remove padding from short control frames that do
* not have payload. */
hdrlen = ieee80211_get_hdrlen_from_skb(skb);
- padsize = hdrlen & 3;
- if (padsize && hdrlen >= 24) {
+ padsize = ath5k_pad_size(hdrlen);
+ if (padsize) {
memmove(skb->data + padsize, skb->data, hdrlen);
skb_pull(skb, padsize);
}
@@ -2620,8 +2620,8 @@ ath5k_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
* if this is not the case we add the padding after the header
*/
hdrlen = ieee80211_get_hdrlen_from_skb(skb);
- padsize = hdrlen & 3;
- if (padsize && hdrlen >= 24) {
+ padsize = ath5k_pad_size(hdrlen);
+ if (padsize) {
if (skb_headroom(skb) < padsize) {
ATH5K_ERR(sc, "tx hdrlen not %%4: %d not enough"
diff --git a/drivers/net/wireless/ath5k/desc.c
b/drivers/net/wireless/ath5k/desc.c
index 5e362a7..b40a928 100644
--- a/drivers/net/wireless/ath5k/desc.c
+++ b/drivers/net/wireless/ath5k/desc.c
@@ -71,7 +71,7 @@ ath5k_hw_setup_2word_tx_desc(struct ath5k_hw *ah,
struct ath5k_desc *desc,
/* Verify and set frame length */
/* remove padding we might have added before */
- frame_len = pkt_len - (hdr_len & 3) + FCS_LEN;
+ frame_len = pkt_len - ath5k_pad_size(hdr_len) + FCS_LEN;
if (frame_len & ~AR5K_2W_TX_DESC_CTL0_FRAME_LEN)
return -EINVAL;
@@ -202,7 +202,7 @@ static int ath5k_hw_setup_4word_tx_desc(struct ath5k_hw *ah,
/* Verify and set frame length */
/* remove padding we might have added before */
- frame_len = pkt_len - (hdr_len & 3) + FCS_LEN;
+ frame_len = pkt_len - ath5k_pad_size(hdr_len) + FCS_LEN;
if (frame_len & ~AR5K_4W_TX_DESC_CTL0_FRAME_LEN)
return -EINVAL;
--
Bob Copeland %% http://www.bobcopeland.com