Skip to contentSkip to conversation

wifi: iwlwifi: Report link-id for transmitted frames.

RFC PATCH In review [email protected] 2 participants 4 messages Aug 28, 2024 – Sep 3, 2024

Subsystems (2)
mac80211 Intel wireless wifi link (iwlwifi)
lore
Share & tools
lore

[email protected]

author
#0 lore raw

RFC PATCH 1/2 wifi: iwlwifi: Report link-id for transmitted frames.

From: Ben Greear <[email protected]>

This will let upper stack properly record stats per link.

Signed-off-by Ben Greear <[email protected]>

---

Patch is against my internal tree at this point.
Will port to upstream kernel once general approach is ironed out.

 drivers/net/wireless/intel/iwlwifi/mvm/tx.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
drivers/net/wireless/intel/iwlwifi/mvm/tx.c +15 -0
index e7cb6dcde182..42bb21a976fc 100644
@@ -1963,6 +1963,9 @@ static void iwl_mvm_rx_tx_cmd_single(struct iwl_mvm *mvm,
 	u8 lq_color;
 	u16 next_reclaimed, seq_ctl;
 	bool is_ndp = false;
+	struct ieee80211_link_sta *link_sta;
+
+	link_sta = rcu_dereference(mvm->fw_id_to_link_sta[sta_id]);
 
 	__skb_queue_head_init(&skbs);
 

@@ -1989,6 +1992,10 @@ static void iwl_mvm_rx_tx_cmd_single(struct iwl_mvm *mvm,
 
 		memset(&info->status, 0, sizeof(info->status));
 		info->flags &= ~(IEEE80211_TX_STAT_ACK | IEEE80211_TX_STAT_TX_FILTERED);
+		if (link_sta) {
+			info->control.flags &= ~(u32_encode_bits(0xF, IEEE80211_TX_CTRL_MLO_LINK));
+			info->control.flags |= u32_encode_bits(link_sta->link_id, IEEE80211_TX_CTRL_MLO_LINK);
+		}
 
 		/* inform mac80211 about what happened with the frame */
 		switch (status & TX_STATUS_MSK) {

@@ -2334,6 +2341,7 @@ static void iwl_mvm_tx_reclaim(struct iwl_mvm *mvm, int sta_id, int tid,
 	struct iwl_mvm_sta *mvmsta = NULL;
 	struct sk_buff *skb;
 	int freed;
+	struct ieee80211_link_sta *link_sta;
 
 	if (WARN_ONCE(sta_id >= mvm->fw->ucode_capa.num_stations ||
 		      tid > IWL_MAX_TID_COUNT,

@@ -2350,6 +2358,8 @@ static void iwl_mvm_tx_reclaim(struct iwl_mvm *mvm, int sta_id, int tid,
 		return;
 	}
 
+	link_sta = rcu_dereference(mvm->fw_id_to_link_sta[sta_id]);
+
 	__skb_queue_head_init(&reclaimed_skbs);
 
 	/*

@@ -2437,6 +2447,11 @@ static void iwl_mvm_tx_reclaim(struct iwl_mvm *mvm, int sta_id, int tid,
 			info->flags |= IEEE80211_TX_STAT_ACK;
 		else
 			info->flags &= ~IEEE80211_TX_STAT_ACK;
+
+		if (link_sta) {
+			info->control.flags &= ~(u32_encode_bits(0xF, IEEE80211_TX_CTRL_MLO_LINK));
+			info->control.flags |= u32_encode_bits(link_sta->link_id, IEEE80211_TX_CTRL_MLO_LINK);
+		}
 	}
 
 	/*
Signature · 1 line
2.42.0
author
#1 lore raw

RFC PATCH 2/2 wifi: mac80211: Assign tx-stats to the proper link.

From: Ben Greear <[email protected]>

For drivers that can report the tx link-id, account tx stats against that link. If we cannot determine tx link, then use deflink.

Signed-off-by Ben Greear <[email protected]>

---

Patch is against my internal tree, won't apply to upstream. Will port to upstream once details are ironed out.

 net/mac80211/status.c | 105 +++++++++++++++++++++++++++---------------
 1 file changed, 67 insertions(+), 38 deletions(-)
net/mac80211/status.c +67 -38
index 71ef0b7bd9a1..b844d2f207aa 100644
@@ -41,6 +41,23 @@ void ieee80211_tx_status_irqsafe(struct ieee80211_hw *hw,
 }
 EXPORT_SYMBOL(ieee80211_tx_status_irqsafe);
 
+static struct link_sta_info*
+ieee80211_get_tx_link_sta(struct sta_info *sta, struct ieee80211_tx_info *info)
+{
+	u8 link_id = u32_get_bits(info->control.flags, IEEE80211_TX_CTRL_MLO_LINK);
+	struct link_sta_info *l_sta_info;
+
+	if (!sta)
+		return NULL;
+
+	if (link_id != IEEE80211_LINK_UNSPECIFIED) {
+		l_sta_info = rcu_access_pointer(sta->link[link_id]);
+		if (l_sta_info)
+			return l_sta_info;
+	}
+	return &sta->deflink;
+}
+
 static void ieee80211_handle_filtered_frame(struct ieee80211_local *local,
 					    struct sta_info *sta,
 					    struct sk_buff *skb)

@@ -48,6 +65,7 @@ static void ieee80211_handle_filtered_frame(struct ieee80211_local *local,
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
 	struct ieee80211_hdr *hdr = (void *)skb->data;
 	int ac;
+	struct link_sta_info *link_sta = ieee80211_get_tx_link_sta(sta, info);
 
 	if (info->flags & (IEEE80211_TX_CTL_NO_PS_BUFFER |
 			   IEEE80211_TX_CTL_AMPDU |

@@ -72,7 +90,7 @@ static void ieee80211_handle_filtered_frame(struct ieee80211_local *local,
 	info->flags |= IEEE80211_TX_INTFL_RETRANSMISSION;
 	info->flags &= ~IEEE80211_TX_TEMPORARY_FLAGS;
 
-	sta->deflink.status_stats.filtered++;
+	link_sta->status_stats.filtered++;
 
 	/*
 	 * Clear more-data bit on filtered frames, it might be set

@@ -826,6 +844,7 @@ static void ieee80211_lost_packet(struct sta_info *sta,
 {
 	unsigned long pkt_time = STA_LOST_PKT_TIME;
 	unsigned int pkt_thr = STA_LOST_PKT_THRESHOLD;
+	struct link_sta_info *link_sta;
 
 	/* If driver relies on its own algorithm for station kickout, skip
 	 * mac80211 packet loss mechanism.

@@ -838,7 +857,8 @@ static void ieee80211_lost_packet(struct sta_info *sta,
 	    !(info->flags & IEEE80211_TX_STAT_AMPDU))
 		return;
 
-	sta->deflink.status_stats.lost_packets++;
+	link_sta = ieee80211_get_tx_link_sta(sta, info);
+	link_sta->status_stats.lost_packets++;
 	if (sta->sta.tdls) {
 		pkt_time = STA_LOST_TDLS_PKT_TIME;
 		pkt_thr = STA_LOST_PKT_THRESHOLD;

@@ -851,14 +871,14 @@ static void ieee80211_lost_packet(struct sta_info *sta,
 	 * mechanism.
 	 * For non-TDLS, use STA_LOST_PKT_THRESHOLD and STA_LOST_PKT_TIME
 	 */
-	if (sta->deflink.status_stats.lost_packets < pkt_thr ||
-	    !time_after(jiffies, sta->deflink.status_stats.last_pkt_time + pkt_time))
+	if (link_sta->status_stats.lost_packets < pkt_thr ||
+	    !time_after(jiffies, link_sta->status_stats.last_pkt_time + pkt_time))
 		return;
 
 	cfg80211_cqm_pktloss_notify(sta->sdata->dev, sta->sta.addr,
-				    sta->deflink.status_stats.lost_packets,
+				    link_sta->status_stats.lost_packets,
 				    GFP_ATOMIC);
-	sta->deflink.status_stats.lost_packets = 0;
+	link_sta->status_stats.lost_packets = 0;
 }
 
 static int ieee80211_tx_get_rates(struct ieee80211_hw *hw,

@@ -970,8 +990,12 @@ static void __ieee80211_tx_status(struct ieee80211_hw *hw,
 	fc = hdr->frame_control;
 
 	if (status->sta) {
+		struct link_sta_info *link_sta;
+
 		sta = container_of(status->sta, struct sta_info, sta);
 
+		link_sta = ieee80211_get_tx_link_sta(sta, info);
+
 		if (info->flags & IEEE80211_TX_STATUS_EOSP)
 			clear_sta_flag(sta, WLAN_STA_SP);
 

@@ -988,7 +1012,7 @@ static void __ieee80211_tx_status(struct ieee80211_hw *hw,
 		if (ieee80211_hw_check(&local->hw, HAS_RATE_CONTROL) &&
 		    (ieee80211_is_data(hdr->frame_control)) &&
 		    (rates_idx != -1))
-			sta->deflink.tx_stats.last_rate =
+			link_sta->tx_stats.last_rate =
 				info->status.rates[rates_idx];
 
 		if ((info->flags & IEEE80211_TX_STAT_AMPDU_NO_BACK) &&

@@ -1034,9 +1058,9 @@ static void __ieee80211_tx_status(struct ieee80211_hw *hw,
 			return;
 		} else if (ieee80211_is_data_present(fc)) {
 			if (!acked && !noack_success)
-				sta->deflink.status_stats.msdu_failed[tid]++;
+				link_sta->status_stats.msdu_failed[tid]++;
 
-			sta->deflink.status_stats.msdu_retries[tid] +=
+			link_sta->status_stats.msdu_retries[tid] +=
 				retry_count;
 		}
 

@@ -1147,12 +1171,15 @@ void ieee80211_tx_status_ext(struct ieee80211_hw *hw,
 	int rates_idx, retry_count;
 	bool acked, noack_success, ack_signal_valid;
 	u16 tx_time_est;
+	struct link_sta_info *link_sta;
 
 	if (pubsta) {
 		sta = container_of(pubsta, struct sta_info, sta);
 
+		link_sta = ieee80211_get_tx_link_sta(sta, info);
+
 		if (status->n_rates)
-			sta->deflink.tx_stats.last_rate_info =
+			link_sta->tx_stats.last_rate_info =
 				status->rates[status->n_rates - 1].rate_idx;
 	}
 

@@ -1185,8 +1212,8 @@ void ieee80211_tx_status_ext(struct ieee80211_hw *hw,
 #endif
 
 		if (!acked && !noack_success)
-			sta->deflink.status_stats.retry_failed++;
-		sta->deflink.status_stats.retry_count += retry_count;
+			link_sta->status_stats.retry_failed++;
+		link_sta->status_stats.retry_count += retry_count;
 
 		if (ieee80211_hw_check(&local->hw, REPORTS_TX_ACK_STATUS)) {
 			if (sdata->vif.type == NL80211_IFTYPE_STATION &&

@@ -1198,13 +1225,12 @@ void ieee80211_tx_status_ext(struct ieee80211_hw *hw,
 #ifdef CONFIG_MAC80211_DEBUG_STA_COUNTERS
 				do_stats = true;
 #endif
-				sta->deflink.status_stats.last_ack = jiffies;
+				link_sta->status_stats.last_ack = jiffies;
 
-				if (sta->deflink.status_stats.lost_packets)
-					sta->deflink.status_stats.lost_packets = 0;
+				link_sta->status_stats.lost_packets = 0;
 
 				/* Track when last packet was ACKed */
-				sta->deflink.status_stats.last_pkt_time = jiffies;
+				link_sta->status_stats.last_pkt_time = jiffies;
 
 				/* Reset connection monitor */
 				if (sdata->vif.type == NL80211_IFTYPE_STATION &&

@@ -1212,10 +1238,10 @@ void ieee80211_tx_status_ext(struct ieee80211_hw *hw,
 					sdata->u.mgd.probe_send_count = 0;
 
 				if (ack_signal_valid) {
-					sta->deflink.status_stats.last_ack_signal =
+					link_sta->status_stats.last_ack_signal =
 							 (s8)info->status.ack_signal;
-					sta->deflink.status_stats.ack_signal_filled = true;
-					ewma_avg_signal_add(&sta->deflink.status_stats.avg_ack_signal,
+					link_sta->status_stats.ack_signal_filled = true;
+					ewma_avg_signal_add(&link_sta->status_stats.avg_ack_signal,
 							    -info->status.ack_signal);
 				}
 			} else if (test_sta_flag(sta, WLAN_STA_PS_STA)) {

@@ -1242,40 +1268,40 @@ void ieee80211_tx_status_ext(struct ieee80211_hw *hw,
 			u8 mcs = 0;
 			struct ieee80211_tx_rate *txrt = &(info->status.rates[rates_idx]);
 			if (txrt->flags & IEEE80211_TX_RC_40_MHZ_WIDTH)
-				sta->deflink.tx_stats.msdu_40++;
+				link_sta->tx_stats.msdu_40++;
 			else if (txrt->flags & IEEE80211_TX_RC_80_MHZ_WIDTH)
-				sta->deflink.tx_stats.msdu_80++;
+				link_sta->tx_stats.msdu_80++;
 			else if (txrt->flags & IEEE80211_TX_RC_160_MHZ_WIDTH)
-				sta->deflink.tx_stats.msdu_160++;
+				link_sta->tx_stats.msdu_160++;
 			else
-				sta->deflink.tx_stats.msdu_20++;
+				link_sta->tx_stats.msdu_20++;
 
 			if (txrt->flags & IEEE80211_TX_RC_MCS) {
 				nss = (txrt->idx / 8);
 				mcs = txrt->idx - (nss * 8);
-				sta->deflink.tx_stats.msdu_ht++;
+				link_sta->tx_stats.msdu_ht++;
 			} else if (txrt->flags & IEEE80211_TX_RC_VHT_MCS) {
 				mcs = ieee80211_rate_get_vht_mcs(txrt);
 				nss = ieee80211_rate_get_vht_nss(txrt);
 				nss -= 1;
-				sta->deflink.tx_stats.msdu_vht++;
+				link_sta->tx_stats.msdu_vht++;
 			} else if (pubsta && status->rates &&
-				 (sta->deflink.tx_stats.last_rate_info.flags & RATE_INFO_FLAGS_HE_MCS)) {
+				 (link_sta->tx_stats.last_rate_info.flags & RATE_INFO_FLAGS_HE_MCS)) {
 				/* get info from status->rate */
-				mcs = sta->deflink.tx_stats.last_rate_info.mcs;
-				nss = sta->deflink.tx_stats.last_rate_info.nss - 1;
-				sta->deflink.tx_stats.msdu_he++;
+				mcs = link_sta->tx_stats.last_rate_info.mcs;
+				nss = link_sta->tx_stats.last_rate_info.nss - 1;
+				link_sta->tx_stats.msdu_he++;
 			} else {
 				mcs = txrt->idx;
-				sta->deflink.tx_stats.msdu_legacy++;
+				link_sta->tx_stats.msdu_legacy++;
 			}
 
-			if (nss > (ARRAY_SIZE(sta->deflink.tx_stats.msdu_nss) - 1))
-				nss = ARRAY_SIZE(sta->deflink.tx_stats.msdu_nss) - 1;
-			if (mcs > (ARRAY_SIZE(sta->deflink.tx_stats.msdu_rate_idx) - 1))
-				mcs = ARRAY_SIZE(sta->deflink.tx_stats.msdu_rate_idx) - 1;
-			sta->deflink.tx_stats.msdu_nss[nss]++;
-			sta->deflink.tx_stats.msdu_rate_idx[mcs]++;
+			if (nss > (ARRAY_SIZE(link_sta->tx_stats.msdu_nss) - 1))
+				nss = ARRAY_SIZE(link_sta->tx_stats.msdu_nss) - 1;
+			if (mcs > (ARRAY_SIZE(link_sta->tx_stats.msdu_rate_idx) - 1))
+				mcs = ARRAY_SIZE(link_sta->tx_stats.msdu_rate_idx) - 1;
+			link_sta->tx_stats.msdu_nss[nss]++;
+			link_sta->tx_stats.msdu_rate_idx[mcs]++;
 		}
 #endif
 

@@ -1326,8 +1352,11 @@ void ieee80211_tx_rate_update(struct ieee80211_hw *hw,
 
 	rate_control_tx_status(local, &status);
 
-	if (ieee80211_hw_check(&local->hw, HAS_RATE_CONTROL))
-		sta->deflink.tx_stats.last_rate = info->status.rates[0];
+	if (ieee80211_hw_check(&local->hw, HAS_RATE_CONTROL)) {
+		struct link_sta_info *link_sta = ieee80211_get_tx_link_sta(sta, info);
+
+		link_sta->tx_stats.last_rate = info->status.rates[0];
+	}
 }
 EXPORT_SYMBOL(ieee80211_tx_rate_update);
Signature · 1 line
2.42.0
maintainer
#2 lore raw

Re: [RFC PATCH 2/2] wifi: mac80211: Assign tx-stats to the proper link.

Quoted 5 lines · On Wed, 2024-08-28 at 08:54 -0700, [email protected] wrote:

From: Ben Greear <[email protected]>

For drivers that can report the tx link-id, account tx stats against that link. If we cannot determine tx link, then use deflink.

Strictly speaking, that's not what happens, since the link bits in the SKB CB might be set on outgoing frames, and then will still be there on the status.

Also using deflink is totally useless for MLO, so maybe just don't do anything at all? But might be simpler to just do deflink and document that the driver must set this? But not sure that really works so well for drivers now.

net/mac80211/status.c void ieee80211_tx_status_irqsafe(struct ieee80211_hw *hw, · line 48

Quoted 5 lines
net/mac80211/status.c +5 -0
+static struct link_sta_info*
+ieee80211_get_tx_link_sta(struct sta_info *sta, struct ieee80211_tx_info *info)
+{
+	u8 link_id = u32_get_bits(info->control.flags, IEEE80211_TX_CTRL_MLO_LINK);
+	struct link_sta_info *l_sta_info;

We usually call that 'link_sta' ...

net/mac80211/status.c static void ieee80211_handle_filtered_frame(struct ieee80211_local *local, · line 68

Quoted 5 lines
net/mac80211/status.c +1 -0
@@ -48,6 +65,7 @@ static void ieee80211_handle_filtered_frame(struct ieee80211_local *local,
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
 	struct ieee80211_hdr *hdr = (void *)skb->data;
 	int ac;
+	struct link_sta_info *link_sta = ieee80211_get_tx_link_sta(sta, info);

and even you do, please be consistent with existing code and yourself.

Review static void ieee80211_handle_filtered_frame(struct ieee80211_local *local, · line 69

Quoted 2 lines
(quoted diff) +1 -0
+		link_sta = ieee80211_get_tx_link_sta(sta, info);

Does it really make sense to keep repeating this, rather than passing an argument?

johannes

author
#3 lore raw

Re: [RFC PATCH 2/2] wifi: mac80211: Assign tx-stats to the proper link.

Quoted 10 lines · Johannes Berg
Quoted 5 lines · On Wed, 2024-08-28 at 08:54 -0700, [email protected] wrote:

From: Ben Greear <[email protected]>

For drivers that can report the tx link-id, account tx stats against that link. If we cannot determine tx link, then use deflink.

Strictly speaking, that's not what happens, since the link bits in the SKB CB might be set on outgoing frames, and then will still be there on the status.

Ok, I can update description to mention that.

Quoted 4 lines

Also using deflink is totally useless for MLO, so maybe just don't do anything at all? But might be simpler to just do deflink and document that the driver must set this? But not sure that really works so well for drivers now.

The stats used to all go to deflink, so that seems a good default, and should work w/out MLO being enabled.

Quoted 7 lines

net/mac80211/status.c void ieee80211_tx_status_irqsafe(struct ieee80211_hw *hw, · line 48

Quoted 5 lines
net/mac80211/status.c +5 -0
+static struct link_sta_info*
+ieee80211_get_tx_link_sta(struct sta_info *sta, struct ieee80211_tx_info *info)
+{
+	u8 link_id = u32_get_bits(info->control.flags, IEEE80211_TX_CTRL_MLO_LINK);
+	struct link_sta_info *l_sta_info;

We usually call that 'link_sta' ...

Yeah, and other similar things are also called link_sta too, which I find confusing. But I will change it back to be link_sta everywhere I am making changes.

Quoted 10 lines

net/mac80211/status.c static void ieee80211_handle_filtered_frame(struct ieee80211_local *local, · line 68

Quoted 6 lines
net/mac80211/status.c +1 -0
@@ -48,6 +65,7 @@ static void ieee80211_handle_filtered_frame(struct ieee80211_local *local,
  	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
  	struct ieee80211_hdr *hdr = (void *)skb->data;
  	int ac;
+	struct link_sta_info *link_sta = ieee80211_get_tx_link_sta(sta, info);

and even you do, please be consistent with existing code and yourself.

Quoted 1 line
Quoted 4 lines

Review

Quoted 1 line
(quoted diff) +1 -0
+		link_sta = ieee80211_get_tx_link_sta(sta, info);

Does it really make sense to keep repeating this, rather than passing an argument?

Ok, happy to pass this into the handlers.

Thanks,
Ben

Quoted 3 lines

johannes

Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com