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

[Anilkumar Kolli <akolli@codeaurora.org>]

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().

Agreed mac80211 is not getting the tx rate value for the last packet 
sent on the air, but this will be delivered to mac80211 at a later time 
for each transmitted packet.

Example:
Firmware TX for peer 1 -> pkt1 -> pkt2 -> pkt3 -> pkt4 -> pkt5 -> pkt6 
-> pkt7 -> pkt8 -> pkt9 -> pkt10
Driver updates                                         -> 
ieee80211_tx_status()        ->ieee80211_tx_status()

On each 4th packet sent in air, driver will update the tx rate.


Thanks
Anil.