As previously discussed in [1] we have a mismatch in struct scan_req_params
between the bitfield scan_f_xxx & the scan_flags when used with WMI_SCAN_XXX
values.
To fix the issue & prevent it from happenning again lets stop using & remove
scan_flags altogether in the driver and only use the bitfield instead. That way
even if the bitfield doesn't match the WMI_SCAN_XXX flags, the right value will
still be sent through the wmi command to the firmware (see
ath11k_wmi_copy_scan_event_cntrl_flags).
Questions:
- In the same rationale shouldn't we remove scan_events from the same struct ?
- The same goes for ath12k, should I send a seperate patch or respin a v3 with
similar patches for ath12k ?
[1] https://lore.kernel.org/all/[email protected]/
v2:
- remove explicit uses of scan_flags with WMI_SCAN_XXX flags
- remove the underlying union of scan_flags to only leave the bitfield
Nicolas Escande (2):
wifi: ath11k: Do not directly use scan_flags in struct scan_req_params
wifi: ath11k: remove scan_flags union from struct scan_req_params
drivers/net/wireless/ath/ath11k/mac.c | 6 +--
drivers/net/wireless/ath/ath11k/wmi.c | 2 +-
drivers/net/wireless/ath/ath11k/wmi.h | 55 ++++++++++++---------------
3 files changed, 29 insertions(+), 34 deletions(-)
--
2.43.0
Now that we do not use scan_flags directly with WMI_SCAN_XXX macros anymore but
only the bitfield scan_f_xxx, lets remove the scan_flags & the underlying union.
This will prevent further problems as some entries in the scan_f_xxx bitfield
don't match their corresponding WMI_SCAN_XXX flags as seen in [1]
[1] https://lore.kernel.org/all/[email protected]/
Signed-off-by: Nicolas Escande <[email protected]>
---
drivers/net/wireless/ath/ath11k/wmi.h | 55 ++++++++++++---------------
1 file changed, 25 insertions(+), 30 deletions(-)
diff --git a/drivers/net/wireless/ath/ath11k/wmi.h b/drivers/net/wireless/ath/ath11k/wmi.h
index 6dcd14700570..bc7adf7f13fb 100644
--- a/drivers/net/wireless/ath/ath11k/wmi.h
+++ b/drivers/net/wireless/ath/ath11k/wmi.h
@@ -3394,36 +3394,31 @@ struct scan_req_params {
u32 idle_time;
u32 max_scan_time;
u32 probe_delay;
- union {
- struct {
- u32 scan_f_passive:1,
- scan_f_bcast_probe:1,
- scan_f_cck_rates:1,
- scan_f_ofdm_rates:1,
- scan_f_chan_stat_evnt:1,
- scan_f_filter_prb_req:1,
- scan_f_bypass_dfs_chn:1,
- scan_f_continue_on_err:1,
- scan_f_offchan_mgmt_tx:1,
- scan_f_offchan_data_tx:1,
- scan_f_promisc_mode:1,
- scan_f_capture_phy_err:1,
- scan_f_strict_passive_pch:1,
- scan_f_half_rate:1,
- scan_f_quarter_rate:1,
- scan_f_force_active_dfs_chn:1,
- scan_f_add_tpc_ie_in_probe:1,
- scan_f_add_ds_ie_in_probe:1,
- scan_f_add_spoofed_mac_in_probe:1,
- scan_f_add_rand_seq_in_probe:1,
- scan_f_en_ie_whitelist_in_probe:1,
- scan_f_forced:1,
- scan_f_2ghz:1,
- scan_f_5ghz:1,
- scan_f_80mhz:1;
- };
- u32 scan_flags;
- };
+ u32 scan_f_passive:1,
+ scan_f_bcast_probe:1,
+ scan_f_cck_rates:1,
+ scan_f_ofdm_rates:1,
+ scan_f_chan_stat_evnt:1,
+ scan_f_filter_prb_req:1,
+ scan_f_bypass_dfs_chn:1,
+ scan_f_continue_on_err:1,
+ scan_f_offchan_mgmt_tx:1,
+ scan_f_offchan_data_tx:1,
+ scan_f_promisc_mode:1,
+ scan_f_capture_phy_err:1,
+ scan_f_strict_passive_pch:1,
+ scan_f_half_rate:1,
+ scan_f_quarter_rate:1,
+ scan_f_force_active_dfs_chn:1,
+ scan_f_add_tpc_ie_in_probe:1,
+ scan_f_add_ds_ie_in_probe:1,
+ scan_f_add_spoofed_mac_in_probe:1,
+ scan_f_add_rand_seq_in_probe:1,
+ scan_f_en_ie_whitelist_in_probe:1,
+ scan_f_forced:1,
+ scan_f_2ghz:1,
+ scan_f_5ghz:1,
+ scan_f_80mhz:1;
enum scan_dwelltime_adaptive_mode adaptive_dwell_time_mode;
u32 burst_duration;
u32 num_chan;
--
2.43.0
On 2/9/2024 3:35 AM, Nicolas Escande wrote:
> As previously discussed in [1] we have a mismatch in struct scan_req_params
> between the bitfield scan_f_xxx & the scan_flags when used with WMI_SCAN_XXX
> values.
>
> To fix the issue & prevent it from happenning again lets stop using & remove
> scan_flags altogether in the driver and only use the bitfield instead. That way
> even if the bitfield doesn't match the WMI_SCAN_XXX flags, the right value will
> still be sent through the wmi command to the firmware (see
> ath11k_wmi_copy_scan_event_cntrl_flags).
>
> Questions:
> - In the same rationale shouldn't we remove scan_events from the same struct ?
yes, scan_events is unused so it and the union+struct can be removed,
leaving just the u32 bitfield. feel free to submit that as a separate patch
> - The same goes for ath12k, should I send a seperate patch or respin a v3 with
> similar patches for ath12k ?
please send a separate series for ath12k since it has a separate mailing
list
>
> [1] https://lore.kernel.org/all/[email protected]/
>
> v2:
> - remove explicit uses of scan_flags with WMI_SCAN_XXX flags
> - remove the underlying union of scan_flags to only leave the bitfield
>
> Nicolas Escande (2):
> wifi: ath11k: Do not directly use scan_flags in struct scan_req_params
> wifi: ath11k: remove scan_flags union from struct scan_req_params
>
> drivers/net/wireless/ath/ath11k/mac.c | 6 +--
> drivers/net/wireless/ath/ath11k/wmi.c | 2 +-
> drivers/net/wireless/ath/ath11k/wmi.h | 55 ++++++++++++---------------
> 3 files changed, 29 insertions(+), 34 deletions(-)
>
Thank you for this cleanup!
/jeff
On 2/9/2024 3:35 AM, Nicolas Escande wrote:
> Now that we do not use scan_flags directly with WMI_SCAN_XXX macros anymore but
> only the bitfield scan_f_xxx, lets remove the scan_flags & the underlying union.
>
> This will prevent further problems as some entries in the scan_f_xxx bitfield
> don't match their corresponding WMI_SCAN_XXX flags as seen in [1]
>
> [1] https://lore.kernel.org/all/[email protected]/
>
> Signed-off-by: Nicolas Escande <[email protected]>
Acked-by: Jeff Johnson <[email protected]>