[PATCH 1/2] mac80211: Build TX radiotap header dynamically

[PATCH 1/2] mac80211: Build TX radiotap header dynamically

[Helmut Schaa]

Get rid of the ieee80211_tx_status_rtap_hdr struct and instead build the
rtap header dynamically. This makes it easier to extend the rtap header
generation in the future.

Add ieee80211_tx_radiotap_len to calculate the expected size of the
rtap header before generating it. Since we can't check if the rtap
header fits into the requested headroom during compile time anymore
add a WARN_ON_ONCE.

Also move the actual rtap header generation into its own function.

Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
---
 net/mac80211/ieee80211_i.h |   12 -----
 net/mac80211/main.c        |    6 +--
 net/mac80211/status.c      |  112 ++++++++++++++++++++++++++++++--------------
 3 files changed, 78 insertions(+), 52 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 9fa5f8a..6e56dc8 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1178,18 +1178,6 @@ netdev_tx_t ieee80211_monitor_start_xmit(struct sk_buff *skb,
 netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
 				       struct net_device *dev);
 
-/*
- * radiotap header for status frames
- */
-struct ieee80211_tx_status_rtap_hdr {
-	struct ieee80211_radiotap_header hdr;
-	u8 rate;
-	u8 padding_for_rate;
-	__le16 tx_flags;
-	u8 data_retries;
-} __packed;
-
-
 /* HT */
 void ieee80211_ht_cap_ie_to_sta_ht_cap(struct ieee80211_supported_band *sband,
 				       struct ieee80211_ht_cap *ht_cap_ie,
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 17b038a..d4ee6d2 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -904,12 +904,8 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
 	 * and we need some headroom for passing the frame to monitor
 	 * interfaces, but never both at the same time.
 	 */
-#ifndef __CHECKER__
-	BUILD_BUG_ON(IEEE80211_TX_STATUS_HEADROOM !=
-			sizeof(struct ieee80211_tx_status_rtap_hdr));
-#endif
 	local->tx_headroom = max_t(unsigned int , local->hw.extra_tx_headroom,
-				   sizeof(struct ieee80211_tx_status_rtap_hdr));
+				   IEEE80211_TX_STATUS_HEADROOM);
 
 	debugfs_hw_add(local);
 
diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index 864a9c3..a9dc7b7 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -228,6 +228,79 @@ static void ieee80211_set_bar_pending(struct sta_info *sta, u8 tid, u16 ssn)
 	tid_tx->bar_pending = true;
 }
 
+static int ieee80211_tx_radiotap_len(struct ieee80211_tx_info *info)
+{
+	int len = sizeof(struct ieee80211_radiotap_header);
+
+	/* IEEE80211_RADIOTAP_RATE rate */
+	if (info->status.rates[0].idx >= 0 &&
+	    !(info->status.rates[0].flags & IEEE80211_TX_RC_MCS))
+		len += 2;
+
+	/* IEEE80211_RADIOTAP_TX_FLAGS */
+	len += 2;
+
+	/* IEEE80211_RADIOTAP_DATA_RETRIES */
+	len += 1;
+	
+	return len;
+}
+
+static void ieee80211_add_tx_radiotap_header(struct ieee80211_supported_band
+					     *sband, struct sk_buff *skb,
+					     int retry_count, int rtap_len)
+{
+	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
+	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
+	struct ieee80211_radiotap_header *rthdr;
+	unsigned char *pos;
+	__le16 txflags;
+
+	rthdr = (struct ieee80211_radiotap_header *) skb_push(skb, rtap_len);
+
+	memset(rthdr, 0, rtap_len);
+	rthdr->it_len = cpu_to_le16(rtap_len);
+	rthdr->it_present =
+		cpu_to_le32((1 << IEEE80211_RADIOTAP_TX_FLAGS) |
+			    (1 << IEEE80211_RADIOTAP_DATA_RETRIES));
+	pos = (unsigned char *)(rthdr + 1);
+
+	/*
+	 * XXX: Once radiotap gets the bitmap reset thing the vendor
+	 *	extensions proposal contains, we can actually report
+	 *	the whole set of tries we did.
+	 */
+
+	/* IEEE80211_RADIOTAP_RATE */
+	if (info->status.rates[0].idx >= 0 &&
+	    !(info->status.rates[0].flags & IEEE80211_TX_RC_MCS)) {
+		rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_RATE);
+		*pos = sband->bitrates[info->status.rates[0].idx].bitrate / 5;
+		/* padding for tx flags */
+		pos += 2;
+	}
+
+	/* IEEE80211_RADIOTAP_TX_FLAGS */
+	txflags = 0;
+	if (!(info->flags & IEEE80211_TX_STAT_ACK) &&
+	    !is_multicast_ether_addr(hdr->addr1))
+		txflags |= cpu_to_le16(IEEE80211_RADIOTAP_F_TX_FAIL);
+
+	if ((info->status.rates[0].flags & IEEE80211_TX_RC_USE_RTS_CTS) ||
+	    (info->status.rates[0].flags & IEEE80211_TX_RC_USE_CTS_PROTECT))
+		txflags |= cpu_to_le16(IEEE80211_RADIOTAP_F_TX_CTS);
+	else if (info->status.rates[0].flags & IEEE80211_TX_RC_USE_RTS_CTS)
+		txflags |= cpu_to_le16(IEEE80211_RADIOTAP_F_TX_RTS);
+
+	put_unaligned_le16(txflags, pos);
+	pos += 2;
+
+	/* IEEE80211_RADIOTAP_DATA_RETRIES */
+	/* for now report the total retry_count */
+	*pos = retry_count;
+	pos++;
+}
+
 /*
  * Use a static threshold for now, best value to be determined
  * by testing ...
@@ -246,7 +319,6 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
 	u16 frag, type;
 	__le16 fc;
 	struct ieee80211_supported_band *sband;
-	struct ieee80211_tx_status_rtap_hdr *rthdr;
 	struct ieee80211_sub_if_data *sdata;
 	struct net_device *prev_dev = NULL;
 	struct sta_info *sta, *tmp;
@@ -256,6 +328,7 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
 	bool acked;
 	struct ieee80211_bar *bar;
 	u16 tid;
+	int rtap_len;
 
 	for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) {
 		if (info->status.rates[i].idx < 0) {
@@ -460,44 +533,13 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
 	}
 
 	/* send frame to monitor interfaces now */
-
-	if (skb_headroom(skb) < sizeof(*rthdr)) {
+	rtap_len = ieee80211_tx_radiotap_len(info);
+	if (WARN_ON_ONCE(skb_headroom(skb) < rtap_len)) {
 		printk(KERN_ERR "ieee80211_tx_status: headroom too small\n");
 		dev_kfree_skb(skb);
 		return;
 	}
-
-	rthdr = (struct ieee80211_tx_status_rtap_hdr *)
-				skb_push(skb, sizeof(*rthdr));
-
-	memset(rthdr, 0, sizeof(*rthdr));
-	rthdr->hdr.it_len = cpu_to_le16(sizeof(*rthdr));
-	rthdr->hdr.it_present =
-		cpu_to_le32((1 << IEEE80211_RADIOTAP_TX_FLAGS) |
-			    (1 << IEEE80211_RADIOTAP_DATA_RETRIES) |
-			    (1 << IEEE80211_RADIOTAP_RATE));
-
-	if (!(info->flags & IEEE80211_TX_STAT_ACK) &&
-	    !is_multicast_ether_addr(hdr->addr1))
-		rthdr->tx_flags |= cpu_to_le16(IEEE80211_RADIOTAP_F_TX_FAIL);
-
-	/*
-	 * XXX: Once radiotap gets the bitmap reset thing the vendor
-	 *	extensions proposal contains, we can actually report
-	 *	the whole set of tries we did.
-	 */
-	if ((info->status.rates[0].flags & IEEE80211_TX_RC_USE_RTS_CTS) ||
-	    (info->status.rates[0].flags & IEEE80211_TX_RC_USE_CTS_PROTECT))
-		rthdr->tx_flags |= cpu_to_le16(IEEE80211_RADIOTAP_F_TX_CTS);
-	else if (info->status.rates[0].flags & IEEE80211_TX_RC_USE_RTS_CTS)
-		rthdr->tx_flags |= cpu_to_le16(IEEE80211_RADIOTAP_F_TX_RTS);
-	if (info->status.rates[0].idx >= 0 &&
-	    !(info->status.rates[0].flags & IEEE80211_TX_RC_MCS))
-		rthdr->rate = sband->bitrates[
-				info->status.rates[0].idx].bitrate / 5;
-
-	/* for now report the total retry_count */
-	rthdr->data_retries = retry_count;
+	ieee80211_add_tx_radiotap_header(sband, skb, retry_count, rtap_len);
 
 	/* XXX: is this sufficient for BPF? */
 	skb_set_mac_header(skb, 0);
-- 
1.7.3.4

[PATCH 2/2] mac80211: Populate radiotap header with MCS info for TX frames

[Helmut Schaa]

mac80211 already filled in the MCS rate info for rx'ed frames but tx'ed
frames that are sent to a monitor interface during the status callback
lack this information.

Add the radiotap fields for MCS info to ieee80211_tx_status_rtap_hdr
and populate them when sending tx'ed frames to the monitors.

The needed headroom is only extended by one byte since we don't include
legacy rate information in the rtap header for HT frames.

Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
---
 include/net/mac80211.h |    2 +-
 net/mac80211/status.c  |   23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index cd108df..b50fc1c 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -2520,7 +2520,7 @@ static inline int ieee80211_sta_ps_transition_ni(struct ieee80211_sta *sta,
  * The TX headroom reserved by mac80211 for its own tx_status functions.
  * This is enough for the radiotap header.
  */
-#define IEEE80211_TX_STATUS_HEADROOM	13
+#define IEEE80211_TX_STATUS_HEADROOM	14
 
 /**
  * ieee80211_sta_set_buffered - inform mac80211 about driver-buffered frames
diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index a9dc7b7..4b49ca7 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -242,6 +242,11 @@ static int ieee80211_tx_radiotap_len(struct ieee80211_tx_info *info)
 
 	/* IEEE80211_RADIOTAP_DATA_RETRIES */
 	len += 1;
+
+	/* IEEE80211_TX_RC_MCS */
+	if (info->status.rates[0].idx >= 0 &&
+	    info->status.rates[0].flags & IEEE80211_TX_RC_MCS)
+		len += 3;
 	
 	return len;
 }
@@ -299,6 +304,24 @@ static void ieee80211_add_tx_radiotap_header(struct ieee80211_supported_band
 	/* for now report the total retry_count */
 	*pos = retry_count;
 	pos++;
+
+	/* IEEE80211_TX_RC_MCS */
+	if (info->status.rates[0].idx >= 0 &&
+	    info->status.rates[0].flags & IEEE80211_TX_RC_MCS) {
+		rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_MCS);
+		pos[0] = IEEE80211_RADIOTAP_MCS_HAVE_MCS |
+			 IEEE80211_RADIOTAP_MCS_HAVE_GI |
+			 IEEE80211_RADIOTAP_MCS_HAVE_BW;
+		if (info->status.rates[0].flags & IEEE80211_TX_RC_SHORT_GI)
+			pos[1] |= IEEE80211_RADIOTAP_MCS_SGI;
+		if (info->status.rates[0].flags & IEEE80211_TX_RC_40_MHZ_WIDTH)
+			pos[1] |= IEEE80211_RADIOTAP_MCS_BW_40;
+		if (info->status.rates[0].flags & IEEE80211_TX_RC_GREEN_FIELD)
+			pos[1] |= IEEE80211_RADIOTAP_MCS_FMT_GF;
+		pos[2] = info->status.rates[0].idx;
+		pos += 3;
+	}
+
 }
 
 /*
-- 
1.7.3.4

Re: [PATCH 1/2] mac80211: Build TX radiotap header dynamically

[Johannes Berg]

On Tue, 2011-10-11 at 18:08 +0200, Helmut Schaa wrote:

> +	/* IEEE80211_RADIOTAP_RATE */
> +	if (info->status.rates[0].idx >= 0 &&
> +	    !(info->status.rates[0].flags & IEEE80211_TX_RC_MCS)) {
> +		rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_RATE);
> +		*pos = sband->bitrates[info->status.rates[0].idx].bitrate / 5;
> +		/* padding for tx flags */
> +		pos += 2;

Since there's no over-all memset I think you need to clear the padding
explicitly now.

johannes

Re: Re: [PATCH 1/2] mac80211: Build TX radiotap header dynamically

[Helmut Schaa]

Am Dienstag, 11. Oktober 2011, 18:25:45 schrieb Johannes Berg:
> On Tue, 2011-10-11 at 18:08 +0200, Helmut Schaa wrote:
> Since there's no over-all memset I think you need to clear the padding
> explicitly now.

Like this one? :)

> +	rthdr = (struct ieee80211_radiotap_header *) skb_push(skb, rtap_len);
> +
> +	memset(rthdr, 0, rtap_len);

Helmut

Re: Re: [PATCH 1/2] mac80211: Build TX radiotap header dynamically

[Johannes Berg]

On Tue, 2011-10-11 at 19:19 +0200, Helmut Schaa wrote:
> Am Dienstag, 11. Oktober 2011, 18:25:45 schrieb Johannes Berg:
> > On Tue, 2011-10-11 at 18:08 +0200, Helmut Schaa wrote:
> > Since there's no over-all memset I think you need to clear the padding
> > explicitly now.
> 
> Like this one? :)
> 
> > +	rthdr = (struct ieee80211_radiotap_header *) skb_push(skb, rtap_len);
> > +
> > +	memset(rthdr, 0, rtap_len);

Oh, oops, sorry!

Looks good to me then.

johannes

Re: [PATCH 1/2] mac80211: Build TX radiotap header dynamically

[Johannes Berg]

On Tue, 2011-10-11 at 18:08 +0200, Helmut Schaa wrote:

> +	__le16 txflags;

> +	put_unaligned_le16(txflags, pos);

Please send a patch to fix this endian bug that sparse found.

johannes

[PATCH] mac80211: Fix endian bug in radiotap header generation

[Helmut Schaa]

I intoduced this bug in commit a2fe81667410723d941a688e1958a49d67ca3346
"mac80211: Build TX radiotap header dynamically"

Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
---
 net/mac80211/status.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index a9da6ee..46222ce 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -260,7 +260,7 @@ static void ieee80211_add_tx_radiotap_header(struct ieee80211_supported_band
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
 	struct ieee80211_radiotap_header *rthdr;
 	unsigned char *pos;
-	__le16 txflags;
+	u16 txflags;
 
 	rthdr = (struct ieee80211_radiotap_header *) skb_push(skb, rtap_len);
 
@@ -290,13 +290,13 @@ static void ieee80211_add_tx_radiotap_header(struct ieee80211_supported_band
 	txflags = 0;
 	if (!(info->flags & IEEE80211_TX_STAT_ACK) &&
 	    !is_multicast_ether_addr(hdr->addr1))
-		txflags |= cpu_to_le16(IEEE80211_RADIOTAP_F_TX_FAIL);
+		txflags |= IEEE80211_RADIOTAP_F_TX_FAIL;
 
 	if ((info->status.rates[0].flags & IEEE80211_TX_RC_USE_RTS_CTS) ||
 	    (info->status.rates[0].flags & IEEE80211_TX_RC_USE_CTS_PROTECT))
-		txflags |= cpu_to_le16(IEEE80211_RADIOTAP_F_TX_CTS);
+		txflags |= IEEE80211_RADIOTAP_F_TX_CTS;
 	else if (info->status.rates[0].flags & IEEE80211_TX_RC_USE_RTS_CTS)
-		txflags |= cpu_to_le16(IEEE80211_RADIOTAP_F_TX_RTS);
+		txflags |= IEEE80211_RADIOTAP_F_TX_RTS;
 
 	put_unaligned_le16(txflags, pos);
 	pos += 2;
-- 
1.7.3.4

Re: [PATCH] mac80211: Fix endian bug in radiotap header generation

[Johannes Berg]

On Fri, 2011-11-18 at 17:02 +0100, Helmut Schaa wrote:
> I intoduced this bug in commit a2fe81667410723d941a688e1958a49d67ca3346
> "mac80211: Build TX radiotap header dynamically"

Acked-by: Johannes Berg <johannes@sipsolutions.net>

> Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
> ---
>  net/mac80211/status.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/net/mac80211/status.c b/net/mac80211/status.c
> index a9da6ee..46222ce 100644
> --- a/net/mac80211/status.c
> +++ b/net/mac80211/status.c
> @@ -260,7 +260,7 @@ static void ieee80211_add_tx_radiotap_header(struct ieee80211_supported_band
>  	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
>  	struct ieee80211_radiotap_header *rthdr;
>  	unsigned char *pos;
> -	__le16 txflags;
> +	u16 txflags;
>  
>  	rthdr = (struct ieee80211_radiotap_header *) skb_push(skb, rtap_len);
>  
> @@ -290,13 +290,13 @@ static void ieee80211_add_tx_radiotap_header(struct ieee80211_supported_band
>  	txflags = 0;
>  	if (!(info->flags & IEEE80211_TX_STAT_ACK) &&
>  	    !is_multicast_ether_addr(hdr->addr1))
> -		txflags |= cpu_to_le16(IEEE80211_RADIOTAP_F_TX_FAIL);
> +		txflags |= IEEE80211_RADIOTAP_F_TX_FAIL;
>  
>  	if ((info->status.rates[0].flags & IEEE80211_TX_RC_USE_RTS_CTS) ||
>  	    (info->status.rates[0].flags & IEEE80211_TX_RC_USE_CTS_PROTECT))
> -		txflags |= cpu_to_le16(IEEE80211_RADIOTAP_F_TX_CTS);
> +		txflags |= IEEE80211_RADIOTAP_F_TX_CTS;
>  	else if (info->status.rates[0].flags & IEEE80211_TX_RC_USE_RTS_CTS)
> -		txflags |= cpu_to_le16(IEEE80211_RADIOTAP_F_TX_RTS);
> +		txflags |= IEEE80211_RADIOTAP_F_TX_RTS;
>  
>  	put_unaligned_le16(txflags, pos);
>  	pos += 2;

Heh I would've fixed it by just using put_unaligned(txflags, pos) here,
but either is fine with me :-)

johannes

Re: Re: [PATCH] mac80211: Fix endian bug in radiotap header generation

[Johannes Berg]

On Fri, 2011-11-18 at 21:56 +0100, Helmut Schaa wrote:
> Am Freitag, 18. November 2011, 17:08:40 schrieb Johannes Berg:
> > Heh I would've fixed it by just using put_unaligned(txflags, pos) here,
> > but either is fine with me :-)
> 
> Hmm, right, but using this approach we even save some cpu_to_le16 calls on
> BE archs

Not really, because the compiler will translate cpu_to_le16(const) to
swapped-const.

>  and maybe some day I'll learn to always build with "make C=1" to
> catch endian bugs before sending a patch :)

:-)

johannes

Re: Re: [PATCH] mac80211: Fix endian bug in radiotap header generation

[Helmut Schaa]

Am Freitag, 18. November 2011, 17:08:40 schrieb Johannes Berg:
> Heh I would've fixed it by just using put_unaligned(txflags, pos) here,
> but either is fine with me :-)

Hmm, right, but using this approach we even save some cpu_to_le16 calls on
BE archs and maybe some day I'll learn to always build with "make C=1" to
catch endian bugs before sending a patch :)

Thanks,
Helmut