[PATCH 0/5] Few mac80211 and ath9k suspend fixes

[PATCH 0/5] Few mac80211 and ath9k suspend fixes

[Luis R. Rodriguez]

John, here are a few fixes for suspend for both mac80211
and ath9k. I'd like to wait for Johanne's ack on both
mac80211 fixes, the ath9k patches don't depend on those
two fixes so they can be merged separately.

The all-in-one patch for wireless-testing is available at:

http://bombadil.infradead.org/~mcgrof/patches/wireless-testing/2009/12/pm-fixes-2009-12-23.patch

Individual backported patches to test against upstream 2.6.32 are available at:

http://kernel.org/pub/linux/kernel/people/mcgrof/patches/var/2009/12/pm-fixes-32-pending/

The upstream commit are pending on the commit logs there.

For those still on 2.6.32, I still recommend to apply:

mac80211: fix WMM AP settings application
mac80211: fix resume

You can get these here:

http://bombadil.infradead.org/~mcgrof/patches/pm-stuff/0001-mac80211-fix-resume.patch
http://bombadil.infradead.org/~mcgrof/patches/pm-stuff/0002-mac80211-fix-WMM-AP-settings-application.patch

These two likely have their respective upstream sha1sum already, not sure.

Luis R. Rodriguez (4):
  mac80211: fix race with suspend and dynamic_ps_disable_work
  mac80211: fix propagation of failed hardware reconfigurations
  ath9k: wake hardware for interface IBSS/AP/Mesh removal
  ath9k: wake hardware during AMPDU TX actions

Sujith (1):
  ath9k: fix suspend by waking device prior to stop

 drivers/net/wireless/ath/ath9k/main.c |   14 ++++++++++++++
 net/mac80211/tx.c                     |    4 ++++
 net/mac80211/util.c                   |   12 ++++++++++++
 3 files changed, 30 insertions(+), 0 deletions(-)

[PATCH 1/5] mac80211: fix race with suspend and dynamic_ps_disable_work

[Luis R. Rodriguez]

When mac80211 suspends it calls a driver's suspend callback
as a last step and after that the driver assumes no calls will
be made to it until we resume and its start callback is kicked.
If such calls are made, however, suspend can end up throwing
hardware in an unexpected state and making the device unusable
upon resume.

This situation is observed with ath9k but likely possible
with any other device which supports and supports dynampic PS
and enabled. When mac80211 suspends it tears down BA sessions
with ieee80211_sta_tear_down_BA_sessions() and since this ends
up transmitting frames through ieee80211_xmit() mac80211 could
end up scheduling the dynamic_ps_disable_work work onto the
mac80211 workqueue. This allows for a race between the work
kicking off and mac80211 completing the suspend work by calling
the driver's stop callback. If the driver's stop callback is
run first and the scheduled work runs later (this is expected
as we don't flush in between) the driver's config callback
could run after the hardware has been turned off which amongst
other things could end up leaving the card with enabled
interrupts and awake leaving the harware in an unpredictable
state prior to suspend. Upon resume the device can become
completely unfunctional displaying PCI-express errors such as
"unsupported request detected" and the driver's respective
start callback would failing. Apart from leaving the hardware
in an unresponsive state since mac80211 currently allows failed
start calls to go through new interrupts will be unhandled and
as such the interrupt for the device will end up getting disabled
as follows:

irq 18: nobody cared (try booting with the "irqpoll" option)
Pid: 0, comm: swapper Not tainted 2.6.31.4-intel-menlow #5
Call Trace:
 [<c105ca3e>] __report_bad_irq+0x2e/0x6f
 [<c105cb74>] note_interrupt+0xf5/0x14d
 [<c105d0a4>] handle_fasteoi_irq+0x7d/0x9b
 [<c10048c1>] handle_irq+0x3b/0x46
 [<c1004103>] do_IRQ+0x41/0x95
 [<c1003189>] common_interrupt+0x29/0x30
 [<c103007b>] ? ptrace_notify+0x12/0x97
 [<c1040f07>] ? tick_nohz_stop_sched_tick+0x2ee/0x2f6
 [<c1001e11>] cpu_idle+0x27/0x5e
 [<c12feb73>] rest_init+0x53/0x55
 [<c14b0805>] start_kernel+0x2f6/0x2fb
 [<c14b0070>] i386_start_kernel+0x70/0x77

Fix this by preventing mac80211 to schedule dynamic_ps_disable_work
by checking for when mac80211 starts to suspend and starts
quiescing. Frames should be allowed to go through though as
that is part of the quiescing steps and we do not flush the
mac80211 workqueue since it was already done towards the
beginning of suspend cycle.

The other mac80211 issue will be hanled in the next patch.

For further details see refer to the thread:

http://marc.info/?t=126144866100001&r=1&w=2

Cc: stable@kernel.org
Cc: johannes@sipsolutions.net
Cc: Jonathan May <jonathan.may@atheros.com>
Cc: David Quan <david.quan@atheros.com>
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/mac80211/tx.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index ac48c86..42bfd97 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1418,6 +1418,10 @@ static bool need_dynamic_ps(struct ieee80211_local *local)
 	if (!local->ps_sdata)
 		return false;
 
+	/* No point if we're going to suspend */
+	if (local->quiescing)
+		return false;
+
 	return true;
 }
 
-- 
1.6.3.3

[PATCH 2/5] mac80211: fix propagation of failed hardware reconfigurations

[Luis R. Rodriguez]

mac80211 does not propagate failed hardware reconfiguration
requests. For suspend and resume this is important due to all
the possible issues that can come out of the suspend <-> resume
cycle. Not propagating the error means cfg80211 will assume
the resume for the device went through fine and mac80211 will
continue on trying to poke at the hardware, enable timers,
queue work, and so on for a device which is completley
unfunctional.

The least we can do is to propagate device start issues and
warn when this occurs upon resume. A side effect of this patch
is we also now propagate the start errors upon harware
reconfigurations (non-suspend), but this should also be desirable
anyway, there is not point in continuing to reconfigure a
device if mac80211 was unable to start the device.

For further details refer to the thread:

http://marc.info/?t=126151038700001&r=1&w=2

Cc: stable@kernel.org
Cc: johannes@sipsolutions.net
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/mac80211/util.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index b019725..57c2c39 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -1047,7 +1047,19 @@ int ieee80211_reconfig(struct ieee80211_local *local)
 
 	/* restart hardware */
 	if (local->open_count) {
+		/*
+		 * Upon resume hardware can sometimes be goofy due to
+		 * various platform / driver / bus issues, so restarting
+		 * the device may at times not work immediately. Propagate
+		 * the error.
+		 */
 		res = drv_start(local);
+		if (res) {
+			WARN(local->suspended, "Harware became unavailable "
+			     "upon resume likely due to a bug on mac80211, "
+			     "your driver or a bus issue\n");
+			return res;
+		}
 
 		ieee80211_led_radio(local, true);
 	}
-- 
1.6.3.3

[PATCH 3/5] ath9k: fix suspend by waking device prior to stop

[Luis R. Rodriguez]

From: Sujith <Sujith.Manoharan@atheros.com>

Ensure the device is awake prior to trying to tell hardware
to stop it. Impact of not doing this is we can likely leave
the device in an undefined state likely causing issues with
suspend and resume. This patch ensures harware is where it
should be prior to suspend.

Cc: stable@kernel.org
Signed-off-by: Sujith <Sujith.Manoharan@atheros.com>
---
 drivers/net/wireless/ath/ath9k/main.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 3f5b887..1464ded 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -2504,6 +2504,9 @@ static void ath9k_stop(struct ieee80211_hw *hw)
 		return; /* another wiphy still in use */
 	}
 
+	/* Ensure HW is awake when we try to shut it down. */
+	ath9k_ps_wakeup(sc);
+
 	if (ah->btcoex_hw.enabled) {
 		ath9k_hw_btcoex_disable(ah);
 		if (ah->btcoex_hw.scheme == ATH_BTCOEX_CFG_3WIRE)
@@ -2524,6 +2527,9 @@ static void ath9k_stop(struct ieee80211_hw *hw)
 	/* disable HAL and put h/w to sleep */
 	ath9k_hw_disable(ah);
 	ath9k_hw_configpcipowersave(ah, 1, 1);
+	ath9k_ps_restore(sc);
+
+	/* Finally, put the chip in FULL SLEEP mode */
 	ath9k_setpower(sc, ATH9K_PM_FULL_SLEEP);
 
 	sc->sc_flags |= SC_OP_INVALID;
-- 
1.6.3.3

[PATCH 4/5] ath9k: wake hardware for interface IBSS/AP/Mesh removal

[Luis R. Rodriguez]

When we remove a IBSS/AP/Mesh interface we stop DMA
but to do this we should ensure hardware is on. Awaken
the device prior to these calls. This should ensure
DMA is stopped upon suspend and plain device removal.

Cc: stable@kernel.org
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath/ath9k/main.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 1464ded..9a4d9b9 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -2643,8 +2643,10 @@ static void ath9k_remove_interface(struct ieee80211_hw *hw,
 	if ((sc->sc_ah->opmode == NL80211_IFTYPE_AP) ||
 	    (sc->sc_ah->opmode == NL80211_IFTYPE_ADHOC) ||
 	    (sc->sc_ah->opmode == NL80211_IFTYPE_MESH_POINT)) {
+		ath9k_ps_wakeup(sc);
 		ath9k_hw_stoptxdma(sc->sc_ah, sc->beacon.beaconq);
 		ath_beacon_return(sc, avp);
+		ath9k_ps_restore(sc);
 	}
 
 	sc->sc_flags &= ~SC_OP_BEACONS;
-- 
1.6.3.3

[PATCH 5/5] ath9k: wake hardware during AMPDU TX actions

[Luis R. Rodriguez]

AMDPDU actions poke hardware for TX operation, as such
we want to turn hardware on for these actions. AMDPU RX operations
do not require hardware on as nothing is done in hardware for
those actions. Without this we cannot guarantee hardware has
been programmed correctly for each AMPDU TX action.

Cc: stable@kernel.org
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath/ath9k/main.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 9a4d9b9..c1c96ea 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -3095,15 +3095,21 @@ static int ath9k_ampdu_action(struct ieee80211_hw *hw,
 	case IEEE80211_AMPDU_RX_STOP:
 		break;
 	case IEEE80211_AMPDU_TX_START:
+		ath9k_ps_wakeup(sc);
 		ath_tx_aggr_start(sc, sta, tid, ssn);
 		ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid);
+		ath9k_ps_restore(sc);
 		break;
 	case IEEE80211_AMPDU_TX_STOP:
+		ath9k_ps_wakeup(sc);
 		ath_tx_aggr_stop(sc, sta, tid);
 		ieee80211_stop_tx_ba_cb_irqsafe(vif, sta->addr, tid);
+		ath9k_ps_restore(sc);
 		break;
 	case IEEE80211_AMPDU_TX_OPERATIONAL:
+		ath9k_ps_wakeup(sc);
 		ath_tx_aggr_resume(sc, sta, tid);
+		ath9k_ps_restore(sc);
 		break;
 	default:
 		ath_print(ath9k_hw_common(sc->sc_ah), ATH_DBG_FATAL,
-- 
1.6.3.3

Re: [PATCH 2/5] mac80211: fix propagation of failed hardware reconfigurations

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 666 bytes --]

On Wed, 2009-12-23 at 20:03 -0500, Luis R. Rodriguez wrote:

>  	/* restart hardware */
>  	if (local->open_count) {
> +		/*
> +		 * Upon resume hardware can sometimes be goofy due to
> +		 * various platform / driver / bus issues, so restarting
> +		 * the device may at times not work immediately. Propagate
> +		 * the error.
> +		 */
>  		res = drv_start(local);
> +		if (res) {
> +			WARN(local->suspended, "Harware became unavailable "
> +			     "upon resume likely due to a bug on mac80211, "
> +			     "your driver or a bus issue\n");
> +			return res;
> +		}

I object to the wording -- how would this be a mac80211 bug? :)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 1/5] mac80211: fix race with suspend and dynamic_ps_disable_work

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 4096 bytes --]

On Wed, 2009-12-23 at 20:03 -0500, Luis R. Rodriguez wrote:
> When mac80211 suspends it calls a driver's suspend callback
> as a last step and after that the driver assumes no calls will
> be made to it until we resume and its start callback is kicked.
> If such calls are made, however, suspend can end up throwing
> hardware in an unexpected state and making the device unusable
> upon resume.
> 
> This situation is observed with ath9k but likely possible
> with any other device which supports and supports dynampic PS
> and enabled. When mac80211 suspends it tears down BA sessions
> with ieee80211_sta_tear_down_BA_sessions() and since this ends
> up transmitting frames through ieee80211_xmit() mac80211 could
> end up scheduling the dynamic_ps_disable_work work onto the
> mac80211 workqueue. This allows for a race between the work
> kicking off and mac80211 completing the suspend work by calling
> the driver's stop callback. If the driver's stop callback is
> run first and the scheduled work runs later (this is expected
> as we don't flush in between) the driver's config callback
> could run after the hardware has been turned off which amongst
> other things could end up leaving the card with enabled
> interrupts and awake leaving the harware in an unpredictable
> state prior to suspend. Upon resume the device can become
> completely unfunctional displaying PCI-express errors such as
> "unsupported request detected" and the driver's respective
> start callback would failing. Apart from leaving the hardware
> in an unresponsive state since mac80211 currently allows failed
> start calls to go through new interrupts will be unhandled and
> as such the interrupt for the device will end up getting disabled
> as follows:
> 
> irq 18: nobody cared (try booting with the "irqpoll" option)
> Pid: 0, comm: swapper Not tainted 2.6.31.4-intel-menlow #5
> Call Trace:
>  [<c105ca3e>] __report_bad_irq+0x2e/0x6f
>  [<c105cb74>] note_interrupt+0xf5/0x14d
>  [<c105d0a4>] handle_fasteoi_irq+0x7d/0x9b
>  [<c10048c1>] handle_irq+0x3b/0x46
>  [<c1004103>] do_IRQ+0x41/0x95
>  [<c1003189>] common_interrupt+0x29/0x30
>  [<c103007b>] ? ptrace_notify+0x12/0x97
>  [<c1040f07>] ? tick_nohz_stop_sched_tick+0x2ee/0x2f6
>  [<c1001e11>] cpu_idle+0x27/0x5e
>  [<c12feb73>] rest_init+0x53/0x55
>  [<c14b0805>] start_kernel+0x2f6/0x2fb
>  [<c14b0070>] i386_start_kernel+0x70/0x77
> 
> Fix this by preventing mac80211 to schedule dynamic_ps_disable_work
> by checking for when mac80211 starts to suspend and starts
> quiescing. Frames should be allowed to go through though as
> that is part of the quiescing steps and we do not flush the
> mac80211 workqueue since it was already done towards the
> beginning of suspend cycle.
> 
> The other mac80211 issue will be hanled in the next patch.
> 
> For further details see refer to the thread:
> 
> http://marc.info/?t=126144866100001&r=1&w=2
> 
> Cc: stable@kernel.org
> Cc: johannes@sipsolutions.net
> Cc: Jonathan May <jonathan.may@atheros.com>
> Cc: David Quan <david.quan@atheros.com>
> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>


Looks fine, but the commit log is way too long I think, nobody will
understand that. All the stuff about PCI and interrupts and crap is just
a pure ath9k specific symptom of the bug that isn't really all that
relevant to this commit ... if you want to log that information anyway
then please make it more of a postscriptum by explaining that it's ath9k
specific and putting it after how you fixed it.

johannes

> ---
>  net/mac80211/tx.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index ac48c86..42bfd97 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -1418,6 +1418,10 @@ static bool need_dynamic_ps(struct
> ieee80211_local *local)
>  	if (!local->ps_sdata)
>  		return false;
>  
> +	/* No point if we're going to suspend */
> +	if (local->quiescing)
> +		return false;
> +
>  	return true;
>  }
>  


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/5] mac80211: fix propagation of failed hardware reconfigurations

[Luis R. Rodriguez]

T24gVGh1LCBEZWMgMjQsIDIwMDkgYXQgNDozOCBBTSwgSm9oYW5uZXMgQmVyZwo8am9oYW5uZXNA
c2lwc29sdXRpb25zLm5ldD4gd3JvdGU6Cj4gT24gV2VkLCAyMDA5LTEyLTIzIGF0IDIwOjAzIC0w
NTAwLCBMdWlzIFIuIFJvZHJpZ3VleiB3cm90ZToKPgo+PiDCoCDCoCDCoCAvKiByZXN0YXJ0IGhh
cmR3YXJlICovCj4+IMKgIMKgIMKgIGlmIChsb2NhbC0+b3Blbl9jb3VudCkgewo+PiArIMKgIMKg
IMKgIMKgIMKgIMKgIC8qCj4+ICsgwqAgwqAgwqAgwqAgwqAgwqAgwqAqIFVwb24gcmVzdW1lIGhh
cmR3YXJlIGNhbiBzb21ldGltZXMgYmUgZ29vZnkgZHVlIHRvCj4+ICsgwqAgwqAgwqAgwqAgwqAg
wqAgwqAqIHZhcmlvdXMgcGxhdGZvcm0gLyBkcml2ZXIgLyBidXMgaXNzdWVzLCBzbyByZXN0YXJ0
aW5nCj4+ICsgwqAgwqAgwqAgwqAgwqAgwqAgwqAqIHRoZSBkZXZpY2UgbWF5IGF0IHRpbWVzIG5v
dCB3b3JrIGltbWVkaWF0ZWx5LiBQcm9wYWdhdGUKPj4gKyDCoCDCoCDCoCDCoCDCoCDCoCDCoCog
dGhlIGVycm9yLgo+PiArIMKgIMKgIMKgIMKgIMKgIMKgIMKgKi8KPj4gwqAgwqAgwqAgwqAgwqAg
wqAgwqAgcmVzID0gZHJ2X3N0YXJ0KGxvY2FsKTsKPj4gKyDCoCDCoCDCoCDCoCDCoCDCoCBpZiAo
cmVzKSB7Cj4+ICsgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgV0FSTihsb2NhbC0+c3Vz
cGVuZGVkLCAiSGFyd2FyZSBiZWNhbWUgdW5hdmFpbGFibGUgIgo+PiArIMKgIMKgIMKgIMKgIMKg
IMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgInVwb24gcmVzdW1lIGxpa2VseSBkdWUgdG8gYSBidWcg
b24gbWFjODAyMTEsICIKPj4gKyDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDC
oCJ5b3VyIGRyaXZlciBvciBhIGJ1cyBpc3N1ZVxuIik7Cj4+ICsgwqAgwqAgwqAgwqAgwqAgwqAg
wqAgwqAgwqAgwqAgcmV0dXJuIHJlczsKPj4gKyDCoCDCoCDCoCDCoCDCoCDCoCB9Cj4KPiBJIG9i
amVjdCB0byB0aGUgd29yZGluZyAtLSBob3cgd291bGQgdGhpcyBiZSBhIG1hYzgwMjExIGJ1Zz8g
OikKCldlbGwgaXQgd2FzIGluIHRoZSBmaXJzdCBwbGFjZSwgc2Vjb25kLCBpZiBhbm90aGVyIGJ1
ZyBpcyBsdXJraW5nIHdlCmFyZSBsaWtlbHkgdG8gZmluZCBpdCB0aGVyZS4gQXJlIHlvdSAxMDAl
IHN1cmUgbm90aGluZyBlbHNlIHdpbGwgY3JlZXAKdXA/CgogIEx1aXMK