Re: [PATCH v2 1/2] mac80211: extend current rate control tx status API

Thomas Hühn
2022-03-10T16:16:06Z

Hiho

> On 9. Mar 2022, at 21:38, Ben Greear <greearb@candelatech.com> wrote:
>
> On 3/9/22 11:57 AM, Jonas Jelonek wrote:
>> This patch adds the new struct ieee80211_rate_status and replaces
>> 'struct rate_info *rate' in ieee80211_tx_status with pointer and length
>> annotation.
>> The struct ieee80211_rate_status allows to:
>> (1) receive tx power status feedback for transmit power control (TPC)
>> per packet or packet retry
>> (2) dynamic mapping of wifi chip specific multi-rate retry (mrr)
>> chains with different lengths
>> (3) increase the limit of annotatable rate indices to support
>> IEEE802.11ac rate sets and beyond
>> ieee80211_tx_info, control and status buffer, and ieee80211_tx_rate
>> cannot be used to achieve these goals due to fixed size limitations.
>> Our new struct contains a struct rate_info to annotate the rate that was
>> used, retry count of the rate and tx power. It is intended for all
>> information related to RC and TPC that needs to be passed from driver to
>> mac80211 and its RC/TPC algorithms like Minstrel_HT. It corresponds to
>> one stage in an mrr. Multiple subsequent instances of this struct can be
>> included in struct ieee80211_tx_status via a pointer and a length variable.
>> Those instances can be allocated on-stack. The former reference to a single
>> instance of struct rate_info is replaced with our new annotation.
>> Further mandatory changes in status.c and mt76 driver due to the
>> removal of 'struct rate_info *rate' are also included.
>> status.c already uses the information in ieee80211_tx_status->rate in
>> radiotap, this is now changed to use ieee80211_rate_status->rate_idx.
>> mt76 driver already uses struct rate_info to pass the tx rate to status
>> path. It is now enclosed in an instance of struct ieee80211_rate_status
>> with default values for retry_count and tx_power. The latter should be
>> adjusted later to pass more accurate values.
>> Compile-Tested: current wireless-next tree with all flags on
>> Tested-on: Xiaomi 4A Gigabit (MediaTek MT7603E, MT7612E) with OpenWrt
>> Linux 5.10.83
>> Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
>> ---
>> drivers/net/wireless/mediatek/mt76/tx.c | 13 +++-
>> include/net/mac80211.h | 10 ++-
>> net/mac80211/status.c | 91 ++++++++++++++-----------
>> 3 files changed, 71 insertions(+), 43 deletions(-)
>> diff --git a/drivers/net/wireless/mediatek/mt76/tx.c b/drivers/net/wireless/mediatek/mt76/tx.c
>> index 6b8c9dc80542..ed3f3654999f 100644
>> --- a/drivers/net/wireless/mediatek/mt76/tx.c
>> +++ b/drivers/net/wireless/mediatek/mt76/tx.c
>> @@ -62,13 +62,20 @@ mt76_tx_status_unlock(struct mt76_dev *dev, struct sk_buff_head *list)
>> };
>> struct mt76_tx_cb *cb = mt76_tx_skb_cb(skb);
>> struct mt76_wcid *wcid;
>> + struct ieee80211_rate_status rate = {0};
>> wcid = rcu_dereference(dev->wcid[cb->wcid]);
>> if (wcid) {
>> status.sta = wcid_to_sta(wcid);
>> -
>> - if (status.sta)
>> - status.rate = &wcid->rate;
>> + if (status.sta) {
>> + rate.rate_idx = wcid->rate;
>> + rate.retry_count = 1;
>> + /* Default 0 for now, can be used by TPC algorithm */
>> + rate.tx_power = 0;
>> +
>> + status.rates = &rate;
>> + status.n_rates = 1;
>> + }
>> }
>> hw = mt76_tx_status_get_hw(dev, skb);
>> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
>> index c50221d7e82c..1e98ed04b446 100644
>> --- a/include/net/mac80211.h
>> +++ b/include/net/mac80211.h
>> @@ -1131,6 +1131,12 @@ ieee80211_info_get_tx_time_est(struct ieee80211_tx_info *info)
>> return info->tx_time_est << 2;
>> }
>> +struct ieee80211_rate_status {
>> + struct rate_info rate_idx;
>> + u8 retry_count;
>> + s8 tx_power;
>> +};
>
> Please document the units for tx_power. Many chips can support 1/2 db increments, for instance,
> so consider that for units... A zero txpower is still a valid number, so you probably need
> something other than 0 to be the 'default'. Like -128?

Certain 802.11a/g/n Atheros chips provide a 0,5dB tx-power step granularity, while Mediatek 802.11ac chips have 1dB or even 3dB step width. So I would argue that a 1dB step width is a good compromise as the common value for new tpc algorithms.

The ath9k chips I have used so far support a minimum tx-power of -5dBm (=0,32mW), Mediatek has a min of 0dBm (=1mW)… so I would argue to use 0dBm (=1mW) as common minimum tx-power value, as the any possible spatial reuse gain happens from 0dBm up to max tx-power.

>
> And, does 'retry_count' actually mean 'try_count'? So a single tx would be retry_count = 1?
> Please document that as well.
>
> Thanks,
> Ben

Greetings Thomas