diff mbox

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

Message ID 1433943751-11221-1-git-send-email-chaitanya.mgit@gmail.com (mailing list archive)
State Accepted
Delegated to: Johannes Berg
Headers show

Commit Message

Krishna Chaitanya June 10, 2015, 1:42 p.m. UTC
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(+)

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Johannes Berg June 17, 2015, 8:47 a.m. UTC | #1
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

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krishna Chaitanya June 17, 2015, 10:22 a.m. UTC | #2
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johannes Berg June 17, 2015, 11:01 a.m. UTC | #3
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

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krishna Chaitanya June 17, 2015, 11:35 a.m. UTC | #4
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johannes Berg June 17, 2015, 11:37 a.m. UTC | #5
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

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krishna Chaitanya June 17, 2015, 12:15 p.m. UTC | #6
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johannes Berg July 17, 2015, 9:16 a.m. UTC | #7
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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krishna Chaitanya July 17, 2015, 9:33 a.m. UTC | #8
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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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);