From: Peng Xu <[email protected]>
Add HT and VHT information in struct cfg80211_ap_settings when
starting ap so that driver does not need to parse IE to obtain
the information.
Signed-off-by: Peng Xu <[email protected]>
Signed-off-by: Jouni Malinen <[email protected]>
---
include/net/cfg80211.h | 15 +++++++++++++++
include/uapi/linux/nl80211.h | 16 ++++++++++++++++
net/wireless/nl80211.c | 28 ++++++++++++++++++++++++++++
3 files changed, 59 insertions(+)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 9c23f4d3..6292d35 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -676,6 +676,12 @@ struct cfg80211_acl_data {
struct mac_address mac_addrs[];
};
+enum ht_vht_support {
+ HT_VHT_DISABLED,
+ HT_VHT_ENABLED,
+ HT_VHT_NOT_INDICATED
+};
+
/**
* struct cfg80211_ap_settings - AP configuration
*
@@ -700,6 +706,10 @@ struct cfg80211_acl_data {
* MAC address based access control
* @pbss: If set, start as a PCP instead of AP. Relevant for DMG
* networks.
+ * @ht_enabled: if HT capability is enabled/disabled/not indicated
+ * @vht_enabled: if VHT capability is enabled/disabled/not indicated
+ * @require_ht: require stations to support HT PHY
+ * @require_vht: require stations to support VHT PHY
*/
struct cfg80211_ap_settings {
struct cfg80211_chan_def chandef;
@@ -719,6 +729,11 @@ struct cfg80211_ap_settings {
bool p2p_opp_ps;
const struct cfg80211_acl_data *acl;
bool pbss;
+ enum ht_vht_support ht_enabled;
+ enum ht_vht_support vht_enabled;
+ enum ht_vht_support require_ht;
+ enum ht_vht_support require_vht;
+
};
/**
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 2206941..3ce88ff 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1867,6 +1867,17 @@ enum nl80211_commands {
* @NL80211_ATTR_MESH_PEER_AID: Association ID for the mesh peer (u16). This is
* used to pull the stored data for mesh peer in power save state.
*
+ * @NL80211_ATTR_HT_ENABLED: u8 value to indicate if HT capability is enabled.
+ * 0=disabled, 1=enabled, not being included as being no value available
+ * @NL80211_ATTR_VHT_ENABLED: u8 value to indicate if VHT capability is enabled.
+ * 0=disabled, 1=enabled, not being included as being no value available
+ * @NL80211_ATTR_REQUIRE_HT: u8 value to require stations to support HT phy.
+ * 0=not required, 1=required, not being included as being no value
+ * available
+ * @NL80211_ATTR_REQUIRE_VHT: u8 value to require stations to support VHT phy.
+ * 0=not required, 1=required, not being included as being no value
+ * available
+ *
* @NUM_NL80211_ATTR: total number of nl80211_attrs available
* @NL80211_ATTR_MAX: highest attribute number currently defined
* @__NL80211_ATTR_AFTER_LAST: internal use
@@ -2261,6 +2272,11 @@ enum nl80211_attrs {
NL80211_ATTR_MESH_PEER_AID,
+ NL80211_ATTR_HT_ENABLED,
+ NL80211_ATTR_VHT_ENABLED,
+ NL80211_ATTR_REQUIRE_HT,
+ NL80211_ATTR_REQUIRE_VHT,
+
/* add attributes here, update the policy in nl80211.c */
__NL80211_ATTR_AFTER_LAST,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 4997857..5f66abf 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -409,6 +409,10 @@ static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
.len = VHT_MUMIMO_GROUPS_DATA_LEN
},
[NL80211_ATTR_MU_MIMO_FOLLOW_MAC_ADDR] = { .len = ETH_ALEN },
+ [NL80211_ATTR_HT_ENABLED] { .type = NLA_U8 },
+ [NL80211_ATTR_VHT_ENABLED] { .type = NLA_U8 },
+ [NL80211_ATTR_REQUIRE_HT] { .type = NLA_U8 },
+ [NL80211_ATTR_REQUIRE_VHT] { .type = NLA_U8 },
};
/* policy for the key attributes */
@@ -3557,6 +3561,30 @@ static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info)
return PTR_ERR(params.acl);
}
+ if (info->attrs[NL80211_ATTR_HT_ENABLED])
+ params.ht_enabled = nla_get_u8(
+ info->attrs[NL80211_ATTR_HT_ENABLED]);
+ else
+ params.ht_enabled = HT_VHT_NOT_INDICATED;
+
+ if (info->attrs[NL80211_ATTR_VHT_ENABLED])
+ params.vht_enabled = nla_get_u8(
+ info->attrs[NL80211_ATTR_VHT_ENABLED]);
+ else
+ params.vht_enabled = HT_VHT_NOT_INDICATED;
+
+ if (info->attrs[NL80211_ATTR_REQUIRE_HT])
+ params.require_ht = nla_get_u8(
+ info->attrs[NL80211_ATTR_REQUIRE_HT]);
+ else
+ params.require_ht = HT_VHT_NOT_INDICATED;
+
+ if (info->attrs[NL80211_ATTR_REQUIRE_VHT])
+ params.require_vht = nla_get_u8(
+ info->attrs[NL80211_ATTR_REQUIRE_VHT]);
+ else
+ params.require_vht = HT_VHT_NOT_INDICATED;
+
wdev_lock(wdev);
err = rdev_start_ap(rdev, dev, ¶ms);
if (!err) {
--
1.9.1
On Tue, Aug 16, 2016 at 08:52:47AM +0200, Johannes Berg wrote:
> On Mon, 2016-08-15 at 21:07 +0300, Jouni Malinen wrote:
> > From: Peng Xu <[email protected]>
> > Add HT and VHT information in struct cfg80211_ap_settings when
> > starting ap so that driver does not need to parse IE to obtain
> > the information.
>=20
> > +enum ht_vht_support {
> > + HT_VHT_DISABLED,
> > + HT_VHT_ENABLED,
> > + HT_VHT_NOT_INDICATED
> > +};
>=20
> So if you get HT_VHT_NOT_INDICATED in the driver, don't you *still*
> have to parse the IEs?
Well.. Yes, I guess one would need to do that for some time until
relevant user space is expected to have been updated to support the new
attribute.
> Arguably, cfg80211 could know itself by parsing though, so it could
> already fall back to that, no?
>=20
> But if you do that, you already need the parsing code, so then perhaps
> it would make sense to just always use the parsing in cfg80211? Or
> export a parsing function to use in driver(s)?
I guess that could be considered reasonable approach for the existing
HT and VHT cases and new attributes would obviously be significantly
easier to introduce with new extensions (need to remember to do this for
HE from the beginning..).
The parsing for these HT/VHT enabled/required is a bit strange
combination having to go over three IEs. I'm not sure a parsing function
to do so would be that nice.. The parsing code could indeed be moved to
cfg80211 so that it would not need to be duplicated into each driver
needing this, though.
--=20
Jouni Malinen PGP id EFC895FA=
On Tue, 2016-08-16 at 12:34 +0000, Malinen, Jouni wrote:
>
> > So if you get HT_VHT_NOT_INDICATED in the driver, don't you *still*
> > have to parse the IEs?
>
> Well.. Yes, I guess one would need to do that for some time until
> relevant user space is expected to have been updated to support the
> new attribute.
So I guess the question is what drivers would use this, and how, and
would we have to introduce this code into the kernel anyway?
If yes, I'd argue it might as well be in cfg80211 instead of the
driver(s).
> I guess that could be considered reasonable approach for the existing
> HT and VHT cases and new attributes would obviously be significantly
> easier to introduce with new extensions (need to remember to do this
> for HE from the beginning..).
Indeed. I'll try to also remember :)
> The parsing for these HT/VHT enabled/required is a bit strange
> combination having to go over three IEs. I'm not sure a parsing
> function to do so would be that nice.. The parsing code could indeed
> be moved to cfg80211 so that it would not need to be duplicated into
> each driver needing this, though.
Right.
johannes
On Mon, 2016-08-15 at 21:07 +0300, Jouni Malinen wrote:
> From: Peng Xu <[email protected]>
>
> Add HT and VHT information in struct cfg80211_ap_settings when
> starting ap so that driver does not need to parse IE to obtain
> the information.
> +enum ht_vht_support {
> + HT_VHT_DISABLED,
> + HT_VHT_ENABLED,
> + HT_VHT_NOT_INDICATED
> +};
So if you get HT_VHT_NOT_INDICATED in the driver, don't you *still*
have to parse the IEs?
Arguably, cfg80211 could know itself by parsing though, so it could
already fall back to that, no?
But if you do that, you already need the parsing code, so then perhaps
it would make sense to just always use the parsing in cfg80211? Or
export a parsing function to use in driver(s)?
> @@ -719,6 +729,11 @@ struct cfg80211_ap_settings {
> bool p2p_opp_ps;
> const struct cfg80211_acl_data *acl;
> bool pbss;
> + enum ht_vht_support ht_enabled;
> + enum ht_vht_support vht_enabled;
> + enum ht_vht_support require_ht;
> + enum ht_vht_support require_vht;
> +
(nit - extra blank line)
johannes
So I haven't seen a response from you guys:
I have no major objections to this. However, a few things:
1) are you planning to add support for this into a kernel driver at
all, anyway?
2) are you planning to have a driver upstream that contains the now
necessary parsing?
Depending on the answers, I suppose we could/should merge this:
no * : don't merge
yes no : merge
yes yes: don't merge, put parsing into cfg80211
I guess?
We don't have to always draw a too rigid line for non-upstream things, but it's still disappointing that you keep proposing things that are only used for non-upstream drivers.
johannes
On Tue, 2016-10-04 at 16:15 +0000, Malinen, Jouni wrote:
>
> And even if the driver were to simply copy the IEs with the BSS
> membership selectors, this would only work with stations that
> implement this part correctly, i.e., the AP would not necessarily
> have any means for rejecting the association if a non-HT/VHT station
> were to try to associate.. With mac80211-drivers, this happens in
> hostapd.
Yes, that's a good point, so we'd still have to pass this information
down.
I think we'll look at adding the necessary parsing to cfg80211, and
then other drivers can pick it up as needed.
johannes
On Tue, Oct 04, 2016 at 03:25:51PM +0200, Johannes Berg wrote:
> > ath6kl:
> > Use cfg80211_get_chandef_type(&info->chandef) !=3D NL80211_CHAN_NO_HT
> > to determine whether HT is enabled. No VHT support. HT-required case
> > not covered. No parsing of HT/VHT IEs used.
>=20
> Hmm. Wouldn't the supported rates IE still advertise the cookie for HT
> only, to make sure HT-required was done?
I'm not completely sure how the driver/firmware behaves for this. There
is a reference to info->beacon.head, but that code looks like dead code
that does nothing as far as configuring the firmware with the supported
rates elements from user space. As such, I'd expect the firmware to
build (Ext)Supp Rates element from scratch and I'd assume ath6kl does
not currently support HT-required signaling. I'm not sure whether the
current firmware would even allow such configuration.
> [snip other drivers]
By the way, I did not find any clear example that would be either using
(Extended) Supported Rates element from start_ap() as-is or parsing it
for the BSS membership selectors in any of the in-tree drivers. In other
words, the HT/VHT required configuration may not really work properly
with non-mac80211 cases.
PS.
And even if the driver were to simply copy the IEs with the BSS
membership selectors, this would only work with stations that implement
this part correctly, i.e., the AP would not necessarily have any means
for rejecting the association if a non-HT/VHT station were to try to
associate.. With mac80211-drivers, this happens in hostapd.
--=20
Jouni Malinen PGP id EFC895FA=
> The main goal of this was to see if we can reduce actual driver
> implementation size and maybe even more so to prepare for 802.11ax
> changes (i.e., see what we are doing currently for configuring HT/VHT
> in a way that could be done better).
Fair enough.
> Looking at the current in-tree drivers, it looks like following
> approaches have been used:
>
> ath6kl:
> Use cfg80211_get_chandef_type(&info->chandef) != NL80211_CHAN_NO_HT
> to determine whether HT is enabled. No VHT support. HT-required case
> not covered. No parsing of HT/VHT IEs used.
Hmm. Wouldn't the supported rates IE still advertise the cookie for HT
only, to make sure HT-required was done?
[snip other drivers]
> So I guess there could be some code sharing and cleanup done with the
> existing in-tree drivers. Especially the mwifiex example looks like
> something that triggered us to look at this. I'm not sure we'd
> propose adding any new driver with the driver code itself doing
> HT/VHT IE parsing and since ath6kl did not do this either, I don't
> see us changing existing in-tree drivers to use this either.
Maybe Marvell folks would like to change something there.
> I'm not sure there would really be enough justification to add this
> specific patch due to the assumed user space update. Adding some of
> the HT/VHT element parsing in cfg80211 might benefit on or two of the
> in-tree drivers if their maintainers are interested in that.
Right, I agree. If we want to unify/combine anything, the best place to
do that would just be cfg80211 at this point. Adding both the API and
the parsing seems fairly pointless now, and evidently drivers can live
with the status quo.
> That said, without additional interest, I'm starting to lean towards
> using this as an example of what type of parameters we need to add
> for HE from the beginning and not merge this patch.
Ok. I agree that for HE we should consider this more carefully. I
haven't looked at HE AP side yet, so I don't really know what might be
needed there, but when we get to it we can discuss it.
I'll drop this patch then, and if Broadcom or you (ath6kl) want to add
more parsing to the respective drivers we can consider code sharing
with mwifiex that seems to have a more (but not fully) complete version
in there - probably would have to be rewritten for cfg80211, but still.
johannes
T24gTW9uLCBTZXAgMTIsIDIwMTYgYXQgMTI6MDk6NDJQTSArMDIwMCwgSm9oYW5uZXMgQmVyZyB3
cm90ZToNCj4gSSBoYXZlIG5vIG1ham9yIG9iamVjdGlvbnMgdG8gdGhpcy4gSG93ZXZlciwgYSBm
ZXcgdGhpbmdzOg0KPiANCj4gMSkgYXJlIHlvdSBwbGFubmluZyB0byBhZGQgc3VwcG9ydCBmb3Ig
dGhpcyBpbnRvIGEga2VybmVsIGRyaXZlciBhdA0KPiDCoCDCoGFsbCwgYW55d2F5Pw0KPiAyKSBh
cmUgeW91IHBsYW5uaW5nIHRvIGhhdmUgYSBkcml2ZXIgdXBzdHJlYW0gdGhhdCBjb250YWlucyB0
aGUgbm93DQo+IMKgIMKgbmVjZXNzYXJ5IHBhcnNpbmc/DQo+IA0KPiBEZXBlbmRpbmcgb24gdGhl
IGFuc3dlcnMsIEkgc3VwcG9zZSB3ZSBjb3VsZC9zaG91bGQgbWVyZ2UgdGhpczoNCj4gDQo+IG5v
IMKgKiDCoDogZG9uJ3QgbWVyZ2UNCj4geWVzIG5vIDogbWVyZ2UNCj4geWVzIHllczogZG9uJ3Qg
bWVyZ2UsIHB1dCBwYXJzaW5nIGludG8gY2ZnODAyMTENCj4gDQo+IEkgZ3Vlc3M/DQoNClRoZSBt
YWluIGdvYWwgb2YgdGhpcyB3YXMgdG8gc2VlIGlmIHdlIGNhbiByZWR1Y2UgYWN0dWFsIGRyaXZl
cg0KaW1wbGVtZW50YXRpb24gc2l6ZSBhbmQgbWF5YmUgZXZlbiBtb3JlIHNvIHRvIHByZXBhcmUg
Zm9yIDgwMi4xMWF4DQpjaGFuZ2VzIChpLmUuLCBzZWUgd2hhdCB3ZSBhcmUgZG9pbmcgY3VycmVu
dGx5IGZvciBjb25maWd1cmluZyBIVC9WSFQgaW4NCmEgd2F5IHRoYXQgY291bGQgYmUgZG9uZSBi
ZXR0ZXIpLg0KDQpBcyB5b3Ugbm90ZWQgZWFybGllciwgdGhpcyB3b3VsZCBub3QgcmVhbGx5IGFs
bG93IHRoZSBkcml2ZXIgdG8gZHJvcCBpdHMNCkhUL1ZIVCByZWxhdGVkIHBhcnNpbmcgb3BlcmF0
aW9ucyB3aXRob3V0IG1ha2luZyBzdXJlIHVzZXIgc3BhY2UNCmNvbXBvbmVudHMgd2VyZSB1cGRh
dGVkIGFzIHdlbGwuIFRoZSBhcHByb2FjaCB0byBhdm9pZCB0aGF0IGlzIHRoYXQNCiJwYXJzaW5n
IGludG8gY2ZnODAyMTEiLg0KDQpMb29raW5nIGF0IHRoZSBjdXJyZW50IGluLXRyZWUgZHJpdmVy
cywgaXQgbG9va3MgbGlrZSBmb2xsb3dpbmcNCmFwcHJvYWNoZXMgaGF2ZSBiZWVuIHVzZWQ6DQoN
CmF0aDZrbDoNClVzZSBjZmc4MDIxMV9nZXRfY2hhbmRlZl90eXBlKCZpbmZvLT5jaGFuZGVmKSAh
PSBOTDgwMjExX0NIQU5fTk9fSFQgdG8NCmRldGVybWluZSB3aGV0aGVyIEhUIGlzIGVuYWJsZWQu
IE5vIFZIVCBzdXBwb3J0LiBIVC1yZXF1aXJlZCBjYXNlIG5vdA0KY292ZXJlZC4gTm8gcGFyc2lu
ZyBvZiBIVC9WSFQgSUVzIHVzZWQuDQoNCmJyY21mbWFjOg0KVXNlcyBpbmZvLT5jaGFuZGVmLiBO
b3QgY2xlYXIgaG93IEhUL1ZIVCBjb3VsZCBiZSBkaXNhYmxlZCBvciByZXF1aXJlZC4NCk5vdGU6
IFBhcnNlcyBCZWFjb24gSUVzIGZvciBDb3VudHJ5IGFuZCBTU0lEIChhcyBiYWNrdXAgZm9yIHNz
aWQgPT0NCk5VTEwhPyksIFJTTiwgV1BBLg0KDQptd2lmaWV4Og0KVXNlcyBpbmZvLT5jaGFuZGVm
LiBQYXJzZXMgQmVhY29uIHRhaWw6IEVuYWJsZXMgVkhUIGJhc2VkIG9uIHNlYXJjaGluZw0KaW5m
by0+YmVhY29uLnRhaWwgZm9yIFZIVCBDYXBhYmlsaXR5IGVsZW1lbnQuIFBhcnNlcyBIVCBDYXBh
YmlsaXR5IHRvDQpkZXRlcm1pbmUgd2hldGhlciBIVCBpcyB0byBiZSBzdXBwb3J0ZWQgYW5kIGFs
c28gdG8gc2V0IHZhcmlvdXMgSFQNCnBhcmFtZXRlcnMuIERvZXMgbm90IHNlZW0gdG8gaGF2ZSBz
dXBwb3J0IGZvciBIVC9WSFQtcmVxdWlyZWQuDQoNCg0KU28gSSBndWVzcyB0aGVyZSBjb3VsZCBi
ZSBzb21lIGNvZGUgc2hhcmluZyBhbmQgY2xlYW51cCBkb25lIHdpdGggdGhlDQpleGlzdGluZyBp
bi10cmVlIGRyaXZlcnMuIEVzcGVjaWFsbHkgdGhlIG13aWZpZXggZXhhbXBsZSBsb29rcyBsaWtl
DQpzb21ldGhpbmcgdGhhdCB0cmlnZ2VyZWQgdXMgdG8gbG9vayBhdCB0aGlzLiBJJ20gbm90IHN1
cmUgd2UnZCBwcm9wb3NlDQphZGRpbmcgYW55IG5ldyBkcml2ZXIgd2l0aCB0aGUgZHJpdmVyIGNv
ZGUgaXRzZWxmIGRvaW5nIEhUL1ZIVCBJRQ0KcGFyc2luZyBhbmQgc2luY2UgYXRoNmtsIGRpZCBu
b3QgZG8gdGhpcyBlaXRoZXIsIEkgZG9uJ3Qgc2VlIHVzIGNoYW5naW5nDQpleGlzdGluZyBpbi10
cmVlIGRyaXZlcnMgdG8gdXNlIHRoaXMgZWl0aGVyLg0KDQpJJ20gbm90IHN1cmUgdGhlcmUgd291
bGQgcmVhbGx5IGJlIGVub3VnaCBqdXN0aWZpY2F0aW9uIHRvIGFkZCB0aGlzDQpzcGVjaWZpYyBw
YXRjaCBkdWUgdG8gdGhlIGFzc3VtZWQgdXNlciBzcGFjZSB1cGRhdGUuIEFkZGluZyBzb21lIG9m
IHRoZQ0KSFQvVkhUIGVsZW1lbnQgcGFyc2luZyBpbiBjZmc4MDIxMSBtaWdodCBiZW5lZml0IG9u
IG9yIHR3byBvZiB0aGUNCmluLXRyZWUgZHJpdmVycyBpZiB0aGVpciBtYWludGFpbmVycyBhcmUg
aW50ZXJlc3RlZCBpbiB0aGF0LiBUaGF0IHNhaWQsDQp3aXRob3V0IGFkZGl0aW9uYWwgaW50ZXJl
c3QsIEknbSBzdGFydGluZyB0byBsZWFuIHRvd2FyZHMgdXNpbmcgdGhpcyBhcw0KYW4gZXhhbXBs
ZSBvZiB3aGF0IHR5cGUgb2YgcGFyYW1ldGVycyB3ZSBuZWVkIHRvIGFkZCBmb3IgSEUgZnJvbSB0
aGUNCmJlZ2lubmluZyBhbmQgbm90IG1lcmdlIHRoaXMgcGF0Y2guDQoNCi0tIA0KSm91bmkgTWFs
aW5lbiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgUEdQIGlkIEVG
Qzg5NUZB