Received-SPF: pass (google.com: domain of linux-wireless+bounces-135-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <20c7a367-2243-4e13-b023-9999dc6c6790@quicinc.com>
Date: Mon, 27 Nov 2023 16:57:29 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] wifi: ath11k: fix layout of scan_flags in struct
 scan_req_params
Content-Language: en-US
To: Nicolas Escande <nico.escande@gmail.com>, Kalle Valo <kvalo@kernel.org>
CC: <linux-wireless@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <ath11k@lists.infradead.org>
References: <20231127180559.1696041-1-nico.escande@gmail.com>
 <bdcdbd06-e9bd-4a92-b27b-d94b2d8fb52d@quicinc.com>
 <CX9YPUDTAT1N.23DMRB5O9FEAO@gmail.com>
From: Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <CX9YPUDTAT1N.23DMRB5O9FEAO@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

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