[PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[Venkateswara Naralasetty]

AP power save feature is to save power in AP mode, where AP goes
to power save mode when no stations associate to it and comes out
of power save when any station associate to AP.

This patch is to add vendor command support to enable/disable
ap power save.

An example of usage: iw dev wlanx vendor send 0x1374 0x4a ap-ps <val>

0x1374: vendor id
0x4a: vendor subcmd id
val: 0 - disable power save
     1 - enable power save

Signed-off-by: Venkateswara Naralasetty <vnaralas@codeaurora.org>
---
v2:
 * added use case in the commit log.

 include/uapi/nl80211-vnd-qca.h | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 include/uapi/nl80211-vnd-qca.h

diff --git a/include/uapi/nl80211-vnd-qca.h b/include/uapi/nl80211-vnd-qca.h
new file mode 100644
index 0000000..357040a
--- /dev/null
+++ b/include/uapi/nl80211-vnd-qca.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: ISC */
+/*
+ * Copyright (c) 2019-2020 The Linux Foundation. All rights reserved.
+ */
+
+#ifndef _UAPI_NL80211_VND_QCA_H
+#define _UAPI_NL80211_VND_QCA_H
+
+/* Vendor id to be used in vendor specific command and events to user space
+ * NOTE: The authoritative place for definition of QCA_NL80211_VENDOR_ID,
+ * vendor subcmd definitions prefixed with QCA_NL80211_VENDOR_SUBCMD, and
+ * qca_wlan_vendor_attr is open source file src/common/qca-vendor.h in
+ * git://w1.fi/srv/git/hostap.git; the values here are just a copy of that
+ */
+#define QCA_NL80211_VENDOR_ID 0x001374
+
+/**
+ * enum qca_nl80211_vendor_subcmds - QCA nl80211 vendor command identifiers
+ *
+ * @QCA_NL80211_VENDOR_SUBCMD_SET_WIFI_CONFIGURATION: This vendor subcommand is
+ * used to set wifi configurations by the attributes defined in
+ * enum qca_wlan_vendor_attr_config.
+ */
+enum qca_nl80211_vendor_subcmds {
+	QCA_NL80211_VENDOR_SUBCMD_SET_WIFI_CONFIGURATION = 74,
+};
+
+/**
+ * enum qca_wlan_vendor_attr_config: Attributes for data used by
+ * QCA_NL80211_VENDOR_SUBCMD_SET_WIFI_CONFIGURATION.
+ *
+ * @QCA_WLAN_VENDOR_ATTR_CONFIG_GTX: 8-bit unsigned value to trigger
+ * green Tx power saving 1-Enable, 0-Disable.
+ */
+enum qca_wlan_vendor_attr_config {
+	QCA_WLAN_VENDOR_ATTR_CONFIG_GTX = 57,
+
+	QCA_WLAN_VENDOR_ATTR_CONFIG_AFTER_LAST,
+	QCA_WLAN_VENDOR_ATTR_CONFIG_MAX =
+		QCA_WLAN_VENDOR_ATTR_CONFIG_AFTER_LAST - 1,
+};
+#endif /* _UAPI_NL80211_VND_QCA_H_ */
-- 
2.7.4

[PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[Venkateswara Naralasetty]

AP power save feature is to save power in AP mode, where AP goes
to power save mode when no stations associate to it and comes out
of power save when any station associate to AP.

This patch is to add vendor command support to enable/disable
ap power save.

An example of usage: iw dev wlanx vendor send 0x1374 0x4a ap-ps <val>

0x1374: vendor id
0x4a: vendor subcmd id
val: 0 - disable power save
     1 - enable power save

Signed-off-by: Venkateswara Naralasetty <vnaralas@codeaurora.org>
---
v2:
 * added use case in the commit log.

 include/uapi/nl80211-vnd-qca.h | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 include/uapi/nl80211-vnd-qca.h

diff --git a/include/uapi/nl80211-vnd-qca.h b/include/uapi/nl80211-vnd-qca.h
new file mode 100644
index 0000000..357040a
--- /dev/null
+++ b/include/uapi/nl80211-vnd-qca.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: ISC */
+/*
+ * Copyright (c) 2019-2020 The Linux Foundation. All rights reserved.
+ */
+
+#ifndef _UAPI_NL80211_VND_QCA_H
+#define _UAPI_NL80211_VND_QCA_H
+
+/* Vendor id to be used in vendor specific command and events to user space
+ * NOTE: The authoritative place for definition of QCA_NL80211_VENDOR_ID,
+ * vendor subcmd definitions prefixed with QCA_NL80211_VENDOR_SUBCMD, and
+ * qca_wlan_vendor_attr is open source file src/common/qca-vendor.h in
+ * git://w1.fi/srv/git/hostap.git; the values here are just a copy of that
+ */
+#define QCA_NL80211_VENDOR_ID 0x001374
+
+/**
+ * enum qca_nl80211_vendor_subcmds - QCA nl80211 vendor command identifiers
+ *
+ * @QCA_NL80211_VENDOR_SUBCMD_SET_WIFI_CONFIGURATION: This vendor subcommand is
+ * used to set wifi configurations by the attributes defined in
+ * enum qca_wlan_vendor_attr_config.
+ */
+enum qca_nl80211_vendor_subcmds {
+	QCA_NL80211_VENDOR_SUBCMD_SET_WIFI_CONFIGURATION = 74,
+};
+
+/**
+ * enum qca_wlan_vendor_attr_config: Attributes for data used by
+ * QCA_NL80211_VENDOR_SUBCMD_SET_WIFI_CONFIGURATION.
+ *
+ * @QCA_WLAN_VENDOR_ATTR_CONFIG_GTX: 8-bit unsigned value to trigger
+ * green Tx power saving 1-Enable, 0-Disable.
+ */
+enum qca_wlan_vendor_attr_config {
+	QCA_WLAN_VENDOR_ATTR_CONFIG_GTX = 57,
+
+	QCA_WLAN_VENDOR_ATTR_CONFIG_AFTER_LAST,
+	QCA_WLAN_VENDOR_ATTR_CONFIG_MAX =
+		QCA_WLAN_VENDOR_ATTR_CONFIG_AFTER_LAST - 1,
+};
+#endif /* _UAPI_NL80211_VND_QCA_H_ */
-- 
2.7.4


-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

[PATCHv2 2/2] ath11k: Add ap power save support

[Venkateswara Naralasetty]

AP power save where AP goes to power save mode when no stations associate
to it and come out of power save when any station associate to AP.

This AP power save capability can be used to save power with the drawback
of reduced range or delayed discovery of the AP

This patch also porvides user configuration to enable/disable
this feature using vendor command. This feature is disabled by default.

Tested-on: IPQ8074 WLAN.HK.2.1.0.1-01228-QCAHKSWPL_SILICONZ-1

Signed-off-by: Venkateswara Naralasetty <vnaralas@codeaurora.org>
---
 drivers/net/wireless/ath/ath11k/Makefile |  3 +-
 drivers/net/wireless/ath/ath11k/core.h   |  8 ++++
 drivers/net/wireless/ath/ath11k/mac.c    | 44 +++++++++++++++++
 drivers/net/wireless/ath/ath11k/mac.h    |  1 +
 drivers/net/wireless/ath/ath11k/vendor.c | 81 ++++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath11k/vendor.h | 12 +++++
 drivers/net/wireless/ath/ath11k/wmi.c    | 32 +++++++++++++
 drivers/net/wireless/ath/ath11k/wmi.h    |  7 +++
 8 files changed, 187 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/wireless/ath/ath11k/vendor.c
 create mode 100644 drivers/net/wireless/ath/ath11k/vendor.h

diff --git a/drivers/net/wireless/ath/ath11k/Makefile b/drivers/net/wireless/ath/ath11k/Makefile
index bc4911f..f0b4975 100644
--- a/drivers/net/wireless/ath/ath11k/Makefile
+++ b/drivers/net/wireless/ath/ath11k/Makefile
@@ -16,7 +16,8 @@ ath11k-y += core.o \
 	    ce.o \
 	    peer.o \
 	    dbring.o \
-	    hw.o
+	    hw.o \
+	    vendor.o
 
 ath11k-$(CONFIG_ATH11K_DEBUGFS) += debug_htt_stats.o debugfs_sta.o
 ath11k-$(CONFIG_NL80211_TESTMODE) += testmode.o
diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h
index d21191c..827020f 100644
--- a/drivers/net/wireless/ath/ath11k/core.h
+++ b/drivers/net/wireless/ath/ath11k/core.h
@@ -23,6 +23,7 @@
 #include "thermal.h"
 #include "dbring.h"
 #include "spectral.h"
+#include "vendor.h"
 
 #define SM(_v, _f) (((_v) << _f##_LSB) & _f##_MASK)
 
@@ -419,6 +420,11 @@ struct ath11k_vdev_stop_status {
 	u32  vdev_id;
 };
 
+enum ath11k_ap_ps_state {
+	ATH11K_AP_PS_STATE_OFF,
+	ATH11K_AP_PS_STATE_ON,
+};
+
 struct ath11k {
 	struct ath11k_base *ab;
 	struct ath11k_pdev *pdev;
@@ -543,6 +549,8 @@ struct ath11k {
 #endif
 	bool dfs_block_radar_events;
 	struct ath11k_thermal thermal;
+	int ap_ps_enabled;
+	enum ath11k_ap_ps_state ap_ps_state;
 };
 
 struct ath11k_band_cap {
diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c
index f4a085b..1885834 100644
--- a/drivers/net/wireless/ath/ath11k/mac.c
+++ b/drivers/net/wireless/ath/ath11k/mac.c
@@ -2898,6 +2898,33 @@ static void ath11k_mac_dec_num_stations(struct ath11k_vif *arvif,
 	ar->num_stations--;
 }
 
+int ath11k_mac_ap_ps_recalc(struct ath11k *ar)
+{
+	struct ath11k_vif *arvif;
+	bool has_sta_iface = false;
+	enum ath11k_ap_ps_state state = ATH11K_AP_PS_STATE_OFF;
+	int ret = 0;
+
+	list_for_each_entry(arvif, &ar->arvifs, list) {
+		if (arvif->vdev_type == WMI_VDEV_TYPE_STA) {
+			has_sta_iface = true;
+			break;
+		}
+	}
+
+	if (!has_sta_iface && !ar->num_stations && ar->ap_ps_enabled)
+		state = ATH11K_AP_PS_STATE_ON;
+
+	if (ar->ap_ps_state == state)
+		return ret;
+
+	ret = ath11k_wmi_pdev_ap_ps_cmd_send(ar, ar->pdev->pdev_id, state);
+	if (!ret)
+		ar->ap_ps_state = state;
+
+	return ret;
+}
+
 static int ath11k_mac_station_add(struct ath11k *ar,
 				  struct ieee80211_vif *vif,
 				  struct ieee80211_sta *sta)
@@ -2937,6 +2964,12 @@ static int ath11k_mac_station_add(struct ath11k *ar,
 	ath11k_dbg(ab, ATH11K_DBG_MAC, "Added peer: %pM for VDEV: %d\n",
 		   sta->addr, arvif->vdev_id);
 
+	ret = ath11k_mac_ap_ps_recalc(ar);
+	if (ret) {
+		ath11k_warn(ar->ab, "failed to send ap ps ret %d\n", ret);
+		goto exit;
+	}
+
 	if (ath11k_debug_is_extd_tx_stats_enabled(ar)) {
 		arsta->tx_stats = kzalloc(sizeof(*arsta->tx_stats), GFP_KERNEL);
 		if (!arsta->tx_stats) {
@@ -3046,6 +3079,10 @@ static int ath11k_mac_op_sta_state(struct ieee80211_hw *hw,
 
 		kfree(arsta->rx_stats);
 		arsta->rx_stats = NULL;
+
+		ret = ath11k_mac_ap_ps_recalc(ar);
+		if (ret)
+			ath11k_warn(ar->ab, "failed to send ap ps ret %d\n", ret);
 	} else if (old_state == IEEE80211_STA_AUTH &&
 		   new_state == IEEE80211_STA_ASSOC &&
 		   (vif->type == NL80211_IFTYPE_AP ||
@@ -4210,6 +4247,7 @@ static void ath11k_mac_op_stop(struct ieee80211_hw *hw)
 
 	clear_bit(ATH11K_CAC_RUNNING, &ar->dev_flags);
 	ar->state = ATH11K_STATE_OFF;
+	ar->ap_ps_state = ATH11K_AP_PS_STATE_OFF;
 	mutex_unlock(&ar->conf_mutex);
 
 	cancel_delayed_work_sync(&ar->scan.timeout);
@@ -4533,6 +4571,10 @@ static int ath11k_mac_op_add_interface(struct ieee80211_hw *hw,
 
 	ath11k_dp_vdev_tx_attach(ar, arvif);
 
+	ret = ath11k_mac_ap_ps_recalc(ar);
+	if (ret)
+		ath11k_warn(ar->ab, "failed to set ap ps ret %d\n", ret);
+
 	mutex_unlock(&ar->conf_mutex);
 
 	return 0;
@@ -4619,6 +4661,7 @@ static void ath11k_mac_op_remove_interface(struct ieee80211_hw *hw,
 
 	/* Recalc txpower for remaining vdev */
 	ath11k_mac_txpower_recalc(ar);
+	ath11k_mac_ap_ps_recalc(ar);
 	clear_bit(ATH11K_FLAG_MONITOR_ENABLED, &ar->monitor_flags);
 
 	/* TODO: recal traffic pause state based on the available vdevs */
@@ -6175,6 +6218,7 @@ static int __ath11k_mac_register(struct ath11k *ar)
 		ARRAY_SIZE(ath11k_iftypes_ext_capa);
 
 	ath11k_reg_init(ar);
+	ath11k_vendor_register(ar);
 
 	/* advertise HW checksum offload capabilities */
 	ar->hw->netdev_features = NETIF_F_HW_CSUM;
diff --git a/drivers/net/wireless/ath/ath11k/mac.h b/drivers/net/wireless/ath/ath11k/mac.h
index 0607479..18d2f28 100644
--- a/drivers/net/wireless/ath/ath11k/mac.h
+++ b/drivers/net/wireless/ath/ath11k/mac.h
@@ -146,4 +146,5 @@ int ath11k_mac_tx_mgmt_pending_free(int buf_id, void *skb, void *ctx);
 u8 ath11k_mac_bw_to_mac80211_bw(u8 bw);
 enum ath11k_supported_bw ath11k_mac_mac80211_bw_to_ath11k_bw(enum rate_info_bw bw);
 enum hal_encrypt_type ath11k_dp_tx_get_encrypt_type(u32 cipher);
+int ath11k_mac_ap_ps_recalc(struct ath11k *ar);
 #endif
diff --git a/drivers/net/wireless/ath/ath11k/vendor.c b/drivers/net/wireless/ath/ath11k/vendor.c
new file mode 100644
index 0000000..b28cc65
--- /dev/null
+++ b/drivers/net/wireless/ath/ath11k/vendor.c
@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: BSD-3-Clause-Clear
+/*
+ * Copyright (c) 2019-2020 The Linux Foundation. All rights reserved.
+ */
+
+#include <net/netlink.h>
+#include <net/mac80211.h>
+#include <uapi/nl80211-vnd-qca.h>
+#include "core.h"
+#include "debug.h"
+
+static const struct nla_policy
+ath11k_vendor_set_wifi_config_policy[QCA_WLAN_VENDOR_ATTR_CONFIG_MAX + 1] = {
+	[QCA_WLAN_VENDOR_ATTR_CONFIG_GTX] = {.type = NLA_FLAG}
+};
+
+static int ath11k_vendor_set_wifi_config(struct wiphy *wihpy,
+					 struct wireless_dev *wdev,
+					 const void *data,
+					 int data_len)
+{
+	struct ieee80211_vif *vif;
+	struct ath11k_vif *arvif;
+	struct ath11k *ar;
+	struct nlattr *tb[QCA_WLAN_VENDOR_ATTR_CONFIG_MAX + 1];
+	int ret = 0;
+
+	if (!wdev)
+		return -EINVAL;
+
+	vif = wdev_to_ieee80211_vif(wdev);
+	if (!vif)
+		return -EINVAL;
+
+	arvif = (struct ath11k_vif *)vif->drv_priv;
+	if (!arvif)
+		return -EINVAL;
+
+	ar = arvif->ar;
+
+	mutex_lock(&ar->conf_mutex);
+
+	ret = nla_parse(tb, QCA_WLAN_VENDOR_ATTR_CONFIG_MAX, data, data_len,
+			ath11k_vendor_set_wifi_config_policy, NULL);
+	if (ret) {
+		ath11k_warn(ar->ab, "invalid set wifi config policy attribute\n");
+		goto exit;
+	}
+
+	ar->ap_ps_enabled = nla_get_flag(tb[QCA_WLAN_VENDOR_ATTR_CONFIG_GTX]);
+	ret = ath11k_mac_ap_ps_recalc(ar);
+	if (ret) {
+		ath11k_warn(ar->ab, "failed to send ap ps ret %d\n", ret);
+		goto exit;
+	}
+
+exit:
+	mutex_unlock(&ar->conf_mutex);
+	return ret;
+}
+
+static struct wiphy_vendor_command ath11k_vendor_commands[] = {
+	{
+		.info.vendor_id = QCA_NL80211_VENDOR_ID,
+		.info.subcmd = QCA_NL80211_VENDOR_SUBCMD_SET_WIFI_CONFIGURATION,
+		.flags = WIPHY_VENDOR_CMD_NEED_WDEV |
+			WIPHY_VENDOR_CMD_NEED_RUNNING,
+		.doit = ath11k_vendor_set_wifi_config,
+		.policy = ath11k_vendor_set_wifi_config_policy,
+		.maxattr = QCA_WLAN_VENDOR_ATTR_CONFIG_MAX
+	}
+};
+
+int ath11k_vendor_register(struct ath11k *ar)
+{
+	ar->hw->wiphy->vendor_commands = ath11k_vendor_commands;
+	ar->hw->wiphy->n_vendor_commands = ARRAY_SIZE(ath11k_vendor_commands);
+
+	return 0;
+}
+
diff --git a/drivers/net/wireless/ath/ath11k/vendor.h b/drivers/net/wireless/ath/ath11k/vendor.h
new file mode 100644
index 0000000..6eaf07e
--- /dev/null
+++ b/drivers/net/wireless/ath/ath11k/vendor.h
@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: BSD-3-Clause-Clear
+/*
+ * Copyright (c) 2019-2020 The Linux Foundation. All rights reserved.
+ */
+
+#ifndef ATH11K_VENDOR_H
+#define ATH11K_VENDOR_H
+
+int ath11k_vendor_register(struct ath11k *ar);
+
+#endif /* QCA_VENDOR_H */
+
diff --git a/drivers/net/wireless/ath/ath11k/wmi.c b/drivers/net/wireless/ath/ath11k/wmi.c
index a66576f..9aa1718 100644
--- a/drivers/net/wireless/ath/ath11k/wmi.c
+++ b/drivers/net/wireless/ath/ath11k/wmi.c
@@ -1188,6 +1188,38 @@ ath11k_wmi_rx_reord_queue_remove(struct ath11k *ar,
 	return ret;
 }
 
+int ath11k_wmi_pdev_ap_ps_cmd_send(struct ath11k *ar, u8 pdev_id,
+				   u32 param_value)
+{
+	struct ath11k_pdev_wmi *wmi = ar->wmi;
+	struct wmi_pdev_ap_ps_cmd *cmd;
+	struct sk_buff *skb;
+	int ret;
+
+	skb = ath11k_wmi_alloc_skb(wmi->wmi_ab, sizeof(*cmd));
+	if (!skb)
+		return -ENOMEM;
+
+	cmd = (struct wmi_pdev_ap_ps_cmd *)skb->data;
+	cmd->tlv_header = FIELD_PREP(WMI_TLV_TAG,
+				     WMI_TAG_PDEV_GREEN_AP_PS_ENABLE_CMD) |
+			  FIELD_PREP(WMI_TLV_LEN, sizeof(*cmd) - TLV_HDR_SIZE);
+	cmd->pdev_id = pdev_id;
+	cmd->param_value = param_value;
+
+	ret = ath11k_wmi_cmd_send(wmi, skb, WMI_PDEV_GREEN_AP_PS_ENABLE_CMDID);
+	if (ret) {
+		ath11k_warn(ar->ab, "failed to send ap ps enable/disable cmd\n");
+		dev_kfree_skb(skb);
+	}
+
+	ath11k_dbg(ar->ab, ATH11K_DBG_WMI,
+		   "wmi pdev ap ps set pdev id %d value %d\n",
+		   pdev_id, param_value);
+
+	return ret;
+}
+
 int ath11k_wmi_pdev_set_param(struct ath11k *ar, u32 param_id,
 			      u32 param_value, u8 pdev_id)
 {
diff --git a/drivers/net/wireless/ath/ath11k/wmi.h b/drivers/net/wireless/ath/ath11k/wmi.h
index 5a32ba0..e3a5751 100644
--- a/drivers/net/wireless/ath/ath11k/wmi.h
+++ b/drivers/net/wireless/ath/ath11k/wmi.h
@@ -2901,6 +2901,12 @@ struct set_fwtest_params {
 	u32 value;
 };
 
+struct wmi_pdev_ap_ps_cmd {
+	u32 tlv_header;
+	u32 pdev_id;
+	u32 param_value;
+} __packed;
+
 struct wmi_fwtest_set_param_cmd_param {
 	u32 tlv_header;
 	u32 param_id;
@@ -5121,4 +5127,5 @@ int ath11k_wmi_vdev_spectral_enable(struct ath11k *ar, u32 vdev_id,
 				    u32 trigger, u32 enable);
 int ath11k_wmi_vdev_spectral_conf(struct ath11k *ar,
 				  struct ath11k_wmi_vdev_spectral_conf_param *param);
+int ath11k_wmi_pdev_ap_ps_cmd_send(struct ath11k *ar, u8 pdev_id, u32 value);
 #endif
-- 
2.7.4

[PATCHv2 2/2] ath11k: Add ap power save support

[Venkateswara Naralasetty]

AP power save where AP goes to power save mode when no stations associate
to it and come out of power save when any station associate to AP.

This AP power save capability can be used to save power with the drawback
of reduced range or delayed discovery of the AP

This patch also porvides user configuration to enable/disable
this feature using vendor command. This feature is disabled by default.

Tested-on: IPQ8074 WLAN.HK.2.1.0.1-01228-QCAHKSWPL_SILICONZ-1

Signed-off-by: Venkateswara Naralasetty <vnaralas@codeaurora.org>
---
 drivers/net/wireless/ath/ath11k/Makefile |  3 +-
 drivers/net/wireless/ath/ath11k/core.h   |  8 ++++
 drivers/net/wireless/ath/ath11k/mac.c    | 44 +++++++++++++++++
 drivers/net/wireless/ath/ath11k/mac.h    |  1 +
 drivers/net/wireless/ath/ath11k/vendor.c | 81 ++++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath11k/vendor.h | 12 +++++
 drivers/net/wireless/ath/ath11k/wmi.c    | 32 +++++++++++++
 drivers/net/wireless/ath/ath11k/wmi.h    |  7 +++
 8 files changed, 187 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/wireless/ath/ath11k/vendor.c
 create mode 100644 drivers/net/wireless/ath/ath11k/vendor.h

diff --git a/drivers/net/wireless/ath/ath11k/Makefile b/drivers/net/wireless/ath/ath11k/Makefile
index bc4911f..f0b4975 100644
--- a/drivers/net/wireless/ath/ath11k/Makefile
+++ b/drivers/net/wireless/ath/ath11k/Makefile
@@ -16,7 +16,8 @@ ath11k-y += core.o \
 	    ce.o \
 	    peer.o \
 	    dbring.o \
-	    hw.o
+	    hw.o \
+	    vendor.o
 
 ath11k-$(CONFIG_ATH11K_DEBUGFS) += debug_htt_stats.o debugfs_sta.o
 ath11k-$(CONFIG_NL80211_TESTMODE) += testmode.o
diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h
index d21191c..827020f 100644
--- a/drivers/net/wireless/ath/ath11k/core.h
+++ b/drivers/net/wireless/ath/ath11k/core.h
@@ -23,6 +23,7 @@
 #include "thermal.h"
 #include "dbring.h"
 #include "spectral.h"
+#include "vendor.h"
 
 #define SM(_v, _f) (((_v) << _f##_LSB) & _f##_MASK)
 
@@ -419,6 +420,11 @@ struct ath11k_vdev_stop_status {
 	u32  vdev_id;
 };
 
+enum ath11k_ap_ps_state {
+	ATH11K_AP_PS_STATE_OFF,
+	ATH11K_AP_PS_STATE_ON,
+};
+
 struct ath11k {
 	struct ath11k_base *ab;
 	struct ath11k_pdev *pdev;
@@ -543,6 +549,8 @@ struct ath11k {
 #endif
 	bool dfs_block_radar_events;
 	struct ath11k_thermal thermal;
+	int ap_ps_enabled;
+	enum ath11k_ap_ps_state ap_ps_state;
 };
 
 struct ath11k_band_cap {
diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c
index f4a085b..1885834 100644
--- a/drivers/net/wireless/ath/ath11k/mac.c
+++ b/drivers/net/wireless/ath/ath11k/mac.c
@@ -2898,6 +2898,33 @@ static void ath11k_mac_dec_num_stations(struct ath11k_vif *arvif,
 	ar->num_stations--;
 }
 
+int ath11k_mac_ap_ps_recalc(struct ath11k *ar)
+{
+	struct ath11k_vif *arvif;
+	bool has_sta_iface = false;
+	enum ath11k_ap_ps_state state = ATH11K_AP_PS_STATE_OFF;
+	int ret = 0;
+
+	list_for_each_entry(arvif, &ar->arvifs, list) {
+		if (arvif->vdev_type == WMI_VDEV_TYPE_STA) {
+			has_sta_iface = true;
+			break;
+		}
+	}
+
+	if (!has_sta_iface && !ar->num_stations && ar->ap_ps_enabled)
+		state = ATH11K_AP_PS_STATE_ON;
+
+	if (ar->ap_ps_state == state)
+		return ret;
+
+	ret = ath11k_wmi_pdev_ap_ps_cmd_send(ar, ar->pdev->pdev_id, state);
+	if (!ret)
+		ar->ap_ps_state = state;
+
+	return ret;
+}
+
 static int ath11k_mac_station_add(struct ath11k *ar,
 				  struct ieee80211_vif *vif,
 				  struct ieee80211_sta *sta)
@@ -2937,6 +2964,12 @@ static int ath11k_mac_station_add(struct ath11k *ar,
 	ath11k_dbg(ab, ATH11K_DBG_MAC, "Added peer: %pM for VDEV: %d\n",
 		   sta->addr, arvif->vdev_id);
 
+	ret = ath11k_mac_ap_ps_recalc(ar);
+	if (ret) {
+		ath11k_warn(ar->ab, "failed to send ap ps ret %d\n", ret);
+		goto exit;
+	}
+
 	if (ath11k_debug_is_extd_tx_stats_enabled(ar)) {
 		arsta->tx_stats = kzalloc(sizeof(*arsta->tx_stats), GFP_KERNEL);
 		if (!arsta->tx_stats) {
@@ -3046,6 +3079,10 @@ static int ath11k_mac_op_sta_state(struct ieee80211_hw *hw,
 
 		kfree(arsta->rx_stats);
 		arsta->rx_stats = NULL;
+
+		ret = ath11k_mac_ap_ps_recalc(ar);
+		if (ret)
+			ath11k_warn(ar->ab, "failed to send ap ps ret %d\n", ret);
 	} else if (old_state == IEEE80211_STA_AUTH &&
 		   new_state == IEEE80211_STA_ASSOC &&
 		   (vif->type == NL80211_IFTYPE_AP ||
@@ -4210,6 +4247,7 @@ static void ath11k_mac_op_stop(struct ieee80211_hw *hw)
 
 	clear_bit(ATH11K_CAC_RUNNING, &ar->dev_flags);
 	ar->state = ATH11K_STATE_OFF;
+	ar->ap_ps_state = ATH11K_AP_PS_STATE_OFF;
 	mutex_unlock(&ar->conf_mutex);
 
 	cancel_delayed_work_sync(&ar->scan.timeout);
@@ -4533,6 +4571,10 @@ static int ath11k_mac_op_add_interface(struct ieee80211_hw *hw,
 
 	ath11k_dp_vdev_tx_attach(ar, arvif);
 
+	ret = ath11k_mac_ap_ps_recalc(ar);
+	if (ret)
+		ath11k_warn(ar->ab, "failed to set ap ps ret %d\n", ret);
+
 	mutex_unlock(&ar->conf_mutex);
 
 	return 0;
@@ -4619,6 +4661,7 @@ static void ath11k_mac_op_remove_interface(struct ieee80211_hw *hw,
 
 	/* Recalc txpower for remaining vdev */
 	ath11k_mac_txpower_recalc(ar);
+	ath11k_mac_ap_ps_recalc(ar);
 	clear_bit(ATH11K_FLAG_MONITOR_ENABLED, &ar->monitor_flags);
 
 	/* TODO: recal traffic pause state based on the available vdevs */
@@ -6175,6 +6218,7 @@ static int __ath11k_mac_register(struct ath11k *ar)
 		ARRAY_SIZE(ath11k_iftypes_ext_capa);
 
 	ath11k_reg_init(ar);
+	ath11k_vendor_register(ar);
 
 	/* advertise HW checksum offload capabilities */
 	ar->hw->netdev_features = NETIF_F_HW_CSUM;
diff --git a/drivers/net/wireless/ath/ath11k/mac.h b/drivers/net/wireless/ath/ath11k/mac.h
index 0607479..18d2f28 100644
--- a/drivers/net/wireless/ath/ath11k/mac.h
+++ b/drivers/net/wireless/ath/ath11k/mac.h
@@ -146,4 +146,5 @@ int ath11k_mac_tx_mgmt_pending_free(int buf_id, void *skb, void *ctx);
 u8 ath11k_mac_bw_to_mac80211_bw(u8 bw);
 enum ath11k_supported_bw ath11k_mac_mac80211_bw_to_ath11k_bw(enum rate_info_bw bw);
 enum hal_encrypt_type ath11k_dp_tx_get_encrypt_type(u32 cipher);
+int ath11k_mac_ap_ps_recalc(struct ath11k *ar);
 #endif
diff --git a/drivers/net/wireless/ath/ath11k/vendor.c b/drivers/net/wireless/ath/ath11k/vendor.c
new file mode 100644
index 0000000..b28cc65
--- /dev/null
+++ b/drivers/net/wireless/ath/ath11k/vendor.c
@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: BSD-3-Clause-Clear
+/*
+ * Copyright (c) 2019-2020 The Linux Foundation. All rights reserved.
+ */
+
+#include <net/netlink.h>
+#include <net/mac80211.h>
+#include <uapi/nl80211-vnd-qca.h>
+#include "core.h"
+#include "debug.h"
+
+static const struct nla_policy
+ath11k_vendor_set_wifi_config_policy[QCA_WLAN_VENDOR_ATTR_CONFIG_MAX + 1] = {
+	[QCA_WLAN_VENDOR_ATTR_CONFIG_GTX] = {.type = NLA_FLAG}
+};
+
+static int ath11k_vendor_set_wifi_config(struct wiphy *wihpy,
+					 struct wireless_dev *wdev,
+					 const void *data,
+					 int data_len)
+{
+	struct ieee80211_vif *vif;
+	struct ath11k_vif *arvif;
+	struct ath11k *ar;
+	struct nlattr *tb[QCA_WLAN_VENDOR_ATTR_CONFIG_MAX + 1];
+	int ret = 0;
+
+	if (!wdev)
+		return -EINVAL;
+
+	vif = wdev_to_ieee80211_vif(wdev);
+	if (!vif)
+		return -EINVAL;
+
+	arvif = (struct ath11k_vif *)vif->drv_priv;
+	if (!arvif)
+		return -EINVAL;
+
+	ar = arvif->ar;
+
+	mutex_lock(&ar->conf_mutex);
+
+	ret = nla_parse(tb, QCA_WLAN_VENDOR_ATTR_CONFIG_MAX, data, data_len,
+			ath11k_vendor_set_wifi_config_policy, NULL);
+	if (ret) {
+		ath11k_warn(ar->ab, "invalid set wifi config policy attribute\n");
+		goto exit;
+	}
+
+	ar->ap_ps_enabled = nla_get_flag(tb[QCA_WLAN_VENDOR_ATTR_CONFIG_GTX]);
+	ret = ath11k_mac_ap_ps_recalc(ar);
+	if (ret) {
+		ath11k_warn(ar->ab, "failed to send ap ps ret %d\n", ret);
+		goto exit;
+	}
+
+exit:
+	mutex_unlock(&ar->conf_mutex);
+	return ret;
+}
+
+static struct wiphy_vendor_command ath11k_vendor_commands[] = {
+	{
+		.info.vendor_id = QCA_NL80211_VENDOR_ID,
+		.info.subcmd = QCA_NL80211_VENDOR_SUBCMD_SET_WIFI_CONFIGURATION,
+		.flags = WIPHY_VENDOR_CMD_NEED_WDEV |
+			WIPHY_VENDOR_CMD_NEED_RUNNING,
+		.doit = ath11k_vendor_set_wifi_config,
+		.policy = ath11k_vendor_set_wifi_config_policy,
+		.maxattr = QCA_WLAN_VENDOR_ATTR_CONFIG_MAX
+	}
+};
+
+int ath11k_vendor_register(struct ath11k *ar)
+{
+	ar->hw->wiphy->vendor_commands = ath11k_vendor_commands;
+	ar->hw->wiphy->n_vendor_commands = ARRAY_SIZE(ath11k_vendor_commands);
+
+	return 0;
+}
+
diff --git a/drivers/net/wireless/ath/ath11k/vendor.h b/drivers/net/wireless/ath/ath11k/vendor.h
new file mode 100644
index 0000000..6eaf07e
--- /dev/null
+++ b/drivers/net/wireless/ath/ath11k/vendor.h
@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: BSD-3-Clause-Clear
+/*
+ * Copyright (c) 2019-2020 The Linux Foundation. All rights reserved.
+ */
+
+#ifndef ATH11K_VENDOR_H
+#define ATH11K_VENDOR_H
+
+int ath11k_vendor_register(struct ath11k *ar);
+
+#endif /* QCA_VENDOR_H */
+
diff --git a/drivers/net/wireless/ath/ath11k/wmi.c b/drivers/net/wireless/ath/ath11k/wmi.c
index a66576f..9aa1718 100644
--- a/drivers/net/wireless/ath/ath11k/wmi.c
+++ b/drivers/net/wireless/ath/ath11k/wmi.c
@@ -1188,6 +1188,38 @@ ath11k_wmi_rx_reord_queue_remove(struct ath11k *ar,
 	return ret;
 }
 
+int ath11k_wmi_pdev_ap_ps_cmd_send(struct ath11k *ar, u8 pdev_id,
+				   u32 param_value)
+{
+	struct ath11k_pdev_wmi *wmi = ar->wmi;
+	struct wmi_pdev_ap_ps_cmd *cmd;
+	struct sk_buff *skb;
+	int ret;
+
+	skb = ath11k_wmi_alloc_skb(wmi->wmi_ab, sizeof(*cmd));
+	if (!skb)
+		return -ENOMEM;
+
+	cmd = (struct wmi_pdev_ap_ps_cmd *)skb->data;
+	cmd->tlv_header = FIELD_PREP(WMI_TLV_TAG,
+				     WMI_TAG_PDEV_GREEN_AP_PS_ENABLE_CMD) |
+			  FIELD_PREP(WMI_TLV_LEN, sizeof(*cmd) - TLV_HDR_SIZE);
+	cmd->pdev_id = pdev_id;
+	cmd->param_value = param_value;
+
+	ret = ath11k_wmi_cmd_send(wmi, skb, WMI_PDEV_GREEN_AP_PS_ENABLE_CMDID);
+	if (ret) {
+		ath11k_warn(ar->ab, "failed to send ap ps enable/disable cmd\n");
+		dev_kfree_skb(skb);
+	}
+
+	ath11k_dbg(ar->ab, ATH11K_DBG_WMI,
+		   "wmi pdev ap ps set pdev id %d value %d\n",
+		   pdev_id, param_value);
+
+	return ret;
+}
+
 int ath11k_wmi_pdev_set_param(struct ath11k *ar, u32 param_id,
 			      u32 param_value, u8 pdev_id)
 {
diff --git a/drivers/net/wireless/ath/ath11k/wmi.h b/drivers/net/wireless/ath/ath11k/wmi.h
index 5a32ba0..e3a5751 100644
--- a/drivers/net/wireless/ath/ath11k/wmi.h
+++ b/drivers/net/wireless/ath/ath11k/wmi.h
@@ -2901,6 +2901,12 @@ struct set_fwtest_params {
 	u32 value;
 };
 
+struct wmi_pdev_ap_ps_cmd {
+	u32 tlv_header;
+	u32 pdev_id;
+	u32 param_value;
+} __packed;
+
 struct wmi_fwtest_set_param_cmd_param {
 	u32 tlv_header;
 	u32 param_id;
@@ -5121,4 +5127,5 @@ int ath11k_wmi_vdev_spectral_enable(struct ath11k *ar, u32 vdev_id,
 				    u32 trigger, u32 enable);
 int ath11k_wmi_vdev_spectral_conf(struct ath11k *ar,
 				  struct ath11k_wmi_vdev_spectral_conf_param *param);
+int ath11k_wmi_pdev_ap_ps_cmd_send(struct ath11k *ar, u8 pdev_id, u32 value);
 #endif
-- 
2.7.4


-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[Johannes Berg]

On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
> AP power save feature is to save power in AP mode, where AP goes
> to power save mode when no stations associate to it and comes out
> of power save when any station associate to AP.

Why do you think this requires a vendor command? I mean, that seems like
fairly reasonable - even by default - behaviour? And if not done by
default, it could possibly even be configured via the normal powersave
mode/command we already have in nl80211, or by a new normal one?

Why does it even require an nl80211 setting, seems like something you'd
really only want to turn off for debugging (e.g. via debugfs)?

johannes

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[Johannes Berg]

On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
> AP power save feature is to save power in AP mode, where AP goes
> to power save mode when no stations associate to it and comes out
> of power save when any station associate to AP.

Why do you think this requires a vendor command? I mean, that seems like
fairly reasonable - even by default - behaviour? And if not done by
default, it could possibly even be configured via the normal powersave
mode/command we already have in nl80211, or by a new normal one?

Why does it even require an nl80211 setting, seems like something you'd
really only want to turn off for debugging (e.g. via debugfs)?

johannes


-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>> AP power save feature is to save power in AP mode, where AP goes
>> to power save mode when no stations associate to it and comes out
>> of power save when any station associate to AP.
>
> Why do you think this requires a vendor command? I mean, that seems like
> fairly reasonable - even by default - behaviour?

I have not studied the details, but doesn't AP power save break normal
functionality? For example, I would guess probe requests from clients
would be lost. So there's a major drawback when enabling this, right?

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

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

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>> AP power save feature is to save power in AP mode, where AP goes
>> to power save mode when no stations associate to it and comes out
>> of power save when any station associate to AP.
>
> Why do you think this requires a vendor command? I mean, that seems like
> fairly reasonable - even by default - behaviour?

I have not studied the details, but doesn't AP power save break normal
functionality? For example, I would guess probe requests from clients
would be lost. So there's a major drawback when enabling this, right?

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

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[Johannes Berg]

On Tue, 2020-09-29 at 10:40 +0300, Kalle Valo wrote:
> Johannes Berg <johannes@sipsolutions.net> writes:
> 
> > On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
> > > AP power save feature is to save power in AP mode, where AP goes
> > > to power save mode when no stations associate to it and comes out
> > > of power save when any station associate to AP.
> > 
> > Why do you think this requires a vendor command? I mean, that seems like
> > fairly reasonable - even by default - behaviour?
> 
> I have not studied the details, but doesn't AP power save break normal
> functionality? For example, I would guess probe requests from clients
> would be lost. So there's a major drawback when enabling this, right?

Not the way it's described above?

johannes

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[Johannes Berg]

On Tue, 2020-09-29 at 10:40 +0300, Kalle Valo wrote:
> Johannes Berg <johannes@sipsolutions.net> writes:
> 
> > On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
> > > AP power save feature is to save power in AP mode, where AP goes
> > > to power save mode when no stations associate to it and comes out
> > > of power save when any station associate to AP.
> > 
> > Why do you think this requires a vendor command? I mean, that seems like
> > fairly reasonable - even by default - behaviour?
> 
> I have not studied the details, but doesn't AP power save break normal
> functionality? For example, I would guess probe requests from clients
> would be lost. So there's a major drawback when enabling this, right?

Not the way it's described above?

johannes


-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[vnaralas]

On 2020-09-29 13:10, Kalle Valo wrote:
> Johannes Berg <johannes@sipsolutions.net> writes:
> 
>> On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>>> AP power save feature is to save power in AP mode, where AP goes
>>> to power save mode when no stations associate to it and comes out
>>> of power save when any station associate to AP.
>> 
>> Why do you think this requires a vendor command? I mean, that seems 
>> like
>> fairly reasonable - even by default - behaviour?
> 
> I have not studied the details, but doesn't AP power save break normal
> functionality? For example, I would guess probe requests from clients
> would be lost. So there's a major drawback when enabling this, right?

This AP power save feature will not break any functionality, Since one 
chain is always active and all other chains will be disabled when this 
feature is enabled. AP can still be able to beacon and receive probe 
request from the clients. The only drawback is reduced network range 
when this feature is enabled. Hence, we don't want to enable it by 
default.

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[vnaralas]

On 2020-09-29 13:10, Kalle Valo wrote:
> Johannes Berg <johannes@sipsolutions.net> writes:
> 
>> On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>>> AP power save feature is to save power in AP mode, where AP goes
>>> to power save mode when no stations associate to it and comes out
>>> of power save when any station associate to AP.
>> 
>> Why do you think this requires a vendor command? I mean, that seems 
>> like
>> fairly reasonable - even by default - behaviour?
> 
> I have not studied the details, but doesn't AP power save break normal
> functionality? For example, I would guess probe requests from clients
> would be lost. So there's a major drawback when enabling this, right?

This AP power save feature will not break any functionality, Since one 
chain is always active and all other chains will be disabled when this 
feature is enabled. AP can still be able to beacon and receive probe 
request from the clients. The only drawback is reduced network range 
when this feature is enabled. Hence, we don't want to enable it by 
default.

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[Kalle Valo]

vnaralas@codeaurora.org writes:

> On 2020-09-29 13:10, Kalle Valo wrote:
>> Johannes Berg <johannes@sipsolutions.net> writes:
>>
>>> On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>>>> AP power save feature is to save power in AP mode, where AP goes
>>>> to power save mode when no stations associate to it and comes out
>>>> of power save when any station associate to AP.
>>>
>>> Why do you think this requires a vendor command? I mean, that seems
>>> like
>>> fairly reasonable - even by default - behaviour?
>>
>> I have not studied the details, but doesn't AP power save break normal
>> functionality? For example, I would guess probe requests from clients
>> would be lost. So there's a major drawback when enabling this, right?
>
> This AP power save feature will not break any functionality, Since one
> chain is always active and all other chains will be disabled when this
> feature is enabled. AP can still be able to beacon and receive probe
> request from the clients. The only drawback is reduced network range
> when this feature is enabled. Hence, we don't want to enable it by
> default.

Yeah, we really would not want to enable that by default. But what
should be the path forward, a vendor command or a proper nl80211
command? Any opinions?

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

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

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[Kalle Valo]

vnaralas@codeaurora.org writes:

> On 2020-09-29 13:10, Kalle Valo wrote:
>> Johannes Berg <johannes@sipsolutions.net> writes:
>>
>>> On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>>>> AP power save feature is to save power in AP mode, where AP goes
>>>> to power save mode when no stations associate to it and comes out
>>>> of power save when any station associate to AP.
>>>
>>> Why do you think this requires a vendor command? I mean, that seems
>>> like
>>> fairly reasonable - even by default - behaviour?
>>
>> I have not studied the details, but doesn't AP power save break normal
>> functionality? For example, I would guess probe requests from clients
>> would be lost. So there's a major drawback when enabling this, right?
>
> This AP power save feature will not break any functionality, Since one
> chain is always active and all other chains will be disabled when this
> feature is enabled. AP can still be able to beacon and receive probe
> request from the clients. The only drawback is reduced network range
> when this feature is enabled. Hence, we don't want to enable it by
> default.

Yeah, we really would not want to enable that by default. But what
should be the path forward, a vendor command or a proper nl80211
command? Any opinions?

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

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1539 bytes --]

On 10/21/2020 7:19 PM, Kalle Valo wrote:
> vnaralas@codeaurora.org writes:
> 
>> On 2020-09-29 13:10, Kalle Valo wrote:
>>> Johannes Berg <johannes@sipsolutions.net> writes:
>>>
>>>> On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>>>>> AP power save feature is to save power in AP mode, where AP goes
>>>>> to power save mode when no stations associate to it and comes out
>>>>> of power save when any station associate to AP.
>>>>
>>>> Why do you think this requires a vendor command? I mean, that seems
>>>> like
>>>> fairly reasonable - even by default - behaviour?
>>>
>>> I have not studied the details, but doesn't AP power save break normal
>>> functionality? For example, I would guess probe requests from clients
>>> would be lost. So there's a major drawback when enabling this, right?
>>
>> This AP power save feature will not break any functionality, Since one
>> chain is always active and all other chains will be disabled when this
>> feature is enabled. AP can still be able to beacon and receive probe
>> request from the clients. The only drawback is reduced network range
>> when this feature is enabled. Hence, we don't want to enable it by
>> default.
> 
> Yeah, we really would not want to enable that by default. But what
> should be the path forward, a vendor command or a proper nl80211
> command? Any opinions?

I would go for a proper nl80211 command or just add an attribute for use 
in NL80211_CMD_START_AP or deal with NL80211_CMD_SET_POWERSAVE when 
operating in AP mode.

Regards,
Arend

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4176 bytes --]

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[Arend Van Spriel]

[-- Attachment #1.1: Type: text/plain, Size: 1539 bytes --]

On 10/21/2020 7:19 PM, Kalle Valo wrote:
> vnaralas@codeaurora.org writes:
> 
>> On 2020-09-29 13:10, Kalle Valo wrote:
>>> Johannes Berg <johannes@sipsolutions.net> writes:
>>>
>>>> On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>>>>> AP power save feature is to save power in AP mode, where AP goes
>>>>> to power save mode when no stations associate to it and comes out
>>>>> of power save when any station associate to AP.
>>>>
>>>> Why do you think this requires a vendor command? I mean, that seems
>>>> like
>>>> fairly reasonable - even by default - behaviour?
>>>
>>> I have not studied the details, but doesn't AP power save break normal
>>> functionality? For example, I would guess probe requests from clients
>>> would be lost. So there's a major drawback when enabling this, right?
>>
>> This AP power save feature will not break any functionality, Since one
>> chain is always active and all other chains will be disabled when this
>> feature is enabled. AP can still be able to beacon and receive probe
>> request from the clients. The only drawback is reduced network range
>> when this feature is enabled. Hence, we don't want to enable it by
>> default.
> 
> Yeah, we really would not want to enable that by default. But what
> should be the path forward, a vendor command or a proper nl80211
> command? Any opinions?

I would go for a proper nl80211 command or just add an attribute for use 
in NL80211_CMD_START_AP or deal with NL80211_CMD_SET_POWERSAVE when 
operating in AP mode.

Regards,
Arend

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4176 bytes --]

[-- Attachment #2: Type: text/plain, Size: 102 bytes --]

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1761 bytes --]

On 10/22/2020 10:00 AM, Arend Van Spriel wrote:
> On 10/21/2020 7:19 PM, Kalle Valo wrote:
>> vnaralas@codeaurora.org writes:
>>
>>> On 2020-09-29 13:10, Kalle Valo wrote:
>>>> Johannes Berg <johannes@sipsolutions.net> writes:
>>>>
>>>>> On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>>>>>> AP power save feature is to save power in AP mode, where AP goes
>>>>>> to power save mode when no stations associate to it and comes out
>>>>>> of power save when any station associate to AP.
>>>>>
>>>>> Why do you think this requires a vendor command? I mean, that seems
>>>>> like
>>>>> fairly reasonable - even by default - behaviour?
>>>>
>>>> I have not studied the details, but doesn't AP power save break normal
>>>> functionality? For example, I would guess probe requests from clients
>>>> would be lost. So there's a major drawback when enabling this, right?
>>>
>>> This AP power save feature will not break any functionality, Since one
>>> chain is always active and all other chains will be disabled when this
>>> feature is enabled. AP can still be able to beacon and receive probe
>>> request from the clients. The only drawback is reduced network range
>>> when this feature is enabled. Hence, we don't want to enable it by
>>> default.
>>
>> Yeah, we really would not want to enable that by default. But what
>> should be the path forward, a vendor command or a proper nl80211
>> command? Any opinions?
> 
> I would go for a proper nl80211 command or just add an attribute for use 
> in NL80211_CMD_START_AP or deal with NL80211_CMD_SET_POWERSAVE when 
> operating in AP mode.

At first I though this functionality was same as SMPS which is in the 
802.11 spec, but reading up on it that seems to be a STA feature.

Regards,
Arend

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4176 bytes --]

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[Arend Van Spriel]

[-- Attachment #1.1: Type: text/plain, Size: 1761 bytes --]

On 10/22/2020 10:00 AM, Arend Van Spriel wrote:
> On 10/21/2020 7:19 PM, Kalle Valo wrote:
>> vnaralas@codeaurora.org writes:
>>
>>> On 2020-09-29 13:10, Kalle Valo wrote:
>>>> Johannes Berg <johannes@sipsolutions.net> writes:
>>>>
>>>>> On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>>>>>> AP power save feature is to save power in AP mode, where AP goes
>>>>>> to power save mode when no stations associate to it and comes out
>>>>>> of power save when any station associate to AP.
>>>>>
>>>>> Why do you think this requires a vendor command? I mean, that seems
>>>>> like
>>>>> fairly reasonable - even by default - behaviour?
>>>>
>>>> I have not studied the details, but doesn't AP power save break normal
>>>> functionality? For example, I would guess probe requests from clients
>>>> would be lost. So there's a major drawback when enabling this, right?
>>>
>>> This AP power save feature will not break any functionality, Since one
>>> chain is always active and all other chains will be disabled when this
>>> feature is enabled. AP can still be able to beacon and receive probe
>>> request from the clients. The only drawback is reduced network range
>>> when this feature is enabled. Hence, we don't want to enable it by
>>> default.
>>
>> Yeah, we really would not want to enable that by default. But what
>> should be the path forward, a vendor command or a proper nl80211
>> command? Any opinions?
> 
> I would go for a proper nl80211 command or just add an attribute for use 
> in NL80211_CMD_START_AP or deal with NL80211_CMD_SET_POWERSAVE when 
> operating in AP mode.

At first I though this functionality was same as SMPS which is in the 
802.11 spec, but reading up on it that seems to be a STA feature.

Regards,
Arend

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4176 bytes --]

[-- Attachment #2: Type: text/plain, Size: 102 bytes --]

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[vnaralas]

On 2020-10-22 13:30, Arend Van Spriel wrote:
> On 10/21/2020 7:19 PM, Kalle Valo wrote:
>> vnaralas@codeaurora.org writes:
>> 
>>> On 2020-09-29 13:10, Kalle Valo wrote:
>>>> Johannes Berg <johannes@sipsolutions.net> writes:
>>>> 
>>>>> On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>>>>>> AP power save feature is to save power in AP mode, where AP goes
>>>>>> to power save mode when no stations associate to it and comes out
>>>>>> of power save when any station associate to AP.
>>>>> 
>>>>> Why do you think this requires a vendor command? I mean, that seems
>>>>> like
>>>>> fairly reasonable - even by default - behaviour?
>>>> 
>>>> I have not studied the details, but doesn't AP power save break 
>>>> normal
>>>> functionality? For example, I would guess probe requests from 
>>>> clients
>>>> would be lost. So there's a major drawback when enabling this, 
>>>> right?
>>> 
>>> This AP power save feature will not break any functionality, Since 
>>> one
>>> chain is always active and all other chains will be disabled when 
>>> this
>>> feature is enabled. AP can still be able to beacon and receive probe
>>> request from the clients. The only drawback is reduced network range
>>> when this feature is enabled. Hence, we don't want to enable it by
>>> default.
>> 
>> Yeah, we really would not want to enable that by default. But what
>> should be the path forward, a vendor command or a proper nl80211
>> command? Any opinions?
> 
> I would go for a proper nl80211 command or just add an attribute for
> use in NL80211_CMD_START_AP or deal with NL80211_CMD_SET_POWERSAVE
> when operating in AP mode.
> 
Sure, I will go with the existing NL80211_CMD_SET_POWERSAVE and I will 
send next version.

> Regards,
> Arend

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[vnaralas]

On 2020-10-22 13:30, Arend Van Spriel wrote:
> On 10/21/2020 7:19 PM, Kalle Valo wrote:
>> vnaralas@codeaurora.org writes:
>> 
>>> On 2020-09-29 13:10, Kalle Valo wrote:
>>>> Johannes Berg <johannes@sipsolutions.net> writes:
>>>> 
>>>>> On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>>>>>> AP power save feature is to save power in AP mode, where AP goes
>>>>>> to power save mode when no stations associate to it and comes out
>>>>>> of power save when any station associate to AP.
>>>>> 
>>>>> Why do you think this requires a vendor command? I mean, that seems
>>>>> like
>>>>> fairly reasonable - even by default - behaviour?
>>>> 
>>>> I have not studied the details, but doesn't AP power save break 
>>>> normal
>>>> functionality? For example, I would guess probe requests from 
>>>> clients
>>>> would be lost. So there's a major drawback when enabling this, 
>>>> right?
>>> 
>>> This AP power save feature will not break any functionality, Since 
>>> one
>>> chain is always active and all other chains will be disabled when 
>>> this
>>> feature is enabled. AP can still be able to beacon and receive probe
>>> request from the clients. The only drawback is reduced network range
>>> when this feature is enabled. Hence, we don't want to enable it by
>>> default.
>> 
>> Yeah, we really would not want to enable that by default. But what
>> should be the path forward, a vendor command or a proper nl80211
>> command? Any opinions?
> 
> I would go for a proper nl80211 command or just add an attribute for
> use in NL80211_CMD_START_AP or deal with NL80211_CMD_SET_POWERSAVE
> when operating in AP mode.
> 
Sure, I will go with the existing NL80211_CMD_SET_POWERSAVE and I will 
send next version.

> Regards,
> Arend

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[Kalle Valo]

vnaralas@codeaurora.org writes:

> On 2020-10-22 13:30, Arend Van Spriel wrote:
>> On 10/21/2020 7:19 PM, Kalle Valo wrote:
>>> vnaralas@codeaurora.org writes:
>>>
>>>> On 2020-09-29 13:10, Kalle Valo wrote:
>>>>> Johannes Berg <johannes@sipsolutions.net> writes:
>>>>>
>>>>>> On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>>>>>>> AP power save feature is to save power in AP mode, where AP goes
>>>>>>> to power save mode when no stations associate to it and comes out
>>>>>>> of power save when any station associate to AP.
>>>>>>
>>>>>> Why do you think this requires a vendor command? I mean, that seems
>>>>>> like
>>>>>> fairly reasonable - even by default - behaviour?
>>>>>
>>>>> I have not studied the details, but doesn't AP power save break
>>>>> normal
>>>>> functionality? For example, I would guess probe requests from
>>>>> clients
>>>>> would be lost. So there's a major drawback when enabling this,
>>>>> right?
>>>>
>>>> This AP power save feature will not break any functionality, Since
>>>> one
>>>> chain is always active and all other chains will be disabled when
>>>> this
>>>> feature is enabled. AP can still be able to beacon and receive probe
>>>> request from the clients. The only drawback is reduced network range
>>>> when this feature is enabled. Hence, we don't want to enable it by
>>>> default.
>>>
>>> Yeah, we really would not want to enable that by default. But what
>>> should be the path forward, a vendor command or a proper nl80211
>>> command? Any opinions?
>>
>> I would go for a proper nl80211 command or just add an attribute for
>> use in NL80211_CMD_START_AP or deal with NL80211_CMD_SET_POWERSAVE
>> when operating in AP mode.
>>
> Sure, I will go with the existing NL80211_CMD_SET_POWERSAVE and I will
> send next version.

Better to wait first so that we have concensus on this. And need to
check if NL80211_CMD_SET_POWERSAVE is even suitable for AP mode.

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

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

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[Kalle Valo]

vnaralas@codeaurora.org writes:

> On 2020-10-22 13:30, Arend Van Spriel wrote:
>> On 10/21/2020 7:19 PM, Kalle Valo wrote:
>>> vnaralas@codeaurora.org writes:
>>>
>>>> On 2020-09-29 13:10, Kalle Valo wrote:
>>>>> Johannes Berg <johannes@sipsolutions.net> writes:
>>>>>
>>>>>> On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>>>>>>> AP power save feature is to save power in AP mode, where AP goes
>>>>>>> to power save mode when no stations associate to it and comes out
>>>>>>> of power save when any station associate to AP.
>>>>>>
>>>>>> Why do you think this requires a vendor command? I mean, that seems
>>>>>> like
>>>>>> fairly reasonable - even by default - behaviour?
>>>>>
>>>>> I have not studied the details, but doesn't AP power save break
>>>>> normal
>>>>> functionality? For example, I would guess probe requests from
>>>>> clients
>>>>> would be lost. So there's a major drawback when enabling this,
>>>>> right?
>>>>
>>>> This AP power save feature will not break any functionality, Since
>>>> one
>>>> chain is always active and all other chains will be disabled when
>>>> this
>>>> feature is enabled. AP can still be able to beacon and receive probe
>>>> request from the clients. The only drawback is reduced network range
>>>> when this feature is enabled. Hence, we don't want to enable it by
>>>> default.
>>>
>>> Yeah, we really would not want to enable that by default. But what
>>> should be the path forward, a vendor command or a proper nl80211
>>> command? Any opinions?
>>
>> I would go for a proper nl80211 command or just add an attribute for
>> use in NL80211_CMD_START_AP or deal with NL80211_CMD_SET_POWERSAVE
>> when operating in AP mode.
>>
> Sure, I will go with the existing NL80211_CMD_SET_POWERSAVE and I will
> send next version.

Better to wait first so that we have concensus on this. And need to
check if NL80211_CMD_SET_POWERSAVE is even suitable for AP mode.

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

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[Johannes Berg]

On Mon, 2020-11-02 at 21:44 +0200, Kalle Valo wrote:
> vnaralas@codeaurora.org writes:
> 
> > On 2020-10-22 13:30, Arend Van Spriel wrote:
> > > On 10/21/2020 7:19 PM, Kalle Valo wrote:
> > > > vnaralas@codeaurora.org writes:
> > > > 
> > > > > On 2020-09-29 13:10, Kalle Valo wrote:
> > > > > > Johannes Berg <johannes@sipsolutions.net> writes:
> > > > > > 
> > > > > > > On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
> > > > > > > > AP power save feature is to save power in AP mode, where AP goes
> > > > > > > > to power save mode when no stations associate to it and comes out
> > > > > > > > of power save when any station associate to AP.
> > > > > > > 
> > > > > > > Why do you think this requires a vendor command? I mean, that seems
> > > > > > > like
> > > > > > > fairly reasonable - even by default - behaviour?
> > > > > > 
> > > > > > I have not studied the details, but doesn't AP power save break
> > > > > > normal
> > > > > > functionality? For example, I would guess probe requests from
> > > > > > clients
> > > > > > would be lost. So there's a major drawback when enabling this,
> > > > > > right?
> > > > > 
> > > > > This AP power save feature will not break any functionality, Since
> > > > > one
> > > > > chain is always active and all other chains will be disabled when
> > > > > this
> > > > > feature is enabled. AP can still be able to beacon and receive probe
> > > > > request from the clients. The only drawback is reduced network range
> > > > > when this feature is enabled. Hence, we don't want to enable it by
> > > > > default.
> > > > 
> > > > Yeah, we really would not want to enable that by default. But what
> > > > should be the path forward, a vendor command or a proper nl80211
> > > > command? Any opinions?
> > > 
> > > I would go for a proper nl80211 command or just add an attribute for
> > > use in NL80211_CMD_START_AP or deal with NL80211_CMD_SET_POWERSAVE
> > > when operating in AP mode.
> > > 
> > Sure, I will go with the existing NL80211_CMD_SET_POWERSAVE and I will
> > send next version.
> 
> Better to wait first so that we have concensus on this. And need to
> check if NL80211_CMD_SET_POWERSAVE is even suitable for AP mode.

I suspect that SET_POWERSAVE might be confusing.

Perhaps just with an attribute used in START_AP (and CHANGE_BEACON if
needed) would be sufficient?

johannes

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[Johannes Berg]

On Mon, 2020-11-02 at 21:44 +0200, Kalle Valo wrote:
> vnaralas@codeaurora.org writes:
> 
> > On 2020-10-22 13:30, Arend Van Spriel wrote:
> > > On 10/21/2020 7:19 PM, Kalle Valo wrote:
> > > > vnaralas@codeaurora.org writes:
> > > > 
> > > > > On 2020-09-29 13:10, Kalle Valo wrote:
> > > > > > Johannes Berg <johannes@sipsolutions.net> writes:
> > > > > > 
> > > > > > > On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
> > > > > > > > AP power save feature is to save power in AP mode, where AP goes
> > > > > > > > to power save mode when no stations associate to it and comes out
> > > > > > > > of power save when any station associate to AP.
> > > > > > > 
> > > > > > > Why do you think this requires a vendor command? I mean, that seems
> > > > > > > like
> > > > > > > fairly reasonable - even by default - behaviour?
> > > > > > 
> > > > > > I have not studied the details, but doesn't AP power save break
> > > > > > normal
> > > > > > functionality? For example, I would guess probe requests from
> > > > > > clients
> > > > > > would be lost. So there's a major drawback when enabling this,
> > > > > > right?
> > > > > 
> > > > > This AP power save feature will not break any functionality, Since
> > > > > one
> > > > > chain is always active and all other chains will be disabled when
> > > > > this
> > > > > feature is enabled. AP can still be able to beacon and receive probe
> > > > > request from the clients. The only drawback is reduced network range
> > > > > when this feature is enabled. Hence, we don't want to enable it by
> > > > > default.
> > > > 
> > > > Yeah, we really would not want to enable that by default. But what
> > > > should be the path forward, a vendor command or a proper nl80211
> > > > command? Any opinions?
> > > 
> > > I would go for a proper nl80211 command or just add an attribute for
> > > use in NL80211_CMD_START_AP or deal with NL80211_CMD_SET_POWERSAVE
> > > when operating in AP mode.
> > > 
> > Sure, I will go with the existing NL80211_CMD_SET_POWERSAVE and I will
> > send next version.
> 
> Better to wait first so that we have concensus on this. And need to
> check if NL80211_CMD_SET_POWERSAVE is even suitable for AP mode.

I suspect that SET_POWERSAVE might be confusing.

Perhaps just with an attribute used in START_AP (and CHANGE_BEACON if
needed) would be sufficient?

johannes


-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[vnaralas]

On 2020-11-06 16:11, Johannes Berg wrote:
> On Mon, 2020-11-02 at 21:44 +0200, Kalle Valo wrote:
>> vnaralas@codeaurora.org writes:
>> 
>> > On 2020-10-22 13:30, Arend Van Spriel wrote:
>> > > On 10/21/2020 7:19 PM, Kalle Valo wrote:
>> > > > vnaralas@codeaurora.org writes:
>> > > >
>> > > > > On 2020-09-29 13:10, Kalle Valo wrote:
>> > > > > > Johannes Berg <johannes@sipsolutions.net> writes:
>> > > > > >
>> > > > > > > On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>> > > > > > > > AP power save feature is to save power in AP mode, where AP goes
>> > > > > > > > to power save mode when no stations associate to it and comes out
>> > > > > > > > of power save when any station associate to AP.
>> > > > > > >
>> > > > > > > Why do you think this requires a vendor command? I mean, that seems
>> > > > > > > like
>> > > > > > > fairly reasonable - even by default - behaviour?
>> > > > > >
>> > > > > > I have not studied the details, but doesn't AP power save break
>> > > > > > normal
>> > > > > > functionality? For example, I would guess probe requests from
>> > > > > > clients
>> > > > > > would be lost. So there's a major drawback when enabling this,
>> > > > > > right?
>> > > > >
>> > > > > This AP power save feature will not break any functionality, Since
>> > > > > one
>> > > > > chain is always active and all other chains will be disabled when
>> > > > > this
>> > > > > feature is enabled. AP can still be able to beacon and receive probe
>> > > > > request from the clients. The only drawback is reduced network range
>> > > > > when this feature is enabled. Hence, we don't want to enable it by
>> > > > > default.
>> > > >
>> > > > Yeah, we really would not want to enable that by default. But what
>> > > > should be the path forward, a vendor command or a proper nl80211
>> > > > command? Any opinions?
>> > >
>> > > I would go for a proper nl80211 command or just add an attribute for
>> > > use in NL80211_CMD_START_AP or deal with NL80211_CMD_SET_POWERSAVE
>> > > when operating in AP mode.
>> > >
>> > Sure, I will go with the existing NL80211_CMD_SET_POWERSAVE and I will
>> > send next version.
>> 
>> Better to wait first so that we have concensus on this. And need to
>> check if NL80211_CMD_SET_POWERSAVE is even suitable for AP mode.
> 
> I suspect that SET_POWERSAVE might be confusing.
> 
> Perhaps just with an attribute used in START_AP (and CHANGE_BEACON if
> needed) would be sufficient?

Actually this ap power save configuration is applicable for per pdev. 
So, I don't think we can use START AP command here.
I would prefer to go with a new NL80211 command. Please share your 
thoughts on this.


> 
> johannes

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[vnaralas]

On 2020-11-06 16:11, Johannes Berg wrote:
> On Mon, 2020-11-02 at 21:44 +0200, Kalle Valo wrote:
>> vnaralas@codeaurora.org writes:
>> 
>> > On 2020-10-22 13:30, Arend Van Spriel wrote:
>> > > On 10/21/2020 7:19 PM, Kalle Valo wrote:
>> > > > vnaralas@codeaurora.org writes:
>> > > >
>> > > > > On 2020-09-29 13:10, Kalle Valo wrote:
>> > > > > > Johannes Berg <johannes@sipsolutions.net> writes:
>> > > > > >
>> > > > > > > On Mon, 2020-08-24 at 13:56 +0530, Venkateswara Naralasetty wrote:
>> > > > > > > > AP power save feature is to save power in AP mode, where AP goes
>> > > > > > > > to power save mode when no stations associate to it and comes out
>> > > > > > > > of power save when any station associate to AP.
>> > > > > > >
>> > > > > > > Why do you think this requires a vendor command? I mean, that seems
>> > > > > > > like
>> > > > > > > fairly reasonable - even by default - behaviour?
>> > > > > >
>> > > > > > I have not studied the details, but doesn't AP power save break
>> > > > > > normal
>> > > > > > functionality? For example, I would guess probe requests from
>> > > > > > clients
>> > > > > > would be lost. So there's a major drawback when enabling this,
>> > > > > > right?
>> > > > >
>> > > > > This AP power save feature will not break any functionality, Since
>> > > > > one
>> > > > > chain is always active and all other chains will be disabled when
>> > > > > this
>> > > > > feature is enabled. AP can still be able to beacon and receive probe
>> > > > > request from the clients. The only drawback is reduced network range
>> > > > > when this feature is enabled. Hence, we don't want to enable it by
>> > > > > default.
>> > > >
>> > > > Yeah, we really would not want to enable that by default. But what
>> > > > should be the path forward, a vendor command or a proper nl80211
>> > > > command? Any opinions?
>> > >
>> > > I would go for a proper nl80211 command or just add an attribute for
>> > > use in NL80211_CMD_START_AP or deal with NL80211_CMD_SET_POWERSAVE
>> > > when operating in AP mode.
>> > >
>> > Sure, I will go with the existing NL80211_CMD_SET_POWERSAVE and I will
>> > send next version.
>> 
>> Better to wait first so that we have concensus on this. And need to
>> check if NL80211_CMD_SET_POWERSAVE is even suitable for AP mode.
> 
> I suspect that SET_POWERSAVE might be confusing.
> 
> Perhaps just with an attribute used in START_AP (and CHANGE_BEACON if
> needed) would be sufficient?

Actually this ap power save configuration is applicable for per pdev. 
So, I don't think we can use START AP command here.
I would prefer to go with a new NL80211 command. Please share your 
thoughts on this.


> 
> johannes

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[Jouni Malinen]

On Fri, Nov 06, 2020 at 11:41:06AM +0100, Johannes Berg wrote:
> I suspect that SET_POWERSAVE might be confusing.

Why? Isn't the use case here very similar to the existing station mode
use of power save even if the power saving mechanism is more of a vendor
specific extension that applies while there are no associated stations?

> Perhaps just with an attribute used in START_AP (and CHANGE_BEACON if
> needed) would be sufficient?

NL80211_CMD_START_AP with a new attribute (or even re-use of
NL80211_ATTR_PS_STATE) might work for a case where this does not need to
be changed dynamically during the lifetime of the BSS.
NL80211_CMD_SET_BEACON (which maps to the change_beacon() callback)
feels like something that is currently limited to Beacon data updates
with its use of struct cfg80211_beacon_data instead of struct
cfg80211_ap_settings..

That SET_BEACON name is still from the old NEW/SET/DEL_BEACON time.
Should that be renamed to NL80211_CMD_UPDATE_AP if we extend this to
changes that are not really targeting the Beacon frame payload itself?
And should the cfg80211_beacon_data argument be replaced with
cfg80211_ap_settings? It looks like we already have some struct
cfg80211_ap_settings values like inactivity_timeout and beacon_rate (and
maybe some HE parameters?) that one might want to update during the
lifetime of the BSS..

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[Jouni Malinen]

On Fri, Nov 06, 2020 at 11:41:06AM +0100, Johannes Berg wrote:
> I suspect that SET_POWERSAVE might be confusing.

Why? Isn't the use case here very similar to the existing station mode
use of power save even if the power saving mechanism is more of a vendor
specific extension that applies while there are no associated stations?

> Perhaps just with an attribute used in START_AP (and CHANGE_BEACON if
> needed) would be sufficient?

NL80211_CMD_START_AP with a new attribute (or even re-use of
NL80211_ATTR_PS_STATE) might work for a case where this does not need to
be changed dynamically during the lifetime of the BSS.
NL80211_CMD_SET_BEACON (which maps to the change_beacon() callback)
feels like something that is currently limited to Beacon data updates
with its use of struct cfg80211_beacon_data instead of struct
cfg80211_ap_settings..

That SET_BEACON name is still from the old NEW/SET/DEL_BEACON time.
Should that be renamed to NL80211_CMD_UPDATE_AP if we extend this to
changes that are not really targeting the Beacon frame payload itself?
And should the cfg80211_beacon_data argument be replaced with
cfg80211_ap_settings? It looks like we already have some struct
cfg80211_ap_settings values like inactivity_timeout and beacon_rate (and
maybe some HE parameters?) that one might want to update during the
lifetime of the BSS..

-- 
Jouni Malinen                                            PGP id EFC895FA

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[Johannes Berg]

On Wed, 2020-12-23 at 14:46 +0200, Jouni Malinen wrote:
> On Fri, Nov 06, 2020 at 11:41:06AM +0100, Johannes Berg wrote:
> > I suspect that SET_POWERSAVE might be confusing.
> 
> Why? Isn't the use case here very similar to the existing station mode
> use of power save even if the power saving mechanism is more of a vendor
> specific extension that applies while there are no associated stations?

Yeah, true, fair point.

However, set-powersave is a bit of a legacy API with state in the
kernel, and sometimes restrictions on how/when you can set it etc. I'm
not sure it's a good idea for those reasons alone?

> > Perhaps just with an attribute used in START_AP (and CHANGE_BEACON if
> > needed) would be sufficient?
> 
> NL80211_CMD_START_AP with a new attribute (or even re-use of
> NL80211_ATTR_PS_STATE) might work for a case where this does not need to
> be changed dynamically during the lifetime of the BSS.
> NL80211_CMD_SET_BEACON (which maps to the change_beacon() callback)
> feels like something that is currently limited to Beacon data updates
> with its use of struct cfg80211_beacon_data instead of struct
> cfg80211_ap_settings..
> 
> That SET_BEACON name is still from the old NEW/SET/DEL_BEACON time.
> Should that be renamed to NL80211_CMD_UPDATE_AP if we extend this to
> changes that are not really targeting the Beacon frame payload itself?

I'd be surprised if we don't already have non-beacon state there ... but
it looks like only very little non-beacon state, namely the FTM
responder state.

Renaming seems reasonable, we've done it before with START_AP.

> And should the cfg80211_beacon_data argument be replaced with
> cfg80211_ap_settings? It looks like we already have some struct
> cfg80211_ap_settings values like inactivity_timeout and beacon_rate (and
> maybe some HE parameters?) that one might want to update during the
> lifetime of the BSS..

That also seems reasonable.

johannes

Re: [PATCHv2 1/2] nl80211: vendor-cmd: qca: add command for ap power save

[Johannes Berg]

On Wed, 2020-12-23 at 14:46 +0200, Jouni Malinen wrote:
> On Fri, Nov 06, 2020 at 11:41:06AM +0100, Johannes Berg wrote:
> > I suspect that SET_POWERSAVE might be confusing.
> 
> Why? Isn't the use case here very similar to the existing station mode
> use of power save even if the power saving mechanism is more of a vendor
> specific extension that applies while there are no associated stations?

Yeah, true, fair point.

However, set-powersave is a bit of a legacy API with state in the
kernel, and sometimes restrictions on how/when you can set it etc. I'm
not sure it's a good idea for those reasons alone?

> > Perhaps just with an attribute used in START_AP (and CHANGE_BEACON if
> > needed) would be sufficient?
> 
> NL80211_CMD_START_AP with a new attribute (or even re-use of
> NL80211_ATTR_PS_STATE) might work for a case where this does not need to
> be changed dynamically during the lifetime of the BSS.
> NL80211_CMD_SET_BEACON (which maps to the change_beacon() callback)
> feels like something that is currently limited to Beacon data updates
> with its use of struct cfg80211_beacon_data instead of struct
> cfg80211_ap_settings..
> 
> That SET_BEACON name is still from the old NEW/SET/DEL_BEACON time.
> Should that be renamed to NL80211_CMD_UPDATE_AP if we extend this to
> changes that are not really targeting the Beacon frame payload itself?

I'd be surprised if we don't already have non-beacon state there ... but
it looks like only very little non-beacon state, namely the FTM
responder state.

Renaming seems reasonable, we've done it before with START_AP.

> And should the cfg80211_beacon_data argument be replaced with
> cfg80211_ap_settings? It looks like we already have some struct
> cfg80211_ap_settings values like inactivity_timeout and beacon_rate (and
> maybe some HE parameters?) that one might want to update during the
> lifetime of the BSS..

That also seems reasonable.

johannes


-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCHv2 2/2] ath11k: Add ap power save support

[Kalle Valo]

Venkateswara Naralasetty <vnaralas@codeaurora.org> writes:

> AP power save where AP goes to power save mode when no stations associate
> to it and come out of power save when any station associate to AP.
>
> This AP power save capability can be used to save power with the drawback
> of reduced range or delayed discovery of the AP
>
> This patch also porvides user configuration to enable/disable
> this feature using vendor command. This feature is disabled by default.
>
> Tested-on: IPQ8074 WLAN.HK.2.1.0.1-01228-QCAHKSWPL_SILICONZ-1
>
> Signed-off-by: Venkateswara Naralasetty <vnaralas@codeaurora.org>

[...]

> +static int ath11k_vendor_set_wifi_config(struct wiphy *wihpy,

s/wihpy/wiphy/

> +					 struct wireless_dev *wdev,
> +					 const void *data,
> +					 int data_len)
> +{
> +	struct ieee80211_vif *vif;
> +	struct ath11k_vif *arvif;
> +	struct ath11k *ar;
> +	struct nlattr *tb[QCA_WLAN_VENDOR_ATTR_CONFIG_MAX + 1];
> +	int ret = 0;
> +
> +	if (!wdev)
> +		return -EINVAL;
> +
> +	vif = wdev_to_ieee80211_vif(wdev);
> +	if (!vif)
> +		return -EINVAL;
> +
> +	arvif = (struct ath11k_vif *)vif->drv_priv;
> +	if (!arvif)
> +		return -EINVAL;
> +
> +	ar = arvif->ar;
> +
> +	mutex_lock(&ar->conf_mutex);
> +
> +	ret = nla_parse(tb, QCA_WLAN_VENDOR_ATTR_CONFIG_MAX, data, data_len,
> +			ath11k_vendor_set_wifi_config_policy, NULL);
> +	if (ret) {
> +		ath11k_warn(ar->ab, "invalid set wifi config policy attribute\n");
> +		goto exit;
> +	}
> +
> +	ar->ap_ps_enabled = nla_get_flag(tb[QCA_WLAN_VENDOR_ATTR_CONFIG_GTX]);
> +	ret = ath11k_mac_ap_ps_recalc(ar);
> +	if (ret) {
> +		ath11k_warn(ar->ab, "failed to send ap ps ret %d\n", ret);
> +		goto exit;
> +	}
> +
> +exit:
> +	mutex_unlock(&ar->conf_mutex);
> +	return ret;
> +}

Something which I find awkward here is that this is per pdev (=all
vdevs), even though the vendor command is per vif. So if you change the
config on one vif, all other vifs will change as well. And there's no
way to check if the state from user space as there's only a set command
and no equivalent get command.

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

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

Re: [PATCHv2 2/2] ath11k: Add ap power save support

[Kalle Valo]

Venkateswara Naralasetty <vnaralas@codeaurora.org> writes:

> AP power save where AP goes to power save mode when no stations associate
> to it and come out of power save when any station associate to AP.
>
> This AP power save capability can be used to save power with the drawback
> of reduced range or delayed discovery of the AP
>
> This patch also porvides user configuration to enable/disable
> this feature using vendor command. This feature is disabled by default.
>
> Tested-on: IPQ8074 WLAN.HK.2.1.0.1-01228-QCAHKSWPL_SILICONZ-1
>
> Signed-off-by: Venkateswara Naralasetty <vnaralas@codeaurora.org>

[...]

> +static int ath11k_vendor_set_wifi_config(struct wiphy *wihpy,

s/wihpy/wiphy/

> +					 struct wireless_dev *wdev,
> +					 const void *data,
> +					 int data_len)
> +{
> +	struct ieee80211_vif *vif;
> +	struct ath11k_vif *arvif;
> +	struct ath11k *ar;
> +	struct nlattr *tb[QCA_WLAN_VENDOR_ATTR_CONFIG_MAX + 1];
> +	int ret = 0;
> +
> +	if (!wdev)
> +		return -EINVAL;
> +
> +	vif = wdev_to_ieee80211_vif(wdev);
> +	if (!vif)
> +		return -EINVAL;
> +
> +	arvif = (struct ath11k_vif *)vif->drv_priv;
> +	if (!arvif)
> +		return -EINVAL;
> +
> +	ar = arvif->ar;
> +
> +	mutex_lock(&ar->conf_mutex);
> +
> +	ret = nla_parse(tb, QCA_WLAN_VENDOR_ATTR_CONFIG_MAX, data, data_len,
> +			ath11k_vendor_set_wifi_config_policy, NULL);
> +	if (ret) {
> +		ath11k_warn(ar->ab, "invalid set wifi config policy attribute\n");
> +		goto exit;
> +	}
> +
> +	ar->ap_ps_enabled = nla_get_flag(tb[QCA_WLAN_VENDOR_ATTR_CONFIG_GTX]);
> +	ret = ath11k_mac_ap_ps_recalc(ar);
> +	if (ret) {
> +		ath11k_warn(ar->ab, "failed to send ap ps ret %d\n", ret);
> +		goto exit;
> +	}
> +
> +exit:
> +	mutex_unlock(&ar->conf_mutex);
> +	return ret;
> +}

Something which I find awkward here is that this is per pdev (=all
vdevs), even though the vendor command is per vif. So if you change the
config on one vif, all other vifs will change as well. And there's no
way to check if the state from user space as there's only a set command
and no equivalent get command.

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

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: [PATCHv2 2/2] ath11k: Add ap power save support

[Kalle Valo]

Kalle Valo <kvalo@codeaurora.org> writes:

> Venkateswara Naralasetty <vnaralas@codeaurora.org> writes:
>
>> AP power save where AP goes to power save mode when no stations associate
>> to it and come out of power save when any station associate to AP.
>>
>> This AP power save capability can be used to save power with the drawback
>> of reduced range or delayed discovery of the AP
>>
>> This patch also porvides user configuration to enable/disable
>> this feature using vendor command. This feature is disabled by default.
>>
>> Tested-on: IPQ8074 WLAN.HK.2.1.0.1-01228-QCAHKSWPL_SILICONZ-1
>>
>> Signed-off-by: Venkateswara Naralasetty <vnaralas@codeaurora.org>
>
> [...]
>
>> +static int ath11k_vendor_set_wifi_config(struct wiphy *wihpy,
>
> s/wihpy/wiphy/
>
>> +					 struct wireless_dev *wdev,
>> +					 const void *data,
>> +					 int data_len)
>> +{
>> +	struct ieee80211_vif *vif;
>> +	struct ath11k_vif *arvif;
>> +	struct ath11k *ar;
>> +	struct nlattr *tb[QCA_WLAN_VENDOR_ATTR_CONFIG_MAX + 1];
>> +	int ret = 0;
>> +
>> +	if (!wdev)
>> +		return -EINVAL;
>> +
>> +	vif = wdev_to_ieee80211_vif(wdev);
>> +	if (!vif)
>> +		return -EINVAL;
>> +
>> +	arvif = (struct ath11k_vif *)vif->drv_priv;
>> +	if (!arvif)
>> +		return -EINVAL;
>> +
>> +	ar = arvif->ar;
>> +
>> +	mutex_lock(&ar->conf_mutex);
>> +
>> +	ret = nla_parse(tb, QCA_WLAN_VENDOR_ATTR_CONFIG_MAX, data, data_len,
>> +			ath11k_vendor_set_wifi_config_policy, NULL);
>> +	if (ret) {
>> +		ath11k_warn(ar->ab, "invalid set wifi config policy attribute\n");
>> +		goto exit;
>> +	}
>> +
>> +	ar->ap_ps_enabled = nla_get_flag(tb[QCA_WLAN_VENDOR_ATTR_CONFIG_GTX]);
>> +	ret = ath11k_mac_ap_ps_recalc(ar);
>> +	if (ret) {
>> +		ath11k_warn(ar->ab, "failed to send ap ps ret %d\n", ret);
>> +		goto exit;
>> +	}
>> +
>> +exit:
>> +	mutex_unlock(&ar->conf_mutex);
>> +	return ret;
>> +}
>
> Something which I find awkward here is that this is per pdev (=all
> vdevs), even though the vendor command is per vif. So if you change the
> config on one vif, all other vifs will change as well. And there's no
> way to check if the state from user space as there's only a set command
> and no equivalent get command.

Actually the problem comes here:

+static struct wiphy_vendor_command ath11k_vendor_commands[] = {
+	{
+		.info.vendor_id = QCA_NL80211_VENDOR_ID,
+		.info.subcmd = QCA_NL80211_VENDOR_SUBCMD_SET_WIFI_CONFIGURATION,
+		.flags = WIPHY_VENDOR_CMD_NEED_WDEV |
+			WIPHY_VENDOR_CMD_NEED_RUNNING,
+		.doit = ath11k_vendor_set_wifi_config,
+		.policy = ath11k_vendor_set_wifi_config_policy,
+		.maxattr = QCA_WLAN_VENDOR_ATTR_CONFIG_MAX
+	}

If it's per pdev you shouldn't set WIPHY_VENDOR_CMD_NEED_WDEV.

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

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

Re: [PATCHv2 2/2] ath11k: Add ap power save support

[Kalle Valo]

Kalle Valo <kvalo@codeaurora.org> writes:

> Venkateswara Naralasetty <vnaralas@codeaurora.org> writes:
>
>> AP power save where AP goes to power save mode when no stations associate
>> to it and come out of power save when any station associate to AP.
>>
>> This AP power save capability can be used to save power with the drawback
>> of reduced range or delayed discovery of the AP
>>
>> This patch also porvides user configuration to enable/disable
>> this feature using vendor command. This feature is disabled by default.
>>
>> Tested-on: IPQ8074 WLAN.HK.2.1.0.1-01228-QCAHKSWPL_SILICONZ-1
>>
>> Signed-off-by: Venkateswara Naralasetty <vnaralas@codeaurora.org>
>
> [...]
>
>> +static int ath11k_vendor_set_wifi_config(struct wiphy *wihpy,
>
> s/wihpy/wiphy/
>
>> +					 struct wireless_dev *wdev,
>> +					 const void *data,
>> +					 int data_len)
>> +{
>> +	struct ieee80211_vif *vif;
>> +	struct ath11k_vif *arvif;
>> +	struct ath11k *ar;
>> +	struct nlattr *tb[QCA_WLAN_VENDOR_ATTR_CONFIG_MAX + 1];
>> +	int ret = 0;
>> +
>> +	if (!wdev)
>> +		return -EINVAL;
>> +
>> +	vif = wdev_to_ieee80211_vif(wdev);
>> +	if (!vif)
>> +		return -EINVAL;
>> +
>> +	arvif = (struct ath11k_vif *)vif->drv_priv;
>> +	if (!arvif)
>> +		return -EINVAL;
>> +
>> +	ar = arvif->ar;
>> +
>> +	mutex_lock(&ar->conf_mutex);
>> +
>> +	ret = nla_parse(tb, QCA_WLAN_VENDOR_ATTR_CONFIG_MAX, data, data_len,
>> +			ath11k_vendor_set_wifi_config_policy, NULL);
>> +	if (ret) {
>> +		ath11k_warn(ar->ab, "invalid set wifi config policy attribute\n");
>> +		goto exit;
>> +	}
>> +
>> +	ar->ap_ps_enabled = nla_get_flag(tb[QCA_WLAN_VENDOR_ATTR_CONFIG_GTX]);
>> +	ret = ath11k_mac_ap_ps_recalc(ar);
>> +	if (ret) {
>> +		ath11k_warn(ar->ab, "failed to send ap ps ret %d\n", ret);
>> +		goto exit;
>> +	}
>> +
>> +exit:
>> +	mutex_unlock(&ar->conf_mutex);
>> +	return ret;
>> +}
>
> Something which I find awkward here is that this is per pdev (=all
> vdevs), even though the vendor command is per vif. So if you change the
> config on one vif, all other vifs will change as well. And there's no
> way to check if the state from user space as there's only a set command
> and no equivalent get command.

Actually the problem comes here:

+static struct wiphy_vendor_command ath11k_vendor_commands[] = {
+	{
+		.info.vendor_id = QCA_NL80211_VENDOR_ID,
+		.info.subcmd = QCA_NL80211_VENDOR_SUBCMD_SET_WIFI_CONFIGURATION,
+		.flags = WIPHY_VENDOR_CMD_NEED_WDEV |
+			WIPHY_VENDOR_CMD_NEED_RUNNING,
+		.doit = ath11k_vendor_set_wifi_config,
+		.policy = ath11k_vendor_set_wifi_config_policy,
+		.maxattr = QCA_WLAN_VENDOR_ATTR_CONFIG_MAX
+	}

If it's per pdev you shouldn't set WIPHY_VENDOR_CMD_NEED_WDEV.

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

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

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k