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: Tue, 21 Dec 2021 16:48:53 +0200	[thread overview]
Message-ID: <87r1a66mju.fsf@kernel.org> (raw)
In-Reply-To: <1934542.dmet8VCoWv@sven-l14> (Sven Eckelmann's message of "Mon, 20 Dec 2021 12:50:55 +0100")

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

  reply	other threads:[~2021-12-21 14:49 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
2021-12-20 11:50           ` Sven Eckelmann
2021-12-21 14:48             ` Kalle Valo [this message]
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=87r1a66mju.fsf@kernel.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).