[v2,1/3] nl80211: Add support for beacon tx mode

Message ID 1619696874-30072-2-git-send-email-mkenna@codeaurora.org
State Superseded
Headers show
Series
  • Add support to configure beacon tx mode
Related show

Commit Message

Maharaja Kennadyrajan April 29, 2021, 11:47 a.m.
User can configure the below beacon tx mode
1. Staggered mode and 2. Burst mode while bring-up the AP
or MESH.

Beacons can be sent out in burst(continuously in a single shot
one after another) or staggered (equally spread out over beacon
interval) mode.

Set the beacon_tx_mode as 1 for Staggered mode and 2 for
burst mode.

Hence, added the support in the nl80211/cfg80211
layer to honour the beacon tx mode configuration and pass
this value to the driver.

Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-00630-QCAHKSWPL_SILICONZ-2

Signed-off-by: Maharaja Kennadyrajan <mkenna@codeaurora.org>
---

V2:
	Addressed Johannes's comment on v1 patch.
	Updated the commit log.

 include/net/cfg80211.h       |  4 ++++
 include/uapi/linux/nl80211.h | 15 +++++++++++++++
 net/wireless/nl80211.c       |  9 +++++++++
 3 files changed, 28 insertions(+)

Comments

Jeff Johnson May 3, 2021, 5:24 p.m. | #1
On 2021-04-29 04:47, Maharaja Kennadyrajan wrote:
[..snip..]
> +/**

> + * enum nl80211_beacon_tx_mode - Beacon Tx Mode enum.

> + *      Used to configure beacon staggered mode or beacon burst mode.

> + */

> +enum nl80211_beacon_tx_mode {

> +	NL80211_BEACON_STAGGERED_MODE = 1,

> +	NL80211_BEACON_BURST_MODE = 2,

> +};

> +

[..snip..]
> @@ -5330,6 +5331,10 @@ static int nl80211_start_ap(struct sk_buff *skb,

> struct genl_info *info)

>  	params.dtim_period =

>  		nla_get_u32(info->attrs[NL80211_ATTR_DTIM_PERIOD]);

> 

> +	if (info->attrs[NL80211_ATTR_BEACON_TX_MODE])

> +		params.beacon_tx_mode =

> +

> nla_get_u32(info->attrs[NL80211_ATTR_BEACON_TX_MODE]);

> +


Note that in the case where NL80211_ATTR_BEACON_TX_MODE is not specified 
that
beacon_tx_mode will be zero, which is not a valid nl80211_beacon_tx_mode 
enumeration.

Should you renumber the nl80211_beacon_tx_mode enumerations so that the 
default
mode has a value of 0? Or add NL80211_BEACON_DEFAULT_MODE = 0 and allow 
the driver
to select a default mode?
Maharaja Kennadyrajan May 5, 2021, 7:18 a.m. | #2
On 2021-05-03 22:54, jjohnson@codeaurora.org wrote:
> On 2021-04-29 04:47, Maharaja Kennadyrajan wrote:

> [..snip..]

>> +/**

>> + * enum nl80211_beacon_tx_mode - Beacon Tx Mode enum.

>> + *      Used to configure beacon staggered mode or beacon burst mode.

>> + */

>> +enum nl80211_beacon_tx_mode {

>> +	NL80211_BEACON_STAGGERED_MODE = 1,

>> +	NL80211_BEACON_BURST_MODE = 2,

>> +};

>> +

> [..snip..]

>> @@ -5330,6 +5331,10 @@ static int nl80211_start_ap(struct sk_buff 

>> *skb,

>> struct genl_info *info)

>>  	params.dtim_period =

>>  		nla_get_u32(info->attrs[NL80211_ATTR_DTIM_PERIOD]);

>> 

>> +	if (info->attrs[NL80211_ATTR_BEACON_TX_MODE])

>> +		params.beacon_tx_mode =

>> +

>> nla_get_u32(info->attrs[NL80211_ATTR_BEACON_TX_MODE]);

>> +

> 

> Note that in the case where NL80211_ATTR_BEACON_TX_MODE is not 

> specified that

> beacon_tx_mode will be zero, which is not a valid

> nl80211_beacon_tx_mode enumeration.

> 

> Should you renumber the nl80211_beacon_tx_mode enumerations so that the 

> default

> mode has a value of 0? Or add NL80211_BEACON_DEFAULT_MODE = 0 and

> allow the driver

> to select a default mode?


[Maha]: Yes, it will select the default mode as STAGGERED mode when the 
user set
beacon_tx_mode as 0 in the hostapd conf file.
Also, it will select the default mode as STAGGERED mode when the user 
didn't add
beacon_tx_mode config in the hostapd conf file.
This is how it is handled here already.

Regards,
Maha
Jeff Johnson May 5, 2021, 2:27 p.m. | #3
On 2021-05-05 00:18, Maharaja Kennadyrajan wrote:
> On 2021-05-03 22:54, jjohnson@codeaurora.org wrote:

>> On 2021-04-29 04:47, Maharaja Kennadyrajan wrote:

>> [..snip..]

>>> +/**

>>> + * enum nl80211_beacon_tx_mode - Beacon Tx Mode enum.

>>> + *      Used to configure beacon staggered mode or beacon burst 

>>> mode.

>>> + */

>>> +enum nl80211_beacon_tx_mode {

>>> +	NL80211_BEACON_STAGGERED_MODE = 1,

>>> +	NL80211_BEACON_BURST_MODE = 2,

>>> +};

>>> +

>> [..snip..]

>>> @@ -5330,6 +5331,10 @@ static int nl80211_start_ap(struct sk_buff 

>>> *skb,

>>> struct genl_info *info)

>>>  	params.dtim_period =

>>>  		nla_get_u32(info->attrs[NL80211_ATTR_DTIM_PERIOD]);

>>> 

>>> +	if (info->attrs[NL80211_ATTR_BEACON_TX_MODE])

>>> +		params.beacon_tx_mode =

>>> +

>>> nla_get_u32(info->attrs[NL80211_ATTR_BEACON_TX_MODE]);

>>> +

>> 

>> Note that in the case where NL80211_ATTR_BEACON_TX_MODE is not 

>> specified that

>> beacon_tx_mode will be zero, which is not a valid

>> nl80211_beacon_tx_mode enumeration.

>> 

>> Should you renumber the nl80211_beacon_tx_mode enumerations so that 

>> the default

>> mode has a value of 0? Or add NL80211_BEACON_DEFAULT_MODE = 0 and

>> allow the driver

>> to select a default mode?

> 

> [Maha]: Yes, it will select the default mode as STAGGERED mode when the 

> user set

> beacon_tx_mode as 0 in the hostapd conf file.

> Also, it will select the default mode as STAGGERED mode when the user 

> didn't add

> beacon_tx_mode config in the hostapd conf file.

> This is how it is handled here already.


Regardless of how it is handled, I still assert that there is a
coding/logic error here since in the case the attribute is not
present you send an invalid enumeration (0) to the driver. I'm
suggesting to fix that logic/coding error either by renumbering
the existing enumerations or by adding a new enumeration so that
0 is a valid enumeration.

Patch

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 5224f88..88ed048 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1182,6 +1182,7 @@  enum cfg80211_ap_settings_flags {
  * @he_oper: HE operation IE (or %NULL if HE isn't enabled)
  * @fils_discovery: FILS discovery transmission parameters
  * @unsol_bcast_probe_resp: Unsolicited broadcast probe response parameters
+ * @beacon_tx_mode: Beacon Tx Mode setting
  */
 struct cfg80211_ap_settings {
 	struct cfg80211_chan_def chandef;
@@ -1214,6 +1215,7 @@  struct cfg80211_ap_settings {
 	struct cfg80211_he_bss_color he_bss_color;
 	struct cfg80211_fils_discovery fils_discovery;
 	struct cfg80211_unsol_bcast_probe_resp unsol_bcast_probe_resp;
+	enum nl80211_beacon_tx_mode beacon_tx_mode;
 };
 
 /**
@@ -2061,6 +2063,7 @@  struct mesh_config {
  *	to operate on DFS channels.
  * @control_port_over_nl80211: TRUE if userspace expects to exchange control
  *	port frames over NL80211 instead of the network interface.
+ * @beacon_tx_mode: Beacon Tx Mode setting.
  *
  * These parameters are fixed when the mesh is created.
  */
@@ -2084,6 +2087,7 @@  struct mesh_setup {
 	struct cfg80211_bitrate_mask beacon_rate;
 	bool userspace_handles_dfs;
 	bool control_port_over_nl80211;
+	enum nl80211_beacon_tx_mode beacon_tx_mode;
 };
 
 /**
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index f962c06..c9a447a 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -2560,6 +2560,10 @@  enum nl80211_commands {
  *	disassoc events to indicate that an immediate reconnect to the AP
  *	is desired.
  *
+ * @NL80211_ATTR_BEACON_TX_MODE: used to configure the beacon tx mode as
+ *      staggered mode = 1 or burst mode = 2 in %NL80211_CMD_START_AP or
+ *      %NL80211_CMD_JOIN_MESH from user-space.
+ *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -3057,6 +3061,8 @@  enum nl80211_attrs {
 
 	NL80211_ATTR_DISABLE_HE,
 
+	NL80211_ATTR_BEACON_TX_MODE,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
@@ -7299,4 +7305,13 @@  enum nl80211_sar_specs_attrs {
 	NL80211_SAR_ATTR_SPECS_MAX = __NL80211_SAR_ATTR_SPECS_LAST - 1,
 };
 
+/**
+ * enum nl80211_beacon_tx_mode - Beacon Tx Mode enum.
+ *      Used to configure beacon staggered mode or beacon burst mode.
+ */
+enum nl80211_beacon_tx_mode {
+	NL80211_BEACON_STAGGERED_MODE = 1,
+	NL80211_BEACON_BURST_MODE = 2,
+};
+
 #endif /* __LINUX_NL80211_H */
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 7e811a3..edf6640 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -759,6 +759,7 @@  static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
 	[NL80211_ATTR_RECONNECT_REQUESTED] = { .type = NLA_REJECT },
 	[NL80211_ATTR_SAR_SPEC] = NLA_POLICY_NESTED(sar_policy),
 	[NL80211_ATTR_DISABLE_HE] = { .type = NLA_FLAG },
+	[NL80211_ATTR_BEACON_TX_MODE] = NLA_POLICY_RANGE(NLA_U32, 1, 2),
 };
 
 /* policy for the key attributes */
@@ -5330,6 +5331,10 @@  static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info)
 	params.dtim_period =
 		nla_get_u32(info->attrs[NL80211_ATTR_DTIM_PERIOD]);
 
+	if (info->attrs[NL80211_ATTR_BEACON_TX_MODE])
+		params.beacon_tx_mode =
+			nla_get_u32(info->attrs[NL80211_ATTR_BEACON_TX_MODE]);
+
 	err = cfg80211_validate_beacon_int(rdev, dev->ieee80211_ptr->iftype,
 					   params.beacon_interval);
 	if (err)
@@ -11884,6 +11889,10 @@  static int nl80211_join_mesh(struct sk_buff *skb, struct genl_info *info)
 			return -EINVAL;
 	}
 
+	if (info->attrs[NL80211_ATTR_BEACON_TX_MODE])
+		setup.beacon_tx_mode =
+			nla_get_u32(info->attrs[NL80211_ATTR_BEACON_TX_MODE]);
+
 	if (info->attrs[NL80211_ATTR_MESH_SETUP]) {
 		/* parse additional setup parameters if given */
 		err = nl80211_parse_mesh_setup(info, &setup);