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

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

[greearb]

From: Ben Greear <greearb@candelatech.com>

This will let upper stack properly record stats per link.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---

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(+)

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
index e7cb6dcde182..42bb21a976fc 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
@@ -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);
+		}
 	}
 
 	/*
-- 
2.42.0

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

[greearb]

From: Ben Greear <greearb@candelatech.com>

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 <greearb@candelatech.com>
---

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(-)

diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index 71ef0b7bd9a1..b844d2f207aa 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -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);
 
-- 
2.42.0

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

[Johannes Berg]

On Wed, 2024-08-28 at 08:54 -0700, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
> 
> 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.

> +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' ...

> @@ -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.

> 
> +		link_sta = ieee80211_get_tx_link_sta(sta, info);

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

johannes

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

[Ben Greear]

On 9/3/24 03:13, Johannes Berg wrote:
> On Wed, 2024-08-28 at 08:54 -0700, greearb@candelatech.com wrote:
>> From: Ben Greear <greearb@candelatech.com>
>>
>> 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.

> 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.

>> +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.

> 
>> @@ -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.
> 
>>
>> +		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

> 
> johannes
> 

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