[PATCH V3] mac80211: wowlan: suspend: Fix power save state stuck in ACTIVE caused by race between suspend and Tx packet.

[PATCH V3] mac80211: wowlan: suspend: Fix power save state stuck in ACTIVE caused by race between suspend and Tx packet.

[Chaitanya T K]

From: Chaitanya T K <chaitanya.mgit@gmail.com>

Background: When wowlan is enabled, the chipset stays in low
power state maintaining the connection as per the
mac80211 power save state.

If suspended during TX in progress, there can be 
race where the driver is put of of power-save due
to TX and during suspend dynamic_ps_time is cancelled
and TX packet is flushed, leaving the driver in ACTIVE
even after resuming until dynamic_ps_time puts 
driver back in DOZE. (Which only happens if there
is another TX).

This can lead to high power consumption of chipset 
during (or) after resuming.

Signed-off-by: Chaitanya T K <chaitanya.mgit@gmail.com>
---
V2: Updated Comment and Commit log.
---
 net/mac80211/pm.c |   16 +++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/net/mac80211/pm.c b/net/mac80211/pm.c
index ac6ad62..8149a3d 100644
--- a/net/mac80211/pm.c
+++ b/net/mac80211/pm.c
@@ -76,6 +76,22 @@ int __ieee80211_suspend(struct ieee80211_hw *hw, struct cfg80211_wowlan *wowlan)
 			if (sdata->vif.type != NL80211_IFTYPE_STATION)
 				continue;
 			ieee80211_mgd_quiesce(sdata);
+			/* If suspended during TX in progress, and wowlan
+			 * is enabled (connection will be active) there
+			 * can be a race where the driver is put out
+			 * of power-save due to TX and during suspend
+			 * dynamic_ps_timer is cancelled and TX packet
+			 * is flushed, leaving the driver in ACTIVE even
+			 * after resuming until dynamic_ps_timer puts
+			 * driver back in DOZE.
+			 */
+			if (sdata->u.mgd.associated &&
+			    sdata->u.mgd.powersave &&
+			     !(local->hw.conf.flags & IEEE80211_CONF_PS)) {
+				local->hw.conf.flags |= IEEE80211_CONF_PS;
+				ieee80211_hw_config(local,
+						    IEEE80211_CONF_CHANGE_PS);
+			}
 		}
 
 		err = drv_suspend(local, wowlan);

Re: [PATCH V3] mac80211: wowlan: suspend: Fix power save state stuck in ACTIVE caused by race between suspend and Tx packet.

[Johannes Berg]

On Wed, 2015-06-10 at 19:12 +0530, Chaitanya T K wrote:
> From: Chaitanya T K <chaitanya.mgit@gmail.com>
> 
> Background: When wowlan is enabled, the chipset stays in low
> power state maintaining the connection as per the
> mac80211 power save state.
> 
> If suspended during TX in progress, there can be 
> race where the driver is put of of power-save due
> to TX and during suspend dynamic_ps_time is cancelled
> and TX packet is flushed, leaving the driver in ACTIVE
> even after resuming until dynamic_ps_time puts 
> driver back in DOZE. (Which only happens if there
> is another TX).
> 
> This can lead to high power consumption of chipset 
> during (or) after resuming.

I still don't like it. I also don't believe that what you're writing is
actually true. The only problem I can see is that it leads to higher
power consumption *while the system is suspended* - at resume time we
send a packet and thus kick the timers...

I was going to change it to this:

-------
mac80211: ensure powersave is enabled when going to WoWLAN

If the system suspends while the client powersave mechanism
is waiting to suspend due to a previous TX, the system will 
go to sleep with the NIC being awake (active) rather than
in doze mode. Unless the driver handles this, it leads to
excessive power consumption while sleeping.
-------

but I'm not convinced all of this is right.

johannes

Re: [PATCH V3] mac80211: wowlan: suspend: Fix power save state stuck in ACTIVE caused by race between suspend and Tx packet.

[Krishna Chaitanya]

On Wed, Jun 17, 2015 at 2:17 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2015-06-10 at 19:12 +0530, Chaitanya T K wrote:
>> From: Chaitanya T K <chaitanya.mgit@gmail.com>
>>
>> Background: When wowlan is enabled, the chipset stays in low
>> power state maintaining the connection as per the
>> mac80211 power save state.
>>
>> If suspended during TX in progress, there can be
>> race where the driver is put of of power-save due
>> to TX and during suspend dynamic_ps_time is cancelled
>> and TX packet is flushed, leaving the driver in ACTIVE
>> even after resuming until dynamic_ps_time puts
>> driver back in DOZE. (Which only happens if there
>> is another TX).
>>
>> This can lead to high power consumption of chipset
>> during (or) after resuming.
>
> I still don't like it. I also don't believe that what you're writing is
> actually true. The only problem I can see is that it leads to higher
> power consumption *while the system is suspended* - at resume time we
> send a packet and thus kick the timers...
"If" we send a packet, but until them the system will still be
active.
>
> I was going to change it to this:
>
> -------
> mac80211: ensure powersave is enabled when going to WoWLAN
>
> If the system suspends while the client powersave mechanism
> is waiting to suspend due to a previous TX, the system will
> go to sleep with the NIC being awake (active) rather than
> in doze mode. Unless the driver handles this, it leads to
> excessive power consumption while sleeping.
> -------
Yes, i am fine with this, but wanted to include about resume
in case there is not response accepted fro the RX packet
which triggered resume.

> but I'm not convinced all of this is right.
Normally all drivers/firmware are honoring mac80211
power state to enter Doze/not. Yes, we can ask drivers
to force power save while entering suspend, but still
up on resumption we must honor the mac80211 power
save , so till the time we send any TX, chipset will be active
drawing more power.

Re: [PATCH V3] mac80211: wowlan: suspend: Fix power save state stuck in ACTIVE caused by race between suspend and Tx packet.

[Johannes Berg]

On Wed, 2015-06-17 at 15:52 +0530, Krishna Chaitanya wrote:

> > I still don't like it. I also don't believe that what you're writing is
> > actually true. The only problem I can see is that it leads to higher
> > power consumption *while the system is suspended* - at resume time we
> > send a packet and thus kick the timers...
> "If" we send a packet, but until them the system will still be
> active.

But we *always* send a packet:

ieee80211_reconfig:
...
        /*
         * The sta might be in psm against the ap (e.g. because
         * this was the state before a hw restart), so we
         * explicitly send a null packet in order to make sure
         * it'll sync against the ap (and get out of psm).
         */
        if (!(local->hw.conf.flags & IEEE80211_CONF_PS)) {
                list_for_each_entry(sdata, &local->interfaces, list) {
                        if (sdata->vif.type != NL80211_IFTYPE_STATION)
                                continue;
                        if (!sdata->u.mgd.associated)
                                continue;

                        ieee80211_send_nullfunc(local, sdata, 0);
                }
        }



Then again, you're talking about WoWLAN, but then ...

johannes

Re: [PATCH V3] mac80211: wowlan: suspend: Fix power save state stuck in ACTIVE caused by race between suspend and Tx packet.

[Krishna Chaitanya]

On Wed, Jun 17, 2015 at 4:31 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2015-06-17 at 15:52 +0530, Krishna Chaitanya wrote:
>
>> > I still don't like it. I also don't believe that what you're writing is
>> > actually true. The only problem I can see is that it leads to higher
>> > power consumption *while the system is suspended* - at resume time we
>> > send a packet and thus kick the timers...
>> "If" we send a packet, but until them the system will still be
>> active.
>
> But we *always* send a packet:
>
> ieee80211_reconfig:
> ...
>         /*
>          * The sta might be in psm against the ap (e.g. because
>          * this was the state before a hw restart), so we
>          * explicitly send a null packet in order to make sure
>          * it'll sync against the ap (and get out of psm).
>          */
>         if (!(local->hw.conf.flags & IEEE80211_CONF_PS)) {
>                 list_for_each_entry(sdata, &local->interfaces, list) {
>                         if (sdata->vif.type != NL80211_IFTYPE_STATION)
>                                 continue;
>                         if (!sdata->u.mgd.associated)
>                                 continue;
>
>                         ieee80211_send_nullfunc(local, sdata, 0);
>                 }
>         }
>
>
>
> Then again, you're talking about WoWLAN, but then ...
In wowlan, we simply call drv_resume and only if it fails
then we will go through full resume. And without wowlan
connection will not be there so this code will not be hit.

Re: [PATCH V3] mac80211: wowlan: suspend: Fix power save state stuck in ACTIVE caused by race between suspend and Tx packet.

[Johannes Berg]

On Wed, 2015-06-17 at 17:05 +0530, Krishna Chaitanya wrote:

> > Then again, you're talking about WoWLAN, but then ...
> In wowlan, we simply call drv_resume and only if it fails
> then we will go through full resume. And without wowlan
> connection will not be there so this code will not be hit.

Right, but then the important part still is that we're enabling
powersave when going to sleep, no? I mean ... anyway it doesn't matter
much. I'll come up with some better comment/description I guess.

I really hate this whole powersave code btw - we really should refactor
that into its own little library module or so...

johannes

Re: [PATCH V3] mac80211: wowlan: suspend: Fix power save state stuck in ACTIVE caused by race between suspend and Tx packet.

[Krishna Chaitanya]

On Wed, Jun 17, 2015 at 5:07 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2015-06-17 at 17:05 +0530, Krishna Chaitanya wrote:
>
>> > Then again, you're talking about WoWLAN, but then ...
>> In wowlan, we simply call drv_resume and only if it fails
>> then we will go through full resume. And without wowlan
>> connection will not be there so this code will not be hit.
>
> Right, but then the important part still is that we're enabling
> powersave when going to sleep, no? I mean ... anyway it doesn't matter
> much. I'll come up with some better comment/description I guess.
Yes, we are forcing it to go in to power-save.
>
> I really hate this whole powersave code btw - we really should refactor
> that into its own little library module or so...
Yes, that sounds good.

Re: [PATCH V3] mac80211: wowlan: suspend: Fix power save state stuck in ACTIVE caused by race between suspend and Tx packet.

[Johannes Berg]

I've applied this patch.

> > I really hate this whole powersave code btw - we really should 
> > refactor
> > that into its own little library module or so...
> Yes, that sounds good.

want to work on that? :)

The whole thing is awfully broken anyway since it does hw-level
configuration based on vif-level state, and ath9k does in fact support
multiple vifs ...

johannes

Re: [PATCH V3] mac80211: wowlan: suspend: Fix power save state stuck in ACTIVE caused by race between suspend and Tx packet.

[Krishna Chaitanya]

On Fri, Jul 17, 2015 at 2:46 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>
>
> I've applied this patch.

Thanks.

>
>
> > > I really hate this whole powersave code btw - we really should
> > > refactor
> > > that into its own little library module or so...
> > Yes, that sounds good.
>
> want to work on that? :)
Sure, but i cannot spend full time on that so the progress might be slow.

>
> The whole thing is awfully broken anyway since it does hw-level
> configuration based on vif-level state, and ath9k does in fact support
> multiple vifs ...
Yes, we have faced problems with this especially handling fake ps,
roc...we should move it to per vif config