[PATCH 0/2] ath10k: SAR power limit vendor command

[PATCH 0/2] ath10k: SAR power limit vendor command

[Kalle Valo]

Hi,

here's a patchset adding dynamic SAR power limit vendor command to
ath10k. This follows the new process documented in the wiki:

https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api

Please review.

Kalle

Wen Gong (2):
  nl80211: vendor-cmd: qca: add dynamic SAR power limits
  ath10k: allow dynamic SAR power limits to be configured

 drivers/net/wireless/ath/ath10k/Makefile |   1 +
 drivers/net/wireless/ath/ath10k/core.c   |   2 +
 drivers/net/wireless/ath/ath10k/core.h   |   2 +
 drivers/net/wireless/ath/ath10k/hw.h     |   3 +
 drivers/net/wireless/ath/ath10k/mac.c    |  64 +++++++++++++++++
 drivers/net/wireless/ath/ath10k/mac.h    |   2 +-
 drivers/net/wireless/ath/ath10k/vendor.c | 114 +++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/vendor.h |  13 ++++
 drivers/net/wireless/ath/ath10k/wmi.h    |   6 ++
 include/uapi/nl80211-vnd-qca.h           |  68 ++++++++++++++++++
 10 files changed, 274 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/wireless/ath/ath10k/vendor.c
 create mode 100644 drivers/net/wireless/ath/ath10k/vendor.h
 create mode 100644 include/uapi/nl80211-vnd-qca.h

-- 
2.7.4

[PATCH 1/2] nl80211: vendor-cmd: qca: add dynamic SAR power limits

[Kalle Valo]

From: Wen Gong <wgong@codeaurora.org>

The vendor commands is to add API for user to configure dynamic SAR
power limits, it will not replace the existing power control
functionality, it is to make more convenient to configure power.

An example of usage(wlan0 is the wireless interface dev name):
iw dev wlan0 vendor send 0x1374 0x92 0x2C 0x00 0x03 0x00 0x14 0x00 0x01
0x00 0x08 0x00 0x07 0x00 0x22 0x00 0x00 0x00 0x08 0x00 0x04 0x00 0x00
0x00 0x00 0x00 0x14 0x00 0x02 0x00 0x08 0x00 0x07 0x00 0x11 0x00 0x00
0x00 0x08 0x00 0x04 0x00 0x01 0x00 0x00 0x00

means of bytes:
0x1374: vendor id
0x92: vendor subcmd id
0x22: 2.4G power limit
0x11: 5G power limit

Tested with QCA6174 SDIO with firmware WLAN.RMH.4.4.1-00029.

Signed-off-by: Wen Gong <wgong@codeaurora.org>
Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
---
 include/uapi/nl80211-vnd-qca.h | 68 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 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 000000000000..482c9409a2c0
--- /dev/null
+++ b/include/uapi/nl80211-vnd-qca.h
@@ -0,0 +1,68 @@
+/* SPDX-License-Identifier: ISC */
+/*
+ * Copyright (c) 2019 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_SAR_LIMITS and is used to retrieve the
+ *	settings currently in use. The attributes returned by this command are
+ *	defined by enum qca_vendor_attr_sar_limits.
+ */
+enum qca_nl80211_vendor_subcmds {
+	QCA_NL80211_VENDOR_SUBCMD_SET_SAR_LIMITS = 146,
+	};
+
+/**
+ * enum qca_vendor_attr_sar_limits - Attributes for SAR power limits
+ *
+ * @QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC: Nested array of SAR power
+ *	limit specifications. The number of specifications is
+ *	specified by @QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_NUM_SPECS. Each
+ *	specification contains a set of
+ *	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_* attributes. A
+ *	specification is uniquely identified by the attributes
+ *	%QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_BAND and always
+ *	contains as a payload the attribute
+ *	%QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_POWER_LIMIT.
+ *
+ * @QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_BAND: Optional (u32) value to
+ *	indicate for which band this specification applies. Valid
+ *	values are enumerated in enum %nl80211_band (although not all
+ *	bands may be supported by a given device). If the attribute is
+ *	not supplied then the specification will be applied to all
+ *	supported bands.
+ *
+ * @QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_POWER_LIMIT: Required (u32)
+ *	value to specify the actual power limit value in units of 0.5
+ *	dBm (i.e., a value of 11 represents 5.5 dBm).
+ *	This is required, when %QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SELECT is
+ *	%QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SELECT_USER.
+ *
+ * These attributes are used with %QCA_NL80211_VENDOR_SUBCMD_SET_SAR_LIMITS
+ * and %QCA_NL80211_VENDOR_SUBCMD_GET_SAR_LIMITS.
+ */
+enum qca_vendor_attr_sar_limits {
+	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_INVALID = 0,
+	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC = 3,
+	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_BAND = 4,
+	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_POWER_LIMIT = 7,
+
+	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_AFTER_LAST,
+	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_MAX =
+		QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_AFTER_LAST - 1
+};
+
+#endif /* _UAPI_NL80211_VND_QCA_H_ */
-- 
2.7.4

[PATCH 2/2] ath10k: allow dynamic SAR power limits to be configured

[Kalle Valo]

From: Wen Gong <wgong@codeaurora.org>

Add support for a vendor command for STATION, the command
QCA_NL80211_VENDOR_SUBCMD_SET_SAR_LIMITS which is already defined in
git://w1.fi/hostap.git (src/command/qca-vendor.h). This allows user
space to configure power limits for 2.4 GHz and 5 GHz bands.

ath10k set pdev parameter WMI_PDEV_PARAM_TXPOWER_LIMIT2G and
WMI_PDEV_PARAM_TXPOWER_LIMIT5G to firmware, the 2 value will
be used as one input source to affect the tx power.

When QCA_NL80211_VENDOR_SUBCMD_SET_SAR_LIMITS set to ath10k, it will
be saved the 2.4G and 5G limit value, If STATION is connected meanwhile,
then the 2.4G and 5G WMI command will be set to firmware, otherwise
it will not set to firmware at this moment. When STATION connect
next time, it will set to firmware.

Tested with QCA6174 SDIO with firmware WLAN.RMH.4.4.1-00029.

Signed-off-by: Wen Gong <wgong@codeaurora.org>
Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
---
 drivers/net/wireless/ath/ath10k/Makefile |   1 +
 drivers/net/wireless/ath/ath10k/core.c   |   2 +
 drivers/net/wireless/ath/ath10k/core.h   |   2 +
 drivers/net/wireless/ath/ath10k/hw.h     |   3 +
 drivers/net/wireless/ath/ath10k/mac.c    |  64 +++++++++++++++++
 drivers/net/wireless/ath/ath10k/mac.h    |   2 +-
 drivers/net/wireless/ath/ath10k/vendor.c | 114 +++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/vendor.h |  13 ++++
 drivers/net/wireless/ath/ath10k/wmi.h    |   6 ++
 9 files changed, 206 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/wireless/ath/ath10k/vendor.c
 create mode 100644 drivers/net/wireless/ath/ath10k/vendor.h

diff --git a/drivers/net/wireless/ath/ath10k/Makefile b/drivers/net/wireless/ath/ath10k/Makefile
index 142c777b287f..6cdc7eba5d25 100644
--- a/drivers/net/wireless/ath/ath10k/Makefile
+++ b/drivers/net/wireless/ath/ath10k/Makefile
@@ -13,6 +13,7 @@ ath10k_core-y += mac.o \
 		 bmi.o \
 		 hw.o \
 		 p2p.o \
+		 vendor.o \
 		 swap.o
 
 ath10k_core-$(CONFIG_ATH10K_SPECTRAL) += spectral.o
diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 5ec16ce19b69..e882d2f2e399 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -25,6 +25,7 @@
 #include "testmode.h"
 #include "wmi-ops.h"
 #include "coredump.h"
+#include "vendor.h"
 
 unsigned int ath10k_debug_mask;
 EXPORT_SYMBOL(ath10k_debug_mask);
@@ -190,6 +191,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.uart_pin_workaround = true,
 		.tx_stats_over_pktlog = false,
 		.bmi_large_size_download = true,
+		.dynamic_sar_support = true,
 	},
 	{
 		.id = QCA6174_HW_2_1_VERSION,
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 5101bf2b5b15..b661e0eee0f8 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -983,6 +983,8 @@ struct ath10k {
 	u8 ps_state_enable;
 
 	bool nlo_enabled;
+	u32 tx_power_2g_limit;
+	u32 tx_power_5g_limit;
 	bool p2p;
 
 	struct {
diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h
index 21b7a2a873b0..8cee355098e3 100644
--- a/drivers/net/wireless/ath/ath10k/hw.h
+++ b/drivers/net/wireless/ath/ath10k/hw.h
@@ -623,6 +623,9 @@ struct ath10k_hw_params {
 
 	/* tx stats support over pktlog */
 	bool tx_stats_over_pktlog;
+
+	/* support dynamic sar */
+	bool dynamic_sar_support;
 };
 
 struct htt_rx_desc;
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 7fee35ff966b..75c600e8fff2 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -24,6 +24,7 @@
 #include "wmi-tlv.h"
 #include "wmi-ops.h"
 #include "wow.h"
+#include "vendor.h"
 
 /*********/
 /* Rates */
@@ -2843,6 +2844,65 @@ static int ath10k_mac_vif_recalc_txbf(struct ath10k *ar,
 	return 0;
 }
 
+static bool ath10k_mac_get_connected(struct ath10k *ar)
+{
+	struct ath10k_vif *arvif;
+
+	list_for_each_entry(arvif, &ar->arvifs, list) {
+		if (arvif->is_up && arvif->vdev_type == WMI_VDEV_TYPE_STA)
+			return true;
+	}
+
+	return false;
+}
+
+int ath10k_mac_set_sar_power_limit(struct ath10k *ar)
+{
+	u32 ret, param, pwr_limit_2G, pwr_limit_5G;
+	bool connected, tx_power_valid;
+
+	if (!ar->hw_params.dynamic_sar_support)
+		return 0;
+
+	connected = ath10k_mac_get_connected(ar);
+	tx_power_valid = (ar->tx_power_2g_limit != 0 && ar->tx_power_5g_limit != 0);
+
+	ath10k_dbg(ar, ATH10K_DBG_MAC, "mac connected %d sar power valid %d\n",
+		   connected, tx_power_valid);
+
+	if (!connected || !tx_power_valid)
+		return 0;
+
+	pwr_limit_2G = ar->tx_power_2g_limit;
+	pwr_limit_5G = ar->tx_power_5g_limit;
+
+	ath10k_dbg(ar, ATH10K_DBG_MAC, "mac sar limits max %d 2.4G %d 5G %d\n",
+		   ar->hw_max_tx_power,
+		   pwr_limit_2G,
+		   pwr_limit_5G);
+
+	param = ar->wmi.pdev_param->txpower_limit2g;
+	ret = ath10k_wmi_pdev_set_param(ar, param, pwr_limit_2G);
+	if (ret) {
+		ath10k_warn(ar, "failed to set 2.4G txpower %d: %d\n",
+			    pwr_limit_2G, ret);
+		return ret;
+	}
+	ath10k_dbg(ar, ATH10K_DBG_MAC, "mac set 2.4G txpower %d success\n", pwr_limit_2G);
+
+	param = ar->wmi.pdev_param->txpower_limit5g;
+	ret = ath10k_wmi_pdev_set_param(ar, param, pwr_limit_5G);
+	if (ret) {
+		ath10k_warn(ar, "failed to set 5G txpower %d: %d\n",
+			    pwr_limit_5G, ret);
+		return ret;
+	}
+
+	ath10k_dbg(ar, ATH10K_DBG_MAC, "mac set 5G txpower %d success\n", pwr_limit_5G);
+
+	return 0;
+}
+
 /* can be called only in mac80211 callbacks due to `key_count` usage */
 static void ath10k_bss_assoc(struct ieee80211_hw *hw,
 			     struct ieee80211_vif *vif,
@@ -2926,6 +2986,8 @@ static void ath10k_bss_assoc(struct ieee80211_hw *hw,
 
 	arvif->is_up = true;
 
+	ath10k_mac_set_sar_power_limit(ar);
+
 	/* Workaround: Some firmware revisions (tested with qca6174
 	 * WLAN.RM.2.0-00073) have buggy powersave state machine and must be
 	 * poked with peer param command.
@@ -9074,6 +9136,8 @@ int ath10k_mac_register(struct ath10k *ar)
 			NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR;
 	}
 
+	ath10k_vendor_register(ar);
+
 	ar->hw->wiphy->cipher_suites = cipher_suites;
 
 	/* QCA988x and QCA6174 family chips do not support CCMP-256, GCMP-128
diff --git a/drivers/net/wireless/ath/ath10k/mac.h b/drivers/net/wireless/ath/ath10k/mac.h
index 98d83a26ea60..54c8cfcef611 100644
--- a/drivers/net/wireless/ath/ath10k/mac.h
+++ b/drivers/net/wireless/ath/ath10k/mac.h
@@ -58,7 +58,7 @@ u8 ath10k_mac_hw_rate_to_idx(const struct ieee80211_supported_band *sband,
 			     u8 hw_rate, bool cck);
 u8 ath10k_mac_bitrate_to_idx(const struct ieee80211_supported_band *sband,
 			     u32 bitrate);
-
+int ath10k_mac_set_sar_power_limit(struct ath10k *ar);
 void ath10k_mac_tx_lock(struct ath10k *ar, int reason);
 void ath10k_mac_tx_unlock(struct ath10k *ar, int reason);
 void ath10k_mac_vif_tx_lock(struct ath10k_vif *arvif, int reason);
diff --git a/drivers/net/wireless/ath/ath10k/vendor.c b/drivers/net/wireless/ath/ath10k/vendor.c
new file mode 100644
index 000000000000..755dbf7146cb
--- /dev/null
+++ b/drivers/net/wireless/ath/ath10k/vendor.c
@@ -0,0 +1,114 @@
+// SPDX-License-Identifier: ISC
+/*
+ * Copyright (c) 2019, The Linux Foundation. All rights reserved.
+ */
+
+#include <net/netlink.h>
+#include <uapi/nl80211-vnd-qca.h>
+
+#include "mac.h"
+#include "debug.h"
+#include "vendor.h"
+
+static const struct nla_policy
+sar_limits_policy[QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_MAX + 1] = {
+	[QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_BAND] = {.type = NLA_U32},
+	[QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_POWER_LIMIT] = {.type = NLA_U32},
+};
+
+static int ath10k_vendor_set_dynamic_sar_power_limits(struct wiphy *wiphy,
+						      struct wireless_dev *wdev,
+						      const void *data,
+						      int data_len)
+{
+	int rem;
+	struct ieee80211_hw *hw = wiphy_to_ieee80211_hw(wiphy);
+	struct ath10k *ar = hw->priv;
+	struct nlattr *spec[QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_MAX + 1];
+	struct nlattr *tb[QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_MAX + 1];
+	struct nlattr *spec_list;
+	u32 limit;
+	enum nl80211_band band;
+	bool sar_valid = false;
+
+	if (!ar->hw_params.dynamic_sar_support)
+		return -ENOTSUPP;
+
+	if (nla_parse(tb, QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_MAX, data, data_len,
+		      sar_limits_policy, NULL)) {
+		ath10k_warn(ar, "invalid SAR attr\n");
+		return -EINVAL;
+	}
+
+	if (!tb[QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC]) {
+		ath10k_warn(ar, "invalid SAR specification list\n");
+		return -EINVAL;
+	}
+
+	nla_for_each_nested(spec_list, tb[QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC], rem) {
+		if (nla_parse(spec,
+			      QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_MAX,
+			      nla_data(spec_list),
+			      nla_len(spec_list),
+			      sar_limits_policy,
+			      NULL)) {
+			ath10k_warn(ar, "nla_parse failed for SAR Spec list\n");
+			return -EINVAL;
+		}
+
+		if (spec[QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_POWER_LIMIT]) {
+			limit = nla_get_u32(
+				spec[QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_POWER_LIMIT]);
+		} else {
+			ath10k_warn(ar, "not have spec power limit\n");
+			return -EINVAL;
+		}
+
+		if (spec[QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_BAND]) {
+			band = nla_get_u32(
+				spec[QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_BAND]);
+		} else {
+			/* if QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_BAND is
+			 * unset the limit applies to both bands
+			 */
+			if (limit <= ar->hw_max_tx_power) {
+				ar->tx_power_2g_limit = limit;
+				ar->tx_power_5g_limit = limit;
+				sar_valid = true;
+			}
+
+			break;
+		}
+
+		sar_valid = true;
+		if (band == NL80211_BAND_2GHZ && limit <= ar->hw_max_tx_power)
+			ar->tx_power_2g_limit = limit;
+		else if (band == NL80211_BAND_5GHZ && limit <= ar->hw_max_tx_power)
+			ar->tx_power_5g_limit = limit;
+	}
+
+	if (sar_valid)
+		return ath10k_mac_set_sar_power_limit(ar);
+
+	return 0;
+}
+
+static const struct wiphy_vendor_command ath10k_vendor_commands[] = {
+	{
+		.info = {
+			.vendor_id = QCA_NL80211_VENDOR_ID,
+			.subcmd = QCA_NL80211_VENDOR_SUBCMD_SET_SAR_LIMITS,
+		},
+		.flags = WIPHY_VENDOR_CMD_NEED_WDEV |
+			 WIPHY_VENDOR_CMD_NEED_RUNNING,
+		.doit = ath10k_vendor_set_dynamic_sar_power_limits,
+		.policy = sar_limits_policy,
+	}
+};
+
+void ath10k_vendor_register(struct ath10k *ar)
+{
+	ar->hw->wiphy->vendor_commands = ath10k_vendor_commands;
+	ar->hw->wiphy->n_vendor_commands = ARRAY_SIZE(ath10k_vendor_commands);
+}
+
diff --git a/drivers/net/wireless/ath/ath10k/vendor.h b/drivers/net/wireless/ath/ath10k/vendor.h
new file mode 100644
index 000000000000..827614cb6991
--- /dev/null
+++ b/drivers/net/wireless/ath/ath10k/vendor.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: ISC */
+/*
+ * Copyright (c) 2019 The Linux Foundation. All rights reserved.
+ */
+
+#ifndef _VENDOR_H_
+#define _VENDOR_H_
+
+#include <linux/kernel.h>
+
+void ath10k_vendor_register(struct ath10k *ar);
+
+#endif /* _VENDOR_H_ */
diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index 972d53d77654..f0cadaa12399 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -3695,6 +3695,12 @@ struct wmi_csa_event {
 #define VDEV_DEFAULT_STATS_UPDATE_PERIOD    500
 #define PEER_DEFAULT_STATS_UPDATE_PERIOD    500
 
+/* the mask of 4 sub band of 5G for SAR parameters */
+#define ATH10K_WMI_SAR_5G_0_MASK GENMASK(7, 0)
+#define ATH10K_WMI_SAR_5G_1_MASK GENMASK(15, 8)
+#define ATH10K_WMI_SAR_5G_2_MASK GENMASK(23, 16)
+#define ATH10K_WMI_SAR_5G_3_MASK GENMASK(31, 24)
+
 struct wmi_pdev_param_map {
 	u32 tx_chain_mask;
 	u32 rx_chain_mask;
-- 
2.7.4

Re: [PATCH 1/2] nl80211: vendor-cmd: qca: add dynamic SAR power limits

[Pkshih]

On Wed, 2019-12-18 at 17:48 +0200, Kalle Valo wrote:
> From: Wen Gong <wgong@codeaurora.org>
> 
> The vendor commands is to add API for user to configure dynamic SAR
> power limits, it will not replace the existing power control
> functionality, it is to make more convenient to configure power.
> 
> An example of usage(wlan0 is the wireless interface dev name):
> iw dev wlan0 vendor send 0x1374 0x92 0x2C 0x00 0x03 0x00 0x14 0x00 0x01
> 0x00 0x08 0x00 0x07 0x00 0x22 0x00 0x00 0x00 0x08 0x00 0x04 0x00 0x00
> 0x00 0x00 0x00 0x14 0x00 0x02 0x00 0x08 0x00 0x07 0x00 0x11 0x00 0x00
> 0x00 0x08 0x00 0x04 0x00 0x01 0x00 0x00 0x00
> 
> means of bytes:
> 0x1374: vendor id
> 0x92: vendor subcmd id
> 0x22: 2.4G power limit
> 0x11: 5G power limit
> 
> Tested with QCA6174 SDIO with firmware WLAN.RMH.4.4.1-00029.
> 
> Signed-off-by: Wen Gong <wgong@codeaurora.org>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
> ---
>  include/uapi/nl80211-vnd-qca.h | 68
> ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 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 000000000000..482c9409a2c0
> --- /dev/null
> +++ b/include/uapi/nl80211-vnd-qca.h
> @@ -0,0 +1,68 @@
> +/* SPDX-License-Identifier: ISC */
> +/*
> + * Copyright (c) 2019 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_SAR_LIMITS and is used to retrieve the
> + *	settings currently in use. The attributes returned by this command
> are
> + *	defined by enum qca_vendor_attr_sar_limits.
> + */
> +enum qca_nl80211_vendor_subcmds {
> +	QCA_NL80211_VENDOR_SUBCMD_SET_SAR_LIMITS = 146,
> +	};
> +
> +/**
> + * enum qca_vendor_attr_sar_limits - Attributes for SAR power limits
> + *
> + * @QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC: Nested array of SAR power
> + *	limit specifications. The number of specifications is
> + *	specified by @QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_NUM_SPECS. Each
> + *	specification contains a set of
> + *	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_* attributes. A
> + *	specification is uniquely identified by the attributes
> + *	%QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_BAND and always
> + *	contains as a payload the attribute
> + *	%QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_POWER_LIMIT.
> + *
> + * @QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_BAND: Optional (u32) value to
> + *	indicate for which band this specification applies. Valid
> + *	values are enumerated in enum %nl80211_band (although not all
> + *	bands may be supported by a given device). If the attribute is

Can we define separated enum to address four 5G sub-bands, likes

enum nl80211_sar_band {
	NL80211_SAR_BAND_2G,
	NL80211_SAR_BAND_5G_BAND1,
	NL80211_SAR_BAND_5G_BAND2,
	NL80211_SAR_BAND_5G_BAND3,
	NL80211_SAR_BAND_5G_BAND4,
};

I think this vendor command can be a generic nl80211 command, because
we need SAR
power limit as well.

> + *	not supplied then the specification will be applied to all
> + *	supported bands.
> + *
> + * @QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_POWER_LIMIT: Required (u32)
> + *	value to specify the actual power limit value in units of 0.5
> + *	dBm (i.e., a value of 11 represents 5.5 dBm).

Can we have higher precision, in unit of 0.125?


> + *	This is required, when %QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SELECT is
> + *	%QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SELECT_USER.
> + *
> + * These attributes are used with %QCA_NL80211_VENDOR_SUBCMD_SET_SAR_LIMITS
> + * and %QCA_NL80211_VENDOR_SUBCMD_GET_SAR_LIMITS.
> + */
> +enum qca_vendor_attr_sar_limits {
> +	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_INVALID = 0,
> +	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC = 3,
> +	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_BAND = 4,
> +	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_POWER_LIMIT = 7,

Why these enum aren't continual?
The reason may be because QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SELECT and something
me
ntioned in above paragraph but missing in this enum?

They will waste memory when doing nla_parse() with tb[MAX];

> +
> +	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_AFTER_LAST,
> +	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_MAX =
> +		QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_AFTER_LAST - 1
> +};
> +
> +#endif /* _UAPI_NL80211_VND_QCA_H_ */

Re: [PATCH 2/2] ath10k: allow dynamic SAR power limits to be configured

[Pkshih]

On Wed, 2019-12-18 at 17:48 +0200, Kalle Valo wrote:
> From: Wen Gong <wgong@codeaurora.org>
> 
> Add support for a vendor command for STATION, the command
> QCA_NL80211_VENDOR_SUBCMD_SET_SAR_LIMITS which is already defined in
> git://w1.fi/hostap.git (src/command/qca-vendor.h). This allows user
> space to configure power limits for 2.4 GHz and 5 GHz bands.
> 
> ath10k set pdev parameter WMI_PDEV_PARAM_TXPOWER_LIMIT2G and
> WMI_PDEV_PARAM_TXPOWER_LIMIT5G to firmware, the 2 value will
> be used as one input source to affect the tx power.
> 
> When QCA_NL80211_VENDOR_SUBCMD_SET_SAR_LIMITS set to ath10k, it will
> be saved the 2.4G and 5G limit value, If STATION is connected meanwhile,
> then the 2.4G and 5G WMI command will be set to firmware, otherwise
> it will not set to firmware at this moment. When STATION connect
> next time, it will set to firmware.
> 
> Tested with QCA6174 SDIO with firmware WLAN.RMH.4.4.1-00029.
> 
> Signed-off-by: Wen Gong <wgong@codeaurora.org>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
> ---
>  drivers/net/wireless/ath/ath10k/Makefile |   1 +
>  drivers/net/wireless/ath/ath10k/core.c   |   2 +
>  drivers/net/wireless/ath/ath10k/core.h   |   2 +
>  drivers/net/wireless/ath/ath10k/hw.h     |   3 +
>  drivers/net/wireless/ath/ath10k/mac.c    |  64 +++++++++++++++++
>  drivers/net/wireless/ath/ath10k/mac.h    |   2 +-
>  drivers/net/wireless/ath/ath10k/vendor.c | 114
> +++++++++++++++++++++++++++++++
>  drivers/net/wireless/ath/ath10k/vendor.h |  13 ++++
>  drivers/net/wireless/ath/ath10k/wmi.h    |   6 ++
>  9 files changed, 206 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/net/wireless/ath/ath10k/vendor.c
>  create mode 100644 drivers/net/wireless/ath/ath10k/vendor.h
> 

[...]

> diff --git a/drivers/net/wireless/ath/ath10k/wmi.h
> b/drivers/net/wireless/ath/ath10k/wmi.h
> index 972d53d77654..f0cadaa12399 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.h
> +++ b/drivers/net/wireless/ath/ath10k/wmi.h
> @@ -3695,6 +3695,12 @@ struct wmi_csa_event {
>  #define VDEV_DEFAULT_STATS_UPDATE_PERIOD    500
>  #define PEER_DEFAULT_STATS_UPDATE_PERIOD    500
>  
> +/* the mask of 4 sub band of 5G for SAR parameters */
> +#define ATH10K_WMI_SAR_5G_0_MASK GENMASK(7, 0)
> +#define ATH10K_WMI_SAR_5G_1_MASK GENMASK(15, 8)
> +#define ATH10K_WMI_SAR_5G_2_MASK GENMASK(23, 16)
> +#define ATH10K_WMI_SAR_5G_3_MASK GENMASK(31, 24)
> +

These masks aren't used.
Do you use 'u32' as four 'u8' SAR power limit for four bands?

>  struct wmi_pdev_param_map {
>  	u32 tx_chain_mask;
>  	u32 rx_chain_mask;

Re: [PATCH 1/2] nl80211: vendor-cmd: qca: add dynamic SAR power limits

[Jouni Malinen]

On Thu, Dec 19, 2019 at 09:44:52AM +0000, Pkshih wrote:
> On Wed, 2019-12-18 at 17:48 +0200, Kalle Valo wrote:
> > diff --git a/include/uapi/nl80211-vnd-qca.h b/include/uapi/nl80211-vnd-qca.h
> > + * 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

> > + * @QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_BAND: Optional (u32) value to
> > + *	indicate for which band this specification applies. Valid
> > + *	values are enumerated in enum %nl80211_band (although not all
> > + *	bands may be supported by a given device). If the attribute is
> 
> Can we define separated enum to address four 5G sub-bands, likes
> 
> enum nl80211_sar_band {
> 	NL80211_SAR_BAND_2G,
> 	NL80211_SAR_BAND_5G_BAND1,
> 	NL80211_SAR_BAND_5G_BAND2,
> 	NL80211_SAR_BAND_5G_BAND3,
> 	NL80211_SAR_BAND_5G_BAND4,
> };

Please note that the vendor subcmd and attributes used here are already
deployed and in use as a kernel interface. As such, the existing
attributes cannot really be modified; if anything else would be needed,
that would need to be defined as a new attribute and/or command.

> I think this vendor command can be a generic nl80211 command, because
> we need SAR
> power limit as well.

This was discussed during the 2019 wireless workshop. The conclusion
from that discussion was that while there is clear need for SAR power
limits for various devices and multiple vendors/drivers, it did not look
clear that a single common interface could be defined cleanly taken into
account the differences in the ways vendors have designed the mechanism
in driver and firmware implementations. As such, vendor specific
commands were identified as the approach.

> > + * @QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_POWER_LIMIT: Required (u32)
> > + *	value to specify the actual power limit value in units of 0.5
> > + *	dBm (i.e., a value of 11 represents 5.5 dBm).
> 
> Can we have higher precision, in unit of 0.125?

This existing attribute cannot be modified, i.e., a new one would need
to be added if a different precision is needed. As far as the specific
need for the vendor command defined here is concerned, 0.5 dB unit is
sufficient.

> > + * These attributes are used with %QCA_NL80211_VENDOR_SUBCMD_SET_SAR_LIMITS
> > + * and %QCA_NL80211_VENDOR_SUBCMD_GET_SAR_LIMITS.
> > + */
> > +enum qca_vendor_attr_sar_limits {
> > +	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_INVALID = 0,
> > +	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC = 3,
> > +	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_BAND = 4,
> > +	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_POWER_LIMIT = 7,
> 
> Why these enum aren't continual?
> The reason may be because QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SELECT and something
> me
> ntioned in above paragraph but missing in this enum?

This patch does not include all the assigned values (see hostap.git for
full details if desired), i.e., all the values are actually assigned,
but the proposed use case for ath10k does not need the values that were
left out from this header file that is a copy of the authoritative
definition of the vendor attributes.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH 1/2] nl80211: vendor-cmd: qca: add dynamic SAR power limits

[Brian Norris]

On Thu, Dec 19, 2019 at 7:48 AM Jouni Malinen <j@w1.fi> wrote:
> On Thu, Dec 19, 2019 at 09:44:52AM +0000, Pkshih wrote:
> > On Wed, 2019-12-18 at 17:48 +0200, Kalle Valo wrote:
> > > diff --git a/include/uapi/nl80211-vnd-qca.h b/include/uapi/nl80211-vnd-qca.h
> > > + * 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

By the way, I wonder -- why have this statement? That's not how I
recall any other piece of kernel development ever working; upstream
Linux defines the upstream Linux API, not some arbitrary user space
project. This statement could be useful for saying, "don't stomp on
those command numbers please," but the response should probably be to
go out and define a totally new vendor ID or something. (See below.)

> > > + * @QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_BAND: Optional (u32) value to
> > > + * indicate for which band this specification applies. Valid
> > > + * values are enumerated in enum %nl80211_band (although not all
> > > + * bands may be supported by a given device). If the attribute is
> >
> > Can we define separated enum to address four 5G sub-bands, likes
> >
> > enum nl80211_sar_band {
> >       NL80211_SAR_BAND_2G,
> >       NL80211_SAR_BAND_5G_BAND1,
> >       NL80211_SAR_BAND_5G_BAND2,
> >       NL80211_SAR_BAND_5G_BAND3,
> >       NL80211_SAR_BAND_5G_BAND4,
> > };
>
> Please note that the vendor subcmd and attributes used here are already
> deployed and in use as a kernel interface. As such, the existing
> attributes cannot really be modified; if anything else would be needed,
> that would need to be defined as a new attribute and/or command.

Clarification: you're talking about out-of-tree drivers, which really
have no relevance in upstream discussion, except possibly as examples.
I don't think it's ever been a valid approach to dictate upstream
kernel design based simply on "what $vendor already implemented for
Android."

Maybe it's a better idea to just use different command numbers (or
vendor ID?) here, to avoid stomping on each others' implementation
choices. Otherwise, it sounds like our only choice here is to copy
your Android driver verbatim, or get lost.

> > I think this vendor command can be a generic nl80211 command, because
> > we need SAR
> > power limit as well.
>
> This was discussed during the 2019 wireless workshop. The conclusion
> from that discussion was that while there is clear need for SAR power
> limits for various devices and multiple vendors/drivers, it did not look
> clear that a single common interface could be defined cleanly taken into
> account the differences in the ways vendors have designed the mechanism
> in driver and firmware implementations. As such, vendor specific
> commands were identified as the approach.

[citation needed]
I was in that workshop, and while I recall the assertion, I don't
recall any evidence [1]. In fact, I've watched (off-list) Wen Gong
propose several variations of this exact same API along the way (hint,
I'm the one requesting he upstream this), and it's clear there's
*some* flexibility in the API. If, for example, the driver attempted
to provide a list of frequency bands it supports controlling TX power
for, that would go a long way toward sharing this API between drivers.

Another hint: this is exactly why Pkshih is speaking up here -- I'm
fielding requests from him and his employer on implementing the same
feature, and his API is starting to look an awful lot like yours. So I
suggested he review this proposal to see where and why they differ.

Anyway, I don't really object with starting out with a
Qualcomm-specific and a Realtek-specific vendor command to implement
nearly the same feature, but I'd prefer if people did engage in some
healthy discussion about why they can't share an API, with the hopes
that maybe they can converge someday. In fact, that's exactly what the
Wiki says about this:

https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api

"The goal with these rules is to enable use of vendor commands in a
fashion that is transparent enough to allow later reuse by other
components with similar needs, and then potentially defining “real”
nl80211 API for the use case in question."

Regards,
Brian

[1] The closest thing to evidence I've seen is that certain $vendors
decide they don't want to give user space any control at all over the
SAR power tables, for $reasons. But such $vendors should not really
have any say in implementing user space APIs for those $vendors that
do provide such control.

Re: [PATCH 1/2] nl80211: vendor-cmd: qca: add dynamic SAR power limits

[Jouni Malinen]

On Thu, Dec 19, 2019 at 10:32:06AM -0800, Brian Norris wrote:
> On Thu, Dec 19, 2019 at 7:48 AM Jouni Malinen <j@w1.fi> wrote:
> > On Thu, Dec 19, 2019 at 09:44:52AM +0000, Pkshih wrote:
> > > On Wed, 2019-12-18 at 17:48 +0200, Kalle Valo wrote:
> > > > diff --git a/include/uapi/nl80211-vnd-qca.h b/include/uapi/nl80211-vnd-qca.h
> > > > + * 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
> 
> By the way, I wonder -- why have this statement? That's not how I
> recall any other piece of kernel development ever working; upstream
> Linux defines the upstream Linux API, not some arbitrary user space
> project. This statement could be useful for saying, "don't stomp on
> those command numbers please," but the response should probably be to
> go out and define a totally new vendor ID or something. (See below.)

As far as the OUI 00:13:74 is concerned, the defined mechanism for
getting any new identifier values assigned is by getting a patch applied
into hostap.git. If another OUI is used, that can clearly use different
mechanism for this. Anyway, I'd expect the process for OUI 00:13:74 to
be the most convenient one to use.

This does not mean that there could not be new subcmds and attributes
defined as part of the upstream kernel review and those changes being
modified as part of that review. However, the final ID values for the
subcmds/attributes would happen through whatever mechanism defined for
the particular OUI. For 00:13:74, that would mean defining the
subcmds/attributes first with TBD ID values and once the definitions are
otherwise fine in the kernel patch review, contributing a patch to
hostap.git to get the identifiers assigned, updating the kernel patch to
use the assigned values, and apply it to the kernel.

> > Please note that the vendor subcmd and attributes used here are already
> > deployed and in use as a kernel interface. As such, the existing
> > attributes cannot really be modified; if anything else would be needed,
> > that would need to be defined as a new attribute and/or command.
> 
> Clarification: you're talking about out-of-tree drivers, which really
> have no relevance in upstream discussion, except possibly as examples.
> I don't think it's ever been a valid approach to dictate upstream
> kernel design based simply on "what $vendor already implemented for
> Android."

There is clearly no requirement to use an existing attribute, but since
there is such an attribute already defined, I'd claim it is perfectly
fine to consider it as an option for this. If something else is
identified to be needed, a new subcmd/attribute can obviously be
defined.

> Maybe it's a better idea to just use different command numbers (or
> vendor ID?) here, to avoid stomping on each others' implementation
> choices. Otherwise, it sounds like our only choice here is to copy
> your Android driver verbatim, or get lost.

I'd use the same OUI 00:13:74 since there is a defined process for
getting identifiers assigned from there for upstream Linux WLAN needs.
Defining other/new non-conflicting subcmds/attributes is of course fine
when needed.

> > This was discussed during the 2019 wireless workshop. The conclusion
> > from that discussion was that while there is clear need for SAR power
> > limits for various devices and multiple vendors/drivers, it did not look
> > clear that a single common interface could be defined cleanly taken into
> > account the differences in the ways vendors have designed the mechanism
> > in driver and firmware implementations. As such, vendor specific
> > commands were identified as the approach.
> 
> [citation needed]

I'm not aware of any publicly available meeting minutes that covered the
details for that discussion. My personal notes indicate that there were
at least two vendors indicating existence of vendor specific commands
for configuring SAR parameters, a discussion about the parameters used
for this being different, and a conclusion that this would be an example
kernel interface where a generic nl80211 interface may not be achievable
and a vendor specific interface would be more likely. This discussion
resulted in the discussion on how to use vendor specific nl80211
commands/attributes in upstream drivers and the eventual documentation
of that in the location you noted.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH 1/2] nl80211: vendor-cmd: qca: add dynamic SAR power limits

[Brian Norris]

On Thu, Dec 19, 2019 at 10:55 AM Jouni Malinen <j@w1.fi> wrote:
>
> On Thu, Dec 19, 2019 at 10:32:06AM -0800, Brian Norris wrote:
> > On Thu, Dec 19, 2019 at 7:48 AM Jouni Malinen <j@w1.fi> wrote:
> > > On Thu, Dec 19, 2019 at 09:44:52AM +0000, Pkshih wrote:
> > > > On Wed, 2019-12-18 at 17:48 +0200, Kalle Valo wrote:
> > > > > diff --git a/include/uapi/nl80211-vnd-qca.h b/include/uapi/nl80211-vnd-qca.h
> > > > > + * 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
> >
> > By the way, I wonder -- why have this statement? That's not how I
> > recall any other piece of kernel development ever working; upstream
> > Linux defines the upstream Linux API, not some arbitrary user space
> > project. This statement could be useful for saying, "don't stomp on
> > those command numbers please," but the response should probably be to
> > go out and define a totally new vendor ID or something. (See below.)
>
> As far as the OUI 00:13:74 is concerned, the defined mechanism for
> getting any new identifier values assigned is by getting a patch applied
> into hostap.git. If another OUI is used, that can clearly use different
> mechanism for this. Anyway, I'd expect the process for OUI 00:13:74 to
> be the most convenient one to use.

OK, I guess that makes a little more sense. But I'm not sure I agree
with the phrase "convenient."

> For 00:13:74, that would mean defining the
> subcmds/attributes first with TBD ID values and once the definitions are
> otherwise fine in the kernel patch review, contributing a patch to
> hostap.git to get the identifiers assigned, updating the kernel patch to
> use the assigned values, and apply it to the kernel.

That doesn't really sound convenient at all. But I suppose that's
beside the point, so I won't harp on that.

> > > Please note that the vendor subcmd and attributes used here are already
> > > deployed and in use as a kernel interface. As such, the existing
> > > attributes cannot really be modified; if anything else would be needed,
> > > that would need to be defined as a new attribute and/or command.
> >
> > Clarification: you're talking about out-of-tree drivers, which really
> > have no relevance in upstream discussion, except possibly as examples.
> > I don't think it's ever been a valid approach to dictate upstream
> > kernel design based simply on "what $vendor already implemented for
> > Android."
>
> There is clearly no requirement to use an existing attribute, but since
> there is such an attribute already defined, I'd claim it is perfectly
> fine to consider it as an option for this. If something else is
> identified to be needed, a new subcmd/attribute can obviously be
> defined.

Ack, thanks. I think I misinterpreted your intention to say, "I won't
take suggestions, because the attributes are already decided
elsewhere."

> > > This was discussed during the 2019 wireless workshop. The conclusion
> > > from that discussion was that while there is clear need for SAR power
> > > limits for various devices and multiple vendors/drivers, it did not look
> > > clear that a single common interface could be defined cleanly taken into
> > > account the differences in the ways vendors have designed the mechanism
> > > in driver and firmware implementations. As such, vendor specific
> > > commands were identified as the approach.
> >
> > [citation needed]
>
> I'm not aware of any publicly available meeting minutes that covered the
> details for that discussion. My personal notes indicate that there were
> at least two vendors indicating existence of vendor specific commands
> for configuring SAR parameters, a discussion about the parameters used
> for this being different, and a conclusion that this would be an example
> kernel interface where a generic nl80211 interface may not be achievable
> and a vendor specific interface would be more likely. This discussion
> resulted in the discussion on how to use vendor specific nl80211
> commands/attributes in upstream drivers and the eventual documentation
> of that in the location you noted.

Hmm, I actually think I was only around for the pre-discussion, in
which y'all suggested you might later meet to decide what eventually
became [1]. So maybe I missed some specific examples that would
provide the [citation] I requested.

That being said, I have personally fielded out-of-tree SAR
implementations from 4 different vendors:

(a) Two of them (this ath10k proposal, roughly; and Realtek's) employ
exactly the same concept: N frequency ranges, each with associated
power limits.
(b) Two of them (Intel/variant-of-iwiwifi and Marvell/mwifiex) utilize
a platform-specific (BIOS or Device Tree) mechanism for enumerating
power tables, and the nl80211 API simply takes an index N (e.g., 0 or
1), so user space can say "switch to mode N"

Unfortunately, for (b), I think there are enough reasons to think they
won't share an API similar to (a) (for Marvell, their
platform-specific tables are large undocumented blobs -- I have a
feeling if we already had a common API for (a), they *could* have
implemented some translation in a nicer way in their driver, but they
haven't chosen to do that work and probably won't be convinced to do
so).

But that still means there's some hope for (a).

Anyway, I am happy that there's a documented policy for vendor APIs
[1], and I'm happy to see this proposal out here. I just want to see a
critical eye put to this particular proposal if possible, to see if we
can improve its flexibility (either now, or in a later version of a
QCA vendor command, or even in a common nl80211-proper command).

So to put a little different spin on Pkshih's request: is there any
value in making this particular ath10k proposal a little more generic
(e.g., more granularity or flexibility in frequency bands, or more
precision in power limits), such that other vendors might implement
the same thing? Or would it be better to let each vendor implement
their similar-looking APIs (i.e., (a); or maybe even (b)) on their
own, and only later look at sharing?

Brian

[1] https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api

Re: [PATCH 1/2] nl80211: vendor-cmd: qca: add dynamic SAR power limits

[Kalle Valo]

Brian Norris <briannorris@chromium.org> writes:

>> > > This was discussed during the 2019 wireless workshop. The conclusion
>> > > from that discussion was that while there is clear need for SAR power
>> > > limits for various devices and multiple vendors/drivers, it did not look
>> > > clear that a single common interface could be defined cleanly taken into
>> > > account the differences in the ways vendors have designed the mechanism
>> > > in driver and firmware implementations. As such, vendor specific
>> > > commands were identified as the approach.
>> >
>> > [citation needed]
>>
>> I'm not aware of any publicly available meeting minutes that covered the
>> details for that discussion. My personal notes indicate that there were
>> at least two vendors indicating existence of vendor specific commands
>> for configuring SAR parameters, a discussion about the parameters used
>> for this being different, and a conclusion that this would be an example
>> kernel interface where a generic nl80211 interface may not be achievable
>> and a vendor specific interface would be more likely. This discussion
>> resulted in the discussion on how to use vendor specific nl80211
>> commands/attributes in upstream drivers and the eventual documentation
>> of that in the location you noted.
>
> Hmm, I actually think I was only around for the pre-discussion, in
> which y'all suggested you might later meet to decide what eventually
> became [1]. So maybe I missed some specific examples that would
> provide the [citation] I requested.
>
> That being said, I have personally fielded out-of-tree SAR
> implementations from 4 different vendors:
>
> (a) Two of them (this ath10k proposal, roughly; and Realtek's) employ
> exactly the same concept: N frequency ranges, each with associated
> power limits.
> (b) Two of them (Intel/variant-of-iwiwifi and Marvell/mwifiex) utilize
> a platform-specific (BIOS or Device Tree) mechanism for enumerating
> power tables, and the nl80211 API simply takes an index N (e.g., 0 or
> 1), so user space can say "switch to mode N"
>
> Unfortunately, for (b), I think there are enough reasons to think they
> won't share an API similar to (a) (for Marvell, their
> platform-specific tables are large undocumented blobs -- I have a
> feeling if we already had a common API for (a), they *could* have
> implemented some translation in a nicer way in their driver, but they
> haven't chosen to do that work and probably won't be convinced to do
> so).
>
> But that still means there's some hope for (a).
>
> Anyway, I am happy that there's a documented policy for vendor APIs
> [1], and I'm happy to see this proposal out here. I just want to see a
> critical eye put to this particular proposal if possible, to see if we
> can improve its flexibility (either now, or in a later version of a
> QCA vendor command, or even in a common nl80211-proper command).
>
> So to put a little different spin on Pkshih's request: is there any
> value in making this particular ath10k proposal a little more generic
> (e.g., more granularity or flexibility in frequency bands, or more
> precision in power limits), such that other vendors might implement
> the same thing? Or would it be better to let each vendor implement
> their similar-looking APIs (i.e., (a); or maybe even (b)) on their
> own, and only later look at sharing?

The downside of accepting SAR vendor commands to upstream is that (in
theory) that should be supported a long time:

  4. The newly proposed API shall be subject to stable API rules.

  https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api

So if sometime in the future we add a generic command the driver would
need to support both vendor and generic commands. So, IF it makes sense
to implement a generic command, I would rather have a generic command
implemented from the beginning and drop the SAR vendor command patches
altogether.

For me either solutions are good enough, I'm not familiar enough with
all the different SAR user space interfaces to make a good decision.

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

Re: [PATCH 1/2] nl80211: vendor-cmd: qca: add dynamic SAR power limits

[Johannes Berg]

On Tue, 2020-03-17 at 18:54 +0200, Kalle Valo wrote:

> For me either solutions are good enough, I'm not familiar enough with
> all the different SAR user space interfaces to make a good decision.

Brian probably has most insight into this :-)

I really didn't want to have to be the referee here, I was hoping you'd
figure this all out between yourselves... oh well.

But as somebody has said on one of these threads, there seem to
basically be two kinds of APIs:

 1) some kind of platform-dependent index into a table that the
    driver/device has, or perhaps the BIOS; and

 2) some kind of per-band (FSVO band) power restriction like here.


The first is like iwlwifi, and I think Marvell was mentioned? But
they're basically out - there's no information, and there's no clue as
to which indices might even be valid, I think, nor what they mean. So
there isn't really much value in a common API for that since you can't
use it in a common fashion - arguably a common API would be worse...


However, the case of 2, arguably the proposals are very similar?

Qualcomm: optional nl80211_band, 1/2 dBm units
Realtek: 2.4, four 5 GHz subbands, 1/4 dBm units

Both have some strange namespace thing where the same namespace contains
both the outer and inner attributes. Probably should think about the
policy with NLA_POLICY_NESTED and see how that works.


But it any case, these two don't seem like an insurmountable issue to
combine? Say, something like defining a list of affected frequency
ranges in the wiphy properties, and then giving a list of TX powers that
matches the list of frequency ranges? We can go to 1/4 dBm or so, that's
not such a big deal, I'd think?


On the other hand, what does that actually buy us? If you cannot have
common userspace that knows how a given platform must behave, then it's
not very worthwhile to have common API for it?

Brian, what do you think from a platform/userspace perspective - how do
you actually determine the SAR limits? I'm guessing you just have a
table of sorts, but how do you get the table? Would you actually have
common userspace and benefit from having common API?

johannes

Re: [PATCH 2/2] ath10k: allow dynamic SAR power limits to be configured

[Kalle Valo]

Kalle Valo <kvalo@codeaurora.org> wrote:

> From: Wen Gong <wgong@codeaurora.org>
> 
> Add support for a vendor command for STATION, the command
> QCA_NL80211_VENDOR_SUBCMD_SET_SAR_LIMITS which is already defined in
> git://w1.fi/hostap.git (src/command/qca-vendor.h). This allows user
> space to configure power limits for 2.4 GHz and 5 GHz bands.
> 
> ath10k set pdev parameter WMI_PDEV_PARAM_TXPOWER_LIMIT2G and
> WMI_PDEV_PARAM_TXPOWER_LIMIT5G to firmware, the 2 value will
> be used as one input source to affect the tx power.
> 
> When QCA_NL80211_VENDOR_SUBCMD_SET_SAR_LIMITS set to ath10k, it will
> be saved the 2.4G and 5G limit value, If STATION is connected meanwhile,
> then the 2.4G and 5G WMI command will be set to firmware, otherwise
> it will not set to firmware at this moment. When STATION connect
> next time, it will set to firmware.
> 
> Tested with QCA6174 SDIO with firmware WLAN.RMH.4.4.1-00029.
> 
> Signed-off-by: Wen Gong <wgong@codeaurora.org>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

I'll drop this from my queue now and will resend once there's a
concensus on what interface to use.

Patch set to Changes Requested.

-- 
https://patchwork.kernel.org/patch/11301107/

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

Re: [PATCH 1/2] nl80211: vendor-cmd: qca: add dynamic SAR power limits

[Brian Norris]

I haven't quite reached the 3 month lag on the prior replies here :)

I do want to see this question resolved somehow, or else we'll be
dragging along out-of-tree patches for...(counts on fingers)...4
different nl80211 APIs for the same feature, and a driver-detecting
user space for it. I think I was half-hoping that we'd get to chat at
netdev about this, but that's not happening any time soon. (Topic for
another day: does a "wireless workshop" still happen at a virtual-only
conference?)

On Fri, Mar 20, 2020 at 5:56 AM Johannes Berg <johannes@sipsolutions.net> wrote:
> But as somebody has said on one of these threads, there seem to
> basically be two kinds of APIs:
>
>  1) some kind of platform-dependent index into a table that the
>     driver/device has, or perhaps the BIOS; and
>
>  2) some kind of per-band (FSVO band) power restriction like here.
>
>
> The first is like iwlwifi, and I think Marvell was mentioned? But
> they're basically out - there's no information, and there's no clue as
> to which indices might even be valid, I think, nor what they mean. So
> there isn't really much value in a common API for that since you can't
> use it in a common fashion - arguably a common API would be worse...

I think they could still be salvageable. In practice, the only way
I've seen them used is to define a "low power" and a "high power"
table. So if there were conventions (e.g., Marvell's could be
documented in Device Tree, where they store their SAR tables), we
could expose an easy high/low knob.

> However, the case of 2, arguably the proposals are very similar?
>
> Qualcomm: optional nl80211_band, 1/2 dBm units
> Realtek: 2.4, four 5 GHz subbands, 1/4 dBm units

nit: I believe it's 1/8 for Realtek. And I thought it was 1 for
Qualcomm, but apparently this changes every week I see a new
(non-upstream-facing) submission from them...which is a part of the
problem I'd like to solve. They don't have to commit to anything if
it's all downstream ;)

> Both have some strange namespace thing where the same namespace contains
> both the outer and inner attributes. Probably should think about the
> policy with NLA_POLICY_NESTED and see how that works.

Nobody really accused vendors of being good at writing netlink APIs :)
And incidentally, that's another reason why I'd rather not just keep
taking vendor command APIs downstream...

> But it any case, these two don't seem like an insurmountable issue to
> combine?

Exactly my point. And that's why I didn't feel like we really got past #2 [0]:
"The submitter shall consider a pure nl80211 implementation and
explain the reasoning against it in the commit message."

> Say, something like defining a list of affected frequency
> ranges in the wiphy properties, and then giving a list of TX powers that
> matches the list of frequency ranges? We can go to 1/4 dBm or so, that's
> not such a big deal, I'd think?

That sounds about right. We'd probably want a reporting API too, so
drivers can tell what ranges they support, so user space doesn't have
to get creative about handling EINVAL or similar.

I've also seen the Qualcomm proposal include a per-chain option
(although it's missing from QCA6174) so it might be reasonable to
account for that up-front (similar to STA_INFO_CHAIN_SIGNAL?).

One note: while I can imagine that vendors might have other
configurability parameters that aren't reflected in this proposal
(e.g., per-PHY-mode? per-chain?), I've also found that some of the
complexities that vendors offer don't *really* end up being needed by
ODMs/OEMs when building and certifying devices.

> On the other hand, what does that actually buy us? If you cannot have
> common userspace that knows how a given platform must behave, then it's
> not very worthwhile to have common API for it?
>
> Brian, what do you think from a platform/userspace perspective - how do
> you actually determine the SAR limits? I'm guessing you just have a
> table of sorts, but how do you get the table? Would you actually have
> common userspace and benefit from having common API?

This strikes at the sort of thing where Chrome OS tailors the
experience to the hardware, whereas the average Linux distro does not.
So I can describe what we do, but it's tough to say how useful this
ends up being to everyone else. But maybe that's beside the point,
because your average distro will probably not know how to hook up SAR
regardless (but much less so if there's no mainline API for it).

Anyway, the table: Chrome OS has a generalized per-SKU configuration
system, which keys off of a thing called "SKU ID." A number of
factors, including board-level straps or EEPROMs, come together to
determine which model and SKU a given system is. Once the config
system knows that, it can retrieve the data table from its database
(it's just a giant JSON blob). NB: while we currently ship separate
builds (with separate JSON configs) for separate families of systems
(think SoC families), the config system *could* work as a single table
for all Chrome OS systems.

Currently, because each vendor implements this differently enough, we
have separate schema for each of the 2 drivers [1]. But we could just
as well unify this into a range-based system to match the range
proposal above.

Common user space: today, we have a single user space; it's just ugly
[2]. Some of that ugliness is due to the unnecessary divergence in
range-based APIs, which probably would clean up a little if we
supported a common range API. (We'd still be left with our Marvell and
Intel index approaches.)

Really, I could live with per-vendor APIs. My primary goal is to get
these upstream in some form, so vendors can stop using things like
this as a reason for shipping us non-upstream code, and so we can
reduce the delta between upstream and Chrome OS kernels.

I also think that, for the cases that warrant it (i.e., the option 2
-- Realtek and Qualcomm cases, so far), it would be good to have a
common API, but that's a somewhat secondary concern for me.

Also, Kalle had some concerns about stable ABI questions: shouldn't we
bake in some kind of "check for support" feature to these kinds of
APIs [3]? That would help ease transition, if we do start with a
vendor API and move to a common one in the future. Given the nature of
this feature, I wouldn't expect there will be a large variety of users
making use of the APIs -- and I, for one, would be happy to migrate my
user space over some period of time, as needed.

Brian

[0] https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api
[1] https://chromium.googlesource.com/chromiumos/platform2/+/master/chromeos-config/README.md#wifi
[2] https://chromium.googlesource.com/chromiumos/platform2/+/master/power_manager/powerd/set_wifi_transmit_power.cc
[3] e.g., we ran into the lack of such support here:
7010998c6caf nl80211: add NL80211_CMD_UPDATE_FT_IES to supported commands

Re: [PATCH 1/2] nl80211: vendor-cmd: qca: add dynamic SAR power limits

[Kalle Valo]

Brian Norris <briannorris@chromium.org> writes:

> Really, I could live with per-vendor APIs. My primary goal is to get
> these upstream in some form, so vendors can stop using things like
> this as a reason for shipping us non-upstream code, and so we can
> reduce the delta between upstream and Chrome OS kernels.
>
> I also think that, for the cases that warrant it (i.e., the option 2
> -- Realtek and Qualcomm cases, so far), it would be good to have a
> common API, but that's a somewhat secondary concern for me.

So to me it feels like the best solution forward is to go with the
vendor API, do you agree? We can, of course, later switch to the common
API if we manage to create one which is usable for everyone.

> Also, Kalle had some concerns about stable ABI questions: shouldn't we
> bake in some kind of "check for support" feature to these kinds of
> APIs [3]? That would help ease transition, if we do start with a
> vendor API and move to a common one in the future.

Yeah, that sounds like a good idea but I don't think that should block
these patches.

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

Re: [PATCH 1/2] nl80211: vendor-cmd: qca: add dynamic SAR power limits

[Brian Norris]

On Thu, Jul 16, 2020 at 2:35 AM Kalle Valo <kvalo@codeaurora.org> wrote:
> Brian Norris <briannorris@chromium.org> writes:
> > Really, I could live with per-vendor APIs. My primary goal is to get
> > these upstream in some form, so vendors can stop using things like
> > this as a reason for shipping us non-upstream code, and so we can
> > reduce the delta between upstream and Chrome OS kernels.
> >
> > I also think that, for the cases that warrant it (i.e., the option 2
> > -- Realtek and Qualcomm cases, so far), it would be good to have a
> > common API, but that's a somewhat secondary concern for me.
>
> So to me it feels like the best solution forward is to go with the
> vendor API, do you agree? We can, of course, later switch to the common
> API if we manage to create one which is usable for everyone.

That's fine with me. That's pretty much what I said in my first email:

"Anyway, I don't really object with starting out with a
Qualcomm-specific and a Realtek-specific vendor command to implement
nearly the same feature, but I'd prefer if people did engage in some
healthy discussion about why they can't share an API, with the hopes
that maybe they can converge someday."

I think we've had some healthy (though very protracted) discussion,
and I don't think I've seen anyone bring up anything I wasn't already
aware of that would prevent eventual consolidation. As long as we
acknowledge those things (item 2 at
https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api),
I'm happy.

> > Also, Kalle had some concerns about stable ABI questions: shouldn't we
> > bake in some kind of "check for support" feature to these kinds of
> > APIs [3]? That would help ease transition, if we do start with a
> > vendor API and move to a common one in the future.
>
> Yeah, that sounds like a good idea but I don't think that should block
> these patches.

OK, well it was your concern first, IIRC :)

So what's next? A v2 that only needs a bit of updated description
about "why a vendor API"? And Realtek can feel free to submit this [1]
shortly?

Thanks,
Brian

[1] Series: rtw88: Add SAR implementation
https://patchwork.kernel.org/project/linux-wireless/list/?series=238219&state=*

Re: [PATCH 1/2] nl80211: vendor-cmd: qca: add dynamic SAR power limits

[Kalle Valo]

Brian Norris <briannorris@chromium.org> writes:

> On Thu, Jul 16, 2020 at 2:35 AM Kalle Valo <kvalo@codeaurora.org> wrote:
>> Brian Norris <briannorris@chromium.org> writes:
>> > Really, I could live with per-vendor APIs. My primary goal is to get
>> > these upstream in some form, so vendors can stop using things like
>> > this as a reason for shipping us non-upstream code, and so we can
>> > reduce the delta between upstream and Chrome OS kernels.
>> >
>> > I also think that, for the cases that warrant it (i.e., the option 2
>> > -- Realtek and Qualcomm cases, so far), it would be good to have a
>> > common API, but that's a somewhat secondary concern for me.
>>
>> So to me it feels like the best solution forward is to go with the
>> vendor API, do you agree? We can, of course, later switch to the common
>> API if we manage to create one which is usable for everyone.
>
> That's fine with me. That's pretty much what I said in my first email:
>
> "Anyway, I don't really object with starting out with a
> Qualcomm-specific and a Realtek-specific vendor command to implement
> nearly the same feature, but I'd prefer if people did engage in some
> healthy discussion about why they can't share an API, with the hopes
> that maybe they can converge someday."
>
> I think we've had some healthy (though very protracted) discussion,
> and I don't think I've seen anyone bring up anything I wasn't already
> aware of that would prevent eventual consolidation. As long as we
> acknowledge those things (item 2 at
> https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api),
> I'm happy.

Good, I was just checking that we all are on the same page.

>> > Also, Kalle had some concerns about stable ABI questions: shouldn't we
>> > bake in some kind of "check for support" feature to these kinds of
>> > APIs [3]? That would help ease transition, if we do start with a
>> > vendor API and move to a common one in the future.
>>
>> Yeah, that sounds like a good idea but I don't think that should block
>> these patches.
>
> OK, well it was your concern first, IIRC :)

I was commenting about the "check for support" feature :) I think it
would be a good idea to have userspace check what vendor interface
features/commands are supported with that driver/hardware/firmware
combo. But how should that be implemented? Should there be a some kind
of generic mechanism used by all drivers or would each driver have their
own method to check the supported features? I think that needs a
separate discussion.

> So what's next? A v2 that only needs a bit of updated description
> about "why a vendor API"?

I'm busy but hopefully Wen (CCed) can submit v2. Wen?

> And Realtek can feel free to submit this [1] shortly?

> [1] Series: rtw88: Add SAR implementation
> https://patchwork.kernel.org/project/linux-wireless/list/?series=238219&state=*

Yeah, please submit that as well.

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

Re: [PATCH 1/2] nl80211: vendor-cmd: qca: add dynamic SAR power limits

[Johannes Berg]

On Mon, 2020-06-01 at 18:32 -0700, Brian Norris wrote:
> I haven't quite reached the 3 month lag on the prior replies here :)

But I did, almost ;-)

> I do want to see this question resolved somehow, or else we'll be
> dragging along out-of-tree patches for...(counts on fingers)...4
> different nl80211 APIs for the same feature, and a driver-detecting
> user space for it.

Right.

> I think I was half-hoping that we'd get to chat at
> netdev about this, but that's not happening any time soon. (Topic for
> another day: does a "wireless workshop" still happen at a virtual-only
> conference?)

Yes, but only a short 1.5hr video call I guess. I'm not even sure what
to do there yet.

[snip]

Thanks for the overview!

> Also, Kalle had some concerns about stable ABI questions: shouldn't we
> bake in some kind of "check for support" feature to these kinds of
> APIs [3]? That would help ease transition, if we do start with a
> vendor API and move to a common one in the future. Given the nature of
> this feature, I wouldn't expect there will be a large variety of users
> making use of the APIs -- and I, for one, would be happy to migrate my
> user space over some period of time, as needed.

We do that? We list out the available vendor APIs, and in most cases
(perhaps not the one you've noticed there) we also list out the
availability of real nl80211 APIs in some way.

johannes

Re: [PATCH 1/2] nl80211: vendor-cmd: qca: add dynamic SAR power limits

[Johannes Berg]

On Fri, 2020-07-24 at 12:26 +0300, Kalle Valo wrote:

> > > So to me it feels like the best solution forward is to go with the
> > > vendor API, do you agree? We can, of course, later switch to the common
> > > API if we manage to create one which is usable for everyone.

But why wouldn't we try that now, while we have it all in our heads (in
a way ... even if this discussion drags out forever)?

I mean, the range-based approach ought to work, and if we define it as a
nested attribute list or so, we can even later add more attributes to it
(chain limits, whatnot) without any backward compatibility concerns.

So what is it that we _cannot_ do in a more common way today?

> > I think we've had some healthy (though very protracted) discussion,
> > and I don't think I've seen anyone bring up anything I wasn't already
> > aware of that would prevent eventual consolidation. As long as we
> > acknowledge those things (item 2 at
> > https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api),
> > I'm happy.
> 
> Good, I was just checking that we all are on the same page.

But are we? ;-)

I don't really see anything in the new proposal [1] that really explains
why the common API that we've sort of vaguely outlined in this thread
couldn't work? It just speaks of technical difficulties ("need a
reporting API too"), but should we let that stop us?

[1] https://patchwork.kernel.org/patch/11686317/


johannes

Re: [PATCH 1/2] nl80211: vendor-cmd: qca: add dynamic SAR power limits

[Brian Norris]

On Thu, Jul 30, 2020 at 6:24 AM Johannes Berg <johannes@sipsolutions.net> wrote:

> > Good, I was just checking that we all are on the same page.
>
> But are we? ;-)

I think you were deferring to, "how would user space use it?" And,
"would a common API really help anyone?" (And then you implied
"anyone" = "Chrome OS.")

I expressed a moderate benefit from a common API, but I'd settle for a
non-common (but upstream) one instead. I also doubt many
non-hardware-customized Linux distributions will make use of this any
time soon.

I don't think that implied you were truly on a different page. But
your response below may say otherwise:

> I don't really see anything in the new proposal [1] that really explains
> why the common API that we've sort of vaguely outlined in this thread
> couldn't work? It just speaks of technical difficulties ("need a
> reporting API too"), but should we let that stop us?
>
> [1] https://patchwork.kernel.org/patch/11686317/

Indeed, I don't see any reason beyond technical difficulties.

I'd love to have a few extra hours in my day to spend on writing such
an API, if that would really unblock months of deadlock.
Unfortunately, those hours tend to get eaten by all sorts of other
things these days, so an honest assessment would probably say I won't
get around to it soon. SAR regulatory concerns aren't really sexy
enough to convince me to spend my weekends on it... (...oh wait, it's
Friday evening already. Hmm.)

Regards,
Brian

Re: [PATCH 1/2] nl80211: vendor-cmd: qca: add dynamic SAR power limits

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Fri, 2020-07-24 at 12:26 +0300, Kalle Valo wrote:
>
>> > > So to me it feels like the best solution forward is to go with the
>> > > vendor API, do you agree? We can, of course, later switch to the common
>> > > API if we manage to create one which is usable for everyone.
>
> But why wouldn't we try that now, while we have it all in our heads (in
> a way ... even if this discussion drags out forever)?
>
> I mean, the range-based approach ought to work, and if we define it as a
> nested attribute list or so, we can even later add more attributes to it
> (chain limits, whatnot) without any backward compatibility concerns.
>
> So what is it that we _cannot_ do in a more common way today?
>
>> > I think we've had some healthy (though very protracted) discussion,
>> > and I don't think I've seen anyone bring up anything I wasn't already
>> > aware of that would prevent eventual consolidation. As long as we
>> > acknowledge those things (item 2 at
>> > https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api),
>> > I'm happy.
>> 
>> Good, I was just checking that we all are on the same page.
>
> But are we? ;-)
>
> I don't really see anything in the new proposal [1] that really explains
> why the common API that we've sort of vaguely outlined in this thread
> couldn't work? It just speaks of technical difficulties ("need a
> reporting API too"), but should we let that stop us?
>
> [1] https://patchwork.kernel.org/patch/11686317/

I misunderstood then, I thought everyone were leaning towards the vendor
API approach. But yeah, of course a common API is much better if people
think it's doable.

So I'll now drop all the vendor API patches from patchwork and
assume/hope that we will get the common API at some point.

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