ath5k : Fix correct padding

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 :

---
 drivers/net/wireless/ath5k/base.c |   49
+++++++++++++++++++++++-------------
 1 files changed, 31 insertions(+), 18 deletions(-)

b/drivers/net/wireless/ath5k/base.c
index bfb0c49..2318b1c 100644

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

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

Greetings, Michael.

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

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

Your patch is also line-wrapped.

johannes

2008/12/11 Johannes Berg <[email protected]>:

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

Without commenting on whether or not this is needed

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

johannes

Johannes Berg a =E9crit :

only

itor

uff *skb)

*)skb;

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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