Received-SPF: pass (google.com: domain of linux-wireless+bounces-5319-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <14699537-99b2-e468-6a7b-7b721193400e@quicinc.com>
Date: Wed, 27 Mar 2024 15:39:28 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v2 1/2] wifi: cfg80211/mac80211: Add support to rx retry
 stats
To: Johannes Berg <johannes@sipsolutions.net>, <ath11k@lists.infradead.org>
CC: <linux-wireless@vger.kernel.org>
References: <20240319134522.4021062-1-quic_haric@quicinc.com>
 <20240319134522.4021062-2-quic_haric@quicinc.com>
 <d364e872eb29f03236630bab49a3243e2118ab22.camel@sipsolutions.net>
Content-Language: en-US
From: Hari Chandrakanthan <quic_haric@quicinc.com>
In-Reply-To: <d364e872eb29f03236630bab49a3243e2118ab22.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit


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