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

[PATCH V2] mac80211: 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>

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 <chaitanya.mgit@gmail.com>
---
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);

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

[Johannes Berg]

On Mon, 2015-06-01 at 22:44 +0530, Chaitanya T K wrote:
> From: Chaitanya T K <chaitanya.mgit@gmail.com>
> 
> 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

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

[Krishna Chaitanya]

On Tue, Jun 2, 2015 at 2:06 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2015-06-01 at 22:44 +0530, Chaitanya T K wrote:
>> From: Chaitanya T K <chaitanya.mgit@gmail.com>
>>
>> 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 :-).

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

[Johannes Berg]

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

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

[Krishna Chaitanya]

On Wed, Jun 10, 2015 at 1:40 AM, Johannes Berg
<johannes@sipsolutions.net> 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.