Transmit power control (TPC) per packet hence per station can be used to
manage interference and increase overall spatial reuse and therefore
increases sum throughput in WiFi networks with multiple APs and STAs.
Although several of today's wifi chips, e.g., from QCA and from Mediatek
support fine-grained TPC per packet, the Linux mac80211 layer does not
provide support for this annotation nor control yet.
This series proposes several changes to introduce TPC support in
mac80211, in particular to annotate tx-power per packet/per mrr stage in
the tx control path.
The patches include new members in the tx control path structs, a
modified tx-power level support annotation, hardware flags, hwsim
support, debugfs support in minstrel_ht for fixed TPC and an utility
function for the convenient use of struct ieee80211_rate_status
(introduced by 44fa75f207d8a106bc75e6230db61e961fdbf8a8) for tx-power
status report in drivers.
An proof-of-concept ath9k support was implemented for testing these
changes on real ath9k hardware, this implementation is planned to be
brought upstream later.
Compile-Tested: current wireless-next tree with all flags on
Tested-on: PCEngines APU with ath9k WiFi device on OpenWrt Linux
Kernel 5.15.68, AP<->STA setup with both ath9k and hwsim
(used current OpenWrt testing kernel)
---
v2:
- added exemplary hwsim support
- added debugfs in minstrel_ht for fixed TPC
---
Signed-off-by: Thomas Huehn <[email protected]>
Signed-off-by: Jonas Jelonek <[email protected]>
Jonas Jelonek (6):
mac80211: modify tx-power level annotation
mac80211: add tx-power annotation in control path
mac80211: add hardware flags for TPC support
mac80211: add utility function for tx_rate - rate_info conversion
mac80211_hwsim: add TPC per packet support
mac80211: minstrel_ht - add debugfs entry per sta for fixed tx-power
drivers/net/wireless/mac80211_hwsim.c | 175 ++++++++++++++++++++-
drivers/net/wireless/mac80211_hwsim.h | 1 +
include/net/mac80211.h | 66 ++++++--
net/mac80211/debugfs.c | 2 +
net/mac80211/rc80211_minstrel_ht.c | 14 ++
net/mac80211/rc80211_minstrel_ht.h | 11 ++
net/mac80211/rc80211_minstrel_ht_debugfs.c | 11 ++
net/mac80211/util.c | 35 +++++
8 files changed, 296 insertions(+), 19 deletions(-)
--
2.30.2
This patch adds an utility function to mac80211 for conversion between
ieee80211_tx_rate (mac80211.h) and rate_info (cfg80211.h).
struct ieee80211_tx_rate is space limited to annotate rates up to IEEE
802.11ac. The new struct rate_info is able to annotate IEEE 802.11ax
rates and beyond. Several drivers internally still use ieee80211_tx_rate
but mac80211 expects rate_info in struct ieee80211_rate_status. This
struct is in turn required to allow, e.g., tx-power status report or
dynamic number of mrr stages.
Signed-off-by: Jonas Jelonek <[email protected]>
---
include/net/mac80211.h | 4 ++++
net/mac80211/util.c | 35 +++++++++++++++++++++++++++++++++++
2 files changed, 39 insertions(+)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index c4b55c7273ed..f17a03caa361 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1051,6 +1051,10 @@ ieee80211_rate_get_vht_nss(const struct ieee80211_tx_rate *rate)
return (rate->idx >> 4) + 1;
}
+void ieee80211_rate_get_rate_info(const struct ieee80211_tx_rate *rate,
+ struct wiphy *wiphy, u8 band,
+ struct rate_info *rate_info);
+
/**
* struct ieee80211_tx_info - skb transmit information
*
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 0ea5d50091dc..c76dc255bec3 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -4867,3 +4867,38 @@ void ieee80211_fragment_element(struct sk_buff *skb, u8 *len_pos)
*len_pos = elem_len;
}
+
+
+void ieee80211_rate_get_rate_info(const struct ieee80211_tx_rate *rate,
+ struct wiphy *wiphy, u8 band,
+ struct rate_info *rate_info)
+{
+ memset(rate_info, 0, sizeof(struct rate_info));
+
+ if (rate->flags & IEEE80211_TX_RC_MCS) { /* 802.11n */
+ rate_info->flags |= RATE_INFO_FLAGS_MCS;
+ rate_info->mcs = rate->idx;
+ } else if (rate->flags & IEEE80211_TX_RC_VHT_MCS) { /* 802.11ac */
+ rate_info->flags |= RATE_INFO_FLAGS_VHT_MCS;
+ rate_info->mcs = ieee80211_rate_get_vht_mcs(rate);
+ rate_info->nss = ieee80211_rate_get_vht_nss(rate);
+ } else { /* 802.11a/b/g */
+ rate_info->legacy = wiphy->bands[band]->bitrates[rate->idx].bitrate;
+ rate_info->bw = RATE_INFO_BW_20;
+ return;
+ }
+
+ if (rate->flags & IEEE80211_TX_RC_40_MHZ_WIDTH)
+ rate_info->bw = RATE_INFO_BW_40;
+ else if (rate->flags & IEEE80211_TX_RC_80_MHZ_WIDTH)
+ rate_info->bw = RATE_INFO_BW_80;
+ else if (rate->flags & IEEE80211_TX_RC_160_MHZ_WIDTH)
+ rate_info->bw = RATE_INFO_BW_160;
+ else
+ rate_info->bw = RATE_INFO_BW_20;
+
+ if (rate->flags & IEEE80211_TX_RC_SHORT_GI)
+ rate_info->flags |= RATE_INFO_FLAGS_SHORT_GI;
+
+}
+EXPORT_SYMBOL(ieee80211_rate_get_rate_info);
--
2.30.2
This patch adds members to ieee80211_tx_info and ieee80211_sta_rates
structures to allow tx-power annotation per packet/per mrr stage.
The added members are always tx-power indices referring to the tx-power
set described by ieee80211_hw->txpower_ranges.
The annotation in ieee80211_tx_info is for probing and compatibility
reasons only, e.g. drivers that only support RC/TPC per packet and do
not yet use ieee80211_sta_rates.
Signed-off-by: Thomas Huehn <[email protected]>
Signed-off-by: Jonas Jelonek <[email protected]>
---
include/net/mac80211.h | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index a047fb5fc207..67d9087e031f 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1073,6 +1073,10 @@ ieee80211_rate_get_vht_nss(const struct ieee80211_tx_rate *rate)
* @control.use_cts_prot: use RTS/CTS
* @control.short_preamble: use short preamble (CCK only)
* @control.skip_table: skip externally configured rate table
+ * @control.txpower_idx: Tx-power level index for whole packet,
+ * referring to an idx described by ieee80211_hw->txpower_ranges. A
+ * negative idx means 'invalid', 'unset' or 'default'. Behavior in this
+ * case is driver-specific.
* @control.jiffies: timestamp for expiry on powersave clients
* @control.vif: virtual interface (may be NULL)
* @control.hw_key: key to encrypt with (may be NULL)
@@ -1121,7 +1125,7 @@ struct ieee80211_tx_info {
u8 use_cts_prot:1;
u8 short_preamble:1;
u8 skip_table:1;
- /* 2 bytes free */
+ s16 txpower_idx;
};
/* only needed before rate control */
unsigned long jiffies;
@@ -1182,14 +1186,16 @@ ieee80211_info_get_tx_time_est(struct ieee80211_tx_info *info)
*
* @rate_idx The actual used rate.
* @try_count How often the rate was tried.
- * @tx_power_idx An idx into the ieee80211_hw->tx_power_levels list of the
- * corresponding wifi hardware. The idx shall point to the power level
- * that was used when sending the packet.
+ * @tx_power_idx An idx into the the tx-power set described by
+ * ieee80211_hw->txpower_ranges for the corresponding wifi hardware.
+ * The idx shall point to the tx-power level that was used when sending
+ * the packet at this rate. A negative value is considered as 'invalid'
+ * or 'no power level reported by the driver'.
*/
struct ieee80211_rate_status {
struct rate_info rate_idx;
u8 try_count;
- u8 tx_power_idx;
+ s16 tx_power_idx;
};
/**
@@ -2113,6 +2119,10 @@ enum ieee80211_sta_rx_bandwidth {
* @rcu_head: RCU head used for freeing the table on update
* @rate: transmit rates/flags to be used by default.
* Overriding entries per-packet is possible by using cb tx control.
+ * @rate.txpower_idx: An idx pointing to a tx-power level described by
+ * ieee80211_hw->txpower_ranges that should be used for the mrr stage.
+ * A negative value means 'invalid', 'unset' or 'default' power level,
+ * actual behavior is driver-specific.
*/
struct ieee80211_sta_rates {
struct rcu_head rcu_head;
@@ -2122,6 +2132,7 @@ struct ieee80211_sta_rates {
u8 count_cts;
u8 count_rts;
u16 flags;
+ s16 txpower_idx;
} rate[IEEE80211_TX_RATE_TABLE_SIZE];
};
--
2.30.2
This patch modifies the annotation of supported tx-power levels for a
wifi device in ieee80211_hw. This annotation was introduced with commit
44fa75f207d8a106bc75e6230db61e961fdbf8a8 to be able to operate on power
indices instead of absolute power values, providing better support for
different hardware capabilities.
The former annotation uses a 'const s8' for each power level. The choice
made with the former commit was not the best as this kind of annotation
may require much memory if there is a high number of power levels.
Thus, it is now replaced by a new struct ieee80211_hw_txpower_range. This
struct describes a tx-power range by specifying a start index, the number
of levels, a start power value and the power step width.
A wifi driver should specify valid tx-power ranges when it registers a
device in mac80211 by providing a pointer to a list and a length in the
corresponding ieee80211_hw members.
Drivers can define multiple tx-power ranges with each different scales
depending on the hardware.
Signed-off-by: Thomas Huehn <[email protected]>
Signed-off-by: Jonas Jelonek <[email protected]>
---
include/net/mac80211.h | 32 ++++++++++++++++++++++++++------
1 file changed, 26 insertions(+), 6 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index ac2bad57933f..a047fb5fc207 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -2661,6 +2661,27 @@ enum ieee80211_hw_flags {
NUM_IEEE80211_HW_FLAGS
};
+/**
+ * struct ieee80211_hw_txpower_range - Power range for transmit power
+ *
+ * This struct can be used by drivers to define multiple tx-power ranges with
+ * different scales according to the hardware capabilities. A tx-power range
+ * describe either absolute power levels or power offsets relative to a base
+ * power.
+ *
+ * @start_idx The starting idx of the range. @start_idx is always the lowest
+ * idx of the power range.
+ * @start_pwr The power level at idx @start_idx in 0.25 dBm units.
+ * @n_levels How many power levels this range has.
+ * @pwr_step The power step width in 0.25 dBm units.
+ */
+struct ieee80211_hw_txpower_range {
+ u8 start_idx;
+ u8 n_levels;
+ s8 start_pwr;
+ s8 pwr_step;
+};
+
/**
* struct ieee80211_hw - hardware information and state
*
@@ -2783,11 +2804,10 @@ enum ieee80211_hw_flags {
*
* @max_mtu: the max mtu could be set.
*
- * @tx_power_levels: a list of power levels supported by the wifi hardware.
- * The power levels can be specified either as integer or fractions.
- * The power level at idx 0 shall be the maximum positive power level.
+ * @txpower_ranges: a list of tx-power level ranges supported by the wifi
+ * hardware. The driver can specify multiple ranges with e.g. different scales.
*
- * @max_txpwr_levels_idx: the maximum valid idx of 'tx_power_levels' list.
+ * @n_txpower_ranges: the number of power ranges defined by the wifi driver.
*/
struct ieee80211_hw {
struct ieee80211_conf conf;
@@ -2824,8 +2844,8 @@ struct ieee80211_hw {
u8 tx_sk_pacing_shift;
u8 weight_multiplier;
u32 max_mtu;
- const s8 *tx_power_levels;
- u8 max_txpwr_levels_idx;
+ struct ieee80211_hw_txpower_range *txpower_ranges;
+ u8 n_txpower_ranges;
};
static inline bool _ieee80211_hw_check(struct ieee80211_hw *hw,
--
2.30.2
Create a new debugfs entry called 'rc_fixed_txpower_idx' in debugfs dir
for each station. By writing a positive static tx-power idx into this
file, minstrel_ht applies this tx-power idx to each packet or each mrr
stage, depending on what the hardware supports. By writing (u32)-1 to
the file, minstrel will return to automatic mode which currently just
passes -1 as tx-power idx, indicating that the driver should use a
default.
The debugfs entry will only be created if either tpc per packet or per
mrr is supported.
Signed-off-by: Jonas Jelonek <[email protected]>
---
net/mac80211/rc80211_minstrel_ht.c | 14 ++++++++++++++
net/mac80211/rc80211_minstrel_ht.h | 11 +++++++++++
net/mac80211/rc80211_minstrel_ht_debugfs.c | 11 +++++++++++
3 files changed, 36 insertions(+)
diff --git a/net/mac80211/rc80211_minstrel_ht.c b/net/mac80211/rc80211_minstrel_ht.c
index 24c3c055db6d..222b51e7d9ee 100644
--- a/net/mac80211/rc80211_minstrel_ht.c
+++ b/net/mac80211/rc80211_minstrel_ht.c
@@ -1486,6 +1486,14 @@ minstrel_ht_set_rate(struct minstrel_priv *mp, struct minstrel_ht_sta *mi,
ratetbl->rate[offset].idx = idx;
ratetbl->rate[offset].flags = flags;
+
+#ifdef CONFIG_MAC80211_DEBUGFS
+ if (mi->fixed_txpower_idx != -1) {
+ ratetbl->rate[offset].txpower_idx = mi->fixed_txpower_idx;
+ return;
+ }
+#endif
+ ratetbl->rate[offset].txpower_idx = -1;
}
static inline int
@@ -1603,8 +1611,14 @@ minstrel_ht_get_rate(void *priv, struct ieee80211_sta *sta, void *priv_sta,
info->flags |= mi->tx_flags;
#ifdef CONFIG_MAC80211_DEBUGFS
+ if (mi->fixed_txpower_idx != -1)
+ info->control.txpower_idx = mi->fixed_txpower_idx;
+
if (mp->fixed_rate_idx != -1)
return;
+#else
+ /* Pass -1 to indicate 'ignore txpower' or 'use default' */
+ info->control.txpower_idx = -1;
#endif
/* Don't use EAPOL frames for sampling on non-mrr hw */
diff --git a/net/mac80211/rc80211_minstrel_ht.h b/net/mac80211/rc80211_minstrel_ht.h
index 1766ff0c78d3..15222d66c4b8 100644
--- a/net/mac80211/rc80211_minstrel_ht.h
+++ b/net/mac80211/rc80211_minstrel_ht.h
@@ -194,6 +194,17 @@ struct minstrel_ht_sta {
/* MCS rate group info and statistics */
struct minstrel_mcs_group_data groups[MINSTREL_GROUPS_NB];
+
+#ifdef CONFIG_MAC80211_DEBUGFS
+ /*
+ * enable fixed tx-power processing per STA
+ * - write static index to debugfs:ieee80211/phyX/netdev:wlanY
+ * /stations/<MAC>/rc_fixed_txpower_idx
+ * - write -1 to enable automatic processing again
+ * - setting will be applied on next update
+ */
+ u32 fixed_txpower_idx;
+#endif
};
void minstrel_ht_add_sta_debugfs(void *priv, void *priv_sta, struct dentry *dir);
diff --git a/net/mac80211/rc80211_minstrel_ht_debugfs.c b/net/mac80211/rc80211_minstrel_ht_debugfs.c
index 25b8a67a63a4..d625d860d01a 100644
--- a/net/mac80211/rc80211_minstrel_ht_debugfs.c
+++ b/net/mac80211/rc80211_minstrel_ht_debugfs.c
@@ -329,8 +329,19 @@ static const struct file_operations minstrel_ht_stat_csv_fops = {
void
minstrel_ht_add_sta_debugfs(void *priv, void *priv_sta, struct dentry *dir)
{
+ struct minstrel_priv *mp = priv;
+ struct minstrel_ht_sta *mi = priv_sta;
+
debugfs_create_file("rc_stats", 0444, dir, priv_sta,
&minstrel_ht_stat_fops);
debugfs_create_file("rc_stats_csv", 0444, dir, priv_sta,
&minstrel_ht_stat_csv_fops);
+
+ if (ieee80211_hw_check(mp->hw, SUPPORTS_TPC_PER_PACKET) ||
+ ieee80211_hw_check(mp->hw, SUPPORTS_TPC_PER_MRR))
+ {
+ mi->fixed_txpower_idx = (u32)-1;
+ debugfs_create_u32("rc_fixed_txpower_idx", S_IRUGO | S_IWUGO,
+ dir, &mi->fixed_txpower_idx);
+ }
}
--
2.30.2
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 <[email protected]>
---
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;
module_param(rctbl, bool, 0444);
MODULE_PARM_DESC(rctbl, "Handle rate control table");
+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)");
+
static bool support_p2p_device = true;
module_param(support_p2p_device, bool, 0444);
MODULE_PARM_DESC(support_p2p_device, "Support P2P-Device interface type");
@@ -196,6 +201,15 @@ static const struct ieee80211_regdomain *hwsim_world_regdom_custom[] = {
&hwsim_world_regdom_custom_03,
};
+#define MAC80211_HWSIM_MAX_POWER 30
+
+struct ieee80211_hw_txpower_range hwsim_txpower_range = {
+ .start_idx = 0,
+ .n_levels = 31,
+ .start_pwr = 0,
+ .pwr_step = 1
+};
+
struct hwsim_vif_priv {
u32 magic;
u8 bssid[ETH_ALEN];
@@ -1364,10 +1378,105 @@ static inline u16 trans_tx_rate_flags_ieee2hwsim(struct ieee80211_tx_rate *rate)
return result;
}
+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;
+}
+
+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 */
+ rate_info->legacy = wiphy->bands[band]->bitrates[rate->idx].bitrate;
+ rate_info->bw = RATE_INFO_BW_20;
+ return;
+ }
+
+ if (rate_flags->flags & MAC80211_HWSIM_TX_RC_40_MHZ_WIDTH)
+ rate_info->bw = RATE_INFO_BW_40;
+ else if (rate_flags->flags & MAC80211_HWSIM_TX_RC_80_MHZ_WIDTH)
+ rate_info->bw = RATE_INFO_BW_80;
+ else if (rate_flags->flags & MAC80211_HWSIM_TX_RC_160_MHZ_WIDTH)
+ rate_info->bw = RATE_INFO_BW_160;
+ else
+ rate_info->bw = RATE_INFO_BW_20;
+
+ if (rate_flags->flags & MAC80211_HWSIM_TX_RC_SHORT_GI)
+ rate_info->flags |= RATE_INFO_FLAGS_SHORT_GI;
+}
+
+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))
+ {
+ struct ieee80211_sta_rates *ratetbl = rcu_dereference(sta->rates);
+
+ for (; i < IEEE80211_TX_MAX_RATES; i++) {
+ int txpwr_val = -1;
+ if (info->control.rates[i].idx < 0 ||
+ info->control.rates[i].count == 0)
+ break;
+
+ if (tpc_per_mrr)
+ txpwr_val = ratetbl->rate[i].txpower_idx;
+ else if (tpc_per_pkt)
+ txpwr_val = ratetbl->rate[0].txpower_idx;
+
+ if (txpwr_val < 0)
+ txpower[i] = MAC80211_HWSIM_MAX_POWER;
+ else
+ txpower[i] = txpwr_val;
+ }
+ } else {
+ for (; i < IEEE80211_TX_MAX_RATES; i++) {
+ int txpwr_val = -1;
+ if (info->control.rates[i].idx < 0 ||
+ info->control.rates[i].count == 0)
+ break;
+
+ if (tpc_per_pkt || tpc_per_mrr)
+ txpwr_val = info->control.txpower_idx;
+
+ if (txpwr_val < 0)
+ txpower[i] = MAC80211_HWSIM_MAX_POWER;
+ else
+ txpower[i] = txpwr_val;
+ }
+ }
+ return;
+}
+
static void mac80211_hwsim_tx_frame_nl(struct ieee80211_hw *hw,
struct sk_buff *my_skb,
int dst_portid,
- struct ieee80211_channel *channel)
+ struct ieee80211_channel *channel,
+ struct ieee80211_sta *sta,
+ s16 *txpower)
{
struct sk_buff *skb;
struct mac80211_hwsim_data *data = hw->priv;
@@ -1434,6 +1543,8 @@ static void mac80211_hwsim_tx_frame_nl(struct ieee80211_hw *hw,
tx_attempts_flags[i].flags =
trans_tx_rate_flags_ieee2hwsim(
&info->status.rates[i]);
+
+ tx_attempts[i].txpower_idx = txpower[i];
}
if (nla_put(skb, HWSIM_ATTR_TX_INFO,
@@ -1449,6 +1560,7 @@ static void mac80211_hwsim_tx_frame_nl(struct ieee80211_hw *hw,
/* We create a cookie to identify this skb */
cookie = atomic_inc_return(&data->pending_cookie);
info->rate_driver_data[0] = (void *)cookie;
+ info->rate_driver_data[1] = (void *)sta;
if (nla_put_u64_64bit(skb, HWSIM_ATTR_COOKIE, cookie, HWSIM_ATTR_PAD))
goto nla_put_failure;
@@ -1792,6 +1904,9 @@ static void mac80211_hwsim_tx(struct ieee80211_hw *hw,
struct ieee80211_hdr *hdr = (void *)skb->data;
struct ieee80211_chanctx_conf *chanctx_conf;
struct ieee80211_channel *channel;
+ struct ieee80211_tx_status status = {0};
+ struct ieee80211_rate_status rate;
+ s16 txpower[IEEE80211_TX_MAX_RATES];
bool ack;
enum nl80211_chan_width confbw = NL80211_CHAN_WIDTH_20_NOHT;
u32 _portid, i;
@@ -1897,6 +2012,8 @@ static void mac80211_hwsim_tx(struct ieee80211_hw *hw,
return;
}
+ mac80211_hwsim_get_txpower(hw, control->sta, skb, &txpower[0]);
+
if (skb->len >= 24 + 8 &&
ieee80211_is_probe_resp(hdr->frame_control)) {
/* fake header transmission time */
@@ -1922,7 +2039,8 @@ static void mac80211_hwsim_tx(struct ieee80211_hw *hw,
_portid = READ_ONCE(data->wmediumd);
if (_portid || hwsim_virtio_enabled)
- return mac80211_hwsim_tx_frame_nl(hw, skb, _portid, channel);
+ return mac80211_hwsim_tx_frame_nl(hw, skb, _portid, channel,
+ control->sta, &txpower[0]);
/* NO wmediumd detected, perfect medium simulation */
data->tx_pkts++;
@@ -1938,9 +2056,21 @@ static void mac80211_hwsim_tx(struct ieee80211_hw *hw,
txi->control.rates[0].count = 1;
txi->control.rates[1].idx = -1;
+ status.sta = control->sta;
+ status.info = txi;
+ status.skb = skb;
+ status.n_rates = 1;
+ rate.try_count = 1;
+ rate.tx_power_idx = txpower[0];
+
+ ieee80211_rate_get_rate_info(&txi->control.rates[0], hw->wiphy,
+ txi->band, &rate.rate_idx);
+ status.rates = &rate;
+
if (!(txi->flags & IEEE80211_TX_CTL_NO_ACK) && ack)
txi->flags |= IEEE80211_TX_STAT_ACK;
- ieee80211_tx_status_irqsafe(hw, skb);
+
+ ieee80211_tx_status_ext(hw, &status);
}
@@ -2030,6 +2160,7 @@ static void mac80211_hwsim_tx_frame(struct ieee80211_hw *hw,
{
struct mac80211_hwsim_data *data = hw->priv;
u32 _pid = READ_ONCE(data->wmediumd);
+ s16 txpower[IEEE80211_TX_MAX_RATES];
if (ieee80211_hw_check(hw, SUPPORTS_RC_TABLE)) {
struct ieee80211_tx_info *txi = IEEE80211_SKB_CB(skb);
@@ -2037,11 +2168,13 @@ static void mac80211_hwsim_tx_frame(struct ieee80211_hw *hw,
txi->control.rates,
ARRAY_SIZE(txi->control.rates));
}
+ mac80211_hwsim_get_txpower(hw, NULL, skb, &txpower[0]);
mac80211_hwsim_monitor_rx(hw, skb, chan);
if (_pid || hwsim_virtio_enabled)
- return mac80211_hwsim_tx_frame_nl(hw, skb, _pid, chan);
+ return mac80211_hwsim_tx_frame_nl(hw, skb, _pid, chan,
+ NULL, &txpower[0]);
data->tx_pkts++;
data->tx_bytes += skb->len;
@@ -4395,6 +4528,8 @@ static int mac80211_hwsim_new_radio(struct genl_info *info,
hw->queues = 5;
hw->offchannel_tx_hw_queue = 4;
+ hw->txpower_ranges = &hwsim_txpower_range;
+ hw->n_txpower_ranges = 1;
ieee80211_hw_set(hw, SUPPORT_FAST_XMIT);
ieee80211_hw_set(hw, CHANCTX_STA_CSA);
@@ -4408,6 +4543,10 @@ static int mac80211_hwsim_new_radio(struct genl_info *info,
ieee80211_hw_set(hw, REPORTS_TX_ACK_STATUS);
ieee80211_hw_set(hw, TDLS_WIDER_BW);
ieee80211_hw_set(hw, SUPPORTS_MULTI_BSSID);
+ if (tpc == 1)
+ ieee80211_hw_set(hw, SUPPORTS_TPC_PER_PACKET);
+ else if (tpc == 2)
+ ieee80211_hw_set(hw, SUPPORTS_TPC_PER_MRR);
if (param->mlo) {
hw->wiphy->flags |= WIPHY_FLAG_SUPPORTS_MLO;
@@ -4784,11 +4923,14 @@ static void hwsim_register_wmediumd(struct net *net, u32 portid)
static int hwsim_tx_info_frame_received_nl(struct sk_buff *skb_2,
struct genl_info *info)
{
-
struct ieee80211_hdr *hdr;
struct mac80211_hwsim_data *data2;
struct ieee80211_tx_info *txi;
struct hwsim_tx_rate *tx_attempts;
+ struct hwsim_tx_rate_flag *tx_attempts_flags;
+ struct ieee80211_sta *sta;
+ struct ieee80211_tx_status status = {0};
+ struct ieee80211_rate_status rates[IEEE80211_TX_MAX_RATES];
u64 ret_skb_cookie;
struct sk_buff *skb, *tmp;
const u8 *src;
@@ -4801,7 +4943,8 @@ static int hwsim_tx_info_frame_received_nl(struct sk_buff *skb_2,
!info->attrs[HWSIM_ATTR_FLAGS] ||
!info->attrs[HWSIM_ATTR_COOKIE] ||
!info->attrs[HWSIM_ATTR_SIGNAL] ||
- !info->attrs[HWSIM_ATTR_TX_INFO])
+ !info->attrs[HWSIM_ATTR_TX_INFO] ||
+ !info->attrs[HWSIM_ATTR_TX_INFO_FLAGS])
goto out;
src = (void *)nla_data(info->attrs[HWSIM_ATTR_ADDR_TRANSMITTER]);
@@ -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];
/* now send back TX status */
txi = IEEE80211_SKB_CB(skb);
ieee80211_tx_info_clear_status(txi);
+ status.sta = sta;
+ status.info = txi;
+ status.skb = skb;
+ status.n_rates = 0;
+
for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) {
+ if (tx_attempts[i].idx < 0 || tx_attempts[i].count == 0)
+ break;
+
+ trans_tx_rate_to_rate_info(&tx_attempts[i], &tx_attempts_flags[i],
+ data2->hw->wiphy, txi->band,
+ &rates[i].rate_idx);
+ status.n_rates++;
txi->status.rates[i].idx = tx_attempts[i].idx;
txi->status.rates[i].count = tx_attempts[i].count;
}
+ status.rates = &rates[0];
txi->status.ack_signal = nla_get_u32(info->attrs[HWSIM_ATTR_SIGNAL]);
@@ -4872,7 +5031,7 @@ static int hwsim_tx_info_frame_received_nl(struct sk_buff *skb_2,
if (hwsim_flags & HWSIM_TX_CTL_NO_ACK)
txi->flags |= IEEE80211_TX_STAT_NOACK_TRANSMITTED;
- ieee80211_tx_status_irqsafe(data2->hw, skb);
+ ieee80211_tx_status_ext(data2->hw, &status);
return 0;
out:
return -EINVAL;
diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h
index 527799b2de0f..31b425216c8e 100644
--- a/drivers/net/wireless/mac80211_hwsim.h
+++ b/drivers/net/wireless/mac80211_hwsim.h
@@ -193,6 +193,7 @@ enum {
struct hwsim_tx_rate {
s8 idx;
u8 count;
+ u8 txpower_idx;
} __packed;
/**
--
2.30.2
This patch adds two hardware flags for drivers to indicate their
transmit power control (TPC) capabilities: TPC per packet or TPC
per mrr stage of each data packet. The driver has to register with its
TPC capabilities when it is registering at the mac80211.
Signed-off-by: Thomas Huehn <[email protected]>
Signed-off-by: Jonas Jelonek <[email protected]>
---
include/net/mac80211.h | 9 +++++++++
net/mac80211/debugfs.c | 2 ++
2 files changed, 11 insertions(+)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 67d9087e031f..c4b55c7273ed 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -2610,6 +2610,13 @@ struct ieee80211_txq {
* @IEEE80211_HW_MLO_MCAST_MULTI_LINK_TX: Hardware/driver handles transmitting
* multicast frames on all links, mac80211 should not do that.
*
+ * @IEEE80211_HW_SUPPORTS_TPC_PER_PACKET: The hardware/driver supports transmit
+ * power control (TPC) with one power level per data packet.
+ *
+ * @IEEE80211_HW_SUPPORTS_TPC_PER_MRR: The hardware/driver supports transmit
+ * power control (TPC) with individual power levels for each
+ * multi-rate-retry (mrr) stage per packet.
+ *
* @NUM_IEEE80211_HW_FLAGS: number of hardware flags, used for sizing arrays
*/
enum ieee80211_hw_flags {
@@ -2667,6 +2674,8 @@ enum ieee80211_hw_flags {
IEEE80211_HW_SUPPORTS_CONC_MON_RX_DECAP,
IEEE80211_HW_DETECTS_COLOR_COLLISION,
IEEE80211_HW_MLO_MCAST_MULTI_LINK_TX,
+ IEEE80211_HW_SUPPORTS_TPC_PER_PACKET,
+ IEEE80211_HW_SUPPORTS_TPC_PER_MRR,
/* keep last, obviously */
NUM_IEEE80211_HW_FLAGS
diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index 78c7d60e8667..daeef1e04cb5 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -496,6 +496,8 @@ static const char *hw_flag_names[] = {
FLAG(SUPPORTS_CONC_MON_RX_DECAP),
FLAG(DETECTS_COLOR_COLLISION),
FLAG(MLO_MCAST_MULTI_LINK_TX),
+ FLAG(SUPPORTS_TPC_PER_PACKET),
+ FLAG(SUPPORTS_TPC_PER_MRR),
#undef FLAG
};
--
2.30.2
On 9/20/2022 3:40 AM, Jonas Jelonek wrote:
> This patch modifies the annotation of supported tx-power levels for a
> wifi device in ieee80211_hw. This annotation was introduced with commit
> 44fa75f207d8a106bc75e6230db61e961fdbf8a8 to be able to operate on power
nit: preferred way to reference a commit is 12 characters of hash + subject:
44fa75f207d8 ("mac80211: extend current rate control tx status API")
Greeting,
FYI, we noticed the following commit (built with gcc-11):
commit: 14f322748fe8490b62751cd30e94642c83049db0 ("[RFC v2 5/6] mac80211_hwsim: add TPC per packet support")
url: https://github.com/intel-lab-lkp/linux/commits/Jonas-Jelonek/mac80211-add-TPC-support-in-control-path/20220920-184304
base: https://git.kernel.org/cgit/linux/kernel/git/wireless/wireless-next.git main
patch link: https://lore.kernel.org/linux-wireless/[email protected]
in testcase: hwsim
version: hwsim-x86_64-717e5d7-1_20220525
with following parameters:
test: group-18
on test machine: 4 threads Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz (Skylake) with 32G memory
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/r/[email protected]
[ 165.323705][ T5117] ieee80211 phy0: hwsim_send_nullfunc: send data::nullfunc to 02:00:00:00:03:00 ps=1
[ 165.333039][ T5117] ------------[ cut here ]------------
[ 165.338338][ T5117] WARNING: CPU: 2 PID: 5117 at include/net/mac80211.h:2968 mac80211_hwsim_monitor_rx+0x6a0/0x800 [mac80211_hwsim]
[ 165.350153][ T5117] Modules linked in: bridge stp llc cmac ccm mac80211_hwsim mac80211 cfg80211 rfkill libarc4 btrfs blake2b_generic xor raid6_pq zstd_co
mpress libcrc32c intel_rapl_msr intel_rapl_common sd_mod t10_pi x86_pkg_temp_thermal intel_powerclamp crc64_rocksoft_generic crc64_rocksoft coretemp crc64 i
pmi_devintf sg kvm_intel ipmi_msghandler i915 mei_wdt kvm wmi_bmof drm_buddy irqbypass crct10dif_pclmul intel_gtt crc32_pclmul crc32c_intel drm_display_help
er ghash_clmulni_intel rapl ttm ahci libahci intel_cstate drm_kms_helper intel_uncore mei_me libata mei syscopyarea sysfillrect sysimgblt intel_pch_thermal wmi fb_sys_fops video intel_pmc_core acpi_pad drm fuse ip_tables
[ 165.409967][ T5117] CPU: 2 PID: 5117 Comm: sh Tainted: G B I 6.0.0-rc3-00733-g14f322748fe8 #1
[ 165.419688][ T5117] Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.1.1 10/07/2015
[ 165.427763][ T5117] RIP: 0010:mac80211_hwsim_monitor_rx+0x6a0/0x800 [mac80211_hwsim]
[ 165.435498][ T5117] Code: b4 00 00 00 e8 81 9e 5a e0 48 89 ef 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f e9 2b 25 a5 e1 41 bc c0 00 00 00 e9 46 fe ff ff <0f> 0b 45 31 e4 45 31 ed e9 b9 fa ff ff e8 8e 95 5a e0 e9 ac f9 ff
[ 165.454894][ T5117] RSP: 0018:ffffc90001a9faa0 EFLAGS: 00010286
[ 165.460808][ T5117] RAX: 0000000000000000 RBX: ffff8888587088e0 RCX: 0000000000000000
[ 165.468623][ T5117] RDX: 1ffff1102911a0f6 RSI: ffff8881488d0780 RDI: ffff8881488d07b0
[ 165.476423][ T5117] RBP: ffff8881488d0780 R08: ffffed102911a0f7 R09: ffff8881488d07be
[ 165.484224][ T5117] R10: ffffed110b0e1127 R11: 0000000000000001 R12: ffffffffffffffff
[ 165.492038][ T5117] R13: ffff88885870a318 R14: 0000000000000000 R15: ffff88885870a080
[ 165.499849][ T5117] FS: 00007f9a1c55c580(0000) GS:ffff8887de900000(0000) knlGS:0000000000000000
[ 165.508612][ T5117] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 165.515046][ T5117] CR2: 000055ce2a11f388 CR3: 0000000249c2e004 CR4: 00000000003706e0
[ 165.522861][ T5117] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 165.530672][ T5117] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 165.538484][ T5117] Call Trace:
[ 165.541619][ T5117] <TASK>
[ 165.544405][ T5117] mac80211_hwsim_tx_frame+0x128/0x300 [mac80211_hwsim]
[ 165.551187][ T5117] ? mac80211_hwsim_get_txpower+0x800/0x800 [mac80211_hwsim]
[ 165.558406][ T5117] hwsim_send_nullfunc+0x179/0x280 [mac80211_hwsim]
[ 165.565448][ T5117] __iterate_interfaces+0x104/0x300 [mac80211]
[ 165.571534][ T5117] ? hwsim_send_nullfunc_no_ps+0x80/0x80 [mac80211_hwsim]
[ 165.578491][ T5117] ? mac80211_hwsim_monitor_ack+0x580/0x580 [mac80211_hwsim]
[ 165.586341][ T5117] ieee80211_iterate_active_interfaces_atomic+0x14/0x40 [mac80211]
[ 165.594161][ T5117] hwsim_fops_ps_write+0x99/0x1c0 [mac80211_hwsim]
[ 165.600494][ T5117] simple_attr_write+0x1f1/0x280
[ 165.605284][ T5117] ? simple_attr_read+0x2c0/0x2c0
[ 165.610145][ T5117] ? debugfs_file_get+0x118/0x380
[ 165.615021][ T5117] ? debugfs_file_put+0x80/0x80
[ 165.619709][ T5117] debugfs_attr_write+0x5b/0x80
[ 165.624398][ T5117] full_proxy_write+0xf0/0x180
[ 165.629002][ T5117] vfs_write+0x20f/0xb80
[ 165.633086][ T5117] ? copy_page_range+0x7c0/0x7c0
[ 165.637874][ T5117] ? __ia32_sys_pread64+0x200/0x200
[ 165.642911][ T5117] ? fd_install+0x340/0x340
[ 165.647254][ T5117] ? __fget_light+0x51/0x240
[ 165.651699][ T5117] ksys_write+0xed/0x1c0
[ 165.655781][ T5117] ? __ia32_sys_read+0xc0/0xc0
[ 165.660384][ T5117] ? fput+0x19/0x140
[ 165.664123][ T5117] do_syscall_64+0x38/0xc0
[ 165.668380][ T5117] entry_SYSCALL_64_after_hwframe+0x63/0xcd
[ 165.674108][ T5117] RIP: 0033:0x7f9a1c484f33
[ 165.678365][ T5117] Code: 8b 15 61 ef 0c 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 64 8b 04 25 18 00 00 00 85 c0 75 14 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 55 c3 0f 1f 40 00 48 83 ec 28 48 89 54 24 18
[ 165.697751][ T5117] RSP: 002b:00007fff41807618 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
[ 165.706001][ T5117] RAX: ffffffffffffffda RBX: 000055ce2a11d380 RCX: 00007f9a1c484f33
[ 165.713816][ T5117] RDX: 0000000000000002 RSI: 000055ce2a11d380 RDI: 0000000000000001
[ 165.721618][ T5117] RBP: 0000000000000002 R08: 000055ce2a11d380 R09: 00007f9a1c554be0
[ 165.729432][ T5117] R10: 0000000000000070 R11: 0000000000000246 R12: 0000000000000001
[ 165.737232][ T5117] R13: 0000000000000002 R14: 7fffffffffffffff R15: 0000000000000000
[ 165.745033][ T5117] </TASK>
[ 165.747919][ T5117] ---[ end trace 0000000000000000 ]---
To reproduce:
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
sudo bin/lkp install job.yaml # job file is attached in this email
bin/lkp split-job --compatible job.yaml # generate the yaml file for lkp run
sudo bin/lkp run generated-yaml-file
# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Tue, 2022-09-20 at 12:40 +0200, Jonas Jelonek wrote:
> This patch adds an utility function to mac80211 for conversion between
> ieee80211_tx_rate (mac80211.h) and rate_info (cfg80211.h).
>
> struct ieee80211_tx_rate is space limited to annotate rates up to IEEE
> 802.11ac. The new struct rate_info is able to annotate IEEE 802.11ax
> rates and beyond. Several drivers internally still use ieee80211_tx_rate
> but mac80211 expects rate_info in struct ieee80211_rate_status. This
> struct is in turn required to allow, e.g., tx-power status report or
> dynamic number of mrr stages.
>
> Signed-off-by: Jonas Jelonek <[email protected]>
> ---
> include/net/mac80211.h | 4 ++++
> net/mac80211/util.c | 35 +++++++++++++++++++++++++++++++++++
> 2 files changed, 39 insertions(+)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index c4b55c7273ed..f17a03caa361 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -1051,6 +1051,10 @@ ieee80211_rate_get_vht_nss(const struct ieee80211_tx_rate *rate)
> return (rate->idx >> 4) + 1;
> }
>
> +void ieee80211_rate_get_rate_info(const struct ieee80211_tx_rate *rate,
> + struct wiphy *wiphy, u8 band,
> + struct rate_info *rate_info);
> +
> /**
> * struct ieee80211_tx_info - skb transmit information
> *
> diff --git a/net/mac80211/util.c b/net/mac80211/util.c
> index 0ea5d50091dc..c76dc255bec3 100644
> --- a/net/mac80211/util.c
> +++ b/net/mac80211/util.c
> @@ -4867,3 +4867,38 @@ void ieee80211_fragment_element(struct sk_buff *skb, u8 *len_pos)
>
> *len_pos = elem_len;
> }
> +
> +
nit: use just one blank line.
johannes
> +void ieee80211_rate_get_rate_info(const struct ieee80211_tx_rate *rate,
> + struct wiphy *wiphy, u8 band,
> + struct rate_info *rate_info)
> +{
> + memset(rate_info, 0, sizeof(struct rate_info));
> +
> + if (rate->flags & IEEE80211_TX_RC_MCS) { /* 802.11n */
> + rate_info->flags |= RATE_INFO_FLAGS_MCS;
> + rate_info->mcs = rate->idx;
> + } else if (rate->flags & IEEE80211_TX_RC_VHT_MCS) { /* 802.11ac */
> + rate_info->flags |= RATE_INFO_FLAGS_VHT_MCS;
> + rate_info->mcs = ieee80211_rate_get_vht_mcs(rate);
> + rate_info->nss = ieee80211_rate_get_vht_nss(rate);
> + } else { /* 802.11a/b/g */
what about HE/EHT?
johannes
On Tue, 2022-09-20 at 12:40 +0200, Jonas Jelonek wrote:
>
> +/**
> + * struct ieee80211_hw_txpower_range - Power range for transmit power
> + *
> + * This struct can be used by drivers to define multiple tx-power ranges with
> + * different scales according to the hardware capabilities. A tx-power range
> + * describe either absolute power levels or power offsets relative to a base
> + * power.
> + *
> + * @start_idx The starting idx of the range. @start_idx is always the lowest
> + * idx of the power range.
> + * @start_pwr The power level at idx @start_idx in 0.25 dBm units.
> + * @n_levels How many power levels this range has.
> + * @pwr_step The power step width in 0.25 dBm units.
Need colons for valid kernel-doc, I believe.
johannes
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 <[email protected]>
> ---
> 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
On Tue, 2022-09-20 at 12:40 +0200, Jonas Jelonek wrote:
>
> * @rate_idx The actual used rate.
> * @try_count How often the rate was tried.
> - * @tx_power_idx An idx into the ieee80211_hw->tx_power_levels list of the
> - * corresponding wifi hardware. The idx shall point to the power level
> - * that was used when sending the packet.
> + * @tx_power_idx An idx into the the tx-power set described by
> + * ieee80211_hw->txpower_ranges for the corresponding wifi hardware.
> + * The idx shall point to the tx-power level that was used when sending
> + * the packet at this rate. A negative value is considered as 'invalid'
> + * or 'no power level reported by the driver'.
maybe fix the kernel-doc (at least for this entry) while at it and add
the missing colon.
johannes
On Tue, 2022-09-20 at 12:40 +0200, Jonas Jelonek wrote:
> Create a new debugfs entry called 'rc_fixed_txpower_idx' in debugfs dir
> for each station. By writing a positive static tx-power idx into this
> file, minstrel_ht applies this tx-power idx to each packet or each mrr
> stage, depending on what the hardware supports. By writing (u32)-1 to
> the file, minstrel will return to automatic mode which currently just
> passes -1 as tx-power idx, indicating that the driver should use a
> default.
> The debugfs entry will only be created if either tpc per packet or per
> mrr is supported.
>
> Signed-off-by: Jonas Jelonek <[email protected]>
> ---
> net/mac80211/rc80211_minstrel_ht.c | 14 ++++++++++++++
> net/mac80211/rc80211_minstrel_ht.h | 11 +++++++++++
> net/mac80211/rc80211_minstrel_ht_debugfs.c | 11 +++++++++++
> 3 files changed, 36 insertions(+)
>
> diff --git a/net/mac80211/rc80211_minstrel_ht.c b/net/mac80211/rc80211_minstrel_ht.c
> index 24c3c055db6d..222b51e7d9ee 100644
> --- a/net/mac80211/rc80211_minstrel_ht.c
> +++ b/net/mac80211/rc80211_minstrel_ht.c
> @@ -1486,6 +1486,14 @@ minstrel_ht_set_rate(struct minstrel_priv *mp, struct minstrel_ht_sta *mi,
>
> ratetbl->rate[offset].idx = idx;
> ratetbl->rate[offset].flags = flags;
> +
> +#ifdef CONFIG_MAC80211_DEBUGFS
> + if (mi->fixed_txpower_idx != -1) {
> + ratetbl->rate[offset].txpower_idx = mi->fixed_txpower_idx;
> + return;
> + }
> +#endif
> + ratetbl->rate[offset].txpower_idx = -1;
> }
>
> static inline int
> @@ -1603,8 +1611,14 @@ minstrel_ht_get_rate(void *priv, struct ieee80211_sta *sta, void *priv_sta,
> info->flags |= mi->tx_flags;
>
> #ifdef CONFIG_MAC80211_DEBUGFS
> + if (mi->fixed_txpower_idx != -1)
> + info->control.txpower_idx = mi->fixed_txpower_idx;
> +
> if (mp->fixed_rate_idx != -1)
> return;
> +#else
> + /* Pass -1 to indicate 'ignore txpower' or 'use default' */
> + info->control.txpower_idx = -1;
> #endif
>
> /* Don't use EAPOL frames for sampling on non-mrr hw */
> diff --git a/net/mac80211/rc80211_minstrel_ht.h b/net/mac80211/rc80211_minstrel_ht.h
> index 1766ff0c78d3..15222d66c4b8 100644
> --- a/net/mac80211/rc80211_minstrel_ht.h
> +++ b/net/mac80211/rc80211_minstrel_ht.h
> @@ -194,6 +194,17 @@ struct minstrel_ht_sta {
>
> /* MCS rate group info and statistics */
> struct minstrel_mcs_group_data groups[MINSTREL_GROUPS_NB];
> +
> +#ifdef CONFIG_MAC80211_DEBUGFS
> + /*
> + * enable fixed tx-power processing per STA
> + * - write static index to debugfs:ieee80211/phyX/netdev:wlanY
> + * /stations/<MAC>/rc_fixed_txpower_idx
> + * - write -1 to enable automatic processing again
> + * - setting will be applied on next update
> + */
> + u32 fixed_txpower_idx;
Use s32? You don't need that large range anyway.
> +
> + if (ieee80211_hw_check(mp->hw, SUPPORTS_TPC_PER_PACKET) ||
> + ieee80211_hw_check(mp->hw, SUPPORTS_TPC_PER_MRR))
> + {
bad indentation
> + mi->fixed_txpower_idx = (u32)-1;
> + debugfs_create_u32("rc_fixed_txpower_idx", S_IRUGO | S_IWUGO,
> + dir, &mi->fixed_txpower_idx);
>
Seems like that should be a function with some range check?
johannes
> On 12. Jan 2023, at 11:26, Johannes Berg <[email protected]> wrote:
>
>> +void ieee80211_rate_get_rate_info(const struct ieee80211_tx_rate *rate,
>> + struct wiphy *wiphy, u8 band,
>> + struct rate_info *rate_info)
>> +{
>> + memset(rate_info, 0, sizeof(struct rate_info));
>> +
>> + if (rate->flags & IEEE80211_TX_RC_MCS) { /* 802.11n */
>> + rate_info->flags |= RATE_INFO_FLAGS_MCS;
>> + rate_info->mcs = rate->idx;
>> + } else if (rate->flags & IEEE80211_TX_RC_VHT_MCS) { /* 802.11ac */
>> + rate_info->flags |= RATE_INFO_FLAGS_VHT_MCS;
>> + rate_info->mcs = ieee80211_rate_get_vht_mcs(rate);
>> + rate_info->nss = ieee80211_rate_get_vht_nss(rate);
>> + } else { /* 802.11a/b/g */
>
> what about HE/EHT?
ieee80211_tx_rate uses an s8 for rate/MCS index, so only up to VHT rates fit in there.
For rates above VHT, rate_info is needed, thus are are no HE/EHT rates occuring in
ieee80211_tx_rate. Same applies to your comment on the hwsim conversion.
Jonas
On Thu, 2023-01-19 at 12:31 +0100, Jonas Jelonek wrote:
> > On 12. Jan 2023, at 11:26, Johannes Berg <[email protected]>
> > wrote:
> >
> > > +void ieee80211_rate_get_rate_info(const struct ieee80211_tx_rate
> > > *rate,
> > > + struct wiphy
> > > *wiphy, u8 band,
> > > + struct rate_info
> > > *rate_info)
> > > +{
> > > + memset(rate_info, 0, sizeof(struct rate_info));
> > > +
> > > + if (rate->flags & IEEE80211_TX_RC_MCS) { /* 802.11n */
> > > + rate_info->flags |= RATE_INFO_FLAGS_MCS;
> > > + rate_info->mcs = rate->idx;
> > > + } else if (rate->flags & IEEE80211_TX_RC_VHT_MCS) { /*
> > > 802.11ac */
> > > + rate_info->flags |= RATE_INFO_FLAGS_VHT_MCS;
> > > + rate_info->mcs =
> > > ieee80211_rate_get_vht_mcs(rate);
> > > + rate_info->nss =
> > > ieee80211_rate_get_vht_nss(rate);
> > > + } else { /* 802.11a/b/g */
> >
> > what about HE/EHT?
>
> ieee80211_tx_rate uses an s8 for rate/MCS index, so only up to VHT
> rates fit in there.
> For rates above VHT, rate_info is needed, thus are are no HE/EHT rates
> occuring in
> ieee80211_tx_rate. Same applies to your comment on the hwsim
> conversion.
I guess I should've read the commit message more closely ;-)
But please add kernel-doc to the function; both in general it'd be good
to have, and in particular explaining that this is more for older
drivers I guess?
johannes
> On 12. Jan 2023, at 11:31, Johannes Berg <[email protected]> wrote:
>
> 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 <[email protected]>
>> ---
>> 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?
just to make sure I got it correctly: so you propose that these params, that are
currently done with module_param(), should be switched to a dynamic netlink approach,
or only for TPC and RCTBL for now?
As a first step I focused on providing a proof-of-concept implementation in hwsim for my
TPC proposal, implementing netlink to set tx power and other could be part of the next step.
Do you think this could be fine or do you propose something different?
(all other comments from your side I will fix in v3)
Jonas
> On 19. Jan 2023, at 12:35, Johannes Berg <[email protected]> wrote:
>
> On Thu, 2023-01-19 at 12:31 +0100, Jonas Jelonek wrote:
>>> On 12. Jan 2023, at 11:26, Johannes Berg <[email protected]>
>>> wrote:
>>>
>>>> +void ieee80211_rate_get_rate_info(const struct ieee80211_tx_rate *rate,
>>>> + struct wiphy *wiphy, u8 band,
>>>> + struct rate_info *rate_info)
>>>> +{
>>>> + memset(rate_info, 0, sizeof(struct rate_info));
>>>> +
>>>> + if (rate->flags & IEEE80211_TX_RC_MCS) { /* 802.11n */
>>>> + rate_info->flags |= RATE_INFO_FLAGS_MCS;
>>>> + rate_info->mcs = rate->idx;
>>>> + } else if (rate->flags & IEEE80211_TX_RC_VHT_MCS) { /* 802.11ac */
>>>> + rate_info->flags |= RATE_INFO_FLAGS_VHT_MCS;
>>>> + rate_info->mcs = ieee80211_rate_get_vht_mcs(rate);
>>>> + rate_info->nss = ieee80211_rate_get_vht_nss(rate);
>>>> + } else { /* 802.11a/b/g */
>>>
>>> what about HE/EHT?
>>
>> ieee80211_tx_rate uses an s8 for rate/MCS index, so only up to VHT
>> rates fit in there.
>> For rates above VHT, rate_info is needed, thus are are no HE/EHT rates
>> occuring in
>> ieee80211_tx_rate. Same applies to your comment on the hwsim
>> conversion.
>
> I guess I should've read the commit message more closely ;-)
>
> But please add kernel-doc to the function; both in general it'd be good
> to have, and in particular explaining that this is more for older
> drivers I guess?
I will add that in my next RFC version to make sure this is clear.
Jonas
On Thu, 2023-01-19 at 15:32 +0100, Jonas Jelonek wrote:
> >
> > Not sure I like this either - I think we should probably create the
> > wiphys dynamically for most features these days?
>
> just to make sure I got it correctly: so you propose that these
> params, that are
> currently done with module_param(), should be switched to a dynamic
> netlink approach, or only for TPC and RCTBL for now?
We do have dynamic parameters for all the module parameters I believe,
but we've shied away from actually removing the existing module
parameters for legacy/compatibility reasons.
However, I think that for new parameters, there's really no good reason
to provide module parameters, since the test scripting etc. can
dynamically create wiphys with the necessary capabilities. Even the
hostap/hwsim tests can and do already do that :)
> As a first step I focused on providing a proof-of-concept
> implementation in hwsim for my
> TPC proposal, implementing netlink to set tx power and other could be
> part of the next step.
> Do you think this could be fine or do you propose something different?
I'm not quite sure what you mean by that, tbh. I guess I kind of thought
you were going to adjust minstrel to do TPC automatically.
We already have netlink support for setting per-station TX power which I
guess this should then listen to? See NL80211_ATTR_STA_TX_POWER_SETTING
and NL80211_ATTR_STA_TX_POWER etc. I think it's not supported in
mac80211, but probably could easily be after your patches?
johannes
> On 19. Jan 2023, at 16:09, Johannes Berg <[email protected]> wrote:
>
> On Thu, 2023-01-19 at 15:32 +0100, Jonas Jelonek wrote:
>>>
>>> Not sure I like this either - I think we should probably create the
>>> wiphys dynamically for most features these days?
>>
>> just to make sure I got it correctly: so you propose that these
>> params, that are
>> currently done with module_param(), should be switched to a dynamic
>> netlink approach, or only for TPC and RCTBL for now?
>
> We do have dynamic parameters for all the module parameters I believe,
> but we've shied away from actually removing the existing module
> parameters for legacy/compatibility reasons.
>
> However, I think that for new parameters, there's really no good reason
> to provide module parameters, since the test scripting etc. can
> dynamically create wiphys with the necessary capabilities. Even the
> hostap/hwsim tests can and do already do that :)
From what I’ve seen there is no dynamic parameter for RCTBL yet but I can combine
this with my additional TPC parameter. Then this can be set via netlink.
>> As a first step I focused on providing a proof-of-concept
>> implementation in hwsim for my
>> TPC proposal, implementing netlink to set tx power and other could be
>> part of the next step.
>> Do you think this could be fine or do you propose something different?
>
> I'm not quite sure what you mean by that, tbh. I guess I kind of thought
> you were going to adjust minstrel to do TPC automatically.
>
> We already have netlink support for setting per-station TX power which I
> guess this should then listen to? See NL80211_ATTR_STA_TX_POWER_SETTING
> and NL80211_ATTR_STA_TX_POWER etc. I think it's not supported in
> mac80211, but probably could easily be after your patches?
I guess that can be part of some follow-up patches after these patches here are upstream.
I would agree that this should somehow listen to the mentioned attributes then.
We want to do joint RC and TPC in minstrel, and to allow fine-grained TPC as it is already
possible with RC. Minstrel will also be adjusted in one of the next steps.
This RFC basically should “prepare” mac80211 to be used for fine-grained TPC. I think,
driver support and Minstrel support should be the next steps after the structures are fixed.
But I include hwsim here to have at least a test-case. Hope you get what I mean :)
Jonas
> On 12. Jan 2023, at 11:31, Johannes Berg <[email protected]> wrote:
>
> On Tue, 2022-09-20 at 12:40 +0200, Jonas Jelonek wrote:
>> @@ -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.
Yes, I see. Is it in general a bad idea to take the sta reference from ieee80211_control, put
it in rate_driver_data and use it for tx-status? I guess I should pass sta to tx_status_ext whenever
possible because it is used for several statistics.
I could think of two ways:
- add NULL checks for the case that the sta pointer might be freed as you said
- get sta by using, e.g., sta_info_get_by_addrs to get the sta if it is available. However, this always
loops through the sta list. Might be a performance issue?
Or do you suggest something different?
Jonas
On Thu, 2023-01-26 at 17:53 +0100, Jonas Jelonek wrote:
> >
> > On Tue, 2022-09-20 at 12:40 +0200, Jonas Jelonek wrote:
> > > @@ -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.
>
> Yes, I see. Is it in general a bad idea to take the sta reference from
> ieee80211_control, put
> it in rate_driver_data and use it for tx-status? I guess I should pass
> sta to tx_status_ext whenever
> possible because it is used for several statistics.
Well you have to think about the lifetime. In most cases you do a lookup
of the STA (under RCU) etc. but
> I could think of two ways:
> - add NULL checks for the case that the sta pointer might be freed as
> you said
How would that pointer even go NULL though? The pointer would remain,
but the STA can be freed, no?
> - get sta by using, e.g., sta_info_get_by_addrs to get the sta if it
> is available. However, this always
> loops through the sta list. Might be a performance issue?
It should use the hashtable?
> Or do you suggest something different?
Well you could keep it here and walk the list of queued skbs (?) when a
STA is removed, and kill them all at that point, or something. Not sure
it's worth it vs. the hash table lookup, this is just hwsim after all.
johannes
> On 28. Feb 2023, at 18:41, Johannes Berg <[email protected]> wrote:
>
> On Thu, 2023-01-26 at 15:26 +0100, Jonas Jelonek wrote:
>>>
>>> However, I think that for new parameters, there's really no good
>>> reason
>>> to provide module parameters, since the test scripting etc. can
>>> dynamically create wiphys with the necessary capabilities. Even the
>>> hostap/hwsim tests can and do already do that :)
>>
>> From what I’ve seen there is no dynamic parameter for RCTBL yet but I
>> can combine
>> this with my additional TPC parameter. Then this can be set via
>> netlink.
>
> Fair enough, but yeah, I think we should move to that.
>
>>> We already have netlink support for setting per-station TX power
>>> which I guess this should then listen to? See
>>> NL80211_ATTR_STA_TX_POWER_SETTING
>>> and NL80211_ATTR_STA_TX_POWER etc. I think it's not supported in
>>> mac80211, but probably could easily be after your patches?
>>
>> I guess that can be part of some follow-up patches after these patches
>> here are upstream.
>> I would agree that this should somehow listen to the mentioned
>> attributes then.
>
> OK.
>
>> We want to do joint RC and TPC in minstrel, and to allow fine-grained
>> TPC as it is already possible with RC. Minstrel will also be adjusted in
>> one of the next steps.
>> This RFC basically should “prepare” mac80211 to be used for fine-
>> grained TPC. I think, driver support and Minstrel support should be the
>> next steps after the structures are fixed.
>> But I include hwsim here to have at least a test-case. Hope you get
>> what I mean :)
>
> Yep, seems good.
>
> I'm slightly worried we'll add this and never get to do the minstrel
> part, but hey.
This is also part of our research so we are going to either implement TPC in
minstrel or export this to user space. Our research also includes looking at
doing RC + TPC in user space with more advanced algorithms.
> Also, most modern devices no longer even use minstrel, so is it even
> worth doing at all from that perspective? What's in it for the users
> who've been using their devices for years (since newer devices in the
> past few years haven't used it, I think) without it?
>
> johannes
Our goal and hope is that, we explore that and then can show some benefits of
more advanced RC and TPC algorithm, so vendors like Atheros and Mediatek
may again expose the MRR capabilities to the mac80211 layer with future
chips. Afaik, the last Mediatek chips supporting mrr rate setting are mt76xx, which
I guess are still pretty common, so we are doing our work on still relevant hardware.
After having a short look at the driver code, it seems like more recent chips (in
particular Mediatek) at least kept support for controlling the TX power per packet.
I am not sure if it could be a good idea to split that up, not having TX power in the
RC table but somehow separate. Then you would be able to do TPC without RC.
Jonas
> On 28. Feb 2023, at 18:44, Johannes Berg <[email protected]> wrote:
>
> On Thu, 2023-01-26 at 17:53 +0100, Jonas Jelonek wrote:
>>>
>>> On Tue, 2022-09-20 at 12:40 +0200, Jonas Jelonek wrote:
>>>> @@ -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.
>>
>> Yes, I see. Is it in general a bad idea to take the sta reference from
>> ieee80211_control, put
>> it in rate_driver_data and use it for tx-status? I guess I should pass
>> sta to tx_status_ext whenever
>> possible because it is used for several statistics.
>
> Well you have to think about the lifetime. In most cases you do a lookup
> of the STA (under RCU) etc. but
>
>> I could think of two ways:
>> - add NULL checks for the case that the sta pointer might be freed as
>> you said
>
> How would that pointer even go NULL though? The pointer would remain,
> but the STA can be freed, no?
Yes, you’re right. Sorry, I mixed that up somehow.
>
>> - get sta by using, e.g., sta_info_get_by_addrs to get the sta if it
>> is available. However, this always
>> loops through the sta list. Might be a performance issue?
>
> It should use the hashtable?
>
>> Or do you suggest something different?
>
> Well you could keep it here and walk the list of queued skbs (?) when a
> STA is removed, and kill them all at that point, or something. Not sure
> it's worth it vs. the hash table lookup, this is just hwsim after all.
>
> johannes
That’s also correct. Sorry, I didn’t have a deeper look into this function at the
time of writing. So I guess I will stick to this for now.
Jonas