2015-06-01 17:15:51

by Krishna Chaitanya

[permalink] [raw]
Subject: [PATCH V2] mac80211: Fix power save state stuck in ACTIVE caused by race between suspend and Tx packet.

From: Chaitanya T K <[email protected]>

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 high power consumption of chipset during (or)
after resuming also.

Signed-off-by: Chaitanya T K <[email protected]>
---
V2: Updated Comment and Commit log.
---
net/mac80211/pm.c | 15 +++++++++++++++
net/mac80211/pm.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)

diff --git a/net/mac80211/pm.c b/net/mac80211/pm.c
index ac6ad62..cc311be 100644
--- a/net/mac80211/pm.c
+++ b/net/mac80211/pm.c
@@ -76,6 +76,21 @@ 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, 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);


2015-06-01 20:36:09

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH V2] mac80211: Fix power save state stuck in ACTIVE caused by race between suspend and Tx packet.

On Mon, 2015-06-01 at 22:44 +0530, Chaitanya T K wrote:
> From: Chaitanya T K <[email protected]>
>
> 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 high power consumption of chipset during (or)
> after resuming also.

This isn't what your patch is actually doing though. You need to mention
WoWLAN at the very least in your commit log; or are you just randomly
changing the code until it fixes your bug? :)

johannes


2015-06-09 20:30:35

by Krishna Chaitanya

[permalink] [raw]
Subject: Re: [PATCH V2] mac80211: Fix power save state stuck in ACTIVE caused by race between suspend and Tx packet.

On Wed, Jun 10, 2015 at 1:40 AM, Johannes Berg
<[email protected]> wrote:
> On Tue, 2015-06-02 at 03:02 +0530, Krishna Chaitanya wrote:
>
>> > This isn't what your patch is actually doing though. You need to mention
>> > WoWLAN at the very least in your commit log;
>
>> Yes, WoWLAN is enabled in our testing. Without wowlan the connection
>> will not be intact. So i assumed the check "is_associated" would
>> imply wowlan.
>
> I meant the commit log.
>
> If I read your commit log without the patch, it doesn't make sense at
> all since regular suspend/resume will turn off the device anyway. You
> really need to mention
> a) the fact that wowlan is used, and
> b) the fact that you care about powersave state *while in wowlan*
Hmm...ok, i will rephrase the commit log and re-submit V3, Thanks.

2015-06-09 20:10:32

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH V2] mac80211: Fix power save state stuck in ACTIVE caused by race between suspend and Tx packet.

On Tue, 2015-06-02 at 03:02 +0530, Krishna Chaitanya wrote:

> > This isn't what your patch is actually doing though. You need to mention
> > WoWLAN at the very least in your commit log;

> Yes, WoWLAN is enabled in our testing. Without wowlan the connection
> will not be intact. So i assumed the check "is_associated" would
> imply wowlan.

I meant the commit log.

If I read your commit log without the patch, it doesn't make sense at
all since regular suspend/resume will turn off the device anyway. You
really need to mention
a) the fact that wowlan is used, and
b) the fact that you care about powersave state *while in wowlan*

johannes


2015-06-01 21:32:25

by Krishna Chaitanya

[permalink] [raw]
Subject: Re: [PATCH V2] mac80211: Fix power save state stuck in ACTIVE caused by race between suspend and Tx packet.

On Tue, Jun 2, 2015 at 2:06 AM, Johannes Berg <[email protected]> wrote:
> On Mon, 2015-06-01 at 22:44 +0530, Chaitanya T K wrote:
>> From: Chaitanya T K <[email protected]>
>>
>> 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 high power consumption of chipset during (or)
>> after resuming also.
>
> This isn't what your patch is actually doing though. You need to mention
> WoWLAN at the very least in your commit log;
Yes, WoWLAN is enabled in our testing. Without wowlan the connection
will not be intact. So i assumed the check "is_associated" would
imply wowlan.

> or are you just randomly
> changing the code until it fixes your bug? :)
I am not that lucky to find a proper fix that way :-).