Linux-Wireless Archive on lore.kernel.org
 help / color / Atom feed
* 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; 4+ 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] 4+ 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; 4+ 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] 4+ 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; 4+ 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] 4+ 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
  0 siblings, 0 replies; 4+ 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] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ 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

Linux-Wireless Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-wireless/0 linux-wireless/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-wireless linux-wireless/ https://lore.kernel.org/linux-wireless \
		linux-wireless@vger.kernel.org
	public-inbox-index linux-wireless

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-wireless


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git