Sven Eckelmann <sven@xxxxxxxxxxxxx> writes:
>> On Thursday, 9 December 2021 17:05:14 CET Kalle Valo wrote:
>>> Isn't ath11k WMI commands and events supposed to be in CPU
>>> endian and the firmware automatically translates them if CPU is little
>>> or big endian?
> [...]
> On Friday, 17 December 2021 12:04:45 CET Carl Huang wrote:
>> Both cpu and firmware are supposed to be little endian in ath11k.
>
> I hope this statement is incorrect. But if it isn't:
>
> You cannot limit a non-architecture dependent driver to be only used by little
> endian CPUs. This would be grave bug in ath11k.
>
> If your firmware requires wmi messages and similar things in little endian
> then you have to mark types correctly as big/little endian. E.g. __le32
> instead of u32. And then you have to convert everything manually with
> cpu_to_le32 and so on. See the ath10k code for examples.
>
> Tools like sparse can assist you in your search for problematic places when
> your kernel has the __CHECK_ENDIAN__ related code activated. This is the
> default for kernels >= 4.10.
This is what I would have preferred to do in ath11k as well but a lot of
people preferred the firmware conversion method as the proprietary
driver uses the same, so I yielded. ath11k should work on big endian
cpus, but to my knowledge nobody has tested it so I do not know if it
really works or not. If someone can test please do let me know, I am
very curious to know if it really works.
ath11k enables the firmware swap feature like this:
/* Host software's Copy Engine configuration. */
#ifdef __BIG_ENDIAN
#define CE_ATTR_FLAGS CE_ATTR_BYTE_SWAP_DATA
#else
#define CE_ATTR_FLAGS 0
#endif
Also grep for BIG_ENDIAN, few functions have that.
> If Kalle' statement is true that the firmware takes care of endianness
> translation of WMI messages to host endianness, then your code would still be
> questionable:
>
>>> + /* supplicant expects big-endian replay counter */
>>> + replay_ctr = cpu_to_be64(le64_to_cpup((__le64
>>> *)ev->replay_counter));
>
> Why isn't the firmware taking care of the conversion at that place?
>
> Why are you defining it as `u8 replay_counter[GTK_REPLAY_COUNTER_BYTES];` in
> the struct instead of using `__le64 replay_counter;`?
>
> What ensures that this is value is 64 bit aligned in memory? Wouldn't it be
> more correct to (see above) use
>
> replay_ctr = cpu_to_be64(get_unaligned_le64(ev->replay_counter));
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;
}
}
}
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
