[RFC 1/2] nl80211: add common API to configure SAR power limitations.

[RFC 1/2] nl80211: add common API to configure SAR power limitations.

[Carl Huang]

NL80211_CMD_SET_SAR_SPECS is added to configure SAR from
user space. NL80211_ATTR_SAR_SPEC is used to pass the SAR
power specification when used with NL80211_CMD_SET_SAR_SPECS.

Wireless driver needs to register SAR type, supported frequency
ranges to wiphy, so user space can query it. The index in
frequency range is used to specify which sub band the power
limitation applies to. The SAR type is for compatibility, so later
other SAR mechanism can be implemented without breaking the user
space SAR applications.

Normal process is user space quries the SAR capability, and
gets the index of supported frequency ranges and associates the
power limitation with this index and sends to kernel.

Here is an example of message send to kernel:
8c 00 00 00 08 00 03 00 15 00 00 00 38 00 26 81
08 00 01 00 00 00 00 00 2c 00 02 80 14 00 01 80
05 00 02 00 00 00 00 00 05 00 01 00 38 00 00 00
14 00 02 80 05 00 02 00 01 00 00 00 05 00 01 00
48 00 00 00

NL80211_CMD_SET_SAR_SPECS:  0x8c
NL80211_ATTR_SAR_SPEC:  0x8126 (NLA_NESTED)
NL80211_SAR_ATTR_TYPE:  0x00 (NL80211_SAR_TYPE_POWER)
NL80211_SAR_ATTR_SPECS: 0x8002 (NLA_NESTED)
freq range 0 power: 0x38 in 0.25dbm unit (14dbm)
freq range 1 power: 0x48 in 0.25dbm unit (18dbm)

Signed-off-by: Carl Huang <cjhuang@codeaurora.org>
---
 include/net/cfg80211.h       |  52 +++++++++++++
 include/net/mac80211.h       |   2 +
 include/uapi/linux/nl80211.h |  86 +++++++++++++++++++++
 net/mac80211/cfg.c           |  12 +++
 net/wireless/nl80211.c       | 173 +++++++++++++++++++++++++++++++++++++++++++
 net/wireless/rdev-ops.h      |  12 +++
 net/wireless/trace.h         |  19 +++++
 7 files changed, 356 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index c9bce9b..94f09dd 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1663,6 +1663,55 @@ struct station_info {
 	u8 connected_to_as;
 };
 
+/**
+ * struct cfg80211_sar_sub_specs - sub specs limit
+ * @power: value in 0.25dbm
+ * @freq_range_index: index the power limitation applies to
+ */
+struct cfg80211_sar_sub_specs {
+	u8 power;
+	u8 freq_range_index;
+};
+
+/**
+ * struct cfg80211_sar_specs - sar limit specs
+ * @type: it's set with power in 0.25dbm or other types
+ * @num_sub_specs: number of sar sub specs
+ * @sub_specs: memory to hold the sar sub specs
+ */
+struct cfg80211_sar_specs {
+	enum nl80211_sar_type type;
+	u16 num_sub_specs;
+	struct cfg80211_sar_sub_specs *sub_specs;
+};
+
+
+/**
+ * @struct cfg80211_sar_chan_ranges - sar frequency ranges
+ * @index: the index of this range. It's used to specify
+ *	the frequency range when setting SAR power limitation
+ * @start_freq:  start channel frequency in kHZ. For example,
+ *	2.4G channel 1 is represented as 2412000
+ * @end_freq:    end channel frequency in kHZ
+ */
+struct cfg80211_sar_freq_ranges {
+	u8 index;
+	u32 start_freq;
+	u32 end_freq;
+};
+
+/**
+ * struct cfg80211_sar_capa - sar limit capability
+ * @type: it's set via power in 0.25dbm or other types
+ * @num_freq_ranges: number of frequency ranges
+ * @chan_ranges: memory to hold the channel ranges.
+ */
+struct cfg80211_sar_capa {
+	enum nl80211_sar_type type;
+	u8 num_freq_ranges;
+	const struct cfg80211_sar_freq_ranges *freq_ranges;
+};
+
 #if IS_ENABLED(CONFIG_CFG80211)
 /**
  * cfg80211_get_station - retrieve information about a given station
@@ -4153,6 +4202,7 @@ struct cfg80211_ops {
 				  struct cfg80211_tid_config *tid_conf);
 	int	(*reset_tid_config)(struct wiphy *wiphy, struct net_device *dev,
 				    const u8 *peer, u8 tids);
+	int	(*set_sar_specs)(struct wiphy *wiphy, struct cfg80211_sar_specs *sar);
 };
 
 /*
@@ -4919,6 +4969,8 @@ struct wiphy {
 
 	u8 max_data_retry_count;
 
+	const struct cfg80211_sar_capa *sar_capa;
+
 	char priv[] __aligned(NETDEV_ALIGN);
 };
 
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index ec148b3..df758ee 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -4125,6 +4125,8 @@ struct ieee80211_ops {
 	int (*reset_tid_config)(struct ieee80211_hw *hw,
 				struct ieee80211_vif *vif,
 				struct ieee80211_sta *sta, u8 tids);
+	int (*set_sar_specs)(struct ieee80211_hw *hw,
+			     const struct cfg80211_sar_specs *sar);
 };
 
 /**
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 0584e0d..2dcfed4 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1177,6 +1177,9 @@
  *	includes the contents of the frame. %NL80211_ATTR_ACK flag is included
  *	if the recipient acknowledged the frame.
  *
+ * @NL80211_CMD_SET_SAR_SPECS: SAR power limitation configuration is
+ *	passed using %NL80211_ATTR_SAR_SPEC.
+ *
  * @NL80211_CMD_MAX: highest used command number
  * @__NL80211_CMD_AFTER_LAST: internal use
  */
@@ -1407,6 +1410,8 @@ enum nl80211_commands {
 
 	NL80211_CMD_CONTROL_PORT_FRAME_TX_STATUS,
 
+	NL80211_CMD_SET_SAR_SPECS,
+
 	/* add new commands above here */
 
 	/* used to define NL80211_CMD_MAX below */
@@ -2513,6 +2518,10 @@ enum nl80211_commands {
  * @NL80211_ATTR_HE_6GHZ_CAPABILITY: HE 6 GHz Band Capability element (from
  *	association request when used with NL80211_CMD_NEW_STATION).
  *
+ * @NL80211_ATTR_SAR_SPEC: SAR power limitation specification when
+ *	used with %NL80211_CMD_SET_SAR_SPECS. It contains array of
+ *	%nl80211_sar_specs_attrs.
+ *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -2995,6 +3004,8 @@ enum nl80211_attrs {
 
 	NL80211_ATTR_HE_6GHZ_CAPABILITY,
 
+	NL80211_ATTR_SAR_SPEC,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
@@ -7004,4 +7015,79 @@ enum nl80211_iftype_akm_attributes {
 	NL80211_IFTYPE_AKM_ATTR_MAX = __NL80211_IFTYPE_AKM_ATTR_LAST - 1,
 };
 
+/**
+ * enum nl80211_sar_type - type of SAR specs
+ *
+ * @NL80211_SAR_TYPE_POWER: the value is specified in 0.25 dbm
+ *
+ */
+enum nl80211_sar_type {
+	NL80211_SAR_TYPE_POWER,
+
+	/* add new type here */
+
+	/* Keep last */
+	NUM_NL80211_SAR_TYPE,
+};
+
+/**
+ * nl80211_sar_attrs - Attributes for SAR spec
+ *
+ * @NL80211_SAR_ATTR_TYPE: Type of SAR: power or index
+ *
+ * @NL80211_SAR_ATTR_SPECS: Nested array of SAR power
+ *	limit specifications. Each specification contains a set
+ *      of %nl80211_sar_specs_attrs
+ *
+ * @__NL80211_SAR_ATTR_LAST: Internal
+ * @NL80211_SAR_ATTR_MAX: highest sar attribute
+ *
+ * These attributes are used with %NL80211_CMD_SET_SAR_SPEC
+ */
+enum nl80211_sar_attrs {
+	__NL80211_SAR_ATTR_INVALID,
+
+	NL80211_SAR_ATTR_TYPE,
+	NL80211_SAR_ATTR_SPECS,
+
+	__NL80211_SAR_ATTR_LAST,
+	NL80211_SAR_ATTR_MAX = __NL80211_SAR_ATTR_LAST - 1,
+};
+
+#define NL80211_SAR_ALL_FREQ_RNAGES	0xff
+#define NUM_MAX_NL80211_SAR_FREQ_RANGES 0xfe
+
+/**
+ * nl80211_sar_specs_attrs - Attributes for SAR power limit specs
+ *
+ * @NL80211_SAR_ATTR_SPECS_POWER: Required (u32)value to specify the actual
+ *	power limit value in units of 0.25 dBm if type is
+ *	NL80211_SAR_TYPE_POWER. (i.e., a value of 44 represents 11 dBm)
+ *
+ * @NL80211_SAR_ATTR_SPECS_FREQ_RANGES_INDEX: optional (u32) value to specify the
+ *	index of exported freq ranges table. If this attribute is not present, then
+ *	the power is applied to all freq ranges, i.e, all bands
+ *
+ * @NL80211_SAR_ATTR_SPECS_START_FREQ: Required (u32) value to specify the start
+ *	frequency of this range to register SAR capability to wihpy and the unit
+ *	is kHZ
+ *
+ * @NL80211_SAR_ATTR_SPECS_END_FREQ: Required (u32) value to specify the end frequency
+ *	of this range to register SAR capability to wiphy and the unit is kHZ
+ *
+ * @__NL80211_SAR_ATTR_SPECS_LAST: Internal
+ * @NL80211_SAR_ATTR_SPECS_MAX: highest sar specs attribute
+ */
+enum nl80211_sar_specs_attrs {
+	__NL80211_SAR_ATTR_SPECS_INVALID,
+
+	NL80211_SAR_ATTR_SPECS_POWER,
+	NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX,
+	NL80211_SAR_ATTR_SPECS_START_FREQ,
+	NL80211_SAR_ATTR_SPECS_END_FREQ,
+
+	__NL80211_SAR_ATTR_SPECS_LAST,
+	NL80211_SAR_ATTR_SPECS_MAX = __NL80211_SAR_ATTR_SPECS_LAST - 1,
+};
+
 #endif /* __LINUX_NL80211_H */
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index b4e39e3..cc74322 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3993,6 +3993,17 @@ static int ieee80211_reset_tid_config(struct wiphy *wiphy,
 	return ret;
 }
 
+static int ieee80211_set_sar_specs(struct wiphy *wiphy,
+				   struct cfg80211_sar_specs *sar)
+{
+	struct ieee80211_local *local = wiphy_priv(wiphy);
+
+	if (!local->ops->set_sar_specs)
+		return -EOPNOTSUPP;
+
+	return local->ops->set_sar_specs(&local->hw, sar);
+}
+
 const struct cfg80211_ops mac80211_config_ops = {
 	.add_virtual_intf = ieee80211_add_iface,
 	.del_virtual_intf = ieee80211_del_iface,
@@ -4096,4 +4107,5 @@ const struct cfg80211_ops mac80211_config_ops = {
 	.probe_mesh_link = ieee80211_probe_mesh_link,
 	.set_tid_config = ieee80211_set_tid_config,
 	.reset_tid_config = ieee80211_reset_tid_config,
+	.set_sar_specs = ieee80211_set_sar_specs,
 };
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 52a35e7..9318b3a 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -367,6 +367,19 @@ nl80211_tid_config_attr_policy[NL80211_TID_CONFIG_ATTR_MAX + 1] = {
 			NLA_POLICY_NESTED(nl80211_txattr_policy),
 };
 
+static const struct nla_policy
+sar_specs_policy[NL80211_SAR_ATTR_SPECS_MAX + 1] = {
+		[NL80211_SAR_ATTR_SPECS_POWER] = { .type = NLA_U8 },
+		[NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX] =
+			NLA_POLICY_MAX(NLA_U8, NUM_MAX_NL80211_SAR_FREQ_RANGES),
+};
+
+static const struct nla_policy
+sar_policy[NL80211_SAR_ATTR_MAX + 1] = {
+	[NL80211_SAR_ATTR_TYPE] = NLA_POLICY_MAX(NLA_U32, NUM_NL80211_SAR_TYPE),
+	[NL80211_SAR_ATTR_SPECS] = NLA_POLICY_NESTED_ARRAY(sar_specs_policy),
+};
+
 static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
 	[0] = { .strict_start_type = NL80211_ATTR_HE_OBSS_PD },
 	[NL80211_ATTR_WIPHY] = { .type = NLA_U32 },
@@ -675,6 +688,7 @@ static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
 	[NL80211_ATTR_SCAN_FREQ_KHZ] = { .type = NLA_NESTED },
 	[NL80211_ATTR_HE_6GHZ_CAPABILITY] =
 		NLA_POLICY_EXACT_LEN(sizeof(struct ieee80211_he_6ghz_capa)),
+	[NL80211_ATTR_SAR_SPEC] = NLA_POLICY_NESTED(sar_policy),
 };
 
 /* policy for the key attributes */
@@ -1856,6 +1870,8 @@ static int nl80211_add_commands_unsplit(struct cfg80211_registered_device *rdev,
 			goto nla_put_failure;
 	}
 
+	CMD(set_sar_specs, SET_SAR_SPECS);
+
 	return i;
  nla_put_failure:
 	return -ENOBUFS;
@@ -2034,6 +2050,55 @@ nl80211_put_tid_config_support(struct cfg80211_registered_device *rdev,
 	return -ENOBUFS;
 }
 
+static int
+nl80211_put_sar_specs(struct cfg80211_registered_device *rdev,
+		      struct sk_buff *msg)
+{
+	struct nlattr *sar_capa, *specs, *sub_freq_range;
+	u8  num_freq_ranges;
+	int i;
+
+	if (!rdev->wiphy.sar_capa)
+		return 0;
+
+	num_freq_ranges = rdev->wiphy.sar_capa->num_freq_ranges;
+
+	sar_capa = nla_nest_start(msg, NL80211_ATTR_SAR_SPEC);
+	if (!sar_capa)
+		return -ENOSPC;
+
+	if (nla_put_u16(msg, NL80211_SAR_ATTR_TYPE, rdev->wiphy.sar_capa->type))
+		goto fail;
+
+	specs = nla_nest_start_noflag(msg, NL80211_SAR_ATTR_SPECS);
+	if (!specs)
+		goto fail;
+
+	/* report supported freq_ranges */
+	for (i = 0; i < num_freq_ranges; i++) {
+		sub_freq_range = nla_nest_start_noflag(msg, i + 1);
+
+		nla_put_u32(msg, NL80211_SAR_ATTR_SPECS_START_FREQ,
+			    rdev->wiphy.sar_capa->freq_ranges[i].start_freq);
+
+		nla_put_u32(msg, NL80211_SAR_ATTR_SPECS_END_FREQ,
+			    rdev->wiphy.sar_capa->freq_ranges[i].end_freq);
+
+		nla_put_u8(msg, NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX,
+			   rdev->wiphy.sar_capa->freq_ranges[i].index);
+
+		nla_nest_end(msg, sub_freq_range);
+	}
+
+	nla_nest_end(msg, specs);
+	nla_nest_end(msg, sar_capa);
+
+	return 0;
+fail:
+	nla_nest_cancel(msg, sar_capa);
+	return -ENOBUFS;
+}
+
 struct nl80211_dump_wiphy_state {
 	s64 filter_wiphy;
 	long start;
@@ -2598,6 +2663,9 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 		if (nl80211_put_tid_config_support(rdev, msg))
 			goto nla_put_failure;
 
+		if (nl80211_put_sar_specs(rdev, msg))
+			goto nla_put_failure;
+
 		/* done */
 		state->split_start = 0;
 		break;
@@ -14473,6 +14541,103 @@ static void nl80211_post_doit(const struct genl_ops *ops, struct sk_buff *skb,
 	}
 }
 
+static int nl80211_set_sar_specs(struct sk_buff *skb, struct genl_info *info)
+{
+	struct cfg80211_registered_device *rdev = info->user_ptr[0];
+	struct nlattr *spec[NL80211_SAR_ATTR_SPECS_MAX + 1];
+	struct nlattr *tb[NL80211_SAR_ATTR_MAX + 1];
+	struct cfg80211_sar_specs *sar_spec;
+	u8 type, power, index, specs;
+	struct nlattr *spec_list;
+	int rem, err;
+
+	if (!rdev->wiphy.sar_capa)
+		return -EOPNOTSUPP;
+
+	if (!info->attrs[NL80211_ATTR_SAR_SPEC])
+		return -EINVAL;
+
+	nla_parse_nested(tb, NL80211_SAR_ATTR_MAX, info->attrs[NL80211_ATTR_SAR_SPEC],
+			 sar_policy, info->extack);
+
+	if (!tb[NL80211_SAR_ATTR_TYPE])
+		return -EINVAL;
+
+	type = nla_get_u32(tb[NL80211_SAR_ATTR_TYPE]);
+
+	if (!tb[NL80211_SAR_ATTR_SPECS])
+		return -EINVAL;
+
+	specs = 0;
+	nla_for_each_nested(spec_list, tb[NL80211_SAR_ATTR_SPECS], rem)
+		specs++;
+
+	sar_spec = kzalloc(sizeof(*sar_spec) +
+			   specs * sizeof(struct cfg80211_sar_sub_specs),
+			   GFP_KERNEL);
+	if (!sar_spec)
+		return -ENOMEM;
+
+	sar_spec->sub_specs = (struct cfg80211_sar_sub_specs *)
+			((char *)sar_spec + sizeof(*sar_spec));
+	specs = 0;
+	sar_spec->type = type;
+
+	nla_for_each_nested(spec_list, tb[NL80211_SAR_ATTR_SPECS], rem) {
+		if (nla_parse(spec,
+			      NL80211_SAR_ATTR_SPECS_MAX,
+			      nla_data(spec_list),
+			      nla_len(spec_list),
+			      sar_specs_policy,
+			      NULL)) {
+			err = -EINVAL;
+			goto error;
+		}
+
+		/* for power type, power value must be presented */
+		if (!spec[NL80211_SAR_ATTR_SPECS_POWER] &&
+		    type == NL80211_SAR_TYPE_POWER) {
+			err = -EINVAL;
+			goto error;
+		}
+
+		power = nla_get_u8(spec[NL80211_SAR_ATTR_SPECS_POWER]);
+		sar_spec->sub_specs[specs].power = power;
+
+		/* if NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX isn't present,
+		 * then the power applies to all bands. But it's only valid
+		 * for the first entry.
+		 */
+		if (!spec[NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX]) {
+			if (specs) {
+				err = -EINVAL;
+				goto error;
+			} else {
+				sar_spec->sub_specs[specs].freq_range_index =
+					NL80211_SAR_ALL_FREQ_RNAGES;
+				specs++;
+				break;
+			}
+		}
+
+		index = nla_get_u8(spec[NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX]);
+		sar_spec->sub_specs[specs].freq_range_index = index;
+		specs++;
+	}
+
+	sar_spec->num_sub_specs = specs;
+
+	rdev->cur_cmd_info = info;
+	if (rdev->ops->set_sar_specs)
+		err = rdev_set_sar_specs(rdev, sar_spec);
+	else
+		err = -EOPNOTSUPP;
+	rdev->cur_cmd_info = NULL;
+error:
+	kfree(sar_spec);
+	return err;
+}
+
 static const struct genl_ops nl80211_ops[] = {
 	{
 		.cmd = NL80211_CMD_GET_WIPHY,
@@ -15331,6 +15496,14 @@ static const struct genl_ops nl80211_ops[] = {
 		.internal_flags = NL80211_FLAG_NEED_NETDEV |
 				  NL80211_FLAG_NEED_RTNL,
 	},
+	{
+		.cmd = NL80211_CMD_SET_SAR_SPECS,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.doit = nl80211_set_sar_specs,
+		.flags = GENL_UNS_ADMIN_PERM,
+		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
+				  NL80211_FLAG_NEED_RTNL,
+	},
 };
 
 static struct genl_family nl80211_fam __ro_after_init = {
diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
index 950d574..d9931c5 100644
--- a/net/wireless/rdev-ops.h
+++ b/net/wireless/rdev-ops.h
@@ -1356,4 +1356,16 @@ static inline int rdev_reset_tid_config(struct cfg80211_registered_device *rdev,
 	return ret;
 }
 
+static inline int rdev_set_sar_specs(struct cfg80211_registered_device *rdev,
+				     struct cfg80211_sar_specs *sar)
+{
+	int ret;
+
+	trace_rdev_set_sar_specs(&rdev->wiphy, sar);
+	ret = rdev->ops->set_sar_specs(&rdev->wiphy, sar);
+	trace_rdev_return_int(&rdev->wiphy, ret);
+
+	return ret;
+}
+
 #endif /* __CFG80211_RDEV_OPS */
diff --git a/net/wireless/trace.h b/net/wireless/trace.h
index 6e218a0..116be64 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -3547,6 +3547,25 @@ TRACE_EVENT(rdev_reset_tid_config,
 	TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT ", peer: " MAC_PR_FMT ", tids: 0x%x",
 		  WIPHY_PR_ARG, NETDEV_PR_ARG, MAC_PR_ARG(peer), __entry->tids)
 );
+
+TRACE_EVENT(rdev_set_sar_specs,
+	TP_PROTO(struct wiphy *wiphy, struct cfg80211_sar_specs *sar),
+	TP_ARGS(wiphy, sar),
+	TP_STRUCT__entry(
+		WIPHY_ENTRY
+		__field(u16, type)
+		__field(u16, num)
+	),
+	TP_fast_assign(
+		WIPHY_ASSIGN;
+		__entry->type = sar->type;
+		__entry->num = sar->num_sub_specs;
+
+	),
+	TP_printk(WIPHY_PR_FMT ", Set type:%d, num_specs:%d",
+		  WIPHY_PR_ARG, __entry->type, __entry->num)
+);
+
 #endif /* !__RDEV_OPS_TRACE || TRACE_HEADER_MULTI_READ */
 
 #undef TRACE_INCLUDE_PATH
-- 
2.7.4


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[RFC 2/2] ath10k: allow dynamic SAR power limits via common API

[Carl Huang]

ath10k assigns ath10k_mac_set_sar_specs to ath10k_ops, and
this function is called when user space application calls
NL80211_CMD_SET_SAR_SPECS. ath10k also registers SAR type,
and supported frequency ranges to wiphy so user space can
query SAR capabilities.

ath10k_mac_set_sar_specs further sets the power to firmware
to limit the TX power.

This feature is controlled by hw parameter: dynamic_sar_support.

Signed-off-by: Carl Huang <cjhuang@codeaurora.org>
---
 drivers/net/wireless/ath/ath10k/core.c |  16 +++++
 drivers/net/wireless/ath/ath10k/core.h |   3 +
 drivers/net/wireless/ath/ath10k/hw.h   |   2 +
 drivers/net/wireless/ath/ath10k/mac.c  | 106 +++++++++++++++++++++++++++++++++
 4 files changed, 127 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 5f4e121..fe53ca9 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -90,6 +90,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.hw_filter_reset_required = true,
 		.fw_diag_ce_download = false,
 		.tx_stats_over_pktlog = true,
+		.dynamic_sar_support = false,
 	},
 	{
 		.id = QCA988X_HW_2_0_VERSION,
@@ -124,6 +125,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.hw_filter_reset_required = true,
 		.fw_diag_ce_download = false,
 		.tx_stats_over_pktlog = true,
+		.dynamic_sar_support = false,
 	},
 	{
 		.id = QCA9887_HW_1_0_VERSION,
@@ -159,6 +161,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.hw_filter_reset_required = true,
 		.fw_diag_ce_download = false,
 		.tx_stats_over_pktlog = false,
+		.dynamic_sar_support = false,
 	},
 	{
 		.id = QCA6174_HW_3_2_VERSION,
@@ -189,6 +192,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_stats_over_pktlog = false,
 		.bmi_large_size_download = true,
 		.supports_peer_stats_info = true,
+		.dynamic_sar_support = true,
 	},
 	{
 		.id = QCA6174_HW_2_1_VERSION,
@@ -223,6 +227,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.hw_filter_reset_required = true,
 		.fw_diag_ce_download = false,
 		.tx_stats_over_pktlog = false,
+		.dynamic_sar_support = false,
 	},
 	{
 		.id = QCA6174_HW_2_1_VERSION,
@@ -257,6 +262,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.hw_filter_reset_required = true,
 		.fw_diag_ce_download = false,
 		.tx_stats_over_pktlog = false,
+		.dynamic_sar_support = false,
 	},
 	{
 		.id = QCA6174_HW_3_0_VERSION,
@@ -291,6 +297,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.hw_filter_reset_required = true,
 		.fw_diag_ce_download = false,
 		.tx_stats_over_pktlog = false,
+		.dynamic_sar_support = false,
 	},
 	{
 		.id = QCA6174_HW_3_2_VERSION,
@@ -329,6 +336,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.fw_diag_ce_download = true,
 		.tx_stats_over_pktlog = false,
 		.supports_peer_stats_info = true,
+		.dynamic_sar_support = true,
 	},
 	{
 		.id = QCA99X0_HW_2_0_DEV_VERSION,
@@ -369,6 +377,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.hw_filter_reset_required = true,
 		.fw_diag_ce_download = false,
 		.tx_stats_over_pktlog = false,
+		.dynamic_sar_support = false,
 	},
 	{
 		.id = QCA9984_HW_1_0_DEV_VERSION,
@@ -416,6 +425,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.hw_filter_reset_required = true,
 		.fw_diag_ce_download = false,
 		.tx_stats_over_pktlog = false,
+		.dynamic_sar_support = false,
 	},
 	{
 		.id = QCA9888_HW_2_0_DEV_VERSION,
@@ -460,6 +470,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.hw_filter_reset_required = true,
 		.fw_diag_ce_download = false,
 		.tx_stats_over_pktlog = false,
+		.dynamic_sar_support = false,
 	},
 	{
 		.id = QCA9377_HW_1_0_DEV_VERSION,
@@ -494,6 +505,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.hw_filter_reset_required = true,
 		.fw_diag_ce_download = false,
 		.tx_stats_over_pktlog = false,
+		.dynamic_sar_support = false,
 	},
 	{
 		.id = QCA9377_HW_1_1_DEV_VERSION,
@@ -530,6 +542,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.hw_filter_reset_required = true,
 		.fw_diag_ce_download = true,
 		.tx_stats_over_pktlog = false,
+		.dynamic_sar_support = false,
 	},
 	{
 		.id = QCA9377_HW_1_1_DEV_VERSION,
@@ -557,6 +570,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.ast_skid_limit = 0x10,
 		.num_wds_entries = 0x20,
 		.uart_pin_workaround = true,
+		.dynamic_sar_support = false,
 	},
 	{
 		.id = QCA4019_HW_1_0_DEV_VERSION,
@@ -598,6 +612,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.hw_filter_reset_required = true,
 		.fw_diag_ce_download = false,
 		.tx_stats_over_pktlog = false,
+		.dynamic_sar_support = false,
 	},
 	{
 		.id = WCN3990_HW_1_0_DEV_VERSION,
@@ -625,6 +640,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.hw_filter_reset_required = false,
 		.fw_diag_ce_download = false,
 		.tx_stats_over_pktlog = false,
+		.dynamic_sar_support = true,
 	},
 };
 
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 4cf5bd4..f90c34a 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -1281,6 +1281,9 @@ struct ath10k {
 	bool coex_support;
 	int coex_gpio_pin;
 
+	u32 tx_power_2g_limit;
+	u32 tx_power_5g_limit;
+
 	/* must be last */
 	u8 drv_priv[] __aligned(sizeof(void *));
 };
diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h
index c6ded21..6b03c77 100644
--- a/drivers/net/wireless/ath/ath10k/hw.h
+++ b/drivers/net/wireless/ath/ath10k/hw.h
@@ -623,6 +623,8 @@ struct ath10k_hw_params {
 
 	/* provides bitrates for sta_statistics using WMI_TLV_PEER_STATS_INFO_EVENTID */
 	bool supports_peer_stats_info;
+
+	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 2e3eb5b..830c61f 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -81,6 +81,17 @@ static struct ieee80211_rate ath10k_rates_rev2[] = {
 	{ .bitrate = 540, .hw_value = ATH10K_HW_RATE_OFDM_54M },
 };
 
+static const struct cfg80211_sar_freq_ranges ath10k_sar_freq_ranges[] = {
+	{ .index = 0, .start_freq = 2412000, .end_freq = 2484000 },
+	{ .index = 1, .start_freq = 2484000, .end_freq = 5865000 },
+};
+
+static const struct cfg80211_sar_capa ath10k_sar_capa = {
+	.type = NL80211_SAR_TYPE_POWER,
+	.num_freq_ranges = (ARRAY_SIZE(ath10k_sar_freq_ranges)),
+	.freq_ranges = &ath10k_sar_freq_ranges[0],
+};
+
 #define ATH10K_MAC_FIRST_OFDM_RATE_IDX 4
 
 #define ath10k_a_rates (ath10k_rates + ATH10K_MAC_FIRST_OFDM_RATE_IDX)
@@ -2880,6 +2891,95 @@ static int ath10k_mac_vif_recalc_txbf(struct ath10k *ar,
 	return 0;
 }
 
+static bool ath10k_mac_is_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(struct ath10k *ar)
+{
+	int ret;
+
+	if (!ar->hw_params.dynamic_sar_support)
+		return 0;
+
+	if (ar->tx_power_2g_limit == 0 || ar->tx_power_5g_limit == 0)
+		return 0;
+
+	if (!ath10k_mac_is_connected(ar))
+		return 0;
+
+	ret = ath10k_wmi_pdev_set_param(ar,
+					ar->wmi.pdev_param->txpower_limit2g,
+					ar->tx_power_2g_limit);
+	if (ret) {
+		ath10k_warn(ar, "failed to set 2.4G txpower %d: %d\n",
+			    ar->tx_power_2g_limit, ret);
+		return ret;
+	}
+
+	ret = ath10k_wmi_pdev_set_param(ar,
+					ar->wmi.pdev_param->txpower_limit5g,
+					ar->tx_power_5g_limit);
+	if (ret) {
+		ath10k_warn(ar, "failed to set 5G txpower %d: %d\n",
+			    ar->tx_power_5g_limit, ret);
+		return ret;
+	}
+
+	ath10k_dbg(ar, ATH10K_DBG_MAC, "set txpower 2G:%d, 5G:%d successfully\n",
+		   ar->tx_power_2g_limit, ar->tx_power_5g_limit);
+
+	return ret;
+}
+
+static int ath10k_mac_set_sar_specs(struct ieee80211_hw *hw,
+				    const struct cfg80211_sar_specs *sar)
+{
+	struct cfg80211_sar_sub_specs *sub_specs;
+	struct ath10k *ar = hw->priv;
+	int i;
+
+	if (!ar->hw_params.dynamic_sar_support)
+		return -EOPNOTSUPP;
+
+	if (!sar || sar->type != NL80211_SAR_TYPE_POWER ||
+	    sar->num_sub_specs == 0 || !sar->sub_specs)
+		return -EINVAL;
+
+	sub_specs = sar->sub_specs;
+
+	/* note the power is in 0.25dbm unit, while ath10k uses
+	 * 0.5dbm unit.
+	 */
+	for (i = 0; i < sar->num_sub_specs; i++) {
+		if (sub_specs->freq_range_index == NL80211_SAR_ALL_FREQ_RNAGES) {
+			ar->tx_power_2g_limit = sub_specs->power / 2;
+			ar->tx_power_5g_limit = sub_specs->power / 2;
+			goto set_power;
+		}
+
+		if (sub_specs->freq_range_index == 0)
+			ar->tx_power_2g_limit = sub_specs->power / 2;
+		else if (sub_specs->freq_range_index == 1)
+			ar->tx_power_5g_limit = sub_specs->power / 2;
+		else
+			return -EINVAL;
+
+		sub_specs++;
+	}
+
+set_power:
+	return ath10k_mac_set_sar_power(ar);
+}
+
 /* 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,
@@ -2968,6 +3068,8 @@ static void ath10k_bss_assoc(struct ieee80211_hw *hw,
 
 	arvif->is_up = true;
 
+	ath10k_mac_set_sar_power(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.
@@ -9270,6 +9372,7 @@ static const struct ieee80211_ops ath10k_ops = {
 #ifdef CONFIG_MAC80211_DEBUGFS
 	.sta_add_debugfs		= ath10k_sta_add_debugfs,
 #endif
+	.set_sar_specs			= ath10k_mac_set_sar_specs,
 };
 
 #define CHAN2G(_channel, _freq, _flags) { \
@@ -10009,6 +10112,9 @@ int ath10k_mac_register(struct ath10k *ar)
 		goto err_free;
 	}
 
+	if (ar->hw_params.dynamic_sar_support)
+		ar->hw->wiphy->sar_capa = &ath10k_sar_capa;
+
 	if (!test_bit(ATH10K_FLAG_RAW_MODE, &ar->dev_flags))
 		ar->hw->netdev_features = NETIF_F_HW_CSUM;
 
-- 
2.7.4


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 1/2] nl80211: add common API to configure SAR power limitations.

[Johannes Berg]

On Tue, 2020-09-22 at 13:49 +0800, Carl Huang wrote:
> NL80211_CMD_SET_SAR_SPECS is added to configure SAR from
> user space. NL80211_ATTR_SAR_SPEC is used to pass the SAR
> power specification when used with NL80211_CMD_SET_SAR_SPECS.
> 
> Wireless driver needs to register SAR type, supported frequency
> ranges to wiphy, so user space can query it. The index in
> frequency range is used to specify which sub band the power
> limitation applies to. The SAR type is for compatibility, so later
> other SAR mechanism can be implemented without breaking the user
> space SAR applications.
> 
> Normal process is user space quries the SAR capability, and

typo: queries

> gets the index of supported frequency ranges and associates the
> power limitation with this index and sends to kernel.
> 
> Here is an example of message send to kernel:
> 8c 00 00 00 08 00 03 00 15 00 00 00 38 00 26 81
> 08 00 01 00 00 00 00 00 2c 00 02 80 14 00 01 80
> 05 00 02 00 00 00 00 00 05 00 01 00 38 00 00 00
> 14 00 02 80 05 00 02 00 01 00 00 00 05 00 01 00
> 48 00 00 00

Heh, that's not super useful, though that doesn't matter much I guess :)

> NL80211_CMD_SET_SAR_SPECS:  0x8c
> NL80211_ATTR_SAR_SPEC:  0x8126 (NLA_NESTED)
> NL80211_SAR_ATTR_TYPE:  0x00 (NL80211_SAR_TYPE_POWER)
> NL80211_SAR_ATTR_SPECS: 0x8002 (NLA_NESTED)
> freq range 0 power: 0x38 in 0.25dbm unit (14dbm)
> freq range 1 power: 0x48 in 0.25dbm unit (18dbm)

that makes more sense :)

> +/**
> + * struct cfg80211_sar_sub_specs - sub specs limit
> + * @power: value in 0.25dbm

I guess the documentation should state that it's a power limit, or
something?

> + * @freq_range_index: index the power limitation applies to
> + */
> +struct cfg80211_sar_sub_specs {
> +	u8 power;
> +	u8 freq_range_index;
> +};
> +
> +/**
> + * struct cfg80211_sar_specs - sar limit specs
> + * @type: it's set with power in 0.25dbm or other types
> + * @num_sub_specs: number of sar sub specs
> + * @sub_specs: memory to hold the sar sub specs
> + */
> +struct cfg80211_sar_specs {
> +	enum nl80211_sar_type type;
> +	u16 num_sub_specs;
> +	struct cfg80211_sar_sub_specs *sub_specs;

Wouldn't it make more sense to just have sub_specs[] and inline? The
pointers here are bigger than many of the sub-specs I guess...

Also, is num_sub_specs even needed? It seems we could just require that
userspace sends all of them all the time?

> +struct cfg80211_sar_freq_ranges {
> +	u8 index;

Does an index here make sense?

> +	u32 start_freq;
> +	u32 end_freq;
> +};
> +
> +/**
> + * struct cfg80211_sar_capa - sar limit capability
> + * @type: it's set via power in 0.25dbm or other types
> + * @num_freq_ranges: number of frequency ranges
> + * @chan_ranges: memory to hold the channel ranges.
> + */
> +struct cfg80211_sar_capa {
> +	enum nl80211_sar_type type;
> +	u8 num_freq_ranges;
> +	const struct cfg80211_sar_freq_ranges *freq_ranges;

You end up with an array here, so some kind of index is implicit, the
array index. I don't see why you'd want to have a separate index
*within* the frequency range, then you just have to check those make
sense, etc.?

> + * @NL80211_ATTR_SAR_SPEC: SAR power limitation specification when
> + *	used with %NL80211_CMD_SET_SAR_SPECS. It contains array of
> + *	%nl80211_sar_specs_attrs.

I think I know what you mean (a nested array where each item has
attributes from &enum nl80211_sar_specs_attrs), but could state that a
bit clearer.

> +#define NL80211_SAR_ALL_FREQ_RNAGES	0xff

typo

> +#define NUM_MAX_NL80211_SAR_FREQ_RANGES 0xfe

but I'm not sure what these are used for in the first place, they seem
more like internal implementation details?

> +/**
> + * nl80211_sar_specs_attrs - Attributes for SAR power limit specs
> + *
> + * @NL80211_SAR_ATTR_SPECS_POWER: Required (u32)value to specify the actual
> + *	power limit value in units of 0.25 dBm if type is
> + *	NL80211_SAR_TYPE_POWER. (i.e., a value of 44 represents 11 dBm)
> + *
> + * @NL80211_SAR_ATTR_SPECS_FREQ_RANGES_INDEX: optional (u32) value to specify the
> + *	index of exported freq ranges table. If this attribute is not present, then
> + *	the power is applied to all freq ranges, i.e, all bands

Especially since this says "[i]f this attribute is not present".

Does that make sense though? It seems possible to just use the nesting
entry as the index, and just require userspace to set up all indexes if
it wants to apply to all of them, why bother having to deal with that
case in the kernel and perhaps even drivers?

> + * @NL80211_SAR_ATTR_SPECS_START_FREQ: Required (u32) value to specify the start
> + *	frequency of this range to register SAR capability to wihpy and the unit

typo: wiphy

> + *	is kHZ

kHz

> + *
> + * @NL80211_SAR_ATTR_SPECS_END_FREQ: Required (u32) value to specify the end frequency
> + *	of this range to register SAR capability to wiphy and the unit is kHZ

kHz

But hm, are you mixing the capability advertisement and the actual
request into the same attributes? Perhaps that makes sense, but the
documentation could be a bit clearer IMHO.

> +++ b/net/mac80211/cfg.c

The mac80211 bits could probably be in a separate patch, just for
clarity.

> +static const struct nla_policy
> +sar_specs_policy[NL80211_SAR_ATTR_SPECS_MAX + 1] = {
> +		[NL80211_SAR_ATTR_SPECS_POWER] = { .type = NLA_U8 },

double indentation?

> +		[NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX] =
> +			NLA_POLICY_MAX(NLA_U8, NUM_MAX_NL80211_SAR_FREQ_RANGES),

I don't see a good reason to limit to 0xfe? The kernel could just use a
wider datatype internally.

Anyway, I'm not sure the attribute is even needed, see above.

> @@ -1856,6 +1870,8 @@ static int nl80211_add_commands_unsplit(struct cfg80211_registered_device *rdev,
>  			goto nla_put_failure;
>  	}
>  
> +	CMD(set_sar_specs, SET_SAR_SPECS);

Don't add to _unsplit() please.

> +	specs = nla_nest_start_noflag(msg, NL80211_SAR_ATTR_SPECS);

Don't use _noflag() please, that's just for backward compatibility.

> +	if (!specs)
> +		goto fail;
> +
> +	/* report supported freq_ranges */
> +	for (i = 0; i < num_freq_ranges; i++) {
> +		sub_freq_range = nla_nest_start_noflag(msg, i + 1);

Same here.

> @@ -2598,6 +2663,9 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
>  		if (nl80211_put_tid_config_support(rdev, msg))
>  			goto nla_put_failure;
>  
> +		if (nl80211_put_sar_specs(rdev, msg))
> +			goto nla_put_failure;

Perhaps better to have a separate message (case entry) since it might
get fairly big?

> +	sar_spec = kzalloc(sizeof(*sar_spec) +
> +			   specs * sizeof(struct cfg80211_sar_sub_specs),
> +			   GFP_KERNEL);
> +	if (!sar_spec)
> +		return -ENOMEM;
> +
> +	sar_spec->sub_specs = (struct cfg80211_sar_sub_specs *)
> +			((char *)sar_spec + sizeof(*sar_spec));

If you anyway set the pointer there then you can actually just make it a
[] array instead of having the pointer in the first place ... would be
equivalent but save you from these contortions :)

> +	specs = 0;
> +	sar_spec->type = type;
> +
> +	nla_for_each_nested(spec_list, tb[NL80211_SAR_ATTR_SPECS], rem) {
> +		if (nla_parse(spec,
> +			      NL80211_SAR_ATTR_SPECS_MAX,
> +			      nla_data(spec_list),
> +			      nla_len(spec_list),
> +			      sar_specs_policy,
> +			      NULL)) {
> +			err = -EINVAL;
> +			goto error;
> +		}
> +
> +		/* for power type, power value must be presented */
> +		if (!spec[NL80211_SAR_ATTR_SPECS_POWER] &&
> +		    type == NL80211_SAR_TYPE_POWER) {
> +			err = -EINVAL;
> +			goto error;
> +		}

Somewhere you should probably also check that the type matches the
advertised type.

> +		power = nla_get_u8(spec[NL80211_SAR_ATTR_SPECS_POWER]);
> +		sar_spec->sub_specs[specs].power = power;
> +
> +		/* if NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX isn't present,
> +		 * then the power applies to all bands. But it's only valid
> +		 * for the first entry.
> +		 */
> +		if (!spec[NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX]) {
> +			if (specs) {
> +				err = -EINVAL;
> +				goto error;
> +			} else {
> +				sar_spec->sub_specs[specs].freq_range_index =
> +					NL80211_SAR_ALL_FREQ_RNAGES;
> +				specs++;
> +				break;
> +			}
> +		}

Yeah... not sure I see this as being worth it, vs. userspace just
filling all the possible entries.

> +		index = nla_get_u8(spec[NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX]);
> +		sar_spec->sub_specs[specs].freq_range_index = index;
> +		specs++;
> +	}
> +
> +	sar_spec->num_sub_specs = specs;
> +
> +	rdev->cur_cmd_info = info;
> +	if (rdev->ops->set_sar_specs)
> +		err = rdev_set_sar_specs(rdev, sar_spec);
> +	else
> +		err = -EOPNOTSUPP;

Might be worth checking this early and avoiding the allocation and all?

OTOH, that gives you error checking even if the driver was completely
confused and advertised sar specs without the method ...

johannes


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 1/2] nl80211: add common API to configure SAR power limitations.

[Abhishek Kumar]

From: kuabhs@chromium.org

These patches do not apply cleanly on kernel 5.4. I fixed the merged conflicts
and verified. I verified these APIs using power_manager in chromeos, the APIs 
works fine.
Tested-by: Abhishek Kumar <kuabhs@chromium.org>

-Abhishek

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 1/2] nl80211: add common API to configure SAR power limitations.

[Carl Huang]

On 2020-10-31 10:46, Abhishek Kumar wrote:
> From: kuabhs@chromium.org
> 
> There are few more additional comments here.
>> + * @NL80211_CMD_SET_SAR_SPECS: SAR power limitation configuration is
>> + *     passed using %NL80211_ATTR_SAR_SPEC.
>> + *
> 
> This command requires NL80211_ATTR_IFINDEX, probably should be better 
> to add
> this in the comment ?
> 
Per Johannes's comments, this explicit index is not required. Are you 
fine
with it?

Instead, user-space application records the array index when querying 
the SAR
capability. When set, the nested array index will be used to set the 
power.
This requires user-space has a strict mapping of index. and 
NL80211_ATTR_IFINDEX
is to be removed.


>> +static int
>> +nl80211_put_sar_specs(struct cfg80211_registered_device *rdev,
>> +                     struct sk_buff *msg)
>> +{
>> +       struct nlattr *sar_capa, *specs, *sub_freq_range;
>> +       u8  num_freq_ranges;
>> +       int i;
>> +
>> +       if (!rdev->wiphy.sar_capa)
>> +               return 0;
>> +
>> +       num_freq_ranges = rdev->wiphy.sar_capa->num_freq_ranges;
>> +
>> +       sar_capa = nla_nest_start(msg, NL80211_ATTR_SAR_SPEC);
>> +       if (!sar_capa)
>> +               return -ENOSPC;
> 
> I see some places nla_nest_start_noflag being used and here 
> nla_nest_start.
> Whats the specific reason to do that ? In the newer Kernel versions, I 
> believe
> nla_nest_start is preferred.
> 
This will be addressed in next version.

>> +               power = 
>> nla_get_u8(spec[NL80211_SAR_ATTR_SPECS_POWER]);
>> +               sar_spec->sub_specs[specs].power = power;
>> +
>> +               /* if NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX isn't 
>> present,
>> +                * then the power applies to all bands. But it's only 
>> valid
>> +                * for the first entry.
>> +                */
>> +               if (!spec[NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX]) {
>> +                       if (specs) {
>> +                               err = -EINVAL;
>> +                               goto error;
>> +                       } else {
>> +                               
>> sar_spec->sub_specs[specs].freq_range_index =
>> +                                       NL80211_SAR_ALL_FREQ_RNAGES;
>> +                               specs++;
>> +                               break;
>> +                       }
>> +               }
> 
> Here I see you are assigning same power to all freq band if for the 
> first band
> the freq index is not provided. Is there any specific reason to only
> check the first
> here ? Probably this logic should move into specific drivers. Thoughts 
> ?
> 
This logic will be removed per Johannes's comments.

Please read  Johannes's comments. If you agree with him and has no other 
advices,
then I will post the second version of it.


> -Abhishek
> 
> 
> _______________________________________________
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 1/2] nl80211: add common API to configure SAR power limitations.

[Johannes Berg]

On Tue, 2020-11-03 at 10:34 +0800, Carl Huang wrote:
> On 2020-10-31 10:46, Abhishek Kumar wrote:
> > From: kuabhs@chromium.org
> > 
> > There are few more additional comments here.
> > > + * @NL80211_CMD_SET_SAR_SPECS: SAR power limitation configuration is
> > > + *     passed using %NL80211_ATTR_SAR_SPEC.
> > > + *
> > 
> > This command requires NL80211_ATTR_IFINDEX, probably should be better 
> > to add
> > this in the comment ?
> > 
> Per Johannes's comments, this explicit index is not required. Are you 
> fine with it?
> 
> Instead, user-space application records the array index when querying 
> the SAR
> capability. When set, the nested array index will be used to set the 
> power.
> This requires user-space has a strict mapping of index. and 
> NL80211_ATTR_IFINDEX
> is to be removed.

Wait, what? The IFINDEX is still required, that identifies the interface
- though it probably shouldn't be required, this should be per wiphy, so
you could also use ATTR_WIPHY_IDX?

You're thinking of ... something
else. NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX perhaps?

johannes


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 1/2] nl80211: add common API to configure SAR power limitations.

[Abhishek Kumar]

On Tue, Nov 3, 2020 at 5:15 AM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Tue, 2020-11-03 at 10:34 +0800, Carl Huang wrote:
> > On 2020-10-31 10:46, Abhishek Kumar wrote:
> > > From: kuabhs@chromium.org
> > >
> > > There are few more additional comments here.
> > > > + * @NL80211_CMD_SET_SAR_SPECS: SAR power limitation configuration is
> > > > + *     passed using %NL80211_ATTR_SAR_SPEC.
> > > > + *
> > >
> > > This command requires NL80211_ATTR_IFINDEX, probably should be better
> > > to add
> > > this in the comment ?
> > >
> > Per Johannes's comments, this explicit index is not required. Are you
> > fine with it?
> >
> > Instead, user-space application records the array index when querying
> > the SAR
> > capability. When set, the nested array index will be used to set the
> > power.
> > This requires user-space has a strict mapping of index. and
> > NL80211_ATTR_IFINDEX
> > is to be removed.
>
> Wait, what? The IFINDEX is still required, that identifies the interface
> - though it probably shouldn't be required, this should be per wiphy, so
> you could also use ATTR_WIPHY_IDX?
>
> You're thinking of ... something
> else. NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX perhaps?

Yes, probably the index mapping that you are talking about is using
NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX .

I think Johannes has already covered most of the comments. I have one
last one  mentioned below.

> +nl80211_put_sar_specs(struct cfg80211_registered_device *rdev,
> +                     struct sk_buff *msg)
> +{
> +       struct nlattr *sar_capa, *specs, *sub_freq_range;
> +       u8  num_freq_ranges;
> +       int i;
> +
> +       if (!rdev->wiphy.sar_capa)
> +               return 0;
> +
> +       num_freq_ranges = rdev->wiphy.sar_capa->num_freq_ranges;
> +
> +       sar_capa = nla_nest_start(msg, NL80211_ATTR_SAR_SPEC);
> +       if (!sar_capa)
> +               return -ENOSPC;
> +
> +       if (nla_put_u16(msg, NL80211_SAR_ATTR_TYPE, rdev->wiphy.sar_capa->type))
> +               goto fail;

The nla_put_u16 NL80211_SAR_ATTR_TYPE here uses u16 whereas in the to
get this property below it uses nla_get_u32 .  I think this should be
consistent ?

> +       if (!tb[NL80211_SAR_ATTR_TYPE])
> +               return -EINVAL;
> +
> +       type = nla_get_u32(tb[NL80211_SAR_ATTR_TYPE]);
> +
> +       if (!tb[NL80211_SAR_ATTR_SPECS])
> +               return -EINVAL;

As mentioned above the get and put for NL80211_SAR_ATTR_TYPE is not consistent.

-Abhishek

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 1/2] nl80211: add common API to configure SAR power limitations.

[Carl Huang]

On 2020-11-04 09:17, Abhishek Kumar wrote:
> On Tue, Nov 3, 2020 at 5:15 AM Johannes Berg 
> <johannes@sipsolutions.net> wrote:
>> 
>> On Tue, 2020-11-03 at 10:34 +0800, Carl Huang wrote:
>> > On 2020-10-31 10:46, Abhishek Kumar wrote:
>> > > From: kuabhs@chromium.org
>> > >
>> > > There are few more additional comments here.
>> > > > + * @NL80211_CMD_SET_SAR_SPECS: SAR power limitation configuration is
>> > > > + *     passed using %NL80211_ATTR_SAR_SPEC.
>> > > > + *
>> > >
>> > > This command requires NL80211_ATTR_IFINDEX, probably should be better
>> > > to add
>> > > this in the comment ?
>> > >
>> > Per Johannes's comments, this explicit index is not required. Are you
>> > fine with it?
>> >
>> > Instead, user-space application records the array index when querying
>> > the SAR
>> > capability. When set, the nested array index will be used to set the
>> > power.
>> > This requires user-space has a strict mapping of index. and
>> > NL80211_ATTR_IFINDEX
>> > is to be removed.
>> 
>> Wait, what? The IFINDEX is still required, that identifies the 
>> interface
>> - though it probably shouldn't be required, this should be per wiphy, 
>> so
>> you could also use ATTR_WIPHY_IDX?
>> 
>> You're thinking of ... something
>> else. NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX perhaps?
> 
> Yes, probably the index mapping that you are talking about is using
> NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX .
> 
You're right. I just remembered Johannes's comments to remove
the NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX and made mistake here.


> I think Johannes has already covered most of the comments. I have one
> last one  mentioned below.
> 
>> +nl80211_put_sar_specs(struct cfg80211_registered_device *rdev,
>> +                     struct sk_buff *msg)
>> +{
>> +       struct nlattr *sar_capa, *specs, *sub_freq_range;
>> +       u8  num_freq_ranges;
>> +       int i;
>> +
>> +       if (!rdev->wiphy.sar_capa)
>> +               return 0;
>> +
>> +       num_freq_ranges = rdev->wiphy.sar_capa->num_freq_ranges;
>> +
>> +       sar_capa = nla_nest_start(msg, NL80211_ATTR_SAR_SPEC);
>> +       if (!sar_capa)
>> +               return -ENOSPC;
>> +
>> +       if (nla_put_u16(msg, NL80211_SAR_ATTR_TYPE, 
>> rdev->wiphy.sar_capa->type))
>> +               goto fail;
> 
> The nla_put_u16 NL80211_SAR_ATTR_TYPE here uses u16 whereas in the to
> get this property below it uses nla_get_u32 .  I think this should be
> consistent ?
> 
>> +       if (!tb[NL80211_SAR_ATTR_TYPE])
>> +               return -EINVAL;
>> +
>> +       type = nla_get_u32(tb[NL80211_SAR_ATTR_TYPE]);
>> +
>> +       if (!tb[NL80211_SAR_ATTR_SPECS])
>> +               return -EINVAL;
> 
> As mentioned above the get and put for NL80211_SAR_ATTR_TYPE is not 
> consistent.
> 
Correct. Thanks for pointing out and it will be fixed in next version.

> -Abhishek
> 
> _______________________________________________
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 1/2] nl80211: add common API to configure SAR power limitations.

[Carl Huang]

On 2020-09-28 20:36, Johannes Berg wrote:
> On Tue, 2020-09-22 at 13:49 +0800, Carl Huang wrote:
>> NL80211_CMD_SET_SAR_SPECS is added to configure SAR from
>> user space. NL80211_ATTR_SAR_SPEC is used to pass the SAR
>> power specification when used with NL80211_CMD_SET_SAR_SPECS.
>> 
>> Wireless driver needs to register SAR type, supported frequency
>> ranges to wiphy, so user space can query it. The index in
>> frequency range is used to specify which sub band the power
>> limitation applies to. The SAR type is for compatibility, so later
>> other SAR mechanism can be implemented without breaking the user
>> space SAR applications.
>> 
>> Normal process is user space quries the SAR capability, and
> 
> typo: queries
> 
sure, I'll fix it in v2.

>> gets the index of supported frequency ranges and associates the
>> power limitation with this index and sends to kernel.
>> 
>> Here is an example of message send to kernel:
>> 8c 00 00 00 08 00 03 00 15 00 00 00 38 00 26 81
>> 08 00 01 00 00 00 00 00 2c 00 02 80 14 00 01 80
>> 05 00 02 00 00 00 00 00 05 00 01 00 38 00 00 00
>> 14 00 02 80 05 00 02 00 01 00 00 00 05 00 01 00
>> 48 00 00 00
> 
> Heh, that's not super useful, though that doesn't matter much I guess 
> :)
> 
>> NL80211_CMD_SET_SAR_SPECS:  0x8c
>> NL80211_ATTR_SAR_SPEC:  0x8126 (NLA_NESTED)
>> NL80211_SAR_ATTR_TYPE:  0x00 (NL80211_SAR_TYPE_POWER)
>> NL80211_SAR_ATTR_SPECS: 0x8002 (NLA_NESTED)
>> freq range 0 power: 0x38 in 0.25dbm unit (14dbm)
>> freq range 1 power: 0x48 in 0.25dbm unit (18dbm)
> 
> that makes more sense :)
> 
As this is an explanation of the message above, is it good to keep the 
message?

>> +/**
>> + * struct cfg80211_sar_sub_specs - sub specs limit
>> + * @power: value in 0.25dbm
> 
> I guess the documentation should state that it's a power limit, or
> something?
> 
>> + * @freq_range_index: index the power limitation applies to
>> + */
>> +struct cfg80211_sar_sub_specs {
>> +	u8 power;
>> +	u8 freq_range_index;
>> +};
>> +
>> +/**
>> + * struct cfg80211_sar_specs - sar limit specs
>> + * @type: it's set with power in 0.25dbm or other types
>> + * @num_sub_specs: number of sar sub specs
>> + * @sub_specs: memory to hold the sar sub specs
>> + */
>> +struct cfg80211_sar_specs {
>> +	enum nl80211_sar_type type;
>> +	u16 num_sub_specs;
>> +	struct cfg80211_sar_sub_specs *sub_specs;
> 
> Wouldn't it make more sense to just have sub_specs[] and inline? The
> pointers here are bigger than many of the sub-specs I guess...
> 
> Also, is num_sub_specs even needed? It seems we could just require that
> userspace sends all of them all the time?
> 
Agree as long as user space sends all of them.

>> +struct cfg80211_sar_freq_ranges {
>> +	u8 index;
> 
> Does an index here make sense?
> 
With agreement from Google, it's OK to remove it.

>> +	u32 start_freq;
>> +	u32 end_freq;
>> +};
>> +
>> +/**
>> + * struct cfg80211_sar_capa - sar limit capability
>> + * @type: it's set via power in 0.25dbm or other types
>> + * @num_freq_ranges: number of frequency ranges
>> + * @chan_ranges: memory to hold the channel ranges.
>> + */
>> +struct cfg80211_sar_capa {
>> +	enum nl80211_sar_type type;
>> +	u8 num_freq_ranges;
>> +	const struct cfg80211_sar_freq_ranges *freq_ranges;
> 
> You end up with an array here, so some kind of index is implicit, the
> array index. I don't see why you'd want to have a separate index
> *within* the frequency range, then you just have to check those make
> sense, etc.?
> 
>> + * @NL80211_ATTR_SAR_SPEC: SAR power limitation specification when
>> + *	used with %NL80211_CMD_SET_SAR_SPECS. It contains array of
>> + *	%nl80211_sar_specs_attrs.
> 
> I think I know what you mean (a nested array where each item has
> attributes from &enum nl80211_sar_specs_attrs), but could state that a
> bit clearer.
> 

I'll try to describe it more clear in v2.

>> +#define NL80211_SAR_ALL_FREQ_RNAGES	0xff
> 
> typo
> 
sure, will fix in v2.

>> +#define NUM_MAX_NL80211_SAR_FREQ_RANGES 0xfe
> 
> but I'm not sure what these are used for in the first place, they seem
> more like internal implementation details?
> 
In v1 design, userspace doesn't need to specify all the sub bands if the 
a power
limitation applies to all sub bands, so NL80211_SAR_ALL_FREQ_RNAGES is 
defined
for this purpose. To distinguish from NL80211_SAR_ALL_FREQ_RNAGES, the 
maximum
number of freq ranges should not exceed  (NL80211_SAR_ALL_FREQ_RNAGES - 
1), so
define NUM_MAX_NL80211_SAR_FREQ_RANGES as 0xfe.

It's right these two definitions are internal, userspace doesn't use 
them.

Again as user-space will sends all the ranges, let me remove these 2 
definitions.

>> +/**
>> + * nl80211_sar_specs_attrs - Attributes for SAR power limit specs
>> + *
>> + * @NL80211_SAR_ATTR_SPECS_POWER: Required (u32)value to specify the 
>> actual
>> + *	power limit value in units of 0.25 dBm if type is
>> + *	NL80211_SAR_TYPE_POWER. (i.e., a value of 44 represents 11 dBm)
>> + *
>> + * @NL80211_SAR_ATTR_SPECS_FREQ_RANGES_INDEX: optional (u32) value to 
>> specify the
>> + *	index of exported freq ranges table. If this attribute is not 
>> present, then
>> + *	the power is applied to all freq ranges, i.e, all bands
> 
> Especially since this says "[i]f this attribute is not present".
> 
> Does that make sense though? It seems possible to just use the nesting
> entry as the index, and just require userspace to set up all indexes if
> it wants to apply to all of them, why bother having to deal with that
> case in the kernel and perhaps even drivers?
> 

Ok, let me drop it in v2.

Johannes, want to confirm with you "nesting entry as the index". Do you 
mean that kernel
take the first entry as index 0, then second entry as index 1 and etc?


>> + * @NL80211_SAR_ATTR_SPECS_START_FREQ: Required (u32) value to 
>> specify the start
>> + *	frequency of this range to register SAR capability to wihpy and 
>> the unit
> 
> typo: wiphy
> 
>> + *	is kHZ
> 
> kHz
> 
Will fix it in v2.

>> + *
>> + * @NL80211_SAR_ATTR_SPECS_END_FREQ: Required (u32) value to specify 
>> the end frequency
>> + *	of this range to register SAR capability to wiphy and the unit is 
>> kHZ
> 
> kHz
> 
> But hm, are you mixing the capability advertisement and the actual
> request into the same attributes? Perhaps that makes sense, but the
> documentation could be a bit clearer IMHO.
> 
>> +++ b/net/mac80211/cfg.c
> 
> The mac80211 bits could probably be in a separate patch, just for
> clarity.
> 
>> +static const struct nla_policy
>> +sar_specs_policy[NL80211_SAR_ATTR_SPECS_MAX + 1] = {
>> +		[NL80211_SAR_ATTR_SPECS_POWER] = { .type = NLA_U8 },
> 
> double indentation?
> 
>> +		[NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX] =
>> +			NLA_POLICY_MAX(NLA_U8, NUM_MAX_NL80211_SAR_FREQ_RANGES),
> 
> I don't see a good reason to limit to 0xfe? The kernel could just use a
> wider datatype internally.
> 
> Anyway, I'm not sure the attribute is even needed, see above.
> 
If userspace has to sends all the sub bands, then
NL80211_SAR_ALL_FREQ_RNAGES is not required, and therefore 
NUM_MAX_NL80211_SAR_FREQ_RANGES
is also not required.

I'll delete these 2 definitions, and change policy to { .type = NLA_U8 }

>> @@ -1856,6 +1870,8 @@ static int nl80211_add_commands_unsplit(struct 
>> cfg80211_registered_device *rdev,
>>  			goto nla_put_failure;
>>  	}
>> 
>> +	CMD(set_sar_specs, SET_SAR_SPECS);
> 
> Don't add to _unsplit() please.
> 
Do you mean add it to below block?
if (state->split) {
...
CMD(set_sar_specs, SET_SAR_SPECS);
}


>> +	specs = nla_nest_start_noflag(msg, NL80211_SAR_ATTR_SPECS);
> 
> Don't use _noflag() please, that's just for backward compatibility.
> 
sure.

>> +	if (!specs)
>> +		goto fail;
>> +
>> +	/* report supported freq_ranges */
>> +	for (i = 0; i < num_freq_ranges; i++) {
>> +		sub_freq_range = nla_nest_start_noflag(msg, i + 1);
> 
> Same here.
> 
>> @@ -2598,6 +2663,9 @@ static int nl80211_send_wiphy(struct 
>> cfg80211_registered_device *rdev,
>>  		if (nl80211_put_tid_config_support(rdev, msg))
>>  			goto nla_put_failure;
>> 
>> +		if (nl80211_put_sar_specs(rdev, msg))
>> +			goto nla_put_failure;
> 
> Perhaps better to have a separate message (case entry) since it might
> get fairly big?
> 
>> +	sar_spec = kzalloc(sizeof(*sar_spec) +
>> +			   specs * sizeof(struct cfg80211_sar_sub_specs),
>> +			   GFP_KERNEL);
>> +	if (!sar_spec)
>> +		return -ENOMEM;
>> +
>> +	sar_spec->sub_specs = (struct cfg80211_sar_sub_specs *)
>> +			((char *)sar_spec + sizeof(*sar_spec));
> 
> If you anyway set the pointer there then you can actually just make it 
> a
> [] array instead of having the pointer in the first place ... would be
> equivalent but save you from these contortions :)
> 
Right.

>> +	specs = 0;
>> +	sar_spec->type = type;
>> +
>> +	nla_for_each_nested(spec_list, tb[NL80211_SAR_ATTR_SPECS], rem) {
>> +		if (nla_parse(spec,
>> +			      NL80211_SAR_ATTR_SPECS_MAX,
>> +			      nla_data(spec_list),
>> +			      nla_len(spec_list),
>> +			      sar_specs_policy,
>> +			      NULL)) {
>> +			err = -EINVAL;
>> +			goto error;
>> +		}
>> +
>> +		/* for power type, power value must be presented */
>> +		if (!spec[NL80211_SAR_ATTR_SPECS_POWER] &&
>> +		    type == NL80211_SAR_TYPE_POWER) {
>> +			err = -EINVAL;
>> +			goto error;
>> +		}
> 
> Somewhere you should probably also check that the type matches the
> advertised type.
> 
Agree.

>> +		power = nla_get_u8(spec[NL80211_SAR_ATTR_SPECS_POWER]);
>> +		sar_spec->sub_specs[specs].power = power;
>> +
>> +		/* if NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX isn't present,
>> +		 * then the power applies to all bands. But it's only valid
>> +		 * for the first entry.
>> +		 */
>> +		if (!spec[NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX]) {
>> +			if (specs) {
>> +				err = -EINVAL;
>> +				goto error;
>> +			} else {
>> +				sar_spec->sub_specs[specs].freq_range_index =
>> +					NL80211_SAR_ALL_FREQ_RNAGES;
>> +				specs++;
>> +				break;
>> +			}
>> +		}
> 
> Yeah... not sure I see this as being worth it, vs. userspace just
> filling all the possible entries.
> 
Yes, let me remove this logic in v2.

>> +		index = nla_get_u8(spec[NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX]);
>> +		sar_spec->sub_specs[specs].freq_range_index = index;
>> +		specs++;
>> +	}
>> +
>> +	sar_spec->num_sub_specs = specs;
>> +
>> +	rdev->cur_cmd_info = info;
>> +	if (rdev->ops->set_sar_specs)
>> +		err = rdev_set_sar_specs(rdev, sar_spec);
>> +	else
>> +		err = -EOPNOTSUPP;
> 
> Might be worth checking this early and avoiding the allocation and all?
> 
> OTOH, that gives you error checking even if the driver was completely
> confused and advertised sar specs without the method ...
> 
Here, it checks whether mac80211 layer implements the function. It's 
ieee80211_set_sar_specs
to check whether driver has implemented set_sar function.

I'll move the check to the beginning of this function.

> johannes

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 1/2] nl80211: add common API to configure SAR power limitations.

[Brian Norris]

On Wed, Nov 4, 2020 at 12:44 AM Carl Huang <cjhuang@codeaurora.org> wrote:
> On 2020-09-28 20:36, Johannes Berg wrote:
> > On Tue, 2020-09-22 at 13:49 +0800, Carl Huang wrote:
> >> +struct cfg80211_sar_freq_ranges {
> >> +    u8 index;
> >
> > Does an index here make sense?
> >
> With agreement from Google, it's OK to remove it.

I'm not sure "Google" is the arbiter of the nl80211 API, even if we
are the current planned users ;)

But I think I agree with Johannes, that given the other plans (user
space must send all bands all the time; dropping the "apply to all
bands" support), an index isn't really necessary in either the user
space API or the internal representation handed down to drivers. All
bands should be specified, in order.

Brian

> >> +    u32 start_freq;
> >> +    u32 end_freq;
> >> +};

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 2/2] ath10k: allow dynamic SAR power limits via common API

[Brian Norris]

Hi,

On Tue, Sep 22, 2020 at 01:49:35PM +0800, Carl Huang wrote:
> ath10k assigns ath10k_mac_set_sar_specs to ath10k_ops, and
> this function is called when user space application calls
> NL80211_CMD_SET_SAR_SPECS. ath10k also registers SAR type,
> and supported frequency ranges to wiphy so user space can
> query SAR capabilities.
> 
> ath10k_mac_set_sar_specs further sets the power to firmware
> to limit the TX power.
> 
> This feature is controlled by hw parameter: dynamic_sar_support.
> 
> Signed-off-by: Carl Huang <cjhuang@codeaurora.org>
> ---

> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index 2e3eb5b..830c61f 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -81,6 +81,17 @@ static struct ieee80211_rate ath10k_rates_rev2[] = {
>  	{ .bitrate = 540, .hw_value = ATH10K_HW_RATE_OFDM_54M },
>  };
>  
> +static const struct cfg80211_sar_freq_ranges ath10k_sar_freq_ranges[] = {
> +	{ .index = 0, .start_freq = 2412000, .end_freq = 2484000 },

2412 MHz is a center frequency, but other parts of the nl80211 API use
band edges. For example:

 * @NL80211_ATTR_FREQ_RANGE_START: starting frequencry for the regulatory
 *      rule in KHz. This is not a center of frequency but an actual regulatory
 *      band edge.
 * @NL80211_ATTR_FREQ_RANGE_END: ending frequency for the regulatory rule
 *      in KHz. This is not a center a frequency but an actual regulatory
 *      band edge.

Seems like we should improve the nl80211.h docs (patch 1) and make these
edges (this patch).

> +	{ .index = 1, .start_freq = 2484000, .end_freq = 5865000 },
> +};
> +
> +static const struct cfg80211_sar_capa ath10k_sar_capa = {
> +	.type = NL80211_SAR_TYPE_POWER,
> +	.num_freq_ranges = (ARRAY_SIZE(ath10k_sar_freq_ranges)),
> +	.freq_ranges = &ath10k_sar_freq_ranges[0],
> +};
> +
>  #define ATH10K_MAC_FIRST_OFDM_RATE_IDX 4
>  
>  #define ath10k_a_rates (ath10k_rates + ATH10K_MAC_FIRST_OFDM_RATE_IDX)
> @@ -2880,6 +2891,95 @@ static int ath10k_mac_vif_recalc_txbf(struct ath10k *ar,
>  	return 0;
>  }
>  
> +static bool ath10k_mac_is_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(struct ath10k *ar)

This function should be static.

> +{
> +	int ret;
> +
> +	if (!ar->hw_params.dynamic_sar_support)
> +		return 0;

return -EOPNOTSUPP ?

> +
> +	if (ar->tx_power_2g_limit == 0 || ar->tx_power_5g_limit == 0)

ath10k_mac_txpower_recalc() doesn't care about this -- why should you?
This also seems especially weird, because one of the 2 could be valid
nonzero values, and yet you're silently rejecting it. Regardless, the
following seems wrong:

> +		return 0;

This should probably be an error.

> +
> +	if (!ath10k_mac_is_connected(ar))
> +		return 0;

Note to self (since this wasn't obvious to me the first read-through):
you're calling this function from ath10k_bss_assoc() too, so even if you
weren't connected the first time around, it'll get called later.

> +
> +	ret = ath10k_wmi_pdev_set_param(ar,
> +					ar->wmi.pdev_param->txpower_limit2g,
> +					ar->tx_power_2g_limit);
> +	if (ret) {
> +		ath10k_warn(ar, "failed to set 2.4G txpower %d: %d\n",
> +			    ar->tx_power_2g_limit, ret);
> +		return ret;
> +	}
> +
> +	ret = ath10k_wmi_pdev_set_param(ar,
> +					ar->wmi.pdev_param->txpower_limit5g,
> +					ar->tx_power_5g_limit);
> +	if (ret) {
> +		ath10k_warn(ar, "failed to set 5G txpower %d: %d\n",
> +			    ar->tx_power_5g_limit, ret);
> +		return ret;
> +	}

Hmm, so these are the same params configured by
ath10k_mac_txpower_recalc(), except that we're not taking into account
the limitations in ath10k_mac_txpower_recalc() (and vice versa) -- isn't
that bad? Should we be merging the SAR limitation into
ath10k_mac_txpower_recalc() and calling that instead?

Brian

> +
> +	ath10k_dbg(ar, ATH10K_DBG_MAC, "set txpower 2G:%d, 5G:%d successfully\n",
> +		   ar->tx_power_2g_limit, ar->tx_power_5g_limit);
> +
> +	return ret;
> +}
> +

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 1/2] nl80211: add common API to configure SAR power limitations.

[Brian Norris]

On Mon, Sep 28, 2020 at 02:36:09PM +0200, Johannes Berg wrote:
> > +#define NUM_MAX_NL80211_SAR_FREQ_RANGES 0xfe
> 
> but I'm not sure what these are used for in the first place, they seem
> more like internal implementation details?

I think the MAX value does have some utility in the API -- as mentioned
in other comments, if we're requiring that user space must SET all
ranges at the same time, then we need an expected way for user space to
SET a "don't care" or "MAX" or "null" value for a band. So if there's
some new band (e.g., 6 GHz?) that user space was not previously aware
of, it will know to use this placeholder.

I think that's kind of approximately what the purpose of this was? It's
not super clear in the documentation, so maybe that should be clarified
in writing in v2.

Brian

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 1/2] nl80211: add common API to configure SAR power limitations.

[Brian Norris]

Hi Carl,

Sorry, I lied; I have a few more notes after spending another day
looking at this:

On Tue, Sep 22, 2020 at 01:49:34PM +0800, Carl Huang wrote:
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -1663,6 +1663,55 @@ struct station_info {
> +/**
> + * @struct cfg80211_sar_chan_ranges - sar frequency ranges
> + * @index: the index of this range. It's used to specify
> + *	the frequency range when setting SAR power limitation
> + * @start_freq:  start channel frequency in kHZ. For example,
> + *	2.4G channel 1 is represented as 2412000
> + * @end_freq:    end channel frequency in kHZ

If you accept my comments in nl80211.h, you'll want to change this too.

> + */
> +struct cfg80211_sar_freq_ranges {
> +	u8 index;
> +	u32 start_freq;
> +	u32 end_freq;
> +};

> --- a/include/uapi/linux/nl80211.h
> +++ b/include/uapi/linux/nl80211.h

> + * @NL80211_SAR_ATTR_SPECS_START_FREQ: Required (u32) value to specify the start
> + *	frequency of this range to register SAR capability to wihpy and the unit
> + *	is kHZ
> + *
> + * @NL80211_SAR_ATTR_SPECS_END_FREQ: Required (u32) value to specify the end frequency
> + *	of this range to register SAR capability to wiphy and the unit is kHZ

The documentation here isn't clear whether these are center frequencies
or band edges. The cfg80211 comments do though (center freq). However,
this is inconsistent with NL80211_ATTR_FREQ_RANGE_START and
NL80211_ATTR_FREQ_RANGE_END -- I'd suggest being consistent?

> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c

> @@ -15331,6 +15496,14 @@ static const struct genl_ops nl80211_ops[] = {
>  		.internal_flags = NL80211_FLAG_NEED_NETDEV |
>  				  NL80211_FLAG_NEED_RTNL,
>  	},
> +	{
> +		.cmd = NL80211_CMD_SET_SAR_SPECS,
> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +		.doit = nl80211_set_sar_specs,
> +		.flags = GENL_UNS_ADMIN_PERM,
> +		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |

Is there a reason this needs to be UP? CMD_SET_WIPHY (which can also
configure TX power, a little differently) does not. Seems like this
could just be NL80211_FLAG_NEED_NETDEV -- or maybe not even that, if we
switch this to a WIPHY command like Johannes noted.

Brian

> +				  NL80211_FLAG_NEED_RTNL,
> +	},
>  };

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 2/2] ath10k: allow dynamic SAR power limits via common API

[Carl Huang]

On 2020-11-05 07:11, Brian Norris wrote:
> Hi,
> 
> On Tue, Sep 22, 2020 at 01:49:35PM +0800, Carl Huang wrote:
>> ath10k assigns ath10k_mac_set_sar_specs to ath10k_ops, and
>> this function is called when user space application calls
>> NL80211_CMD_SET_SAR_SPECS. ath10k also registers SAR type,
>> and supported frequency ranges to wiphy so user space can
>> query SAR capabilities.
>> 
>> ath10k_mac_set_sar_specs further sets the power to firmware
>> to limit the TX power.
>> 
>> This feature is controlled by hw parameter: dynamic_sar_support.
>> 
>> Signed-off-by: Carl Huang <cjhuang@codeaurora.org>
>> ---
> 
>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c 
>> b/drivers/net/wireless/ath/ath10k/mac.c
>> index 2e3eb5b..830c61f 100644
>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>> @@ -81,6 +81,17 @@ static struct ieee80211_rate ath10k_rates_rev2[] = 
>> {
>>  	{ .bitrate = 540, .hw_value = ATH10K_HW_RATE_OFDM_54M },
>>  };
>> 
>> +static const struct cfg80211_sar_freq_ranges ath10k_sar_freq_ranges[] 
>> = {
>> +	{ .index = 0, .start_freq = 2412000, .end_freq = 2484000 },
> 
> 2412 MHz is a center frequency, but other parts of the nl80211 API use
> band edges. For example:
> 
>  * @NL80211_ATTR_FREQ_RANGE_START: starting frequencry for the 
> regulatory
>  *      rule in KHz. This is not a center of frequency but an actual 
> regulatory
>  *      band edge.
>  * @NL80211_ATTR_FREQ_RANGE_END: ending frequency for the regulatory 
> rule
>  *      in KHz. This is not a center a frequency but an actual 
> regulatory
>  *      band edge.
> 
> Seems like we should improve the nl80211.h docs (patch 1) and make 
> these
> edges (this patch).
> 
>> +	{ .index = 1, .start_freq = 2484000, .end_freq = 5865000 },
>> +};
>> +
>> +static const struct cfg80211_sar_capa ath10k_sar_capa = {
>> +	.type = NL80211_SAR_TYPE_POWER,
>> +	.num_freq_ranges = (ARRAY_SIZE(ath10k_sar_freq_ranges)),
>> +	.freq_ranges = &ath10k_sar_freq_ranges[0],
>> +};
>> +
>>  #define ATH10K_MAC_FIRST_OFDM_RATE_IDX 4
>> 
>>  #define ath10k_a_rates (ath10k_rates + 
>> ATH10K_MAC_FIRST_OFDM_RATE_IDX)
>> @@ -2880,6 +2891,95 @@ static int ath10k_mac_vif_recalc_txbf(struct 
>> ath10k *ar,
>>  	return 0;
>>  }
>> 
>> +static bool ath10k_mac_is_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(struct ath10k *ar)
> 
> This function should be static.
> 
Right.

>> +{
>> +	int ret;
>> +
>> +	if (!ar->hw_params.dynamic_sar_support)
>> +		return 0;
> 
> return -EOPNOTSUPP ?
> 
sure

>> +
>> +	if (ar->tx_power_2g_limit == 0 || ar->tx_power_5g_limit == 0)
> 
> ath10k_mac_txpower_recalc() doesn't care about this -- why should you?
> This also seems especially weird, because one of the 2 could be valid
> nonzero values, and yet you're silently rejecting it. Regardless, the
> following seems wrong:
> 
Per current design, it's required for userspace to always set meaningful
power limitations.

Now in V2, 0 will be treated as "don't have SAR on this range".


>> +		return 0;
> 
> This should probably be an error.
> 
>> +
>> +	if (!ath10k_mac_is_connected(ar))
>> +		return 0;
> 
> Note to self (since this wasn't obvious to me the first read-through):
> you're calling this function from ath10k_bss_assoc() too, so even if 
> you
> weren't connected the first time around, it'll get called later.
> 
>> +
>> +	ret = ath10k_wmi_pdev_set_param(ar,
>> +					ar->wmi.pdev_param->txpower_limit2g,
>> +					ar->tx_power_2g_limit);
>> +	if (ret) {
>> +		ath10k_warn(ar, "failed to set 2.4G txpower %d: %d\n",
>> +			    ar->tx_power_2g_limit, ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = ath10k_wmi_pdev_set_param(ar,
>> +					ar->wmi.pdev_param->txpower_limit5g,
>> +					ar->tx_power_5g_limit);
>> +	if (ret) {
>> +		ath10k_warn(ar, "failed to set 5G txpower %d: %d\n",
>> +			    ar->tx_power_5g_limit, ret);
>> +		return ret;
>> +	}
> 
> Hmm, so these are the same params configured by
> ath10k_mac_txpower_recalc(), except that we're not taking into account
> the limitations in ath10k_mac_txpower_recalc() (and vice versa) -- 
> isn't
> that bad? Should we be merging the SAR limitation into
> ath10k_mac_txpower_recalc() and calling that instead?
> 
Good suggestions.

> Brian
> 
>> +
>> +	ath10k_dbg(ar, ATH10K_DBG_MAC, "set txpower 2G:%d, 5G:%d 
>> successfully\n",
>> +		   ar->tx_power_2g_limit, ar->tx_power_5g_limit);
>> +
>> +	return ret;
>> +}
>> +

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 1/2] nl80211: add common API to configure SAR power limitations.

[Carl Huang]

On 2020-11-05 07:27, Brian Norris wrote:
> Hi Carl,
> 
> Sorry, I lied; I have a few more notes after spending another day
> looking at this:
> 
> On Tue, Sep 22, 2020 at 01:49:34PM +0800, Carl Huang wrote:
>> --- a/include/net/cfg80211.h
>> +++ b/include/net/cfg80211.h
>> @@ -1663,6 +1663,55 @@ struct station_info {
>> +/**
>> + * @struct cfg80211_sar_chan_ranges - sar frequency ranges
>> + * @index: the index of this range. It's used to specify
>> + *	the frequency range when setting SAR power limitation
>> + * @start_freq:  start channel frequency in kHZ. For example,
>> + *	2.4G channel 1 is represented as 2412000
>> + * @end_freq:    end channel frequency in kHZ
> 
> If you accept my comments in nl80211.h, you'll want to change this too.
> 
Yes.

>> + */
>> +struct cfg80211_sar_freq_ranges {
>> +	u8 index;
>> +	u32 start_freq;
>> +	u32 end_freq;
>> +};
> 
>> --- a/include/uapi/linux/nl80211.h
>> +++ b/include/uapi/linux/nl80211.h
> 
>> + * @NL80211_SAR_ATTR_SPECS_START_FREQ: Required (u32) value to 
>> specify the start
>> + *	frequency of this range to register SAR capability to wihpy and 
>> the unit
>> + *	is kHZ
>> + *
>> + * @NL80211_SAR_ATTR_SPECS_END_FREQ: Required (u32) value to specify 
>> the end frequency
>> + *	of this range to register SAR capability to wiphy and the unit is 
>> kHZ
> 
> The documentation here isn't clear whether these are center frequencies
> or band edges. The cfg80211 comments do though (center freq). However,
> this is inconsistent with NL80211_ATTR_FREQ_RANGE_START and
> NL80211_ATTR_FREQ_RANGE_END -- I'd suggest being consistent?
> 
Yes. Will change to band edge.

>> --- a/net/mac80211/cfg.c
>> +++ b/net/mac80211/cfg.c
> 
>> @@ -15331,6 +15496,14 @@ static const struct genl_ops nl80211_ops[] = 
>> {
>>  		.internal_flags = NL80211_FLAG_NEED_NETDEV |
>>  				  NL80211_FLAG_NEED_RTNL,
>>  	},
>> +	{
>> +		.cmd = NL80211_CMD_SET_SAR_SPECS,
>> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
>> +		.doit = nl80211_set_sar_specs,
>> +		.flags = GENL_UNS_ADMIN_PERM,
>> +		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
> 
> Is there a reason this needs to be UP? CMD_SET_WIPHY (which can also
> configure TX power, a little differently) does not. Seems like this
> could just be NL80211_FLAG_NEED_NETDEV -- or maybe not even that, if we
> switch this to a WIPHY command like Johannes noted.
> 

Will change to NL80211_FLAG_NEED_WIPHY.

> Brian
> 
>> +				  NL80211_FLAG_NEED_RTNL,
>> +	},
>>  };
> 
> _______________________________________________
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 1/2] nl80211: add common API to configure SAR power limitations.

[Carl Huang]

On 2020-11-05 01:48, Brian Norris wrote:
> On Wed, Nov 4, 2020 at 12:44 AM Carl Huang <cjhuang@codeaurora.org> 
> wrote:
>> On 2020-09-28 20:36, Johannes Berg wrote:
>> > On Tue, 2020-09-22 at 13:49 +0800, Carl Huang wrote:
>> >> +struct cfg80211_sar_freq_ranges {
>> >> +    u8 index;
>> >
>> > Does an index here make sense?
>> >
>> With agreement from Google, it's OK to remove it.
> 
> I'm not sure "Google" is the arbiter of the nl80211 API, even if we
> are the current planned users ;)
> 
> But I think I agree with Johannes, that given the other plans (user
> space must send all bands all the time; dropping the "apply to all
> bands" support), an index isn't really necessary in either the user
> space API or the internal representation handed down to drivers. All
> bands should be specified, in order.
> 
> Brian
> 
The index here will be removed.

But let's keep the explicit index in SET command. I think it adds no
burden to userspace but has flexibility to skip some ranges as we
remove "all or nothing" limitation.


>> >> +    u32 start_freq;
>> >> +    u32 end_freq;
>> >> +};

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 2/2] ath10k: allow dynamic SAR power limits via common API

[Brian Norris]

On Thu, Nov 5, 2020 at 3:27 AM Carl Huang <cjhuang@codeaurora.org> wrote:
> On 2020-11-05 07:11, Brian Norris wrote:
> > On Tue, Sep 22, 2020 at 01:49:35PM +0800, Carl Huang wrote:
> >> +    if (ar->tx_power_2g_limit == 0 || ar->tx_power_5g_limit == 0)
> >
> > ath10k_mac_txpower_recalc() doesn't care about this -- why should you?
> > This also seems especially weird, because one of the 2 could be valid
> > nonzero values, and yet you're silently rejecting it. Regardless, the
> > following seems wrong:
> >
> Per current design, it's required for userspace to always set meaningful
> power limitations.

Well, that wasn't specified anywhere (in the docs nor in the nl80211
implementation); if one wanted to enforce that, it should probably go
in the nl80211 layer, not the driver implementation. Otherwise, we're
very prone to varying driver implementations (not a good thing).

> Now in V2, 0 will be treated as "don't have SAR on this range".

OK... I guess that's reasonable, although that should be explicitly
called out in nl80211.h.

Brian

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 1/2] nl80211: add common API to configure SAR power limitations.

[Carl Huang]

On 2020-11-06 02:25, Brian Norris wrote:
> On Thu, Nov 5, 2020 at 3:10 AM Carl Huang <cjhuang@codeaurora.org> 
> wrote:
>> On 2020-11-05 16:35, Kalle Valo wrote:
>> > Brian Norris <briannorris@chromium.org> writes:
>> >> On Tue, Nov 3, 2020 at 11:32 PM Carl Huang <cjhuang@codeaurora.org>
>> >> wrote:
>> >>> On 2020-11-04 10:00, Brian Norris wrote:
>> >>> > What are the ABI guarantees around a given driver/chip's 'sar_capa'?
>> >>> > Do we guarantee that if the driver supports N ranges of certain bands,
>> >>> > that it will always continue to support those bands?
> 
> To be clear: the answer here is "no." So we have to map out what the
> user/kernel interaction looks like when they change.
> 
>> >> ...
>> >>> For a given chip(at least a QCOM chip), we don't see that the
>> >>> range will grow or change.
>> >>
>> >> That's good to know. But that's not quite the same as an ABI
>> >> guarantee.
>> >
>> > I'm not sure if I understood Brian's question correctly, but I have
>> > concerns on the assumption that frequency ranges never change. For
>> > example, in ath10k we have a patch[1] under discussion which adds more
>> > channels and in ath11k we added 6 GHz band after initial ath11k support
>> > landed. And I would not be surprised if in some boards/platforms a
>> > certain band is disabled due to cotting costs (no antenna etc).
> 
> Right, I certainly was not taking the "never change bands" claim from
> Carl at face value ;) This is exactly why I was asking.
> 
>> > My
>> > preference is to have a robust interface which would be designed to
>> > handle these kind of changes.
> 
> Sure.
> 
>> > [1] [PATCH] ath10k: enable advertising support for channels 32, 68 and
>> > 98
>> 
>> So the trick here is even if more channels are supported, it doesn't
>> mean
>> that it can support different SAR setting on these new channels. In 
>> this
>> case,
>> it likely falls into 5G range. It's safe for driver to extend the 5G
>> range and
>> doesn't break userspace. (68 and 98 are already in the 5G range, so
>> driver just
>> extends the start edge freq to 32 here.).
> 
> You can't just wave your hands and say it "doesn't break userspace" --
> you have to think about how user space can use this API.
> 
> Specifically, consider that user space is not going to memorize
> indeces, as those are per-driver implementation details; it's going to
> memorize frequency bands. It wants to cross reference those with the
> results of the GET/DUMP API before it translates those into indeces
> for SET. As you're describing it, user space will have to have some
> kind of "fuzziness" to its logic -- today, it thinks the 5G band is
> [X,Y], but tomorrow it might expand to [X-N, Y+M]. So user space
> should just ensure that it configures any band that intersects with
> [X,Y], even though it didn't know about [X-N,X] or [Y,Y+M]? That logic
> covers splits too, I suppose.
> 
> There's still the question of ranges that user space has no knowledge
> of (i.e., no intersection with any known [X,Y]). I think there's two
> approaches that are roughly equivalent:
> 1) require SET operations to specify all bands, and designate a NULL
> or MAX value that user space should use for unknown/unconfigured bands
> [or, user space uses some kind of "extension" from the nearest known
> band, just to be safe?]
> 2) allow SET operations to specify a subset of supported bands [gray
> area: what happens with the unconfigured band(s)? left as-is? use
> max?]
> 
> We're approximately in #1 right now. If we're explicit about how
> that's supposed to work, then I think we can stay with that. Although
> it sounds like Carl is moving toward #2 (allow subsets).
> 
>> But for flexibility, given 6 GHz as example here, let's keep the
>> explicit
>> index for SET command. For sar_capa advertisement, the explicit index 
>> is
>> dropped as Johannes suggested. New ranges can only be appended to
>> existing
>> ones. Like Brian said, only add or split is allowed.
> 
>> The complexity to
>> handle
>> splitted range Vs whole range is left to WLAN driver itself.
> 
> Hmm? I thought we're keeping the driver simple. I'm OK with that (and
> moving a little more complexity into user space) as long as we're
> clear about it.
> 

I've sent  [PATCH 0/3] add common API to configure SAR, please let's 
start
from there again.


> Brian
> 
>> Userspace can SET any ranges which are advertised by WLAN driver. It's
>> not required to set all ranges and userspace can skip any ranges.
> 
> _______________________________________________
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 1/2] nl80211: add common API to configure SAR power limitations.

[Brian Norris]

On Thu, Nov 5, 2020 at 3:10 AM Carl Huang <cjhuang@codeaurora.org> wrote:
> On 2020-11-05 16:35, Kalle Valo wrote:
> > Brian Norris <briannorris@chromium.org> writes:
> >> On Tue, Nov 3, 2020 at 11:32 PM Carl Huang <cjhuang@codeaurora.org>
> >> wrote:
> >>> On 2020-11-04 10:00, Brian Norris wrote:
> >>> > What are the ABI guarantees around a given driver/chip's 'sar_capa'?
> >>> > Do we guarantee that if the driver supports N ranges of certain bands,
> >>> > that it will always continue to support those bands?

To be clear: the answer here is "no." So we have to map out what the
user/kernel interaction looks like when they change.

> >> ...
> >>> For a given chip(at least a QCOM chip), we don't see that the
> >>> range will grow or change.
> >>
> >> That's good to know. But that's not quite the same as an ABI
> >> guarantee.
> >
> > I'm not sure if I understood Brian's question correctly, but I have
> > concerns on the assumption that frequency ranges never change. For
> > example, in ath10k we have a patch[1] under discussion which adds more
> > channels and in ath11k we added 6 GHz band after initial ath11k support
> > landed. And I would not be surprised if in some boards/platforms a
> > certain band is disabled due to cotting costs (no antenna etc).

Right, I certainly was not taking the "never change bands" claim from
Carl at face value ;) This is exactly why I was asking.

> > My
> > preference is to have a robust interface which would be designed to
> > handle these kind of changes.

Sure.

> > [1] [PATCH] ath10k: enable advertising support for channels 32, 68 and
> > 98
>
> So the trick here is even if more channels are supported, it doesn't
> mean
> that it can support different SAR setting on these new channels. In this
> case,
> it likely falls into 5G range. It's safe for driver to extend the 5G
> range and
> doesn't break userspace. (68 and 98 are already in the 5G range, so
> driver just
> extends the start edge freq to 32 here.).

You can't just wave your hands and say it "doesn't break userspace" --
you have to think about how user space can use this API.

Specifically, consider that user space is not going to memorize
indeces, as those are per-driver implementation details; it's going to
memorize frequency bands. It wants to cross reference those with the
results of the GET/DUMP API before it translates those into indeces
for SET. As you're describing it, user space will have to have some
kind of "fuzziness" to its logic -- today, it thinks the 5G band is
[X,Y], but tomorrow it might expand to [X-N, Y+M]. So user space
should just ensure that it configures any band that intersects with
[X,Y], even though it didn't know about [X-N,X] or [Y,Y+M]? That logic
covers splits too, I suppose.

There's still the question of ranges that user space has no knowledge
of (i.e., no intersection with any known [X,Y]). I think there's two
approaches that are roughly equivalent:
1) require SET operations to specify all bands, and designate a NULL
or MAX value that user space should use for unknown/unconfigured bands
[or, user space uses some kind of "extension" from the nearest known
band, just to be safe?]
2) allow SET operations to specify a subset of supported bands [gray
area: what happens with the unconfigured band(s)? left as-is? use
max?]

We're approximately in #1 right now. If we're explicit about how
that's supposed to work, then I think we can stay with that. Although
it sounds like Carl is moving toward #2 (allow subsets).

> But for flexibility, given 6 GHz as example here, let's keep the
> explicit
> index for SET command. For sar_capa advertisement, the explicit index is
> dropped as Johannes suggested. New ranges can only be appended to
> existing
> ones. Like Brian said, only add or split is allowed.

> The complexity to
> handle
> splitted range Vs whole range is left to WLAN driver itself.

Hmm? I thought we're keeping the driver simple. I'm OK with that (and
moving a little more complexity into user space) as long as we're
clear about it.

Brian

> Userspace can SET any ranges which are advertised by WLAN driver. It's
> not required to set all ranges and userspace can skip any ranges.

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 1/2] nl80211: add common API to configure SAR power limitations.

[Carl Huang]

On 2020-11-05 01:44, Brian Norris wrote:
> + ath10k
> 
> [ I realize I replied to the "wrong" RFC v1; I fell trap to Kalle's 
> note:
> 
> "When you submit a new version mark it as "v2". Otherwise people don't
> know what's the latest version." ]
> 
> On Tue, Nov 3, 2020 at 11:32 PM Carl Huang <cjhuang@codeaurora.org> 
> wrote:
>> On 2020-11-04 10:00, Brian Norris wrote:
>> > What are the ABI guarantees around a given driver/chip's 'sar_capa'?
>> > Do we guarantee that if the driver supports N ranges of certain bands,
>> > that it will always continue to support those bands?
> ...
>> For a given chip(at least a QCOM chip), we don't see that the
>> range will grow or change.
> 
> That's good to know. But that's not quite the same as an ABI guarantee.
> 
>> In addition, with current index-power SET method, it's hard for driver
>> to know what the index mean given your example. Does the index mean
>> [5,5 + x] or [5, 5 + x/2] ?  So it's required for user-space to 
>> specify
>> all the ranges.
> 
> Well, we'd have to change the API (which is why I'm asking now) if we
> wanted the kernel to handle this gracefully. We'd have to retain the
> index (which, it sounds like you might be dropping if things go as
> Johannes suggested), so user space can continue to request the old
> range even if the driver also splits up a new range.
> 
> More explicitly, something like this:
> 
> Linux version A:
> ath10k supports:
> 0: 2G band
> 1: 5G band
> 
> Linux version B:
> ath10k supports:
> 0: 2G band
> 1: 5G band
> 2: 1st half of 5G band
> 3: 2nd half of 5G band
> 
> Indexes 2 and 3 would be overlapping with 1 of course, but it does
> mean that the following SET request will work on both A and B:
> 
> SET
> index 0 -> power X
> index 1 -> power Y
> 
> Notably, that also requires nl80211 allow specifying only a subset of
> bands in a SET request.
> 
> But spelling that out, the proposal sounds more complicated than it's
> worth -- it's probably better to let user space handle the complexity.
> So that goes back to making ABI requirements clear, so we don't have
> kernel/user mixups/regressions down the line.
> 
>> The number of ranges is quite small, so the SET itself is not a
>> problem to specify all.
> 
> Sure, but it does mean that if I (user space) don't trust that driver
> support remains constant, I have to do some negotiation. I would
> already do some verification via the get/dump API of course, but
> negotiation is pretty complex if there is unbounded flexibility. It
> would be nice to spell out what sort of negotiation is reasonable as
> part of the ABI. For example, it might be reasonable to say that bands
> should never be combined; only ever added or split.
> 
> BTW, considering the possibility of "added" bands, how about this
> example: if ath10k were to add 6GHz support (and SAR to go with it),
> user space would have to learn to specify limits for it too, due to
> the "all or nothing" limitation? It'd be good to be up front about
> this, similar to the previous paragraph.
> 
Like what I've replied in another email. Let's remove "all or nothing"
limitation. If certain ranges are not SET, then these ranges just don't
have SAR power limitation.

Just FYI, ath10k will never support 6GHz, it's ath11k to support it.


>> Brian, are you fine that we go with this proposal? I'll send
>> V2 based on the comments from Johannes and Abhishek.
> 
> I think I'm fine with it; my main concerns around ambiguities, so
> maybe it just needs more explicit mentions in the docs (commit
> messages and/or comments). I'd be happy to see v2 and go from there.
> 
> Brian

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 1/2] nl80211: add common API to configure SAR power limitations.

[Carl Huang]

On 2020-11-05 16:35, Kalle Valo wrote:
> Brian Norris <briannorris@chromium.org> writes:
> 
>> + ath10k
>> 
>> [ I realize I replied to the "wrong" RFC v1; I fell trap to Kalle's 
>> note:
>> 
>> "When you submit a new version mark it as "v2". Otherwise people don't
>> know what's the latest version." ]
>> 
>> On Tue, Nov 3, 2020 at 11:32 PM Carl Huang <cjhuang@codeaurora.org> 
>> wrote:
>>> On 2020-11-04 10:00, Brian Norris wrote:
>>> > What are the ABI guarantees around a given driver/chip's 'sar_capa'?
>>> > Do we guarantee that if the driver supports N ranges of certain bands,
>>> > that it will always continue to support those bands?
>> ...
>>> For a given chip(at least a QCOM chip), we don't see that the
>>> range will grow or change.
>> 
>> That's good to know. But that's not quite the same as an ABI 
>> guarantee.
> 
> I'm not sure if I understood Brian's question correctly, but I have
> concerns on the assumption that frequency ranges never change. For
> example, in ath10k we have a patch[1] under discussion which adds more
> channels and in ath11k we added 6 GHz band after initial ath11k support
> landed. And I would not be surprised if in some boards/platforms a
> certain band is disabled due to cotting costs (no antenna etc). My
> preference is to have a robust interface which would be designed to
> handle these kind of changes.
> 
> [1] [PATCH] ath10k: enable advertising support for channels 32, 68 and 
> 98

So the trick here is even if more channels are supported, it doesn't 
mean
that it can support different SAR setting on these new channels. In this 
case,
it likely falls into 5G range. It's safe for driver to extend the 5G 
range and
doesn't break userspace. (68 and 98 are already in the 5G range, so 
driver just
extends the start edge freq to 32 here.).

But for flexibility, given 6 GHz as example here, let's keep the 
explicit
index for SET command. For sar_capa advertisement, the explicit index is
dropped as Johannes suggested. New ranges can only be appended to 
existing
ones. Like Brian said, only add or split is allowed. The complexity to 
handle
splitted range Vs whole range is left to WLAN driver itself.

Userspace can SET any ranges which are advertised by WLAN driver. It's
not required to set all ranges and userspace can skip any ranges.

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 1/2] nl80211: add common API to configure SAR power limitations.

[Kalle Valo]

Brian Norris <briannorris@chromium.org> writes:

> + ath10k
>
> [ I realize I replied to the "wrong" RFC v1; I fell trap to Kalle's note:
>
> "When you submit a new version mark it as "v2". Otherwise people don't
> know what's the latest version." ]
>
> On Tue, Nov 3, 2020 at 11:32 PM Carl Huang <cjhuang@codeaurora.org> wrote:
>> On 2020-11-04 10:00, Brian Norris wrote:
>> > What are the ABI guarantees around a given driver/chip's 'sar_capa'?
>> > Do we guarantee that if the driver supports N ranges of certain bands,
>> > that it will always continue to support those bands?
> ...
>> For a given chip(at least a QCOM chip), we don't see that the
>> range will grow or change.
>
> That's good to know. But that's not quite the same as an ABI guarantee.

I'm not sure if I understood Brian's question correctly, but I have
concerns on the assumption that frequency ranges never change. For
example, in ath10k we have a patch[1] under discussion which adds more
channels and in ath11k we added 6 GHz band after initial ath11k support
landed. And I would not be surprised if in some boards/platforms a
certain band is disabled due to cotting costs (no antenna etc). My
preference is to have a robust interface which would be designed to
handle these kind of changes.

[1] [PATCH] ath10k: enable advertising support for channels 32, 68 and 98

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

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

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 1/2] nl80211: add common API to configure SAR power limitations.

[Brian Norris]

+ ath10k

[ I realize I replied to the "wrong" RFC v1; I fell trap to Kalle's note:

"When you submit a new version mark it as "v2". Otherwise people don't
know what's the latest version." ]

On Tue, Nov 3, 2020 at 11:32 PM Carl Huang <cjhuang@codeaurora.org> wrote:
> On 2020-11-04 10:00, Brian Norris wrote:
> > What are the ABI guarantees around a given driver/chip's 'sar_capa'?
> > Do we guarantee that if the driver supports N ranges of certain bands,
> > that it will always continue to support those bands?
...
> For a given chip(at least a QCOM chip), we don't see that the
> range will grow or change.

That's good to know. But that's not quite the same as an ABI guarantee.

> In addition, with current index-power SET method, it's hard for driver
> to know what the index mean given your example. Does the index mean
> [5,5 + x] or [5, 5 + x/2] ?  So it's required for user-space to specify
> all the ranges.

Well, we'd have to change the API (which is why I'm asking now) if we
wanted the kernel to handle this gracefully. We'd have to retain the
index (which, it sounds like you might be dropping if things go as
Johannes suggested), so user space can continue to request the old
range even if the driver also splits up a new range.

More explicitly, something like this:

Linux version A:
ath10k supports:
0: 2G band
1: 5G band

Linux version B:
ath10k supports:
0: 2G band
1: 5G band
2: 1st half of 5G band
3: 2nd half of 5G band

Indexes 2 and 3 would be overlapping with 1 of course, but it does
mean that the following SET request will work on both A and B:

SET
index 0 -> power X
index 1 -> power Y

Notably, that also requires nl80211 allow specifying only a subset of
bands in a SET request.

But spelling that out, the proposal sounds more complicated than it's
worth -- it's probably better to let user space handle the complexity.
So that goes back to making ABI requirements clear, so we don't have
kernel/user mixups/regressions down the line.

> The number of ranges is quite small, so the SET itself is not a
> problem to specify all.

Sure, but it does mean that if I (user space) don't trust that driver
support remains constant, I have to do some negotiation. I would
already do some verification via the get/dump API of course, but
negotiation is pretty complex if there is unbounded flexibility. It
would be nice to spell out what sort of negotiation is reasonable as
part of the ABI. For example, it might be reasonable to say that bands
should never be combined; only ever added or split.

BTW, considering the possibility of "added" bands, how about this
example: if ath10k were to add 6GHz support (and SAR to go with it),
user space would have to learn to specify limits for it too, due to
the "all or nothing" limitation? It'd be good to be up front about
this, similar to the previous paragraph.

> Brian, are you fine that we go with this proposal? I'll send
> V2 based on the comments from Johannes and Abhishek.

I think I'm fine with it; my main concerns around ambiguities, so
maybe it just needs more explicit mentions in the docs (commit
messages and/or comments). I'd be happy to see v2 and go from there.

Brian

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k