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@xxxxxxxxxxxxxxxx> > 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
