| Message ID | 20231127180559.1696041-1-nico.escande@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: ath11k: fix layout of scan_flags in struct scan_req_params | expand |
On 11/27/2023 10:05 AM, Nicolas Escande wrote: > The is a layout mismatch between the bitfield representing scan_flags in > struct scan_req_params & the bits as defined in the WMI_SCAN_XXX macros. > Lets fix it by making the struct match the #defines. > > I tried to correct it by making the struct match the #define and it > worked for WMI_SCAN_FLAG_FORCE_ACTIVE_ON_DFS / scan_f_force_active_dfs_chn > so I'm assuming this is the right thing to do. > > Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.7.0.1-01744-QCAHKSWPL_SILICONZ-1 > > Signed-off-by: Nicolas Escande <nico.escande@gmail.com> > --- > drivers/net/wireless/ath/ath11k/wmi.h | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath11k/wmi.h b/drivers/net/wireless/ath/ath11k/wmi.h > index 100bb816b592..0b4e6c2f7860 100644 > --- a/drivers/net/wireless/ath/ath11k/wmi.h > +++ b/drivers/net/wireless/ath/ath11k/wmi.h > @@ -3348,17 +3348,17 @@ struct scan_req_params { > scan_f_filter_prb_req:1, > scan_f_bypass_dfs_chn:1, > scan_f_continue_on_err:1, > + scan_f_promisc_mode: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_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, You are convoluting two different data structures. struct scan_req_params is used to represent a scan request within the host driver. This does not use the WMI_SCAN_XXX macros. struct wmi_start_scan_cmd is used to represent the scan request command sent to firmware. This struct uses the WMI_SCAN_XXX macros to fill some members of this struct in ath11k_wmi_copy_scan_event_cntrl_flags(). So your change has no effect on the driver operation and incorrectly tries to foist the firmware definition upon the host internal representation. So NAK to this patch. /jeff
On Mon Nov 27, 2023 at 7:38 PM CET, Jeff Johnson wrote: > On 11/27/2023 10:05 AM, Nicolas Escande wrote: > > The is a layout mismatch between the bitfield representing scan_flags in > > struct scan_req_params & the bits as defined in the WMI_SCAN_XXX macros. > > Lets fix it by making the struct match the #defines. > > > > I tried to correct it by making the struct match the #define and it > > worked for WMI_SCAN_FLAG_FORCE_ACTIVE_ON_DFS / scan_f_force_active_dfs_chn > > so I'm assuming this is the right thing to do. > > > > Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.7.0.1-01744-QCAHKSWPL_SILICONZ-1 > > > > Signed-off-by: Nicolas Escande <nico.escande@gmail.com> > > --- > > drivers/net/wireless/ath/ath11k/wmi.h | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/wireless/ath/ath11k/wmi.h b/drivers/net/wireless/ath/ath11k/wmi.h > > index 100bb816b592..0b4e6c2f7860 100644 > > --- a/drivers/net/wireless/ath/ath11k/wmi.h > > +++ b/drivers/net/wireless/ath/ath11k/wmi.h > > @@ -3348,17 +3348,17 @@ struct scan_req_params { > > scan_f_filter_prb_req:1, > > scan_f_bypass_dfs_chn:1, > > scan_f_continue_on_err:1, > > + scan_f_promisc_mode: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_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, > > You are convoluting two different data structures. So maybe I'm missing something and please correct me where I'm wrong. > struct scan_req_params is used to represent a scan request within the > host driver. This does not use the WMI_SCAN_XXX macros. > In mac.c when we start a scan with ath11k_mac_op_hw_scan() for example we first initialize a struct scan_req_params with ath11k_wmi_start_scan_init(). ath11k_wmi_start_scan_init() by itself does use the WMI_SCAN_XXX macros arg->scan_flags |= WMI_SCAN_CHAN_STAT_EVENT; Then later on in ath11k_mac_op_hw_scan() we either use the bitfield like with arg->scan_f_add_spoofed_mac_in_probe = 1; or we directly modify scan_flags like with arg->scan_flags |= WMI_SCAN_FLAG_PASSIVE; So is it not expected to use those flags there ? > struct wmi_start_scan_cmd is used to represent the scan request command > sent to firmware. This struct uses the WMI_SCAN_XXX macros to fill some > members of this struct in ath11k_wmi_copy_scan_event_cntrl_flags(). Indeed ath11k_wmi_copy_scan_event_cntrl_flags() copies from struct scan_req_params to struct wmi_start_scan_cmd but this time we do not use scan_flags directly, only ever use the bitfield that is in the same union as scan_flags So having the bitfield out of sync does cause the struct wmi_start_scan_cmd that gets sent to the driver to not reflect the desired state set in scan_req_params. > So your change has no effect on the driver operation and incorrectly > tries to foist the firmware definition upon the host internal > representation. So either we should not use WMI_SCAN_XXX with scan_req_params.scan_flags ever and only use the bitfield to set scan parameters or if we use WMI_SCAN_XXX with scan_req_params.scan_flags they need to match the corresponding bitfield. > > So NAK to this patch. > > /jeff
On 11/27/2023 2:54 PM, Nicolas Escande wrote: > On Mon Nov 27, 2023 at 7:38 PM CET, Jeff Johnson wrote: >> On 11/27/2023 10:05 AM, Nicolas Escande wrote: >>> The is a layout mismatch between the bitfield representing scan_flags in >>> struct scan_req_params & the bits as defined in the WMI_SCAN_XXX macros. >>> Lets fix it by making the struct match the #defines. >>> >>> I tried to correct it by making the struct match the #define and it >>> worked for WMI_SCAN_FLAG_FORCE_ACTIVE_ON_DFS / scan_f_force_active_dfs_chn >>> so I'm assuming this is the right thing to do. >>> >>> Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.7.0.1-01744-QCAHKSWPL_SILICONZ-1 >>> >>> Signed-off-by: Nicolas Escande <nico.escande@gmail.com> >>> --- >>> drivers/net/wireless/ath/ath11k/wmi.h | 10 +++++----- >>> 1 file changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/net/wireless/ath/ath11k/wmi.h b/drivers/net/wireless/ath/ath11k/wmi.h >>> index 100bb816b592..0b4e6c2f7860 100644 >>> --- a/drivers/net/wireless/ath/ath11k/wmi.h >>> +++ b/drivers/net/wireless/ath/ath11k/wmi.h >>> @@ -3348,17 +3348,17 @@ struct scan_req_params { >>> scan_f_filter_prb_req:1, >>> scan_f_bypass_dfs_chn:1, >>> scan_f_continue_on_err:1, >>> + scan_f_promisc_mode: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_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, >> >> You are convoluting two different data structures. > > So maybe I'm missing something and please correct me where I'm wrong. > >> struct scan_req_params is used to represent a scan request within the >> host driver. This does not use the WMI_SCAN_XXX macros. >> > > In mac.c when we start a scan with ath11k_mac_op_hw_scan() for example we first > initialize a struct scan_req_params with ath11k_wmi_start_scan_init(). > ath11k_wmi_start_scan_init() by itself does use the WMI_SCAN_XXX macros > > arg->scan_flags |= WMI_SCAN_CHAN_STAT_EVENT; > > Then later on in ath11k_mac_op_hw_scan() we either use the bitfield like with > > arg->scan_f_add_spoofed_mac_in_probe = 1; > > or we directly modify scan_flags like with > > arg->scan_flags |= WMI_SCAN_FLAG_PASSIVE; > > So is it not expected to use those flags there ? IMO it is unexpected to use those flag there. And I'm actually surprised we have the union there. > >> struct wmi_start_scan_cmd is used to represent the scan request command >> sent to firmware. This struct uses the WMI_SCAN_XXX macros to fill some >> members of this struct in ath11k_wmi_copy_scan_event_cntrl_flags(). And IMO that is broken. We should only be using the bitfields. > > Indeed ath11k_wmi_copy_scan_event_cntrl_flags() copies from struct > scan_req_params to struct wmi_start_scan_cmd but this time we do not use > scan_flags directly, only ever use the bitfield that is in the same union > as scan_flags > > So having the bitfield out of sync does cause the struct wmi_start_scan_cmd that > gets sent to the driver to not reflect the desired state set in scan_req_params. > >> So your change has no effect on the driver operation and incorrectly >> tries to foist the firmware definition upon the host internal >> representation. > > So either we should not use WMI_SCAN_XXX with scan_req_params.scan_flags ever > and only use the bitfield to set scan parameters or if we use WMI_SCAN_XXX with > scan_req_params.scan_flags they need to match the corresponding bitfield. IMO the correct thing to do is to remove the unions from that struct and only leave behind the bitfields and not use the WMI_SCAN_XXX masks except when filling the firmware structure. But don't spin an update to your patches until Kalle has a chance to give his opinion. I'm new to maintaining these drivers and Kalle may have a different opinion on this. /jeff
On Tue Nov 28, 2023 at 1:57 AM CET, Jeff Johnson wrote: > On 11/27/2023 2:54 PM, Nicolas Escande wrote: [...] > > So either we should not use WMI_SCAN_XXX with scan_req_params.scan_flags ever > > and only use the bitfield to set scan parameters or if we use WMI_SCAN_XXX with > > scan_req_params.scan_flags they need to match the corresponding bitfield. > > IMO the correct thing to do is to remove the unions from that struct and > only leave behind the bitfields and not use the WMI_SCAN_XXX masks > except when filling the firmware structure. > > But don't spin an update to your patches until Kalle has a chance to > give his opinion. I'm new to maintaining these drivers and Kalle may > have a different opinion on this. > > /jeff No problem, I'll wait for Kalle's input on this before doing anything. As soon as we decide which way is the right way, I'll work on this. I only care that this gets resolved.
On Thu Nov 30, 2023 at 9:24 AM CET, Nicolas Escande wrote: > On Tue Nov 28, 2023 at 1:57 AM CET, Jeff Johnson wrote: > > On 11/27/2023 2:54 PM, Nicolas Escande wrote: > [...] > > > So either we should not use WMI_SCAN_XXX with scan_req_params.scan_flags ever > > > and only use the bitfield to set scan parameters or if we use WMI_SCAN_XXX with > > > scan_req_params.scan_flags they need to match the corresponding bitfield. > > > > IMO the correct thing to do is to remove the unions from that struct and > > only leave behind the bitfields and not use the WMI_SCAN_XXX masks > > except when filling the firmware structure. > > > > But don't spin an update to your patches until Kalle has a chance to > > give his opinion. I'm new to maintaining these drivers and Kalle may > > have a different opinion on this. > > > > /jeff > > No problem, I'll wait for Kalle's input on this before doing anything. > As soon as we decide which way is the right way, I'll work on this. I only care > that this gets resolved. Hi Kalle/Jeff, Any new input on this so I can move forward on fixing this ? Otherwise I think I'll end up going on with Jeff's proposal of only using the bitfield for intra driver representation & then converting the bitfields to their corresponding WMI_SCAN_XXX when transmiting the req to the hw with wmi.
"Nicolas Escande" <nico.escande@gmail.com> writes: > On Thu Nov 30, 2023 at 9:24 AM CET, Nicolas Escande wrote: >> On Tue Nov 28, 2023 at 1:57 AM CET, Jeff Johnson wrote: >> > On 11/27/2023 2:54 PM, Nicolas Escande wrote: >> [...] >> > > So either we should not use WMI_SCAN_XXX with scan_req_params.scan_flags ever >> > > and only use the bitfield to set scan parameters or if we use WMI_SCAN_XXX with >> > > scan_req_params.scan_flags they need to match the corresponding bitfield. >> > >> > IMO the correct thing to do is to remove the unions from that struct and >> > only leave behind the bitfields and not use the WMI_SCAN_XXX masks >> > except when filling the firmware structure. >> > >> > But don't spin an update to your patches until Kalle has a chance to >> > give his opinion. I'm new to maintaining these drivers and Kalle may >> > have a different opinion on this. >> > >> > /jeff >> >> No problem, I'll wait for Kalle's input on this before doing anything. >> As soon as we decide which way is the right way, I'll work on this. I only care >> that this gets resolved. > > Hi Kalle/Jeff, > > Any new input on this so I can move forward on fixing this ? Sorry, too many patches... > Otherwise I think I'll end up going on with Jeff's proposal of only using the > bitfield for intra driver representation & then converting the bitfields to > their corresponding WMI_SCAN_XXX when transmiting the req to the hw with wmi. Yeah, I only took a quick glimpse but Jeff's proposal does make sense.
diff --git a/drivers/net/wireless/ath/ath11k/wmi.h b/drivers/net/wireless/ath/ath11k/wmi.h index 100bb816b592..0b4e6c2f7860 100644 --- a/drivers/net/wireless/ath/ath11k/wmi.h +++ b/drivers/net/wireless/ath/ath11k/wmi.h @@ -3348,17 +3348,17 @@ struct scan_req_params { scan_f_filter_prb_req:1, scan_f_bypass_dfs_chn:1, scan_f_continue_on_err:1, + scan_f_promisc_mode: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_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,
The is a layout mismatch between the bitfield representing scan_flags in struct scan_req_params & the bits as defined in the WMI_SCAN_XXX macros. Lets fix it by making the struct match the #defines. I tried to correct it by making the struct match the #define and it worked for WMI_SCAN_FLAG_FORCE_ACTIVE_ON_DFS / scan_f_force_active_dfs_chn so I'm assuming this is the right thing to do. Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.7.0.1-01744-QCAHKSWPL_SILICONZ-1 Signed-off-by: Nicolas Escande <nico.escande@gmail.com> --- drivers/net/wireless/ath/ath11k/wmi.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)