Sven Eckelmann <sven@xxxxxxxxxxxxx> writes:
> On Monday, 20 December 2021 11:03:08 CET Kalle Valo wrote:
> [...]
>
> Thanks for all the explanation and pointers. I will try to use this to more
> clearly formulate my concern.
Good idea, this is too complex.
> If I understood it correctly then ev->replay_counter is:
>
> * __le64 on little endian systems
> * __be64 on big endian systems
>
> Or in short: it is just an u64.
My understanding is that on little endian host it's (the number representing
the byte index):
1 2 3 4 5 6 7 8
And on big endian host it's (as the firmware automatically swapped the
values):
4 3 2 1 8 7 6 5
So for on big endian we need to use ath11k_ce_byte_swap() to get them
back to correct order. (Or to be exact we need to use
ath11k_ce_byte_swap() every time as it does nothing on a little endian
host.)
Completely untested, of course. I don't have a big endian system.
>> Yeah, if the host does the conversion we would use __le64. But at the
>> moment the firmware does the conversion so I think we should use
>> ath11k_ce_byte_swap():
>>
>> /* For Big Endian Host, Copy Engine byte_swap is enabled
>> * When Copy Engine does byte_swap, need to byte swap again for the
>> * Host to get/put buffer content in the correct byte order
>> */
>> void ath11k_ce_byte_swap(void *mem, u32 len)
>> {
>> int i;
>>
>> if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) {
>> if (!mem)
>> return;
>>
>> for (i = 0; i < (len / 4); i++) {
>> *(u32 *)mem = swab32(*(u32 *)mem);
>> mem += 4;
>> }
>> }
>> }
>
> This function doesn't work for 64 bit values (if they are actually in big
> endian). It just rearranges (len / 4) u32s to host byte order - so the upper
> and lower 32 bit values for an u64 would still be swapped.
>
> Unless I misunderstood what CE_ATTR_BYTE_SWAP_DATA is supposed to do. Maybe it
> is not causing returned data to be in big/little endian but causes for one of
> the host endianess' that the data for 64-bit values in mixed
> endianness.
So my understanding is that when CE_ATTR_BYTE_SWAP_DATA is enabled the
firmware automatically swaps the packets per every four bytes. That's
why all the fields in WMI commands and events are u32.
> And if the function would operate on a struct with 16 bit or 8 bit values then
> we have something which we call here Kuddelmuddel [1].
Heh, need to remember that word :)
> But if the value is an u64, then the code in the patch is wrong:
The firmware interface should not have u16 or u8 fields. And for
anything larger ath11k_ce_byte_swap() should be used. Again, this is
just my recollection from discussions years back and I have not tested
this myself.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
