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
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 <p...@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