| Message ID | 20220920104032.496697-6-jelonek.jonas@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | mac80211: add TPC support in control path | expand |
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/20220920104032.496697-6-jelonek.jonas@gmail.com
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 <oliver.sang@intel.com>
| Link: https://lore.kernel.org/r/202209261514.98322e99-oliver.sang@intel.com
[ 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.
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@gmail.com> > --- > 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 12. Jan 2023, at 11:31, Johannes Berg <johannes@sipsolutions.net> 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 <jelonek.jonas@gmail.com> >> --- >> 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 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 <johannes@sipsolutions.net> 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 <johannes@sipsolutions.net> 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 <johannes@sipsolutions.net> 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 <johannes@sipsolutions.net> 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
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; /**
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@gmail.com> --- drivers/net/wireless/mac80211_hwsim.c | 175 ++++++++++++++++++++++++-- drivers/net/wireless/mac80211_hwsim.h | 1 + 2 files changed, 168 insertions(+), 8 deletions(-)