mac80211: initialize SMPS field in HT capabilities

Commit Message

Felix Fietkau Jan. 13, 2017, 8:54 a.m. UTC

On 2017-01-13 09:20, Johannes Berg wrote:
> On Wed, 2017-01-11 at 23:33 +0100, Felix Fietkau wrote:
>> ibss and mesh modes copy the ht capabilites from the band without
>> overriding the SMPS state. Unfortunately the default value 0 for the
>> SMPS field means static SMPS instead of disabled.
>> 
>> This results in HT ibss and mesh setups using only single-stream
>> rates,
>> even though SMPS is not supposed to be active.
>> 
>> Initialize SMPS to disabled for all bands on ieee80211_hw_register to
>> ensure that the value is sane where it is not overriden with the real
>> SMPS state.
> 
> Hmm. I guess the only other place affected by it will be scanning?
Right.

>> Reported-by: Elektra Wagenrad <[e-mail]>
>> Signed-off-by: Felix Fietkau <[e-mail]>
>> ---
>>  net/mac80211/main.c | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>> 
>> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
>> index 1822c77f2b1c..c269046aa02b 100644
>> --- a/net/mac80211/main.c
>> +++ b/net/mac80211/main.c
>> @@ -913,10 +913,15 @@ int ieee80211_register_hw(struct ieee80211_hw
>> *hw)
>>  		supp_ht = supp_ht || sband->ht_cap.ht_supported;
>>  		supp_vht = supp_vht || sband->vht_cap.vht_supported;
>>  
>> -		if (sband->ht_cap.ht_supported)
>> -			local->rx_chains =
>> -				max(ieee80211_mcs_to_chains(&sband-
>> >ht_cap.mcs),
>> -				    local->rx_chains);
>> +		if (!sband->ht_cap.ht_supported)
>> +			continue;
>> +
>> +		local->rx_chains =
>> +			max(ieee80211_mcs_to_chains(&sband-
>> >ht_cap.mcs),
>> +			    local->rx_chains);
>> +
>> +		sband->ht_cap.cap |= WLAN_HT_CAP_SM_PS_DISABLED <<
>> +			             IEEE80211_HT_CAP_SM_PS_SHIFT;
> 
> This ... looks fishy. I know it's not, since it sets both bits, but
> still.
> 
> Additionally, ath10k appears to be setting this to
> WLAN_HT_CAP_SM_PS_DYNAMIC already, so apparently it's expecting
> something to happen with that value? Is it really correct then to be
> overwriting it?
Actually, that code seems to leave the value at 
WLAN_HT_CAP_SM_PS_DISABLED, because it sets that first and doesn't mask
out the field before trying to set it to WLAN_HT_CAP_SM_PS_DYNAMIC.
I don't think it even makes sense to set WLAN_HT_CAP_SM_PS_DYNAMIC at
this point, since it's up to mac80211 to deal with the SMPS state.

Either way, WLAN_HT_CAP_SM_PS_STATIC is a really bad default to have at
init time. If you want, I can change the patch to check for that value
before changing it, but I don't really see the point.

Additionally, I found this ath10k commit:


commit e33a99e227e430a788467e5a85dc29f6df16b983
Author: Peter Oh <[e-mail]>
Date:   Thu Dec 31 15:26:20 2015 +0200

    ath10k: set SM power save disabled to default value
    
    Use SMPS disabled as default because FW does not indicate
    any support of SMPS.
    
    This change will help STAs out that don’t support SMPS from
    sticking on 1SS, since they don’t have method to change it
    back to multiple chains.
    
    This change also should not affect power consumption of STAs
    supporting SMPS, because they are capable to switch the mode
    to dynamic or static either at the end of frame sequence or
    by using SMPS action frame.
    
    Signed-off-by: Peter Oh <[e-mail]>
    Signed-off-by: Kalle Valo <[e-mail]>


- Felix

Comments

Johannes Berg Jan. 13, 2017, 10:20 a.m. UTC | #1

On Fri, 2017-01-13 at 09:54 +0100, Felix Fietkau wrote:

> > Additionally, ath10k appears to be setting this to
> > WLAN_HT_CAP_SM_PS_DYNAMIC already, so apparently it's expecting
> > something to happen with that value? Is it really correct then to
> > be overwriting it?
> 
> Actually, that code seems to leave the value at 
> WLAN_HT_CAP_SM_PS_DISABLED, because it sets that first and doesn't
> mask out the field before trying to set it to
> WLAN_HT_CAP_SM_PS_DYNAMIC.

Hah, that's funny.

> I don't think it even makes sense to set WLAN_HT_CAP_SM_PS_DYNAMIC at
> this point, since it's up to mac80211 to deal with the SMPS state.
> 
> Either way, WLAN_HT_CAP_SM_PS_STATIC is a really bad default to have
> at init time. If you want, I can change the patch to check for that
> value before changing it, but I don't really see the point.

No, I think I agree. But please add a comment that OR'ing in the two
bits will not result in it having strange values - it's a bit
unexpected to see this here and then one has to remember (or look up)
the value of DISABLED to understand the code is fine.

> Additionally, I found this ath10k commit:
> 
> 
> commit e33a99e227e430a788467e5a85dc29f6df16b983
> Author: Peter Oh <[e-mail]>
> Date:   Thu Dec 31 15:26:20 2015 +0200
> 
>     ath10k: set SM power save disabled to default value
>     
>     Use SMPS disabled as default because FW does not indicate
>     any support of SMPS.
>     
>     This change will help STAs out that don’t support SMPS from
>     sticking on 1SS, since they don’t have method to change it
>     back to multiple chains.
>     
>     This change also should not affect power consumption of STAs
>     supporting SMPS, because they are capable to switch the mode
>     to dynamic or static either at the end of frame sequence or
>     by using SMPS action frame.

Fun. Though I'd argue that this whole thing should then just be removed
from ath10k.

johannes

Patch

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index b4bdeb07a012..6146a293601a 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -3860,7 +3860,8 @@  static struct ieee80211_sta_ht_cap ath10k_get_ht_cap(struct ath10k *ar)
        ht_cap.ampdu_density = IEEE80211_HT_MPDU_DENSITY_8;
        ht_cap.cap |= IEEE80211_HT_CAP_SUP_WIDTH_20_40;
        ht_cap.cap |= IEEE80211_HT_CAP_DSSSCCK40;
-       ht_cap.cap |= WLAN_HT_CAP_SM_PS_STATIC << IEEE80211_HT_CAP_SM_PS_SHIFT;
+       ht_cap.cap |=
+               WLAN_HT_CAP_SM_PS_DISABLED << IEEE80211_HT_CAP_SM_PS_SHIFT;
 
        if (ar->ht_cap_info & WMI_HT_CAP_HT20_SGI)
                ht_cap.cap |= IEEE80211_HT_CAP_SGI_20;