Skip to contentSkip to conversation

mac80211: add private driver flags and ampdu length to tx info

RFC In review Felix Fietkau 4 participants 6 messages Nov 14, 2009

Subsystems (2)
mac80211 Qualcomm Atheros ATH9K wireless driver
lore
Share & tools
lore

Felix Fietkau

author
#0 lore raw

RFC 1/2 mac80211: add private driver flags and ampdu length to tx info

Some drivers may want to store a few flags of their own in the tx info. Also it is useful for 11n rate control to know the total number of aggregated frames in an AMPDU, not just the ones that were acked. Use two previously unused padding bytes to store those two values

Signed-off-by Felix Fietkau <[email protected]>

---

include/net/mac80211.h +3 -1
@@ -386,6 +386,7 @@ struct ieee80211_tx_rate {
  * @flags: transmit info flags, defined above
  * @band: the band to transmit on (use for checking for races)
  * @antenna_sel_tx: antenna to use, 0 for automatic diversity
+ * @driver_flags: flags for internal driver use
  * @pad: padding, ignore
  * @control: union for control data
  * @status: union for status data

@@ -404,7 +405,8 @@ struct ieee80211_tx_info {
 	u8 antenna_sel_tx;
 
 	/* 2 byte hole */
-	u8 pad[2];
+	u8 driver_flags;
+	u8 ampdu_len;
 
 	union {
 		struct {
author
#1 lore raw

RFC 2/2 ath9k: get rid of tx_info_priv

This patch removes the need for separately allocated private tx info data in ath9k and brings the driver one small step closer to using the mac80211 rate control API properly.

Signed-off-by Felix Fietkau <[email protected]>

---

drivers/net/wireless/ath/ath9k/common.h +1 -0
@@ -81,6 +81,7 @@ struct ath_buf {
 	u16 bf_flags;
 	struct ath_buf_state bf_state;
 	dma_addr_t bf_dmacontext;
+	struct ath_wiphy *aphy;
 };
 
 struct ath_atx_tid {
drivers/net/wireless/ath/ath9k/rc.h +6 -12
@@ -167,24 +167,18 @@ struct ath_rate_priv {
 	struct ath_rate_softc *asc;
 };
 
+#define ATH_TX_INFO_FRAME_TYPE_INTERNAL	(1 << 0)
+#define ATH_TX_INFO_FRAME_TYPE_PAUSE	(1 << 1)
+#define ATH_TX_INFO_UPDATE_RC		(1 << 2)
+#define ATH_TX_INFO_XRETRY		(1 << 3)
+#define ATH_TX_INFO_UNDERRUN		(1 << 4)
+
 enum ath9k_internal_frame_type {
 	ATH9K_NOT_INTERNAL,
 	ATH9K_INT_PAUSE,
 	ATH9K_INT_UNPAUSE
 };
 
-struct ath_tx_info_priv {
-	struct ath_wiphy *aphy;
-	struct ath_tx_status tx;
-	int n_frames;
-	int n_bad_frames;
-	bool update_rc;
-	enum ath9k_internal_frame_type frame_type;
-};
-
-#define ATH_TX_INFO_PRIV(tx_info) \
-	((struct ath_tx_info_priv *)((tx_info)->rate_driver_data[0]))
-
 void ath_rate_attach(struct ath_softc *sc);
 u8 ath_rate_findrateix(struct ath_softc *sc, u8 dot11_rate);
 int ath_rate_control_register(void);
drivers/net/wireless/ath/ath9k/virtual.c +2 -7
@@ -338,13 +338,11 @@ void ath9k_wiphy_chan_work(struct work_s
 void ath9k_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
 {
 	struct ath_wiphy *aphy = hw->priv;
-	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
 	struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb);
-	struct ath_tx_info_priv *tx_info_priv = ATH_TX_INFO_PRIV(tx_info);
 
-	if (tx_info_priv && tx_info_priv->frame_type == ATH9K_INT_PAUSE &&
+	if ((tx_info->driver_flags & ATH_TX_INFO_FRAME_TYPE_PAUSE) &&
 	    aphy->state == ATH_WIPHY_PAUSING) {
-		if (!(info->flags & IEEE80211_TX_STAT_ACK)) {
+		if (!(tx_info->flags & IEEE80211_TX_STAT_ACK)) {
 			printk(KERN_DEBUG "ath9k: %s: no ACK for pause "
 			       "frame\n", wiphy_name(hw->wiphy));
 			/*

@@ -363,9 +361,6 @@ void ath9k_tx_status(struct ieee80211_hw
 		}
 	}
 
-	kfree(tx_info_priv);
-	tx_info->rate_driver_data[0] = NULL;
-
 	dev_kfree_skb(skb);
 }
drivers/net/wireless/ath/ath9k/xmit.c +32 -38

165 lines not on this page · raw message

drivers/net/wireless/ath/ath9k/debug.c +7 -4
@@ -257,14 +257,17 @@ static const struct file_operations fops
 
 void ath_debug_stat_rc(struct ath_softc *sc, struct sk_buff *skb)
 {
-	struct ath_tx_info_priv *tx_info_priv = NULL;
 	struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb);
 	struct ieee80211_tx_rate *rates = tx_info->status.rates;
-	int final_ts_idx, idx;
+	int final_ts_idx = 0, idx, i;
 	struct ath_rc_stats *stats;
 
-	tx_info_priv = ATH_TX_INFO_PRIV(tx_info);
-	final_ts_idx = tx_info_priv->tx.ts_rateindex;
+	for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) {
+		if (!rates[i].count)
+			break;
+
+		final_ts_idx = i;
+	}
 	idx = rates[final_ts_idx].idx;
 	stats = &sc->debug.stats.rcstats[idx];
 	stats->success++;
drivers/net/wireless/ath/ath9k/rc.c +35 -36

171 lines not on this page · raw message

Reviewed-by on v1 2/2
#2 lore raw

Re: [RFC 2/2] ath9k: get rid of tx_info_priv

Quoted 5 lines · Felix Fietkau

This patch removes the need for separately allocated private tx info data in ath9k and brings the driver one small step closer to using the mac80211 rate control API properly.

Signed-off-by Felix Fietkau <[email protected]>

Thanks for doing this! Comments below.

Quoted 1 line
(quoted diff) +0 -0
@@ -1301,23 +1298,30 @@ static void ath_tx_status(void *priv, st

< ... >

Quoted 4 lines

       hdr = (struct ieee80211_hdr *)skb->data; fc = hdr->frame_control;
-       tx_info_priv = ATH_TX_INFO_PRIV(tx_info);
-       final_ts_idx = tx_info_priv->tx.ts_rateindex;

Quoted 9 lines

+       for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) {
+               struct ieee80211_tx_rate *rate = &tx_info->status.rates[i];
+               if (!rate->count)
+                       break;
+
+               final_ts_idx = i;
+               long_retry = rate->count - 1;
+       }

This seems to be the only trade off, doing a loop on tx complete but I think I'm willing to trade that for the kmalloc()/kfree() removal.

Reviewed-by Luis R. Rodriguez <[email protected]>

Luis

#3 lore raw

Re: [RFC 2/2] ath9k: get rid of tx_info_priv

Quoted 27 lines · Luis R. Rodriguez
Quoted 5 lines · Felix Fietkau

This patch removes the need for separately allocated private tx info data in ath9k and brings the driver one small step closer to using the mac80211 rate control API properly.

Signed-off-by Felix Fietkau <[email protected]>

Thanks for doing this! Comments below.

Quoted 1 line
(quoted diff) +0 -0
@@ -1301,23 +1298,30 @@ static void ath_tx_status(void *priv, st

< ... >

Quoted 4 lines

       hdr = (struct ieee80211_hdr *)skb->data; fc = hdr->frame_control;
-       tx_info_priv = ATH_TX_INFO_PRIV(tx_info);
-       final_ts_idx = tx_info_priv->tx.ts_rateindex;

Quoted 9 lines

+       for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) {
+               struct ieee80211_tx_rate *rate = &tx_info->status.rates[i];
+               if (!rate->count)
+                       break;
+
+               final_ts_idx = i;
+               long_retry = rate->count - 1;
+       }

Quoted 4 lines

This seems to be the only trade off, doing a loop on tx complete but I think I'm willing to trade that for the kmalloc()/kfree() removal.

Reviewed-by Luis R. Rodriguez <[email protected]>

Oh and can you add an unlikely for this:

- else
+ if (tx_info->driver_flags & ATH_TX_INFO_FRAME_TYPE_INTERNAL)

                ath9k_tx_status(hw, skb);

+ else
+ ieee80211_tx_status(hw, skb);
}

Thanks!

Luis

maintainer
#4 lore raw
In reply to Felix Fietkau
Quoted 4 lines · Felix Fietkau

Some drivers may want to store a few flags of their own in the tx info. Also it is useful for 11n rate control to know the total number of aggregated frames in an AMPDU, not just the ones that were acked. Use two previously unused padding bytes to store those two values

Can the ampdu_len not be part of status info? There's lots more space there.

johannes

#5 lore raw
In reply to Felix Fietkau

Hi,

Quoted 19 lines · Felix Fietkau

Some drivers may want to store a few flags of their own in the tx info. Also it is useful for 11n rate control to know the total number of aggregated frames in an AMPDU, not just the ones that were acked. Use two previously unused padding bytes to store those two values

Signed-off-by Felix Fietkau <[email protected]>

---

include/net/mac80211.h +1 -0
@@ -386,6 +386,7 @@ struct ieee80211_tx_rate {
  * @flags: transmit info flags, defined above
  * @band: the band to transmit on (use for checking for races)
  * @antenna_sel_tx: antenna to use, 0 for automatic diversity
+ * @driver_flags: flags for internal driver use
  * @pad: padding, ignore
  * @control: union for control data
  * @status: union for status data

include/net/mac80211.h struct ieee80211_tx_info { · line 412

Quoted 9 lines
include/net/mac80211.h +2 -1
@@ -404,7 +405,8 @@ struct ieee80211_tx_info {
        u8 antenna_sel_tx;
 
        /* 2 byte hole */
-       u8 pad[2];
+       u8 driver_flags;
+       u8 ampdu_len;
 
        union {
                struct {

You have missed something in the comment above the structure, you've added the fields "driver_flags" and "ampdu_len" in the structure and removed the "pad" field. But in the comment you only add "driver_flags" and kept the "pad" field.

Thanks,

Ivo