Re: [RFC 1/3] cfg80211: Add support for HE
On 5/25/2018 9:51 PM, Luca Coelho wrote:
Arend,
On Fri, 2018-05-25 at 13:11 +0300, Luca Coelho wrote:
On Mon, 2018-05-21 at 21:47 +0200, Arend van Spriel wrote:
On 5/18/2018 4:05 PM, Luca Coelho wrote:
@@ -781,6 +783,23 @@ int wiphy_register(struct wiphy *wiphy)
sband->channels[i].band = band;
}
+ for (i = 0; i < sband->n_iftype_data; i++) {
+ const struct ieee80211_sband_iftype_data
*iftd;
+
+ iftd = &sband->iftype_data[i];
+
+ if (WARN_ON(!iftd->types))
+ return -EINVAL;
+ if (WARN_ON(types & iftd->types))
+ return -EINVAL;
I suspected the types mask was not allowed to overlap for the
iftype_data entries, but may be worth documenting that in struct
ieee80211_sband_iftype_data kerneldoc.
Sure, I'll add it.
Actually, looking into this again, I'm not sure I understand your
comment. AFAICT this prevents the same type from appearing twice,
right? I don't get the "not allowed to overlap"... Can you clarify?
I mean that iftdata entries can not overlap by having the same iftypes
set in their mask. So yes, a particular iftype may only be used in a
single entry (yet another way to say it ;-) ).
Gr. AvS
Re: [RFC 1/3] cfg80211: Add support for HE
Arend,
On Fri, 2018-05-25 at 13:11 +0300, Luca Coelho wrote:
> On Mon, 2018-05-21 at 21:47 +0200, Arend van Spriel wrote:
> > On 5/18/2018 4:05 PM, Luca Coelho wrote:
> > > @@ -781,6 +783,23 @@ int wiphy_register(struct wiphy *wiphy)
> > > sband->channels[i].band = band;
> > > }
> > >
> > > + for (i = 0; i < sband->n_iftype_data; i++) {
> > > + const struct ieee80211_sband_iftype_data
> > > *iftd;
> > > +
> > > + iftd = &sband->iftype_data[i];
> > > +
> > > + if (WARN_ON(!iftd->types))
> > > + return -EINVAL;
> > > + if (WARN_ON(types & iftd->types))
> > > + return -EINVAL;
> >
> > I suspected the types mask was not allowed to overlap for the
> > iftype_data entries, but may be worth documenting that in struct
> > ieee80211_sband_iftype_data kerneldoc.
>
> Sure, I'll add it.
Actually, looking into this again, I'm not sure I understand your
comment. AFAICT this prevents the same type from appearing twice,
right? I don't get the "not allowed to overlap"... Can you clarify?
--
Cheers,
Luca.
Re: [RFC 1/3] cfg80211: Add support for HE
On 5/25/2018 12:11 PM, Luca Coelho wrote:
+static int
> >+nl80211_send_ift_data(struct sk_buff *msg,
> >+const struct ieee80211_sband_iftype_data *iftdata)
>
>make it nl80211_send_iftype_data.
Okay, I'll replace all ift instances to iftype.
My comment is mainly about function names.
> > static int nl80211_send_band_rateinfo(struct sk_buff *msg,
> > struct ieee80211_supported_band *sband)
> > {
> >@@ -1353,6 +1383,32 @@ static int nl80211_send_band_rateinfo(struct sk_buff
*msg,
> >sband->vht_cap.cap)))
> > return -ENOBUFS;
> >
> >+ if (sband->n_iftype_data) {
> >+ struct nlattr *nl_iftype_data =
> >+ nla_nest_start(msg, NL80211_BAND_ATTR_IFTYPE_DATA);
> >+ int err;
> >+
> >+ if (!nl_iftype_data)
> >+ return -ENOBUFS;
> >+
> >+ for (i = 0; i < sband->n_iftype_data; i++) {
> >+ struct nlattr *iftdata;
> >+
> >+ iftdata = nla_nest_start(msg, i + 1);
> >+ if (!iftdata)
> >+ return -ENOBUFS;
>
>bit inconsistent dealing with error path. Here errno is returned
>
> >@@ -4490,6 +4549,19 @@ static bool nl80211_put_sta_rate(struct sk_buff *msg,
struct rate_info *info,
> > if (info->flags & RATE_INFO_FLA
return false;
> >+ } else if (info->flags & RATE_INFO_FLAGS_HE_MCS) {
> >+ if (nla_put_u8(msg, NL80211_RATE_INFO_HE_MCS, info->mcs))
> >+ return false;
>
>... and here bool is returned. Admittedly, this seems to have been the
>case already before this patch.
Sure.
> >diff --git a/net/wireless/util.c b/net/wireless/util.c
> >index d112e9a89364..b66a68a41cd6 100644
> >--- a/net/wireless/util.c
> >+++ b/net/wireless/util.c
> >@@ -4,6 +4,7 @@
> >*
> >* Copyright 2007-2009 Johannes Berg
> >* Copyright 2013-2014 Intel Mobile Communications GmbH
> >+ * Copyright 2017 Intel Deutschland GmbH
> >*/
> > #include
> > #include
> >@@ -1142,6 +1143,85 @@ static u32 cfg80211_calculate_bitrate_vht(struct
rate_info *rate)
> > return 0;
> > }
> >
> >+static u32 cfg80211_calculate_bitrate_he(struct rate_info *rate)
> >+{
> >+#define SCALE 2048
> >+ u16 mcs_divisors[12] = {
> >+ 34133, /* 16.66... */
> >+ 17067, /* 8.33... */
> >+ 11378, /* 5.55... */
> >+ 8533, /* 4.16... */
> >+ 5689, /* 2.77... */
> >+ 4267, /* 2.08... */
> >+ 3923, /* 1.851851... */
> >+ 3413, /* 1.66... */
> >+ 2844, /* 1.38... */
> >+ 2560, /* 1.25... */
> >+ 2276, /* 1.11... */
> >+ 2048, /* 1.00... */
> >+ };
> >+ u32 rates_160M[3] = { 96077, 90740, 81666 };
> >+ u32 rates_969[3] = { 48038, 45370, 40833 };
> >+ u32 rates_484[3] = { 22941, 21666, 19500 };
> >+ u32 rates_242[3] = { 11471, 10833, 9750 };
> >+ u32 rates_106[3] = { 4000, 3777, 3400 };
> >+ u32 rates_52[3] = { 1882, 1777, 1600 };
> >+ u32 rates_26[3] = { 941, 888, 800 };
> >+ u64 tmp;
> >+ u32 result;
> >+
> >+ if (WARN_ON_ONCE(rate->mcs > 11))
> >+ return 0;
> >+
> >+ if (WARN_ON_ONCE(rate->he_gi > NL80211_RATE_INFO_HE_GI_3_2))
> >+ return 0;
> >+ if (WARN_ON_ONCE(rate->he_ru_alloc >
> >+ NL80211_RATE_INFO_HE_RU_ALLOC_2x996))
> >+ return 0;
> >+ if (WARN_ON_ONCE(rate->nss < 1 || rate->nss > 8))
> >+ return 0;
> >+
> >+ if (rate->bw == RATE_INFO_BW_160)
> >+ result = rates_160M[rate->he_gi];
> >+ else if (rate->bw == RATE_INFO_BW_80 ||
> >+ (rate->bw == RATE_INFO_BW_HE_RU &&
> >+rate->he_ru_alloc == NL80211_RATE_INFO_HE_RU_ALLOC_996))
> >+ result = rates_969[rate->he_gi];
> >+ else if (rate->bw == RATE_INFO_BW_40 ||
> >+ (rate->bw == RATE_INFO_BW_HE_RU &&
> >+rate->he_ru_alloc == NL80211_RATE_INFO_HE_RU_ALLOC_484))
> >+ result = rates_484[rate->he_gi];
> >+ else if (rate->bw == RATE_INFO_BW_20 ||
> >+ (rate->bw == RATE_INFO_BW_HE_RU &&
> >+rate->he_ru_alloc == NL80211_RATE_INFO_HE_RU_ALLOC_242))
> >+ result = rates_242[rate->he_gi];
> >+ else if (rate->bw == RATE_INFO_BW_HE_RU &&
> >+ rate->he_ru_alloc == NL80211_RATE_INFO_HE_RU_ALLOC_106)
> >+ result = rates_106[rate->he_gi];
> >+ else if (rate->bw == RATE_INFO_BW_HE_RU &&
> >+ rate->he_ru_alloc == NL80211_RATE_INFO_HE_RU_ALLOC_52)
> >+ result
Re: [RFC 1/3] cfg80211: Add support for HE
Hi Arend,
On Mon, 2018-05-21 at 21:47 +0200, Arend van Spriel wrote:
> On 5/18/2018 4:05 PM, Luca Coelho wrote:
> > From: Luca Coelho
> >
> > Add support for the HE in cfg80211 and also add userspace API to
> > nl80211 to send rate information out, conforming with
> > P802.11ax_D1.4.
>
> A couple of things changed in D2.0 so does it make sense to
> introduce
> stuff from older draft?
That was my mistake. We support D2.0, it's just that the changes for
D2.0 were mixed in an iwlwifi driver patch and I missed it.
> > +/**
> > + * struct ieee80211_he_mcs_nss_supp - HE Tx/Rx HE MCS NSS Support Field
> > + *
> > + * This structure holds the data required for the Tx/Rx HE MCS NSS Support
> > Field
> > + * described in P802.11ax_D1.4 section 9.4.2.237.4
> > + *
> > + * @rx_msc_80: Rx MCS map 2 bits for each stream, total 8 streams, for
> > channel
> > + * widths less than 80MHz.
> > + * @tx_msc_80: Tx MCS map 2 bits for each stream, total 8 streams, for
> > channel
> > + * widths less than 80MHz.
> > + * @rx_msc_160: Rx MCS map 2 bits for each stream, total 8 streams, for
> > channel
> > + * width 160MHz.
> > + * @tx_msc_160: Tx MCS map 2 bits for each stream, total 8 streams, for
> > channel
> > + * width 160MHz.
> > + * @rx_msc_80p80: Rx MCS map 2 bits for each stream, total 8 streams, for
> > + * channel width 80p80MHz.
> > + * @tx_msc_80p80: Tx MCS map 2 bits for each stream, total 8 streams, for
> > + * channel width 80p80MHz.
> > + */
> > +struct ieee80211_he_mcs_nss_supp {
> > + __le16 rx_msc_80;
>
> Should 'msc' in these fields be 'mcs'?
Certainly, it's a typo and I'll fix it.
> > + __le16 tx_msc_80;
> > + __le16 rx_msc_160;
> > + __le16 tx_msc_160;
> > + __le16 rx_msc_80p80;
> > + __le16 tx_msc_80p80;
> > +} __packed;
> > +
> > +/**
> > + * struct ieee80211_he_operation - HE capabilities element
> > + *
> > + * This structure is the "HE operation element" fields as
> > + * described in P802.11ax_D1.4 section 9.4.2.238
> > + */
> > +struct ieee80211_he_operation {
> > + __le32 he_oper_params;
> > + __le16 he_mcs_nss_set;
> > + /* Optional 0,1,3 or 4 bytes: depends on %he_oper_params */
> > + u8 optional[0];
> > +} __packed;
>
> If I recall correctly the he operation element changed significantly in
> later revisions of the spec. So do we want to introduce (stale) D1.4
> stuff when currently at D2.3?
Yeah, in our internal tree we support D2.0 and I'll update accordingly.
Do you think it's okay to support D2.0 for now and update to D2.3 if
needed later on? I don't really know how much changed between these
versions...
> > +/* Link adaptation is split between byte #2 and byte #3. It should
> > be set only
> > + * if IEEE80211_HE_MAC_CAP0_HTC_HE in which case the following values
> > apply:
> > + * 0 = No feedback.
> > + * 1 = reserved.
> > + * 2 = Unsolicited feedback.
> > + * 3 = both
> > + */
> > +#define IEEE80211_HE_MAC_CAP1_LINK_ADAPTATION 0x80
>
> This is confusing. I suspect 'byte #2' is HE_MAC_CAP1 and 'byte #3' is
> HE_MAC_CAP2. Just refer to that instead of the byte-number reference.
Right, I'll do it.
> > +/**
> > + * struct ieee80211_sband_iftype_data
> > + *
> > + * This structure encapsulates sband data that is relevant for the
> > interface
> > + * types defined in %types
> > + *
> > + * @types: interface types (bits)
>
> maybe better named @types_mask.
Makes sense.
> > +/**
> > + * ieee80211_get_sband_ift_data - return sband data for a given iftype
> > + * @sband: the sband to search for the STA on
> > + * @iftype: enum nl80211_iftype
> > + *
> > + * Return: pointer to the struct ieee80211_sband_iftype_data, or NULL is
> > none found
> > + */
> > +static inline const struct ieee80211_sband_iftype_data *
> > +ieee80211_get_sband_ift_data(const struct ieee80211_supported_band *sband,
>
> Just call this function ieee80211_get_sband_iftype_data. It's only 3
> additional chars.
Yeah, I bumped into this in one of the internal reviews, but didn't
bother. But you're right, iftype is better.
@@ -781,6 +783,23 @@ int wiphy_register(struct wiphy *wiphy)
> > sband->channels[i].band = band;
> > }
> >
> > + for (i = 0; i < sband->n_iftype_data; i++) {
> > + const struct ieee80211_sband_iftype_data *iftd;
> > +
> > + iftd = &sband->iftype_data[i];
> > +
> > + if (WARN_ON(!iftd->types))
> > + return -EINVAL;
> > + if (WARN_ON(types & iftd->types))
> > + return -EINVAL;
>
> I suspected the types mask was not allowed to overlap for the
> iftype_data entries, but may be worth documenting that in struct
> ieee80211_sband_iftype_data kerneldoc.
Sure, I'll add it.
> > diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> > index f7715b85fd2b..661728dbf989 100644
> > --- a/net/wireless/nl80211.c
> > +++ b/net/wireless/nl80211.c
> >
Re: [RFC 1/3] cfg80211: Add support for HE
On 5/18/2018 4:05 PM, Luca Coelho wrote:
From: Luca Coelho
Add support for the HE in cfg80211 and also add userspace API to
nl80211 to send rate information out, conforming with P802.11ax_D1.4.
A couple of things changed in D2.0 so does it make sense to introduce
stuff from older draft?
Additionally, remove the IEEE80211_MAX_AMPDU_BUF definition from some
realtek drivers in staging because they are now conflicting with the
new definitions and are not used anyway.
Signed-off-by: Liad Kaufman
Signed-off-by: Johannes Berg
Signed-off-by: Ilan Peer
Signed-off-by: Ido Yariv
Signed-off-by: Luca Coelho
---
drivers/staging/rtl8188eu/include/wifi.h | 1 -
drivers/staging/rtl8712/wifi.h | 1 -
drivers/staging/rtl8723bs/include/wifi.h | 1 -
include/linux/ieee80211.h| 431 ++-
include/net/cfg80211.h | 102 +-
include/uapi/linux/nl80211.h | 87 -
net/wireless/core.c | 21 +-
net/wireless/nl80211.c | 99 +-
net/wireless/util.c | 82 +
9 files changed, 813 insertions(+), 12 deletions(-)
[snip]
diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
index 8fe7e4306816..7e1a650be329 100644
--- a/include/linux/ieee80211.h
+++ b/include/linux/ieee80211.h
[snip]
+/**
+ * struct ieee80211_he_mcs_nss_supp - HE Tx/Rx HE MCS NSS Support Field
+ *
+ * This structure holds the data required for the Tx/Rx HE MCS NSS Support
Field
+ * described in P802.11ax_D1.4 section 9.4.2.237.4
+ *
+ * @rx_msc_80: Rx MCS map 2 bits for each stream, total 8 streams, for channel
+ * widths less than 80MHz.
+ * @tx_msc_80: Tx MCS map 2 bits for each stream, total 8 streams, for channel
+ * widths less than 80MHz.
+ * @rx_msc_160: Rx MCS map 2 bits for each stream, total 8 streams, for channel
+ * width 160MHz.
+ * @tx_msc_160: Tx MCS map 2 bits for each stream, total 8 streams, for channel
+ * width 160MHz.
+ * @rx_msc_80p80: Rx MCS map 2 bits for each stream, total 8 streams, for
+ * channel width 80p80MHz.
+ * @tx_msc_80p80: Tx MCS map 2 bits for each stream, total 8 streams, for
+ * channel width 80p80MHz.
+ */
+struct ieee80211_he_mcs_nss_supp {
+ __le16 rx_msc_80;
Should 'msc' in these fields be 'mcs'?
+ __le16 tx_msc_80;
+ __le16 rx_msc_160;
+ __le16 tx_msc_160;
+ __le16 rx_msc_80p80;
+ __le16 tx_msc_80p80;
+} __packed;
+
+/**
+ * struct ieee80211_he_operation - HE capabilities element
+ *
+ * This structure is the "HE operation element" fields as
+ * described in P802.11ax_D1.4 section 9.4.2.238
+ */
+struct ieee80211_he_operation {
+ __le32 he_oper_params;
+ __le16 he_mcs_nss_set;
+ /* Optional 0,1,3 or 4 bytes: depends on %he_oper_params */
+ u8 optional[0];
+} __packed;
If I recall correctly the he operation element changed significantly in
later revisions of the spec. So do we want to introduce (stale) D1.4
stuff when currently at D2.3?
+/**
+ * struct ieee80211_he_mu_edca_param_ac_rec - MU AC Parameter Record field
+ *
+ * This structure is the "MU AC Parameter Record" fields as
+ * described in P802.11ax_D1.4 section 9.4.2.240
+ */
[snip]
@@ -1577,6 +1679,322 @@ struct ieee80211_vht_operation {
#define IEEE80211_VHT_CAP_RX_ANTENNA_PATTERN 0x1000
#define IEEE80211_VHT_CAP_TX_ANTENNA_PATTERN 0x2000
+/* 802.11ax HE MAC capabilities */
+#define IEEE80211_HE_MAC_CAP0_HTC_HE 0x01
[snip]
+
+/* Link adaptation is split between byte #2 and byte #3. It should be set only
+ * if IEEE80211_HE_MAC_CAP0_HTC_HE in which case the following values apply:
+ * 0 = No feedback.
+ * 1 = reserved.
+ * 2 = Unsolicited feedback.
+ * 3 = both
+ */
+#define IEEE80211_HE_MAC_CAP1_LINK_ADAPTATION 0x80
This is confusing. I suspect 'byte #2' is HE_MAC_CAP1 and 'byte #3' is
HE_MAC_CAP2. Just refer to that instead of the byte-number reference.
+#define IEEE80211_HE_MAC_CAP2_LINK_ADAPTATION 0x01
+#define IEEE80211_HE_MAC_CAP2_ALL_ACK 0x02
+#define IEEE80211_HE_MAC_CAP2_UL_MU_RESP_SCHED 0x04
[snip]
+/**
+ * struct ieee80211_sband_iftype_data
+ *
+ * This structure encapsulates sband data that is relevant for the interface
+ * types defined in %types
+ *
+ * @types: interface types (bits)
maybe better named @types_mask.
+ * @he_cap: holds the HE capabilities
+ */
+struct ieee80211_sband_iftype_data {
+ u16 types;
+ struct ieee80211_sta_he_cap he_cap;
+};
+
/**
* struct ieee80211_supported_band - frequency band definition
*
@@ -301,6 +335,8 @@ struct ieee80211_sta_vht_cap {
* @n_bitrates: Number of bitrates in @bitrates
* @ht_cap: HT capabilities in this band
* @vht_cap: VHT capabilities in this band
+ * @n_iftype_data: number of iftype data entries
+ * @iftype_data: interface t
