[PATCH 0/3] add common API to configure SAR

[PATCH 0/3] add common API to configure SAR

[Carl Huang]

This patchset is to add common API to configure SAR.
The mechanism is wlan driver registers sar capability
to wiphy and userspace queries it. Userspace then sets
power limiation to wlan driver.

Carl Huang (3):
  nl80211: add common API to configure SAR power limitations.
  mac80211: add ieee80211_set_sar_specs
  ath10k: allow dynamic SAR power limits via common API

 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  | 221 ++++++++++++++++++++++++---------
 include/net/cfg80211.h                 |  51 ++++++++
 include/net/mac80211.h                 |   2 +
 include/uapi/linux/nl80211.h           | 101 +++++++++++++++
 net/mac80211/cfg.c                     |  12 ++
 net/wireless/nl80211.c                 | 150 ++++++++++++++++++++++
 net/wireless/rdev-ops.h                |  12 ++
 net/wireless/trace.h                   |  19 +++
 11 files changed, 530 insertions(+), 59 deletions(-)

-- 
2.7.4

[PATCH 0/3] add common API to configure SAR

[Carl Huang]

This patchset is to add common API to configure SAR.
The mechanism is wlan driver registers sar capability
to wiphy and userspace queries it. Userspace then sets
power limiation to wlan driver.

Carl Huang (3):
  nl80211: add common API to configure SAR power limitations.
  mac80211: add ieee80211_set_sar_specs
  ath10k: allow dynamic SAR power limits via common API

 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  | 221 ++++++++++++++++++++++++---------
 include/net/cfg80211.h                 |  51 ++++++++
 include/net/mac80211.h                 |   2 +
 include/uapi/linux/nl80211.h           | 101 +++++++++++++++
 net/mac80211/cfg.c                     |  12 ++
 net/wireless/nl80211.c                 | 150 ++++++++++++++++++++++
 net/wireless/rdev-ops.h                |  12 ++
 net/wireless/trace.h                   |  19 +++
 11 files changed, 530 insertions(+), 59 deletions(-)

-- 
2.7.4


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

[PATCH 1/3] 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 01 00 00 00 00 00 38 00 26 81
08 00 01 00 00 00 00 00 2c 00 02 80 14 00 00 80
05 00 02 00 00 00 00 00 05 00 01 00 38 00 00 00
14 00 01 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_WIPHY:     0x01(phy idx is 0)
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       |  51 +++++++++++++++
 include/uapi/linux/nl80211.h | 101 +++++++++++++++++++++++++++++
 net/wireless/nl80211.c       | 150 +++++++++++++++++++++++++++++++++++++++++++
 net/wireless/rdev-ops.h      |  12 ++++
 net/wireless/trace.h         |  19 ++++++
 5 files changed, 333 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index c9bce9b..07e44d5 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1663,6 +1663,54 @@ struct station_info {
 	u8 connected_to_as;
 };
 
+/**
+ * struct cfg80211_sar_sub_specs - sub specs limit
+ * @power: power limitation 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
+ * @start_freq:  start range edge frequency
+ * @end_freq:    end range edge frequency
+ */
+struct cfg80211_sar_freq_ranges {
+	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
+ * @freq_ranges: memory to hold the freq ranges.
+ *
+ * Note: WLAN driver may append new ranges or split an existing
+ * range to small ones and then append them.
+ */
+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 +4201,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 +4968,8 @@ struct wiphy {
 
 	u8 max_data_retry_count;
 
+	const struct cfg80211_sar_capa *sar_capa;
+
 	char priv[] __aligned(NETDEV_ALIGN);
 };
 
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 0584e0d..a8fd18a 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1177,6 +1177,10 @@
  *	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_ATTR_WIPHY is used to
+ *	specify the wiphy index to be applied to.
+ *
  * @NL80211_CMD_MAX: highest used command number
  * @__NL80211_CMD_AFTER_LAST: internal use
  */
@@ -1407,6 +1411,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 +2519,11 @@ 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. The message contains fileds
+ *	of %nl80211_sar_attrs which specifies the sar type and related
+ *	sar specs. Sar specs 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 +3006,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 +7017,92 @@ 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: power limitation specified in 0.25dbm unit
+ *
+ */
+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: the SAR type and it's defined in %nl80211_sar_type.
+ *
+ * @NL80211_SAR_ATTR_SPECS: Nested array of SAR power
+ *	limit specifications. Each specification contains a set
+ *      of %nl80211_sar_specs_attrs.
+ *
+ *      For SET operation, it contains array of NL80211_SAR_ATTR_SPECS_POWER
+ *	and NL80211_SAR_ATTR_SPECS_RANGE_INDEX.
+ *
+ *	For sar_capa dump, it contains array of NL80211_SAR_ATTR_SPECS_START_FREQ
+ *	and NL80211_SAR_ATTR_SPECS_END_FREQ.
+ *
+ * @__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,
+};
+
+/**
+ * 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).
+ *	0 means userspace doesn't have SAR limitation on this associated range.
+ *
+ * @NL80211_SAR_ATTR_SPECS_RANGE_INDEX: Required (u32) value to specify the
+ *	index of exported freq range table and the associated power limitation
+ *	is applied to this range.
+ *
+ *	Userspace isn't required to set all the ranges advertised by WLAN driver,
+ *	and userspace can skip some certain ranges. These skipped ranges don't
+ *	have SAR limitations, and these are same as setting the
+ *	%NL80211_SAR_ATTR_SPECS_POWER to 0. But it's required to set at least one range,
+ *	no matter the power limiation is 0 or not.
+ *
+ *	Every SET operation overwrites previous SET operation.
+ *
+ * @NL80211_SAR_ATTR_SPECS_START_FREQ: Required (u32) value to specify the start
+ *	frequency of this range edge when registering SAR capability to wiphy. It's
+ *	not a channel center frequency. The unit is KHz.
+ *
+ * @NL80211_SAR_ATTR_SPECS_END_FREQ: Required (u32) value to specify the end frequency
+ *	of this range edge when registering SAR capability to wiphy. It's not a channel
+ *	center frequency. 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_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/wireless/nl80211.c b/net/wireless/nl80211.c
index 262b04d..c278e0d 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -367,6 +367,18 @@ 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_RANGE_INDEX] = {.type = NLA_U8 },
+};
+
+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 +687,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 */
@@ -2034,6 +2047,52 @@ 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_u32(msg, NL80211_SAR_ATTR_TYPE, rdev->wiphy.sar_capa->type))
+		goto fail;
+
+	specs = nla_nest_start(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(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_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;
@@ -2284,6 +2343,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 			CMD(set_multicast_to_unicast, SET_MULTICAST_TO_UNICAST);
 			CMD(update_connect_params, UPDATE_CONNECT_PARAMS);
 			CMD(update_ft_ies, UPDATE_FT_IES);
+			CMD(set_sar_specs, SET_SAR_SPECS);
 		}
 #undef CMD
 
@@ -2597,6 +2657,11 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 
 		if (nl80211_put_tid_config_support(rdev, msg))
 			goto nla_put_failure;
+		state->split_start++;
+		break;
+	case 16:
+		if (nl80211_put_sar_specs(rdev, msg))
+			goto nla_put_failure;
 
 		/* done */
 		state->split_start = 0;
@@ -14477,6 +14542,83 @@ 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;
+	enum nl80211_sar_type type;
+	struct nlattr *spec_list;
+	u8 power, index, specs;
+	int rem, err;
+
+	if (!rdev->wiphy.sar_capa || !rdev->ops->set_sar_specs)
+		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] || !tb[NL80211_SAR_ATTR_SPECS])
+		return -EINVAL;
+
+	type = nla_get_u32(tb[NL80211_SAR_ATTR_TYPE]);
+	if (type != rdev->wiphy.sar_capa->type)
+		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->type = type;
+	specs = 0;
+	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 and index must be presented */
+		if ((!spec[NL80211_SAR_ATTR_SPECS_POWER] ||
+		     !spec[NL80211_SAR_ATTR_SPECS_RANGE_INDEX]) &&
+		    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;
+
+		index = nla_get_u8(spec[NL80211_SAR_ATTR_SPECS_RANGE_INDEX]);
+		sar_spec->sub_specs[specs].freq_range_index = index;
+
+		specs++;
+	}
+
+	sar_spec->num_sub_specs = specs;
+
+	rdev->cur_cmd_info = info;
+	err = rdev_set_sar_specs(rdev, sar_spec);
+	rdev->cur_cmd_info = NULL;
+error:
+	kfree(sar_spec);
+	return err;
+}
+
 static const struct genl_ops nl80211_ops[] = {
 	{
 		.cmd = NL80211_CMD_GET_WIPHY,
@@ -15335,6 +15477,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_WIPHY |
+				  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

[PATCH 1/3] 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 01 00 00 00 00 00 38 00 26 81
08 00 01 00 00 00 00 00 2c 00 02 80 14 00 00 80
05 00 02 00 00 00 00 00 05 00 01 00 38 00 00 00
14 00 01 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_WIPHY:     0x01(phy idx is 0)
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       |  51 +++++++++++++++
 include/uapi/linux/nl80211.h | 101 +++++++++++++++++++++++++++++
 net/wireless/nl80211.c       | 150 +++++++++++++++++++++++++++++++++++++++++++
 net/wireless/rdev-ops.h      |  12 ++++
 net/wireless/trace.h         |  19 ++++++
 5 files changed, 333 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index c9bce9b..07e44d5 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1663,6 +1663,54 @@ struct station_info {
 	u8 connected_to_as;
 };
 
+/**
+ * struct cfg80211_sar_sub_specs - sub specs limit
+ * @power: power limitation 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
+ * @start_freq:  start range edge frequency
+ * @end_freq:    end range edge frequency
+ */
+struct cfg80211_sar_freq_ranges {
+	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
+ * @freq_ranges: memory to hold the freq ranges.
+ *
+ * Note: WLAN driver may append new ranges or split an existing
+ * range to small ones and then append them.
+ */
+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 +4201,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 +4968,8 @@ struct wiphy {
 
 	u8 max_data_retry_count;
 
+	const struct cfg80211_sar_capa *sar_capa;
+
 	char priv[] __aligned(NETDEV_ALIGN);
 };
 
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 0584e0d..a8fd18a 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1177,6 +1177,10 @@
  *	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_ATTR_WIPHY is used to
+ *	specify the wiphy index to be applied to.
+ *
  * @NL80211_CMD_MAX: highest used command number
  * @__NL80211_CMD_AFTER_LAST: internal use
  */
@@ -1407,6 +1411,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 +2519,11 @@ 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. The message contains fileds
+ *	of %nl80211_sar_attrs which specifies the sar type and related
+ *	sar specs. Sar specs 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 +3006,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 +7017,92 @@ 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: power limitation specified in 0.25dbm unit
+ *
+ */
+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: the SAR type and it's defined in %nl80211_sar_type.
+ *
+ * @NL80211_SAR_ATTR_SPECS: Nested array of SAR power
+ *	limit specifications. Each specification contains a set
+ *      of %nl80211_sar_specs_attrs.
+ *
+ *      For SET operation, it contains array of NL80211_SAR_ATTR_SPECS_POWER
+ *	and NL80211_SAR_ATTR_SPECS_RANGE_INDEX.
+ *
+ *	For sar_capa dump, it contains array of NL80211_SAR_ATTR_SPECS_START_FREQ
+ *	and NL80211_SAR_ATTR_SPECS_END_FREQ.
+ *
+ * @__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,
+};
+
+/**
+ * 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).
+ *	0 means userspace doesn't have SAR limitation on this associated range.
+ *
+ * @NL80211_SAR_ATTR_SPECS_RANGE_INDEX: Required (u32) value to specify the
+ *	index of exported freq range table and the associated power limitation
+ *	is applied to this range.
+ *
+ *	Userspace isn't required to set all the ranges advertised by WLAN driver,
+ *	and userspace can skip some certain ranges. These skipped ranges don't
+ *	have SAR limitations, and these are same as setting the
+ *	%NL80211_SAR_ATTR_SPECS_POWER to 0. But it's required to set at least one range,
+ *	no matter the power limiation is 0 or not.
+ *
+ *	Every SET operation overwrites previous SET operation.
+ *
+ * @NL80211_SAR_ATTR_SPECS_START_FREQ: Required (u32) value to specify the start
+ *	frequency of this range edge when registering SAR capability to wiphy. It's
+ *	not a channel center frequency. The unit is KHz.
+ *
+ * @NL80211_SAR_ATTR_SPECS_END_FREQ: Required (u32) value to specify the end frequency
+ *	of this range edge when registering SAR capability to wiphy. It's not a channel
+ *	center frequency. 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_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/wireless/nl80211.c b/net/wireless/nl80211.c
index 262b04d..c278e0d 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -367,6 +367,18 @@ 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_RANGE_INDEX] = {.type = NLA_U8 },
+};
+
+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 +687,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 */
@@ -2034,6 +2047,52 @@ 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_u32(msg, NL80211_SAR_ATTR_TYPE, rdev->wiphy.sar_capa->type))
+		goto fail;
+
+	specs = nla_nest_start(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(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_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;
@@ -2284,6 +2343,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 			CMD(set_multicast_to_unicast, SET_MULTICAST_TO_UNICAST);
 			CMD(update_connect_params, UPDATE_CONNECT_PARAMS);
 			CMD(update_ft_ies, UPDATE_FT_IES);
+			CMD(set_sar_specs, SET_SAR_SPECS);
 		}
 #undef CMD
 
@@ -2597,6 +2657,11 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 
 		if (nl80211_put_tid_config_support(rdev, msg))
 			goto nla_put_failure;
+		state->split_start++;
+		break;
+	case 16:
+		if (nl80211_put_sar_specs(rdev, msg))
+			goto nla_put_failure;
 
 		/* done */
 		state->split_start = 0;
@@ -14477,6 +14542,83 @@ 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;
+	enum nl80211_sar_type type;
+	struct nlattr *spec_list;
+	u8 power, index, specs;
+	int rem, err;
+
+	if (!rdev->wiphy.sar_capa || !rdev->ops->set_sar_specs)
+		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] || !tb[NL80211_SAR_ATTR_SPECS])
+		return -EINVAL;
+
+	type = nla_get_u32(tb[NL80211_SAR_ATTR_TYPE]);
+	if (type != rdev->wiphy.sar_capa->type)
+		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->type = type;
+	specs = 0;
+	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 and index must be presented */
+		if ((!spec[NL80211_SAR_ATTR_SPECS_POWER] ||
+		     !spec[NL80211_SAR_ATTR_SPECS_RANGE_INDEX]) &&
+		    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;
+
+		index = nla_get_u8(spec[NL80211_SAR_ATTR_SPECS_RANGE_INDEX]);
+		sar_spec->sub_specs[specs].freq_range_index = index;
+
+		specs++;
+	}
+
+	sar_spec->num_sub_specs = specs;
+
+	rdev->cur_cmd_info = info;
+	err = rdev_set_sar_specs(rdev, sar_spec);
+	rdev->cur_cmd_info = NULL;
+error:
+	kfree(sar_spec);
+	return err;
+}
+
 static const struct genl_ops nl80211_ops[] = {
 	{
 		.cmd = NL80211_CMD_GET_WIPHY,
@@ -15335,6 +15477,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_WIPHY |
+				  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

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

[Johannes Berg]

Hi,

Looks pretty good. Some comments, mostly nits, below.


> +/**
> + * nl80211_sar_attrs - Attributes for SAR spec

missing enum

> + *
> + * @NL80211_SAR_ATTR_TYPE: the SAR type and it's defined in %nl80211_sar_type.

better use &enum nl80211_sar_type for a link in docs

> + *
> + * @NL80211_SAR_ATTR_SPECS: Nested array of SAR power
> + *	limit specifications. Each specification contains a set
> + *      of %nl80211_sar_specs_attrs.
> + *
> + *      For SET operation, it contains array of NL80211_SAR_ATTR_SPECS_POWER

some odd indent?

Usually we just use a single tab.

> +/**
> + * nl80211_sar_specs_attrs - Attributes for SAR power limit specs

again, enum missing

> + *
> + * @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).
> + *	0 means userspace doesn't have SAR limitation on this associated range.
> + *
> + * @NL80211_SAR_ATTR_SPECS_RANGE_INDEX: Required (u32) value to specify the
> + *	index of exported freq range table and the associated power limitation
> + *	is applied to this range.
> + *
> + *	Userspace isn't required to set all the ranges advertised by WLAN driver,
> + *	and userspace can skip some certain ranges. These skipped ranges don't
> + *	have SAR limitations, and these are same as setting the
> + *	%NL80211_SAR_ATTR_SPECS_POWER to 0. But it's required to set at least one range,
> + *	no matter the power limiation is 0 or not.

(typo - limitation)

Should "0" really be the magic value? Theoretically, 0 and even negative
values are valid. Perhaps we should just use something big (0xffffffff)
to indicate no limit, or just not have such a "no limitation" value
because userspace can always set it to something very big that means no
practical limitation anyway?

OK actually you have a U8 now so the high limit is 63.75dBm, but there's
not really a good reason for that, since U32 takes the same space in
netlink anyway.

And wait, I thought we agreed to remove the index? Now I'm confused.

And even if we do need the index, then perhaps we should use the
(otherwise anyway ignored) nla_type() of the container, instead of an
explicit inner attribute?

> + *
> + *	Every SET operation overwrites previous SET operation.
> + *
> + * @NL80211_SAR_ATTR_SPECS_START_FREQ: Required (u32) value to specify the start
> + *	frequency of this range edge when registering SAR capability to wiphy. It's
> + *	not a channel center frequency. The unit is KHz.

"kHz" not "KHz", in a few places other than this too

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

extra space?

> +	for (i = 0; i < num_freq_ranges; i++) {
> +		sub_freq_range = nla_nest_start(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);


Need to check the return values of these three calls.


And an aside, unrelated to this particular code: Should we do some kind
of validation that the ranges reported actually overlap all supported
channels (taking 20 MHz bandwidth into account)?

> +	nla_parse_nested(tb, NL80211_SAR_ATTR_MAX, info->attrs[NL80211_ATTR_SAR_SPEC],
> +			 sar_policy, info->extack);

If you're not checking the return value then no point in passing a
policy or extack :-)

And yes, it's already validated, so you don't have to do it again.

> +	sar_spec->type = type;
> +	specs = 0;
> +	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)) {

Similar here, don't really need to validate it since it's done by the
policy.

> +			err = -EINVAL;
> +			goto error;
> +		}
> +
> +		/* for power type, power value and index must be presented */
> +		if ((!spec[NL80211_SAR_ATTR_SPECS_POWER] ||
> +		     !spec[NL80211_SAR_ATTR_SPECS_RANGE_INDEX]) &&
> +		    type == NL80211_SAR_TYPE_POWER) {

maybe "switch (type) {...}" or something and return -EINVAL also if it's
a type not supported in the code yet, i.e. default case?

Otherwise we might add a type, and forget this pretty easily.

> +			err = -EINVAL;
> +			goto error;
> +		}
> +
> +		power = nla_get_u8(spec[NL80211_SAR_ATTR_SPECS_POWER]);
> +		sar_spec->sub_specs[specs].power = power;

and that probably should then be in a sub function or something also
inside the particular type.

or maybe just all in a separate function? dunno. not really _necessary_,
but the lines are getting kinda long already, and one more indentation
level with the switch won't help ...

johannes

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

[Johannes Berg]

Hi,

Looks pretty good. Some comments, mostly nits, below.


> +/**
> + * nl80211_sar_attrs - Attributes for SAR spec

missing enum

> + *
> + * @NL80211_SAR_ATTR_TYPE: the SAR type and it's defined in %nl80211_sar_type.

better use &enum nl80211_sar_type for a link in docs

> + *
> + * @NL80211_SAR_ATTR_SPECS: Nested array of SAR power
> + *	limit specifications. Each specification contains a set
> + *      of %nl80211_sar_specs_attrs.
> + *
> + *      For SET operation, it contains array of NL80211_SAR_ATTR_SPECS_POWER

some odd indent?

Usually we just use a single tab.

> +/**
> + * nl80211_sar_specs_attrs - Attributes for SAR power limit specs

again, enum missing

> + *
> + * @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).
> + *	0 means userspace doesn't have SAR limitation on this associated range.
> + *
> + * @NL80211_SAR_ATTR_SPECS_RANGE_INDEX: Required (u32) value to specify the
> + *	index of exported freq range table and the associated power limitation
> + *	is applied to this range.
> + *
> + *	Userspace isn't required to set all the ranges advertised by WLAN driver,
> + *	and userspace can skip some certain ranges. These skipped ranges don't
> + *	have SAR limitations, and these are same as setting the
> + *	%NL80211_SAR_ATTR_SPECS_POWER to 0. But it's required to set at least one range,
> + *	no matter the power limiation is 0 or not.

(typo - limitation)

Should "0" really be the magic value? Theoretically, 0 and even negative
values are valid. Perhaps we should just use something big (0xffffffff)
to indicate no limit, or just not have such a "no limitation" value
because userspace can always set it to something very big that means no
practical limitation anyway?

OK actually you have a U8 now so the high limit is 63.75dBm, but there's
not really a good reason for that, since U32 takes the same space in
netlink anyway.

And wait, I thought we agreed to remove the index? Now I'm confused.

And even if we do need the index, then perhaps we should use the
(otherwise anyway ignored) nla_type() of the container, instead of an
explicit inner attribute?

> + *
> + *	Every SET operation overwrites previous SET operation.
> + *
> + * @NL80211_SAR_ATTR_SPECS_START_FREQ: Required (u32) value to specify the start
> + *	frequency of this range edge when registering SAR capability to wiphy. It's
> + *	not a channel center frequency. The unit is KHz.

"kHz" not "KHz", in a few places other than this too

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

extra space?

> +	for (i = 0; i < num_freq_ranges; i++) {
> +		sub_freq_range = nla_nest_start(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);


Need to check the return values of these three calls.


And an aside, unrelated to this particular code: Should we do some kind
of validation that the ranges reported actually overlap all supported
channels (taking 20 MHz bandwidth into account)?

> +	nla_parse_nested(tb, NL80211_SAR_ATTR_MAX, info->attrs[NL80211_ATTR_SAR_SPEC],
> +			 sar_policy, info->extack);

If you're not checking the return value then no point in passing a
policy or extack :-)

And yes, it's already validated, so you don't have to do it again.

> +	sar_spec->type = type;
> +	specs = 0;
> +	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)) {

Similar here, don't really need to validate it since it's done by the
policy.

> +			err = -EINVAL;
> +			goto error;
> +		}
> +
> +		/* for power type, power value and index must be presented */
> +		if ((!spec[NL80211_SAR_ATTR_SPECS_POWER] ||
> +		     !spec[NL80211_SAR_ATTR_SPECS_RANGE_INDEX]) &&
> +		    type == NL80211_SAR_TYPE_POWER) {

maybe "switch (type) {...}" or something and return -EINVAL also if it's
a type not supported in the code yet, i.e. default case?

Otherwise we might add a type, and forget this pretty easily.

> +			err = -EINVAL;
> +			goto error;
> +		}
> +
> +		power = nla_get_u8(spec[NL80211_SAR_ATTR_SPECS_POWER]);
> +		sar_spec->sub_specs[specs].power = power;

and that probably should then be in a sub function or something also
inside the particular type.

or maybe just all in a separate function? dunno. not really _necessary_,
but the lines are getting kinda long already, and one more indentation
level with the switch won't help ...

johannes


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

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

[Carl Huang]

On 2020-11-06 18:25, Johannes Berg wrote:
> Hi,
> 
> Looks pretty good. Some comments, mostly nits, below.
> 
Thank you for the comments, Johannes.

I don't understand below well, please help explain:
> And even if we do need the index, then perhaps we should use the
> (otherwise anyway ignored) nla_type() of the container, instead of an
> explicit inner attribute?
> 



> 
>> +/**
>> + * nl80211_sar_attrs - Attributes for SAR spec
> 
> missing enum
> 
sure

>> + *
>> + * @NL80211_SAR_ATTR_TYPE: the SAR type and it's defined in 
>> %nl80211_sar_type.
> 
> better use &enum nl80211_sar_type for a link in docs
> 
>> + *
>> + * @NL80211_SAR_ATTR_SPECS: Nested array of SAR power
>> + *	limit specifications. Each specification contains a set
>> + *      of %nl80211_sar_specs_attrs.
>> + *
>> + *      For SET operation, it contains array of 
>> NL80211_SAR_ATTR_SPECS_POWER
> 
> some odd indent?
> 
> Usually we just use a single tab.
> 
sure

>> +/**
>> + * nl80211_sar_specs_attrs - Attributes for SAR power limit specs
> 
> again, enum missing
> 
>> + *
>> + * @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).
>> + *	0 means userspace doesn't have SAR limitation on this associated 
>> range.
>> + *
>> + * @NL80211_SAR_ATTR_SPECS_RANGE_INDEX: Required (u32) value to 
>> specify the
>> + *	index of exported freq range table and the associated power 
>> limitation
>> + *	is applied to this range.
>> + *
>> + *	Userspace isn't required to set all the ranges advertised by WLAN 
>> driver,
>> + *	and userspace can skip some certain ranges. These skipped ranges 
>> don't
>> + *	have SAR limitations, and these are same as setting the
>> + *	%NL80211_SAR_ATTR_SPECS_POWER to 0. But it's required to set at 
>> least one range,
>> + *	no matter the power limiation is 0 or not.
> 
> (typo - limitation)
> 
> Should "0" really be the magic value? Theoretically, 0 and even 
> negative
> values are valid. Perhaps we should just use something big (0xffffffff)
> to indicate no limit, or just not have such a "no limitation" value
> because userspace can always set it to something very big that means no
> practical limitation anyway?
> 
> OK actually you have a U8 now so the high limit is 63.75dBm, but 
> there's
> not really a good reason for that, since U32 takes the same space in
> netlink anyway.
> 
Looks 0 and negative value are not practical as it means <= 1mw,
but I can use S32 instead.

Not sure if a magic value is needed? If it's needed, then perhaps 
0x7fffffff
is good for it?

> And wait, I thought we agreed to remove the index? Now I'm confused.
> 
Using index in SET operation doesn't add burden to userspace and kernel,
but it provides some flexibility so userspace can skip some certain 
ranges.


> And even if we do need the index, then perhaps we should use the
> (otherwise anyway ignored) nla_type() of the container, instead of an
> explicit inner attribute?
> 
I don't understand what means here. Use nla_type for what?

>> + *
>> + *	Every SET operation overwrites previous SET operation.
>> + *
>> + * @NL80211_SAR_ATTR_SPECS_START_FREQ: Required (u32) value to 
>> specify the start
>> + *	frequency of this range edge when registering SAR capability to 
>> wiphy. It's
>> + *	not a channel center frequency. The unit is KHz.
> 
> "kHz" not "KHz", in a few places other than this too
> 
>> +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;
> 
> extra space?
> 
>> +	for (i = 0; i < num_freq_ranges; i++) {
>> +		sub_freq_range = nla_nest_start(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);
> 
> 
> Need to check the return values of these three calls.
> 
sure

> 
> And an aside, unrelated to this particular code: Should we do some kind
> of validation that the ranges reported actually overlap all supported
> channels (taking 20 MHz bandwidth into account)?
> 
>> +	nla_parse_nested(tb, NL80211_SAR_ATTR_MAX, 
>> info->attrs[NL80211_ATTR_SAR_SPEC],
>> +			 sar_policy, info->extack);
> 
> If you're not checking the return value then no point in passing a
> policy or extack :-)
> 
> And yes, it's already validated, so you don't have to do it again.
> 
Yes, will use NULL instead of info->extack

>> +	sar_spec->type = type;
>> +	specs = 0;
>> +	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)) {
> 
> Similar here, don't really need to validate it since it's done by the
> policy.
> 
sure

>> +			err = -EINVAL;
>> +			goto error;
>> +		}
>> +
>> +		/* for power type, power value and index must be presented */
>> +		if ((!spec[NL80211_SAR_ATTR_SPECS_POWER] ||
>> +		     !spec[NL80211_SAR_ATTR_SPECS_RANGE_INDEX]) &&
>> +		    type == NL80211_SAR_TYPE_POWER) {
> 
> maybe "switch (type) {...}" or something and return -EINVAL also if 
> it's
> a type not supported in the code yet, i.e. default case?
> 
> Otherwise we might add a type, and forget this pretty easily.
> 
Good suggestion, will change to switch case.

>> +			err = -EINVAL;
>> +			goto error;
>> +		}
>> +
>> +		power = nla_get_u8(spec[NL80211_SAR_ATTR_SPECS_POWER]);
>> +		sar_spec->sub_specs[specs].power = power;
> 
> and that probably should then be in a sub function or something also
> inside the particular type.
> 
> or maybe just all in a separate function? dunno. not really 
> _necessary_,
> but the lines are getting kinda long already, and one more indentation
> level with the switch won't help ...
> 
I'll move this to a separate function.


> johannes

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

[Carl Huang]

On 2020-11-06 18:25, Johannes Berg wrote:
> Hi,
> 
> Looks pretty good. Some comments, mostly nits, below.
> 
Thank you for the comments, Johannes.

I don't understand below well, please help explain:
> And even if we do need the index, then perhaps we should use the
> (otherwise anyway ignored) nla_type() of the container, instead of an
> explicit inner attribute?
> 



> 
>> +/**
>> + * nl80211_sar_attrs - Attributes for SAR spec
> 
> missing enum
> 
sure

>> + *
>> + * @NL80211_SAR_ATTR_TYPE: the SAR type and it's defined in 
>> %nl80211_sar_type.
> 
> better use &enum nl80211_sar_type for a link in docs
> 
>> + *
>> + * @NL80211_SAR_ATTR_SPECS: Nested array of SAR power
>> + *	limit specifications. Each specification contains a set
>> + *      of %nl80211_sar_specs_attrs.
>> + *
>> + *      For SET operation, it contains array of 
>> NL80211_SAR_ATTR_SPECS_POWER
> 
> some odd indent?
> 
> Usually we just use a single tab.
> 
sure

>> +/**
>> + * nl80211_sar_specs_attrs - Attributes for SAR power limit specs
> 
> again, enum missing
> 
>> + *
>> + * @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).
>> + *	0 means userspace doesn't have SAR limitation on this associated 
>> range.
>> + *
>> + * @NL80211_SAR_ATTR_SPECS_RANGE_INDEX: Required (u32) value to 
>> specify the
>> + *	index of exported freq range table and the associated power 
>> limitation
>> + *	is applied to this range.
>> + *
>> + *	Userspace isn't required to set all the ranges advertised by WLAN 
>> driver,
>> + *	and userspace can skip some certain ranges. These skipped ranges 
>> don't
>> + *	have SAR limitations, and these are same as setting the
>> + *	%NL80211_SAR_ATTR_SPECS_POWER to 0. But it's required to set at 
>> least one range,
>> + *	no matter the power limiation is 0 or not.
> 
> (typo - limitation)
> 
> Should "0" really be the magic value? Theoretically, 0 and even 
> negative
> values are valid. Perhaps we should just use something big (0xffffffff)
> to indicate no limit, or just not have such a "no limitation" value
> because userspace can always set it to something very big that means no
> practical limitation anyway?
> 
> OK actually you have a U8 now so the high limit is 63.75dBm, but 
> there's
> not really a good reason for that, since U32 takes the same space in
> netlink anyway.
> 
Looks 0 and negative value are not practical as it means <= 1mw,
but I can use S32 instead.

Not sure if a magic value is needed? If it's needed, then perhaps 
0x7fffffff
is good for it?

> And wait, I thought we agreed to remove the index? Now I'm confused.
> 
Using index in SET operation doesn't add burden to userspace and kernel,
but it provides some flexibility so userspace can skip some certain 
ranges.


> And even if we do need the index, then perhaps we should use the
> (otherwise anyway ignored) nla_type() of the container, instead of an
> explicit inner attribute?
> 
I don't understand what means here. Use nla_type for what?

>> + *
>> + *	Every SET operation overwrites previous SET operation.
>> + *
>> + * @NL80211_SAR_ATTR_SPECS_START_FREQ: Required (u32) value to 
>> specify the start
>> + *	frequency of this range edge when registering SAR capability to 
>> wiphy. It's
>> + *	not a channel center frequency. The unit is KHz.
> 
> "kHz" not "KHz", in a few places other than this too
> 
>> +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;
> 
> extra space?
> 
>> +	for (i = 0; i < num_freq_ranges; i++) {
>> +		sub_freq_range = nla_nest_start(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);
> 
> 
> Need to check the return values of these three calls.
> 
sure

> 
> And an aside, unrelated to this particular code: Should we do some kind
> of validation that the ranges reported actually overlap all supported
> channels (taking 20 MHz bandwidth into account)?
> 
>> +	nla_parse_nested(tb, NL80211_SAR_ATTR_MAX, 
>> info->attrs[NL80211_ATTR_SAR_SPEC],
>> +			 sar_policy, info->extack);
> 
> If you're not checking the return value then no point in passing a
> policy or extack :-)
> 
> And yes, it's already validated, so you don't have to do it again.
> 
Yes, will use NULL instead of info->extack

>> +	sar_spec->type = type;
>> +	specs = 0;
>> +	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)) {
> 
> Similar here, don't really need to validate it since it's done by the
> policy.
> 
sure

>> +			err = -EINVAL;
>> +			goto error;
>> +		}
>> +
>> +		/* for power type, power value and index must be presented */
>> +		if ((!spec[NL80211_SAR_ATTR_SPECS_POWER] ||
>> +		     !spec[NL80211_SAR_ATTR_SPECS_RANGE_INDEX]) &&
>> +		    type == NL80211_SAR_TYPE_POWER) {
> 
> maybe "switch (type) {...}" or something and return -EINVAL also if 
> it's
> a type not supported in the code yet, i.e. default case?
> 
> Otherwise we might add a type, and forget this pretty easily.
> 
Good suggestion, will change to switch case.

>> +			err = -EINVAL;
>> +			goto error;
>> +		}
>> +
>> +		power = nla_get_u8(spec[NL80211_SAR_ATTR_SPECS_POWER]);
>> +		sar_spec->sub_specs[specs].power = power;
> 
> and that probably should then be in a sub function or something also
> inside the particular type.
> 
> or maybe just all in a separate function? dunno. not really 
> _necessary_,
> but the lines are getting kinda long already, and one more indentation
> level with the switch won't help ...
> 
I'll move this to a separate function.


> johannes

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

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

[Abhishek Kumar]

Hi,

Johannes has some good comments, apart for that I have some nits.
> > And wait, I thought we agreed to remove the index? Now I'm confused.
> >
> Using index in SET operation doesn't add burden to userspace and kernel,
> but it provides some flexibility so userspace can skip some certain
> ranges.

I agree with Carl's comment, we do need the frequency index. If the
frequency index is provided, then the order is not important which
makes the data more clear or the set_sar_spec function needs to parse
the frequency ranges (and ofcourse userspace has to populate that as
well). If the frequency index is not provided, then the driver has to
assume that the userspace is not making any error in mapping of the
power and desired frequency.
Other reason is, might be a bit unlikely, but if in future there are
new subbands, then it gives a flexibility to the userspace to
explicitly provide the band for which it needs to set the power for.

> + *     used with %NL80211_CMD_SET_SAR_SPECS. The message contains fileds
> + *     of %nl80211_sar_attrs which specifies the sar type and related

typo: fileds .. you mean fields

-Abhishek

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

[Abhishek Kumar]

Hi,

Johannes has some good comments, apart for that I have some nits.
> > And wait, I thought we agreed to remove the index? Now I'm confused.
> >
> Using index in SET operation doesn't add burden to userspace and kernel,
> but it provides some flexibility so userspace can skip some certain
> ranges.

I agree with Carl's comment, we do need the frequency index. If the
frequency index is provided, then the order is not important which
makes the data more clear or the set_sar_spec function needs to parse
the frequency ranges (and ofcourse userspace has to populate that as
well). If the frequency index is not provided, then the driver has to
assume that the userspace is not making any error in mapping of the
power and desired frequency.
Other reason is, might be a bit unlikely, but if in future there are
new subbands, then it gives a flexibility to the userspace to
explicitly provide the band for which it needs to set the power for.

> + *     used with %NL80211_CMD_SET_SAR_SPECS. The message contains fileds
> + *     of %nl80211_sar_attrs which specifies the sar type and related

typo: fileds .. you mean fields

-Abhishek

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

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

[Carl Huang]

On 2020-11-20 04:25, Abhishek Kumar wrote:
> Hi,
> 
> Johannes has some good comments, apart for that I have some nits.
>> > And wait, I thought we agreed to remove the index? Now I'm confused.
>> >
>> Using index in SET operation doesn't add burden to userspace and 
>> kernel,
>> but it provides some flexibility so userspace can skip some certain
>> ranges.
> 
> I agree with Carl's comment, we do need the frequency index. If the
> frequency index is provided, then the order is not important which
> makes the data more clear or the set_sar_spec function needs to parse
> the frequency ranges (and ofcourse userspace has to populate that as
> well). If the frequency index is not provided, then the driver has to
> assume that the userspace is not making any error in mapping of the
> power and desired frequency.
> Other reason is, might be a bit unlikely, but if in future there are
> new subbands, then it gives a flexibility to the userspace to
> explicitly provide the band for which it needs to set the power for.
> 
>> + *     used with %NL80211_CMD_SET_SAR_SPECS. The message contains 
>> fileds
>> + *     of %nl80211_sar_attrs which specifies the sar type and related
> 
> typo: fileds .. you mean fields
> 
I will fix all the spelling errors and send V2.

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

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

[Carl Huang]

On 2020-11-20 04:25, Abhishek Kumar wrote:
> Hi,
> 
> Johannes has some good comments, apart for that I have some nits.
>> > And wait, I thought we agreed to remove the index? Now I'm confused.
>> >
>> Using index in SET operation doesn't add burden to userspace and 
>> kernel,
>> but it provides some flexibility so userspace can skip some certain
>> ranges.
> 
> I agree with Carl's comment, we do need the frequency index. If the
> frequency index is provided, then the order is not important which
> makes the data more clear or the set_sar_spec function needs to parse
> the frequency ranges (and ofcourse userspace has to populate that as
> well). If the frequency index is not provided, then the driver has to
> assume that the userspace is not making any error in mapping of the
> power and desired frequency.
> Other reason is, might be a bit unlikely, but if in future there are
> new subbands, then it gives a flexibility to the userspace to
> explicitly provide the band for which it needs to set the power for.
> 
>> + *     used with %NL80211_CMD_SET_SAR_SPECS. The message contains 
>> fileds
>> + *     of %nl80211_sar_attrs which specifies the sar type and related
> 
> typo: fileds .. you mean fields
> 
I will fix all the spelling errors and send V2.

> _______________________________________________
> 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

[PATCH 2/3] mac80211: add ieee80211_set_sar_specs

[Carl Huang]

This change registers ieee80211_set_sar_specs to
mac80211_config_ops, so cfg80211 can call it.

Signed-off-by: Carl Huang <cjhuang@codeaurora.org>
---
 include/net/mac80211.h |  2 ++
 net/mac80211/cfg.c     | 12 ++++++++++++
 2 files changed, 14 insertions(+)

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/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,
 };
-- 
2.7.4

[PATCH 2/3] mac80211: add ieee80211_set_sar_specs

[Carl Huang]

This change registers ieee80211_set_sar_specs to
mac80211_config_ops, so cfg80211 can call it.

Signed-off-by: Carl Huang <cjhuang@codeaurora.org>
---
 include/net/mac80211.h |  2 ++
 net/mac80211/cfg.c     | 12 ++++++++++++
 2 files changed, 14 insertions(+)

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/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,
 };
-- 
2.7.4


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

[PATCH 3/3] 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.

This SAR power limitation is compared to regulatory txpower
and selects the minimal one to set when station is connected.
Otherwise, it delays until the station is connected. If the
station is disconnected, it returns to regulatory txpower.

This feature is controlled by hw parameter: dynamic_sar_support.

Tested-on: QCA6174 hw3.2 PCI WLAN.RM.4.4.1-00110-QCARMSWP-1

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  | 221 ++++++++++++++++++++++++---------
 4 files changed, 183 insertions(+), 59 deletions(-)

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..c86a90c 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 },
 };
 
+const static struct cfg80211_sar_freq_ranges ath10k_sar_freq_ranges[] = {
+	{.start_freq = 2402, .end_freq = 2494 },
+	{.start_freq = 5170, .end_freq = 5875 },
+};
+
+const static 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,149 @@ 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;
+}
+
+static int ath10k_mac_txpower_setup(struct ath10k *ar, int txpower)
+{
+	int ret;
+	u32 param;
+	int tx_power_2g, tx_power_5g;
+	bool connected;
+
+	lockdep_assert_held(&ar->conf_mutex);
+
+	/* ath10k internally uses unit of 0.5 dBm so multiply by 2 */
+	tx_power_2g = txpower * 2;
+	tx_power_5g = txpower * 2;
+
+	connected = ath10k_mac_is_connected(ar);
+
+	if (connected && ar->tx_power_2g_limit)
+		if (tx_power_2g > ar->tx_power_2g_limit)
+			tx_power_2g = ar->tx_power_2g_limit;
+
+	if (connected && ar->tx_power_5g_limit)
+		if (tx_power_5g > ar->tx_power_5g_limit)
+			tx_power_5g = ar->tx_power_5g_limit;
+
+	ath10k_dbg(ar, ATH10K_DBG_MAC, "mac txpower 2g:%d 5g:%d\n",
+		   tx_power_2g, tx_power_5g);
+
+	param = ar->wmi.pdev_param->txpower_limit2g;
+	ret = ath10k_wmi_pdev_set_param(ar, param, tx_power_2g);
+	if (ret) {
+		ath10k_warn(ar, "failed to set 2g txpower %d: %d\n",
+			    tx_power_2g, ret);
+		return ret;
+	}
+
+	param = ar->wmi.pdev_param->txpower_limit5g;
+	ret = ath10k_wmi_pdev_set_param(ar, param, tx_power_5g);
+	if (ret) {
+		ath10k_warn(ar, "failed to set 5g txpower %d: %d\n",
+			    tx_power_5g, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int ath10k_mac_txpower_recalc(struct ath10k *ar)
+{
+	struct ath10k_vif *arvif;
+	int ret, txpower = -1;
+
+	lockdep_assert_held(&ar->conf_mutex);
+
+	list_for_each_entry(arvif, &ar->arvifs, list) {
+		/* txpower not initialized yet? */
+		if (arvif->txpower == INT_MIN)
+			continue;
+
+		if (txpower == -1)
+			txpower = arvif->txpower;
+		else
+			txpower = min(txpower, arvif->txpower);
+	}
+
+	if (txpower == -1)
+		return 0;
+
+	ret = ath10k_mac_txpower_setup(ar, txpower);
+	if (ret) {
+		ath10k_warn(ar, "failed to setup tx power %d: %d\n",
+			    txpower, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int ath10k_mac_set_sar_power(struct ath10k *ar)
+{
+	if (!ar->hw_params.dynamic_sar_support)
+		return -EOPNOTSUPP;
+
+	if (!ath10k_mac_is_connected(ar))
+		return 0;
+
+	/* if connected, then arvif->txpower must be valid */
+	return ath10k_mac_txpower_recalc(ar);
+}
+
+static int ath10k_mac_set_sar_specs(struct ieee80211_hw *hw,
+				    const struct cfg80211_sar_specs *sar)
+{
+	const struct cfg80211_sar_sub_specs *sub_specs;
+	struct ath10k *ar = hw->priv;
+	int ret, i;
+
+	mutex_lock(&ar->conf_mutex);
+
+	if (!ar->hw_params.dynamic_sar_support) {
+		ret = -EOPNOTSUPP;
+		goto error;
+	}
+
+	if (!sar || sar->type != NL80211_SAR_TYPE_POWER ||
+	    sar->num_sub_specs == 0 || !sar->sub_specs) {
+		ret = -EINVAL;
+		goto error;
+	}
+
+	sub_specs = sar->sub_specs;
+
+	ar->tx_power_2g_limit = 0;
+	ar->tx_power_5g_limit = 0;
+
+	/* note the power is in 0.25dbm unit, while ath10k uses
+	 * 0.5dbm unit.
+	 */
+	for (i = 0; i < sar->num_sub_specs; i++) {
+		if (i == 0)
+			ar->tx_power_2g_limit = sub_specs->power / 2;
+		else if (i == 1)
+			ar->tx_power_5g_limit = sub_specs->power / 2;
+
+		sub_specs++;
+	}
+
+	ret = ath10k_mac_set_sar_power(ar);
+error:
+	mutex_unlock(&ar->conf_mutex);
+	return ret;
+}
+
 /* 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 +3122,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.
@@ -3010,6 +3166,8 @@ static void ath10k_bss_disassoc(struct ieee80211_hw *hw,
 
 	arvif->is_up = false;
 
+	ath10k_mac_txpower_recalc(ar);
+
 	cancel_delayed_work_sync(&arvif->connection_loss_work);
 }
 
@@ -5207,65 +5365,6 @@ static int ath10k_config_ps(struct ath10k *ar)
 	return ret;
 }
 
-static int ath10k_mac_txpower_setup(struct ath10k *ar, int txpower)
-{
-	int ret;
-	u32 param;
-
-	lockdep_assert_held(&ar->conf_mutex);
-
-	ath10k_dbg(ar, ATH10K_DBG_MAC, "mac txpower %d\n", txpower);
-
-	param = ar->wmi.pdev_param->txpower_limit2g;
-	ret = ath10k_wmi_pdev_set_param(ar, param, txpower * 2);
-	if (ret) {
-		ath10k_warn(ar, "failed to set 2g txpower %d: %d\n",
-			    txpower, ret);
-		return ret;
-	}
-
-	param = ar->wmi.pdev_param->txpower_limit5g;
-	ret = ath10k_wmi_pdev_set_param(ar, param, txpower * 2);
-	if (ret) {
-		ath10k_warn(ar, "failed to set 5g txpower %d: %d\n",
-			    txpower, ret);
-		return ret;
-	}
-
-	return 0;
-}
-
-static int ath10k_mac_txpower_recalc(struct ath10k *ar)
-{
-	struct ath10k_vif *arvif;
-	int ret, txpower = -1;
-
-	lockdep_assert_held(&ar->conf_mutex);
-
-	list_for_each_entry(arvif, &ar->arvifs, list) {
-		/* txpower not initialized yet? */
-		if (arvif->txpower == INT_MIN)
-			continue;
-
-		if (txpower == -1)
-			txpower = arvif->txpower;
-		else
-			txpower = min(txpower, arvif->txpower);
-	}
-
-	if (txpower == -1)
-		return 0;
-
-	ret = ath10k_mac_txpower_setup(ar, txpower);
-	if (ret) {
-		ath10k_warn(ar, "failed to setup tx power %d: %d\n",
-			    txpower, ret);
-		return ret;
-	}
-
-	return 0;
-}
-
 static int ath10k_config(struct ieee80211_hw *hw, u32 changed)
 {
 	struct ath10k *ar = hw->priv;
@@ -9270,6 +9369,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 +10109,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

[PATCH 3/3] 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.

This SAR power limitation is compared to regulatory txpower
and selects the minimal one to set when station is connected.
Otherwise, it delays until the station is connected. If the
station is disconnected, it returns to regulatory txpower.

This feature is controlled by hw parameter: dynamic_sar_support.

Tested-on: QCA6174 hw3.2 PCI WLAN.RM.4.4.1-00110-QCARMSWP-1

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  | 221 ++++++++++++++++++++++++---------
 4 files changed, 183 insertions(+), 59 deletions(-)

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..c86a90c 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 },
 };
 
+const static struct cfg80211_sar_freq_ranges ath10k_sar_freq_ranges[] = {
+	{.start_freq = 2402, .end_freq = 2494 },
+	{.start_freq = 5170, .end_freq = 5875 },
+};
+
+const static 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,149 @@ 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;
+}
+
+static int ath10k_mac_txpower_setup(struct ath10k *ar, int txpower)
+{
+	int ret;
+	u32 param;
+	int tx_power_2g, tx_power_5g;
+	bool connected;
+
+	lockdep_assert_held(&ar->conf_mutex);
+
+	/* ath10k internally uses unit of 0.5 dBm so multiply by 2 */
+	tx_power_2g = txpower * 2;
+	tx_power_5g = txpower * 2;
+
+	connected = ath10k_mac_is_connected(ar);
+
+	if (connected && ar->tx_power_2g_limit)
+		if (tx_power_2g > ar->tx_power_2g_limit)
+			tx_power_2g = ar->tx_power_2g_limit;
+
+	if (connected && ar->tx_power_5g_limit)
+		if (tx_power_5g > ar->tx_power_5g_limit)
+			tx_power_5g = ar->tx_power_5g_limit;
+
+	ath10k_dbg(ar, ATH10K_DBG_MAC, "mac txpower 2g:%d 5g:%d\n",
+		   tx_power_2g, tx_power_5g);
+
+	param = ar->wmi.pdev_param->txpower_limit2g;
+	ret = ath10k_wmi_pdev_set_param(ar, param, tx_power_2g);
+	if (ret) {
+		ath10k_warn(ar, "failed to set 2g txpower %d: %d\n",
+			    tx_power_2g, ret);
+		return ret;
+	}
+
+	param = ar->wmi.pdev_param->txpower_limit5g;
+	ret = ath10k_wmi_pdev_set_param(ar, param, tx_power_5g);
+	if (ret) {
+		ath10k_warn(ar, "failed to set 5g txpower %d: %d\n",
+			    tx_power_5g, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int ath10k_mac_txpower_recalc(struct ath10k *ar)
+{
+	struct ath10k_vif *arvif;
+	int ret, txpower = -1;
+
+	lockdep_assert_held(&ar->conf_mutex);
+
+	list_for_each_entry(arvif, &ar->arvifs, list) {
+		/* txpower not initialized yet? */
+		if (arvif->txpower == INT_MIN)
+			continue;
+
+		if (txpower == -1)
+			txpower = arvif->txpower;
+		else
+			txpower = min(txpower, arvif->txpower);
+	}
+
+	if (txpower == -1)
+		return 0;
+
+	ret = ath10k_mac_txpower_setup(ar, txpower);
+	if (ret) {
+		ath10k_warn(ar, "failed to setup tx power %d: %d\n",
+			    txpower, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int ath10k_mac_set_sar_power(struct ath10k *ar)
+{
+	if (!ar->hw_params.dynamic_sar_support)
+		return -EOPNOTSUPP;
+
+	if (!ath10k_mac_is_connected(ar))
+		return 0;
+
+	/* if connected, then arvif->txpower must be valid */
+	return ath10k_mac_txpower_recalc(ar);
+}
+
+static int ath10k_mac_set_sar_specs(struct ieee80211_hw *hw,
+				    const struct cfg80211_sar_specs *sar)
+{
+	const struct cfg80211_sar_sub_specs *sub_specs;
+	struct ath10k *ar = hw->priv;
+	int ret, i;
+
+	mutex_lock(&ar->conf_mutex);
+
+	if (!ar->hw_params.dynamic_sar_support) {
+		ret = -EOPNOTSUPP;
+		goto error;
+	}
+
+	if (!sar || sar->type != NL80211_SAR_TYPE_POWER ||
+	    sar->num_sub_specs == 0 || !sar->sub_specs) {
+		ret = -EINVAL;
+		goto error;
+	}
+
+	sub_specs = sar->sub_specs;
+
+	ar->tx_power_2g_limit = 0;
+	ar->tx_power_5g_limit = 0;
+
+	/* note the power is in 0.25dbm unit, while ath10k uses
+	 * 0.5dbm unit.
+	 */
+	for (i = 0; i < sar->num_sub_specs; i++) {
+		if (i == 0)
+			ar->tx_power_2g_limit = sub_specs->power / 2;
+		else if (i == 1)
+			ar->tx_power_5g_limit = sub_specs->power / 2;
+
+		sub_specs++;
+	}
+
+	ret = ath10k_mac_set_sar_power(ar);
+error:
+	mutex_unlock(&ar->conf_mutex);
+	return ret;
+}
+
 /* 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 +3122,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.
@@ -3010,6 +3166,8 @@ static void ath10k_bss_disassoc(struct ieee80211_hw *hw,
 
 	arvif->is_up = false;
 
+	ath10k_mac_txpower_recalc(ar);
+
 	cancel_delayed_work_sync(&arvif->connection_loss_work);
 }
 
@@ -5207,65 +5365,6 @@ static int ath10k_config_ps(struct ath10k *ar)
 	return ret;
 }
 
-static int ath10k_mac_txpower_setup(struct ath10k *ar, int txpower)
-{
-	int ret;
-	u32 param;
-
-	lockdep_assert_held(&ar->conf_mutex);
-
-	ath10k_dbg(ar, ATH10K_DBG_MAC, "mac txpower %d\n", txpower);
-
-	param = ar->wmi.pdev_param->txpower_limit2g;
-	ret = ath10k_wmi_pdev_set_param(ar, param, txpower * 2);
-	if (ret) {
-		ath10k_warn(ar, "failed to set 2g txpower %d: %d\n",
-			    txpower, ret);
-		return ret;
-	}
-
-	param = ar->wmi.pdev_param->txpower_limit5g;
-	ret = ath10k_wmi_pdev_set_param(ar, param, txpower * 2);
-	if (ret) {
-		ath10k_warn(ar, "failed to set 5g txpower %d: %d\n",
-			    txpower, ret);
-		return ret;
-	}
-
-	return 0;
-}
-
-static int ath10k_mac_txpower_recalc(struct ath10k *ar)
-{
-	struct ath10k_vif *arvif;
-	int ret, txpower = -1;
-
-	lockdep_assert_held(&ar->conf_mutex);
-
-	list_for_each_entry(arvif, &ar->arvifs, list) {
-		/* txpower not initialized yet? */
-		if (arvif->txpower == INT_MIN)
-			continue;
-
-		if (txpower == -1)
-			txpower = arvif->txpower;
-		else
-			txpower = min(txpower, arvif->txpower);
-	}
-
-	if (txpower == -1)
-		return 0;
-
-	ret = ath10k_mac_txpower_setup(ar, txpower);
-	if (ret) {
-		ath10k_warn(ar, "failed to setup tx power %d: %d\n",
-			    txpower, ret);
-		return ret;
-	}
-
-	return 0;
-}
-
 static int ath10k_config(struct ieee80211_hw *hw, u32 changed)
 {
 	struct ath10k *ar = hw->priv;
@@ -9270,6 +9369,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 +10109,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: [PATCH 3/3] ath10k: allow dynamic SAR power limits via common API

[Abhishek Kumar]

Hi,
This patch looks good to me, there is one small nit, If the maintainer
can take care of it then probably we don't need a new rev.

> @@ -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,
>         },
Orthogonal to this patch, other people might probably differ, I guess
putting dynamic sar support as a firmware feature capability should be
more scalable and then we don't need a structure table for each
firmware. This might hold for other firmware features as well.

> +       ath10k_dbg(ar, ATH10K_DBG_MAC, "mac txpower 2g:%d 5g:%d\n",
> +                  tx_power_2g, tx_power_5g);
just a nit: space after colon. This might throw a warning in checkpatch.pl

-Abhishek

Re: [PATCH 3/3] ath10k: allow dynamic SAR power limits via common API

[Abhishek Kumar]

Hi,
This patch looks good to me, there is one small nit, If the maintainer
can take care of it then probably we don't need a new rev.

> @@ -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,
>         },
Orthogonal to this patch, other people might probably differ, I guess
putting dynamic sar support as a firmware feature capability should be
more scalable and then we don't need a structure table for each
firmware. This might hold for other firmware features as well.

> +       ath10k_dbg(ar, ATH10K_DBG_MAC, "mac txpower 2g:%d 5g:%d\n",
> +                  tx_power_2g, tx_power_5g);
just a nit: space after colon. This might throw a warning in checkpatch.pl

-Abhishek

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