Re: [PATCH RESEND] wifi: ath12k: fix channel list copy on big endian

[Baochen Qiang <baochen.qiang@oss.qualcomm.com>]

On 7/3/2026 1:49 PM, Alexander Wilhelm wrote:
> On Fri, Jul 03, 2026 at 10:48:11AM +0800, Baochen Qiang wrote:
>>
>>
>> On 7/2/2026 8:18 PM, Alexander Wilhelm wrote:
>>> On Thu, Jul 02, 2026 at 10:44:46AM +0200, Alexander Wilhelm wrote:
>>>> On Thu, Jul 02, 2026 at 04:27:44PM +0800, Baochen Qiang wrote:
>>>>>
>>>>>
>>>>> On 6/29/2026 3:46 PM, Alexander Wilhelm wrote:
>>>>>> The ath12k_wmi_scan_req_arg structure defines the channel list in
>>>>>> CPU-native order, while wmi_start_scan_cmd expects the values in
>>>>>> little-endian format. The simple memcpy causes the hardware scan to fail on
>>>>>> big-endian architectures. Set __le32* type for the tmp_ptr and swap channel
>>>>>> values to support both architectures correctly.
>>>>>>
>>>>>> Signed-off-by: Alexander Wilhelm <alexander.wilhelm@westermo.com>
>>>>>> ---
>>>>>>  drivers/net/wireless/ath/ath12k/wmi.c | 8 +++++---
>>>>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c
>>>>>> index 65a05a9520ff..9e1d3c662852 100644
>>>>>> --- a/drivers/net/wireless/ath/ath12k/wmi.c
>>>>>> +++ b/drivers/net/wireless/ath/ath12k/wmi.c
>>>>>> @@ -2571,7 +2571,8 @@ int ath12k_wmi_send_scan_start_cmd(struct ath12k *ar,
>>>>>>  	struct wmi_tlv *tlv;
>>>>>>  	void *ptr;
>>>>>>  	int i, ret, len;
>>>>>> -	u32 *tmp_ptr, extraie_len_with_pad = 0;
>>>>>> +	__le32 *tmp_ptr;
>>>>>> +	u32 extraie_len_with_pad = 0;
>>>>>>  	struct ath12k_wmi_hint_short_ssid_arg *s_ssid = NULL;
>>>>>>  	struct ath12k_wmi_hint_bssid_arg *hint_bssid = NULL;
>>>>>>  
>>>>>> @@ -2656,9 +2657,10 @@ int ath12k_wmi_send_scan_start_cmd(struct ath12k *ar,
>>>>>>  	tlv = ptr;
>>>>>>  	tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_UINT32, len);
>>>>>>  	ptr += TLV_HDR_SIZE;
>>>>>> -	tmp_ptr = (u32 *)ptr;
>>>>>> +	tmp_ptr = (__le32 *)ptr;
>>>>>>  
>>>>>> -	memcpy(tmp_ptr, arg->chan_list, arg->num_chan * 4);
>>>>>> +	for (i = 0; i < arg->num_chan; i++)
>>>>>> +		tmp_ptr[i] = cpu_to_le32(arg->chan_list[i]);
>>>>>>  
>>>>>>  	ptr += len;
>>>>>>  
>>>>>>
>>>>>
>>>>> seems hint_s_ssid and hint_bssid at the last also need the endian conversion?
>>>
>>> `hist_s_ssid` and `hint_bssid` are both structs within
>>> `ath12k_wmi_scan_req_arg`, and the remaining member variables are also stored in
>>> CPU order. Therefore, it seems wrong to me to perform byte swapping at this
>>> point. What I actually need to swap is the data written through `ptr`, not the
>>> structure members themselves. For that reason, I could also use the `__le32
>>> *tmp_ptr` approach. What do you think about that?
>>
>> Maybe I was not clear. I intended to mean the ptr:
>>
>> @@ -2728,8 +2728,8 @@ int ath12k_wmi_send_scan_start_cmd(struct ath12k *ar,
>>                 ptr += TLV_HDR_SIZE;
>>                 s_ssid = ptr;
>>                 for (i = 0; i < arg->num_hint_s_ssid; ++i) {
>> -                       s_ssid->freq_flags = arg->hint_s_ssid[i].freq_flags;
>> -                       s_ssid->short_ssid = arg->hint_s_ssid[i].short_ssid;
>> +                       s_ssid->freq_flags = cpu_to_le32(arg->hint_s_ssid[i].freq_flags);
>> +                       s_ssid->short_ssid = cpu_to_le32(arg->hint_s_ssid[i].short_ssid);
> 
> No, I understood what you meant. The issue is that the upper two lines will
> trigger sparse warnings. The member variables `freq_flags` and `short_ssid` are
> used on both the left-hand side and the right-hand side as members of the same
> `struct ath12k_wmi_hint_short_ssid_arg`. I would keep these fields as `u32` and
> use `tmp_ptr` on the left-hand side instead of `s_ssid` to keep things
> consistent. Alternatively, I could introduce a separate structure specifically
> for this use case.

Ah, I get your point. You are right, the issue is both the WMI cmd member and the arg use
the same definition.

Please introduce new structures for this. Note ath12k has a guidance on WMI interface
naming: for those interfacing with firmware, use the _params as the suffix:

e.g.:
	struct ath12k_wmi_hint_short_ssid_params
	struct ath12k_wmi_hint_bssid_params

for those host-used only use the _arg suffix, like the existing ones:

	struct ath12k_wmi_hint_short_ssid_arg;
	struct ath12k_wmi_hint_bssid_arg;

> 
>>                         s_ssid++;
>>                 }
>>                 ptr += len;
>> @@ -2743,7 +2743,7 @@ int ath12k_wmi_send_scan_start_cmd(struct ath12k *ar,
>>                 hint_bssid = ptr;
>>                 for (i = 0; i < arg->num_hint_bssid; ++i) {
>>                         hint_bssid->freq_flags =
>> -                               arg->hint_bssid[i].freq_flags;
>> +                               cpu_to_le32(arg->hint_bssid[i].freq_flags);
>>                         ether_addr_copy(&arg->hint_bssid[i].bssid.addr[0],
>>                                         &hint_bssid->bssid.addr[0]);
>>
>>>
>>> One thing I am still unsure about is the use of `ether_addr_copy()`. Can I
>>> simply copy the bytes as-is here, or does the address also need to be
>>> byte-swapped? I could not find any place where this address is being populated,
>>> so I am not sure what byte order it is expected to be in.
>>
>> I think you are talking about ath12k_wmi_hint_bssid_arg::bssid, right? For now ath12k does
>> not populate hint_bssid or hint_s_ssid members, so bssid always remain empty. But if we
>> are going to populate it I think the address originates from userspace and there it is
>> passed in byte steams, so byte-swapping not required I think.
> 
> Okay, sounds good. I'll keep the `memcpy()` at this location and prepare the
> next patch version.
> 
> 
> Best regards
> Alexander Wilhelm