diff mbox series

[v3,4/9] wifi: cfg80211: add NL command to set 6 GHz power mode

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

Commit Message

Aditya Kumar Singh March 15, 2023, 1:28 p.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 a new NL attributes to set power mode for AP
and client interfaces - NL80211_ATTR_6GHZ_REG_POWER_MODE.

Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com>
---
 include/net/cfg80211.h       |  6 +++-
 include/uapi/linux/nl80211.h | 10 ++++++
 net/wireless/ap.c            |  2 ++
 net/wireless/nl80211.c       | 66 +++++++++++++++++++++++++++++++++++-
 net/wireless/sme.c           |  2 ++
 5 files changed, 84 insertions(+), 2 deletions(-)

Comments

Johannes Berg Aug. 29, 2023, 5:51 p.m. UTC | #1 
On Wed, 2023-03-15 at 18:58 +0530, Aditya Kumar Singh wrote:
> 
> +	{
> +		.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 |
> +					 NL80211_FLAG_MLO_VALID_LINK_ID),
> +	},

Why is this even a new command, rather than a parameter to start AP or
similar?

Why do we even set it in client mode from userspace?

>  static struct genl_family nl80211_fam __ro_after_init = {
> @@ -17409,7 +17473,7 @@ static struct genl_family nl80211_fam __ro_after_init = {
>  	.n_ops = ARRAY_SIZE(nl80211_ops),
>  	.small_ops = nl80211_small_ops,
>  	.n_small_ops = ARRAY_SIZE(nl80211_small_ops),
> -	.resv_start_op = NL80211_CMD_REMOVE_LINK_STA + 1,
> +	.resv_start_op = NL80211_CMD_SET_6GHZ_POWER_MODE + 1,
> 

Obviously, this should not be done.

But in any case, I don't think there's a lot of value in doing a
detailed review of the code if we haven't gotten a good grasp of the
semantics that you want.

johannes
Aditya Kumar Singh Aug. 30, 2023, 5:05 a.m. UTC | #2 
On 8/29/23 23:21, Johannes Berg wrote:
> On Wed, 2023-03-15 at 18:58 +0530, Aditya Kumar Singh wrote:
>>
>> +	{
>> +		.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 |
>> +					 NL80211_FLAG_MLO_VALID_LINK_ID),
>> +	},
> 
> Why is this even a new command, rather than a parameter to start AP or
> similar?
> 
A new command was introduced because to give user space a knob to change 
power mode as and when required. Let's suppose AFC response has not yet 
arrived, AP could be started in Low Power mode. Now once AFC rules are 
applied (not going in detail here how that will happen) and user space
knows about it, it can send command to switch to Standard Power Mode right?


> Why do we even set it in client mode from userspace?
> 
>>   static struct genl_family nl80211_fam __ro_after_init = {
>> @@ -17409,7 +17473,7 @@ static struct genl_family nl80211_fam __ro_after_init = {
>>   	.n_ops = ARRAY_SIZE(nl80211_ops),
>>   	.small_ops = nl80211_small_ops,
>>   	.n_small_ops = ARRAY_SIZE(nl80211_small_ops),
>> -	.resv_start_op = NL80211_CMD_REMOVE_LINK_STA + 1,
>> +	.resv_start_op = NL80211_CMD_SET_6GHZ_POWER_MODE + 1,
>>
> 
> Obviously, this should not be done.
If this hunk is not there, the command was not going through. Upon 
digging further found out that the number of commands declared in the 
array and the count provided here has some relation. And that too with 
the last element added. Since a new element was added, modified it 
accordingly.

> 
> But in any case, I don't think there's a lot of value in doing a
> detailed review of the code if we haven't gotten a good grasp of the
> semantics that you want.
> 
> johannes
Sure let me try to address all your comments and then come up with new 
version probably which clears the doubts. Anyways, thanks for your 
comments.

Aditya
Johannes Berg Aug. 30, 2023, 7:41 a.m. UTC | #3 
On Wed, 2023-08-30 at 10:35 +0530, Aditya Kumar Singh wrote:
> On 8/29/23 23:21, Johannes Berg wrote:
> > On Wed, 2023-03-15 at 18:58 +0530, Aditya Kumar Singh wrote:
> > > 
> > > +	{
> > > +		.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 |
> > > +					 NL80211_FLAG_MLO_VALID_LINK_ID),
> > > +	},
> > 
> > Why is this even a new command, rather than a parameter to start AP or
> > similar?
> > 
> A new command was introduced because to give user space a knob to change 
> power mode as and when required. Let's suppose AFC response has not yet 
> arrived, AP could be started in Low Power mode. Now once AFC rules are 
> applied (not going in detail here how that will happen) and user space
> knows about it, it can send command to switch to Standard Power Mode right?

At least for AP that could also be a beacon update, like most other
operational parameters. You have to update the beacon _anyway_ since you
indicate in the beacon what you're doing.

So that by itself doesn't really introduce the requirement to have a
separate command.

> > Why do we even set it in client mode from userspace?

And you didn't comment on this - what userspace do you expect to use it
in client mode? I can see hostapd in AP mode, I guess, but in client
mode I don't see how this could be used? Anyway you're tied to what the
AP is doing, no?

> > >   static struct genl_family nl80211_fam __ro_after_init = {
> > > @@ -17409,7 +17473,7 @@ static struct genl_family nl80211_fam __ro_after_init = {
> > >   	.n_ops = ARRAY_SIZE(nl80211_ops),
> > >   	.small_ops = nl80211_small_ops,
> > >   	.n_small_ops = ARRAY_SIZE(nl80211_small_ops),
> > > -	.resv_start_op = NL80211_CMD_REMOVE_LINK_STA + 1,
> > > +	.resv_start_op = NL80211_CMD_SET_6GHZ_POWER_MODE + 1,
> > > 
> > 
> > Obviously, this should not be done.
> If this hunk is not there, the command was not going through. Upon 
> digging further found out that the number of commands declared in the 
> array and the count provided here has some relation. And that too with 
> the last element added. Since a new element was added, modified it 
> accordingly.

No no, that wasn't a discussion. I'm telling you, this is wrong, and I
will not apply a patch that does this. If you needed it, you did
something wrong in userspace.

johannes
Aditya Kumar Singh Aug. 30, 2023, 7:50 a.m. UTC | #4 
On 8/30/23 13:11, Johannes Berg wrote:
> On Wed, 2023-08-30 at 10:35 +0530, Aditya Kumar Singh wrote:
>> On 8/29/23 23:21, Johannes Berg wrote:
>>> On Wed, 2023-03-15 at 18:58 +0530, Aditya Kumar Singh wrote:
>>>>
>>>> +	{
>>>> +		.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 |
>>>> +					 NL80211_FLAG_MLO_VALID_LINK_ID),
>>>> +	},
>>>
>>> Why is this even a new command, rather than a parameter to start AP or
>>> similar?
>>>
>> A new command was introduced because to give user space a knob to change
>> power mode as and when required. Let's suppose AFC response has not yet
>> arrived, AP could be started in Low Power mode. Now once AFC rules are
>> applied (not going in detail here how that will happen) and user space
>> knows about it, it can send command to switch to Standard Power Mode right?
> 
> At least for AP that could also be a beacon update, like most other
> operational parameters. You have to update the beacon _anyway_ since you
> indicate in the beacon what you're doing.
Yeah this option can be explored too. Thanks for the suggestion.

> 
> So that by itself doesn't really introduce the requirement to have a
> separate command.
Yes I see your point. Let me re-work on this aspect as well.

> 
>>> Why do we even set it in client mode from userspace?
> 
> And you didn't comment on this - what userspace do you expect to use it
> in client mode? I can see hostapd in AP mode, I guess, but in client
> mode I don't see how this could be used? Anyway you're tied to what the
> AP is doing, no?
Yes, as you saw already replied to the similar question in the cover 
letter. Will reply to the last question here in that thread it self (to 
keep client related discussion in same place).


> 
>>>>    static struct genl_family nl80211_fam __ro_after_init = {
>>>> @@ -17409,7 +17473,7 @@ static struct genl_family nl80211_fam __ro_after_init = {
>>>>    	.n_ops = ARRAY_SIZE(nl80211_ops),
>>>>    	.small_ops = nl80211_small_ops,
>>>>    	.n_small_ops = ARRAY_SIZE(nl80211_small_ops),
>>>> -	.resv_start_op = NL80211_CMD_REMOVE_LINK_STA + 1,
>>>> +	.resv_start_op = NL80211_CMD_SET_6GHZ_POWER_MODE + 1,
>>>>
>>>
>>> Obviously, this should not be done.
>> If this hunk is not there, the command was not going through. Upon
>> digging further found out that the number of commands declared in the
>> array and the count provided here has some relation. And that too with
>> the last element added. Since a new element was added, modified it
>> accordingly.
> 
> No no, that wasn't a discussion. I'm telling you, this is wrong, and I
> will not apply a patch that does this. If you needed it, you did
> something wrong in userspace.
Oh okay. Fine, let me check again.

> 
> johannes
diff mbox series

Patch

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 7bc9ff9c3f36..d8d78141bab6 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -5805,7 +5805,8 @@  static inline void wiphy_unlock(struct wiphy *wiphy)
  * @unprot_beacon_reported: (private) timestamp of last
  *	unprotected beacon report
  * @links: array of %IEEE80211_MLD_MAX_NUM_LINKS elements containing @addr
- *	@ap and @client for each link
+ *	@ap and @client for each link. If link is 6 GHz link then uses
+ *	@reg_6ghz_pwr_configured as well.
  * @valid_links: bitmap describing what elements of @links are valid
  */
 struct wireless_dev {
@@ -5914,11 +5915,14 @@  struct wireless_dev {
 			struct {
 				unsigned int beacon_interval;
 				struct cfg80211_chan_def chandef;
+				enum ieee80211_ap_reg_power power_mode_6ghz;
 			} ap;
 			struct {
 				struct cfg80211_internal_bss *current_bss;
+				enum ieee80211_client_reg_power power_mode_6ghz;
 			} client;
 		};
+		bool reg_6ghz_pwr_configured;
 	} links[IEEE80211_MLD_MAX_NUM_LINKS];
 	u16 valid_links;
 };
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index eddccd491fc9..bff81489fa8a 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1309,6 +1309,8 @@ 
  *	The number of peers that HW timestamping can be enabled for concurrently
  *	is indicated by %NL80211_ATTR_MAX_HW_TIMESTAMP_PEERS.
  *
+ * @NL80211_CMD_SET_6GHZ_POWER_MODE: Set 6 GHz power mode for the interface
+ *
  * @NL80211_CMD_MAX: highest used command number
  * @__NL80211_CMD_AFTER_LAST: internal use
  */
@@ -1562,6 +1564,8 @@  enum nl80211_commands {
 
 	NL80211_CMD_SET_HW_TIMESTAMP,
 
+	NL80211_CMD_SET_6GHZ_POWER_MODE,
+
 	/* add new commands above here */
 
 	/* used to define NL80211_CMD_MAX below */
@@ -2786,6 +2790,10 @@  enum nl80211_commands {
  *	bit corresponds to the lowest 20 MHz channel. Each bit set to 1
  *	indicates that the sub-channel is punctured. Higher 16 bits are
  *	reserved.
+ * @NL80211_ATTR_6GHZ_REG_POWER_MODE: Regulatory power mode for 6 GHz operation.
+ *	This can be used for both AP and clients. Values are defined in
+ *	&enum ieee80211_ap_reg_power and &enum ieee80211_client_reg_power.
+ *	Therefore, iftype should be taken into consideration. u8 value.
  *
  * @NL80211_ATTR_MAX_HW_TIMESTAMP_PEERS: Maximum number of peers that HW
  *	timestamping can be enabled for concurrently (u16), a wiphy attribute.
@@ -3328,6 +3336,8 @@  enum nl80211_attrs {
 	NL80211_ATTR_MAX_HW_TIMESTAMP_PEERS,
 	NL80211_ATTR_HW_TIMESTAMP_ENABLED,
 
+	NL80211_ATTR_6GHZ_REG_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 0962770303b2..b8e2d9466434 100644
--- a/net/wireless/ap.c
+++ b/net/wireless/ap.c
@@ -30,6 +30,8 @@  static int ___cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
 	if (!wdev->links[link_id].ap.beacon_interval)
 		return -ENOENT;
 
+	wdev->links[link_id].reg_6ghz_pwr_configured = false;
+
 	err = rdev_stop_ap(rdev, dev, link_id);
 	if (!err) {
 		wdev->conn_owner_nlportid = 0;
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 1e8fe560078f..36cb4574145c 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -809,6 +809,7 @@  static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
 
 	[NL80211_ATTR_MAX_HW_TIMESTAMP_PEERS] = { .type = NLA_U16 },
 	[NL80211_ATTR_HW_TIMESTAMP_ENABLED] = { .type = NLA_FLAG },
+	[NL80211_ATTR_6GHZ_REG_POWER_MODE] = { .type = NLA_U8 },
 };
 
 /* policy for the key attributes */
@@ -16213,6 +16214,61 @@  static int nl80211_set_hw_timestamp(struct sk_buff *skb,
 	return rdev_set_hw_timestamp(rdev, dev, &hwts);
 }
 
+static bool nl80211_6ghz_power_mode_is_valid(enum nl80211_iftype iftype,
+					     u8 power_mode)
+{
+	/* For APs, referenced from enum ieee80211_ap_reg_power, and
+	 * For clients, referenced from enum ieee80211_client_reg_power
+	 */
+	switch (iftype) {
+	case NL80211_IFTYPE_AP:
+		return ((power_mode >= IEEE80211_REG_LPI_AP) &&
+			(power_mode <= IEEE80211_REG_AP_POWER_MAX));
+	case NL80211_IFTYPE_STATION:
+		return ((power_mode >= IEEE80211_REG_DEFAULT_CLIENT) &&
+			(power_mode <= IEEE80211_REG_CLIENT_POWER_MAX));
+	default:
+		return false;
+	}
+}
+
+static int nl80211_set_6ghz_power_mode(struct sk_buff *skb,
+				       struct genl_info *info)
+{
+	struct net_device *netdev = info->user_ptr[1];
+	struct wireless_dev *wdev = netdev->ieee80211_ptr;
+	enum nl80211_iftype iftype = wdev->iftype;
+	unsigned int link_id = nl80211_link_id(info->attrs);
+	u8 power_mode;
+
+	if (iftype != NL80211_IFTYPE_AP &&
+	    iftype != NL80211_IFTYPE_STATION)
+		return -EOPNOTSUPP;
+
+	if (!info->attrs[NL80211_ATTR_6GHZ_REG_POWER_MODE])
+		return -EINVAL;
+
+	power_mode = nla_get_u8(info->attrs[NL80211_ATTR_6GHZ_REG_POWER_MODE]);
+	if (!nl80211_6ghz_power_mode_is_valid(iftype, power_mode))
+		return -EINVAL;
+
+	wdev_lock(wdev);
+	if (wdev->links[link_id].reg_6ghz_pwr_configured) {
+		wdev_unlock(wdev);
+		return -EALREADY;
+	}
+
+	if (iftype == NL80211_IFTYPE_AP)
+		wdev->links[link_id].ap.power_mode_6ghz = power_mode;
+	else
+		wdev->links[link_id].client.power_mode_6ghz = power_mode;
+
+	wdev->links[link_id].reg_6ghz_pwr_configured = true;
+
+	wdev_unlock(wdev);
+	return 0;
+}
+
 #define NL80211_FLAG_NEED_WIPHY		0x01
 #define NL80211_FLAG_NEED_NETDEV	0x02
 #define NL80211_FLAG_NEED_RTNL		0x04
@@ -17393,6 +17449,14 @@  static const struct genl_small_ops nl80211_small_ops[] = {
 		.flags = GENL_UNS_ADMIN_PERM,
 		.internal_flags = IFLAGS(NL80211_FLAG_NEED_NETDEV_UP),
 	},
+	{
+		.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 |
+					 NL80211_FLAG_MLO_VALID_LINK_ID),
+	},
 };
 
 static struct genl_family nl80211_fam __ro_after_init = {
@@ -17409,7 +17473,7 @@  static struct genl_family nl80211_fam __ro_after_init = {
 	.n_ops = ARRAY_SIZE(nl80211_ops),
 	.small_ops = nl80211_small_ops,
 	.n_small_ops = ARRAY_SIZE(nl80211_small_ops),
-	.resv_start_op = NL80211_CMD_REMOVE_LINK_STA + 1,
+	.resv_start_op = NL80211_CMD_SET_6GHZ_POWER_MODE + 1,
 	.mcgrps = nl80211_mcgrps,
 	.n_mcgrps = ARRAY_SIZE(nl80211_mcgrps),
 	.parallel_ops = true,
diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index 28ce13840a88..5ccc371fdfaf 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -1558,6 +1558,8 @@  int cfg80211_disconnect(struct cfg80211_registered_device *rdev,
 	if (!wdev->connected)
 		wdev->u.client.ssid_len = 0;
 
+	wdev->links[0].reg_6ghz_pwr_configured = false;
+
 	return err;
 }