Sender: cjhuang=codeaurora.org@mg.codeaurora.org
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Fri, 17 Dec 2021 19:04:45 +0800
From:   Carl Huang <cjhuang@codeaurora.org>
To:     Kalle Valo <kvalo@kernel.org>
Cc:     ath11k@lists.infradead.org, linux-wireless@vger.kernel.org,
        quic_cjhuang@quicinc.com
Subject: Re: [PATCH 6/6] ath11k: support GTK rekey offload
In-Reply-To: <87lf0tydad.fsf@codeaurora.org>
References: <20211011193750.4891-1-cjhuang@codeaurora.org>
 <20211011193750.4891-7-cjhuang@codeaurora.org>
 <87lf0tydad.fsf@codeaurora.org>
Message-ID: <4f28496abae7743ab2a9fa7150c5d64c@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Precedence: bulk

On 2021-12-10 00:05, Kalle Valo wrote:
> Carl Huang <cjhuang@codeaurora.org> writes:
> 
>> Host sets GTK related info to firmware before WoW is enabled, and
>> gets rekey replay_count and then disables GTK rekey when WoW quits.
>> 
>> Tested-on: QCA6390 hw2.0 PCI 
>> WLAN.HST.1.0.1-01740-QCAHSTSWPLZ_V2_TO_X86-1
>> 
>> Signed-off-by: Carl Huang <cjhuang@codeaurora.org>
> 
> [...]
> 
>> --- a/drivers/net/wireless/ath/ath11k/wmi.h
>> +++ b/drivers/net/wireless/ath/ath11k/wmi.h
>> @@ -5850,6 +5850,41 @@ struct wmi_set_arp_ns_offload_cmd {
>>  	 */
>>  } __packed;
>> 
>> +#define GTK_OFFLOAD_OPCODE_MASK             0xFF000000
>> +#define GTK_OFFLOAD_ENABLE_OPCODE           0x01000000
>> +#define GTK_OFFLOAD_DISABLE_OPCODE          0x02000000
>> +#define GTK_OFFLOAD_REQUEST_STATUS_OPCODE   0x04000000
>> +
>> +#define GTK_OFFLOAD_KEK_BYTES       16
>> +#define GTK_OFFLOAD_KCK_BYTES       16
>> +#define GTK_REPLAY_COUNTER_BYTES    8
>> +#define WMI_MAX_KEY_LEN             32
>> +#define IGTK_PN_SIZE                6
>> +
>> +struct wmi_gtk_offload_status_event {
>> +	u32 vdev_id;
>> +	u32 flags;
>> +	u32 refresh_cnt;
>> +	u8 replay_counter[GTK_REPLAY_COUNTER_BYTES];
>> +	u8 igtk_key_index;
>> +	u8 igtk_key_length;
>> +	u8 igtk_key_rsc[IGTK_PN_SIZE];
>> +	u8 igtk_key[WMI_MAX_KEY_LEN];
>> +	u8 gtk_key_index;
>> +	u8 gtk_key_length;
>> +	u8 gtk_key_rsc[GTK_REPLAY_COUNTER_BYTES];
>> +	u8 gtk_key[WMI_MAX_KEY_LEN];
>> +} __packed;
> 
> [...]
> 
>> +	arvif->rekey_data.replay_ctr = le64_to_cpup((__le64 
>> *)ev->replay_counter);
>> +	/* supplicant expects big-endian replay counter */
>> +	replay_ctr = cpu_to_be64(le64_to_cpup((__le64 
>> *)ev->replay_counter));
>> +
>> +	ieee80211_gtk_rekey_notify(arvif->vif, arvif->bssid,
>> +				   (void *)&replay_ctr, GFP_KERNEL);
> 
> Please avoid casting as much possible, and also otherwise this just
> looks weird. 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? So why do you cast ev->replay_counter to __le64 then?
> Wouldn't that break on big endian?
> 
Both cpu and firmware are supposed to be little endian in ath11k.
However, supplicant expects the replay_ctr in big endian format, that's
why an endian conversion is done here.

> --
> https://patchwork.kernel.org/project/linux-wireless/list/
> 
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches