Re: [PATCH] ath5k : Fix correct padding

Re: [PATCH] ath5k : Fix correct padding

[Michael Buesch]

On Thursday 11 December 2008 22:58:59 Benoit PAPILLAULT wrote:
> Padding the 802.11 header to a multiple of 4 bytes needs to be done only
> for DATA frames.

Uhm, can you explain _why_ it's only needed for data frames?


-- 
Greetings, Michael.

[PATCH] ath5k : Fix correct padding

[Benoit PAPILLAULT]

Padding the 802.11 header to a multiple of 4 bytes needs to be done only
for DATA frames. This fixes a bug where 2 bytes were missing in monitor
mode for ACK frames.

Ref: http://bugzilla.kernel.org/show_bug.cgi?id=12101 :
Signed-off-by: Benoit Papillault <benoit.papillault@free.fr>
---
  drivers/net/wireless/ath5k/base.c |   49
+++++++++++++++++++++++-------------
  1 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/drivers/net/wireless/ath5k/base.c
b/drivers/net/wireless/ath5k/base.c
index bfb0c49..2318b1c 100644
--- a/drivers/net/wireless/ath5k/base.c
+++ b/drivers/net/wireless/ath5k/base.c
@@ -1668,6 +1668,7 @@ ath5k_tasklet_rx(unsigned long data)
  	int ret;
  	int hdrlen;
  	int pad;
+	struct ieee80211_hdr * hdr;

  	spin_lock(&sc->rxbuflock);
  	if (list_empty(&sc->rxbuf)) {
@@ -1754,14 +1755,19 @@ accept:

  		/*
  		 * the hardware adds a padding to 4 byte boundaries between
-		 * the header and the payload data if the header length is
-		 * not multiples of 4 - remove it
+		 * the header and the payload data if the header length is not
+		 * multiple of 4 - remove it. This only affect data frames.
  		 */
-		hdrlen = ieee80211_get_hdrlen_from_skb(skb);
-		if (hdrlen & 3) {
-			pad = hdrlen % 4;
-			memmove(skb->data + pad, skb->data, hdrlen);
-			skb_pull(skb, pad);
+
+		hdr = (struct ieee80211_hdr *) skb;
+		if ((skb->len >= 2) && ieee80211_is_data(hdr->frame_control)) {
+
+			hdrlen = ieee80211_get_hdrlen_from_skb(skb);
+			if (hdrlen & 3) {
+				pad = hdrlen % 4;
+				memmove(skb->data + pad, skb->data, hdrlen);
+				skb_pull(skb, pad);
+			}
  		}

  		/*
@@ -2623,6 +2629,7 @@ ath5k_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
  	unsigned long flags;
  	int hdrlen;
  	int pad;
+	const struct ieee80211_hdr * hdr = (const struct ieee80211_hdr *)skb;

  	ath5k_debug_dump_skb(sc, skb, "TX  ", 1);

@@ -2630,19 +2637,25 @@ ath5k_tx(struct ieee80211_hw *hw, struct sk_buff
*skb)
  		ATH5K_DBG(sc, ATH5K_DEBUG_XMIT, "tx in monitor (scan?)\n");

  	/*
-	 * the hardware expects the header padded to 4 byte boundaries
-	 * if this is not the case we add the padding after the header
+	 * the hardware expects the header padded to 4 byte boundaries for
+	 * data frames, if this is not the case we add the padding after the
+	 * header
  	 */
-	hdrlen = ieee80211_get_hdrlen_from_skb(skb);
-	if (hdrlen & 3) {
-		pad = hdrlen % 4;
-		if (skb_headroom(skb) < pad) {
-			ATH5K_ERR(sc, "tx hdrlen not %%4: %d not enough"
-				" headroom to pad %d\n", hdrlen, pad);
-			return -1;
+
+	if ((skb->len >= 2) && ieee80211_is_data(hdr->frame_control)) {
+
+		hdrlen = ieee80211_get_hdrlen_from_skb(skb);
+		if (hdrlen & 3) {
+			pad = hdrlen % 4;
+			if (skb_headroom(skb) < pad) {
+				ATH5K_ERR(sc,
+					  "tx hdrlen not %%4: %d not enough"
+					  " headroom to pad %d\n", hdrlen, pad);
+				return -1;
+			}
+			skb_push(skb, pad);
+			memmove(skb->data, skb->data+pad, hdrlen);
  		}
-		skb_push(skb, pad);
-		memmove(skb->data, skb->data+pad, hdrlen);
  	}

  	spin_lock_irqsave(&sc->txbuflock, flags);
-- 1.5.6.5

Re: [PATCH] ath5k : Fix correct padding

[Johannes Berg]

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

On Thu, 2008-12-11 at 22:58 +0100, Michael Buesch wrote:
> On Thursday 11 December 2008 22:58:59 Benoit PAPILLAULT wrote:
> > Padding the 802.11 header to a multiple of 4 bytes needs to be done only
> > for DATA frames.
> 
> Uhm, can you explain _why_ it's only needed for data frames?

Well, it's only _needed_ for data frames because the alignment is
something the IP stack needs. That doesn't answer the question, of
course, the answer to that would have to be something like "because the
hardware uses the same algorithm to insert the padding"

johannes

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

Re: [PATCH] ath5k : Fix correct padding

[Johannes Berg]

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

On Thu, 2008-12-11 at 22:58 +0100, Benoit PAPILLAULT wrote:
> Padding the 802.11 header to a multiple of 4 bytes needs to be done only
> for DATA frames. This fixes a bug where 2 bytes were missing in monitor
> mode for ACK frames.

That's the wrong way around, you're not padding anything, you're
unpadding it.

Your patch is also line-wrapped.

johannes

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

Re: [PATCH] ath5k : Fix correct padding

[Harvey Harrison]

On Thu, 2008-12-11 at 22:58 +0100, Benoit PAPILLAULT wrote:
> Padding the 802.11 header to a multiple of 4 bytes needs to be done only
> for DATA frames. This fixes a bug where 2 bytes were missing in monitor
> mode for ACK frames.

Without commenting on whether or not this is needed

>   		/*
> @@ -2623,6 +2629,7 @@ ath5k_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
>   	unsigned long flags;
>   	int hdrlen;
>   	int pad;
> +	const struct ieee80211_hdr * hdr = (const struct ieee80211_hdr *)skb;

skb->data perhaps.

Harvey

Re: [PATCH] ath5k : Fix correct padding

[Johannes Berg]

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

On Thu, 2008-12-11 at 14:08 -0800, Harvey Harrison wrote:
> On Thu, 2008-12-11 at 22:58 +0100, Benoit PAPILLAULT wrote:
> > Padding the 802.11 header to a multiple of 4 bytes needs to be done only
> > for DATA frames. This fixes a bug where 2 bytes were missing in monitor
> > mode for ACK frames.
> 
> Without commenting on whether or not this is needed
> 
> >   		/*
> > @@ -2623,6 +2629,7 @@ ath5k_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
> >   	unsigned long flags;
> >   	int hdrlen;
> >   	int pad;
> > +	const struct ieee80211_hdr * hdr = (const struct ieee80211_hdr *)skb;
> 
> skb->data perhaps.

For sure, there's a second place like that too.

johannes

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

Re: [ath5k-devel] [PATCH] ath5k : Fix correct padding

[Bob Copeland]

2008/12/11 Johannes Berg <johannes@sipsolutions.net>:
> On Thu, 2008-12-11 at 22:58 +0100, Benoit PAPILLAULT wrote:
>> Padding the 802.11 header to a multiple of 4 bytes needs to be done only
>> for DATA frames. This fixes a bug where 2 bytes were missing in monitor
>> mode for ACK frames.
>
> That's the wrong way around, you're not padding anything, you're
> unpadding it.
>

Agreed, does the hardware really care for control frames if there's
a padding on the TX side?

Also, Patrick already posted a patch:

http://marc.info/?l=linux-wireless&m=122888546315200&w=2


-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH] ath5k : Fix correct padding

[Benoit PAPILLAULT]

Johannes Berg a =E9crit :
> On Thu, 2008-12-11 at 14:08 -0800, Harvey Harrison wrote:
>> On Thu, 2008-12-11 at 22:58 +0100, Benoit PAPILLAULT wrote:
>>> Padding the 802.11 header to a multiple of 4 bytes needs to be done=
 only
>>> for DATA frames. This fixes a bug where 2 bytes were missing in mon=
itor
>>> mode for ACK frames.
>> Without commenting on whether or not this is needed
>>
>>>   		/*
>>> @@ -2623,6 +2629,7 @@ ath5k_tx(struct ieee80211_hw *hw, struct sk_b=
uff *skb)
>>>   	unsigned long flags;
>>>   	int hdrlen;
>>>   	int pad;
>>> +	const struct ieee80211_hdr * hdr =3D (const struct ieee80211_hdr =
*)skb;
>> skb->data perhaps.
>=20
> For sure, there's a second place like that too.
>=20
> johannes

Nice catch! I wonder how it was working in my preliminary tests. Sorry=20
for the duplicates with the same patch for ath9k. The padding/unpadding=
=20
stuff is needed by the hardware, ie the hardware insert padding on RX=20
and expect padding on TX, as mentioned in earlier ath5k and madwifi=20
source code.

According to my tests, there are 4 kinds of frames:
- Management
- Control
- Data (including QoS data)
- Invalid

Management frames have a fixed header of 24 bytes, so are already a=20
multiple of 4. Control frames are either 10 (ACK!) or 16 bytes and it's=
=20
clear that none of them needs padding (hence the current bug). Data=20
frames can be 24, 26, 30 or 32 bytes so it clearly needs some=20
padding/unpadding in some cases. At last, we don't care about Invalid f=
rame.

The ath9k patch does padding/unpadding for hdrlen >=3D 24, which appear=
s=20
to be equivalent and simpler to compute. I will rewrite the patch using=
=20
the ath9k formula.

Regards,
Benoit
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ath5k-devel] [PATCH] ath5k : Fix correct padding

[Bob Copeland]

On Fri, Dec 12, 2008 at 3:49 AM, Benoit PAPILLAULT
<benoit.papillault@free.fr> wrote:
> Nice catch! I wonder how it was working in my preliminary tests. Sorry
> for the duplicates with the same patch for ath9k. The padding/unpadding
> stuff is needed by the hardware, ie the hardware insert padding on RX
> and expect padding on TX, as mentioned in earlier ath5k and madwifi
> source code.

I should clarify: I meant Patrick already posted a fix for ath5k (and
possibly the ensuing discussion led to the ath9k patch).  Anyway, let's
just get a patch in :)

The extra padding on TX side shouldn't hurt anything, OTOH, yeah it is
probably unnecessary to do it for ACK/CTS.

Also take a look at desc.c when setting up TX descriptor:

     frame_len = pkt_len - (hdr_len & 3) + FCS_LEN;

What we have now will be 10 - 2 + 4 = 12 vs what should probably be 14.

-- 
Bob Copeland %% www.bobcopeland.com