| Message ID | 1589399105-25472-2-git-send-email-rmanohar@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [v3,01/11] cfg80211: use only HE capability to set prohibited flags in 6 GHz | expand |
On Wed, 2020-05-13 at 12:44 -0700, Rajkumar Manoharan wrote: > Handle 6 GHz HE capability while adding new station. It will be used > later in mac80211 station processing. This doesn't compile without the next patch. > + const struct ieee80211_he_6ghz_band_cap *he_6ghz_capa; This we made just an __le16, any particular reason to have the struct? It does need to be a pointer for the "no changes" case, but the struct seems a bit overkill? > + * @NL80211_ATTR_HE_6GHZ_CAPABILITY: HE 6 GHz Band Capability element (from > + * association request when used with NL80211_CMD_NEW_STATION). That we have pretty much identically. > @@ -2998,6 +3003,7 @@ enum nl80211_attrs { > #define NL80211_HE_MAX_CAPABILITY_LEN 54 > #define NL80211_MAX_NR_CIPHER_SUITES 5 > #define NL80211_MAX_NR_AKM_SUITES 2 > +#define NL80211_HE_6GHZ_CAPABILITY_LEN 2 This not, we defined it just to be a U16. > + [NL80211_ATTR_HE_6GHZ_CAPABILITY] = { > + .type = NLA_EXACT_LEN, > + .len = NL80211_HE_6GHZ_CAPABILITY_LEN, > + }, This no longer exists, but I guess I'll just take our patch for the U16 here. > + /* Ensure that HT/VHT capabilities are not set for 6 GHz HE STA */ > + if (params.he_6ghz_capa && (params.ht_capa || params.vht_capa)) > + return -EINVAL; Not sure this makes much sense? We can only check what's being set at the same time, so multiple calls here would still be possible ... doesn't hurt much though. We didn't have this check, and have one additional check: @@ -6170,7 +6200,7 @@ static int nl80211_new_station(struct sk_buff *skb, struct genl_info *info) params.vht_capa = NULL; /* HE requires WME */ - if (params.he_capa_len) + if (params.he_capa_len || params.he_6ghz_capa) return -EINVAL; } johannes
On 2020-05-27 07:00, Johannes Berg wrote: > On Wed, 2020-05-13 at 12:44 -0700, Rajkumar Manoharan wrote: >> Handle 6 GHz HE capability while adding new station. It will be used >> later in mac80211 station processing. > > This doesn't compile without the next patch. > My bad.. I must have overlooked while splitting the patch. :( >> + const struct ieee80211_he_6ghz_band_cap *he_6ghz_capa; > > This we made just an __le16, any particular reason to have the struct? > It does need to be a pointer for the "no changes" case, but the struct > seems a bit overkill? > Initially I thought of splitting into two u8 for a_mpdu_params and info. Later changed to __le16 but retained struct. Nothing else :) >> + * @NL80211_ATTR_HE_6GHZ_CAPABILITY: HE 6 GHz Band Capability element >> (from >> + * association request when used with NL80211_CMD_NEW_STATION). > > That we have pretty much identically. > >> @@ -2998,6 +3003,7 @@ enum nl80211_attrs { >> #define NL80211_HE_MAX_CAPABILITY_LEN 54 >> #define NL80211_MAX_NR_CIPHER_SUITES 5 >> #define NL80211_MAX_NR_AKM_SUITES 2 >> +#define NL80211_HE_6GHZ_CAPABILITY_LEN 2 > > This not, we defined it just to be a U16. > >> + [NL80211_ATTR_HE_6GHZ_CAPABILITY] = { >> + .type = NLA_EXACT_LEN, >> + .len = NL80211_HE_6GHZ_CAPABILITY_LEN, >> + }, > > This no longer exists, but I guess I'll just take our patch for the U16 > here. > >> + /* Ensure that HT/VHT capabilities are not set for 6 GHz HE STA */ >> + if (params.he_6ghz_capa && (params.ht_capa || params.vht_capa)) >> + return -EINVAL; > > Not sure this makes much sense? We can only check what's being set at > the same time, so multiple calls here would still be possible ... > doesn't hurt much though. > > We didn't have this check, and have one additional check: > > @@ -6170,7 +6200,7 @@ static int nl80211_new_station(struct sk_buff > *skb, struct genl_info *info) > params.vht_capa = NULL; > > /* HE requires WME */ > - if (params.he_capa_len) > + if (params.he_capa_len || params.he_6ghz_capa) > return -EINVAL; > } > Fine. One more thing. Pradeep found that 6 GHz capability is not filled in set_station. Please handle that in your series. I'm fine with rest of the changes you mentioned. --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -5893,6 +5893,10 @@ static int nl80211_set_station_tdls(struct genl_info *info, return -EINVAL; } + if (info->attrs[NL80211_ATTR_HE_6GHZ_CAPABILITY]) + params->he_6ghz_capa = + nla_data(info->attrs[NL80211_ATTR_HE_6GHZ_CAPABILITY]); + -Rajkumar
On Wed, 2020-05-27 at 16:24 -0700, Rajkumar Manoharan wrote: > On 2020-05-27 07:00, Johannes Berg wrote: > > On Wed, 2020-05-13 at 12:44 -0700, Rajkumar Manoharan wrote: > > > Handle 6 GHz HE capability while adding new station. It will be used > > > later in mac80211 station processing. > > > > This doesn't compile without the next patch. > > > My bad.. I must have overlooked while splitting the patch. :( No worries. Looks like I'm reshuffling everything anyway :) > > > + const struct ieee80211_he_6ghz_band_cap *he_6ghz_capa; > > > > This we made just an __le16, any particular reason to have the struct? > > It does need to be a pointer for the "no changes" case, but the struct > > seems a bit overkill? > > > Initially I thought of splitting into two u8 for a_mpdu_params and info. > Later changed to __le16 but retained struct. Nothing else :) Right. I even saw that we're inconsistent - in mac80211 we used a struct too, and in cfg80211 I just did __le16 ... I'll be consistent with a struct, I guess. > > > @@ -2998,6 +3003,7 @@ enum nl80211_attrs { > > > #define NL80211_HE_MAX_CAPABILITY_LEN 54 > > > #define NL80211_MAX_NR_CIPHER_SUITES 5 > > > #define NL80211_MAX_NR_AKM_SUITES 2 > > > +#define NL80211_HE_6GHZ_CAPABILITY_LEN 2 > > > > This not, we defined it just to be a U16. And this should probably not be defined anyway since it comes from the spec (and we now export the policy to userspace even!) and in the policy we can then use sizeof(struct ...). > > > + [NL80211_ATTR_HE_6GHZ_CAPABILITY] = { > > > + .type = NLA_EXACT_LEN, > > > + .len = NL80211_HE_6GHZ_CAPABILITY_LEN, > > > + }, > > > > This no longer exists, but I guess I'll just take our patch for the U16 > > here. Sorry, I was confused - of course this still exists. NLA_EXACT_LEN_WARN no longer exists since my recent rework in this area. > > /* HE requires WME */ > > - if (params.he_capa_len) > > + if (params.he_capa_len || params.he_6ghz_capa) > > return -EINVAL; > > } > > > Fine. One more thing. Pradeep found that 6 GHz capability is not filled > in set_station. > Please handle that in your series. I'm fine with rest of the changes you > mentioned. > > --- a/net/wireless/nl80211.c > +++ b/net/wireless/nl80211.c > @@ -5893,6 +5893,10 @@ static int nl80211_set_station_tdls(struct > genl_info *info, > return -EINVAL; > } > > + if (info->attrs[NL80211_ATTR_HE_6GHZ_CAPABILITY]) > + params->he_6ghz_capa = > + > nla_data(info->attrs[NL80211_ATTR_HE_6GHZ_CAPABILITY]); > + OK, thanks! johannes
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 70e48f66dac8..0797a296c083 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -1225,6 +1225,7 @@ struct sta_txpwr { * @he_capa_len: the length of the HE capabilities * @airtime_weight: airtime scheduler weight for this station * @txpwr: transmit power for an associated station + * @he_6ghz_capa: HE 6 GHz Band capabilities of station */ struct station_parameters { const u8 *supported_rates; @@ -1257,6 +1258,7 @@ struct station_parameters { u8 he_capa_len; u16 airtime_weight; struct sta_txpwr txpwr; + const struct ieee80211_he_6ghz_band_cap *he_6ghz_capa; }; /** diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h index 2b691161830f..9c0a912f1684 100644 --- a/include/uapi/linux/nl80211.h +++ b/include/uapi/linux/nl80211.h @@ -2470,6 +2470,9 @@ enum nl80211_commands { * no roaming occurs between the reauth threshold and PMK expiration, * disassociation is still forced. * + * @NL80211_ATTR_HE_6GHZ_CAPABILITY: HE 6 GHz Band Capability element (from + * association request when used with NL80211_CMD_NEW_STATION). + * * @NUM_NL80211_ATTR: total number of nl80211_attrs available * @NL80211_ATTR_MAX: highest attribute number currently defined * @__NL80211_ATTR_AFTER_LAST: internal use @@ -2945,6 +2948,8 @@ enum nl80211_attrs { NL80211_ATTR_PMK_LIFETIME, NL80211_ATTR_PMK_REAUTH_THRESHOLD, + NL80211_ATTR_HE_6GHZ_CAPABILITY, + /* add attributes here, update the policy in nl80211.c */ __NL80211_ATTR_AFTER_LAST, @@ -2998,6 +3003,7 @@ enum nl80211_attrs { #define NL80211_HE_MAX_CAPABILITY_LEN 54 #define NL80211_MAX_NR_CIPHER_SUITES 5 #define NL80211_MAX_NR_AKM_SUITES 2 +#define NL80211_HE_6GHZ_CAPABILITY_LEN 2 #define NL80211_MIN_REMAIN_ON_CHANNEL_TIME 10 diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 692bcd35f809..bcd7a452e8b1 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -661,6 +661,10 @@ const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = { [NL80211_ATTR_CONTROL_PORT_NO_PREAUTH] = { .type = NLA_FLAG }, [NL80211_ATTR_PMK_LIFETIME] = NLA_POLICY_MIN(NLA_U32, 1), [NL80211_ATTR_PMK_REAUTH_THRESHOLD] = NLA_POLICY_RANGE(NLA_U8, 1, 100), + [NL80211_ATTR_HE_6GHZ_CAPABILITY] = { + .type = NLA_EXACT_LEN, + .len = NL80211_HE_6GHZ_CAPABILITY_LEN, + }, }; /* policy for the key attributes */ @@ -6129,6 +6133,10 @@ static int nl80211_new_station(struct sk_buff *skb, struct genl_info *info) return -EINVAL; } + if (info->attrs[NL80211_ATTR_HE_6GHZ_CAPABILITY]) + params.he_6ghz_capa = + nla_data(info->attrs[NL80211_ATTR_HE_6GHZ_CAPABILITY]); + if (info->attrs[NL80211_ATTR_OPMODE_NOTIF]) { params.opmode_notif_used = true; params.opmode_notif = @@ -6177,6 +6185,10 @@ static int nl80211_new_station(struct sk_buff *skb, struct genl_info *info) return -EINVAL; } + /* Ensure that HT/VHT capabilities are not set for 6 GHz HE STA */ + if (params.he_6ghz_capa && (params.ht_capa || params.vht_capa)) + return -EINVAL; + /* When you run into this, adjust the code below for the new flag */ BUILD_BUG_ON(NL80211_STA_FLAG_MAX != 7);
Handle 6 GHz HE capability while adding new station. It will be used later in mac80211 station processing. Signed-off-by: Rajkumar Manoharan <rmanohar@codeaurora.org> --- include/net/cfg80211.h | 2 ++ include/uapi/linux/nl80211.h | 6 ++++++ net/wireless/nl80211.c | 12 ++++++++++++ 3 files changed, 20 insertions(+)