[PATCH v5 1/3] wifi: mac80211: Assign tx-stats to the proper link.

[PATCH v5 1/3] 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>
---

v5:  Properly increment tx pkt and bytes counters per-link, add
     optional 3/3 patch to make it more clear requested vs actual
     tx packet counters.
v4:  Reorder patches, use new tx_link_id status field 4 bits wide.

 include/net/mac80211.h  |  4 ++-
 net/mac80211/sta_info.h |  3 ++
 net/mac80211/status.c   | 79 ++++++++++++++++++++++++++++++-----------
 3 files changed, 64 insertions(+), 22 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index ab8dce1f2c27..19d3b0517dcd 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1276,7 +1276,9 @@ struct ieee80211_tx_info {
 			u8 ampdu_ack_len;
 			u8 ampdu_len;
 			u8 antenna;
-			u8 pad;
+			/* link ID which transmitted + 1 (0 means default or unknown) */
+			u8 tx_link_id:4;
+			u8 pad:4;
 			u16 tx_time;
 			u8 flags;
 			u8 pad2;
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 9f89fb5bee37..fc59e6410082 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -555,6 +555,9 @@ struct link_sta_info {
 	struct {
 		u64 packets[IEEE80211_NUM_ACS];
 		u64 bytes[IEEE80211_NUM_ACS];
+		/* Packets and bytes reported transmitted (per link) */
+		u64 rep_packets;
+		u64 rep_bytes;
 		struct ieee80211_tx_rate last_rate;
 		struct rate_info last_rate_info;
 		u64 msdu[IEEE80211_NUM_TIDS + 1];
diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index 5f28f3633fa0..c46fd6ea2873 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -41,6 +41,26 @@ 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)
+{
+	int link_id = 0;
+	struct link_sta_info *link_sta;
+
+	if (!sta)
+		return NULL;
+
+	if (info)
+		link_id = info->status.tx_link_id;
+
+	if (link_id) {
+		link_sta = rcu_access_pointer(sta->link[link_id - 1]);
+		if (link_sta)
+			return link_sta;
+	}
+	return &sta->deflink;
+}
+
 static void ieee80211_handle_filtered_frame(struct ieee80211_local *local,
 					    struct sta_info *sta,
 					    struct sk_buff *skb)
@@ -48,6 +68,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 +93,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 +847,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 +860,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 +874,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,
@@ -973,8 +996,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);
 
@@ -991,7 +1018,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) &&
@@ -1037,12 +1064,17 @@ 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;
 		}
 
+		if (acked || noack_success) {
+			link_sta->tx_stats.rep_packets++;
+			link_sta->tx_stats.rep_bytes += skb->len;
+		}
+
 		if (!(info->flags & IEEE80211_TX_CTL_INJECTED) && acked)
 			ieee80211_frame_acked(sta, skb);
 
@@ -1147,12 +1179,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;
 	}
 
@@ -1182,8 +1217,8 @@ void ieee80211_tx_status_ext(struct ieee80211_hw *hw,
 		struct ieee80211_sub_if_data *sdata = sta->sdata;
 
 		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 &&
@@ -1192,13 +1227,12 @@ void ieee80211_tx_status_ext(struct ieee80211_hw *hw,
 							acked, info->status.tx_time);
 
 			if (acked) {
-				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 &&
@@ -1206,10 +1240,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)) {
@@ -1273,8 +1307,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

[PATCH v5 2/3] 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>
---
 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 c9867d26361b..c8b5d5cd3298 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
@@ -1709,6 +1709,14 @@ 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;
+	int link_sta_id = -1;
+
+	rcu_read_lock();
+	link_sta = rcu_dereference(mvm->fw_id_to_link_sta[sta_id]);
+	if (link_sta)
+		link_sta_id = link_sta->link_id;
+	rcu_read_unlock();
 
 	__skb_queue_head_init(&skbs);
 
@@ -1732,6 +1740,7 @@ 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);
+		info->status.tx_link_id = link_sta_id + 1;
 
 		/* inform mac80211 about what happened with the frame */
 		switch (status & TX_STATUS_MSK) {
@@ -2048,6 +2057,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,
@@ -2064,6 +2074,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);
 
 	/*
@@ -2087,6 +2099,9 @@ 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->status.tx_link_id = link_sta->link_id + 1;
 	}
 
 	/*
-- 
2.42.0

[PATCH v5 3/3] wifi: mac80211: Clarify per-link tx stats.

[greearb]

From: Ben Greear <greearb@candelatech.com>

In the tx path, deflink tx stats are increased for the packets
requested to be transmitted.  At that point, we cannot know the
actual link that will be used to transmit.  Rename those stats with
req_ prefix to show they are requested.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 net/mac80211/sta_info.c |  6 +++---
 net/mac80211/sta_info.h |  8 +++++---
 net/mac80211/tx.c       | 20 ++++++++++----------
 3 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index aa22f09e6d14..c8c64271eb26 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -2522,7 +2522,7 @@ static void sta_set_tidstats(struct sta_info *sta,
 
 	if (!(tidstats->filled & BIT(NL80211_TID_STATS_TX_MSDU))) {
 		tidstats->filled |= BIT(NL80211_TID_STATS_TX_MSDU);
-		tidstats->tx_msdu = sta->deflink.tx_stats.msdu[tid];
+		tidstats->tx_msdu = sta->deflink.tx_stats.req_msdu[tid];
 	}
 
 	if (!(tidstats->filled & BIT(NL80211_TID_STATS_TX_MSDU_RETRIES)) &&
@@ -2606,14 +2606,14 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo,
 			       BIT_ULL(NL80211_STA_INFO_TX_BYTES)))) {
 		sinfo->tx_bytes = 0;
 		for (ac = 0; ac < IEEE80211_NUM_ACS; ac++)
-			sinfo->tx_bytes += sta->deflink.tx_stats.bytes[ac];
+			sinfo->tx_bytes += sta->deflink.tx_stats.req_bytes[ac];
 		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_TX_BYTES64);
 	}
 
 	if (!(sinfo->filled & BIT_ULL(NL80211_STA_INFO_TX_PACKETS))) {
 		sinfo->tx_packets = 0;
 		for (ac = 0; ac < IEEE80211_NUM_ACS; ac++)
-			sinfo->tx_packets += sta->deflink.tx_stats.packets[ac];
+			sinfo->tx_packets += sta->deflink.tx_stats.req_packets[ac];
 		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_TX_PACKETS);
 	}
 
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index fc59e6410082..40179a9ec0bc 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -553,14 +553,16 @@ struct link_sta_info {
 
 	/* Updated from TX path only, no locking requirements */
 	struct {
-		u64 packets[IEEE80211_NUM_ACS];
-		u64 bytes[IEEE80211_NUM_ACS];
+		/* Packets and bytes requested to be transmitted, deflink only */
+		u64 req_packets[IEEE80211_NUM_ACS];
+		u64 req_bytes[IEEE80211_NUM_ACS];
 		/* Packets and bytes reported transmitted (per link) */
 		u64 rep_packets;
 		u64 rep_bytes;
 		struct ieee80211_tx_rate last_rate;
 		struct rate_info last_rate_info;
-		u64 msdu[IEEE80211_NUM_TIDS + 1];
+		/* Requested to be transmitted */
+		u64 req_msdu[IEEE80211_NUM_TIDS + 1];
 	} tx_stats;
 
 	enum ieee80211_sta_rx_bandwidth cur_max_bandwidth;
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index a24636bda679..ab89dac99cf5 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -866,7 +866,7 @@ ieee80211_tx_h_sequence(struct ieee80211_tx_data *tx)
 		hdr->seq_ctrl = cpu_to_le16(tx->sdata->sequence_number);
 		tx->sdata->sequence_number += 0x10;
 		if (tx->sta)
-			tx->sta->deflink.tx_stats.msdu[IEEE80211_NUM_TIDS]++;
+			tx->sta->deflink.tx_stats.req_msdu[IEEE80211_NUM_TIDS]++;
 		return TX_CONTINUE;
 	}
 
@@ -880,7 +880,7 @@ ieee80211_tx_h_sequence(struct ieee80211_tx_data *tx)
 
 	/* include per-STA, per-TID sequence counter */
 	tid = ieee80211_get_tid(hdr);
-	tx->sta->deflink.tx_stats.msdu[tid]++;
+	tx->sta->deflink.tx_stats.req_msdu[tid]++;
 
 	hdr->seq_ctrl = ieee80211_tx_next_seq(tx->sta, tid);
 
@@ -1033,10 +1033,10 @@ ieee80211_tx_h_stats(struct ieee80211_tx_data *tx)
 
 	skb_queue_walk(&tx->skbs, skb) {
 		ac = skb_get_queue_mapping(skb);
-		tx->sta->deflink.tx_stats.bytes[ac] += skb->len;
+		tx->sta->deflink.tx_stats.req_bytes[ac] += skb->len;
 	}
 	if (ac >= 0)
-		tx->sta->deflink.tx_stats.packets[ac]++;
+		tx->sta->deflink.tx_stats.req_packets[ac]++;
 
 	return TX_CONTINUE;
 }
@@ -3575,18 +3575,18 @@ ieee80211_xmit_fast_finish(struct ieee80211_sub_if_data *sdata,
 	}
 
 	if (skb_shinfo(skb)->gso_size)
-		sta->deflink.tx_stats.msdu[tid] +=
+		sta->deflink.tx_stats.req_msdu[tid] +=
 			DIV_ROUND_UP(skb->len, skb_shinfo(skb)->gso_size);
 	else
-		sta->deflink.tx_stats.msdu[tid]++;
+		sta->deflink.tx_stats.req_msdu[tid]++;
 
 	info->hw_queue = sdata->vif.hw_queue[skb_get_queue_mapping(skb)];
 
 	/* statistics normally done by ieee80211_tx_h_stats (but that
 	 * has to consider fragmentation, so is more complex)
 	 */
-	sta->deflink.tx_stats.bytes[skb_get_queue_mapping(skb)] += skb->len;
-	sta->deflink.tx_stats.packets[skb_get_queue_mapping(skb)]++;
+	sta->deflink.tx_stats.req_bytes[skb_get_queue_mapping(skb)] += skb->len;
+	sta->deflink.tx_stats.req_packets[skb_get_queue_mapping(skb)]++;
 
 	if (pn_offs) {
 		u64 pn;
@@ -4672,8 +4672,8 @@ static void ieee80211_8023_xmit(struct ieee80211_sub_if_data *sdata,
 	}
 
 	dev_sw_netstats_tx_add(dev, skbs, len);
-	sta->deflink.tx_stats.packets[queue] += skbs;
-	sta->deflink.tx_stats.bytes[queue] += len;
+	sta->deflink.tx_stats.req_packets[queue] += skbs;
+	sta->deflink.tx_stats.req_bytes[queue] += len;
 
 	ieee80211_tpt_led_trig_tx(local, len);
 
-- 
2.42.0

Re: [PATCH v5 1/3] wifi: mac80211: Assign tx-stats to the proper link.

[Johannes Berg]

On Wed, 2024-12-18 at 15:25 -0800, 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.

According to all the RX stats discussion [1] you need some changes here,
so I'd appreciate if you could take a look and rebase/resend.

[1] https://lore.kernel.org/linux-wireless/c22a9e7e-d0f7-477b-b732-c2454a0ac904@quicinc.com/


And please, as I very frequently keep asking you, don't mix different
things into a single patch (such as 'rep_packets'/'rep_bytes' in this
patch). By insisting on not splitting your patches properly before
submitting them you're effectively saying your time is more important
than mine, and I don't appreciate that. All that achieves is that I
don't even want to look at your patches.

johannes

Re: [PATCH v5 1/3] wifi: mac80211: Assign tx-stats to the proper link.

[Ben Greear]

On 7/8/25 05:31, Johannes Berg wrote:
> On Wed, 2024-12-18 at 15:25 -0800, 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.
> 
> According to all the RX stats discussion [1] you need some changes here,
> so I'd appreciate if you could take a look and rebase/resend.
> 
> [1] https://lore.kernel.org/linux-wireless/c22a9e7e-d0f7-477b-b732-c2454a0ac904@quicinc.com/
> 
> 
> And please, as I very frequently keep asking you, don't mix different
> things into a single patch (such as 'rep_packets'/'rep_bytes' in this
> patch). By insisting on not splitting your patches properly before
> submitting them you're effectively saying your time is more important
> than mine, and I don't appreciate that. All that achieves is that I
> don't even want to look at your patches.

As far as I can tell, I split them properly.  I added new counters in one
patch you reference above, and I named them as I think they should be named.
In another patch, I renamed existing variables with a commit message as to why.

I am currently working to rebase our patches on 6.16, and can post a new
stats series once we can verify that the patches work on 6.16.

Thanks,
Ben

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

Re: [PATCH v5 1/3] wifi: mac80211: Assign tx-stats to the proper link.

[Johannes Berg]

On Tue, 2025-07-08 at 06:58 -0700, Ben Greear wrote:
> On 7/8/25 05:31, Johannes Berg wrote:
> > On Wed, 2024-12-18 at 15:25 -0800, 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.
> > 
> > According to all the RX stats discussion [1] you need some changes here,
> > so I'd appreciate if you could take a look and rebase/resend.
> > 
> > [1] https://lore.kernel.org/linux-wireless/c22a9e7e-d0f7-477b-b732-c2454a0ac904@quicinc.com/
> > 
> > 
> > And please, as I very frequently keep asking you, don't mix different
> > things into a single patch (such as 'rep_packets'/'rep_bytes' in this
> > patch). By insisting on not splitting your patches properly before
> > submitting them you're effectively saying your time is more important
> > than mine, and I don't appreciate that. All that achieves is that I
> > don't even want to look at your patches.
> 
> As far as I can tell, I split them properly.  I added new counters in one
> patch you reference above, and I named them as I think they should be named.
> In another patch, I renamed existing variables with a commit message as to why.

This patch said:

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

which I think we can agree should be only a refactoring/fix of the link
that the counters go on?

But you also have

> +		/* Packets and bytes reported transmitted (per link) */
> +		u64 rep_packets;
> +		u64 rep_bytes;

which was probably meant to be in the third patch?

johannes

Re: [PATCH v5 1/3] wifi: mac80211: Assign tx-stats to the proper link.

[Ben Greear]

On 7/8/25 07:21, Johannes Berg wrote:
> On Tue, 2025-07-08 at 06:58 -0700, Ben Greear wrote:
>> On 7/8/25 05:31, Johannes Berg wrote:
>>> On Wed, 2024-12-18 at 15:25 -0800, 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.
>>>
>>> According to all the RX stats discussion [1] you need some changes here,
>>> so I'd appreciate if you could take a look and rebase/resend.
>>>
>>> [1] https://lore.kernel.org/linux-wireless/c22a9e7e-d0f7-477b-b732-c2454a0ac904@quicinc.com/
>>>
>>>
>>> And please, as I very frequently keep asking you, don't mix different
>>> things into a single patch (such as 'rep_packets'/'rep_bytes' in this
>>> patch). By insisting on not splitting your patches properly before
>>> submitting them you're effectively saying your time is more important
>>> than mine, and I don't appreciate that. All that achieves is that I
>>> don't even want to look at your patches.
>>
>> As far as I can tell, I split them properly.  I added new counters in one
>> patch you reference above, and I named them as I think they should be named.
>> In another patch, I renamed existing variables with a commit message as to why.
> 
> This patch said:
> 
>> For drivers that can report the tx link-id, account tx
>> stats against that link.  If we cannot determine tx link,
>> then use deflink.
> 
> which I think we can agree should be only a refactoring/fix of the link
> that the counters go on?
> 
> But you also have
> 
>> +		/* Packets and bytes reported transmitted (per link) */
>> +		u64 rep_packets;
>> +		u64 rep_bytes;
> 
> which was probably meant to be in the third patch?

In today's linus tree, the code looks like this:

        /* Updated from TX path only, no locking requirements */
         struct {
                 u64 packets[IEEE80211_NUM_ACS];
                 u64 bytes[IEEE80211_NUM_ACS];
                 struct ieee80211_tx_rate last_rate;
                 struct rate_info last_rate_info;
                 u64 msdu[IEEE80211_NUM_TIDS + 1];
         } tx_stats;

There are no counters for tx bytes or tx packets (I think the ACS classification
counters are already used for other purposes).

So I added new counters, and I wanted them named to indicate it is what is reported by
the driver, not something specified by the mac80211 stack.

Thanks,
Ben

> 
> johannes
> 

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

Re: [PATCH v5 1/3] wifi: mac80211: Assign tx-stats to the proper link.

[Johannes Berg]

On Tue, 2025-07-08 at 07:32 -0700, Ben Greear wrote:
> 
> 
> So I added new counters, and I wanted them named to indicate it is what is reported by
> the driver, not something specified by the mac80211 stack.

I'm not sure what you're arguing. I know you did. I'm saying you
shouldn't have done it in this 1/3 patch, but rather in 3/3 or maybe
4/4...

johannes

Re: [PATCH v5 1/3] wifi: mac80211: Assign tx-stats to the proper link.

[Ben Greear]

On 7/8/25 07:46, Johannes Berg wrote:
> On Tue, 2025-07-08 at 07:32 -0700, Ben Greear wrote:
>>
>>
>> So I added new counters, and I wanted them named to indicate it is what is reported by
>> the driver, not something specified by the mac80211 stack.
> 
> I'm not sure what you're arguing. I know you did. I'm saying you
> shouldn't have done it in this 1/3 patch, but rather in 3/3 or maybe
> 4/4...

So you want this moved out of the first patch too?  That makes no sense
to me, but if you want it moved, let me know to what patch so I don't
have to guess and go through all of this again.

+		if (acked || noack_success) {
+			link_sta->tx_stats.rep_packets++;
+			link_sta->tx_stats.rep_bytes += skb->len;
+		}
+

Thanks,
Ben

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

Re: [PATCH v5 1/3] wifi: mac80211: Assign tx-stats to the proper link.

[Johannes Berg]

On Tue, 2025-07-08 at 08:16 -0700, Ben Greear wrote:
> On 7/8/25 07:46, Johannes Berg wrote:
> > On Tue, 2025-07-08 at 07:32 -0700, Ben Greear wrote:
> > > 
> > > 
> > > So I added new counters, and I wanted them named to indicate it is what is reported by
> > > the driver, not something specified by the mac80211 stack.
> > 
> > I'm not sure what you're arguing. I know you did. I'm saying you
> > shouldn't have done it in this 1/3 patch, but rather in 3/3 or maybe
> > 4/4...
> 
> So you want this moved out of the first patch too?  That makes no sense
> to me, but if you want it moved, let me know to what patch so I don't
> have to guess and go through all of this again.

Of _course_ I do, I don't know why this is controversial to you. Don't
actively _hide_ something in a patch that purports to change something
else entirely. This is a whole new feature, completely unrelated to
"assign tx-stats to the proper link".

johannes

Re: [PATCH v5 1/3] wifi: mac80211: Assign tx-stats to the proper link.

[Ben Greear]

On 7/8/25 08:26, Johannes Berg wrote:
> On Tue, 2025-07-08 at 08:16 -0700, Ben Greear wrote:
>> On 7/8/25 07:46, Johannes Berg wrote:
>>> On Tue, 2025-07-08 at 07:32 -0700, Ben Greear wrote:
>>>>
>>>>
>>>> So I added new counters, and I wanted them named to indicate it is what is reported by
>>>> the driver, not something specified by the mac80211 stack.
>>>
>>> I'm not sure what you're arguing. I know you did. I'm saying you
>>> shouldn't have done it in this 1/3 patch, but rather in 3/3 or maybe
>>> 4/4...
>>
>> So you want this moved out of the first patch too?  That makes no sense
>> to me, but if you want it moved, let me know to what patch so I don't
>> have to guess and go through all of this again.
> 
> Of _course_ I do, I don't know why this is controversial to you. Don't
> actively _hide_ something in a patch that purports to change something
> else entirely. This is a whole new feature, completely unrelated to
> "assign tx-stats to the proper link".

I feel we are not communicating well at all and that your complaint about
the patch is without technical merit.  Accusing me of trying to hide something
is just pure insult.  I am going to take a break from this,
and will re-post a new series later if I can find the motivation.
Anyone else that wants to deal with this is welcome to it.

Thanks,
Ben

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

Re: [PATCH v5 1/3] wifi: mac80211: Assign tx-stats to the proper link.

[Johannes Berg]

On Tue, 2025-07-08 at 10:32 -0700, Ben Greear wrote:
> 
> I feel we are not communicating well at all

Clearly.

>  and that your complaint about
> the patch is without technical merit.

https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
[...]
"Solve only one problem per patch."


Anyway, yeah, let's just take a break. I'll stop reviewing your patches,
maybe someone else will be motivated to have this discussion with you.

johannes