Skip to contentSkip to conversation

ath11k: support WoW functionalities

PATCH In review Carl Huang 3 participants 25 messages Oct 11, 2021 – Dec 21, 2021 Qualcomm Atheros ATH11K wireless driver

lore
Share & tools
lore

Carl Huang

author
#0 lore raw

cover ath11k: support WoW functionalities

Ath11k WoW basic funtionalities are merged from ath10k such as magic-pattern, PNO, disconnect and patterns.

Hw data filter and pktlog purge are required for ath11k.

ARP and NS offload, GTK rekey offload are new WoW features implemented on ath11k.

Carl Huang (6):
ath11k: Add basic WoW functionalities
ath11k: Add WoW net-detect functionality
ath11k: implement hw data filter
ath11k: purge rx pktlog when entering WoW
ath11k: support ARP and NS offload
ath11k: support GTK rekey offload

 drivers/net/wireless/ath/ath11k/core.c  |  29 +-
 drivers/net/wireless/ath/ath11k/core.h  |  31 ++
 drivers/net/wireless/ath/ath11k/dp_rx.c |   4 +-
 drivers/net/wireless/ath/ath11k/dp_rx.h |   2 +-
 drivers/net/wireless/ath/ath11k/htc.c   |   6 +
 drivers/net/wireless/ath/ath11k/htc.h   |   1 +
 drivers/net/wireless/ath/ath11k/mac.c   | 217 +++++++++-
 drivers/net/wireless/ath/ath11k/mac.h   |   1 +
 drivers/net/wireless/ath/ath11k/wmi.c   | 593 +++++++++++++++++++++++++
 drivers/net/wireless/ath/ath11k/wmi.h   | 343 ++++++++++++++-
 drivers/net/wireless/ath/ath11k/wow.c   | 747 ++++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath11k/wow.h   |  36 ++
 12 files changed, 1984 insertions(+), 26 deletions(-)
Signature · 1 line
2.7.4
author
#1 lore raw

PATCH 1/6 ath11k: Add basic WoW functionalities

Implement basic WoW functionalities such as magic-packet, disconnect and pattern. The logic is very similar to ath10k.

When WoW is configured, ath11k_core_suspend and ath11k_core_resume are skipped as WoW configuration and hif suspend/resume are done in ath11k_wow_op_suspend() and ath11k_wow_op_resume().

Tested-on: QCA6390 hw2.0 PCI WLAN.HST.1.0.1-01740-QCAHSTSWPLZ_V2_TO_X86-1

---
 drivers/net/wireless/ath/ath11k/core.c |  22 ++
 drivers/net/wireless/ath/ath11k/core.h |   4 +
 drivers/net/wireless/ath/ath11k/htc.c  |   6 +
 drivers/net/wireless/ath/ath11k/htc.h  |   1 +
 drivers/net/wireless/ath/ath11k/mac.c  |  59 +++--
 drivers/net/wireless/ath/ath11k/mac.h  |   1 +
 drivers/net/wireless/ath/ath11k/wmi.c  | 156 +++++++++++++
 drivers/net/wireless/ath/ath11k/wmi.h  |  76 +++++-
 drivers/net/wireless/ath/ath11k/wow.c  | 409 +++++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath11k/wow.h  |  36 +++
 10 files changed, 753 insertions(+), 17 deletions(-)
drivers/net/wireless/ath/ath11k/core.c +22 -0
index 7e4b5b3..e67136b 100644
@@ -244,10 +244,18 @@ static const struct ath11k_hw_params ath11k_hw_params[] = {
 int ath11k_core_suspend(struct ath11k_base *ab)
 {
 	int ret;
+	struct ath11k *ar;
 
 	if (!ab->hw_params.supports_suspend)
 		return -EOPNOTSUPP;
 
+	/* so far single_pdev_only chips have supports_suspend as true
+	 * and only the first pdev is valid.
+	 */
+	ar = ab->pdevs[0].ar;
+	if (!ar || ar->state != ATH11K_STATE_OFF)
+		return 0;
+
 	/* TODO: there can frames in queues so for now add delay as a hack.
 	 * Need to implement to handle and remove this delay.
 	 */
@@ -260,6 +268,12 @@ int ath11k_core_suspend(struct ath11k_base *ab)
 		return ret;
 	}
 
+	ret = ath11k_mac_wait_tx_complete(ar);
+	if (ret) {
+		ath11k_warn(ab, "failed to wait tx complete: %d\n", ret);
+		return ret;
+	}
+
 	ret = ath11k_wow_enable(ab);
 	if (ret) {
 		ath11k_warn(ab, "failed to enable wow during suspend: %d\n", ret);
@@ -292,10 +306,18 @@ EXPORT_SYMBOL(ath11k_core_suspend);
 int ath11k_core_resume(struct ath11k_base *ab)
 {
 	int ret;
+	struct ath11k *ar;
 
 	if (!ab->hw_params.supports_suspend)
 		return -EOPNOTSUPP;
 
+	/* so far signle_pdev_only chips have supports_suspend as true
+	 * and only the first pdev is valid.
+	 */
+	ar = ab->pdevs[0].ar;
+	if (!ar || ar->state != ATH11K_STATE_OFF)
+		return 0;
+
 	ret = ath11k_hif_resume(ab);
 	if (ret) {
 		ath11k_warn(ab, "failed to resume hif during resume: %d\n", ret);
drivers/net/wireless/ath/ath11k/core.h +4 -0
index 61e9684..565cffc 100644
@@ -23,6 +23,7 @@
 #include "thermal.h"
 #include "dbring.h"
 #include "spectral.h"
+#include "wow.h"
 
 #define SM(_v, _f) (((_v) << _f##_LSB) & _f##_MASK)
 
@@ -609,6 +610,9 @@ struct ath11k {
 	struct work_struct wmi_mgmt_tx_work;
 	struct sk_buff_head wmi_mgmt_tx_queue;
 
+	struct ath11k_wow wow;
+	struct completion target_suspend;
+	bool target_suspend_ack;
 	struct ath11k_per_peer_tx_stats peer_tx_stats;
 	struct list_head ppdu_stats_info;
 	u32 ppdu_stat_list_depth;
drivers/net/wireless/ath/ath11k/htc.c +6 -0
index 54b1d34..7711443 100644
@@ -245,6 +245,11 @@ static void ath11k_htc_suspend_complete(struct ath11k_base *ab, bool ack)
 	complete(&ab->htc_suspend);
 }
 
+static void ath11k_wakeup_from_suspend(struct ath11k_base *ab)
+{
+	ath11k_dbg(ab, ATH11K_DBG_BOOT, "wakeup from suspend is received\n");
+}
+
 void ath11k_htc_rx_completion_handler(struct ath11k_base *ab,
 				      struct sk_buff *skb)
 {
@@ -349,6 +354,7 @@ void ath11k_htc_rx_completion_handler(struct ath11k_base *ab,
 			ath11k_htc_suspend_complete(ab, false);
 			break;
 		case ATH11K_HTC_MSG_WAKEUP_FROM_SUSPEND_ID:
+			ath11k_wakeup_from_suspend(ab);
 			break;
 		default:
 			ath11k_warn(ab, "ignoring unsolicited htc ep0 event %ld\n",
drivers/net/wireless/ath/ath11k/htc.h +1 -0
index 6c8a469..eb22c29 100644
@@ -13,6 +13,7 @@
 #include <linux/timer.h>
 
 struct ath11k_base;
+struct ath11k;
 
 #define HTC_HDR_ENDPOINTID                       GENMASK(7, 0)
 #define HTC_HDR_FLAGS                            GENMASK(15, 8)
drivers/net/wireless/ath/ath11k/mac.c +45 -14

94 lines not on this page · raw message

drivers/net/wireless/ath/ath11k/mac.h +1 -0
index fd04807..d04d4aa 100644
@@ -174,4 +174,5 @@ void ath11k_mac_handle_beacon_miss(struct ath11k *ar, u32 vdev_id);
 void ath11k_mac_fill_reg_tpc_info(struct ath11k *ar,
 				  struct ieee80211_vif *vif,
 				  struct ieee80211_chanctx_conf *ctx);
+int ath11k_mac_wait_tx_complete(struct ath11k *ar);
 #endif
drivers/net/wireless/ath/ath11k/wmi.c +156 -0

159 lines not on this page · raw message

drivers/net/wireless/ath/ath11k/wmi.h +73 -3

95 lines not on this page · raw message

drivers/net/wireless/ath/ath11k/wow.c +409 -0

424 lines not on this page · raw message

drivers/net/wireless/ath/ath11k/wow.h +36 -0
index dabc4ee..6e27297 100644
@@ -3,8 +3,44 @@
  * Copyright (c) 2020 The Linux Foundation. All rights reserved.
  */
 
+#ifndef _WOW_H_
+#define _WOW_H_
+
+struct ath11k_wow {
+	u32 max_num_patterns;
+	struct completion wakeup_completed;
+	struct wiphy_wowlan_support wowlan_support;
+};
+
+struct rfc1042_hdr {
+	u8 llc_dsap;
+	u8 llc_ssap;
+	u8 llc_ctrl;
+	u8 snap_oui[3];
+	__be16 snap_type;
+} __packed;
+
 #define ATH11K_WOW_RETRY_NUM		3
 #define ATH11K_WOW_RETRY_WAIT_MS	200
+#define ATH11K_WOW_PATTERNS		22
 
 int ath11k_wow_enable(struct ath11k_base *ab);
 int ath11k_wow_wakeup(struct ath11k_base *ab);
+
+#ifdef CONFIG_PM
+
+int ath11k_wow_init(struct ath11k *ar);
+int ath11k_wow_op_suspend(struct ieee80211_hw *hw,
+			  struct cfg80211_wowlan *wowlan);
+int ath11k_wow_op_resume(struct ieee80211_hw *hw);
+void ath11k_wow_op_set_wakeup(struct ieee80211_hw *hw, bool enabled);
+
+#else
+
+static inline int ath11k_wow_init(struct ath11k *ar)
+{
+	return 0;
+}
+
+#endif /* CONFIG_PM */
+#endif /* _WOW_H_ */
Signature · 1 line
2.7.4
#2 lore raw

Re: [PATCH 1/6] ath11k: Add basic WoW functionalities

Quoted 11 lines · Carl Huang

Implement basic WoW functionalities such as magic-packet, disconnect and pattern. The logic is very similar to ath10k.

When WoW is configured, ath11k_core_suspend and ath11k_core_resume are skipped as WoW configuration and hif suspend/resume are done in ath11k_wow_op_suspend() and ath11k_wow_op_resume().

Tested-on: QCA6390 hw2.0 PCI WLAN.HST.1.0.1-01740-QCAHSTSWPLZ_V2_TO_X86-1

[...]

drivers/net/wireless/ath/ath11k/core.c static const struct ath11k_hw_params ath11k_hw_params[] = { · line 255

Quoted 12 lines

int ath11k_core_suspend(struct ath11k_base *ab)
{

drivers/net/wireless/ath/ath11k/core.c +5 -0
 	int ret;
+	struct ath11k *ar;
 
 	if (!ab->hw_params.supports_suspend)
 		return -EOPNOTSUPP;
 
+	/* so far single_pdev_only chips have supports_suspend as true
+	 * and only the first pdev is valid.
+	 */
+	ar = ab->pdevs[0].ar;

I'm a bit worried about this assumption, it might change in the future. What about a helper like this to add a test for it:

static inline struct ath11k_pdev *ath11k_core_get_single_pdev()

{
        WARN_ON(!ab->hw_params.single_pdev_only);

        return ab->pdevs[0];
}
Quoted 4 lines

int ath11k_core_resume(struct ath11k_base *ab)
{

drivers/net/wireless/ath/ath11k/core.c +1 -0
 	int ret;
+	struct ath11k *ar;

drivers/net/wireless/ath/ath11k/core.c EXPORT_SYMBOL(ath11k_core_suspend); · line 319

Quoted 10 lines
drivers/net/wireless/ath/ath11k/core.c +6 -0
 
 	if (!ab->hw_params.supports_suspend)
 		return -EOPNOTSUPP;
 
+	/* so far signle_pdev_only chips have supports_suspend as true
+	 * and only the first pdev is valid.
+	 */
+	ar = ab->pdevs[0].ar;
+	if (!ar || ar->state != ATH11K_STATE_OFF)
+		return 0;

The same helper could be used here.

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
author
#3 lore raw

PATCH 2/6 ath11k: Add WoW net-detect functionality

In reply to Carl Huang

Implement net-detect feature by setting flag WIPHY_WOWLAN_NET_DETECT if firmware supports this feature. Driver sets the related PNO configuration to firmware before entering WoW and firmware then scans periodically and wakes up host if a specific SSID is found.

Tested-on: QCA6390 hw2.0 PCI WLAN.HST.1.0.1-01740-QCAHSTSWPLZ_V2_TO_X86-1

---
 drivers/net/wireless/ath/ath11k/core.h |   1 +
 drivers/net/wireless/ath/ath11k/mac.c  |  12 +++
 drivers/net/wireless/ath/ath11k/wmi.c  | 150 ++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath11k/wmi.h  | 169 ++++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath11k/wow.c  | 172 ++++++++++++++++++++++++++++++++-
 5 files changed, 503 insertions(+), 1 deletion(-)
drivers/net/wireless/ath/ath11k/core.h +1 -0
index 565cffc..6e40a33 100644
@@ -636,6 +636,7 @@ struct ath11k {
 	struct completion finish_11d_ch_list;
 	bool pending_11d;
 	bool regdom_set_by_user;
+	bool nlo_enabled;
 };
 
 struct ath11k_band_cap {
drivers/net/wireless/ath/ath11k/mac.c +12 -0
index b32ef40..ce235ca 100644
@@ -7742,6 +7742,18 @@ static int __ath11k_mac_register(struct ath11k *ar)
 
 	ar->hw->wiphy->max_ap_assoc_sta = ar->max_num_stations;
 
+	if (test_bit(WMI_TLV_SERVICE_NLO, ar->wmi->wmi_ab->svc_map)) {
+		ar->hw->wiphy->max_sched_scan_ssids = WMI_PNO_MAX_SUPP_NETWORKS;
+		ar->hw->wiphy->max_match_sets = WMI_PNO_MAX_SUPP_NETWORKS;
+		ar->hw->wiphy->max_sched_scan_ie_len = WMI_PNO_MAX_IE_LENGTH;
+		ar->hw->wiphy->max_sched_scan_plans = WMI_PNO_MAX_SCHED_SCAN_PLANS;
+		ar->hw->wiphy->max_sched_scan_plan_interval =
+			WMI_PNO_MAX_SCHED_SCAN_PLAN_INT;
+		ar->hw->wiphy->max_sched_scan_plan_iterations =
+			WMI_PNO_MAX_SCHED_SCAN_PLAN_ITRNS;
+		ar->hw->wiphy->features |= NL80211_FEATURE_ND_RANDOM_MAC_ADDR;
+	}
+
 	ret = ath11k_wow_init(ar);
 	if (ret) {
 		ath11k_warn(ar->ab, "failed to init wow: %d\n", ret);
drivers/net/wireless/ath/ath11k/wmi.c +150 -0

153 lines not on this page · raw message

drivers/net/wireless/ath/ath11k/wmi.h +169 -0

181 lines not on this page · raw message

drivers/net/wireless/ath/ath11k/wow.c +171 -1

203 lines not on this page · raw message

Signature · 1 line
2.7.4
#4 lore raw

Re: [PATCH 2/6] ath11k: Add WoW net-detect functionality

Quoted 10 lines · Carl Huang

Implement net-detect feature by setting flag WIPHY_WOWLAN_NET_DETECT if firmware supports this feature. Driver sets the related PNO configuration to firmware before entering WoW and firmware then scans periodically and wakes up host if a specific SSID is found.

Tested-on: QCA6390 hw2.0 PCI WLAN.HST.1.0.1-01740-QCAHSTSWPLZ_V2_TO_X86-1

[...]

Quoted 5 lines
drivers/net/wireless/ath/ath11k/wmi.h +0 -0
@@ -5626,6 +5626,173 @@ struct wmi_wow_del_pattern_cmd {
 	u32 pattern_type;
 } __packed;

drivers/net/wireless/ath/ath11k/wmi.h struct wmi_wow_del_pattern_cmd { · line 5637

Quoted 10 lines
drivers/net/wireless/ath/ath11k/wmi.h +9 -0
 
+#define WMI_PNO_MAX_SCHED_SCAN_PLANS      2
+#define WMI_PNO_MAX_SCHED_SCAN_PLAN_INT   7200
+#define WMI_PNO_MAX_SCHED_SCAN_PLAN_ITRNS 100
+#define WMI_PNO_MAX_NETW_CHANNELS         26
+#define WMI_PNO_MAX_NETW_CHANNELS_EX      60
+#define WMI_PNO_MAX_SUPP_NETWORKS         WLAN_SCAN_PARAMS_MAX_SSID
+#define WMI_PNO_MAX_IE_LENGTH             WLAN_SCAN_PARAMS_MAX_IE_LEN
+
+/*size based of dot11 declaration without extra IEs as we will not carry those for PNO*/

The comment style is:

/* foo bar */

That is add space after and before asterisk.

drivers/net/wireless/ath/ath11k/wmi.h struct wmi_wow_del_pattern_cmd { · line 5646

Quoted 9 lines
drivers/net/wireless/ath/ath11k/wmi.h +9 -0
+
+		memcpy(pno->a_networks[i].ssid.ssid,
+		       nd_config->match_sets[i].ssid.ssid,
+		       nd_config->match_sets[i].ssid.ssid_len);
+		pno->a_networks[i].authentication = 0;
+		pno->a_networks[i].encryption     = 0;
+		pno->a_networks[i].bcast_nw_type  = 0;
+
+		/*Copying list of valid channel into request */

Same here.

Quoted 6 lines
drivers/net/wireless/ath/ath11k/wmi.h +1 -1
@@ -433,8 +592,14 @@ int ath11k_wow_op_resume(struct ieee80211_hw *hw)
 	ath11k_hif_irq_enable(ar->ab);
 
 	ret = ath11k_wow_wakeup(ar->ab);
-	if (ret)
+	if (ret) {

drivers/net/wireless/ath/ath11k/wmi.h int ath11k_wow_op_resume(struct ieee80211_hw *hw) · line 605

Quoted 10 lines
drivers/net/wireless/ath/ath11k/wmi.h +6 -0
 		ath11k_warn(ar->ab, "failed to wakeup from wow: %d\n", ret);
+		goto exit;
+	}
+
+	ret = ath11k_wow_nlo_cleanup(ar);
+	if (ret)
+		ath11k_warn(ar->ab, "failed to cleanup nlo: %d\n", ret);
 
 exit:
 	if (ret) {

For consistency please add goto exit also for ath11k_wow_nlo_cleanup().

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
#5 lore raw

Re: [PATCH 2/6] ath11k: Add WoW net-detect functionality

In reply to Carl Huang
Quoted 10 lines · Carl Huang

Implement net-detect feature by setting flag WIPHY_WOWLAN_NET_DETECT if firmware supports this feature. Driver sets the related PNO configuration to firmware before entering WoW and firmware then scans periodically and wakes up host if a specific SSID is found.

Tested-on: QCA6390 hw2.0 PCI WLAN.HST.1.0.1-01740-QCAHSTSWPLZ_V2_TO_X86-1

[...]

Quoted 14 lines
drivers/net/wireless/ath/ath11k/wow.c +14 -0
+static int ath11k_wmi_pno_check_and_convert(struct ath11k *ar, u32 vdev_id,
+					    struct cfg80211_sched_scan_request *nd_config,
+					    struct wmi_pno_scan_req *pno)
+{
+	int i, j;
+	u8 ssid_len;
+
+	pno->enable = 1;
+	pno->vdev_id = vdev_id;
+	pno->uc_networks_count = nd_config->n_match_sets;
+
+	if (!pno->uc_networks_count ||
+	    pno->uc_networks_count > WMI_PNO_MAX_SUPP_NETWORKS)
+		return -EINVAL;

drivers/net/wireless/ath/ath11k/wow.c static void ath11k_wow_convert_8023_to_80211(struct cfg80211_pkt_pattern *new, · line 254

Quoted 10 lines
drivers/net/wireless/ath/ath11k/wow.c +10 -0
+
+	if (nd_config->n_channels > WMI_PNO_MAX_NETW_CHANNELS_EX)
+		return -EINVAL;
+
+	/* Filling per profile params */
+	for (i = 0; i < pno->uc_networks_count; i++) {
+		ssid_len = nd_config->match_sets[i].ssid.ssid_len;
+
+		if (ssid_len == 0 || ssid_len > 32)
+			return -EINVAL;

A define for 32 would be nice, WMI_MAC_MAX_SSID_LENGTH has a bad name but I couldn't find anything better from wmi.h. sizeof(struct wmi_ssid.ssid) might be other alternative.

But length from cfg80211 should be trustworthy anyway, so I'm not sure if the check is even needed?

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
author
#6 lore raw

PATCH 3/6 ath11k: implement hw data filter

In reply to Carl Huang

Host needs to set hw data filter before entering WoW to let firmware drop needless bc/mc frames to avoid frequent wakeup. Host clears hw data filter when leaving WoW.

Tested-on: QCA6390 hw2.0 PCI WLAN.HST.1.0.1-01740-QCAHSTSWPLZ_V2_TO_X86-1

---
 drivers/net/wireless/ath/ath11k/wmi.c | 32 ++++++++++++++++++++
 drivers/net/wireless/ath/ath11k/wmi.h | 15 +++++++++
 drivers/net/wireless/ath/ath11k/wow.c | 57 ++++++++++++++++++++++++++++++++++-
 3 files changed, 103 insertions(+), 1 deletion(-)
drivers/net/wireless/ath/ath11k/wmi.c +32 -0
index 9f466fe..be880dc 100644
@@ -8348,6 +8348,38 @@ void ath11k_wmi_detach(struct ath11k_base *ab)
 	ath11k_wmi_free_dbring_caps(ab);
 }
 
+int ath11k_wmi_send_hw_data_filter_cmd(struct ath11k *ar, u32 vdev_id,
+				       u32 filter_bitmap, bool enable)
+{
+	struct wmi_hw_data_filter_cmd *cmd;
+	struct sk_buff *skb;
+	int len;
+
+	len = sizeof(*cmd);
+	skb = ath11k_wmi_alloc_skb(ar->wmi->wmi_ab, len);
+
+	if (!skb)
+		return -ENOMEM;
+
+	cmd = (struct wmi_hw_data_filter_cmd *)skb->data;
+	cmd->tlv_header = FIELD_PREP(WMI_TLV_TAG, WMI_TAG_HW_DATA_FILTER_CMD) |
+			  FIELD_PREP(WMI_TLV_LEN, sizeof(*cmd) - TLV_HDR_SIZE);
+
+	cmd->vdev_id = vdev_id;
+	cmd->enable = enable;
+	/* Set all modes in case of disable */
+	if (cmd->enable)
+		cmd->hw_filter_bitmap = filter_bitmap;
+	else
+		cmd->hw_filter_bitmap = ((u32)~0U);
+
+	ath11k_dbg(ar->ab, ATH11K_DBG_WMI,
+		   "wmi set hw filter enable %d, filter_bitmap:0x%x\n",
+		   enable, filter_bitmap);
+
+	return ath11k_wmi_cmd_send(ar->wmi, skb, WMI_HW_DATA_FILTER_CMDID);
+}
+
 int ath11k_wmi_wow_host_wakeup_ind(struct ath11k *ar)
 {
 	struct wmi_wow_host_wakeup_ind *cmd;
drivers/net/wireless/ath/ath11k/wmi.h +15 -0
index 6d3dfb9..1301b17 100644
@@ -5400,6 +5400,19 @@ struct ath11k_wmi_base {
 	struct ath11k_targ_cap *targ_cap;
 };
 
+/* Definition of HW data filtering */
+enum hw_data_filter_type {
+	WMI_HW_DATA_FILTER_DROP_NON_ARP_BC = BIT(0),
+	WMI_HW_DATA_FILTER_DROP_NON_ICMPV6_MC = BIT(1),
+};
+
+struct wmi_hw_data_filter_cmd {
+	u32 tlv_header;
+	u32 vdev_id;
+	u32 enable;
+	u32 hw_filter_bitmap;
+} __packed;
+
 /* WOW structures */
 enum wmi_wow_wakeup_event {
 	WOW_BMISS_EVENT = 0,
@@ -5956,4 +5969,6 @@ int ath11k_wmi_wow_add_pattern(struct ath11k *ar, u32 vdev_id, u32 pattern_id,
 int ath11k_wmi_wow_add_wakeup_event(struct ath11k *ar, u32 vdev_id,
 				    enum wmi_wow_wakeup_event event,
 				    u32 enable);
+int ath11k_wmi_send_hw_data_filter_cmd(struct ath11k *ar, u32 vdev_id,
+				       u32 filter_bitmap, bool enable);
 #endif
drivers/net/wireless/ath/ath11k/wow.c +56 -1

76 lines not on this page · raw message

Signature · 1 line
2.7.4
#7 lore raw

Re: [PATCH 3/6] ath11k: implement hw data filter

Quoted 3 lines · Carl Huang

Host needs to set hw data filter before entering WoW to let firmware drop needless bc/mc frames to avoid frequent wakeup. Host clears hw data filter when leaving WoW.

Please try to use full words to keep the commit log readable:

ath11k: implement hardware data filter

Host needs to set hardware data filter before entering WoW to let firmware drop needless broadcast/multicast frames to avoid frequent wakeup. Host clears the hardware data filter when leaving WoW.

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
#8 lore raw

Re: [PATCH 3/6] ath11k: implement hw data filter

In reply to Carl Huang
Quoted 7 lines · Carl Huang

Host needs to set hw data filter before entering WoW to let firmware drop needless bc/mc frames to avoid frequent wakeup. Host clears hw data filter when leaving WoW.

Tested-on: QCA6390 hw2.0 PCI WLAN.HST.1.0.1-01740-QCAHSTSWPLZ_V2_TO_X86-1

[...]

Quoted 6 lines
drivers/net/wireless/ath/ath11k/wow.c +1 -1
@@ -598,8 +647,14 @@ int ath11k_wow_op_resume(struct ieee80211_hw *hw)
 	}
 
 	ret = ath11k_wow_nlo_cleanup(ar);
-	if (ret)
+	if (ret) {

drivers/net/wireless/ath/ath11k/wow.c int ath11k_wow_op_resume(struct ieee80211_hw *hw) · line 660

Quoted 10 lines
drivers/net/wireless/ath/ath11k/wow.c +6 -0
 		ath11k_warn(ar->ab, "failed to cleanup nlo: %d\n", ret);
+		goto exit;
+	}
+
+	ret = ath11k_wow_clear_hw_filter(ar);
+	if (ret)
+		ath11k_warn(ar->ab, "failed to clear hw filter: %d\n", ret);
 
 exit:
 	if (ret) {

For consistency please add goto exit for ath11k_wow_clear_hw_filter().

Signature · 8 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

-- 
ath11k mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/ath11k
author
#9 lore raw

PATCH 4/6 ath11k: purge rx pktlog when entering WoW

In reply to Carl Huang

This change is to purge rx pktlog when entering WoW and reap the mon_status buffer to keep it empty. When leaving WoW, host restarts the reap timer. In WoW state, it's not allowed to feed into mon_status rings per firmware request.

Tested-on: QCA6390 hw2.0 PCI WLAN.HST.1.0.1-01740-QCAHSTSWPLZ_V2_TO_X86-1

---
 drivers/net/wireless/ath/ath11k/core.c  |  7 +------
 drivers/net/wireless/ath/ath11k/dp_rx.c |  4 +---
 drivers/net/wireless/ath/ath11k/dp_rx.h |  2 +-
 drivers/net/wireless/ath/ath11k/wow.c   | 19 +++++++++++++++++++
 4 files changed, 22 insertions(+), 10 deletions(-)
drivers/net/wireless/ath/ath11k/core.c +1 -6
index e67136b..9747659 100644
@@ -327,12 +327,7 @@ int ath11k_core_resume(struct ath11k_base *ab)
 	ath11k_hif_ce_irq_enable(ab);
 	ath11k_hif_irq_enable(ab);
 
-	ret = ath11k_dp_rx_pktlog_start(ab);
-	if (ret) {
-		ath11k_warn(ab, "failed to start rx pktlog during resume: %d\n",
-			    ret);
-		return ret;
-	}
+	ath11k_dp_rx_pktlog_start(ab);
 
 	ret = ath11k_wow_wakeup(ab);
 	if (ret) {
drivers/net/wireless/ath/ath11k/dp_rx.c +1 -3
index 6d769ba..ae6db5d 100644
@@ -5105,13 +5105,11 @@ int ath11k_dp_rx_pdev_mon_detach(struct ath11k *ar)
 	return 0;
 }
 
-int ath11k_dp_rx_pktlog_start(struct ath11k_base *ab)
+void ath11k_dp_rx_pktlog_start(struct ath11k_base *ab)
 {
 	/* start reap timer */
 	mod_timer(&ab->mon_reap_timer,
 		  jiffies + msecs_to_jiffies(ATH11K_MON_TIMER_INTERVAL));
-
-	return 0;
 }
 
 int ath11k_dp_rx_pktlog_stop(struct ath11k_base *ab, bool stop_timer)
drivers/net/wireless/ath/ath11k/dp_rx.h +1 -1
index 623da3b..2e63f2f 100644
@@ -92,7 +92,7 @@ int ath11k_dp_rx_pdev_mon_detach(struct ath11k *ar);
 int ath11k_dp_rx_pdev_mon_attach(struct ath11k *ar);
 int ath11k_peer_rx_frag_setup(struct ath11k *ar, const u8 *peer_mac, int vdev_id);
 
-int ath11k_dp_rx_pktlog_start(struct ath11k_base *ab);
+void ath11k_dp_rx_pktlog_start(struct ath11k_base *ab);
 int ath11k_dp_rx_pktlog_stop(struct ath11k_base *ab, bool stop_timer);
 
 #endif /* ATH11K_DP_RX_H */
drivers/net/wireless/ath/ath11k/wow.c +19 -0
index 35f1c5c..8bd2ca2 100644
@@ -13,6 +13,7 @@
 #include "debug.h"
 #include "wmi.h"
 #include "wow.h"
+#include "dp_rx.h"
 
 static const struct wiphy_wowlan_support ath11k_wowlan_support = {
 	.flags = WIPHY_WOWLAN_DISCONNECT |
@@ -557,6 +558,14 @@ int ath11k_wow_op_suspend(struct ieee80211_hw *hw,
 
 	mutex_lock(&ar->conf_mutex);
 
+	ret = ath11k_dp_rx_pktlog_stop(ar->ab, true);
+	if (ret) {
+		ath11k_warn(ar->ab,
+			    "failed to stop dp rx (and timer) pktlog during wow suspend: %d\n",
+			    ret);
+		goto exit;
+	}
+
 	ret =  ath11k_wow_cleanup(ar);
 	if (ret) {
 		ath11k_warn(ar->ab, "failed to clear wow wakeup events: %d\n",
@@ -590,6 +599,14 @@ int ath11k_wow_op_suspend(struct ieee80211_hw *hw,
 		goto cleanup;
 	}
 
+	ret = ath11k_dp_rx_pktlog_stop(ar->ab, false);
+	if (ret) {
+		ath11k_warn(ar->ab,
+			    "failed to stop dp rx pktlog during wow suspend: %d\n",
+			    ret);
+		goto cleanup;
+	}
+
 	ath11k_ce_stop_shadow_timers(ar->ab);
 	ath11k_dp_stop_shadow_timers(ar->ab);
 
@@ -640,6 +657,8 @@ int ath11k_wow_op_resume(struct ieee80211_hw *hw)
 	ath11k_hif_ce_irq_enable(ar->ab);
 	ath11k_hif_irq_enable(ar->ab);
 
+	ath11k_dp_rx_pktlog_start(ar->ab);
+
 	ret = ath11k_wow_wakeup(ar->ab);
 	if (ret) {
 		ath11k_warn(ar->ab, "failed to wakeup from wow: %d\n", ret);
Signature · 1 line
2.7.4
#10 lore raw

Re: [PATCH 4/6] ath11k: purge rx pktlog when entering WoW

Quoted 4 lines · Carl Huang

This change is to purge rx pktlog when entering WoW and reap the mon_status buffer to keep it empty. When leaving WoW, host restarts the reap timer. In WoW state, it's not allowed to feed into mon_status rings per firmware request.

"per firmware request" is not clear for me. Do you "per firmware team's recommendation" or what?

drivers/net/wireless/ath/ath11k/core.c int ath11k_core_resume(struct ath11k_base *ab) · line 330

Quoted 11 lines
drivers/net/wireless/ath/ath11k/core.c +1 -6
@@ -327,12 +327,7 @@ int ath11k_core_resume(struct ath11k_base *ab)
 	ath11k_hif_ce_irq_enable(ab);
 	ath11k_hif_irq_enable(ab);
 
-	ret = ath11k_dp_rx_pktlog_start(ab);
-	if (ret) {
-		ath11k_warn(ab, "failed to start rx pktlog during resume: %d\n",
-			    ret);
-		return ret;
-	}
+	ath11k_dp_rx_pktlog_start(ab);

Why remove error handling? We should always handle the errors.

Quoted 10 lines
drivers/net/wireless/ath/ath11k/core.c +0 -0
 	ret = ath11k_wow_wakeup(ab);
 	if (ret) {
drivers/net/wireless/ath/ath11k/dp_rx.c +0 -0
index 6d769ba..ae6db5d 100644
@@ -5105,13 +5105,11 @@ int ath11k_dp_rx_pdev_mon_detach(struct ath11k *ar)
 	return 0;
 }

drivers/net/wireless/ath/ath11k/dp_rx.c int ath11k_dp_rx_pdev_mon_detach(struct ath11k *ar) · line 5113

Quoted 10 lines
drivers/net/wireless/ath/ath11k/dp_rx.c +1 -3
 
-int ath11k_dp_rx_pktlog_start(struct ath11k_base *ab)
+void ath11k_dp_rx_pktlog_start(struct ath11k_base *ab)
 {
 	/* start reap timer */
 	mod_timer(&ab->mon_reap_timer,
 		  jiffies + msecs_to_jiffies(ATH11K_MON_TIMER_INTERVAL));
-
-	return 0;
 }

Ah, you remove it here as well. But for consistency it's better to have this return an error code.

drivers/net/wireless/ath/ath11k/dp_rx.c int ath11k_wow_op_resume(struct ieee80211_hw *hw) · line 660

Quoted 5 lines
drivers/net/wireless/ath/ath11k/dp_rx.c +1 -0
@@ -640,6 +657,8 @@ int ath11k_wow_op_resume(struct ieee80211_hw *hw)
 	ath11k_hif_ce_irq_enable(ar->ab);
 	ath11k_hif_irq_enable(ar->ab);
 
+	ath11k_dp_rx_pktlog_start(ar->ab);

Error handling here as well, please.

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
author
#11 lore raw

PATCH 5/6 ath11k: support ARP and NS offload

In reply to Carl Huang

Support ARP and NS offload in WoW state.

Tested this way: put machineA with QCA6390 to WoW state, ping/ping6 machineA from another machineB, check sniffer to see any ARP response and Neighbour advertisement from machineA.

Tested-on: QCA6390 hw2.0 PCI WLAN.HST.1.0.1-01740-QCAHSTSWPLZ_V2_TO_X86-1

---
 drivers/net/wireless/ath/ath11k/core.h |  18 ++++
 drivers/net/wireless/ath/ath11k/mac.c  | 114 ++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath11k/wmi.c  | 145 +++++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath11k/wmi.h  |  46 +++++++++++
 drivers/net/wireless/ath/ath11k/wow.c  |  49 ++++++++++-
 5 files changed, 371 insertions(+), 1 deletion(-)
drivers/net/wireless/ath/ath11k/core.h +18 -0
index 6e40a33..d9c6c1d 100644
@@ -239,6 +239,23 @@ struct ath11k_reg_tpc_power_info {
 	struct chan_power_info chan_power_info[MAX_NUM_PWR_LEVEL];
 };
 
+#define ATH11K_IPV6_UC_TYPE     0
+#define ATH11K_IPV6_AC_TYPE     1
+
+#define ATH11K_IPV6_MAX_COUNT   16
+#define ATH11K_IPV4_MAX_COUNT   2
+
+struct ath11k_arp_ns_offload {
+	u8  ipv4_addr[ATH11K_IPV4_MAX_COUNT][4];
+	u32 ipv4_count;
+	u32 ipv6_count;
+	u8  ipv6_addr[ATH11K_IPV6_MAX_COUNT][16];
+	u8  self_ipv6_addr[ATH11K_IPV6_MAX_COUNT][16];
+	u8  ipv6_type[ATH11K_IPV6_MAX_COUNT];
+	bool ipv6_valid[ATH11K_IPV6_MAX_COUNT];
+	u8  mac_addr[ETH_ALEN];
+};
+
 struct ath11k_vif {
 	u32 vdev_id;
 	enum wmi_vdev_type vdev_type;
@@ -289,6 +306,7 @@ struct ath11k_vif {
 	bool wpaie_present;
 	struct ieee80211_chanctx_conf chanctx;
 	struct ath11k_reg_tpc_power_info reg_tpc_info;
+	struct ath11k_arp_ns_offload arp_ns_offload;
 };
 
 struct ath11k_vif_iter {
drivers/net/wireless/ath/ath11k/mac.c +114 -0

144 lines not on this page · raw message

drivers/net/wireless/ath/ath11k/wmi.c +145 -0

148 lines not on this page · raw message

drivers/net/wireless/ath/ath11k/wmi.h +46 -0

62 lines not on this page · raw message

drivers/net/wireless/ath/ath11k/wow.c +48 -1

68 lines not on this page · raw message

Signature · 1 line
2.7.4
#12 lore raw

Re: [PATCH 5/6] ath11k: support ARP and NS offload

Quoted 5 lines · Carl Huang

Support ARP and NS offload in WoW state.

Tested this way: put machineA with QCA6390 to WoW state, ping/ping6 machineA from another machineB, check sniffer to see any ARP response and Neighbour advertisement from machineA.

s/machineA/machine A/
s/machineB/machine B/

Quoted 7 lines
drivers/net/wireless/ath/ath11k/wow.c +1 -1
@@ -672,8 +712,15 @@ int ath11k_wow_op_resume(struct ieee80211_hw *hw)
 	}
 
 	ret = ath11k_wow_clear_hw_filter(ar);
-	if (ret)
+	if (ret) {
 		ath11k_warn(ar->ab, "failed to clear hw filter: %d\n", ret);

drivers/net/wireless/ath/ath11k/wow.c int ath11k_wow_op_resume(struct ieee80211_hw *hw) · line 726

Quoted 10 lines
drivers/net/wireless/ath/ath11k/wow.c +7 -0
+		goto exit;
+	}
+
+	ret = ath11k_wow_protocol_offload(ar, false);
+	if (ret)
+		ath11k_warn(ar->ab, "failed to clear wow protocol offload events: %d\n",
+			    ret);
 
 exit:
 	if (ret) {

For consistency please add goto exit for ath11k_wow_protocol_offload().

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
#13 lore raw

Re: [PATCH 5/6] ath11k: support ARP and NS offload

In reply to Carl Huang
Quoted 10 lines · Carl Huang

Support ARP and NS offload in WoW state.

Tested this way: put machineA with QCA6390 to WoW state, ping/ping6 machineA from another machineB, check sniffer to see any ARP response and Neighbour advertisement from machineA.

Tested-on: QCA6390 hw2.0 PCI WLAN.HST.1.0.1-01740-QCAHSTSWPLZ_V2_TO_X86-1

[...]

drivers/net/wireless/ath/ath11k/wow.c static int ath11k_wow_clear_hw_filter(struct ath11k *ar) · line 582

Quoted 7 lines
drivers/net/wireless/ath/ath11k/wow.c +7 -0
+static int ath11k_wow_protocol_offload(struct ath11k *ar, bool enable)
+{
+	int ret;
+
+	ret = ath11k_wow_arp_ns_offload(ar, enable);
+
+	return ret;

For consistency, please add error handling for
ath11k_wow_arp_ns_offload().

Signature · 8 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

-- 
ath11k mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/ath11k
author
#14 lore raw

PATCH 6/6 ath11k: support GTK rekey offload

In reply to Carl Huang

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

---
 drivers/net/wireless/ath/ath11k/core.h |   8 +++
 drivers/net/wireless/ath/ath11k/mac.c  |  32 ++++++++++
 drivers/net/wireless/ath/ath11k/wmi.c  | 110 +++++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath11k/wmi.h  |  39 ++++++++++++
 drivers/net/wireless/ath/ath11k/wow.c  |  49 ++++++++++++++-
 5 files changed, 237 insertions(+), 1 deletion(-)
drivers/net/wireless/ath/ath11k/core.h +8 -0
index d9c6c1d..69461c3 100644
@@ -256,6 +256,13 @@ struct ath11k_arp_ns_offload {
 	u8  mac_addr[ETH_ALEN];
 };
 
+struct ath11k_rekey_data {
+	u8 kck[NL80211_KCK_LEN];
+	u8 kek[NL80211_KCK_LEN];
+	u64 replay_ctr;
+	bool enable_offload;
+};
+
 struct ath11k_vif {
 	u32 vdev_id;
 	enum wmi_vdev_type vdev_type;
@@ -307,6 +314,7 @@ struct ath11k_vif {
 	struct ieee80211_chanctx_conf chanctx;
 	struct ath11k_reg_tpc_power_info reg_tpc_info;
 	struct ath11k_arp_ns_offload arp_ns_offload;
+	struct ath11k_rekey_data rekey_data;
 };
 
 struct ath11k_vif_iter {
drivers/net/wireless/ath/ath11k/mac.c +32 -0
index e3dc998..797e45e 100644
@@ -2206,6 +2206,7 @@ static void ath11k_bss_assoc(struct ieee80211_hw *hw,
 	}
 
 	arvif->is_up = true;
+	arvif->rekey_data.enable_offload = false;
 
 	ath11k_dbg(ar->ab, ATH11K_DBG_MAC,
 		   "mac vdev %d up (associated) bssid %pM aid %d\n",
@@ -2239,6 +2240,8 @@ static void ath11k_bss_disassoc(struct ieee80211_hw *hw,
 
 	arvif->is_up = false;
 
+	memset(&arvif->rekey_data, 0, sizeof(arvif->rekey_data));
+
 	cancel_delayed_work_sync(&arvif->connection_loss_work);
 }
 
@@ -7451,6 +7454,34 @@ static void ath11k_mac_op_ipv6_changed(struct ieee80211_hw *hw,
 	ath11k_generate_ns_mc_addr(ar, offload);
 }
 
+static void ath11k_mac_op_set_rekey_data(struct ieee80211_hw *hw,
+					 struct ieee80211_vif *vif,
+					 struct cfg80211_gtk_rekey_data *data)
+{
+	struct ath11k *ar = hw->priv;
+	struct ath11k_vif *arvif = ath11k_vif_to_arvif(vif);
+	struct ath11k_rekey_data *rekey_data = &arvif->rekey_data;
+
+	ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "Set rekey data vdev_id %d\n",
+		   arvif->vdev_id);
+	mutex_lock(&ar->conf_mutex);
+
+	memcpy(rekey_data->kck, data->kck, NL80211_KCK_LEN);
+	memcpy(rekey_data->kek, data->kek, NL80211_KEK_LEN);
+
+	/* supplicant works on big-endian, converts to cpu-endian */
+	rekey_data->replay_ctr = be64_to_cpup((__be64 *)data->replay_ctr);
+	arvif->rekey_data.enable_offload = true;
+
+	ath11k_dbg_dump(ar->ab, ATH11K_DBG_MAC, "KCK", NULL,
+			rekey_data->kck, NL80211_KCK_LEN);
+	ath11k_dbg_dump(ar->ab, ATH11K_DBG_MAC, "KEK", NULL,
+			rekey_data->kck, NL80211_KEK_LEN);
+	ath11k_dbg_dump(ar->ab, ATH11K_DBG_MAC, "replay ctr", NULL,
+			&rekey_data->replay_ctr, sizeof(rekey_data->replay_ctr));
+	mutex_unlock(&ar->conf_mutex);
+}
+
 static const struct ieee80211_ops ath11k_ops = {
 	.tx				= ath11k_mac_op_tx,
 	.start                          = ath11k_mac_op_start,
@@ -7465,6 +7496,7 @@ static const struct ieee80211_ops ath11k_ops = {
 	.hw_scan                        = ath11k_mac_op_hw_scan,
 	.cancel_hw_scan                 = ath11k_mac_op_cancel_hw_scan,
 	.set_key                        = ath11k_mac_op_set_key,
+	.set_rekey_data	                = ath11k_mac_op_set_rekey_data,
 	.sta_state                      = ath11k_mac_op_sta_state,
 	.sta_set_txpwr			= ath11k_mac_op_sta_set_txpwr,
 	.sta_rc_update			= ath11k_mac_op_sta_rc_update,
drivers/net/wireless/ath/ath11k/wmi.c +110 -0

125 lines not on this page · raw message

drivers/net/wireless/ath/ath11k/wmi.h +39 -0
index c2e5557..61dc5ea 100644
@@ -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;
+
+struct wmi_gtk_rekey_offload_cmd {
+	u32 tlv_header;
+	u32 vdev_id;
+	u32 flags;
+	u8 kek[GTK_OFFLOAD_KEK_BYTES];
+	u8 kck[GTK_OFFLOAD_KCK_BYTES];
+	u8 replay_ctr[GTK_REPLAY_COUNTER_BYTES];
+} __packed;
+
 int ath11k_wmi_cmd_send(struct ath11k_pdev_wmi *wmi, struct sk_buff *skb,
 			u32 cmd_id);
 struct sk_buff *ath11k_wmi_alloc_skb(struct ath11k_wmi_base *wmi_sc, u32 len);
@@ -6017,4 +6052,8 @@ int ath11k_wmi_send_hw_data_filter_cmd(struct ath11k *ar, u32 vdev_id,
 				       u32 filter_bitmap, bool enable);
 int ath11k_wmi_arp_ns_offload(struct ath11k *ar,
 			      struct ath11k_vif *arvif, bool enable);
+int ath11k_wmi_gtk_rekey_offload(struct ath11k *ar,
+				 struct ath11k_vif *arvif, bool enable);
+int ath11k_wmi_gtk_rekey_getinfo(struct ath11k *ar,
+				 struct ath11k_vif *arvif);
 #endif
drivers/net/wireless/ath/ath11k/wow.c +48 -1

66 lines not on this page · raw message

Signature · 1 line
2.7.4
#15 lore raw

Re: [PATCH 6/6] ath11k: support GTK rekey offload

Quoted 6 lines · Carl Huang

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

[...]

Quoted 31 lines
drivers/net/wireless/ath/ath11k/wmi.h +25 -0
@@ -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;

[...]

drivers/net/wireless/ath/ath11k/wmi.h

Quoted 6 lines
drivers/net/wireless/ath/ath11k/wmi.h +6 -0
+	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?

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
author
#16 lore raw

Re: [PATCH 6/6] ath11k: support GTK rekey offload

Quoted 45 lines · Kalle Valo
Quoted 6 lines · Carl Huang

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

[...]

Quoted 31 lines
drivers/net/wireless/ath/ath11k/wmi.h +25 -0
@@ -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;

[...]

Quoted 15 lines

drivers/net/wireless/ath/ath11k/wmi.h

Quoted 8 lines
drivers/net/wireless/ath/ath11k/wmi.h +1 -0
+	arvif->rekey_data.replay_ctr = le64_to_cpup((__le64 

*)ev->replay_counter);

drivers/net/wireless/ath/ath11k/wmi.h +2 -0
+	/* supplicant expects big-endian replay counter */
+	replay_ctr = cpu_to_be64(le64_to_cpup((__le64 

*)ev->replay_counter));

drivers/net/wireless/ath/ath11k/wmi.h +3 -0
+
+	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.

Quoted 4 lines

--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

#17 lore raw

Re: [PATCH 6/6] ath11k: support GTK rekey offload

Quoted 4 lines · On Thursday, 9 December 2021 17:05:14 CET Kalle Valo wrote:
Quoted 3 lines

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?

[...]

Quoted 1 line · 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.

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:

Quoted 3 lines
Quoted 3 lines
(quoted diff) +2 -0
+       /* 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));

Kind regards,
Sven

#18 lore raw

Re: [PATCH 6/6] ath11k: support GTK rekey offload

Quoted 1 line · On Saturday, 18 December 2021 09:37:02 CET Sven Eckelmann wrote:

Why isn't the firmware taking care of the conversion at that place?

Forget this part - this was non-sense.

But the rest of the questions still stand.

Kind regards,
Sven

#19 lore raw

Re: [PATCH 6/6] ath11k: support GTK rekey offload

In reply to Sven Eckelmann
Quoted 8 lines · On Saturday, 18 December 2021 09:37:02 CET Sven Eckelmann wrote:

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));

Sorry for the noise, but the part of not knowing in which endianness the firmware return multi-byte values is freaking me out. The above statements assume that it is returning everything as little endian.

If it is actually returns in host byte order (no idea how the firmware determines this) then of course, the questions should be:

* Why are you defining it as u8 replay_counter[GTK_REPLAY_COUNTER_BYTES]; in the struct instead of using u64 replay_counter;?

* What ensures that this is value is 64 bit aligned in memory? Wouldn't it be more correct (assuming it is a u64) to use

    replay_ctr = cpu_to_be64(get_unaligned64(ev->replay_counter));

Kind regards,
Sven

#20 lore raw

Re: [PATCH 6/6] ath11k: support GTK rekey offload

In reply to Sven Eckelmann
Quoted 13 lines · Sven Eckelmann
Quoted 4 lines · On Thursday, 9 December 2021 17:05:14 CET Kalle Valo wrote:
Quoted 3 lines

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?

[...]

Quoted 1 line · 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.

Quoted 8 lines

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.

Quoted 10 lines

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:

Quoted 3 lines
Quoted 3 lines
(quoted diff) +2 -0
+       /* 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?

Quoted 7 lines

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;
		}
	}
}
Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
#21 lore raw

Re: [PATCH 6/6] ath11k: support GTK rekey offload

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.

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.

Quoted 9 lines

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)

Quoted 13 lines
{
	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.

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].

But if the value is an u64, then the code in the patch is wrong:

Quoted 2 lines

/* supplicant expects big-endian replay counter */ replay_ctr = cpu_to_be64(le64_to_cpup((__le64 *)ev->replay_counter));

This would break on big endian architectures because ev->replay_counter is a __be64 and not a __le64 on these systems. Just from the way the byte ordering is supposed to look like - not the data type for the C-compiler).

If you have a look at what the code does (beside 64 bit load by _cpup), is just to add a single swap64 - either by cpu_to_be64 or by le64_to_cpup - depending on whether the host system is little endian or big endian.

So for a __le64, it would (besides the incorrectly aligned 64 bit load from struct wmi_gtk_offload_status_event), do a single swap64 to __be64. This swap64 is from cpu_to_be64 and le64_to_cpup doesn't swap anything.

But on a big endian system, the __be64 would also be sent through a swap64 (from le64_to_cpup) and cpu_to_be64 wouldn't swap anything. So at the end, it would be a __le64. So something which conflicts with the comment above this code line.

There are now various ways to correctly implement it:

* change replay_counter to an u64 in the (packed) struct and:

      replay_ctr = cpu_to_be64(ev->replay_counter);

* keep it as u8[8] in the struct and make sure yourself that an unaligned-safe 64 bit load is used:

      replay_ctr = cpu_to_be64(get_unaligned((u64 *)ev->replay_counter));

Kind regards,
Sven

[1] It is something like "jumble" or "mess"

#22 lore raw

Re: [PATCH 6/6] ath11k: support GTK rekey offload

Quoted 5 lines · Sven Eckelmann

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.

Quoted 6 lines

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.

Quoted 23 lines
Quoted 9 lines

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)

Quoted 13 lines
{
	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;
		}
	}
}
Quoted 8 lines

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.

Quoted 2 lines

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 :)

Quoted 1 line

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.

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
#23 lore raw

Re: [PATCH 6/6] ath11k: support GTK rekey offload

Quoted 8 lines · On Tuesday, 21 December 2021 15:48:53 CET Kalle Valo wrote:

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):

Quoted 8 lines

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.

Ok, just to summarize: the value is 0x0807060504030201 -> which is is correctly stored in memory as 0102030405060708 for little endian systems. Fine with this part. So if there would only be little endian systems than following code would be fine:

     replay_ctr = cpu_to_be64(get_unaligned((u64 *)ev->replay_counter));

According to the info from here, the memory from the firmware on big endian systems is 0403020108070605. So after switching it with the ath11k swap function, it is back to 0102030405060708 -> which is little endian again (and not aligned in memory). We must take care of it by converting in from __le64 to a u64 before converting it to __be64. So we would end up with:

    __le64 replay_ctr_le;
    __be64 replay_ctr_be;
    u64 replay_ctr;

    /* TODO also swap bytes for (i)gt_key* back to little endian */
    ath11k_ce_byte_swap(ev->replay_counter, sizeof(ev->replay_counter));

    replay_ctr_le = get_unaligned((__le64 *)ev->replay_counter);
    replay_ctr = le64_to_cpu(replay_ctr_le);
    arvif->rekey_data.replay_ctr = replay_ctr;
    replay_ctr_be = cpu_to_be64(replay_ctr);

Of course, completely untested.

Another idea would be to change wmi_gtk_offload_status_event->replay_counter into two u32. In that case, it would be enough to do:

   __be64 replay_ctr_be;
    u64 replay_ctr;

    replay_ctr = ev->replay_counter[1];
    replay_ctr <<= 32;
    replay_ctr |= ev->replay_counter[0];

    arvif->rekey_data.replay_ctr = replay_ctr;
    replay_ctr_be = cpu_to_be64(replay_ctr);

replay_counter[1] could also be called replay_counter_upper - and replay_counter[0] just replay_counter_lower.

Which reminds me of that the memcpy from a u64 to wmi_gtk_rekey_offload_cmd->replay_ctrl in ath11k_wmi_gtk_rekey_offload. This is of course also wrong. It must first be converted into a __le64 and the bytes must be pre-swapped (see below).

Quoted 3 lines

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.

[...]

Quoted 4 lines

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.

Ok, so wmi_gtk_offload_status_event and wmi_gtk_rekey_offload_cmd are breaking this assertion because they are full of u8's. :(

Especially wmi_gtk_offload_status_event is problematic here because there are now a lot of single u8's in it. The original code must therefore use ath11k_ce_byte_swap on everything after wmi_gtk_offload_status_event->refresh_cnt before accessing any of the members.

And ath11k_wmi_gtk_rekey_offload must perform the ath11k_ce_byte_swap on

* wmi_gtk_rekey_offload_cmd->kek
* wmi_gtk_rekey_offload_cmd->kck
* wmi_gtk_rekey_offload_cmd->replay_ctr

See also above why the memcpy to wmi_gtk_rekey_offload_cmd->replay_ctr is wrong in this function.

And it seems like wmi_obss_color_collision_event->obss_color_bitmap (see ath11k_wmi_obss_color_collision_event) could suffer from a similar problem. Maybe key_rsc_counter in ath11k_wmi_vdev_install_key too - but this doesn't have any producers yet.

Kind regards,
Sven

#24 lore raw
In reply to Carl Huang
Quoted 7 lines · Carl Huang

Ath11k WoW basic funtionalities are merged from ath10k such as magic-pattern, PNO, disconnect and patterns.

Hw data filter and pktlog purge are required for ath11k.

ARP and NS offload, GTK rekey offload are new WoW features implemented on ath11k.

Had few minor comments, but this looked good. Next time remember to use ath.git master branch as the baseline and use --base, makes it easier for me to apply the patches.

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches