[RFC 0/3] ath9k: sleep paranoia series

[RFC 0/3] ath9k: sleep paranoia series

[Luis R. Rodriguez]

We need to ensure the device is in full sleep when going to
suspend, otherwise the device becomes completely unresponsive
upon resume. This series has one fix on a theoretical imbalance
which would leave the device awake all the time, and some two
paranoia warnings which we should look out for. Hitting any
of these warnings should be investigated.

I'm marking only one patch as a ready patch, the two others
are RFCs as I'd like more wider testing first. At least with
my AR9280 I get no warnings.

Luis R. Rodriguez (3):
  ath9k: warn when we get a ATH9K_INT_TIM_TIMER and are idle
  ath9k: Fix power save count imbalance on ath_radio_enable()
  ath9k_hw: warn if we cannot change the power to the chip

 drivers/net/wireless/ath/ath9k/hw.c   |    2 ++
 drivers/net/wireless/ath/ath9k/main.c |    5 +++--
 2 files changed, 5 insertions(+), 2 deletions(-)

-- 
1.7.3.2.90.gd4c43

[RFC 1/3] ath9k: warn when we get a ATH9K_INT_TIM_TIMER and are idle

[Luis R. Rodriguez]

We should not be idle when we get the ATH9K_INT_TIM_TIMER,
otherwise we wake up the chip and that throws off the idle
state, the driver needs to be in full sleep when idle and
nothing should turn it awake without turning it back to
full sleep again. If we leave the chip idle and suspend,
upon resume the device will become unusable and we get:

ath: Starting driver with initial channel: 5745 MHz
ath: timeout (100000 us) on reg 0x7000: 0xdeadbeef & 0x00000003 != 0x00000000

Cc: Paul Stewart <pstew@google.com>
Cc: Amod Bodas <amod.bodas@atheros.com>
signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath/ath9k/main.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index f026a03..fd27ec9 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -770,6 +770,7 @@ irqreturn_t ath_isr(int irq, void *dev)
 		if (status & ATH9K_INT_TIM_TIMER) {
 			/* Clear RxAbort bit so that we can
 			 * receive frames */
+			WARN_ON(sc->ps_idle);
 			ath9k_setpower(sc, ATH9K_PM_AWAKE);
 			ath9k_hw_setrxabort(sc->sc_ah, 0);
 			sc->ps_flags |= PS_WAIT_FOR_BEACON;
-- 
1.7.3.2.90.gd4c43

[PATCH 2/3] ath9k: Fix power save count imbalance on ath_radio_enable()

[Luis R. Rodriguez]

Upon a failure we never call ath9k_ps_restore() on ath_radio_enable(),
this will throw off the sc->ps_usecount. When the sc->ps_usecount
is > 0 we never put the chip to full sleep. This drains battery,
and will also make the chip fail upon resume with:

ath: Starting driver with initial channel: 5745 MHz
ath: timeout (100000 us) on reg 0x7000: 0xdeadbeef & 0x00000003 != 0x00000000

This would make the chip useless upon resume.

I cannot prove this can happen but in theory it is so best to
avoid this race completely and not have users complain about
a broken device after resume.

Cc: stable@kernel.org
Cc: Paul Stewart <pstew@google.com>
Cc: Amod Bodas <amod.bodas@atheros.com>
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath/ath9k/main.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index fd27ec9..97ddb32 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -903,8 +903,7 @@ void ath_radio_enable(struct ath_softc *sc, struct ieee80211_hw *hw)
 	if (ath_startrecv(sc) != 0) {
 		ath_print(common, ATH_DBG_FATAL,
 			  "Unable to restart recv logic\n");
-		spin_unlock_bh(&sc->sc_pcu_lock);
-		return;
+		goto out;
 	}
 	if (sc->sc_flags & SC_OP_BEACONS)
 		ath_beacon_config(sc, NULL);	/* restart beacons */
@@ -918,6 +917,7 @@ void ath_radio_enable(struct ath_softc *sc, struct ieee80211_hw *hw)
 	ath9k_hw_set_gpio(ah, ah->led_pin, 0);
 
 	ieee80211_wake_queues(hw);
+out:
 	spin_unlock_bh(&sc->sc_pcu_lock);
 
 	ath9k_ps_restore(sc);
-- 
1.7.3.2.90.gd4c43

Re: [PATCH 2/3] ath9k: Fix power save count imbalance on ath_radio_enable()

[Luis R. Rodriguez]

On Fri, Dec 3, 2010 at 5:20 PM, Luis R. Rodriguez
<lrodriguez@atheros.com> wrote:
> Upon a failure we never call ath9k_ps_restore() on ath_radio_enable(),
> this will throw off the sc->ps_usecount. When the sc->ps_usecount
> is > 0 we never put the chip to full sleep. This drains battery,
> and will also make the chip fail upon resume with:
>
> ath: Starting driver with initial channel: 5745 MHz
> ath: timeout (100000 us) on reg 0x7000: 0xdeadbeef & 0x00000003 != 0x00000000
>
> This would make the chip useless upon resume.
>
> I cannot prove this can happen but in theory it is so best to
> avoid this race completely and not have users complain about
> a broken device after resume.
>
> Cc: stable@kernel.org
> Cc: Paul Stewart <pstew@google.com>
> Cc: Amod Bodas <amod.bodas@atheros.com>
> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>

John, this one is for you, sorry I failed to send it to you. And
Johannes, sorry, I forgot to remove you from my send script :)

  Luis

Re: [PATCH 2/3] ath9k: Fix power save count imbalance on ath_radio_enable()

[John W. Linville]

On Fri, Dec 03, 2010 at 05:34:08PM -0800, Luis R. Rodriguez wrote:
> On Fri, Dec 3, 2010 at 5:20 PM, Luis R. Rodriguez
> <lrodriguez@atheros.com> wrote:
> > Upon a failure we never call ath9k_ps_restore() on ath_radio_enable(),
> > this will throw off the sc->ps_usecount. When the sc->ps_usecount
> > is > 0 we never put the chip to full sleep. This drains battery,
> > and will also make the chip fail upon resume with:
> >
> > ath: Starting driver with initial channel: 5745 MHz
> > ath: timeout (100000 us) on reg 0x7000: 0xdeadbeef & 0x00000003 != 0x00000000
> >
> > This would make the chip useless upon resume.
> >
> > I cannot prove this can happen but in theory it is so best to
> > avoid this race completely and not have users complain about
> > a broken device after resume.
> >
> > Cc: stable@kernel.org
> > Cc: Paul Stewart <pstew@google.com>
> > Cc: Amod Bodas <amod.bodas@atheros.com>
> > Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> 
> John, this one is for you, sorry I failed to send it to you. And
> Johannes, sorry, I forgot to remove you from my send script :)

So, this is for 2.6.37?  FWIW, it was the only PATCH in a series
of RFCs...

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH 2/3] ath9k: Fix power save count imbalance on ath_radio_enable()

[Luis R. Rodriguez]

On Tue, Dec 07, 2010 at 01:01:30PM -0800, John W. Linville wrote:
> On Fri, Dec 03, 2010 at 05:34:08PM -0800, Luis R. Rodriguez wrote:
> > On Fri, Dec 3, 2010 at 5:20 PM, Luis R. Rodriguez
> > <lrodriguez@atheros.com> wrote:
> > > Upon a failure we never call ath9k_ps_restore() on ath_radio_enable(),
> > > this will throw off the sc->ps_usecount. When the sc->ps_usecount
> > > is > 0 we never put the chip to full sleep. This drains battery,
> > > and will also make the chip fail upon resume with:
> > >
> > > ath: Starting driver with initial channel: 5745 MHz
> > > ath: timeout (100000 us) on reg 0x7000: 0xdeadbeef & 0x00000003 != 0x00000000
> > >
> > > This would make the chip useless upon resume.
> > >
> > > I cannot prove this can happen but in theory it is so best to
> > > avoid this race completely and not have users complain about
> > > a broken device after resume.
> > >
> > > Cc: stable@kernel.org
> > > Cc: Paul Stewart <pstew@google.com>
> > > Cc: Amod Bodas <amod.bodas@atheros.com>
> > > Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> > 
> > John, this one is for you, sorry I failed to send it to you. And
> > Johannes, sorry, I forgot to remove you from my send script :)
> 
> So, this is for 2.6.37?

Yes.

> FWIW, it was the only PATCH in a series of RFCs...

Thanks, yes, I noted this on my PATCH 0 cover letter. If you want
you can disrecard this though and just consider the PATCH I posted
in my new series. Its in that series as well, but let me just iron
out that series first, please ignore that series as well for now.

  Luis

[RFC 3/3] ath9k_hw: warn if we cannot change the power to the chip

[Luis R. Rodriguez]

Suspend requires the device to be in fullsleep otherwise upon
resume the device becomes unresponsive. We need to ensure
that when we want the device to go to sleep it yields to
the request, otherwise we'll have a useless devices upon
resume. Warn when changing the power fails as we need
to look into these issues.

Cc: Paul Stewart <pstew@google.com>
Cc: Amod Bodas <amod.bodas@atheros.com>
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath/ath9k/hw.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
index 9b1ee7f..8e87113 100644
--- a/drivers/net/wireless/ath/ath9k/hw.c
+++ b/drivers/net/wireless/ath/ath9k/hw.c
@@ -1615,6 +1615,8 @@ bool ath9k_hw_setpower(struct ath_hw *ah, enum ath9k_power_mode mode)
 	}
 	ah->power_mode = mode;
 
+	WARN_ON(!status);
+
 	return status;
 }
 EXPORT_SYMBOL(ath9k_hw_setpower);
-- 
1.7.3.2.90.gd4c43