* mwifiex & cfg80211: Problems with power save mode @ 2020-06-09 11:15 Pali Rohár 2020-07-08 9:35 ` Pali Rohár 0 siblings, 1 reply; 5+ messages in thread From: Pali Rohár @ 2020-06-09 11:15 UTC (permalink / raw) To: Ganapathi Bhat, Amitkumar Karwar, Xinming Hu, Kalle Valo, Johannes Berg, linux-wireless Hello! I'm looking at power save mode provided by mwifiex driver and I see that current implementation is suboptimal and probably default settings quite suspicious for me. If I understand mwifiex driver correctly, it supports power save mode, but its setting is global for all interfaces. mwifiex is fullmac driver and power save mode is controlled by firmware and it is not possible to set it per-bssid. There is just command which affects all bssids. Ganapathi, please correct me if I'm wrong here. On the other hand, current cfg80211 layer supports controlling power save mode by NL80211_CMD_SET_POWER_SAVE and NL80211_CMD_GET_POWER_SAVE commands, which are per-net-interface, not per-wiphy-interface. Moreover NL80211_CMD_GET_POWER_SAVE does not use any driver callback, just use cached value from the last NL80211_CMD_SET_POWER_SAVE call. So kernel driver cannot provide correct state of power save by asking firmware for it. So currently it is not possible to implement power save mode for mwifiex driver correctly when more bssid interfaces for wiphy are used. Current situation is following: If I create two wlan interfaces (wlan0 and wlan1) for mwifiex wiphy and turn power save just only for wlan0 then mwifiex turn power save for both wlan0 and wlan1 (as firmware does not support per-bssid powe save), but cfg80211 for wlan1 still inform that power save is enabled. Do you have any idea how to fix this issue "properly"? So kernel would not lay about power save state of mwifiex interfaces? I guess that some extension or modification would be needed to core wireless cfg80211 code. And I have another question about power save. How should be handled WIPHY_FLAG_PS_ON_BY_DEFAULT flag? I see that kernel by default set it based on CONFIG_CFG80211_DEFAULT_PS compile option. But mwifiex driver always enable it independently of how is CONFIG_CFG80211_DEFAULT_PS was set at compile time. Should mwifiex driver respect CONFIG_CFG80211_DEFAULT_PS compile time option and do not enable WIPHY_FLAG_PS_ON_BY_DEFAULT when CONFIG_CFG80211_DEFAULT_PS is not set to power save? And the last question about power save mode and mwifiex. As power save mode for mwifiex firmware is global for all interfaces, what should be the correct behavior when there is wlan0 interface in managed/sta mode with enabled power save mode and then user add a new wlan1 interface in AP mode? Should driver turn off power save mode automatically (as it AP with enabled power save mode may cause problems) or should power save mode stay enabled (as user explicitly did not turned it off for wlan0)? And what should be the correct behavior when there are two interfaces wlan0 and wlan1, both have power save mode turned off and user try to enable power save mode just for wlan0? Should mwifiex driver turn power save mode and therefore enable it for both wlan0 and wlan1? Or it should not enable power save mode until command is send for both wlan0 and wlan1 interfaces? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: mwifiex & cfg80211: Problems with power save mode 2020-06-09 11:15 mwifiex & cfg80211: Problems with power save mode Pali Rohár @ 2020-07-08 9:35 ` Pali Rohár 2020-07-30 13:33 ` Johannes Berg 0 siblings, 1 reply; 5+ messages in thread From: Pali Rohár @ 2020-07-08 9:35 UTC (permalink / raw) To: Ganapathi Bhat, Amitkumar Karwar, Xinming Hu, Kalle Valo, Johannes Berg, linux-wireless Hello! I would like to remind issue with NL80211_CMD_GET_POWER_SAVE described below. Do you have any idea how to fix / workaround it for nl80211 API? On Tuesday 09 June 2020 13:15:44 Pali Rohár wrote: > Hello! > > I'm looking at power save mode provided by mwifiex driver and I see that > current implementation is suboptimal and probably default settings quite > suspicious for me. > > If I understand mwifiex driver correctly, it supports power save mode, > but its setting is global for all interfaces. mwifiex is fullmac driver > and power save mode is controlled by firmware and it is not possible to > set it per-bssid. There is just command which affects all bssids. > Ganapathi, please correct me if I'm wrong here. > > On the other hand, current cfg80211 layer supports controlling power > save mode by NL80211_CMD_SET_POWER_SAVE and NL80211_CMD_GET_POWER_SAVE > commands, which are per-net-interface, not per-wiphy-interface. > > Moreover NL80211_CMD_GET_POWER_SAVE does not use any driver callback, > just use cached value from the last NL80211_CMD_SET_POWER_SAVE call. So > kernel driver cannot provide correct state of power save by asking > firmware for it. > > So currently it is not possible to implement power save mode for mwifiex > driver correctly when more bssid interfaces for wiphy are used. > > Current situation is following: If I create two wlan interfaces (wlan0 > and wlan1) for mwifiex wiphy and turn power save just only for wlan0 > then mwifiex turn power save for both wlan0 and wlan1 (as firmware does > not support per-bssid powe save), but cfg80211 for wlan1 still inform > that power save is enabled. > > Do you have any idea how to fix this issue "properly"? So kernel would > not lay about power save state of mwifiex interfaces? I guess that some > extension or modification would be needed to core wireless cfg80211 > code. > > > And I have another question about power save. How should be handled > WIPHY_FLAG_PS_ON_BY_DEFAULT flag? I see that kernel by default set it > based on CONFIG_CFG80211_DEFAULT_PS compile option. But mwifiex driver > always enable it independently of how is CONFIG_CFG80211_DEFAULT_PS was > set at compile time. > > Should mwifiex driver respect CONFIG_CFG80211_DEFAULT_PS compile time > option and do not enable WIPHY_FLAG_PS_ON_BY_DEFAULT when > CONFIG_CFG80211_DEFAULT_PS is not set to power save? > > > And the last question about power save mode and mwifiex. As power save > mode for mwifiex firmware is global for all interfaces, what should be > the correct behavior when there is wlan0 interface in managed/sta mode > with enabled power save mode and then user add a new wlan1 interface in > AP mode? Should driver turn off power save mode automatically (as it AP > with enabled power save mode may cause problems) or should power save > mode stay enabled (as user explicitly did not turned it off for wlan0)? > > And what should be the correct behavior when there are two interfaces > wlan0 and wlan1, both have power save mode turned off and user try to > enable power save mode just for wlan0? Should mwifiex driver turn power > save mode and therefore enable it for both wlan0 and wlan1? Or it should > not enable power save mode until command is send for both wlan0 and > wlan1 interfaces? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: mwifiex & cfg80211: Problems with power save mode 2020-07-08 9:35 ` Pali Rohár @ 2020-07-30 13:33 ` Johannes Berg 2020-08-03 14:30 ` Pali Rohár 0 siblings, 1 reply; 5+ messages in thread From: Johannes Berg @ 2020-07-30 13:33 UTC (permalink / raw) To: Pali Rohár, Ganapathi Bhat, Amitkumar Karwar, Xinming Hu, Kalle Valo, linux-wireless Hi, Sorry about the long delay! > > I'm looking at power save mode provided by mwifiex driver and I see that > > current implementation is suboptimal and probably default settings quite > > suspicious for me. > > > > If I understand mwifiex driver correctly, it supports power save mode, > > but its setting is global for all interfaces. mwifiex is fullmac driver > > and power save mode is controlled by firmware and it is not possible to > > set it per-bssid. There is just command which affects all bssids. > > Ganapathi, please correct me if I'm wrong here. > > > > On the other hand, current cfg80211 layer supports controlling power > > save mode by NL80211_CMD_SET_POWER_SAVE and NL80211_CMD_GET_POWER_SAVE > > commands, which are per-net-interface, not per-wiphy-interface. > > > > Moreover NL80211_CMD_GET_POWER_SAVE does not use any driver callback, > > just use cached value from the last NL80211_CMD_SET_POWER_SAVE call. So > > kernel driver cannot provide correct state of power save by asking > > firmware for it. > > > > So currently it is not possible to implement power save mode for mwifiex > > driver correctly when more bssid interfaces for wiphy are used. > > > > Current situation is following: If I create two wlan interfaces (wlan0 > > and wlan1) for mwifiex wiphy and turn power save just only for wlan0 > > then mwifiex turn power save for both wlan0 and wlan1 (as firmware does > > not support per-bssid powe save), but cfg80211 for wlan1 still inform > > that power save is enabled. > > > > Do you have any idea how to fix this issue "properly"? So kernel would > > not lay about power save state of mwifiex interfaces? I guess that some > > extension or modification would be needed to core wireless cfg80211 > > code. I'm not sure I'd consider it "lying"? After all, powersave is sort of opportunistic anyway, and turned off when there's "enough" traffic, etc. But if you wanted to, I think you could just iterate all your interfaces and change wdev->ps for each one accordingly. It's a bit of a hack, but I'm not convinced that adding new API to nl80211 for this is worthwhile. That'd require disallowing the per-interface API, adding new API that is global, etc. So IMHO easier and good enough to just set wdev->ps for all of them whenever one of them is changed? > > And I have another question about power save. How should be handled > > WIPHY_FLAG_PS_ON_BY_DEFAULT flag? I see that kernel by default set it > > based on CONFIG_CFG80211_DEFAULT_PS compile option. But mwifiex driver > > always enable it independently of how is CONFIG_CFG80211_DEFAULT_PS was > > set at compile time. > > > > Should mwifiex driver respect CONFIG_CFG80211_DEFAULT_PS compile time > > option and do not enable WIPHY_FLAG_PS_ON_BY_DEFAULT when > > CONFIG_CFG80211_DEFAULT_PS is not set to power save? Uh, not sure really, sorry. I don't think it matters much? I think we should probably just remove the CFG80211_DEFAULT_PS config entirely. A lot of drivers won't really honour it, I suspect, and the runtime setting is trivially available. > > And the last question about power save mode and mwifiex. As power save > > mode for mwifiex firmware is global for all interfaces, what should be > > the correct behavior when there is wlan0 interface in managed/sta mode > > with enabled power save mode and then user add a new wlan1 interface in > > AP mode? Should driver turn off power save mode automatically (as it AP > > with enabled power save mode may cause problems) or should power save > > mode stay enabled (as user explicitly did not turned it off for wlan0)? That's up to the driver I guess, but I'd consider turning it off. Btw if you keep "wdev->ps" then you can undo this when the AP interface goes away, or in general when the list of interfaces changes. > > And what should be the correct behavior when there are two interfaces > > wlan0 and wlan1, both have power save mode turned off and user try to > > enable power save mode just for wlan0? Should mwifiex driver turn power > > save mode and therefore enable it for both wlan0 and wlan1? Or it should > > not enable power save mode until command is send for both wlan0 and > > wlan1 interfaces? Yeah, no idea :) I'd probably do something like this * keep wdev->ps as it is, so "lie" to userspace about it in some cases * disable powersave for AP interfaces completely (probably not even allowing it enabled); keep it for P2P_GO if supported though * whenever any interfaces come and go or any interface's state changes, recalculate the overall powersave state of the device and apply that johannes ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: mwifiex & cfg80211: Problems with power save mode 2020-07-30 13:33 ` Johannes Berg @ 2020-08-03 14:30 ` Pali Rohár 2020-09-17 10:08 ` Johannes Berg 0 siblings, 1 reply; 5+ messages in thread From: Pali Rohár @ 2020-08-03 14:30 UTC (permalink / raw) To: Johannes Berg Cc: Ganapathi Bhat, Amitkumar Karwar, Xinming Hu, Kalle Valo, linux-wireless Hello! On Thursday 30 July 2020 15:33:44 Johannes Berg wrote: > Hi, > > Sorry about the long delay! > > > > I'm looking at power save mode provided by mwifiex driver and I see that > > > current implementation is suboptimal and probably default settings quite > > > suspicious for me. > > > > > > If I understand mwifiex driver correctly, it supports power save mode, > > > but its setting is global for all interfaces. mwifiex is fullmac driver > > > and power save mode is controlled by firmware and it is not possible to > > > set it per-bssid. There is just command which affects all bssids. > > > Ganapathi, please correct me if I'm wrong here. > > > > > > On the other hand, current cfg80211 layer supports controlling power > > > save mode by NL80211_CMD_SET_POWER_SAVE and NL80211_CMD_GET_POWER_SAVE > > > commands, which are per-net-interface, not per-wiphy-interface. > > > > > > Moreover NL80211_CMD_GET_POWER_SAVE does not use any driver callback, > > > just use cached value from the last NL80211_CMD_SET_POWER_SAVE call. So > > > kernel driver cannot provide correct state of power save by asking > > > firmware for it. > > > > > > So currently it is not possible to implement power save mode for mwifiex > > > driver correctly when more bssid interfaces for wiphy are used. > > > > > > Current situation is following: If I create two wlan interfaces (wlan0 > > > and wlan1) for mwifiex wiphy and turn power save just only for wlan0 > > > then mwifiex turn power save for both wlan0 and wlan1 (as firmware does > > > not support per-bssid powe save), but cfg80211 for wlan1 still inform > > > that power save is enabled. > > > > > > Do you have any idea how to fix this issue "properly"? So kernel would > > > not lay about power save state of mwifiex interfaces? I guess that some > > > extension or modification would be needed to core wireless cfg80211 > > > code. > > I'm not sure I'd consider it "lying"? After all, powersave is sort of > opportunistic anyway, and turned off when there's "enough" traffic, etc. More people with 8997 chip are seeing instability issues and it looks like that some are related to power save mode. So I would like to know if power save is disabled. But because of above problem, I cannot use standard "iw dev wlan0 get power_save" as it may return information that power save is enabled when it is disabled. So for me this is "lying", not providing me correct information. There is bugzilla ticket about instability issues: https://bugzilla.kernel.org/show_bug.cgi?id=109681 > But if you wanted to, I think you could just iterate all your interfaces > and change > > wdev->ps > > for each one accordingly. It's a bit of a hack, but I'm not convinced > that adding new API to nl80211 for this is worthwhile. That'd require > disallowing the per-interface API, adding new API that is global, etc. > > So IMHO easier and good enough to just set wdev->ps for all of them > whenever one of them is changed? Well, I agree this is a hack. But if you as maintainer says that this is how drivers with global power save state should implement nl80211 power save API, I have no problem with it. Basically with current API I was not able to fix/implement it properly and that is why I wrote previous email: to ask how to deal with it. > > > And I have another question about power save. How should be handled > > > WIPHY_FLAG_PS_ON_BY_DEFAULT flag? I see that kernel by default set it > > > based on CONFIG_CFG80211_DEFAULT_PS compile option. But mwifiex driver > > > always enable it independently of how is CONFIG_CFG80211_DEFAULT_PS was > > > set at compile time. > > > > > > Should mwifiex driver respect CONFIG_CFG80211_DEFAULT_PS compile time > > > option and do not enable WIPHY_FLAG_PS_ON_BY_DEFAULT when > > > CONFIG_CFG80211_DEFAULT_PS is not set to power save? > > Uh, not sure really, sorry. > > I don't think it matters much? This is something for consistency. If driver API provides some option I would expect that drivers would respect / implement it in the same way. And if some option are not respected then it is probably good idea to remove them? (Or fixing them) > I think we should probably just remove the CFG80211_DEFAULT_PS config > entirely. A lot of drivers won't really honour it, I suspect, and the > runtime setting is trivially available. Of course userspace can set power save mode after creating interface. If I'm looking at the code correctly, CONFIG_CFG80211_DEFAULT_PS controls WIPHY_FLAG_PS_ON_BY_DEFAULT flag which directly sets wdev->ps state during NETDEV_REGISTER event. So checking if drivers really honor it, is not easy. As it is needed to check if wdev->ps is used during register. And not only option CONFIG_CFG80211_DEFAULT_PS or flag WIPHY_FLAG_PS_ON_BY_DEFAULT. > > > And the last question about power save mode and mwifiex. As power save > > > mode for mwifiex firmware is global for all interfaces, what should be > > > the correct behavior when there is wlan0 interface in managed/sta mode > > > with enabled power save mode and then user add a new wlan1 interface in > > > AP mode? Should driver turn off power save mode automatically (as it AP > > > with enabled power save mode may cause problems) or should power save > > > mode stay enabled (as user explicitly did not turned it off for wlan0)? > > That's up to the driver I guess, but I'd consider turning it off. In driver we can implement anything. So I think some consensus or decision should be done. As I do not think that it make sense what one driver would behave difference as other drivers. Such thing should prevent users surprise that one card behave differently as other card. Just because one driver enable power save by default other not. > Btw if you keep "wdev->ps" then you can undo this when the AP interface > goes away, or in general when the list of interfaces changes. > > > > > And what should be the correct behavior when there are two interfaces > > > wlan0 and wlan1, both have power save mode turned off and user try to > > > enable power save mode just for wlan0? Should mwifiex driver turn power > > > save mode and therefore enable it for both wlan0 and wlan1? Or it should > > > not enable power save mode until command is send for both wlan0 and > > > wlan1 interfaces? > > Yeah, no idea :) > > I'd probably do something like this > > * keep wdev->ps as it is, so "lie" to userspace about it in some cases > * disable powersave for AP interfaces completely (probably not even > allowing it enabled); keep it for P2P_GO if supported though > * whenever any interfaces come and go or any interface's state changes, > recalculate the overall powersave state of the device and apply that Personally I see a problem with first point that wdev->ps would not contain current state of power save. I really would like to see some meaningful output from "iw dev wlan0 get power_save". If kernel would "lie" then there is no point to use "get power_save" command as it just return bogus value. What I need to know is: "Is mwifixex power save mode enabled or not?" If "get power_save" command cannot provide correct answer then I need some other way how to retrieve this kind of information. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: mwifiex & cfg80211: Problems with power save mode 2020-08-03 14:30 ` Pali Rohár @ 2020-09-17 10:08 ` Johannes Berg 0 siblings, 0 replies; 5+ messages in thread From: Johannes Berg @ 2020-09-17 10:08 UTC (permalink / raw) To: Pali Rohár Cc: Ganapathi Bhat, Amitkumar Karwar, Xinming Hu, Kalle Valo, linux-wireless Hi Pali, Again, sorry ... > > But if you wanted to, I think you could just iterate all your interfaces > > and change > > > > wdev->ps > > > > for each one accordingly. It's a bit of a hack, but I'm not convinced > > that adding new API to nl80211 for this is worthwhile. That'd require > > disallowing the per-interface API, adding new API that is global, etc. > > > > So IMHO easier and good enough to just set wdev->ps for all of them > > whenever one of them is changed? > > Well, I agree this is a hack. But if you as maintainer says that this is > how drivers with global power save state should implement nl80211 power > save API, I have no problem with it. > > Basically with current API I was not able to fix/implement it properly > and that is why I wrote previous email: to ask how to deal with it. Right. And I don't really see a good way to fix the API either, since it goes all the way to userspace that way. Even back in iwconfig ... So the best thing we could do is add new global API like I described, and then we'd probably somehow need to support the old API for older userspace (otherwise all your users must upgrade to be able to fix this!) and then ... I just don't think it's worth it, hence the suggestion of just propagating it across all interfaces and effectively limiting it to a global state in the driver that way. Even in cfg80211 we'd have to do that if we implemented a global state there! But I don't see it worthwhile much there ... > This is something for consistency. If driver API provides some option I > would expect that drivers would respect / implement it in the same way. > > And if some option are not respected then it is probably good idea to > remove them? (Or fixing them) There's very little consistency in this area to start with ... > > I think we should probably just remove the CFG80211_DEFAULT_PS config > > entirely. A lot of drivers won't really honour it, I suspect, and the > > runtime setting is trivially available. > > Of course userspace can set power save mode after creating interface. > > If I'm looking at the code correctly, CONFIG_CFG80211_DEFAULT_PS > controls WIPHY_FLAG_PS_ON_BY_DEFAULT flag which directly sets wdev->ps > state during NETDEV_REGISTER event. > > So checking if drivers really honor it, is not easy. As it is needed to > check if wdev->ps is used during register. And not only option > CONFIG_CFG80211_DEFAULT_PS or flag WIPHY_FLAG_PS_ON_BY_DEFAULT. Yeah. I doubt most drivers will honor it. > In driver we can implement anything. So I think some consensus or > decision should be done. As I do not think that it make sense what one > driver would behave difference as other drivers. Such thing should > prevent users surprise that one card behave differently as other card. > Just because one driver enable power save by default other not. Yeah, but if you look at things ... very little consistency expected in this area today. It's a legacy interface coming from wireless extensions though, so that's not very unexpected. > > * keep wdev->ps as it is, so "lie" to userspace about it in some cases > > * disable powersave for AP interfaces completely (probably not even > > allowing it enabled); keep it for P2P_GO if supported though > > * whenever any interfaces come and go or any interface's state changes, > > recalculate the overall powersave state of the device and apply that > > Personally I see a problem with first point that wdev->ps would not > contain current state of power save. I really would like to see some > meaningful output from "iw dev wlan0 get power_save". If kernel would > "lie" then there is no point to use "get power_save" command as it just > return bogus value. > > What I need to know is: "Is mwifixex power save mode enabled or not?" > If "get power_save" command cannot provide correct answer then I need > some other way how to retrieve this kind of information. OK :) So then I'd probably recommend to implement propagation to all netdevs you have. In the most common cases that'll probably just be a single one anyway. johannes ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-09-17 10:08 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-09 11:15 mwifiex & cfg80211: Problems with power save mode Pali Rohár 2020-07-08 9:35 ` Pali Rohár 2020-07-30 13:33 ` Johannes Berg 2020-08-03 14:30 ` Pali Rohár 2020-09-17 10:08 ` Johannes Berg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).