mac80211: Set IEEE80211_TXPD_REQ_TX_STATUS for all TX frames

PATCH In review Ivo van Doorn 2 participants 7 messages May 11, 2008 – May 13, 2008 mac80211

Share & tools

Ivo van Doorn

author

2008-05-11 12:24 UTC #0 lore raw

All interfaces should set the IEEE80211_TXPD_REQ_TX_STATUS flag for all TX frames which will force the master interface to set the IEEE80211_TX_CTL_REQ_TX_STATUS flag. This in turn will allow drivers to check for that flag before reporting the TX status to mac80211.

This is very usefull when frames (like beacons, RTS and CTS-to-self) should not be reported back to mac80211. Later we could add more extensive checks to exclude more frames from being reported, or let mac80211 decide if it wants the frame for status reporting or not.

Signed-off-by Ivo van Doorn <[email protected]>

---

net/mac80211/tx.c +3 -0

index f35eaea..0024892 100644
@@ -1617,6 +1617,9 @@ int ieee80211_subif_start_xmit(struct sk_buff *skb,
 	if (ethertype == ETH_P_PAE)
 		pkt_data->flags |= IEEE80211_TXPD_EAPOL_FRAME;
 
+	/* Interfaces should always request a status report */
+	pkt_data->flags |= IEEE80211_TXPD_REQ_TX_STATUS;
+
 	skb->dev = local->mdev;
 	dev->stats.tx_packets++;
 	dev->stats.tx_bytes += skb->len;

Johannes Berg

maintainer

2008-05-13 08:43 UTC #1 lore raw

Quoted 17 lines · Ivo van Doorn

All interfaces should set the IEEE80211_TXPD_REQ_TX_STATUS flag for all TX frames which will force the master interface to set the IEEE80211_TX_CTL_REQ_TX_STATUS flag. This in turn will allow drivers to check for that flag before reporting the TX status to mac80211.
This is very usefull when frames (like beacons, RTS and CTS-to-self) should not be reported back to mac80211. Later we could add more extensive checks to exclude more frames from being reported, or let mac80211 decide if it wants the frame for status reporting or not.
Signed-off-by Ivo van Doorn <[email protected]>
---
net/mac80211/tx.c +0 -0
index f35eaea..0024892 100644

net/mac80211/tx.c int ieee80211_subif_start_xmit(struct sk_buff *skb, · line 1622

Quoted 6 lines

net/mac80211/tx.c +3 -0

@@ -1617,6 +1617,9 @@ int ieee80211_subif_start_xmit(struct sk_buff *skb,
 	if (ethertype == ETH_P_PAE)
 		pkt_data->flags |= IEEE80211_TXPD_EAPOL_FRAME;
 
+	/* Interfaces should always request a status report */
+	pkt_data->flags |= IEEE80211_TXPD_REQ_TX_STATUS;
+

What about monitor interfaces/injection? Do they already set it?

Other than that, looks good to me, as long as you're aware that in the future this might be more dynamic and don't rely on it too much for internal housekeeping (but you seem to be according to your changelog entry) :)

johannes

Ivo van Doorn

author

2008-05-13 11:35 UTC #2 lore raw

Quoted 21 lines · Johannes Berg

Quoted 20 lines · Ivo van Doorn
All interfaces should set the IEEE80211_TXPD_REQ_TX_STATUS flag for all TX frames which will force the master interface to set the IEEE80211_TX_CTL_REQ_TX_STATUS flag. This in turn will allow drivers to check for that flag before reporting the TX status to mac80211.
This is very usefull when frames (like beacons, RTS and CTS-to-self) should not be reported back to mac80211. Later we could add more extensive checks to exclude more frames from being reported, or let mac80211 decide if it wants the frame for status reporting or not.
Signed-off-by Ivo van Doorn <[email protected]>
---
net/mac80211/tx.c +0 -0
index f35eaea..0024892 100644
@@ -1617,6 +1617,9 @@ int ieee80211_subif_start_xmit(struct sk_buff *skb,
 	if (ethertype == ETH_P_PAE)
 		pkt_data->flags |= IEEE80211_TXPD_EAPOL_FRAME;
 

Quoted 5 lines

net/mac80211/tx.c int ieee80211_subif_start_xmit(struct sk_buff *skb, · line 1622
Quoted 3 lines
net/mac80211/tx.c +3 -0
+	/* Interfaces should always request a status report */
+	pkt_data->flags |= IEEE80211_TXPD_REQ_TX_STATUS;
+
What about monitor interfaces/injection? Do they already set it?

No, should they?

Quoted 4 lines

Other than that, looks good to me, as long as you're aware that in the future this might be more dynamic and don't rely on it too much for internal housekeeping (but you seem to be according to your changelog entry) :)

Well it shouldn't matter if it will become more flexible, what I am looking for is making the flag actually mean something for drivers. ;) We could even make it so that the mac80211 tx_status handler will ignore the frame when the flag is set so drivers won't have to look at it at all.

Ivo

Johannes Berg

maintainer

2008-05-13 12:30 UTC #3 lore raw

Quoted 4 lines

Quoted 2 lines
What about monitor interfaces/injection? Do they already set it?
No, should they?

Yes, I think so, since hostapd uses injection for frames and definitely needs status callback. We could later define a radiotap bit for that, of course.

Quoted 7 lines

Quoted 4 lines
Other than that, looks good to me, as long as you're aware that in the future this might be more dynamic and don't rely on it too much for internal housekeeping (but you seem to be according to your changelog entry) :)
Well it shouldn't matter if it will become more flexible, what I am looking for is making the flag actually mean something for drivers. ;)

Right :)

Quoted 2 lines

We could even make it so that the mac80211 tx_status handler will ignore the frame when the flag is set so drivers won't have to look at it at all.

You mean "when the flag is unset"? Not sure, maybe we should, but we'll see that when more uses for it come up/better rate control algorithms are developed.

johannes

Ivo van Doorn

author

2008-05-13 12:41 UTC #4 lore raw

Quoted 8 lines · Johannes Berg

Quoted 4 lines
Quoted 2 lines
What about monitor interfaces/injection? Do they already set it?
No, should they?
Yes, I think so, since hostapd uses injection for frames and definitely needs status callback. We could later define a radiotap bit for that, of course.

Ok, I'll update the patch.

Quoted 6 lines

Quoted 2 lines
We could even make it so that the mac80211 tx_status handler will ignore the frame when the flag is set so drivers won't have to look at it at all.
You mean "when the flag is unset"? Not sure, maybe we should, but we'll see that when more uses for it come up/better rate control algorithms are developed.

The most important frames the issue currently arises is with RTS and CTS-to-self frames. rt2x00 requests those frames from mac80211 and currently clears the IEEE80211_TXCTL_REQ_TX_STATUS flag to prevent it being send to mac80211 to not upset the rc-algorithm.

Ivo

Ivo van Doorn

author

2008-05-13 12:51 UTC #5 lore raw

PATCH v2 1/1 mac80211: Set IEEE80211_TXPD_REQ_TX_STATUS for all TX frames

In reply to Ivo van Doorn

v2: Monitor interfaces should also set IEEE80211_TXPD_REQ_TX_STATUS

Signed-off-by Ivo van Doorn <[email protected]>

---

net/mac80211/tx.c +5 -0

index f35eaea..472dab0 100644
@@ -1336,6 +1336,8 @@ int ieee80211_monitor_start_xmit(struct sk_buff *skb,
 	pkt_data->ifindex = dev->ifindex;
 
 	pkt_data->flags |= IEEE80211_TXPD_DO_NOT_ENCRYPT;
+	/* Interfaces should always request a status report */
+	pkt_data->flags |= IEEE80211_TXPD_REQ_TX_STATUS;
 
 	/*
 	 * fix up the pointers accounting for the radiotap

@@ -1617,6 +1619,9 @@ int ieee80211_subif_start_xmit(struct sk_buff *skb,
 	if (ethertype == ETH_P_PAE)
 		pkt_data->flags |= IEEE80211_TXPD_EAPOL_FRAME;
 
+	/* Interfaces should always request a status report */
+	pkt_data->flags |= IEEE80211_TXPD_REQ_TX_STATUS;
+
 	skb->dev = local->mdev;
 	dev->stats.tx_packets++;
 	dev->stats.tx_bytes += skb->len;

Johannes Berg

maintainer Acked-by on v2 1/1

2008-05-13 12:54 UTC #6 lore raw

Quoted 5 lines · Ivo van Doorn

All interfaces should set the IEEE80211_TXPD_REQ_TX_STATUS flag for all TX frames which will force the master interface to set the IEEE80211_TX_CTL_REQ_TX_STATUS flag. This in turn will allow drivers to check for that flag before reporting the TX status to mac80211.

Quoted 8 lines

This is very usefull when frames (like beacons, RTS and CTS-to-self) should not be reported back to mac80211. Later we could add more extensive checks to exclude more frames from being reported, or let mac80211 decide if it wants the frame for status reporting or not.
v2: Monitor interfaces should also set IEEE80211_TXPD_REQ_TX_STATUS
Signed-off-by Ivo van Doorn <[email protected]>

Looks good, thanks.

Acked-by Johannes Berg <[email protected]>

Quoted 25 lines

---

net/mac80211/tx.c +5 -0

index f35eaea..472dab0 100644
@@ -1336,6 +1336,8 @@ int ieee80211_monitor_start_xmit(struct sk_buff *skb,
 	pkt_data->ifindex = dev->ifindex;
 
 	pkt_data->flags |= IEEE80211_TXPD_DO_NOT_ENCRYPT;
+	/* Interfaces should always request a status report */
+	pkt_data->flags |= IEEE80211_TXPD_REQ_TX_STATUS;
 
 	/*
 	 * fix up the pointers accounting for the radiotap

@@ -1617,6 +1619,9 @@ int ieee80211_subif_start_xmit(struct sk_buff *skb,
 	if (ethertype == ETH_P_PAE)
 		pkt_data->flags |= IEEE80211_TXPD_EAPOL_FRAME;
 
+	/* Interfaces should always request a status report */
+	pkt_data->flags |= IEEE80211_TXPD_REQ_TX_STATUS;
+
 	skb->dev = local->mdev;
 	dev->stats.tx_packets++;
 	dev->stats.tx_bytes += skb->len;