diff mbox series

[v2] cfg80211: add support to configure HE MCS for beacon rate

Message ID 1601762257-14934-1-git-send-email-rmanohar@codeaurora.org (mailing list archive)
State Changes Requested
Delegated to: Johannes Berg
Headers show
Series [v2] cfg80211: add support to configure HE MCS for beacon rate | expand

Commit Message

Rajkumar Manoharan Oct. 3, 2020, 9:57 p.m. UTC 
This allows an option to configure a single HE MCS beacon tx rate.

Signed-off-by: Rajkumar Manoharan <rmanohar@codeaurora.org>
---
v2: clear mcs ratemask when beacon tx rate is not set.

 include/uapi/linux/nl80211.h |  9 +++++++--
 net/wireless/nl80211.c       | 48 +++++++++++++++++++++++++++++++++++---------
 2 files changed, 46 insertions(+), 11 deletions(-)

Comments

Johannes Berg Oct. 8, 2020, 9:06 a.m. UTC | #1 
On Sat, 2020-10-03 at 14:57 -0700, Rajkumar Manoharan wrote:
> This allows an option to configure a single HE MCS beacon tx rate.
> 
> Signed-off-by: Rajkumar Manoharan <rmanohar@codeaurora.org>
> ---
> v2: clear mcs ratemask when beacon tx rate is not set.

I can't believe I didn't realize this before, but ...

> @@ -4672,14 +4673,20 @@ static int nl80211_parse_tx_bitrate_mask(struct genl_info *info,
>  					nla_len(tb[NL80211_TXRATE_HT]),
>  					mask->control[band].ht_mcs))
>  				return -EINVAL;
> -		}
> +		} else if (is_beacon_tx_rate)
> +			memset(&mask->control[band].ht_mcs, 0,
> +			       sizeof(mask->control[band].ht_mcs));

I think this is a bit strange?

I mean, why not just skip the

        /* Default to all rates enabled */
        for (i = 0; i < NUM_NL80211_BANDS; i++) {
		[...]
	}

code? Right now you're first setting it, and then clearing it again when
is_beacon_tx_rate is set ...


I'd prefer a "default_all_enabled" parameter, and then call the above
loop conditionally on it (perhaps moving it to a separate function to
not have all the deep indentation to worry about).

johannes
Johannes Berg Oct. 8, 2020, 9:09 a.m. UTC | #2 
On Thu, 2020-10-08 at 11:06 +0200, Johannes Berg wrote:
> On Sat, 2020-10-03 at 14:57 -0700, Rajkumar Manoharan wrote:
> > This allows an option to configure a single HE MCS beacon tx rate.
> > 
> > Signed-off-by: Rajkumar Manoharan <rmanohar@codeaurora.org>
> > ---
> > v2: clear mcs ratemask when beacon tx rate is not set.
> 
> I can't believe I didn't realize this before, but ...

Never mind that comment ... it wasn't there before, I was confused :)

> I mean, why not just skip the
> 
>         /* Default to all rates enabled */
>         for (i = 0; i < NUM_NL80211_BANDS; i++) {
> 		[...]
> 	}
> 
> code? Right now you're first setting it, and then clearing it again when
> is_beacon_tx_rate is set ...
> 
> 
> I'd prefer a "default_all_enabled" parameter, and then call the above
> loop conditionally on it (perhaps moving it to a separate function to
> not have all the deep indentation to worry about).

And maybe that should be a separate patch.

johannes
Rajkumar Manoharan Oct. 16, 2020, 7:17 p.m. UTC | #3 
On 2020-10-08 02:09, Johannes Berg wrote:
> On Thu, 2020-10-08 at 11:06 +0200, Johannes Berg wrote:
>> On Sat, 2020-10-03 at 14:57 -0700, Rajkumar Manoharan wrote:
>> > This allows an option to configure a single HE MCS beacon tx rate.
>> >
>> > Signed-off-by: Rajkumar Manoharan <rmanohar@codeaurora.org>
>> > ---
>> > v2: clear mcs ratemask when beacon tx rate is not set.
>> 
>> I can't believe I didn't realize this before, but ...
> 
> Never mind that comment ... it wasn't there before, I was confused :)
> 
No worries. :)

>> I mean, why not just skip the
>> 
>>         /* Default to all rates enabled */
>>         for (i = 0; i < NUM_NL80211_BANDS; i++) {
>> 		[...]
>> 	}
>> 
>> code? Right now you're first setting it, and then clearing it again 
>> when
>> is_beacon_tx_rate is set ...
>> 
>> 
>> I'd prefer a "default_all_enabled" parameter, and then call the above
>> loop conditionally on it (perhaps moving it to a separate function to
>> not have all the deep indentation to worry about).
> 
> And maybe that should be a separate patch.
> 
Neat.. Will split the patch and send a new series.

-Rajkumar
diff mbox series

Patch

diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 47700a2b9af9..0c9f0b8a1e4e 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1750,8 +1750,9 @@  enum nl80211_commands {
  *	specify just a single bitrate, which is to be used for the beacon.
  *	The driver must also specify support for this with the extended
  *	features NL80211_EXT_FEATURE_BEACON_RATE_LEGACY,
- *	NL80211_EXT_FEATURE_BEACON_RATE_HT and
- *	NL80211_EXT_FEATURE_BEACON_RATE_VHT.
+ *	NL80211_EXT_FEATURE_BEACON_RATE_HT,
+ *	NL80211_EXT_FEATURE_BEACON_RATE_VHT and
+ *	NL80211_EXT_FEATURE_BEACON_RATE_HE.
  *
  * @NL80211_ATTR_FRAME_MATCH: A binary attribute which typically must contain
  *	at least one byte, currently used with @NL80211_CMD_REGISTER_FRAME.
@@ -5896,6 +5897,9 @@  enum nl80211_feature_flags {
  * @NL80211_EXT_FEATURE_UNSOL_BCAST_PROBE_RESP: Driver/device supports
  *	unsolicited broadcast probe response transmission
  *
+ * @NL80211_EXT_FEATURE_BEACON_RATE_HE: Driver supports beacon rate
+ *	configuration (AP/mesh) with HE rates.
+ *
  * @NUM_NL80211_EXT_FEATURES: number of extended features.
  * @MAX_NL80211_EXT_FEATURES: highest extended feature index.
  */
@@ -5956,6 +5960,7 @@  enum nl80211_ext_feature_index {
 	NL80211_EXT_FEATURE_SAE_OFFLOAD_AP,
 	NL80211_EXT_FEATURE_FILS_DISCOVERY,
 	NL80211_EXT_FEATURE_UNSOL_BCAST_PROBE_RESP,
+	NL80211_EXT_FEATURE_BEACON_RATE_HE,
 
 	/* add new features before the definition below */
 	NUM_NL80211_EXT_FEATURES,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 91d4550677d0..c1c1f516b093 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -4591,7 +4591,8 @@  static int nl80211_parse_tx_bitrate_mask(struct genl_info *info,
 					 struct nlattr *attrs[],
 					 enum nl80211_attrs attr,
 					 struct cfg80211_bitrate_mask *mask,
-					 struct net_device *dev)
+					 struct net_device *dev,
+					 bool is_beacon_tx_rate)
 {
 	struct nlattr *tb[NL80211_TXRATE_MAX + 1];
 	struct cfg80211_registered_device *rdev = info->user_ptr[0];
@@ -4672,14 +4673,20 @@  static int nl80211_parse_tx_bitrate_mask(struct genl_info *info,
 					nla_len(tb[NL80211_TXRATE_HT]),
 					mask->control[band].ht_mcs))
 				return -EINVAL;
-		}
+		} else if (is_beacon_tx_rate)
+			memset(&mask->control[band].ht_mcs, 0,
+			       sizeof(mask->control[band].ht_mcs));
+
 		if (tb[NL80211_TXRATE_VHT]) {
 			if (!vht_set_mcs_mask(
 					sband,
 					nla_data(tb[NL80211_TXRATE_VHT]),
 					mask->control[band].vht_mcs))
 				return -EINVAL;
-		}
+		} else if (is_beacon_tx_rate)
+			memset(&mask->control[band].vht_mcs, 0,
+			       sizeof(mask->control[band].vht_mcs));
+
 		if (tb[NL80211_TXRATE_GI]) {
 			mask->control[band].gi =
 				nla_get_u8(tb[NL80211_TXRATE_GI]);
@@ -4691,6 +4698,10 @@  static int nl80211_parse_tx_bitrate_mask(struct genl_info *info,
 				     nla_data(tb[NL80211_TXRATE_HE]),
 				     mask->control[band].he_mcs))
 			return -EINVAL;
+		else if (is_beacon_tx_rate)
+			memset(&mask->control[band].he_mcs, 0,
+			       sizeof(mask->control[band].he_mcs));
+
 		if (tb[NL80211_TXRATE_HE_GI])
 			mask->control[band].he_gi =
 				nla_get_u8(tb[NL80211_TXRATE_HE_GI]);
@@ -4732,7 +4743,7 @@  static int validate_beacon_tx_rate(struct cfg80211_registered_device *rdev,
 				   enum nl80211_band band,
 				   struct cfg80211_bitrate_mask *beacon_rate)
 {
-	u32 count_ht, count_vht, i;
+	u32 count_ht, count_vht, count_he, i;
 	u32 rate = beacon_rate->control[band].legacy;
 
 	/* Allow only one rate */
@@ -4765,7 +4776,21 @@  static int validate_beacon_tx_rate(struct cfg80211_registered_device *rdev,
 			return -EINVAL;
 	}
 
-	if ((count_ht && count_vht) || (!rate && !count_ht && !count_vht))
+	count_he = 0;
+	for (i = 0; i < NL80211_HE_NSS_MAX; i++) {
+		if (hweight16(beacon_rate->control[band].he_mcs[i]) > 1) {
+			return -EINVAL;
+		} else if (beacon_rate->control[band].he_mcs[i]) {
+			count_he++;
+			if (count_he > 1)
+				return -EINVAL;
+		}
+		if (count_he && rate)
+			return -EINVAL;
+	}
+
+	if ((count_ht && count_vht && count_he) ||
+	    (!rate && !count_ht && !count_vht && !count_he))
 		return -EINVAL;
 
 	if (rate &&
@@ -4780,6 +4805,10 @@  static int validate_beacon_tx_rate(struct cfg80211_registered_device *rdev,
 	    !wiphy_ext_feature_isset(&rdev->wiphy,
 				     NL80211_EXT_FEATURE_BEACON_RATE_VHT))
 		return -EINVAL;
+	if (count_he &&
+	    !wiphy_ext_feature_isset(&rdev->wiphy,
+				     NL80211_EXT_FEATURE_BEACON_RATE_HE))
+		return -EINVAL;
 
 	return 0;
 }
@@ -5240,7 +5269,7 @@  static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info)
 		err = nl80211_parse_tx_bitrate_mask(info, info->attrs,
 						    NL80211_ATTR_TX_RATES,
 						    &params.beacon_rate,
-						    dev);
+						    dev, true);
 		if (err)
 			return err;
 
@@ -11084,7 +11113,7 @@  static int nl80211_set_tx_bitrate_mask(struct sk_buff *skb,
 
 	err = nl80211_parse_tx_bitrate_mask(info, info->attrs,
 					    NL80211_ATTR_TX_RATES, &mask,
-					    dev);
+					    dev, false);
 	if (err)
 		return err;
 
@@ -11693,7 +11722,7 @@  static int nl80211_join_mesh(struct sk_buff *skb, struct genl_info *info)
 		err = nl80211_parse_tx_bitrate_mask(info, info->attrs,
 						    NL80211_ATTR_TX_RATES,
 						    &setup.beacon_rate,
-						    dev);
+						    dev, true);
 		if (err)
 			return err;
 
@@ -14473,7 +14502,8 @@  static int parse_tid_conf(struct cfg80211_registered_device *rdev,
 		if (tid_conf->txrate_type != NL80211_TX_RATE_AUTOMATIC) {
 			attr = NL80211_TID_CONFIG_ATTR_TX_RATE;
 			err = nl80211_parse_tx_bitrate_mask(info, attrs, attr,
-						    &tid_conf->txrate_mask, dev);
+						    &tid_conf->txrate_mask, dev,
+						    false);
 			if (err)
 				return err;