Re: [PATCH] mac80211: initialize SMPS field in HT capabilities

[Johannes Berg <johannes@sipsolutions.net>]

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 <poh@qca.qualcomm.com>
> 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