[PATCH v2 0/2] mac80211: extend current rate control tx status API

[PATCH v2 0/2] mac80211: extend current rate control tx status API

[Jonas Jelonek]

This patch series extends the current rate control tx status API.
ieee80211_tx_info has a fixed limit in size (SKB_CB) and its ieee80211_tx_rate
is not suitable to annotate e.g. the mcs rate set from IEEE 802.11ax nor
additional per packet information like tx-power.

The commit 18fb84d986b3 introduced the extended on-stack struct
ieee80211_tx_status, which makes the tx status API more extensible.

With this patch we introduce a new struct ieee80211_rate_status that extends
current rate control tx status API further, in order to achieve:
(1)     receive tx power status feedback for transmit power control per
        packet or packet retry
(2)     dynamic mapping of wifi chip specific multi-rate retry (mrr) chains
        with different lengths
(3)     increase the limit of annotatable rate indices to support
        IEEE802.11ax rate sets and beyond

(1) cannot be achieved with the use of struct ieee80211_tx_info because both
control and status buffer have a fixed SKB_CB size limit. E.g. the current 
control buffer size would only allow tx-power annotations per packet,
not per mrr chain. The status buffer has no free space left. Struct
ieee80211_tx_status is intended to add such extensions.

(2) is motivated by the varying length of mrr chains in common wireless
hardware supported by Linux. E.g. Atheros chipsets support four mrr chain
stages, mediatek chips vary from a single mrr chain stage up to 8 stages, all
with specific restrictions in rate configuration, retry count and tx-power
control ability.
Currently the number of mrr chain stages is fixed to 4 (defined by
IEEE80211_TX_MAX_RATES). Although this value could be increased, a change
would affect all wireless drivers and probably cause additional problems.
Therefore we introduce a dynamic-sized solution that supports different
numbers of mrr chain stages hence a dynamic allocation of chain stages
supported by different wifi chipsets.

(3) The current struct ieee80211_tx_info uses a s8 integer for rate idx,
which would be to less for e.g. IEEE802.11ax rate annotations. To overcome
this limitation, we introduce struct rate_info from cfg80211.h. This struct
is not limited to annotate rates hence addressing rates up to 802.11ax and
also future rate sets are usable.

Our new struct is intended for all information related to RC and TPC that
needs to be passed from driver to mac80211 and its RC/TPC algorithms like
Minstrel_HT. Multiple instances of this struct can be included in struct
ieee80211_tx_status via a pointer and a length variable. Those can be
allocated on-stack. The former reference to a single instance of struct
rate_info is replaced with our new annotation.

Compile-Tested: current wireless-next tree with all flags on
Tested-on: Xiaomi 4A Gigabit (MediaTek MT7603E, MT7612E) with OpenWrt
                Linux 5.10.83

---
v2: Fixed some typos and a missing ! in changes in status.c
---

Jonas Jelonek (2):
  mac80211: extend current rate control tx status API
  mac80211: minstrel_ht: support ieee80211_rate_status

 drivers/net/wireless/mediatek/mt76/tx.c |  13 ++-
 include/net/mac80211.h                  |  10 +-
 net/mac80211/rc80211_minstrel_ht.c      | 131 ++++++++++++++++++++++--
 net/mac80211/rc80211_minstrel_ht.h      |   2 +-
 net/mac80211/status.c                   |  91 +++++++++-------
 5 files changed, 195 insertions(+), 52 deletions(-)

-- 
2.30.2

[PATCH v2 1/2] mac80211: extend current rate control tx status API

[Jonas Jelonek]

This patch adds the new struct ieee80211_rate_status and replaces
'struct rate_info *rate' in ieee80211_tx_status with pointer and length
annotation.

The struct ieee80211_rate_status allows to:
(1)	receive tx power status feedback for transmit power control (TPC)
	per packet or packet retry
(2)	dynamic mapping of wifi chip specific multi-rate retry (mrr)
	chains with different lengths
(3)	increase the limit of annotatable rate indices to support
	IEEE802.11ac rate sets and beyond

ieee80211_tx_info, control and status buffer, and ieee80211_tx_rate
cannot be used to achieve these goals due to fixed size limitations.

Our new struct contains a struct rate_info to annotate the rate that was
used, retry count of the rate and tx power. It is intended for all
information related to RC and TPC that needs to be passed from driver to
mac80211 and its RC/TPC algorithms like Minstrel_HT. It corresponds to
one stage in an mrr. Multiple subsequent instances of this struct can be
included in struct ieee80211_tx_status via a pointer and a length variable.
Those instances can be allocated on-stack. The former reference to a single
instance of struct rate_info is replaced with our new annotation.

Further mandatory changes in status.c and mt76 driver due to the
removal of 'struct rate_info *rate' are also included.
status.c already uses the information in ieee80211_tx_status->rate in
radiotap, this is now changed to use ieee80211_rate_status->rate_idx.
mt76 driver already uses struct rate_info to pass the tx rate to status
path. It is now enclosed in an instance of struct ieee80211_rate_status
with default values for retry_count and tx_power. The latter should be
adjusted later to pass more accurate values.

Compile-Tested: current wireless-next tree with all flags on
Tested-on: Xiaomi 4A Gigabit (MediaTek MT7603E, MT7612E) with OpenWrt
		Linux 5.10.83

Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
---
 drivers/net/wireless/mediatek/mt76/tx.c | 13 +++-
 include/net/mac80211.h                  | 10 ++-
 net/mac80211/status.c                   | 91 ++++++++++++++-----------
 3 files changed, 71 insertions(+), 43 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/tx.c b/drivers/net/wireless/mediatek/mt76/tx.c
index 6b8c9dc80542..ed3f3654999f 100644
--- a/drivers/net/wireless/mediatek/mt76/tx.c
+++ b/drivers/net/wireless/mediatek/mt76/tx.c
@@ -62,13 +62,20 @@ mt76_tx_status_unlock(struct mt76_dev *dev, struct sk_buff_head *list)
 		};
 		struct mt76_tx_cb *cb = mt76_tx_skb_cb(skb);
 		struct mt76_wcid *wcid;
+		struct ieee80211_rate_status rate = {0};
 
 		wcid = rcu_dereference(dev->wcid[cb->wcid]);
 		if (wcid) {
 			status.sta = wcid_to_sta(wcid);
-
-			if (status.sta)
-				status.rate = &wcid->rate;
+			if (status.sta) {
+				rate.rate_idx = wcid->rate;
+				rate.retry_count = 1;
+				/* Default 0 for now, can be used by TPC algorithm */
+				rate.tx_power = 0;
+
+				status.rates = &rate;
+				status.n_rates = 1;
+			}
 		}
 
 		hw = mt76_tx_status_get_hw(dev, skb);
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index c50221d7e82c..1e98ed04b446 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1131,6 +1131,12 @@ ieee80211_info_get_tx_time_est(struct ieee80211_tx_info *info)
 	return info->tx_time_est << 2;
 }
 
+struct ieee80211_rate_status {
+	struct rate_info rate_idx;
+	u8 retry_count;
+	s8 tx_power;
+};
+
 /**
  * struct ieee80211_tx_status - extended tx status info for rate control
  *
@@ -1144,7 +1150,9 @@ struct ieee80211_tx_status {
 	struct ieee80211_sta *sta;
 	struct ieee80211_tx_info *info;
 	struct sk_buff *skb;
-	struct rate_info *rate;
+	struct ieee80211_rate_status *rates;
+	u8 n_rates;
+
 	struct list_head *free_list;
 };
 
diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index f6f63a0b1b72..23c4c35182a5 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -246,15 +246,19 @@ static void ieee80211_set_bar_pending(struct sta_info *sta, u8 tid, u16 ssn)
 static int ieee80211_tx_radiotap_len(struct ieee80211_tx_info *info,
 				     struct ieee80211_tx_status *status)
 {
+	struct ieee80211_rate_status *status_rate = NULL;
 	int len = sizeof(struct ieee80211_radiotap_header);
 
+	if (status && status->rates)
+		status_rate = &status->rates[status->n_rates - 1];
+
 	/* IEEE80211_RADIOTAP_RATE rate */
-	if (status && status->rate && !(status->rate->flags &
-					(RATE_INFO_FLAGS_MCS |
-					 RATE_INFO_FLAGS_DMG |
-					 RATE_INFO_FLAGS_EDMG |
-					 RATE_INFO_FLAGS_VHT_MCS |
-					 RATE_INFO_FLAGS_HE_MCS)))
+	if (status_rate && !(status_rate->rate_idx.flags &
+				(RATE_INFO_FLAGS_MCS |
+	       			 RATE_INFO_FLAGS_DMG |
+				 RATE_INFO_FLAGS_EDMG |
+				 RATE_INFO_FLAGS_VHT_MCS |
+				 RATE_INFO_FLAGS_HE_MCS)))
 		len += 2;
 	else if (info->status.rates[0].idx >= 0 &&
 		 !(info->status.rates[0].flags &
@@ -269,12 +273,12 @@ static int ieee80211_tx_radiotap_len(struct ieee80211_tx_info *info,
 
 	/* IEEE80211_RADIOTAP_MCS
 	 * IEEE80211_RADIOTAP_VHT */
-	if (status && status->rate) {
-		if (status->rate->flags & RATE_INFO_FLAGS_MCS)
+	if (status_rate) {
+		if (status_rate->rate_idx.flags & RATE_INFO_FLAGS_MCS)
 			len += 3;
-		else if (status->rate->flags & RATE_INFO_FLAGS_VHT_MCS)
+		else if (status_rate->rate_idx.flags & RATE_INFO_FLAGS_VHT_MCS)
 			len = ALIGN(len, 2) + 12;
-		else if (status->rate->flags & RATE_INFO_FLAGS_HE_MCS)
+		else if (status_rate->rate_idx.flags & RATE_INFO_FLAGS_HE_MCS)
 			len = ALIGN(len, 2) + 12;
 	} else if (info->status.rates[0].idx >= 0) {
 		if (info->status.rates[0].flags & IEEE80211_TX_RC_MCS)
@@ -296,10 +300,14 @@ ieee80211_add_tx_radiotap_header(struct ieee80211_local *local,
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
 	struct ieee80211_radiotap_header *rthdr;
+	struct ieee80211_rate_status *status_rate = NULL;
 	unsigned char *pos;
 	u16 legacy_rate = 0;
 	u16 txflags;
 
+	if (status && status->rates)
+		status_rate = &status->rates[status->n_rates - 1];
+
 	rthdr = skb_push(skb, rtap_len);
 
 	memset(rthdr, 0, rtap_len);
@@ -317,13 +325,14 @@ ieee80211_add_tx_radiotap_header(struct ieee80211_local *local,
 
 	/* IEEE80211_RADIOTAP_RATE */
 
-	if (status && status->rate) {
-		if (!(status->rate->flags & (RATE_INFO_FLAGS_MCS |
-					     RATE_INFO_FLAGS_DMG |
-					     RATE_INFO_FLAGS_EDMG |
-					     RATE_INFO_FLAGS_VHT_MCS |
-					     RATE_INFO_FLAGS_HE_MCS)))
-			legacy_rate = status->rate->legacy;
+	if (status_rate) {
+		if (!(status_rate->rate_idx.flags &
+		      			(RATE_INFO_FLAGS_MCS |
+		       		 	 RATE_INFO_FLAGS_DMG |
+		       		 	 RATE_INFO_FLAGS_EDMG |
+		       			 RATE_INFO_FLAGS_VHT_MCS |
+		       			 RATE_INFO_FLAGS_HE_MCS)))
+			legacy_rate = status_rate->rate_idx.legacy;
 	} else if (info->status.rates[0].idx >= 0 &&
 		 !(info->status.rates[0].flags & (IEEE80211_TX_RC_MCS |
 						  IEEE80211_TX_RC_VHT_MCS)))
@@ -356,20 +365,22 @@ ieee80211_add_tx_radiotap_header(struct ieee80211_local *local,
 	*pos = retry_count;
 	pos++;
 
-	if (status && status->rate &&
-	    (status->rate->flags & RATE_INFO_FLAGS_MCS)) {
+	if (status_rate && (status_rate->rate_idx.flags &
+				RATE_INFO_FLAGS_MCS))
+	{
 		rthdr->it_present |= cpu_to_le32(BIT(IEEE80211_RADIOTAP_MCS));
 		pos[0] = IEEE80211_RADIOTAP_MCS_HAVE_MCS |
 			 IEEE80211_RADIOTAP_MCS_HAVE_GI |
 			 IEEE80211_RADIOTAP_MCS_HAVE_BW;
-		if (status->rate->flags & RATE_INFO_FLAGS_SHORT_GI)
+		if (status_rate->rate_idx.flags & RATE_INFO_FLAGS_SHORT_GI)
 			pos[1] |= IEEE80211_RADIOTAP_MCS_SGI;
-		if (status->rate->bw == RATE_INFO_BW_40)
+		if (status_rate->rate_idx.bw == RATE_INFO_BW_40)
 			pos[1] |= IEEE80211_RADIOTAP_MCS_BW_40;
-		pos[2] = status->rate->mcs;
+		pos[2] = status_rate->rate_idx.mcs;
 		pos += 3;
-	} else if (status && status->rate &&
-		   (status->rate->flags & RATE_INFO_FLAGS_VHT_MCS)) {
+	} else if (status_rate && (status_rate->rate_idx.flags &
+					RATE_INFO_FLAGS_VHT_MCS))
+	{
 		u16 known = local->hw.radiotap_vht_details &
 			(IEEE80211_RADIOTAP_VHT_KNOWN_GI |
 			 IEEE80211_RADIOTAP_VHT_KNOWN_BANDWIDTH);
@@ -384,12 +395,12 @@ ieee80211_add_tx_radiotap_header(struct ieee80211_local *local,
 		pos += 2;
 
 		/* u8 flags - IEEE80211_RADIOTAP_VHT_FLAG_* */
-		if (status->rate->flags & RATE_INFO_FLAGS_SHORT_GI)
+		if (status_rate->rate_idx.flags & RATE_INFO_FLAGS_SHORT_GI)
 			*pos |= IEEE80211_RADIOTAP_VHT_FLAG_SGI;
 		pos++;
 
 		/* u8 bandwidth */
-		switch (status->rate->bw) {
+		switch (status_rate->rate_idx.bw) {
 		case RATE_INFO_BW_160:
 			*pos = 11;
 			break;
@@ -406,7 +417,8 @@ ieee80211_add_tx_radiotap_header(struct ieee80211_local *local,
 		pos++;
 
 		/* u8 mcs_nss[4] */
-		*pos = (status->rate->mcs << 4) | status->rate->nss;
+		*pos = (status_rate->rate_idx.mcs << 4) |
+				status_rate->rate_idx.nss;
 		pos += 4;
 
 		/* u8 coding */
@@ -415,8 +427,9 @@ ieee80211_add_tx_radiotap_header(struct ieee80211_local *local,
 		pos++;
 		/* u16 partial_aid */
 		pos += 2;
-	} else if (status && status->rate &&
-		   (status->rate->flags & RATE_INFO_FLAGS_HE_MCS)) {
+	} else if (status_rate && (status_rate->rate_idx.flags &
+					RATE_INFO_FLAGS_HE_MCS))
+	{
 		struct ieee80211_radiotap_he *he;
 
 		rthdr->it_present |= cpu_to_le32(BIT(IEEE80211_RADIOTAP_HE));
@@ -434,7 +447,7 @@ ieee80211_add_tx_radiotap_header(struct ieee80211_local *local,
 
 #define HE_PREP(f, val) le16_encode_bits(val, IEEE80211_RADIOTAP_HE_##f)
 
-		he->data6 |= HE_PREP(DATA6_NSTS, status->rate->nss);
+		he->data6 |= HE_PREP(DATA6_NSTS, status_rate->rate_idx.nss);
 
 #define CHECK_GI(s) \
 	BUILD_BUG_ON(IEEE80211_RADIOTAP_HE_DATA5_GI_##s != \
@@ -444,12 +457,12 @@ ieee80211_add_tx_radiotap_header(struct ieee80211_local *local,
 		CHECK_GI(1_6);
 		CHECK_GI(3_2);
 
-		he->data3 |= HE_PREP(DATA3_DATA_MCS, status->rate->mcs);
-		he->data3 |= HE_PREP(DATA3_DATA_DCM, status->rate->he_dcm);
+		he->data3 |= HE_PREP(DATA3_DATA_MCS, status_rate->rate_idx.mcs);
+		he->data3 |= HE_PREP(DATA3_DATA_DCM, status_rate->rate_idx.he_dcm);
 
-		he->data5 |= HE_PREP(DATA5_GI, status->rate->he_gi);
+		he->data5 |= HE_PREP(DATA5_GI, status_rate->rate_idx.he_gi);
 
-		switch (status->rate->bw) {
+		switch (status_rate->rate_idx.bw) {
 		case RATE_INFO_BW_20:
 			he->data5 |= HE_PREP(DATA5_DATA_BW_RU_ALLOC,
 					     IEEE80211_RADIOTAP_HE_DATA5_DATA_BW_RU_ALLOC_20MHZ);
@@ -480,16 +493,16 @@ ieee80211_add_tx_radiotap_header(struct ieee80211_local *local,
 			CHECK_RU_ALLOC(2x996);
 
 			he->data5 |= HE_PREP(DATA5_DATA_BW_RU_ALLOC,
-					     status->rate->he_ru_alloc + 4);
+					     status_rate->rate_idx.he_ru_alloc + 4);
 			break;
 		default:
-			WARN_ONCE(1, "Invalid SU BW %d\n", status->rate->bw);
+			WARN_ONCE(1, "Invalid SU BW %d\n", status_rate->rate_idx.bw);
 		}
 
 		pos += sizeof(struct ieee80211_radiotap_he);
 	}
 
-	if ((status && status->rate) || info->status.rates[0].idx < 0)
+	if (status_rate || info->status.rates[0].idx < 0)
 		return;
 
 	/* IEEE80211_RADIOTAP_MCS
@@ -1108,8 +1121,8 @@ void ieee80211_tx_status_ext(struct ieee80211_hw *hw,
 	if (pubsta) {
 		sta = container_of(pubsta, struct sta_info, sta);
 
-		if (status->rate)
-			sta->tx_stats.last_rate_info = *status->rate;
+		if (status->rates)
+			sta->tx_stats.last_rate_info = status->rates[0].rate_idx;
 	}
 
 	if (skb && (tx_time_est =
-- 
2.30.2

[PATCH v2 2/2] mac80211: minstrel_ht: support ieee80211_rate_status

[Jonas Jelonek]

This patch adds support for the new struct ieee80211_rate_status and its
annotation in struct ieee80211_tx_status in minstrel_ht.

In minstrel_ht_tx_status, a check for the presence of instances of the
new struct in ieee80211_tx_status is added. Based on this, minstrel_ht
then gets and updates internal rate stats with either struct
ieee80211_rate_status or ieee80211_tx_info->status.rates.
Adjusted variants of minstrel_ht_txstat_valid, minstrel_ht_get_stats,
minstrel_{ht/vht}_get_group_idx are added which use struct
ieee80211_rate_status and struct rate_info instead of the legacy structs.

struct rate_info from cfg80211.h does not provide whether short preamble
was used for the transmission. So we retrieve this information from VIF
and STA configuration and cache it in a new flag in struct minstrel_ht_sta
per rate control instance.

Compile-Tested: current wireless-next tree with all flags on
Tested-on: Xiaomi 4A Gigabit (MediaTek MT7603E, MT7612E) with OpenWrt
                Linux 5.10.83

Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
---
 net/mac80211/rc80211_minstrel_ht.c | 131 +++++++++++++++++++++++++++--
 net/mac80211/rc80211_minstrel_ht.h |   2 +-
 2 files changed, 124 insertions(+), 9 deletions(-)

diff --git a/net/mac80211/rc80211_minstrel_ht.c b/net/mac80211/rc80211_minstrel_ht.c
index 9c3b7fc377c1..68e3a972c2fc 100644
--- a/net/mac80211/rc80211_minstrel_ht.c
+++ b/net/mac80211/rc80211_minstrel_ht.c
@@ -333,6 +333,16 @@ minstrel_ht_get_group_idx(struct ieee80211_tx_rate *rate)
 			 !!(rate->flags & IEEE80211_TX_RC_40_MHZ_WIDTH));
 }
 
+/*
+ * Look up an MCS group index based on new cfg80211 rate_info.
+ */
+static int
+minstrel_ht_ri_get_group_idx(struct rate_info *rate) {
+	return GROUP_IDX((rate->mcs / 8) + 1,
+			 !!(rate->flags & RATE_INFO_FLAGS_SHORT_GI),
+			 !!(rate->bw & RATE_INFO_BW_40));
+}
+
 static int
 minstrel_vht_get_group_idx(struct ieee80211_tx_rate *rate)
 {
@@ -342,6 +352,17 @@ minstrel_vht_get_group_idx(struct ieee80211_tx_rate *rate)
 			     2*!!(rate->flags & IEEE80211_TX_RC_80_MHZ_WIDTH));
 }
 
+/*
+ * Look up an MCS group index based on new cfg80211 rate_info.
+ */
+static int
+minstrel_vht_ri_get_group_idx(struct rate_info *rate) {
+	return VHT_GROUP_IDX(rate->nss,
+			     !!(rate->flags & RATE_INFO_FLAGS_SHORT_GI),
+			     !!(rate->bw & RATE_INFO_BW_40) +
+			     2*!!(rate->bw & RATE_INFO_BW_80));
+}
+
 static struct minstrel_rate_stats *
 minstrel_ht_get_stats(struct minstrel_priv *mp, struct minstrel_ht_sta *mi,
 		      struct ieee80211_tx_rate *rate)
@@ -382,6 +403,49 @@ minstrel_ht_get_stats(struct minstrel_priv *mp, struct minstrel_ht_sta *mi,
 	return &mi->groups[group].rates[idx];
 }
 
+/*
+ * Get the minstrel rate statistics for specified STA and rate info.
+ */
+static struct minstrel_rate_stats *
+minstrel_ht_ri_get_stats(struct minstrel_priv *mp, struct minstrel_ht_sta *mi,
+			  struct ieee80211_rate_status *rate_status)
+{
+	int group, idx;
+	struct rate_info *rate = &rate_status->rate_idx;
+
+	if (rate->flags & RATE_INFO_FLAGS_MCS) {
+		group = minstrel_ht_ri_get_group_idx(rate);
+		idx = rate->mcs % 8;
+		goto out;
+	}
+
+	if (rate->flags & RATE_INFO_FLAGS_VHT_MCS) {
+		group = minstrel_vht_ri_get_group_idx(rate);
+		idx = rate->mcs;
+		goto out;
+	}
+
+	group = MINSTREL_CCK_GROUP;
+	for (idx = 0; idx < ARRAY_SIZE(mp->cck_rates); idx++) {
+		if (rate->legacy != minstrel_cck_bitrates[ mp->cck_rates[idx] ])
+			continue;
+
+		/* short preamble */
+		if ((mi->supported[group] & BIT(idx + 4)) && mi->use_short_preamble)
+			idx += 4;
+		goto out;
+	}
+
+	group = MINSTREL_OFDM_GROUP;
+	for (idx = 0; idx < ARRAY_SIZE(mp->ofdm_rates[0]); idx++)
+		if (rate->legacy == minstrel_ofdm_bitrates[ mp->ofdm_rates[mi->band][idx] ])
+			goto out;
+
+	idx = 0;
+out:
+	return &mi->groups[group].rates[idx];
+}
+
 static inline struct minstrel_rate_stats *
 minstrel_get_ratestats(struct minstrel_ht_sta *mi, int index)
 {
@@ -1149,6 +1213,37 @@ minstrel_ht_txstat_valid(struct minstrel_priv *mp, struct minstrel_ht_sta *mi,
 	return false;
 }
 
+/*
+ * Check whether rate_status contains valid information.
+ */
+static bool
+minstrel_ht_ri_txstat_valid(struct minstrel_priv *mp, struct minstrel_ht_sta *mi,
+			     struct ieee80211_rate_status *rate_status)
+{
+	int i;
+
+	if (!rate_status)
+		return false;
+	if (!rate_status->retry_count)
+		return false;
+
+	if (rate_status->rate_idx.flags & RATE_INFO_FLAGS_MCS ||
+	    rate_status->rate_idx.flags & RATE_INFO_FLAGS_VHT_MCS)
+		return true;
+
+	for (i = 0; i < ARRAY_SIZE(mp->cck_rates); i++) {
+		if (rate_status->rate_idx.legacy == minstrel_cck_bitrates[ mp->cck_rates[i] ])
+			return true;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(mp->ofdm_rates); i++) {
+		if (rate_status->rate_idx.legacy == minstrel_ofdm_bitrates[ mp->ofdm_rates[mi->band][i] ])
+			return true;
+	}
+
+	return false;
+}
+
 static void
 minstrel_downgrade_rate(struct minstrel_ht_sta *mi, u16 *idx, bool primary)
 {
@@ -1214,16 +1309,31 @@ minstrel_ht_tx_status(void *priv, struct ieee80211_supported_band *sband,
 	mi->ampdu_packets++;
 	mi->ampdu_len += info->status.ampdu_len;
 
-	last = !minstrel_ht_txstat_valid(mp, mi, &ar[0]);
-	for (i = 0; !last; i++) {
-		last = (i == IEEE80211_TX_MAX_RATES - 1) ||
-		       !minstrel_ht_txstat_valid(mp, mi, &ar[i + 1]);
+	if (unlikely(st->rates && st->n_rates)) {
+		last = !minstrel_ht_ri_txstat_valid(mp, mi, &(st->rates[0]));
+		for (i = 0; !last; i++) {
+			last = (i == st->n_rates - 1) ||
+				!minstrel_ht_ri_txstat_valid(mp, mi, &(st->rates[i + 1]));
+
+			rate = minstrel_ht_ri_get_stats(mp, mi, &(st->rates[i]));
+
+			if (last)
+				rate->success += info->status.ampdu_ack_len;
+
+			rate->attempts += st->rates[i].retry_count * info->status.ampdu_len;
+		}
+	} else {
+		last = !minstrel_ht_txstat_valid(mp, mi, &ar[0]);
+		for (i = 0; !last; i++) {
+			last = (i == IEEE80211_TX_MAX_RATES - 1) ||
+				!minstrel_ht_txstat_valid(mp, mi, &ar[i + 1]);
 
-		rate = minstrel_ht_get_stats(mp, mi, &ar[i]);
-		if (last)
-			rate->success += info->status.ampdu_ack_len;
+			rate = minstrel_ht_get_stats(mp, mi, &ar[i]);
+			if (last)
+				rate->success += info->status.ampdu_ack_len;
 
-		rate->attempts += ar[i].count * info->status.ampdu_len;
+			rate->attempts += ar[i].count * info->status.ampdu_len;
+		}
 	}
 
 	if (mp->hw->max_rates > 1) {
@@ -1576,6 +1686,7 @@ minstrel_ht_update_caps(void *priv, struct ieee80211_supported_band *sband,
 {
 	struct minstrel_priv *mp = priv;
 	struct minstrel_ht_sta *mi = priv_sta;
+	struct sta_info *sta_info;
 	struct ieee80211_mcs_info *mcs = &sta->ht_cap.mcs;
 	u16 ht_cap = sta->ht_cap.cap;
 	struct ieee80211_sta_vht_cap *vht_cap = &sta->vht_cap;
@@ -1698,6 +1809,10 @@ minstrel_ht_update_caps(void *priv, struct ieee80211_supported_band *sband,
 			n_supported++;
 	}
 
+	sta_info = container_of(sta, struct sta_info, sta);
+	mi->use_short_preamble = test_sta_flag(sta_info, WLAN_STA_SHORT_PREAMBLE) &&
+				 sta_info->sdata->vif.bss_conf.use_short_preamble;
+
 	minstrel_ht_update_cck(mp, mi, sband, sta);
 	minstrel_ht_update_ofdm(mp, mi, sband, sta);
 
diff --git a/net/mac80211/rc80211_minstrel_ht.h b/net/mac80211/rc80211_minstrel_ht.h
index 06e7126727ad..1766ff0c78d3 100644
--- a/net/mac80211/rc80211_minstrel_ht.h
+++ b/net/mac80211/rc80211_minstrel_ht.h
@@ -180,7 +180,7 @@ struct minstrel_ht_sta {
 
 	/* tx flags to add for frames for this sta */
 	u32 tx_flags;
-
+	bool use_short_preamble;
 	u8 band;
 
 	u8 sample_seq;
-- 
2.30.2

Re: [PATCH v2 1/2] mac80211: extend current rate control tx status API

[Ben Greear]

On 3/9/22 11:57 AM, Jonas Jelonek wrote:
> This patch adds the new struct ieee80211_rate_status and replaces
> 'struct rate_info *rate' in ieee80211_tx_status with pointer and length
> annotation.
> 
> The struct ieee80211_rate_status allows to:
> (1)	receive tx power status feedback for transmit power control (TPC)
> 	per packet or packet retry
> (2)	dynamic mapping of wifi chip specific multi-rate retry (mrr)
> 	chains with different lengths
> (3)	increase the limit of annotatable rate indices to support
> 	IEEE802.11ac rate sets and beyond
> 
> ieee80211_tx_info, control and status buffer, and ieee80211_tx_rate
> cannot be used to achieve these goals due to fixed size limitations.
> 
> Our new struct contains a struct rate_info to annotate the rate that was
> used, retry count of the rate and tx power. It is intended for all
> information related to RC and TPC that needs to be passed from driver to
> mac80211 and its RC/TPC algorithms like Minstrel_HT. It corresponds to
> one stage in an mrr. Multiple subsequent instances of this struct can be
> included in struct ieee80211_tx_status via a pointer and a length variable.
> Those instances can be allocated on-stack. The former reference to a single
> instance of struct rate_info is replaced with our new annotation.
> 
> Further mandatory changes in status.c and mt76 driver due to the
> removal of 'struct rate_info *rate' are also included.
> status.c already uses the information in ieee80211_tx_status->rate in
> radiotap, this is now changed to use ieee80211_rate_status->rate_idx.
> mt76 driver already uses struct rate_info to pass the tx rate to status
> path. It is now enclosed in an instance of struct ieee80211_rate_status
> with default values for retry_count and tx_power. The latter should be
> adjusted later to pass more accurate values.
> 
> Compile-Tested: current wireless-next tree with all flags on
> Tested-on: Xiaomi 4A Gigabit (MediaTek MT7603E, MT7612E) with OpenWrt
> 		Linux 5.10.83
> 
> Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
> ---
>   drivers/net/wireless/mediatek/mt76/tx.c | 13 +++-
>   include/net/mac80211.h                  | 10 ++-
>   net/mac80211/status.c                   | 91 ++++++++++++++-----------
>   3 files changed, 71 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/tx.c b/drivers/net/wireless/mediatek/mt76/tx.c
> index 6b8c9dc80542..ed3f3654999f 100644
> --- a/drivers/net/wireless/mediatek/mt76/tx.c
> +++ b/drivers/net/wireless/mediatek/mt76/tx.c
> @@ -62,13 +62,20 @@ mt76_tx_status_unlock(struct mt76_dev *dev, struct sk_buff_head *list)
>   		};
>   		struct mt76_tx_cb *cb = mt76_tx_skb_cb(skb);
>   		struct mt76_wcid *wcid;
> +		struct ieee80211_rate_status rate = {0};
>   
>   		wcid = rcu_dereference(dev->wcid[cb->wcid]);
>   		if (wcid) {
>   			status.sta = wcid_to_sta(wcid);
> -
> -			if (status.sta)
> -				status.rate = &wcid->rate;
> +			if (status.sta) {
> +				rate.rate_idx = wcid->rate;
> +				rate.retry_count = 1;
> +				/* Default 0 for now, can be used by TPC algorithm */
> +				rate.tx_power = 0;
> +
> +				status.rates = &rate;
> +				status.n_rates = 1;
> +			}
>   		}
>   
>   		hw = mt76_tx_status_get_hw(dev, skb);
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index c50221d7e82c..1e98ed04b446 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -1131,6 +1131,12 @@ ieee80211_info_get_tx_time_est(struct ieee80211_tx_info *info)
>   	return info->tx_time_est << 2;
>   }
>   
> +struct ieee80211_rate_status {
> +	struct rate_info rate_idx;
> +	u8 retry_count;
> +	s8 tx_power;
> +};

Please document the units for tx_power.  Many chips can support 1/2 db increments, for instance,
so consider that for units...  A zero txpower is still a valid number, so you probably need
something other than 0 to be the 'default'.  Like -128?

And, does 'retry_count' actually mean 'try_count'?  So a single tx would be retry_count = 1?
Please document that as well.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH v2 1/2] mac80211: extend current rate control tx status API

[Thomas Hühn]

Hiho

> On 9. Mar 2022, at 21:38, Ben Greear <greearb@candelatech.com> wrote:
> 
> On 3/9/22 11:57 AM, Jonas Jelonek wrote:
>> This patch adds the new struct ieee80211_rate_status and replaces
>> 'struct rate_info *rate' in ieee80211_tx_status with pointer and length
>> annotation.
>> The struct ieee80211_rate_status allows to:
>> (1)	receive tx power status feedback for transmit power control (TPC)
>> 	per packet or packet retry
>> (2)	dynamic mapping of wifi chip specific multi-rate retry (mrr)
>> 	chains with different lengths
>> (3)	increase the limit of annotatable rate indices to support
>> 	IEEE802.11ac rate sets and beyond
>> ieee80211_tx_info, control and status buffer, and ieee80211_tx_rate
>> cannot be used to achieve these goals due to fixed size limitations.
>> Our new struct contains a struct rate_info to annotate the rate that was
>> used, retry count of the rate and tx power. It is intended for all
>> information related to RC and TPC that needs to be passed from driver to
>> mac80211 and its RC/TPC algorithms like Minstrel_HT. It corresponds to
>> one stage in an mrr. Multiple subsequent instances of this struct can be
>> included in struct ieee80211_tx_status via a pointer and a length variable.
>> Those instances can be allocated on-stack. The former reference to a single
>> instance of struct rate_info is replaced with our new annotation.
>> Further mandatory changes in status.c and mt76 driver due to the
>> removal of 'struct rate_info *rate' are also included.
>> status.c already uses the information in ieee80211_tx_status->rate in
>> radiotap, this is now changed to use ieee80211_rate_status->rate_idx.
>> mt76 driver already uses struct rate_info to pass the tx rate to status
>> path. It is now enclosed in an instance of struct ieee80211_rate_status
>> with default values for retry_count and tx_power. The latter should be
>> adjusted later to pass more accurate values.
>> Compile-Tested: current wireless-next tree with all flags on
>> Tested-on: Xiaomi 4A Gigabit (MediaTek MT7603E, MT7612E) with OpenWrt
>> 		Linux 5.10.83
>> Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
>> ---
>>  drivers/net/wireless/mediatek/mt76/tx.c | 13 +++-
>>  include/net/mac80211.h                  | 10 ++-
>>  net/mac80211/status.c                   | 91 ++++++++++++++-----------
>>  3 files changed, 71 insertions(+), 43 deletions(-)
>> diff --git a/drivers/net/wireless/mediatek/mt76/tx.c b/drivers/net/wireless/mediatek/mt76/tx.c
>> index 6b8c9dc80542..ed3f3654999f 100644
>> --- a/drivers/net/wireless/mediatek/mt76/tx.c
>> +++ b/drivers/net/wireless/mediatek/mt76/tx.c
>> @@ -62,13 +62,20 @@ mt76_tx_status_unlock(struct mt76_dev *dev, struct sk_buff_head *list)
>>  		};
>>  		struct mt76_tx_cb *cb = mt76_tx_skb_cb(skb);
>>  		struct mt76_wcid *wcid;
>> +		struct ieee80211_rate_status rate = {0};
>>    		wcid = rcu_dereference(dev->wcid[cb->wcid]);
>>  		if (wcid) {
>>  			status.sta = wcid_to_sta(wcid);
>> -
>> -			if (status.sta)
>> -				status.rate = &wcid->rate;
>> +			if (status.sta) {
>> +				rate.rate_idx = wcid->rate;
>> +				rate.retry_count = 1;
>> +				/* Default 0 for now, can be used by TPC algorithm */
>> +				rate.tx_power = 0;
>> +
>> +				status.rates = &rate;
>> +				status.n_rates = 1;
>> +			}
>>  		}
>>    		hw = mt76_tx_status_get_hw(dev, skb);
>> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
>> index c50221d7e82c..1e98ed04b446 100644
>> --- a/include/net/mac80211.h
>> +++ b/include/net/mac80211.h
>> @@ -1131,6 +1131,12 @@ ieee80211_info_get_tx_time_est(struct ieee80211_tx_info *info)
>>  	return info->tx_time_est << 2;
>>  }
>>  +struct ieee80211_rate_status {
>> +	struct rate_info rate_idx;
>> +	u8 retry_count;
>> +	s8 tx_power;
>> +};
> 
> Please document the units for tx_power.  Many chips can support 1/2 db increments, for instance,
> so consider that for units...  A zero txpower is still a valid number, so you probably need
> something other than 0 to be the 'default'.  Like -128?

Certain 802.11a/g/n Atheros chips provide a 0,5dB tx-power step granularity, while Mediatek 802.11ac chips have 1dB or even 3dB step width. So I would argue that a 1dB step width is a good compromise as the common value for new tpc algorithms.

The ath9k chips I have used so far support a minimum tx-power of -5dBm (=0,32mW), Mediatek has a min of 0dBm (=1mW)… so I would argue to use 0dBm  (=1mW) as common minimum tx-power value, as the any possible spatial reuse gain happens from 0dBm up to max tx-power.

> 
> And, does 'retry_count' actually mean 'try_count'?  So a single tx would be retry_count = 1?
> Please document that as well.
> 
> Thanks,
> Ben

Greetings Thomas

Re: [PATCH v2 1/2] mac80211: extend current rate control tx status API

[Ben Greear]

On 3/10/22 8:07 AM, Thomas Hühn wrote:
> Hiho
> 
>> On 9. Mar 2022, at 21:38, Ben Greear <greearb@candelatech.com> wrote:
>>
>> On 3/9/22 11:57 AM, Jonas Jelonek wrote:
>>> This patch adds the new struct ieee80211_rate_status and replaces
>>> 'struct rate_info *rate' in ieee80211_tx_status with pointer and length
>>> annotation.
>>> The struct ieee80211_rate_status allows to:
>>> (1)	receive tx power status feedback for transmit power control (TPC)
>>> 	per packet or packet retry
>>> (2)	dynamic mapping of wifi chip specific multi-rate retry (mrr)
>>> 	chains with different lengths
>>> (3)	increase the limit of annotatable rate indices to support
>>> 	IEEE802.11ac rate sets and beyond
>>> ieee80211_tx_info, control and status buffer, and ieee80211_tx_rate
>>> cannot be used to achieve these goals due to fixed size limitations.
>>> Our new struct contains a struct rate_info to annotate the rate that was
>>> used, retry count of the rate and tx power. It is intended for all
>>> information related to RC and TPC that needs to be passed from driver to
>>> mac80211 and its RC/TPC algorithms like Minstrel_HT. It corresponds to
>>> one stage in an mrr. Multiple subsequent instances of this struct can be
>>> included in struct ieee80211_tx_status via a pointer and a length variable.
>>> Those instances can be allocated on-stack. The former reference to a single
>>> instance of struct rate_info is replaced with our new annotation.
>>> Further mandatory changes in status.c and mt76 driver due to the
>>> removal of 'struct rate_info *rate' are also included.
>>> status.c already uses the information in ieee80211_tx_status->rate in
>>> radiotap, this is now changed to use ieee80211_rate_status->rate_idx.
>>> mt76 driver already uses struct rate_info to pass the tx rate to status
>>> path. It is now enclosed in an instance of struct ieee80211_rate_status
>>> with default values for retry_count and tx_power. The latter should be
>>> adjusted later to pass more accurate values.
>>> Compile-Tested: current wireless-next tree with all flags on
>>> Tested-on: Xiaomi 4A Gigabit (MediaTek MT7603E, MT7612E) with OpenWrt
>>> 		Linux 5.10.83
>>> Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
>>> ---
>>>   drivers/net/wireless/mediatek/mt76/tx.c | 13 +++-
>>>   include/net/mac80211.h                  | 10 ++-
>>>   net/mac80211/status.c                   | 91 ++++++++++++++-----------
>>>   3 files changed, 71 insertions(+), 43 deletions(-)
>>> diff --git a/drivers/net/wireless/mediatek/mt76/tx.c b/drivers/net/wireless/mediatek/mt76/tx.c
>>> index 6b8c9dc80542..ed3f3654999f 100644
>>> --- a/drivers/net/wireless/mediatek/mt76/tx.c
>>> +++ b/drivers/net/wireless/mediatek/mt76/tx.c
>>> @@ -62,13 +62,20 @@ mt76_tx_status_unlock(struct mt76_dev *dev, struct sk_buff_head *list)
>>>   		};
>>>   		struct mt76_tx_cb *cb = mt76_tx_skb_cb(skb);
>>>   		struct mt76_wcid *wcid;
>>> +		struct ieee80211_rate_status rate = {0};
>>>     		wcid = rcu_dereference(dev->wcid[cb->wcid]);
>>>   		if (wcid) {
>>>   			status.sta = wcid_to_sta(wcid);
>>> -
>>> -			if (status.sta)
>>> -				status.rate = &wcid->rate;
>>> +			if (status.sta) {
>>> +				rate.rate_idx = wcid->rate;
>>> +				rate.retry_count = 1;
>>> +				/* Default 0 for now, can be used by TPC algorithm */
>>> +				rate.tx_power = 0;
>>> +
>>> +				status.rates = &rate;
>>> +				status.n_rates = 1;
>>> +			}
>>>   		}
>>>     		hw = mt76_tx_status_get_hw(dev, skb);
>>> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
>>> index c50221d7e82c..1e98ed04b446 100644
>>> --- a/include/net/mac80211.h
>>> +++ b/include/net/mac80211.h
>>> @@ -1131,6 +1131,12 @@ ieee80211_info_get_tx_time_est(struct ieee80211_tx_info *info)
>>>   	return info->tx_time_est << 2;
>>>   }
>>>   +struct ieee80211_rate_status {
>>> +	struct rate_info rate_idx;
>>> +	u8 retry_count;
>>> +	s8 tx_power;
>>> +};
>>
>> Please document the units for tx_power.  Many chips can support 1/2 db increments, for instance,
>> so consider that for units...  A zero txpower is still a valid number, so you probably need
>> something other than 0 to be the 'default'.  Like -128?
> 
> Certain 802.11a/g/n Atheros chips provide a 0,5dB tx-power step granularity, while Mediatek 802.11ac chips have 1dB or even 3dB step width. So I would argue that a 1dB step width is a good compromise as the common value for new tpc algorithms.

If you use 0.5db units for that struct, then it will support anything with that granularity or higher.
But, fine with me if you want to just have it be 1db units.

> 
> The ath9k chips I have used so far support a minimum tx-power of -5dBm (=0,32mW), Mediatek has a min of 0dBm (=1mW)… so I would argue to use 0dBm  (=1mW) as common minimum tx-power value, as the any possible spatial reuse gain happens from 0dBm up to max tx-power.

If a chip supports setting to txpower to -5, then why not let the API support that?  Have The value -128
be 'do not set', and anything else will mean 'try to set the chip to this power or the nearest thing to it
that the chip supports'.

Thanks,
Ben

> 
>>
>> And, does 'retry_count' actually mean 'try_count'?  So a single tx would be retry_count = 1?
>> Please document that as well.
>>
>> Thanks,
>> Ben
> 
> Greetings Thomas
> 


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH v2 1/2] mac80211: extend current rate control tx status API

[Jonas Jelonek]

On 3/10/22 16:43 UTC, Ben Greear wrote:
> >
> > Certain 802.11a/g/n Atheros chips provide a 0,5dB tx-power step granularity, while Mediatek 802.11ac chips have 1dB or even 3dB step width. So I would argue that a 1dB step width is a good compromise as the common value for new tpc algorithms.
>
> If you use 0.5db units for that struct, then it will support anything with that granularity or higher.
> But, fine with me if you want to just have it be 1db units.
>
using 0.5db is more appropriate for the already existing chips that
support this granularity, and is more future-proof.
1db units may be easier to handle for the API and/or TPC algorithms
but again limits existing hardware capabilities.

> > The ath9k chips I have used so far support a minimum tx-power of -5dBm (=0,32mW), Mediatek has a min of 0dBm (=1mW)… so I would argue to use 0dBm  (=1mW) as common minimum tx-power value, as the any possible spatial reuse gain happens from 0dBm up to max tx-power.
>
> If a chip supports setting to txpower to -5, then why not let the API support that?  Have The value -128
> be 'do not set', and anything else will mean 'try to set the chip to this power or the nearest thing to it
> that the chip supports'.

I agree with that, having -128 as value for 'not set' or 'invalid'
would leave the negative dBm for chips that support this.
Whether the TPC then actually makes use of this should not be the
reason to use 0 as default.

To your previous question:
retry_count = 1 is intended to be a single tx, so naming the struct
member 'try_count' would be more appropriate?

Besides this, I will add proper documentation for this in the
following patch version to clarify the units and meanings.

Re: [PATCH v2 1/2] mac80211: extend current rate control tx status API

[Ben Greear]

On 3/10/22 9:27 AM, Jonas Jelonek wrote:
> On 3/10/22 16:43 UTC, Ben Greear wrote:
>>>
>>> Certain 802.11a/g/n Atheros chips provide a 0,5dB tx-power step granularity, while Mediatek 802.11ac chips have 1dB or even 3dB step width. So I would argue that a 1dB step width is a good compromise as the common value for new tpc algorithms.
>>
>> If you use 0.5db units for that struct, then it will support anything with that granularity or higher.
>> But, fine with me if you want to just have it be 1db units.
>>
> using 0.5db is more appropriate for the already existing chips that
> support this granularity, and is more future-proof.
> 1db units may be easier to handle for the API and/or TPC algorithms
> but again limits existing hardware capabilities.
> 
>>> The ath9k chips I have used so far support a minimum tx-power of -5dBm (=0,32mW), Mediatek has a min of 0dBm (=1mW)… so I would argue to use 0dBm  (=1mW) as common minimum tx-power value, as the any possible spatial reuse gain happens from 0dBm up to max tx-power.
>>
>> If a chip supports setting to txpower to -5, then why not let the API support that?  Have The value -128
>> be 'do not set', and anything else will mean 'try to set the chip to this power or the nearest thing to it
>> that the chip supports'.
> 
> I agree with that, having -128 as value for 'not set' or 'invalid'
> would leave the negative dBm for chips that support this.
> Whether the TPC then actually makes use of this should not be the
> reason to use 0 as default.
> 
> To your previous question:
> retry_count = 1 is intended to be a single tx, so naming the struct
> member 'try_count' would be more appropriate?

Yes, I think so.

In my own hackings, I have also used a try_count of '0' to mean try once
but request NOACK on the frame.  I am not sure if that even applies in
your case though...

Thanks,
Ben

> 
> Besides this, I will add proper documentation for this in the
> following patch version to clarify the units and meanings.




-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com