[PATCH v2 0/2] wifi: cfg80211/ath12k: Add support to rx retry stats

[PATCH v2 0/2] wifi: cfg80211/ath12k: Add support to rx retry stats

[Hari Chandrakanthan]

This patch series adds support to count station level rx retries.

wifi: cfg80211/mac80211: Add support to rx retry stats
wifi: ath12k: Add support to rx retry stats

Tested-on: QCN9274 hw1.0 PCI WLAN.WBE.1.2.1-00148-QCAHKSWPL_SILICONZ-1

Hari Chandrakanthan (2):
  wifi: cfg80211/mac80211: Add support to rx retry stats
  wifi: ath12k: Add support to rx retry stats

 drivers/net/wireless/ath/ath12k/core.h   | 1 +
 drivers/net/wireless/ath/ath12k/dp_mon.c | 8 ++++++++
 drivers/net/wireless/ath/ath12k/hal_rx.h | 7 ++++++-
 drivers/net/wireless/ath/ath12k/mac.c    | 3 +++
 include/net/cfg80211.h                   | 2 ++
 include/uapi/linux/nl80211.h             | 3 +++
 net/mac80211/rx.c                        | 6 ++++++
 net/mac80211/sta_info.c                  | 5 +++++
 net/mac80211/sta_info.h                  | 1 +
 net/wireless/nl80211.c                   | 1 +
 10 files changed, 36 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH v2 1/2] wifi: cfg80211/mac80211: Add support to rx retry stats

[Hari Chandrakanthan]

Add support to count station level rx retries.
It denotes the number of data frames(MPDUs) with rx retry bit set.

The rx retry stats helps in understanding the medium
during UL transmission.

The counting logic of rx retry stats in the mac80211 rx path accounts the
data frames for which decap is not offloaded.

Signed-off-by: Hari Chandrakanthan <quic_haric@quicinc.com>
---
v2:
Added mac80211 support to count the rx retry stats
---
 include/net/cfg80211.h       | 2 ++
 include/uapi/linux/nl80211.h | 3 +++
 net/mac80211/rx.c            | 6 ++++++
 net/mac80211/sta_info.c      | 5 +++++
 net/mac80211/sta_info.h      | 1 +
 net/wireless/nl80211.c       | 1 +
 6 files changed, 18 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 2e2be4fd2bb6..21400bcc0646 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2049,6 +2049,7 @@ struct cfg80211_tid_stats {
  * @rxrate: current unicast bitrate to this station
  * @rx_packets: packets (MSDUs & MMPDUs) received from this station
  * @tx_packets: packets (MSDUs & MMPDUs) transmitted to this station
+ * @rx_retries: number of rx packets(MPDUs) from this station with retry bit set.
  * @tx_retries: cumulative retry counts (MPDUs)
  * @tx_failed: number of failed transmissions (MPDUs) (retries exceeded, no ACK)
  * @rx_dropped_misc:  Dropped for un-specified reason.
@@ -2129,6 +2130,7 @@ struct station_info {
 	struct rate_info rxrate;
 	u32 rx_packets;
 	u32 tx_packets;
+	u32 rx_retries;
 	u32 tx_retries;
 	u32 tx_failed;
 	u32 rx_dropped_misc;
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index f23ecbdd84a2..2e31909bf8af 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -3885,6 +3885,8 @@ enum nl80211_sta_bss_param {
  *	of STA's association
  * @NL80211_STA_INFO_CONNECTED_TO_AS: set to true if STA has a path to a
  *	authentication server (u8, 0 or 1)
+ * @NL80211_STA_INFO_RX_RETRIES: number of rx packets(MPDUs) from this station
+ *	with retry bit set (u32)
  * @__NL80211_STA_INFO_AFTER_LAST: internal
  * @NL80211_STA_INFO_MAX: highest possible station info attribute
  */
@@ -3933,6 +3935,7 @@ enum nl80211_sta_info {
 	NL80211_STA_INFO_AIRTIME_LINK_METRIC,
 	NL80211_STA_INFO_ASSOC_AT_BOOTTIME,
 	NL80211_STA_INFO_CONNECTED_TO_AS,
+	NL80211_STA_INFO_RX_RETRIES,
 
 	/* keep last */
 	__NL80211_STA_INFO_AFTER_LAST,
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index c1f850138405..f8d408ac6742 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -3191,6 +3191,9 @@ ieee80211_rx_h_data(struct ieee80211_rx_data *rx)
 	if (!ieee80211_frame_allowed(rx, fc))
 		return RX_DROP_MONITOR;
 
+	if (ieee80211_has_retry(hdr->frame_control) && rx->sta)
+		rx->link_sta->rx_stats.rx_retries++;
+
 	/* directly handle TDLS channel switch requests/responses */
 	if (unlikely(((struct ethhdr *)rx->skb->data)->h_proto ==
 						cpu_to_be16(ETH_P_TDLS))) {
@@ -4976,6 +4979,9 @@ static bool ieee80211_invoke_fast_rx(struct ieee80211_rx_data *rx,
 		goto drop;
 	}
 
+	if (ieee80211_has_retry(hdr->frame_control) && rx->sta)
+		rx->link_sta->rx_stats.rx_retries++;
+
 	ieee80211_rx_8023(rx, fast_rx, orig_len);
 
 	return true;
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index da5fdd6f5c85..b04092b6ea64 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -2653,6 +2653,11 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo,
 		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_TX_RETRIES);
 	}
 
+	if (!(sinfo->filled & BIT_ULL(NL80211_STA_INFO_RX_RETRIES))) {
+		sinfo->rx_retries = sta->deflink.rx_stats.rx_retries;
+		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_RX_RETRIES);
+	}
+
 	if (!(sinfo->filled & BIT_ULL(NL80211_STA_INFO_TX_FAILED))) {
 		sinfo->tx_failed = sta->deflink.status_stats.retry_failed;
 		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_TX_FAILED);
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index a52fb76386d0..a4c47e3f0d5a 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -436,6 +436,7 @@ struct ieee80211_sta_rx_stats {
 	struct u64_stats_sync syncp;
 	u64 bytes;
 	u64 msdu[IEEE80211_NUM_TIDS + 1];
+	u32 rx_retries;
 };
 
 /*
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index b4edba6b0b7b..fb643d3061c5 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -6626,6 +6626,7 @@ static int nl80211_send_station(struct sk_buff *msg, u32 cmd, u32 portid,
 
 	PUT_SINFO(RX_PACKETS, rx_packets, u32);
 	PUT_SINFO(TX_PACKETS, tx_packets, u32);
+	PUT_SINFO(RX_RETRIES, rx_retries, u32);
 	PUT_SINFO(TX_RETRIES, tx_retries, u32);
 	PUT_SINFO(TX_FAILED, tx_failed, u32);
 	PUT_SINFO(EXPECTED_THROUGHPUT, expected_throughput, u32);
-- 
2.34.1

Re: [PATCH v2 1/2] wifi: cfg80211/mac80211: Add support to rx retry stats

[Jeff Johnson]

On 3/19/2024 6:45 AM, Hari Chandrakanthan wrote:
> Add support to count station level rx retries.
> It denotes the number of data frames(MPDUs) with rx retry bit set.
> 
> The rx retry stats helps in understanding the medium
> during UL transmission.
> 
> The counting logic of rx retry stats in the mac80211 rx path accounts the
> data frames for which decap is not offloaded.
> 
> Signed-off-by: Hari Chandrakanthan <quic_haric@quicinc.com>

Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>

Re: [PATCH v2 1/2] wifi: cfg80211/mac80211: Add support to rx retry stats

[Johannes Berg]

On Tue, 2024-03-19 at 19:15 +0530, Hari Chandrakanthan wrote:
> Add support to count station level rx retries.

Should the subject say "for ... stats"?

> +++ b/net/mac80211/sta_info.c
> @@ -2653,6 +2653,11 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo,
>  		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_TX_RETRIES);
>  	}
>  
> +	if (!(sinfo->filled & BIT_ULL(NL80211_STA_INFO_RX_RETRIES))) {
> +		sinfo->rx_retries = sta->deflink.rx_stats.rx_retries;
> +		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_RX_RETRIES);
> +	}

The use of deflink here seems ... questionable?

I know we've not really done any stats properly here for link STA
(patches welcome), but I guess this could be a first one that at least
sums up all the links like all of these should, and then find a way to
expose per-link as well?

Although possibly we should just expose per-link to cfg80211, and then
have cfg80211 sum up for the MLD representation...


Either way, seems odd to add something now that absolutely cannot work
for MLO?


johannes

Re: [PATCH v2 1/2] wifi: cfg80211/mac80211: Add support to rx retry stats

[Hari Chandrakanthan]

On 3/25/2024 9:13 PM, Johannes Berg wrote:
> On Tue, 2024-03-19 at 19:15 +0530, Hari Chandrakanthan wrote:
>> Add support to count station level rx retries.
> Should the subject say "for ... stats"?
>
>> +++ b/net/mac80211/sta_info.c
>> @@ -2653,6 +2653,11 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo,
>>   		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_TX_RETRIES);
>>   	}
>>   
>> +	if (!(sinfo->filled & BIT_ULL(NL80211_STA_INFO_RX_RETRIES))) {
>> +		sinfo->rx_retries = sta->deflink.rx_stats.rx_retries;
>> +		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_RX_RETRIES);
>> +	}
> The use of deflink here seems ... questionable?
>
> I know we've not really done any stats properly here for link STA
> (patches welcome), but I guess this could be a first one that at least
> sums up all the links like all of these should, and then find a way to
> expose per-link as well?
>
> Although possibly we should just expose per-link to cfg80211, and then
> have cfg80211 sum up for the MLD representation...
>
>
> Either way, seems odd to add something now that absolutely cannot work
> for MLO?
>
ok.

Fields such packet count, retries etc can be summed up for the MLD 
representation and the existing NL
attribute can be used for exposing the summed up value.

But there are fields such as signal avg, bitrate etc which cannot be 
summed up.
Should we expose such fields of each link STA through NL?

Sample station dump log for reference:
         inactive time:  271110 ms
         rx bytes:       1129
         rx packets:     13
         tx bytes:       219
         tx packets:     3
         tx retries:     38
         tx failed:      0
         rx drop misc:   0
         signal:         -16 dBm
         signal avg:     -20 dBm
         tx bitrate:     6.0 MBit/s
         tx duration:    452 us
         rx bitrate:     260.0 MBit/s VHT-MCS 6 short GI VHT-NSS 4
         rx duration:    792 us
         last ack signal:0 dBm
         avg ack signal: 0 dBm
         authorized:     yes
         authenticated:  yes
         associated:     yes
         preamble:       long
         WMM/WME:        yes
         MFP:            no
         TDLS peer:      no
         DTIM period:    2
         beacon interval:100
         short slot time:yes
         connected time: 869 seconds
         associated at [boottime]:       1040062.600s
         associated at:  1695979678173 ms
         current time:   1695980547138 ms

> johannes

Re: [PATCH v2 1/2] wifi: cfg80211/mac80211: Add support to rx retry stats

[Johannes Berg]

On Wed, 2024-03-27 at 15:39 +0530, Hari Chandrakanthan wrote:

> Fields such packet count, retries etc can be summed up for the MLD 
> representation and the existing NL attribute can be used for exposing
> the summed up value.

I think the existing attributes can also be used for per-link STA?

I'm kind of imagining that - once we actually do all of this properly -
the representation in nl80211 would be something like


STA 00:00:00:00:00:00
 - TX bytes: 123456
 - RX bytes: 654321
 - signal avg: -60 dBm     // picking the best of all links?
 ...
 - LINK 00:00:00:00:00:01
    - link ID: 10
    - TX bytes: 100000
    - RX bytes: 600000
    - signal avg: -60 dBm
 - LINK 00:00:00:00:00:02
    - link ID: 11
    - TX bytes: 23456
    - RX bytes: 54321
    - signal avg: -70 dBm

etc.

> But there are fields such as signal avg, bitrate etc which cannot be 
> summed up.

Right, but I guess we can pick 'best' for those, to at least have a
value? Or we could just not emit those attributes I guess, but not sure
if that's then all that useful?

> Should we expose such fields of each link STA through NL?

All of them, I guess?

I'm also imagining that we change the API from cfg80211 to the drivers
to get the *link* STA information, and do the summing up and/or "best"
selection there in cfg80211 itself. However, I am prepared to accept the
possibility that we may do _both_ in the API, if not all drivers can
even do all of the statistics per link. We should probably still have
the link STAs in the statistics in nl80211, but then they may not be
populated?

johannes

Re: [PATCH v2 1/2] wifi: cfg80211/mac80211: Add support to rx retry stats

[Jeff Johnson]

On 3/27/2024 3:32 AM, Johannes Berg wrote:
> On Wed, 2024-03-27 at 15:39 +0530, Hari Chandrakanthan wrote:
> 
>> Fields such packet count, retries etc can be summed up for the MLD 
>> representation and the existing NL attribute can be used for exposing
>> the summed up value.
> 
> I think the existing attributes can also be used for per-link STA?
> 
> I'm kind of imagining that - once we actually do all of this properly -
> the representation in nl80211 would be something like
> 
> 
> STA 00:00:00:00:00:00
>  - TX bytes: 123456
>  - RX bytes: 654321
>  - signal avg: -60 dBm     // picking the best of all links?
>  ...
>  - LINK 00:00:00:00:00:01
>     - link ID: 10
>     - TX bytes: 100000
>     - RX bytes: 600000
>     - signal avg: -60 dBm
>  - LINK 00:00:00:00:00:02
>     - link ID: 11
>     - TX bytes: 23456
>     - RX bytes: 54321
>     - signal avg: -70 dBm
> 
> etc.
> 
>> But there are fields such as signal avg, bitrate etc which cannot be 
>> summed up.
> 
> Right, but I guess we can pick 'best' for those, to at least have a
> value? Or we could just not emit those attributes I guess, but not sure
> if that's then all that useful?
> 
>> Should we expose such fields of each link STA through NL?
> 
> All of them, I guess?
> 
> I'm also imagining that we change the API from cfg80211 to the drivers
> to get the *link* STA information, and do the summing up and/or "best"
> selection there in cfg80211 itself. However, I am prepared to accept the
> possibility that we may do _both_ in the API, if not all drivers can
> even do all of the statistics per link. We should probably still have
> the link STAs in the statistics in nl80211, but then they may not be
> populated?

First remember that there are a lot of statistics, and each driver is free to
return as many or as few as they support, indicating the ones they are
returning using the "filled" bitmap. I would expect MLO-capable drivers to
provide the same information on a per-link basis that they previously provided
on a per-interface basis, but the "filled" bitmap leaves that to the drivers.

But I think a fundamental question needs to be answered: To what extent do we
need to support legacy userspace applications that are not MLO-aware?

My expectation is that MLO-aware applications only need the per-link
information, and from that they can derive their own "aggregate" of the
per-link information. So to support that we'd need per-link nesting of the
associated attributes. And if we don't need to support legacy userspace we
could completely remove populating the top-level statistic attributes. Non-MLO
interfaces would have a single link nest that contains the same information
that is now in the top-level of the NL message.

But if we need to support legacy userspace then we would indeed need to
continue to populate those top-level attributes with some form of aggregate data.

And even for the MLO-aware case there is the issue of how do we want to handle
the case that links may come and go, and hence aggregate counters would appear
to have huge fluctuations in values when links are added or removed if the
aggregate values are only calculated by adding the values from the
currently-attached links.

/jeff

Re: [PATCH v2 1/2] wifi: cfg80211/mac80211: Add support to rx retry stats

[Johannes Berg]

On Wed, 2024-03-27 at 08:02 -0700, Jeff Johnson wrote:
> > I'm also imagining that we change the API from cfg80211 to the drivers
> > to get the *link* STA information, and do the summing up and/or "best"
> > selection there in cfg80211 itself. However, I am prepared to accept the
> > possibility that we may do _both_ in the API, if not all drivers can
> > even do all of the statistics per link. We should probably still have
> > the link STAs in the statistics in nl80211, but then they may not be
> > populated?
> 
> First remember that there are a lot of statistics, and each driver is free to
> return as many or as few as they support, indicating the ones they are
> returning using the "filled" bitmap. 

Yes, I'd think we want to use the same data structure for both, though
setting something in *both* links and *mld* would (should) be an error.

> I would expect MLO-capable drivers to
> provide the same information on a per-link basis that they previously provided
> on a per-interface basis, but the "filled" bitmap leaves that to the drivers.

Unless we don't actually ask the drivers at the MLD level if (the
station is an MLD). But I suspect we will have to do both, ask for MLD-
level stats and link-level stats.

> But I think a fundamental question needs to be answered: To what extent do we
> need to support legacy userspace applications that are not MLO-aware?

I have no idea who even uses this and how :-) But I guess things like
NetworkManager might, even just to show some signal strength values
etc.?

> My expectation is that MLO-aware applications only need the per-link
> information, and from that they can derive their own "aggregate" of the
> per-link information.

Agree, though it'll be a long time until all applications are MLO-aware?
Unless there aren't many using it, but ...

> So to support that we'd need per-link nesting of the
> associated attributes.

Sure, that's a given.

> And if we don't need to support legacy userspace we
> could completely remove populating the top-level statistic attributes. Non-MLO
> interfaces would have a single link nest that contains the same information
> that is now in the top-level of the NL message.
> 
> But if we need to support legacy userspace then we would indeed need to
> continue to populate those top-level attributes with some form of aggregate data.

I think we probably have to.

> And even for the MLO-aware case there is the issue of how do we want to handle
> the case that links may come and go, and hence aggregate counters would appear
> to have huge fluctuations in values when links are added or removed if the
> aggregate values are only calculated by adding the values from the
> currently-attached links.

Good point, when they're really removed we'd want to probably keep that
value as a bias for the MLD-level stats?

johannes

Re: [PATCH v2 1/2] wifi: cfg80211/mac80211: Add support to rx retry stats

[Hari Chandrakanthan]

On 3/27/2024 8:37 PM, Johannes Berg wrote:
> On Wed, 2024-03-27 at 08:02 -0700, Jeff Johnson wrote:
>>> I'm also imagining that we change the API from cfg80211 to the drivers
>>> to get the *link* STA information, and do the summing up and/or "best"
>>> selection there in cfg80211 itself. However, I am prepared to accept the
>>> possibility that we may do _both_ in the API, if not all drivers can
>>> even do all of the statistics per link. We should probably still have
>>> the link STAs in the statistics in nl80211, but then they may not be
>>> populated?
>> First remember that there are a lot of statistics, and each driver is free to
>> return as many or as few as they support, indicating the ones they are
>> returning using the "filled" bitmap.
> Yes, I'd think we want to use the same data structure for both, though
> setting something in *both* links and *mld* would (should) be an error.
The statistics can be populated by driver or mac80211.(say tx retries, 
tx packets etc)
So we should also change the existing stats update in mac80211 on link 
STA basis instead of deflink?
>> I would expect MLO-capable drivers to
>> provide the same information on a per-link basis that they previously provided
>> on a per-interface basis, but the "filled" bitmap leaves that to the drivers.
> Unless we don't actually ask the drivers at the MLD level if (the
> station is an MLD). But I suspect we will have to do both, ask for MLD-
> level stats and link-level stats.
>
>> But I think a fundamental question needs to be answered: To what extent do we
>> need to support legacy userspace applications that are not MLO-aware?
> I have no idea who even uses this and how :-) But I guess things like
> NetworkManager might, even just to show some signal strength values
> etc.?
>
>> My expectation is that MLO-aware applications only need the per-link
>> information, and from that they can derive their own "aggregate" of the
>> per-link information.
> Agree, though it'll be a long time until all applications are MLO-aware?
> Unless there aren't many using it, but ...
>
>> So to support that we'd need per-link nesting of the
>> associated attributes.
> Sure, that's a given.
>
>> And if we don't need to support legacy userspace we
>> could completely remove populating the top-level statistic attributes. Non-MLO
>> interfaces would have a single link nest that contains the same information
>> that is now in the top-level of the NL message.
>>
>> But if we need to support legacy userspace then we would indeed need to
>> continue to populate those top-level attributes with some form of aggregate data.
> I think we probably have to.
>
>> And even for the MLO-aware case there is the issue of how do we want to handle
>> the case that links may come and go, and hence aggregate counters would appear
>> to have huge fluctuations in values when links are added or removed if the
>> aggregate values are only calculated by adding the values from the
>> currently-attached links.
> Good point, when they're really removed we'd want to probably keep that
> value as a bias for the MLD-level stats?

ok. Then the statistics value in MLD STA would be bias + summed up value 
of currently alive links?

On the same line , ethtool stats(*interface level stats*) in 
mac80211(ieee80211_get_stats())
computes the stats by summing up the current STA statistics.
Here stations can come and go and the ethtool stats may not reflect the 
total packets transmitted or received by the interface.
It just reflects the summed up value of current alive stations.

Since these two problems are similar (ethtool stats and MLD stats 
calculation),
would like to understand what type of value would be more useful to user?

> johannes

Re: [PATCH v2 1/2] wifi: cfg80211/mac80211: Add support to rx retry stats

[Johannes Berg]

On Thu, 2024-03-28 at 22:49 +0530, Hari Chandrakanthan wrote:
> On 3/27/2024 8:37 PM, Johannes Berg wrote:
> > On Wed, 2024-03-27 at 08:02 -0700, Jeff Johnson wrote:
> > > > I'm also imagining that we change the API from cfg80211 to the drivers
> > > > to get the *link* STA information, and do the summing up and/or "best"
> > > > selection there in cfg80211 itself. However, I am prepared to accept the
> > > > possibility that we may do _both_ in the API, if not all drivers can
> > > > even do all of the statistics per link. We should probably still have
> > > > the link STAs in the statistics in nl80211, but then they may not be
> > > > populated?
> > > First remember that there are a lot of statistics, and each driver is free to
> > > return as many or as few as they support, indicating the ones they are
> > > returning using the "filled" bitmap.
> > Yes, I'd think we want to use the same data structure for both, though
> > setting something in *both* links and *mld* would (should) be an error.
> The statistics can be populated by driver or mac80211.(say tx retries, 
> tx packets etc)

Right.

> So we should also change the existing stats update in mac80211 on link 
> STA basis instead of deflink?

Absolutely, we need to do that, it's been on my list forever, since the
early MLO work... I'm a bit torn between not wanting you to have to do
all that work (even if we know that we'll have to do it) and on the
other hand not wanting to make it worse with more statistics now ...
There's no good middle ground here though now.

> > Good point, when they're really removed we'd want to probably keep that
> > value as a bias for the MLD-level stats?
> 
> ok. Then the statistics value in MLD STA would be bias + summed up value 
> of currently alive links?

I guess? But I'm not sure where we'd actually _keep_ the bias values.
Maybe give up on that idea that cfg80211 could sum it all up, and just
require the underlying driver (or mac80211) to report both per-link and
total stats, where available? That way, mac80211 could keep the bias
somewhere and just add it to the total before reporting _that_.

> On the same line , ethtool stats(*interface level stats*) in 
> mac80211(ieee80211_get_stats())
> computes the stats by summing up the current STA statistics.
> Here stations can come and go and the ethtool stats may not reflect the 
> total packets transmitted or received by the interface.
> It just reflects the summed up value of current alive stations.

Yeah ... I know Ben loves it, but personally I kind of think of ethtool
as a dead legacy interface for this respect, it just doesn't have the
ability to reflect the required structures/hierarchy/etc. well since
it's just a flat list. Sure you can structure the names in some way, but
it's iffy at best. I'd just ignore that for now. If we have better
statistics to nl80211, we can always make ethtool support on top of
that, perhaps even moving it to cfg80211, if we even need more support
there. I'm not hugely in favour, but if it stays contained somewhere and
consumes existing APIs I'm OK with it.

> Since these two problems are similar (ethtool stats and MLD stats 
> calculation),
> would like to understand what type of value would be more useful to user?
> 

What do you mean by that?

johannes

Re: [PATCH v2 1/2] wifi: cfg80211/mac80211: Add support to rx retry stats

[Ben Greear]

On 3/28/24 10:54, Johannes Berg wrote:
> On Thu, 2024-03-28 at 22:49 +0530, Hari Chandrakanthan wrote:
>> On 3/27/2024 8:37 PM, Johannes Berg wrote:
>>> On Wed, 2024-03-27 at 08:02 -0700, Jeff Johnson wrote:
>>>>> I'm also imagining that we change the API from cfg80211 to the drivers
>>>>> to get the *link* STA information, and do the summing up and/or "best"
>>>>> selection there in cfg80211 itself. However, I am prepared to accept the
>>>>> possibility that we may do _both_ in the API, if not all drivers can
>>>>> even do all of the statistics per link. We should probably still have
>>>>> the link STAs in the statistics in nl80211, but then they may not be
>>>>> populated?
>>>> First remember that there are a lot of statistics, and each driver is free to
>>>> return as many or as few as they support, indicating the ones they are
>>>> returning using the "filled" bitmap.
>>> Yes, I'd think we want to use the same data structure for both, though
>>> setting something in *both* links and *mld* would (should) be an error.
>> The statistics can be populated by driver or mac80211.(say tx retries,
>> tx packets etc)
> 
> Right.
> 
>> So we should also change the existing stats update in mac80211 on link
>> STA basis instead of deflink?
> 
> Absolutely, we need to do that, it's been on my list forever, since the
> early MLO work... I'm a bit torn between not wanting you to have to do
> all that work (even if we know that we'll have to do it) and on the
> other hand not wanting to make it worse with more statistics now ...
> There's no good middle ground here though now.
> 
>>> Good point, when they're really removed we'd want to probably keep that
>>> value as a bias for the MLD-level stats?
>>
>> ok. Then the statistics value in MLD STA would be bias + summed up value
>> of currently alive links?
> 
> I guess? But I'm not sure where we'd actually _keep_ the bias values.
> Maybe give up on that idea that cfg80211 could sum it all up, and just
> require the underlying driver (or mac80211) to report both per-link and
> total stats, where available? That way, mac80211 could keep the bias
> somewhere and just add it to the total before reporting _that_.
> 
>> On the same line , ethtool stats(*interface level stats*) in
>> mac80211(ieee80211_get_stats())
>> computes the stats by summing up the current STA statistics.
>> Here stations can come and go and the ethtool stats may not reflect the
>> total packets transmitted or received by the interface.
>> It just reflects the summed up value of current alive stations.
> 
> Yeah ... I know Ben loves it, but personally I kind of think of ethtool
> as a dead legacy interface for this respect, it just doesn't have the
> ability to reflect the required structures/hierarchy/etc. well since
> it's just a flat list. Sure you can structure the names in some way, but
> it's iffy at best. I'd just ignore that for now. If we have better
> statistics to nl80211, we can always make ethtool support on top of
> that, perhaps even moving it to cfg80211, if we even need more support
> there. I'm not hugely in favour, but if it stays contained somewhere and
> consumes existing APIs I'm OK with it.

I have my own hacks to make ethtool be minimally useful for MLO,
but I think it can be revisited later once the over-all MLO
architecture is solidified.

As Hari says, even without MLO, the AP's ethtool stats are not well
defined (it works better for STA vdevs).  Something that can also be fixed later after MLO calms down,
but my initial thought is to keep per-vdev counters that accumulate
any per-station counters and then the ethtool logic would not
try to sum the current stations but rather just grab the accumulated stats.

Thanks,
Ben

> 
>> Since these two problems are similar (ethtool stats and MLD stats
>> calculation),
>> would like to understand what type of value would be more useful to user?
>>
> 
> What do you mean by that?
> 
> johannes
> 

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

[PATCH v2 2/2] wifi: ath12k: Add support to rx retry stats

[Hari Chandrakanthan]

Add support to count station level rx retries.
It denotes the number of data frames(MPDUs) received with rx retry bit
set.

The rx retry stats helps in understanding the medium during
UL transmission.

Tested-on: QCN9274 hw1.0 PCI WLAN.WBE.1.2.1-00148-QCAHKSWPL_SILICONZ-1

Signed-off-by: Hari Chandrakanthan <quic_haric@quicinc.com>
---
v2: Rebased the patch
---
 drivers/net/wireless/ath/ath12k/core.h   | 1 +
 drivers/net/wireless/ath/ath12k/dp_mon.c | 8 ++++++++
 drivers/net/wireless/ath/ath12k/hal_rx.h | 7 ++++++-
 drivers/net/wireless/ath/ath12k/mac.c    | 3 +++
 4 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
index 97e5a0ccd233..8ffa9feb58d9 100644
--- a/drivers/net/wireless/ath/ath12k/core.h
+++ b/drivers/net/wireless/ath/ath12k/core.h
@@ -425,6 +425,7 @@ struct ath12k_sta {
 	struct ath12k_rx_peer_stats *rx_stats;
 	struct ath12k_wbm_tx_stats *wbm_tx_stats;
 	u32 bw_prev;
+	u32 rx_retries;
 };
 
 #define ATH12K_MIN_5G_FREQ 4150
diff --git a/drivers/net/wireless/ath/ath12k/dp_mon.c b/drivers/net/wireless/ath/ath12k/dp_mon.c
index 2d56913a75d0..eb2f156b3e82 100644
--- a/drivers/net/wireless/ath/ath12k/dp_mon.c
+++ b/drivers/net/wireless/ath/ath12k/dp_mon.c
@@ -774,9 +774,11 @@ ath12k_dp_mon_rx_parse_status_tlv(struct ath12k_base *ab,
 		u16 peer_id;
 
 		info[1] = __le32_to_cpu(mpdu_start->info1);
+		info[3] = __le32_to_cpu(mpdu_start->info3);
 		peer_id = u32_get_bits(info[1], HAL_RX_MPDU_START_INFO1_PEERID);
 		if (peer_id)
 			ppdu_info->peer_id = peer_id;
+		ppdu_info->mpdu_retry = info[3] & HAL_RX_MPDU_START_INFO3_MPDU_RETRY;
 
 		ppdu_info->mpdu_len += u32_get_bits(info[1],
 						    HAL_RX_MPDU_START_INFO2_MPDU_LEN);
@@ -785,6 +787,9 @@ ath12k_dp_mon_rx_parse_status_tlv(struct ath12k_base *ab,
 			ppdu_info->userid = userid;
 			ppdu_info->ampdu_id[userid] =
 				u32_get_bits(info[0], HAL_RX_MPDU_START_INFO1_PEERID);
+			ppdu_info->userstats[userid].mpdu_retry =
+				info[3] & HAL_RX_MPDU_START_INFO3_MPDU_RETRY;
+
 		}
 
 		mon_mpdu = kzalloc(sizeof(*mon_mpdu), GFP_ATOMIC);
@@ -2209,6 +2214,8 @@ static void ath12k_dp_mon_rx_update_peer_su_stats(struct ath12k *ar,
 	struct ath12k_rx_peer_stats *rx_stats = arsta->rx_stats;
 	u32 num_msdu;
 
+	arsta->rx_retries  += ppdu_info->mpdu_retry;
+
 	if (!rx_stats)
 		return;
 
@@ -2378,6 +2385,7 @@ ath12k_dp_mon_rx_update_user_stats(struct ath12k *ar,
 
 	arsta = ath12k_sta_to_arsta(peer->sta);
 	rx_stats = arsta->rx_stats;
+	arsta->rx_retries = user_stats->mpdu_retry;
 
 	if (!rx_stats)
 		return;
diff --git a/drivers/net/wireless/ath/ath12k/hal_rx.h b/drivers/net/wireless/ath/ath12k/hal_rx.h
index 095216eabc01..ea2b89896abd 100644
--- a/drivers/net/wireless/ath/ath12k/hal_rx.h
+++ b/drivers/net/wireless/ath/ath12k/hal_rx.h
@@ -143,6 +143,7 @@ struct hal_rx_user_status {
 	u32 mpdu_fcs_ok_bitmap[HAL_RX_NUM_WORDS_PER_PPDU_BITMAP];
 	u32 mpdu_ok_byte_count;
 	u32 mpdu_err_byte_count;
+	bool mpdu_retry;
 };
 
 #define HAL_MAX_UL_MU_USERS	37
@@ -230,6 +231,7 @@ struct hal_rx_mon_ppdu_info {
 	bool first_msdu_in_mpdu;
 	bool is_ampdu;
 	u8 medium_prot_type;
+	bool mpdu_retry;
 };
 
 #define HAL_RX_PPDU_START_INFO0_PPDU_ID		GENMASK(15, 0)
@@ -447,10 +449,13 @@ struct hal_rx_phyrx_rssi_legacy_info {
 #define HAL_RX_MPDU_START_INFO0_PPDU_ID	GENMASK(31, 16)
 #define HAL_RX_MPDU_START_INFO1_PEERID	GENMASK(31, 16)
 #define HAL_RX_MPDU_START_INFO2_MPDU_LEN GENMASK(13, 0)
+#define HAL_RX_MPDU_START_INFO3_MPDU_RETRY	BIT(19)
 struct hal_rx_mpdu_start {
 	__le32 info0;
 	__le32 info1;
-	__le32 rsvd1[11];
+	__le32 rsvd1[8];
+	__le32 info3;
+	__le32 rsvd3[2];
 	__le32 info2;
 	__le32 rsvd2[9];
 } __packed;
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 52a5fb8b03e9..f050c5079e80 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -7467,6 +7467,9 @@ static void ath12k_mac_op_sta_statistics(struct ieee80211_hw *hw,
 	sinfo->txrate.flags = arsta->txrate.flags;
 	sinfo->filled |= BIT_ULL(NL80211_STA_INFO_TX_BITRATE);
 
+	sinfo->rx_retries = arsta->rx_retries;
+	sinfo->filled |= BIT_ULL(NL80211_STA_INFO_RX_RETRIES);
+
 	/* TODO: Use real NF instead of default one. */
 	sinfo->signal = arsta->rssi_comb + ATH12K_DEFAULT_NOISE_FLOOR;
 	sinfo->filled |= BIT_ULL(NL80211_STA_INFO_SIGNAL);
-- 
2.34.1

Re: [PATCH v2 2/2] wifi: ath12k: Add support to rx retry stats

[Jeff Johnson]

On 3/19/2024 6:45 AM, Hari Chandrakanthan wrote:
> Add support to count station level rx retries.
> It denotes the number of data frames(MPDUs) received with rx retry bit
> set.
> 
> The rx retry stats helps in understanding the medium during
> UL transmission.
> 
> Tested-on: QCN9274 hw1.0 PCI WLAN.WBE.1.2.1-00148-QCAHKSWPL_SILICONZ-1
> 
> Signed-off-by: Hari Chandrakanthan <quic_haric@quicinc.com>
Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>

Re: [PATCH v2 0/2] wifi: cfg80211/ath12k: Add support to rx retry stats

[Jeff Johnson]

On 3/19/2024 6:45 AM, Hari Chandrakanthan wrote:
> This patch series adds support to count station level rx retries.
> 
> wifi: cfg80211/mac80211: Add support to rx retry stats
> wifi: ath12k: Add support to rx retry stats
> 
> Tested-on: QCN9274 hw1.0 PCI WLAN.WBE.1.2.1-00148-QCAHKSWPL_SILICONZ-1
> 
> Hari Chandrakanthan (2):
>   wifi: cfg80211/mac80211: Add support to rx retry stats
>   wifi: ath12k: Add support to rx retry stats
> 
>  drivers/net/wireless/ath/ath12k/core.h   | 1 +
>  drivers/net/wireless/ath/ath12k/dp_mon.c | 8 ++++++++
>  drivers/net/wireless/ath/ath12k/hal_rx.h | 7 ++++++-
>  drivers/net/wireless/ath/ath12k/mac.c    | 3 +++
>  include/net/cfg80211.h                   | 2 ++
>  include/uapi/linux/nl80211.h             | 3 +++
>  net/mac80211/rx.c                        | 6 ++++++
>  net/mac80211/sta_info.c                  | 5 +++++
>  net/mac80211/sta_info.h                  | 1 +
>  net/wireless/nl80211.c                   | 1 +
>  10 files changed, 36 insertions(+), 1 deletion(-)
> 

Incorrectly sent to the ath11k list and not the ath12k list.
But please don't repost just due to that

cc'ing the ath12k list as an FYI