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

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 <[email protected]>
> >>>> ---
> >>>>  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.

>                         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