2012-09-24 03:57:58

by Mahesh Palivela

[permalink] [raw]
Subject: [PATCH v2] mac80211: VHT peer STA caps

From: Mahesh Palivela <[email protected]>

To save peer STA VHT capabilities in mac80211.

Signed-off-by: Mahesh Palivela <[email protected]>
---
Taken care of Johannes comments in patch v1

include/linux/ieee80211.h | 45 ++++++++++++++++++++++++++++++-------------
include/net/cfg80211.h | 2 +
include/net/mac80211.h | 2 +
net/mac80211/Makefile | 1 +
net/mac80211/cfg.c | 5 ++++
net/mac80211/ieee80211_i.h | 7 ++++++
net/mac80211/main.c | 2 +-
net/mac80211/mlme.c | 9 ++++++-
net/mac80211/util.c | 16 +++++++++++++-
net/mac80211/vht.c | 36 +++++++++++++++++++++++++++++++++++
10 files changed, 106 insertions(+), 19 deletions(-)

diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
index 2385119..8c803f0 100644
--- a/include/linux/ieee80211.h
+++ b/include/linux/ieee80211.h
@@ -1107,20 +1107,6 @@ struct ieee80211_ht_operation {
#define WLAN_HT_SMPS_CONTROL_STATIC 1
#define WLAN_HT_SMPS_CONTROL_DYNAMIC 3

-#define VHT_MCS_SUPPORTED_SET_SIZE 8
-
-struct ieee80211_vht_capabilities {
- __le32 vht_capabilities_info;
- u8 vht_supported_mcs_set[VHT_MCS_SUPPORTED_SET_SIZE];
-} __packed;
-
-struct ieee80211_vht_operation {
- u8 vht_op_info_chwidth;
- u8 vht_op_info_chan_center_freq_seg1_idx;
- u8 vht_op_info_chan_center_freq_seg2_idx;
- __le16 vht_basic_mcs_set;
-} __packed;
-
/**
* struct ieee80211_vht_mcs_info - VHT MCS information
* @rx_mcs_map: RX MCS map 2 bits for each stream, total 8 streams
@@ -1141,6 +1127,37 @@ struct ieee80211_vht_mcs_info {
__le16 tx_highest;
} __packed;

+/**
+ * struct ieee80211_vht_cap - VHT capabilities
+ *
+ * This structure is the "VHT capabilities element" as
+ * described in 802.11ac D3.0 8.4.2.160
+ * @vht_cap_info: VHT capability info
+ * @supp_mcs: VHT MCS supported rates
+ */
+struct ieee80211_vht_cap {
+ __le32 vht_cap_info;
+ struct ieee80211_vht_mcs_info supp_mcs;
+} __packed;
+
+/**
+ * struct ieee80211_vht_operation - VHT operation IE
+ *
+ * This structure is the "VHT operation element" as
+ * described in 802.11ac D3.0 8.4.2.161
+ * @chan_width: Operating channel width
+ * @center_freq_seg1_idx: center freq segment 1 index
+ * @center_freq_seg2_idx: center freq segment 2 index
+ * @basic_mcs_set: VHT Basic MCS rate set
+ */
+struct ieee80211_vht_operation {
+ u8 chan_width;
+ u8 center_freq_seg1_idx;
+ u8 center_freq_seg2_idx;
+ __le16 basic_mcs_set;
+} __packed;
+
+
#define IEEE80211_VHT_MCS_ZERO_TO_SEVEN_SUPPORT 0
#define IEEE80211_VHT_MCS_ZERO_TO_EIGHT_SUPPORT 1
#define IEEE80211_VHT_MCS_ZERO_TO_NINE_SUPPORT 2
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index ab78b53..9cf6327 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -498,6 +498,7 @@ enum station_parameters_apply_mask {
* @plink_action: plink action to take
* @plink_state: set the peer link state for a station
* @ht_capa: HT capabilities of station
+ * @vht_capa: VHT capabilities of station
* @uapsd_queues: bitmap of queues configured for uapsd. same format
* as the AC bitmap in the QoS info field
* @max_sp: max Service Period. same format as the MAX_SP in the
@@ -517,6 +518,7 @@ struct station_parameters {
u8 plink_action;
u8 plink_state;
struct ieee80211_ht_cap *ht_capa;
+ struct ieee80211_vht_cap *vht_capa;
u8 uapsd_queues;
u8 max_sp;
};
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 82558c8..b0369f7 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1076,6 +1076,7 @@ enum ieee80211_sta_state {
* @aid: AID we assigned to the station if we're an AP
* @supp_rates: Bitmap of supported rates (per band)
* @ht_cap: HT capabilities of this STA; restricted to our own TX capabilities
+ * @vht_cap: VHT capabilities of this STA
* @wme: indicates whether the STA supports WME. Only valid during AP-mode.
* @drv_priv: data area for driver use, will always be aligned to
* sizeof(void *), size is determined in hw information.
@@ -1088,6 +1089,7 @@ struct ieee80211_sta {
u8 addr[ETH_ALEN];
u16 aid;
struct ieee80211_sta_ht_cap ht_cap;
+ struct ieee80211_sta_vht_cap vht_cap;
bool wme;
u8 uapsd_queues;
u8 max_sp;
diff --git a/net/mac80211/Makefile b/net/mac80211/Makefile
index a7dd110..4911202 100644
--- a/net/mac80211/Makefile
+++ b/net/mac80211/Makefile
@@ -8,6 +8,7 @@ mac80211-y := \
wpa.o \
scan.o offchannel.o \
ht.o agg-tx.o agg-rx.o \
+ vht.o \
ibss.o \
iface.o \
rate.o \
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 05f3a31..cf63a9e 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1144,6 +1144,11 @@ static int sta_apply_parameters(struct ieee80211_local *local,
params->ht_capa,
&sta->sta.ht_cap);

+ if (params->vht_capa)
+ ieee80211_vht_cap_ie_to_sta_vht_cap(sdata, sband,
+ params->vht_capa,
+ &sta->sta.vht_cap);
+
if (ieee80211_vif_is_mesh(&sdata->vif)) {
#ifdef CONFIG_MAC80211_MESH
if (sdata->u.mesh.security & IEEE80211_MESH_SEC_SECURED)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 8c80455..62037e2 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1133,6 +1133,8 @@ struct ieee802_11_elems {
u8 *wmm_param;
struct ieee80211_ht_cap *ht_cap_elem;
struct ieee80211_ht_operation *ht_operation;
+ struct ieee80211_vht_cap *vht_cap_elem;
+ struct ieee80211_vht_operation *vht_operation;
struct ieee80211_meshconf_ie *mesh_config;
u8 *mesh_id;
u8 *peering;
@@ -1359,6 +1361,11 @@ void ieee80211_ba_session_work(struct work_struct *work);
void ieee80211_tx_ba_session_handle_start(struct sta_info *sta, int tid);
void ieee80211_release_reorder_timeout(struct sta_info *sta, int tid);

+/* VHT */
+void ieee80211_vht_cap_ie_to_sta_vht_cap(struct ieee80211_sub_if_data *sdata,
+ struct ieee80211_supported_band *sband,
+ struct ieee80211_vht_cap *vht_cap_ie,
+ struct ieee80211_sta_vht_cap *vht_cap);
/* Spectrum management */
void ieee80211_process_measurement_req(struct ieee80211_sub_if_data *sdata,
struct ieee80211_mgmt *mgmt,
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index c80c449..0ec170f 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -832,7 +832,7 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)

if (supp_vht)
local->scan_ies_len +=
- 2 + sizeof(struct ieee80211_vht_capabilities);
+ 2 + sizeof(struct ieee80211_vht_cap);

if (!local->ops->hw_scan) {
/* For hw_scan, driver needs to set these up. */
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 2dbd9e1..79b1123 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -343,7 +343,7 @@ static void ieee80211_add_vht_ie(struct ieee80211_sub_if_data *sdata,
cap = vht_cap.cap;

/* reserve and fill IE */
- pos = skb_put(skb, sizeof(struct ieee80211_vht_capabilities) + 2);
+ pos = skb_put(skb, sizeof(struct ieee80211_vht_cap) + 2);
ieee80211_ie_build_vht_cap(pos, &vht_cap, cap);
}

@@ -392,7 +392,7 @@ static void ieee80211_send_assoc(struct ieee80211_sub_if_data *sdata)
4 + /* power capability */
2 + 2 * sband->n_channels + /* supported channels */
2 + sizeof(struct ieee80211_ht_cap) + /* HT */
- 2 + sizeof(struct ieee80211_vht_capabilities) + /* VHT */
+ 2 + sizeof(struct ieee80211_vht_cap) + /* VHT */
assoc_data->ie_len + /* extra IEs */
9, /* WMM */
GFP_KERNEL);
@@ -2100,6 +2100,11 @@ static bool ieee80211_assoc_success(struct ieee80211_sub_if_data *sdata,
sta->supports_40mhz =
sta->sta.ht_cap.cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40;

+ if (elems.vht_cap_elem && !(ifmgd->flags & IEEE80211_STA_DISABLE_VHT))
+ ieee80211_vht_cap_ie_to_sta_vht_cap(sdata, sband,
+ elems.vht_cap_elem,
+ &sta->sta.vht_cap);
+
rate_control_rate_init(sta);

if (ifmgd->flags & IEEE80211_STA_MFP_ENABLED)
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 22ca350..c808906 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -741,6 +741,18 @@ u32 ieee802_11_parse_elems_crc(u8 *start, size_t len,
else
elem_parse_failed = true;
break;
+ case WLAN_EID_VHT_CAPABILITY:
+ if (elen >= sizeof(struct ieee80211_vht_cap))
+ elems->vht_cap_elem = (void *)pos;
+ else
+ elem_parse_failed = true;
+ break;
+ case WLAN_EID_VHT_OPERATION:
+ if (elen >= sizeof(struct ieee80211_vht_operation))
+ elems->vht_operation = (void *)pos;
+ else
+ elem_parse_failed = true;
+ break;
case WLAN_EID_MESH_ID:
elems->mesh_id = pos;
elems->mesh_id_len = elen;
@@ -1788,8 +1800,8 @@ u8 *ieee80211_ie_build_vht_cap(u8 *pos, struct ieee80211_sta_vht_cap *vht_cap,
__le32 tmp;

*pos++ = WLAN_EID_VHT_CAPABILITY;
- *pos++ = sizeof(struct ieee80211_vht_capabilities);
- memset(pos, 0, sizeof(struct ieee80211_vht_capabilities));
+ *pos++ = sizeof(struct ieee80211_vht_cap);
+ memset(pos, 0, sizeof(struct ieee80211_vht_cap));

/* capability flags */
tmp = cpu_to_le32(cap);
diff --git a/net/mac80211/vht.c b/net/mac80211/vht.c
new file mode 100644
index 0000000..df3dd72
--- /dev/null
+++ b/net/mac80211/vht.c
@@ -0,0 +1,36 @@
+/*
+ * VHT handling
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/ieee80211.h>
+#include <linux/export.h>
+#include <net/mac80211.h>
+#include "ieee80211_i.h"
+
+
+void ieee80211_vht_cap_ie_to_sta_vht_cap(struct ieee80211_sub_if_data *sdata,
+ struct ieee80211_supported_band *sband,
+ struct ieee80211_vht_cap *vht_cap_ie,
+ struct ieee80211_sta_vht_cap *vht_cap)
+{
+ if (WARN_ON_ONCE(!vht_cap))
+ return;
+
+ memset(vht_cap, 0, sizeof(*vht_cap));
+
+ if (!vht_cap_ie || !sband->vht_cap.vht_supported)
+ return;
+
+ vht_cap->vht_supported = true;
+
+ vht_cap->cap = le16_to_cpu(vht_cap_ie->vht_cap_info);
+
+ /* Copy peer MCS info, the driver might need them. */
+ memcpy((u8 *)&vht_cap->vht_mcs,
+ (u8 *)&vht_cap_ie->supp_mcs,
+ sizeof(struct ieee80211_vht_mcs_info));
+}


2012-09-24 08:58:40

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH v2] mac80211: VHT peer STA caps

On Mon, 2012-09-24 at 03:57 +0000, Mahesh Palivela wrote:
> From: Mahesh Palivela <[email protected]>
>
> To save peer STA VHT capabilities in mac80211.
>
> Signed-off-by: Mahesh Palivela <[email protected]>
> ---
> Taken care of Johannes comments in patch v1


> +void ieee80211_vht_cap_ie_to_sta_vht_cap(struct ieee80211_sub_if_data *sdata,
> + struct ieee80211_supported_band *sband,
> + struct ieee80211_vht_cap *vht_cap_ie,
> + struct ieee80211_sta_vht_cap *vht_cap)
> +{
> + if (WARN_ON_ONCE(!vht_cap))
> + return;
> +
> + memset(vht_cap, 0, sizeof(*vht_cap));
> +
> + if (!vht_cap_ie || !sband->vht_cap.vht_supported)
> + return;
> +
> + vht_cap->vht_supported = true;
> +
> + vht_cap->cap = le16_to_cpu(vht_cap_ie->vht_cap_info);
> +

Well I think it'd be better to actually restrict/copy/update the feature
bits correctly, this would simplify drivers.

johannes


2012-09-27 06:23:58

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH v2] mac80211: VHT peer STA caps

On Thu, 2012-09-27 at 08:53 +0530, Mahesh Palivela wrote:
> On 09/26/2012 11:23 PM, Johannes Berg wrote:
> >
> > Well struct ieee80211_sta is what the driver uses. That's where it's
> > "telling" the driver, by making the data accessible to the driver. Your
> > new vht_cap field there is also "telling the driver" that way.
> >
> Thanks for the pointer. will revert with code change.

Well, reverting that change would be pointless?

Better just add some text there to the docs.

johannes



2012-09-26 17:22:14

by Mahesh Palivela

[permalink] [raw]
Subject: Re: [PATCH v2] mac80211: VHT peer STA caps

On 9/26/2012 5:23 PM, Johannes Berg wrote:
>> you mean to say some drivers which dont provide its own VHT caps, will
>> use remote STA VHT caps as its own?
>> If so I feel not correct posing remote STA caps as local caps.
>
> No, no. If you look at HT, then you'll see that if the driver has HT but
> isn't capable of doing everything the peer STA can do, then mac80211
> restricts tells the driver

Sorry. Where is it telling the driver? I couldn't find this part of code
in mac80211, could you point me?

> only about the peer capabilities that the
> device can do as well.
>
> I agree that this isn't really necessary and can be rather complex in
> VHT, but I think you need to document the difference very clearly in the
> documentation for the ieee80211_sta VHT caps.

Can this go as different patch? take my v2 patch as framework. More
implementation of vht.c at later point of time?

>
> johannes
>

--
Thanks,
Mahesh

2012-09-27 07:21:29

by Mahesh Palivela

[permalink] [raw]
Subject: Re: [PATCH v2] mac80211: VHT peer STA caps

On 09/27/2012 12:31 PM, Johannes Berg wrote:
>
> I think it's fine if you leave the code as is, but change the comment to
> make the HT/VHT difference clear.

Ok. So we will go with patch v2 with document section clearly mentioning
"vht_cap of remote STA taking as is"

Thanks,
Mahesh

2012-09-26 11:53:07

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH v2] mac80211: VHT peer STA caps

On Wed, 2012-09-26 at 08:45 +0530, Mahesh Palivela wrote:
> On 09/25/2012 07:39 PM, Johannes Berg wrote:
> >
> > Well imagine you have a driver for some hardware that can do
> > beamforming, and the same driver can also work with hardware that
> > doesn't. If we restrict remote caps, like we do for HT, the driver
> > doesn't have to worry about this but can just do whatever the remote
> > caps say is possible. If we don't restrict it, the driver has to also
> > check its own caps.
>
> you mean to say some drivers which dont provide its own VHT caps, will
> use remote STA VHT caps as its own?
> If so I feel not correct posing remote STA caps as local caps.

No, no. If you look at HT, then you'll see that if the driver has HT but
isn't capable of doing everything the peer STA can do, then mac80211
restricts tells the driver only about the peer capabilities that the
device can do as well.

I agree that this isn't really necessary and can be rather complex in
VHT, but I think you need to document the difference very clearly in the
documentation for the ieee80211_sta VHT caps.

johannes


2012-09-25 09:49:40

by Mahesh Palivela

[permalink] [raw]
Subject: Re: [PATCH v2] mac80211: VHT peer STA caps

On 09/25/2012 12:55 PM, Johannes Berg wrote:
>>
>> vht_cap->cap = le16_to_cpu(vht_cap_ie->vht_cap_info) &
>> (sband->ht_cap.cap |
>
> err, vht_cap

Sorry, I meant vht_cap only. Typo..

>
>> ~(IEEE80211_VHT_CAP_RXLDPC |
>> IEEE80211_VHT_CAP_SU_BEAMFORMER_CAPABLE |
>> IEEE80211_VHT_CAP_MU_BEAMFORMER_CAPABLE |
>> IEEE80211_VHT_CAP_HTC_VHT));
>
> I don't think that can be right. It seems to me it should be something
> like
>
> cap = cap_info & sband->vht_cap.cap &
> (MAX_MPDU_LENGTH_... |
> SHORT_GI_... | HTC_VHT | ... ? )
>
> if (cap_info & SU_BEAMFORMEE_CAPABLE &&
> sband->vht_cap.cap & SU_BEAMFORMER_CAPABLE)
> cap |= SU_BEAMFORMEE_CAPABLE;
>
> etc.?
>
> OTOH, this would only matter to drivers that actually support all these
> features, and those drivers could take care of not enabling the features
> if their hardware doesn't support them. However, think of a driver that
> is for different hardware, some that supports beamforming and some that
> doesn't. Then if we mask out the beamforming capability of a station if
> it's not locally supported, that driver could be simpler? Then again, if
> we don't the driver just has to check if the hardware supports it, which
> seems reasonable as well.
>
> Do you understand the issue?
>

why should we mask out remote STA caps bits. Our local hw caps are in
sband->sta.vht_caps. remote caps sta_info->sta.vht_caps. In what way
drivers are connected with remote STA vht caps bits.

> So maybe if you document it right, your original v2 patch is sufficient.
> Convince me :)
>
> johannes
>

2012-09-27 07:00:29

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH v2] mac80211: VHT peer STA caps

On Thu, 2012-09-27 at 12:04 +0530, Mahesh Palivela wrote:
> On 09/27/2012 11:54 AM, Johannes Berg wrote:
> >> Thanks for the pointer. will revert with code change.
> >
> > Well, reverting that change would be pointless?
> >
> > Better just add some text there to the docs.
> >
>
> Sorry wrong usage of word revert. what I meant was get back with proper
> mask bits code change.
> Also update docs section accordingly.

I think it's fine if you leave the code as is, but change the comment to
make the HT/VHT difference clear.

johannes


2012-09-25 05:26:38

by Mahesh Palivela

[permalink] [raw]
Subject: Re: [PATCH v2] mac80211: VHT peer STA caps

On 09/24/2012 02:29 PM, Johannes Berg wrote:
> On Mon, 2012-09-24 at 03:57 +0000, Mahesh Palivela wrote:
>> From: Mahesh Palivela <[email protected]>
>>
>> To save peer STA VHT capabilities in mac80211.
>>
>> Signed-off-by: Mahesh Palivela <[email protected]>
>> ---
>> Taken care of Johannes comments in patch v1

>> + vht_cap->cap = le16_to_cpu(vht_cap_ie->vht_cap_info);
>> +
>
> Well I think it'd be better to actually restrict/copy/update the feature
> bits correctly, this would simplify drivers.

I am not sure on what bits to be masked. wanted to keep it as future item.
Anyways I attempted few bits below. Let me know if they are ok...

vht_cap->cap = le16_to_cpu(vht_cap_ie->vht_cap_info) &
(sband->ht_cap.cap |
~(IEEE80211_VHT_CAP_RXLDPC |
IEEE80211_VHT_CAP_SU_BEAMFORMER_CAPABLE |
IEEE80211_VHT_CAP_MU_BEAMFORMER_CAPABLE |
IEEE80211_VHT_CAP_HTC_VHT));

>
> johannes
>

2012-09-27 03:23:37

by Mahesh Palivela

[permalink] [raw]
Subject: Re: [PATCH v2] mac80211: VHT peer STA caps

On 09/26/2012 11:23 PM, Johannes Berg wrote:
>
> Well struct ieee80211_sta is what the driver uses. That's where it's
> "telling" the driver, by making the data accessible to the driver. Your
> new vht_cap field there is also "telling the driver" that way.
>
Thanks for the pointer. will revert with code change.
>
>
> I don't think this makes sense, since we're talking only about a
> documentation change to your patch. See:
>
> * @ht_cap: HT capabilities of this STA; restricted to our own TX capabilities
> + * @vht_cap: VHT capabilities of this STA
>
> The difference is that you don't say "restricted to own", but I think
> you should explicitly say that it isn't restricted.

Oh, I got it. Thanks Johannes.

Thanks
Mahesh

2012-09-26 17:53:22

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH v2] mac80211: VHT peer STA caps

On Wed, 2012-09-26 at 22:52 +0530, Mahesh Palivela wrote:
> On 9/26/2012 5:23 PM, Johannes Berg wrote:
> >> you mean to say some drivers which dont provide its own VHT caps, will
> >> use remote STA VHT caps as its own?
> >> If so I feel not correct posing remote STA caps as local caps.
> >
> > No, no. If you look at HT, then you'll see that if the driver has HT but
> > isn't capable of doing everything the peer STA can do, then mac80211
> > restricts tells the driver
>
> Sorry. Where is it telling the driver? I couldn't find this part of code
> in mac80211, could you point me?

Well struct ieee80211_sta is what the driver uses. That's where it's
"telling" the driver, by making the data accessible to the driver. Your
new vht_cap field there is also "telling the driver" that way.


> > I agree that this isn't really necessary and can be rather complex in
> > VHT, but I think you need to document the difference very clearly in the
> > documentation for the ieee80211_sta VHT caps.
>
> Can this go as different patch? take my v2 patch as framework. More
> implementation of vht.c at later point of time?

I don't think this makes sense, since we're talking only about a
documentation change to your patch. See:

* @ht_cap: HT capabilities of this STA; restricted to our own TX capabilities
+ * @vht_cap: VHT capabilities of this STA

The difference is that you don't say "restricted to own", but I think
you should explicitly say that it isn't restricted.

johannes


2012-09-25 07:24:26

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH v2] mac80211: VHT peer STA caps

On Tue, 2012-09-25 at 10:56 +0530, Mahesh Palivela wrote:

> > Well I think it'd be better to actually restrict/copy/update the feature
> > bits correctly, this would simplify drivers.
>
> I am not sure on what bits to be masked. wanted to keep it as future item.
> Anyways I attempted few bits below. Let me know if they are ok...
>
> vht_cap->cap = le16_to_cpu(vht_cap_ie->vht_cap_info) &
> (sband->ht_cap.cap |

err, vht_cap

> ~(IEEE80211_VHT_CAP_RXLDPC |
> IEEE80211_VHT_CAP_SU_BEAMFORMER_CAPABLE |
> IEEE80211_VHT_CAP_MU_BEAMFORMER_CAPABLE |
> IEEE80211_VHT_CAP_HTC_VHT));

I don't think that can be right. It seems to me it should be something
like

cap = cap_info & sband->vht_cap.cap &
(MAX_MPDU_LENGTH_... |
SHORT_GI_... | HTC_VHT | ... ? )

if (cap_info & SU_BEAMFORMEE_CAPABLE &&
sband->vht_cap.cap & SU_BEAMFORMER_CAPABLE)
cap |= SU_BEAMFORMEE_CAPABLE;

etc.?

OTOH, this would only matter to drivers that actually support all these
features, and those drivers could take care of not enabling the features
if their hardware doesn't support them. However, think of a driver that
is for different hardware, some that supports beamforming and some that
doesn't. Then if we mask out the beamforming capability of a station if
it's not locally supported, that driver could be simpler? Then again, if
we don't the driver just has to check if the hardware supports it, which
seems reasonable as well.

Do you understand the issue?

So maybe if you document it right, your original v2 patch is sufficient.
Convince me :)

johannes


2012-09-27 06:34:57

by Mahesh Palivela

[permalink] [raw]
Subject: Re: [PATCH v2] mac80211: VHT peer STA caps

On 09/27/2012 11:54 AM, Johannes Berg wrote:
>> Thanks for the pointer. will revert with code change.
>
> Well, reverting that change would be pointless?
>
> Better just add some text there to the docs.
>

Sorry wrong usage of word revert. what I meant was get back with proper
mask bits code change.
Also update docs section accordingly.

2012-09-25 14:08:37

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH v2] mac80211: VHT peer STA caps

On Tue, 2012-09-25 at 15:19 +0530, Mahesh Palivela wrote:

> > OTOH, this would only matter to drivers that actually support all these
> > features, and those drivers could take care of not enabling the features
> > if their hardware doesn't support them. However, think of a driver that
> > is for different hardware, some that supports beamforming and some that
> > doesn't. Then if we mask out the beamforming capability of a station if
> > it's not locally supported, that driver could be simpler? Then again, if
> > we don't the driver just has to check if the hardware supports it, which
> > seems reasonable as well.
> >
> > Do you understand the issue?
> >
>
> why should we mask out remote STA caps bits. Our local hw caps are in
> sband->sta.vht_caps. remote caps sta_info->sta.vht_caps. In what way
> drivers are connected with remote STA vht caps bits.

Well imagine you have a driver for some hardware that can do
beamforming, and the same driver can also work with hardware that
doesn't. If we restrict remote caps, like we do for HT, the driver
doesn't have to worry about this but can just do whatever the remote
caps say is possible. If we don't restrict it, the driver has to also
check its own caps.

We can do this, and I suspect it will be simpler, but it needs to be
*very clearly* documented since VHT will then be different from HT.

johannes


2012-09-26 03:16:00

by Mahesh Palivela

[permalink] [raw]
Subject: Re: [PATCH v2] mac80211: VHT peer STA caps

On 09/25/2012 07:39 PM, Johannes Berg wrote:
>
> Well imagine you have a driver for some hardware that can do
> beamforming, and the same driver can also work with hardware that
> doesn't. If we restrict remote caps, like we do for HT, the driver
> doesn't have to worry about this but can just do whatever the remote
> caps say is possible. If we don't restrict it, the driver has to also
> check its own caps.

you mean to say some drivers which dont provide its own VHT caps, will
use remote STA VHT caps as its own?
If so I feel not correct posing remote STA caps as local caps.

>
> We can do this, and I suspect it will be simpler, but it needs to be
> *very clearly* documented since VHT will then be different from HT.
>
> johannes
>