linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Kalle Valo <kvalo@kernel.org>
To: Sven Eckelmann <sven@narfation.org>
Cc: ath11k@lists.infradead.org, linux-wireless@vger.kernel.org,
	quic_cjhuang@quicinc.com, Carl Huang <cjhuang@codeaurora.org>
Subject: Re: [PATCH 6/6] ath11k: support GTK rekey offload
Date: Mon, 20 Dec 2021 12:03:08 +0200	[thread overview]
Message-ID: <871r27bnkz.fsf@codeaurora.org> (raw)
In-Reply-To: <2102838.219ycuhFCz@sven-l14> (Sven Eckelmann's message of "Sat, 18 Dec 2021 09:37:02 +0100")

Sven Eckelmann <sven@narfation.org> 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

  parent reply	other threads:[~2021-12-20 10:03 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-11 19:37 [PATCH 0/6] ath11k: support WoW functionalities Carl Huang
2021-10-11 19:37 ` [PATCH 1/6] ath11k: Add basic " Carl Huang
2021-12-09 14:48   ` Kalle Valo
2021-10-11 19:37 ` [PATCH 2/6] ath11k: Add WoW net-detect functionality Carl Huang
2021-12-09 14:57   ` Kalle Valo
2021-12-09 15:03   ` Kalle Valo
2021-10-11 19:37 ` [PATCH 3/6] ath11k: implement hw data filter Carl Huang
2021-12-09 15:07   ` Kalle Valo
2021-12-09 15:47   ` Kalle Valo
2021-10-11 19:37 ` [PATCH 4/6] ath11k: purge rx pktlog when entering WoW Carl Huang
2021-12-09 15:12   ` Kalle Valo
2021-10-11 19:37 ` [PATCH 5/6] ath11k: support ARP and NS offload Carl Huang
2021-12-09 15:16   ` Kalle Valo
2021-12-09 16:07   ` Kalle Valo
2021-10-11 19:37 ` [PATCH 6/6] ath11k: support GTK rekey offload Carl Huang
2021-12-09 16:05   ` Kalle Valo
2021-12-17 11:04     ` Carl Huang
2021-12-18  8:37       ` Sven Eckelmann
2021-12-18  8:43         ` Sven Eckelmann
2021-12-18  9:16         ` Sven Eckelmann
2021-12-20 10:03         ` Kalle Valo [this message]
2021-12-20 11:50           ` Sven Eckelmann
2021-12-21 14:48             ` Kalle Valo
2021-12-21 20:39               ` Sven Eckelmann
2021-12-09 17:45 ` [PATCH 0/6] ath11k: support WoW functionalities Kalle Valo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=871r27bnkz.fsf@codeaurora.org \
    --to=kvalo@kernel.org \
    --cc=ath11k@lists.infradead.org \
    --cc=cjhuang@codeaurora.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=quic_cjhuang@quicinc.com \
    --cc=sven@narfation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).