[PATCH v3 3/3] mac80211: mesh: fixed HT ies in beacon template

[PATCH v3 3/3] mac80211: mesh: fixed HT ies in beacon template

[Yaniv Machani]

The HT capab info field inside the HT capab IE of the mesh beacon
is incorrect (in the case of 20MHz channel width).
To fix this driver will check configuration from cfg and
will build it accordingly.

Signed-off-by: Meirav Kama <meiravk@ti.com>
Signed-off-by: Yaniv Machani <yanivma@ti.com>
---
V3 - Fixes redundant spaces,empty lines and added FALLTHROUGH note.

 net/mac80211/mesh.c | 33 ++++++++++++++++++++++++++++++++-
 net/mac80211/util.c |  3 ---
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index 9214bc1..6a67049 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -422,6 +422,7 @@ int mesh_add_ht_cap_ie(struct ieee80211_sub_if_data *sdata,
 	enum nl80211_band band = ieee80211_get_sdata_band(sdata);
 	struct ieee80211_supported_band *sband;
 	u8 *pos;
+	u16 cap;
 
 	sband = local->hw.wiphy->bands[band];
 	if (!sband->ht_cap.ht_supported ||
@@ -430,11 +431,41 @@ int mesh_add_ht_cap_ie(struct ieee80211_sub_if_data *sdata,
 	    sdata->vif.bss_conf.chandef.width == NL80211_CHAN_WIDTH_10)
 		return 0;
 
+	/* determine capability flags */
+	cap = sband->ht_cap.cap;
+
+	/* if channel width is 20MHz - configure HT capab accordingly*/
+	if (sdata->vif.bss_conf.chandef.width == NL80211_CHAN_WIDTH_20) {
+		cap &= ~IEEE80211_HT_CAP_SUP_WIDTH_20_40;
+		cap &= ~IEEE80211_HT_CAP_DSSSCCK40;
+	}
+
+	/* set SM PS mode properly */
+	cap &= ~IEEE80211_HT_CAP_SM_PS;
+	switch (sdata->smps_mode) {
+	case IEEE80211_SMPS_AUTOMATIC:
+	case IEEE80211_SMPS_NUM_MODES:
+		WARN_ON(1);
+		/* FALLTHROUGH */
+	case IEEE80211_SMPS_OFF:
+		cap |= WLAN_HT_CAP_SM_PS_DISABLED <<
+			IEEE80211_HT_CAP_SM_PS_SHIFT;
+		break;
+	case IEEE80211_SMPS_STATIC:
+		cap |= WLAN_HT_CAP_SM_PS_STATIC <<
+			IEEE80211_HT_CAP_SM_PS_SHIFT;
+		break;
+	case IEEE80211_SMPS_DYNAMIC:
+		cap |= WLAN_HT_CAP_SM_PS_DYNAMIC <<
+			IEEE80211_HT_CAP_SM_PS_SHIFT;
+		break;
+	}
+
 	if (skb_tailroom(skb) < 2 + sizeof(struct ieee80211_ht_cap))
 		return -ENOMEM;
 
 	pos = skb_put(skb, 2 + sizeof(struct ieee80211_ht_cap));
-	ieee80211_ie_build_ht_cap(pos, &sband->ht_cap, sband->ht_cap.cap);
+	ieee80211_ie_build_ht_cap(pos, &sband->ht_cap, cap);
 
 	return 0;
 }
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 42bf0b6..5375a82 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -2349,10 +2349,7 @@ u8 *ieee80211_ie_build_ht_oper(u8 *pos, struct ieee80211_sta_ht_cap *ht_cap,
 	ht_oper->operation_mode = cpu_to_le16(prot_mode);
 	ht_oper->stbc_param = 0x0000;
 
-	/* It seems that Basic MCS set and Supported MCS set
-	   are identical for the first 10 bytes */
 	memset(&ht_oper->basic_set, 0, 16);
-	memcpy(&ht_oper->basic_set, &ht_cap->mcs, 10);
 
 	return pos + sizeof(struct ieee80211_ht_operation);
 }
-- 
2.9.0

Re: [PATCH v3 3/3] mac80211: mesh: fixed HT ies in beacon template

[Masashi Honma]

On 2016年07月14日 05:07, Yaniv Machani wrote:
> +
> +	/* if channel width is 20MHz - configure HT capab accordingly*/
> +	if (sdata->vif.bss_conf.chandef.width == NL80211_CHAN_WIDTH_20) {
> +		cap &= ~IEEE80211_HT_CAP_SUP_WIDTH_20_40;
> +		cap &= ~IEEE80211_HT_CAP_DSSSCCK40;
> +	}

I have tested this part of your patch and this works for me.

Previouly, "Supported Channel Width Set bit" in HT Capabilities element
was 1 even though disable_ht40=1 existed in wpa_supplicant.conf.
After appllication of patch, the bit was 0.

Re: [PATCH v3 3/3] mac80211: mesh: fixed HT ies in beacon template

[Masashi Honma]

On 2016年07月22日 14:26, Masashi Honma wrote:
 > On 2016年07月14日 05:07, Yaniv Machani wrote:
 >> +
 >> +    /* if channel width is 20MHz - configure HT capab accordingly*/
 >> +    if (sdata->vif.bss_conf.chandef.width == NL80211_CHAN_WIDTH_20) {
 >> +        cap &= ~IEEE80211_HT_CAP_SUP_WIDTH_20_40;
 >> +        cap &= ~IEEE80211_HT_CAP_DSSSCCK40;
 >> +    }
 >
 > I have tested this part of your patch and this works for me.
 >
 > Previouly, "Supported Channel Width Set bit" in HT Capabilities element
 > was 1 even though disable_ht40=1 existed in wpa_supplicant.conf.
 > After appllication of patch, the bit was 0.
 >
 >

# I retransmit this because of mail delivery errors.

I forgot to mention I have used this patch to test.
http://lists.infradead.org/pipermail/hostap/2016-July/036029.html

Re: [PATCH v3 3/3] mac80211: mesh: fixed HT ies in beacon template

[Johannes Berg]

On Fri, 2016-07-22 at 14:26 +0900, Masashi Honma wrote:
> On 2016年07月14日 05:07, Yaniv Machani wrote:
> > +
> > +	/* if channel width is 20MHz - configure HT capab
> > accordingly*/
> > +	if (sdata->vif.bss_conf.chandef.width ==
> > NL80211_CHAN_WIDTH_20) {
> > +		cap &= ~IEEE80211_HT_CAP_SUP_WIDTH_20_40;
> > +		cap &= ~IEEE80211_HT_CAP_DSSSCCK40;
> > +	}
> 
> I have tested this part of your patch and this works for me.
> 
> Previouly, "Supported Channel Width Set bit" in HT Capabilities
> element was 1 even though disable_ht40=1 existed in
> wpa_supplicant.conf. After appllication of patch, the bit was 0.
> 

But why is that behaviour *correct*? We still support 40 MHz bandwidth
things, we just don't use them if we disable HT40.

johannes

Re: [PATCH v3 3/3] mac80211: mesh: fixed HT ies in beacon template

[Masashi Honma]

On 2016年08月01日 19:03, Johannes Berg wrote:
>
> But why is that behaviour *correct*? We still support 40 MHz bandwidth
> things, we just don't use them if we disable HT40.

I could not fully understand your concern...

Do you mean we have 2 bugs about disabling HT40 ?

1) bits in HT capabilities IE
2) HT40 still enabled even if it was disabled by wpa_supplicant or 
hostapd with disable_ht40

And do you mean 1) and 2) should be fixed at one time ?
Indeed, currently on the view point of opposite peer, HT40 was enabled 
even though it was disabled because both 1) and 2) are wrong.
But if only 1) was fixed, this causes unmatch.

Now I do not recognize bug 2).
Do you have any information about 2) ?

Re: [PATCH v3 3/3] mac80211: mesh: fixed HT ies in beacon template

[Masashi Honma]

> On 2016年08月01日 19:03, Johannes Berg wrote:
>>
>> But why is that behaviour *correct*? We still support 40 MHz bandwidth
>> things, we just don't use them if we disable HT40.

Or do you mean difference between "hardware capability" and "software 
capability" ?
Do you think IEEE80211_HT_CAP_SUP_WIDTH_20_40 bit should be 1 if the 
hardware capable of HT40 even though HT40 is disabled by 
wpa_supplicant/hostapd ?

I have tested with hostapd. I compared these 2 configfiles.

hostapd0.conf
	ht_capab=[HT40-]
hostapd1.conf
	#ht_capab=[HT40-]

The IEEE80211_HT_CAP_SUP_WIDTH_20_40 bit in beacon was below.

hostapd0.conf
	IEEE80211_HT_CAP_SUP_WIDTH_20_40 = 1
hostapd1.conf
	IEEE80211_HT_CAP_SUP_WIDTH_20_40 = 0

So I think the bit should be zero if disabled also for mesh peer.

Masashi Honma.

Re: [PATCH v3 3/3] mac80211: mesh: fixed HT ies in beacon template

[Johannes Berg]

On Tue, 2016-08-02 at 11:59 +0900, Masashi Honma wrote:
> > 
> > On 2016年08月01日 19:03, Johannes Berg wrote:
> > > 
> > > But why is that behaviour *correct*? We still support 40 MHz
> > > bandwidth
> > > things, we just don't use them if we disable HT40.
> 
> Or do you mean difference between "hardware capability" and "software
> capability" ?
> Do you think IEEE80211_HT_CAP_SUP_WIDTH_20_40 bit should be 1 if the 
> hardware capable of HT40 even though HT40 is disabled by 
> wpa_supplicant/hostapd ?

I basically think that the CAP_SUP_WIDTH_20_40 bit shouldn't matter at
all, so it's not clear to me why there's so much talk about it.

After all, if 40 MHz isn't actually *used* as indicated by the HT
operation (rather than HT capability) IE, then the fact that the device
may or may not support 40 MHz is pretty much irrelevant.

> I have tested with hostapd. I compared these 2 configfiles.
> 
> hostapd0.conf
> 	ht_capab=[HT40-]
> hostapd1.conf
> 	#ht_capab=[HT40-]
> 

This explicitly configures *HT capability* though - that's even the
name of the parameter. If you enable HT40 in the capability, the
resulting BSS might still not actually *use* 40 MHz bandwidth, as
required by overlapping BSS detection.

In this patch, they're taking one thing (current HT channel width
configuration) and applying it to another thing (HT capability), and
then even selling it as a bugfix - which I simply cannot understand.
The HT capability shouldn't matter at all, if HT operation is correct.

johannes

Re: [PATCH v3 3/3] mac80211: mesh: fixed HT ies in beacon template

[Masashi Honma]

On 2016年08月02日 16:27, Johannes Berg wrote:
> This explicitly configures *HT capability* though - that's even the
> name of the parameter. If you enable HT40 in the capability, the
> resulting BSS might still not actually *use* 40 MHz bandwidth, as
> required by overlapping BSS detection.

OK, I see.

HT Capabilities element = Defined by hardware and software spec of the 
node. So it does not be modified after boot.

HT Operation element = Defined by surrounding environment and 
configuration of the node. So it could be modified after boot.

So, if the node supports HT40, HT Capabilities shows HT40 is capable.
Now, I understand why you rejected this patch.

But now, when disable_ht=1, no HT Capabilities element in beacon even 
though the node supports HT.

My trailing patch could solve the issue.

Masashi Honma.

Re: [PATCH v3 3/3] mac80211: mesh: fixed HT ies in beacon template

[Johannes Berg]

On Wed, 2016-08-03 at 11:51 +0900, Masashi Honma wrote:
> On 2016年08月02日 16:27, Johannes Berg wrote:
> > This explicitly configures *HT capability* though - that's even the
> > name of the parameter. If you enable HT40 in the capability, the
> > resulting BSS might still not actually *use* 40 MHz bandwidth, as
> > required by overlapping BSS detection.
> 
> OK, I see.
> 
> HT Capabilities element = Defined by hardware and software spec of
> the node. So it does not be modified after boot.

It shouldn't really need to be modified, but perhaps for
interoperability reasons one might want to, like for example we do in
assoc request (we restrict our own capabilities to what the AP
supports, because some APs are stupid.)

That said, I'm basically only objecting to calling this a bugfix. If
the behaviour of restricting the information is desired, I see no real
problem with that, I just don't see how it could possibly be a bugfix.

> HT Operation element = Defined by surrounding environment and 
> configuration of the node. So it could be modified after boot.
> 
> So, if the node supports HT40, HT Capabilities shows HT40 is capable.
> Now, I understand why you rejected this patch.
> 
> But now, when disable_ht=1, no HT Capabilities element in beacon even
> though the node supports HT.
> 
> My trailing patch could solve the issue.

Actually, *this* one I'm not sure is correct. If you want to disable HT
completely, then HT operation can't actually indicate that, and having
HT capabilities without HT operation would likely just confuse peers,
so I think in this case it's quite possibly necessary to remove HT
capabilities.

johannes