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

Ben Greear
2022-03-09T20:38:44Z

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?

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

Thanks,
Ben

--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com