Re: [RFC v2] ath10k: report tx rate using ieee80211_tx_status()

["Toke Høiland-Jørgensen" <toke@toke.dk>]

Kalle Valo <kvalo@codeaurora.org> writes:

> Anilkumar Kolli <akolli@codeaurora.org> writes:
>
>> Mesh path metric needs txrate information from ieee80211_tx_status()
>> call but in ath10k there is no mechanism to report tx rate information
>> via ieee80211_tx_status(), the rate is only accessible via
>> sta_statiscs() op.
>>
>> Per peer stats has tx rate info available, this patch stores per peer
>> last tx rate and updates the tx rate info structures in tx completition.
>> The rate updated in ieee80211_tx_status() is not exactly for the last
>> transmitted frame instead the rate is from one of the previous frames.
>>
>> Per peer txrate information is updated through per peer statistics
>> and is available for QCA9888/QCA9984/QCA4019/QCA998X only
>>
>> Tested on QCA9984 with firmware-5.bin_10.4-3.5.3-00053
>> Tested on QCA998X with firmware-5.bin_10.2.4-1.0-00036
>>
>> Signed-off-by: Anilkumar Kolli <akolli@codeaurora.org>
>
> This is a patch from last March, full patch here:
>
> https://patchwork.kernel.org/patch/10267693/
>
>> @@ -119,6 +122,18 @@ int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
>>  			info->flags &= ~IEEE80211_TX_STAT_ACK;
>>  	}
>>  
>> +	if (sta) {
>> +		spin_lock_bh(&ar->data_lock);
>> +		arsta = (struct ath10k_sta *)sta->drv_priv;
>> +		info->status.rates[0].idx =
>> +			arsta->tx_info.status.rates[0].idx;
>> +		info->status.rates[0].count =
>> +			arsta->tx_info.status.rates[0].count;
>> +		info->status.rates[0].flags =
> <> +			arsta->tx_info.status.rates[0].flags;
>> +		spin_unlock_bh(&ar->data_lock);
>> +	}
>> +
>>  	ieee80211_tx_status(htt->ar->hw, msdu);
>>  	/* we do not own the msdu anymore */
>
> "is not exactly" is IMHO an understatement. What it means that with this
> patch ath10k reports the rate information from another frame instead of
> the current skb, because the firmware provides the rate information "out
> of band". A simple example to clarify:
>
>    Let's say ath10k transmits frames A, B and C. Then ath10k calls
>    ieee80211_tx_status() for frame C the rate information could be for
>    frame A, or it could be the other around for frame A status the rate
>    information is from frame C.
>
> In other words, there's no guarantee from what frame the rate
> information is from.
>
> Too me this feels like a bad idea but I'm not familiar enough with
> mac80211 to really comment on this. What kind of implications does this
> have for Mesh or ATF, for example? Adding Johannes and Toke to hear
> about their opinion about this.

I was under the impression that the values gathered (at least for
airtime) would be cumulative values? If it's just the airtime of a
single random frame, which is what I understand from your example, it's
not going to be terribly useful to provide ATF at least...

-Toke