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

[Anilkumar Kolli <akolli@codeaurora.org>]

On 2018-09-03 15:43, Toke Høiland-Jørgensen wrote:
> Anilkumar Kolli <akolli@codeaurora.org> writes:
> 
>> On 2018-08-31 19:52, Toke Høiland-Jørgensen wrote:
>>> 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
>> 
>> The design:
>> Whenever radio transmits packet, firmware will record numbers of bytes
>> sent, MSDU’s sent, TX duration, AMPDU information, ACK fail, BA fail,
>> Rate at which packet is sent. This is recorded for 4 frames sent on 
>> that
>> peer. Once 4 frames are sent for that peer, TX packet event is sent to
>> ath10k driver with the recorded values. These rate values are updated 
>> to
>> mac80211 using ieee80211_tx_status().
> 
> So the values reported are the sums for all four packets? But the 
> latest
> value for rate information?
> 
> -Toke

Tx rate is updated for the 4th packet.

Thanks
Anil.