On Tue, 2022-09-20 at 12:40 +0200, Jonas Jelonek wrote:
> Enable RC_TABLE in hwsim for TPC support and replace the
> ieee80211_tx_status_irqsafe calls with regular ieee80211_tx_status_ext
> calls to be able to pass additional information, i.e., tx-power.
> Add some variables, members and functions in both tx control and tx
> status path to pass and process tx-power.
>
> Signed-off-by: Jonas Jelonek <jelonek.jonas@xxxxxxxxx>
> ---
> drivers/net/wireless/mac80211_hwsim.c | 175 ++++++++++++++++++++++++--
> drivers/net/wireless/mac80211_hwsim.h | 1 +
> 2 files changed, 168 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
> index df51b5b1f171..a56fb2505047 100644
> --- a/drivers/net/wireless/mac80211_hwsim.c
> +++ b/drivers/net/wireless/mac80211_hwsim.c
> @@ -57,10 +57,15 @@ static bool paged_rx = false;
> module_param(paged_rx, bool, 0644);
> MODULE_PARM_DESC(paged_rx, "Use paged SKBs for RX instead of linear ones");
>
> -static bool rctbl = false;
> +static bool rctbl = true;
should we really change the default?
Is there a netlink control to set it for newly created wiphys?
> module_param(rctbl, bool, 0444);
>
> +static int tpc = 0;
> +module_param(tpc, int, 0444);
> +MODULE_PARM_DESC(tpc, "Support transmit power control (TPC) (0 = no,\
> + 1 = per packet, 2 = per mrr stage)");
Not sure I like this either - I think we should probably create the
wiphys dynamically for most features these days?
> +static inline u8
> +hwsim_rate_get_vht_mcs(const struct hwsim_tx_rate *rate) {
> + return rate->idx & 0xf;
> +}
> +
> +static inline u8
> +hwsim_rate_get_vht_nss(const struct hwsim_tx_rate *rate) {
> + return (rate->idx >> 4) + 1;
> +}
odd indentation for functions - should have linebreak before {
> +static void trans_tx_rate_to_rate_info(const struct hwsim_tx_rate *rate,
> + const struct hwsim_tx_rate_flag *rate_flags,
> + struct wiphy *wiphy, u8 band,
> + struct rate_info *rate_info)
> +{
> + memset(rate_info, 0, sizeof(struct rate_info));
> +
> + if (rate_flags->flags & MAC80211_HWSIM_TX_RC_MCS) { /* 802.11n */
> + rate_info->flags |= RATE_INFO_FLAGS_MCS;
> + rate_info->mcs = rate->idx;
> + } else if (rate_flags->flags & MAC80211_HWSIM_TX_RC_VHT_MCS) { /* 802.11ac */
> + rate_info->flags |= RATE_INFO_FLAGS_VHT_MCS;
> + rate_info->mcs = hwsim_rate_get_vht_mcs(rate);
> + rate_info->nss = hwsim_rate_get_vht_nss(rate);
> + } else { /* 802.11a/b/g */
again what about HE/EHT?
> +static void mac80211_hwsim_get_txpower(struct ieee80211_hw *hw,
> + struct ieee80211_sta *sta,
> + struct sk_buff *skb,
> + s16 *txpower)
> +{
> + struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> + bool tpc_per_pkt = ieee80211_hw_check(hw, SUPPORTS_TPC_PER_PACKET);
> + bool tpc_per_mrr = ieee80211_hw_check(hw, SUPPORTS_TPC_PER_MRR);
> + u8 i = 0;
> +
> + if (sta && sta->rates && !info->control.skip_table &&
> + ieee80211_hw_check(hw, SUPPORTS_RC_TABLE))
> + {
misplaced {, should be at end of previous line
> + struct ieee80211_sta_rates *ratetbl = rcu_dereference(sta->rates);
> +
> + for (; i < IEEE80211_TX_MAX_RATES; i++) {
those loops are weird - prefer to spell out 'i = 0' in the loops rather
than common initialization above (and remove the =0 from the init then)
> @@ -4846,16 +4989,32 @@ static int hwsim_tx_info_frame_received_nl(struct sk_buff *skb_2,
>
> tx_attempts = (struct hwsim_tx_rate *)nla_data(
> info->attrs[HWSIM_ATTR_TX_INFO]);
> + tx_attempts_flags = (struct hwsim_tx_rate_flag *)nla_data(
> + info->attrs[HWSIM_ATTR_TX_INFO_FLAGS]);
> + sta = (struct ieee80211_sta *)txi->rate_driver_data[1];
That seems dangerous - what if the STA was freed already? You don't walk
the pending list or something if the STA goes away.
johannes
