diff mbox series

[4/7] cfg80211: add NL command to set 6 GHz power mode

Message ID 20220704102341.5692-5-quic_adisi@quicinc.com (mailing list archive)
State Changes Requested
Delegated to: Johannes Berg
Headers show
Series cfg80211/mac80211: extend 6 GHz support for all power modes | expand

Commit Message

Aditya Kumar Singh July 4, 2022, 10:23 a.m. UTC 
6 GHz introduces various power modes for access points and for clients.
When user configures these power modes, currently cfg80211 does not
have support to store the configured power mode.

Add support to store the 6 GHz configured power mode in the structure
wireless_dev via a new NL command - NL80211_CMD_SET_6GHZ_POWER_MODE.

The  above command uses two new NL attributes to set power mode for AP
and clients - NL80211_ATTR_6GHZ_REG_AP_POWER_MODE and
NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE respectively.

Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com>
---
 include/net/cfg80211.h       |  9 ++++++
 include/uapi/linux/nl80211.h | 15 +++++++++
 net/wireless/ap.c            |  4 +++
 net/wireless/nl80211.c       | 62 ++++++++++++++++++++++++++++++++++++
 net/wireless/sme.c           |  3 ++
 5 files changed, 93 insertions(+)

Comments

Johannes Berg Sept. 6, 2022, 11:05 a.m. UTC | #1 
On Mon, 2022-07-04 at 15:53 +0530, Aditya Kumar Singh wrote:
> 
> + * @NL80211_ATTR_6GHZ_REG_AP_POWER_MODE: Configure 6 GHz regulatory power mode
> + *	for access points. Referenced from &enum ieee80211_ap_reg_power.
> + *
> + * @NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE: Configure 6 GHz regulatory power
> + *	mode for clients. Referenced from &enum ieee80211_client_reg_power.

I don't really see a good reason to have two attributes for this, rather
than validating their value based on the iftype?

>  	err = __cfg80211_stop_ap(rdev, dev, link_id, notify);
> +
> +	if (wdev->reg_6ghz_pwr_configured)
> +		wdev->reg_6ghz_pwr_configured = false;

no need to check first

> +static int nl80211_set_6ghz_power_mode(struct sk_buff *skb,
> +				       struct genl_info *info)
> +{
> +	struct net_device *dev = info->user_ptr[1];
> +	struct wireless_dev *wdev = NULL;
> +	enum nl80211_iftype iftype = NL80211_IFTYPE_UNSPECIFIED;
> +	int ret = -EINVAL;
> +
> +	if (dev)
> +		wdev = dev->ieee80211_ptr;
> +
> +	if (wdev)
> +		iftype = wdev->iftype;

that's all pretty useless

> +	if (iftype != NL80211_IFTYPE_AP &&
> +	    iftype != NL80211_IFTYPE_STATION)
> +		return -EOPNOTSUPP;

you're going to return here anyway ...

Better to just simplify that and return if there's no wdev, and actually
you can enforce that anyway through the internal flags?? Not sure why
you do all this.

Btw, all of that probably also needs to be per *link* rather than per
*interface* now, with MLO?

> +	if (!info->attrs[NL80211_ATTR_6GHZ_REG_AP_POWER_MODE] &&
> +	    !info->attrs[NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE])
> +		return -EINVAL;
> +
> +	wdev_lock(wdev);
> +	if (wdev->reg_6ghz_pwr_configured) {
> +		wdev_unlock(wdev);
> +		return -EALREADY;
> +	}
> +
> +	if (iftype == NL80211_IFTYPE_AP &&
> +	    info->attrs[NL80211_ATTR_6GHZ_REG_AP_POWER_MODE]) {
> +		wdev->ap_6ghz_power =
> +		  nla_get_u8(info->attrs[NL80211_ATTR_6GHZ_REG_AP_POWER_MODE]);
> +		ret = 0;
> +	}
> +
> +	if (iftype == NL80211_IFTYPE_STATION &&
> +	    info->attrs[NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE]) {
> +		wdev->client_6ghz_power =
> +		  nla_get_u8(info->attrs[NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE]);
> +		ret = 0;
> +	}
> +
> +	if (!ret)
> +		wdev->reg_6ghz_pwr_configured = true;

and honestly that logic here with the two attributes is pretty
strange...

I'd even say you should remove the union in the wdev struct since you
only can have one of them at a time anyway

> +	{
> +		.cmd = NL80211_CMD_SET_6GHZ_POWER_MODE,
> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +		.doit = nl80211_set_6ghz_power_mode,
> +		.flags = GENL_UNS_ADMIN_PERM,
> +		.internal_flags = IFLAGS(NL80211_FLAG_NEED_NETDEV),

if you have netdev then you have wdev too, later

>  
> +	if (wdev->reg_6ghz_pwr_configured)
> +		wdev->reg_6ghz_pwr_configured = false;
> 

No need for if

johannes
Aditya Kumar Singh Oct. 19, 2022, 4:24 a.m. UTC | #2 
On 9/6/2022 16:35, Johannes Berg wrote:
> On Mon, 2022-07-04 at 15:53 +0530, Aditya Kumar Singh wrote:
>>
>> + * @NL80211_ATTR_6GHZ_REG_AP_POWER_MODE: Configure 6 GHz regulatory power mode
>> + *	for access points. Referenced from &enum ieee80211_ap_reg_power.
>> + *
>> + * @NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE: Configure 6 GHz regulatory power
>> + *	mode for clients. Referenced from &enum ieee80211_client_reg_power.
> 
> I don't really see a good reason to have two attributes for this, rather
> than validating their value based on the iftype?
> 
The policy for each varies. For AP power mode, it can vary from 0 to 2 
(total 3 power modes currently), and for clients 0 to 1 (total 2 power 
modes). So, if we have just 1 NL_ATTR, while parsing obviously we can do 
based on iftype but in NL_ATTR policy validation, for clients it will 
pass value 2 where actually it should not. Will that be fine?


>>   	err = __cfg80211_stop_ap(rdev, dev, link_id, notify);
>> +
>> +	if (wdev->reg_6ghz_pwr_configured)
>> +		wdev->reg_6ghz_pwr_configured = false;
> 
> no need to check first
Sure, will improvise in next version.

> 
>>
...
> 
> Btw, all of that probably also needs to be per *link* rather than per
> *interface* now, with MLO?
Yes, it should be per *link*. Im working on the changes, will send next 
version soon.

> 
>> +	if (!info->attrs[NL80211_ATTR_6GHZ_REG_AP_POWER_MODE] &&
>> +	    !info->attrs[NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE])
>> +		return -EINVAL;
>> +
>> +	wdev_lock(wdev);
>> +	if (wdev->reg_6ghz_pwr_configured) {
>> +		wdev_unlock(wdev);
>> +		return -EALREADY;
>> +	}
>> +
>> +	if (iftype == NL80211_IFTYPE_AP &&
>> +	    info->attrs[NL80211_ATTR_6GHZ_REG_AP_POWER_MODE]) {
>> +		wdev->ap_6ghz_power =
>> +		  nla_get_u8(info->attrs[NL80211_ATTR_6GHZ_REG_AP_POWER_MODE]);
>> +		ret = 0;
>> +	}
>> +
>> +	if (iftype == NL80211_IFTYPE_STATION &&
>> +	    info->attrs[NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE]) {
>> +		wdev->client_6ghz_power =
>> +		  nla_get_u8(info->attrs[NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE]);
>> +		ret = 0;
>> +	}
>> +
>> +	if (!ret)
>> +		wdev->reg_6ghz_pwr_configured = true;
> 
> and honestly that logic here with the two attributes is pretty
> strange...
> 
> I'd even say you should remove the union in the wdev struct since you
> only can have one of them at a time anyway
Sure will do.

> 
>> +	{
>> +		.cmd = NL80211_CMD_SET_6GHZ_POWER_MODE,
>> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
>> +		.doit = nl80211_set_6ghz_power_mode,
>> +		.flags = GENL_UNS_ADMIN_PERM,
>> +		.internal_flags = IFLAGS(NL80211_FLAG_NEED_NETDEV),
> 
> if you have netdev then you have wdev too, later
Yes, got it.

> 
>>   
>> +	if (wdev->reg_6ghz_pwr_configured)
>> +		wdev->reg_6ghz_pwr_configured = false;
>>
> 
> No need for if
Got it, thanks. Will rectify.

> 
> johannes

Aditya
Johannes Berg Oct. 19, 2022, 7:42 a.m. UTC | #3 
On Wed, 2022-10-19 at 09:54 +0530, Aditya Kumar Singh wrote:
> On 9/6/2022 16:35, Johannes Berg wrote:
> > On Mon, 2022-07-04 at 15:53 +0530, Aditya Kumar Singh wrote:
> > > 
> > > + * @NL80211_ATTR_6GHZ_REG_AP_POWER_MODE: Configure 6 GHz regulatory power mode
> > > + *	for access points. Referenced from &enum ieee80211_ap_reg_power.
> > > + *
> > > + * @NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE: Configure 6 GHz regulatory power
> > > + *	mode for clients. Referenced from &enum ieee80211_client_reg_power.
> > 
> > I don't really see a good reason to have two attributes for this, rather
> > than validating their value based on the iftype?
> > 
> The policy for each varies. For AP power mode, it can vary from 0 to 2 
> (total 3 power modes currently), and for clients 0 to 1 (total 2 power 
> modes). So, if we have just 1 NL_ATTR, while parsing obviously we can do 
> based on iftype but in NL_ATTR policy validation, for clients it will 
> pass value 2 where actually it should not. Will that be fine?

Yeah, I dunno. That just means we'd have to validate it in the code
rather than the policy. Not really sure which is better.

johannes
diff mbox series

Patch

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index ed482b23fb9c..1e8852c6149f 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -5644,6 +5644,9 @@  static inline void wiphy_unlock(struct wiphy *wiphy)
  * @links: array of %IEEE80211_MLD_MAX_NUM_LINKS elements containing @addr
  *	@ap and @client for each link
  * @valid_links: bitmap describing what elements of @links are valid
+ * @ap_6ghz_power: 6 GHz regulatory power mode for Access Points
+ * @client_6ghz_power: 6 GHz regulatory power mode for Clients
+ * @reg_6ghz_pwr_configured: true if 6 GHz power mode is configured
  */
 struct wireless_dev {
 	struct wiphy *wiphy;
@@ -5758,6 +5761,12 @@  struct wireless_dev {
 		};
 	} links[IEEE80211_MLD_MAX_NUM_LINKS];
 	u16 valid_links;
+
+	union {
+		enum ieee80211_ap_reg_power ap_6ghz_power;
+		enum ieee80211_client_reg_power client_6ghz_power;
+	};
+	bool reg_6ghz_pwr_configured;
 };
 
 static inline const u8 *wdev_address(struct wireless_dev *wdev)
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 789f73878f50..e62838887802 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1254,6 +1254,11 @@ 
  *	without %NL80211_ATTR_MLO_LINK_ID as an easy way to remove all links
  *	in preparation for e.g. roaming to a regular (non-MLO) AP.
  *
+ * @NL80211_CMD_SET_6GHZ_POWER_MODE: Set 6 GHz power mode for the interface
+ *	using -
+ *	&NL80211_ATTR_6GHZ_REG_AP_POWER_MODE - for access points
+ *	&NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE - for clients
+ *
  * @NL80211_CMD_MAX: highest used command number
  * @__NL80211_CMD_AFTER_LAST: internal use
  */
@@ -1501,6 +1506,8 @@  enum nl80211_commands {
 	NL80211_CMD_ADD_LINK,
 	NL80211_CMD_REMOVE_LINK,
 
+	NL80211_CMD_SET_6GHZ_POWER_MODE,
+
 	/* add new commands above here */
 
 	/* used to define NL80211_CMD_MAX below */
@@ -2701,6 +2708,12 @@  enum nl80211_commands {
  *	suites allowed as %NL80211_MAX_NR_AKM_SUITES which is the legacy maximum
  *	number prior to the introduction of this attribute.
  *
+ * @NL80211_ATTR_6GHZ_REG_AP_POWER_MODE: Configure 6 GHz regulatory power mode
+ *	for access points. Referenced from &enum ieee80211_ap_reg_power.
+ *
+ * @NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE: Configure 6 GHz regulatory power
+ *	mode for clients. Referenced from &enum ieee80211_client_reg_power.
+ *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -3223,6 +3236,8 @@  enum nl80211_attrs {
 
 	NL80211_ATTR_MAX_NUM_AKM_SUITES,
 
+	NL80211_ATTR_6GHZ_REG_AP_POWER_MODE,
+	NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE,
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
diff --git a/net/wireless/ap.c b/net/wireless/ap.c
index e68923200018..be4e6177d72a 100644
--- a/net/wireless/ap.c
+++ b/net/wireless/ap.c
@@ -82,6 +82,10 @@  int cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
 
 	wdev_lock(wdev);
 	err = __cfg80211_stop_ap(rdev, dev, link_id, notify);
+
+	if (wdev->reg_6ghz_pwr_configured)
+		wdev->reg_6ghz_pwr_configured = false;
+
 	wdev_unlock(wdev);
 
 	return err;
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index afa8cd686e0e..4d5c45f303ec 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -799,6 +799,12 @@  static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
 	[NL80211_ATTR_MLD_ADDR] = NLA_POLICY_EXACT_LEN(ETH_ALEN),
 	[NL80211_ATTR_MLO_SUPPORT] = { .type = NLA_FLAG },
 	[NL80211_ATTR_MAX_NUM_AKM_SUITES] = { .type = NLA_REJECT },
+	[NL80211_ATTR_6GHZ_REG_AP_POWER_MODE] =
+		NLA_POLICY_RANGE(NLA_U8, IEEE80211_REG_LPI_AP,
+				 IEEE80211_REG_AP_POWER_MAX),
+	[NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE] =
+		NLA_POLICY_RANGE(NLA_U8, IEEE80211_REG_DEFAULT_CLIENT,
+				 IEEE80211_REG_CLIENT_POWER_MAX),
 };
 
 /* policy for the key attributes */
@@ -15699,6 +15705,55 @@  static int nl80211_remove_link(struct sk_buff *skb, struct genl_info *info)
 	return 0;
 }
 
+static int nl80211_set_6ghz_power_mode(struct sk_buff *skb,
+				       struct genl_info *info)
+{
+	struct net_device *dev = info->user_ptr[1];
+	struct wireless_dev *wdev = NULL;
+	enum nl80211_iftype iftype = NL80211_IFTYPE_UNSPECIFIED;
+	int ret = -EINVAL;
+
+	if (dev)
+		wdev = dev->ieee80211_ptr;
+
+	if (wdev)
+		iftype = wdev->iftype;
+
+	if (iftype != NL80211_IFTYPE_AP &&
+	    iftype != NL80211_IFTYPE_STATION)
+		return -EOPNOTSUPP;
+
+	if (!info->attrs[NL80211_ATTR_6GHZ_REG_AP_POWER_MODE] &&
+	    !info->attrs[NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE])
+		return -EINVAL;
+
+	wdev_lock(wdev);
+	if (wdev->reg_6ghz_pwr_configured) {
+		wdev_unlock(wdev);
+		return -EALREADY;
+	}
+
+	if (iftype == NL80211_IFTYPE_AP &&
+	    info->attrs[NL80211_ATTR_6GHZ_REG_AP_POWER_MODE]) {
+		wdev->ap_6ghz_power =
+		  nla_get_u8(info->attrs[NL80211_ATTR_6GHZ_REG_AP_POWER_MODE]);
+		ret = 0;
+	}
+
+	if (iftype == NL80211_IFTYPE_STATION &&
+	    info->attrs[NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE]) {
+		wdev->client_6ghz_power =
+		  nla_get_u8(info->attrs[NL80211_ATTR_6GHZ_REG_CLIENT_POWER_MODE]);
+		ret = 0;
+	}
+
+	if (!ret)
+		wdev->reg_6ghz_pwr_configured = true;
+
+	wdev_unlock(wdev);
+	return ret;
+}
+
 #define NL80211_FLAG_NEED_WIPHY		0x01
 #define NL80211_FLAG_NEED_NETDEV	0x02
 #define NL80211_FLAG_NEED_RTNL		0x04
@@ -16849,6 +16904,13 @@  static const struct genl_small_ops nl80211_small_ops[] = {
 		.internal_flags = IFLAGS(NL80211_FLAG_NEED_NETDEV_UP |
 					 NL80211_FLAG_MLO_VALID_LINK_ID),
 	},
+	{
+		.cmd = NL80211_CMD_SET_6GHZ_POWER_MODE,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.doit = nl80211_set_6ghz_power_mode,
+		.flags = GENL_UNS_ADMIN_PERM,
+		.internal_flags = IFLAGS(NL80211_FLAG_NEED_NETDEV),
+	}
 };
 
 static struct genl_family nl80211_fam __ro_after_init = {
diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index 00be498aab2e..8858d396e4f8 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -1500,6 +1500,9 @@  int cfg80211_disconnect(struct cfg80211_registered_device *rdev,
 	if (!wdev->connected)
 		wdev->u.client.ssid_len = 0;
 
+	if (wdev->reg_6ghz_pwr_configured)
+		wdev->reg_6ghz_pwr_configured = false;
+
 	return err;
 }