[PATCH 0/5] mac80211/ath9k: few suspend checks and fixes

[PATCH 0/5] mac80211/ath9k: few suspend checks and fixes

[Luis R. Rodriguez]

Here's a few suspend fixes on both ath9k and mac80211. The issue
was mainly in mac80211 but since we know we *have* to be asleep
when going to suspend we place some warnings on a few places on
ath9k to ensure we get warning for these cases. I know at least
Ben has tested his setup with these warnings and he hasn't seen
issues. 3 of these patches are stable fixes.

Luis R. Rodriguez (5):
  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
  mac80211: fix idle change notifications upon device stop
  mac80211: fix issuing idle calls when device open count is 0

 drivers/net/wireless/ath/ath9k/hw.c   |    2 ++
 drivers/net/wireless/ath/ath9k/main.c |    5 +++--
 net/mac80211/ieee80211_i.h            |   10 +++++++++-
 net/mac80211/iface.c                  |   25 ++++++++++++++++---------
 net/mac80211/main.c                   |    4 ++--
 net/mac80211/pm.c                     |    2 ++
 net/mac80211/scan.c                   |    2 +-
 net/mac80211/util.c                   |    1 +
 8 files changed, 36 insertions(+), 15 deletions(-)

-- 
1.7.3.2.90.gd4c43

[PATCH 1/5] 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

Re: [PATCH 1/5] ath9k: warn when we get a ATH9K_INT_TIM_TIMER and are idle

[Ben Greear]

On 12/06/2010 06:48 PM, Luis R. Rodriguez wrote:
> 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;

Looks like this patch never made it in.

Should it be in, or should I just drop it from my queue?

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 1/5] ath9k: warn when we get a ATH9K_INT_TIM_TIMER and are idle

[Luis R. Rodriguez]

On Tue, Jan 18, 2011 at 5:34 PM, Ben Greear <greearb@candelatech.com> wrote:
> On 12/06/2010 06:48 PM, Luis R. Rodriguez wrote:
>>
>> 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;
>
> Looks like this patch never made it in.
>
> Should it be in, or should I just drop it from my queue?
>

Eh, it shouldn't happen and if we warn we should use the new debugging
warn once thingy you added. I never saw that trigger so probably we
can just ignore it.

  Luis

Re: [PATCH 1/5] ath9k: warn when we get a ATH9K_INT_TIM_TIMER and are idle

[Ben Greear]

On 01/18/2011 05:38 PM, Luis R. Rodriguez wrote:
> On Tue, Jan 18, 2011 at 5:34 PM, Ben Greear<greearb@candelatech.com>  wrote:
>> On 12/06/2010 06:48 PM, Luis R. Rodriguez wrote:
>>>
>>> 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;
>>
>> Looks like this patch never made it in.
>>
>> Should it be in, or should I just drop it from my queue?
>>
>
> Eh, it shouldn't happen and if we warn we should use the new debugging
> warn once thingy you added. I never saw that trigger so probably we
> can just ignore it.

I've been running this patch for months and haven't noticed the
WARN_ON hit, so probably it's not needed.  Of course, I disable
power-saving..so maybe I'm not a good test case :P

Thanks,
Ben

>
>    Luis


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 1/5] ath9k: warn when we get a ATH9K_INT_TIM_TIMER and are idle

[Luis R. Rodriguez]

On Tue, Jan 18, 2011 at 05:42:20PM -0800, Ben Greear wrote:
> On 01/18/2011 05:38 PM, Luis R. Rodriguez wrote:
> > On Tue, Jan 18, 2011 at 5:34 PM, Ben Greear<greearb@candelatech.com>  wrote:
> >> On 12/06/2010 06:48 PM, Luis R. Rodriguez wrote:
> >>>
> >>> 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;
> >>
> >> Looks like this patch never made it in.
> >>
> >> Should it be in, or should I just drop it from my queue?
> >>
> >
> > Eh, it shouldn't happen and if we warn we should use the new debugging
> > warn once thingy you added. I never saw that trigger so probably we
> > can just ignore it.
> 
> I've been running this patch for months and haven't noticed the
> WARN_ON hit, so probably it's not needed.  Of course, I disable
> power-saving..so maybe I'm not a good test case :P

Which is why its OK if this goes in with the debug message thingy
only.

  Luis

[PATCH 2/5] 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

[PATCH 3/5] 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

[PATCH 4/5] mac80211: fix idle change notifications upon device stop

[Luis R. Rodriguez]

Drivers that depend on tuning their devices to disable the radio
completely and properly through mac80211' idle state change
were being left with the radio turned on once a device interface
is stopped, the reason is that we first were checking for the open
count prior to issuing any further config changes. For some devices
this could mean breaking suspend and resume completely as they were
under the impression that the idle state change to idle would be
issued either during or after the stop callback. Fix this by
allowing state changes through upon the device stop.

This fixes suspend and resume on ath9k and likely a few other
drivers.

Cc: stable@kernel.org
Cc: Paul Stewart <pstew@google.com>
Cc: Amod Bodas <amod.bodas@atheros.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/mac80211/ieee80211_i.h |    9 ++++++++-
 net/mac80211/iface.c       |    9 ++-------
 net/mac80211/main.c        |    4 ++--
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 72499fe..ae32349 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1023,8 +1023,15 @@ static inline int ieee80211_bssid_match(const u8 *raddr, const u8 *addr)
 	       is_broadcast_ether_addr(raddr);
 }
 
+int __ieee80211_hw_config(struct ieee80211_local *local,
+			  u32 changed, bool nocheck);
+
+static inline int ieee80211_hw_config(struct ieee80211_local *local,
+				      u32 changed)
+{
+	return __ieee80211_hw_config(local, changed, false);
+}
 
-int ieee80211_hw_config(struct ieee80211_local *local, u32 changed);
 void ieee80211_tx_set_protected(struct ieee80211_tx_data *tx);
 void ieee80211_bss_info_change_notify(struct ieee80211_sub_if_data *sdata,
 				      u32 changed);
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index f0f11bb..36b7000 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -530,20 +530,15 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 
 	ieee80211_recalc_ps(local, -1);
 
+	__ieee80211_hw_config(local, hw_reconf_flags, true);
+
 	if (local->open_count == 0) {
 		if (local->ops->napi_poll)
 			napi_disable(&local->napi);
 		ieee80211_clear_tx_pending(local);
 		ieee80211_stop_device(local);
-
-		/* no reconfiguring after stop! */
-		hw_reconf_flags = 0;
 	}
 
-	/* do after stop to avoid reconfiguring when we stop anyway */
-	if (hw_reconf_flags)
-		ieee80211_hw_config(local, hw_reconf_flags);
-
 	spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
 	for (i = 0; i < IEEE80211_MAX_QUEUES; i++) {
 		skb_queue_walk_safe(&local->pending[i], skb, tmp) {
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 2de6976..45d6f21 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -96,7 +96,7 @@ static void ieee80211_reconfig_filter(struct work_struct *work)
 	ieee80211_configure_filter(local);
 }
 
-int ieee80211_hw_config(struct ieee80211_local *local, u32 changed)
+int __ieee80211_hw_config(struct ieee80211_local *local, u32 changed, bool nocheck)
 {
 	struct ieee80211_channel *chan, *scan_chan;
 	int ret = 0;
@@ -159,7 +159,7 @@ int ieee80211_hw_config(struct ieee80211_local *local, u32 changed)
 		local->hw.conf.power_level = power;
 	}
 
-	if (changed && local->open_count) {
+	if (changed && (nocheck || local->open_count)) {
 		ret = drv_config(local, changed);
 		/*
 		 * Goal:
-- 
1.7.3.2.90.gd4c43

[PATCH 5/5] mac80211: fix issuing idle calls when device open count is 0

[Luis R. Rodriguez]

There are a few places where mac80211 may issue a hw config
but the hw config will be ignored and got into a black hole if
the device count is already 0. Further hw config calls will also
be discarded as the device is already marked as idle, in these
cases mac80211 assumes the radio is idle but the driver never
really got the notification. We need to ensure that places that
will call a hw reconfig with open_count set to 0 will send the
notification to the driver. This forces these checks in a few
other key missing places.

Without this suspend and resume is broken on devices which require
notifying the driver to become idle once the open_count already
has reached 0. This fixes suspend/resume when using new DBus APIs
are used which idle the device in places which had not yet gotten
widely tested.

Cc: stable@kernel.org
Cc: Paul Stewart <pstew@google.com>
Cc: Amod Bodas <amod.bodas@atheros.com>
Cc: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/mac80211/ieee80211_i.h |    1 +
 net/mac80211/iface.c       |   16 ++++++++++++++--
 net/mac80211/pm.c          |    2 ++
 net/mac80211/scan.c        |    2 +-
 net/mac80211/util.c        |    1 +
 5 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index ae32349..78ab6ff 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1211,6 +1211,7 @@ void ieee80211_process_measurement_req(struct ieee80211_sub_if_data *sdata,
 
 /* Suspend/resume and hw reconfiguration */
 int ieee80211_reconfig(struct ieee80211_local *local);
+void ieee80211_recalc_idle_force(struct ieee80211_local *local);
 void ieee80211_stop_device(struct ieee80211_local *local);
 
 #ifdef CONFIG_PM
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 36b7000..a590bae 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1320,7 +1320,8 @@ u32 __ieee80211_recalc_idle(struct ieee80211_local *local)
 	return 0;
 }
 
-void ieee80211_recalc_idle(struct ieee80211_local *local)
+static void ieee80211_recalc_idle_check(struct ieee80211_local *local,
+					bool force)
 {
 	u32 chg;
 
@@ -1328,7 +1329,18 @@ void ieee80211_recalc_idle(struct ieee80211_local *local)
 	chg = __ieee80211_recalc_idle(local);
 	mutex_unlock(&local->iflist_mtx);
 	if (chg)
-		ieee80211_hw_config(local, chg);
+		__ieee80211_hw_config(local, chg, force);
+}
+
+void ieee80211_recalc_idle(struct ieee80211_local *local)
+{
+	ieee80211_recalc_idle_check(local, false);
+}
+
+void ieee80211_recalc_idle_force(struct ieee80211_local *local)
+
+{
+	ieee80211_recalc_idle_check(local, true);
 }
 
 static int netdev_notify(struct notifier_block *nb,
diff --git a/net/mac80211/pm.c b/net/mac80211/pm.c
index e373551..fc65b8d 100644
--- a/net/mac80211/pm.c
+++ b/net/mac80211/pm.c
@@ -98,6 +98,8 @@ int __ieee80211_suspend(struct ieee80211_hw *hw)
 	if (local->open_count)
 		ieee80211_stop_device(local);
 
+	ieee80211_recalc_idle_force(local);
+
 	local->suspended = true;
 	/* need suspended to be visible before quiescing is false */
 	barrier();
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index fb274db..30cd2a0 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -301,7 +301,7 @@ static void __ieee80211_scan_completed_finish(struct ieee80211_hw *hw,
 	}
 
 	mutex_lock(&local->mtx);
-	ieee80211_recalc_idle(local);
+	ieee80211_recalc_idle_force(local);
 	mutex_unlock(&local->mtx);
 
 	ieee80211_mlme_notify_scan_completed(local);
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index e497476..e5fb03e 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -1121,6 +1121,7 @@ void ieee80211_stop_device(struct ieee80211_local *local)
 
 	flush_workqueue(local->workqueue);
 	drv_stop(local);
+	ieee80211_recalc_idle_force(local);
 }
 
 int ieee80211_reconfig(struct ieee80211_local *local)
-- 
1.7.3.2.90.gd4c43

Re: [PATCH 5/5] mac80211: fix issuing idle calls when device open count is 0

[Johannes Berg]

On Mon, 2010-12-06 at 18:48 -0800, Luis R. Rodriguez wrote:
> There are a few places where mac80211 may issue a hw config
> but the hw config will be ignored and got into a black hole if
> the device count is already 0. Further hw config calls will also
> be discarded as the device is already marked as idle, in these
> cases mac80211 assumes the radio is idle but the driver never
> really got the notification. We need to ensure that places that
> will call a hw reconfig with open_count set to 0 will send the
> notification to the driver. This forces these checks in a few
> other key missing places.
> 
> Without this suspend and resume is broken on devices which require
> notifying the driver to become idle once the open_count already
> has reached 0. This fixes suspend/resume when using new DBus APIs
> are used which idle the device in places which had not yet gotten
> widely tested.
> 
> Cc: stable@kernel.org
> Cc: Paul Stewart <pstew@google.com>
> Cc: Amod Bodas <amod.bodas@atheros.com>
> Cc: Johannes Berg <johannes.berg@intel.com>
> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> ---
>  net/mac80211/ieee80211_i.h |    1 +
>  net/mac80211/iface.c       |   16 ++++++++++++++--
>  net/mac80211/pm.c          |    2 ++
>  net/mac80211/scan.c        |    2 +-
>  net/mac80211/util.c        |    1 +
>  5 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> index ae32349..78ab6ff 100644
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -1211,6 +1211,7 @@ void ieee80211_process_measurement_req(struct ieee80211_sub_if_data *sdata,
>  
>  /* Suspend/resume and hw reconfiguration */
>  int ieee80211_reconfig(struct ieee80211_local *local);
> +void ieee80211_recalc_idle_force(struct ieee80211_local *local);
>  void ieee80211_stop_device(struct ieee80211_local *local);
>  
>  #ifdef CONFIG_PM
> diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
> index 36b7000..a590bae 100644
> --- a/net/mac80211/iface.c
> +++ b/net/mac80211/iface.c
> @@ -1320,7 +1320,8 @@ u32 __ieee80211_recalc_idle(struct ieee80211_local *local)
>  	return 0;
>  }
>  
> -void ieee80211_recalc_idle(struct ieee80211_local *local)
> +static void ieee80211_recalc_idle_check(struct ieee80211_local *local,
> +					bool force)
>  {
>  	u32 chg;
>  
> @@ -1328,7 +1329,18 @@ void ieee80211_recalc_idle(struct ieee80211_local *local)
>  	chg = __ieee80211_recalc_idle(local);
>  	mutex_unlock(&local->iflist_mtx);
>  	if (chg)
> -		ieee80211_hw_config(local, chg);
> +		__ieee80211_hw_config(local, chg, force);
> +}
> +
> +void ieee80211_recalc_idle(struct ieee80211_local *local)
> +{
> +	ieee80211_recalc_idle_check(local, false);
> +}

I think I'd probably prefer an inline too.

> --- a/net/mac80211/pm.c
> +++ b/net/mac80211/pm.c
> @@ -98,6 +98,8 @@ int __ieee80211_suspend(struct ieee80211_hw *hw)
>  	if (local->open_count)
>  		ieee80211_stop_device(local);
>  
> +	ieee80211_recalc_idle_force(local);

Err? You just added a call to stop_device() too, and at the wrong place
as well...

> @@ -301,7 +301,7 @@ static void __ieee80211_scan_completed_finish(struct ieee80211_hw *hw,
>  	}
>  
>  	mutex_lock(&local->mtx);
> -	ieee80211_recalc_idle(local);
> +	ieee80211_recalc_idle_force(local);

Does this really occur afterwards closing interfaces?

> @@ -1121,6 +1121,7 @@ void ieee80211_stop_device(struct ieee80211_local *local)
>  
>  	flush_workqueue(local->workqueue);
>  	drv_stop(local);
> +	ieee80211_recalc_idle_force(local);

That one definitely can't be after drv_stop().

johannes

Re: [PATCH 5/5] mac80211: fix issuing idle calls when device open count is 0

[Paul Stewart]

On Tue, Dec 7, 2010 at 1:38 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2010-12-06 at 18:48 -0800, Luis R. Rodriguez wrote:
>> --- a/net/mac80211/pm.c
>> +++ b/net/mac80211/pm.c
>> @@ -301,7 +301,7 @@ static void __ieee80211_scan_completed_finish(struct ieee80211_hw *hw,
>>       }
>>
>>       mutex_lock(&local->mtx);
>> -     ieee80211_recalc_idle(local);
>> +     ieee80211_recalc_idle_force(local);
>
> Does this really occur afterwards closing interfaces?

It appears possible using wpa_supplicant with nl80211, at least using
the new DBus API.

--
Paul

Re: [PATCH 5/5] mac80211: fix issuing idle calls when device open count is 0

[Johannes Berg]

On Tue, 2010-12-07 at 07:20 -0800, Paul Stewart wrote:
> On Tue, Dec 7, 2010 at 1:38 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Mon, 2010-12-06 at 18:48 -0800, Luis R. Rodriguez wrote:
> >> --- a/net/mac80211/pm.c
> >> +++ b/net/mac80211/pm.c
> >> @@ -301,7 +301,7 @@ static void __ieee80211_scan_completed_finish(struct ieee80211_hw *hw,
> >>       }
> >>
> >>       mutex_lock(&local->mtx);
> >> -     ieee80211_recalc_idle(local);
> >> +     ieee80211_recalc_idle_force(local);
> >
> > Does this really occur afterwards closing interfaces?
> 
> It appears possible using wpa_supplicant with nl80211, at least using
> the new DBus API.

Actually I believe it happens _during_ ifdown, but when the running bit
is already cleared, so I guess this is about right -- although it
shouldn't matter since the recalc_idle in do_stop should catch it later.

johannes

Re: [PATCH 5/5] mac80211: fix issuing idle calls when device open count is 0

[Paul Stewart]

On Tue, Dec 7, 2010 at 7:23 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Tue, 2010-12-07 at 07:20 -0800, Paul Stewart wrote:
>> On Tue, Dec 7, 2010 at 1:38 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
>> > On Mon, 2010-12-06 at 18:48 -0800, Luis R. Rodriguez wrote:
>> >> --- a/net/mac80211/pm.c
>> >> +++ b/net/mac80211/pm.c
>> >> @@ -301,7 +301,7 @@ static void __ieee80211_scan_completed_finish(struct ieee80211_hw *hw,
>> >>       }
>> >>
>> >>       mutex_lock(&local->mtx);
>> >> -     ieee80211_recalc_idle(local);
>> >> +     ieee80211_recalc_idle_force(local);
>> >
>> > Does this really occur afterwards closing interfaces?
>>
>> It appears possible using wpa_supplicant with nl80211, at least using
>> the new DBus API.
>
> Actually I believe it happens _during_ ifdown, but when the running bit
> is already cleared, so I guess this is about right -- although it
> shouldn't matter since the recalc_idle in do_stop should catch it later.

So the scenario is as follows.  If you down an interface
ieee80211_do_stop() first decrements local->open_count, then later
calls __ieee80211_recalc_idle, et al.  This means that later in this
call graph if ieee80211_hw_config() finally gets called, open_count is
already 0 and therefore drv_config is never called.  Luis' reference
to scan_completed() above is probable in reference to older versions
of compat where ieee80211_stop() called ieee80211_scan_cancel()
directly.  My admittedly casual look at wireless-testing seems to
imply that path may not exist in current compat, although my tests
seem to indicate the same class of problem still exists.

--
Paul

Re: [PATCH 5/5] mac80211: fix issuing idle calls when device open count is 0

[Johannes Berg]

On Tue, 2010-12-07 at 09:28 -0800, Paul Stewart wrote:

> > Actually I believe it happens _during_ ifdown, but when the running bit
> > is already cleared, so I guess this is about right -- although it
> > shouldn't matter since the recalc_idle in do_stop should catch it later.
> 
> So the scenario is as follows.  If you down an interface
> ieee80211_do_stop() first decrements local->open_count, then later
> calls __ieee80211_recalc_idle, et al.  This means that later in this
> call graph if ieee80211_hw_config() finally gets called, open_count is
> already 0 and therefore drv_config is never called.  Luis' reference
> to scan_completed() above is probable in reference to older versions
> of compat where ieee80211_stop() called ieee80211_scan_cancel()
> directly.  My admittedly casual look at wireless-testing seems to
> imply that path may not exist in current compat, although my tests
> seem to indicate the same class of problem still exists.

Yeah I can see how a problem like this happens -- but I think the change
in scan_finished() is pointless because it'll be handled during
do_stop() anyway.

johannes

Re: [PATCH 5/5] mac80211: fix issuing idle calls when device open count is 0

[Luis R. Rodriguez]

On Tue, Dec 7, 2010 at 1:38 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2010-12-06 at 18:48 -0800, Luis R. Rodriguez wrote:
>> There are a few places where mac80211 may issue a hw config
>> but the hw config will be ignored and got into a black hole if
>> the device count is already 0. Further hw config calls will also
>> be discarded as the device is already marked as idle, in these
>> cases mac80211 assumes the radio is idle but the driver never
>> really got the notification. We need to ensure that places that
>> will call a hw reconfig with open_count set to 0 will send the
>> notification to the driver. This forces these checks in a few
>> other key missing places.
>>
>> Without this suspend and resume is broken on devices which require
>> notifying the driver to become idle once the open_count already
>> has reached 0. This fixes suspend/resume when using new DBus APIs
>> are used which idle the device in places which had not yet gotten
>> widely tested.
>>
>> Cc: stable@kernel.org
>> Cc: Paul Stewart <pstew@google.com>
>> Cc: Amod Bodas <amod.bodas@atheros.com>
>> Cc: Johannes Berg <johannes.berg@intel.com>
>> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
>> ---
>>  net/mac80211/ieee80211_i.h |    1 +
>>  net/mac80211/iface.c       |   16 ++++++++++++++--
>>  net/mac80211/pm.c          |    2 ++
>>  net/mac80211/scan.c        |    2 +-
>>  net/mac80211/util.c        |    1 +
>>  5 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
>> index ae32349..78ab6ff 100644
>> --- a/net/mac80211/ieee80211_i.h
>> +++ b/net/mac80211/ieee80211_i.h
>> @@ -1211,6 +1211,7 @@ void ieee80211_process_measurement_req(struct ieee80211_sub_if_data *sdata,
>>
>>  /* Suspend/resume and hw reconfiguration */
>>  int ieee80211_reconfig(struct ieee80211_local *local);
>> +void ieee80211_recalc_idle_force(struct ieee80211_local *local);
>>  void ieee80211_stop_device(struct ieee80211_local *local);
>>
>>  #ifdef CONFIG_PM
>> diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
>> index 36b7000..a590bae 100644
>> --- a/net/mac80211/iface.c
>> +++ b/net/mac80211/iface.c
>> @@ -1320,7 +1320,8 @@ u32 __ieee80211_recalc_idle(struct ieee80211_local *local)
>>       return 0;
>>  }
>>
>> -void ieee80211_recalc_idle(struct ieee80211_local *local)
>> +static void ieee80211_recalc_idle_check(struct ieee80211_local *local,
>> +                                     bool force)
>>  {
>>       u32 chg;
>>
>> @@ -1328,7 +1329,18 @@ void ieee80211_recalc_idle(struct ieee80211_local *local)
>>       chg = __ieee80211_recalc_idle(local);
>>       mutex_unlock(&local->iflist_mtx);
>>       if (chg)
>> -             ieee80211_hw_config(local, chg);
>> +             __ieee80211_hw_config(local, chg, force);
>> +}
>> +
>> +void ieee80211_recalc_idle(struct ieee80211_local *local)
>> +{
>> +     ieee80211_recalc_idle_check(local, false);
>> +}
>
> I think I'd probably prefer an inline too.

OK

>> --- a/net/mac80211/pm.c
>> +++ b/net/mac80211/pm.c
>> @@ -98,6 +98,8 @@ int __ieee80211_suspend(struct ieee80211_hw *hw)
>>       if (local->open_count)
>>               ieee80211_stop_device(local);
>>
>> +     ieee80211_recalc_idle_force(local);
>
> Err? You just added a call to stop_device() too,

What if open_count is 0?

> and at the wrong place as well...

I'll fix thanks.


>> @@ -1121,6 +1121,7 @@ void ieee80211_stop_device(struct ieee80211_local *local)
>>
>>       flush_workqueue(local->workqueue);
>>       drv_stop(local);
>> +     ieee80211_recalc_idle_force(local);
>
> That one definitely can't be after drv_stop().

OK why though?

 Luis

Re: [PATCH 5/5] mac80211: fix issuing idle calls when device open count is 0

[Johannes Berg]

On Tue, 2010-12-07 at 07:44 -0800, Luis R. Rodriguez wrote:

> >>       drv_stop(local);
> >> +     ieee80211_recalc_idle_force(local);
> >
> > That one definitely can't be after drv_stop().
> 
> OK why though?

Because you can't call into drivers after you stop them? Shouldn't this
be obvious?

johannes

Re: [PATCH 5/5] mac80211: fix issuing idle calls when device open count is 0

[Luis R. Rodriguez]

On Tue, Dec 7, 2010 at 7:48 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Tue, 2010-12-07 at 07:44 -0800, Luis R. Rodriguez wrote:
>
>> >>       drv_stop(local);
>> >> +     ieee80211_recalc_idle_force(local);
>> >
>> > That one definitely can't be after drv_stop().
>>
>> OK why though?
>
> Because you can't call into drivers after you stop them? Shouldn't this
> be obvious?

That makes sense, but mac80211 is the one who tells the driver when
its idle or non-idle, if we stop the device do we want it to handle an
idle check itself? This change would only force out an idle change
notification, nothing else.

  Luis

Re: [PATCH 5/5] mac80211: fix issuing idle calls when device open count is 0

[Johannes Berg]

On Tue, 2010-12-07 at 07:51 -0800, Luis R. Rodriguez wrote:

> >> >>       drv_stop(local);
> >> >> +     ieee80211_recalc_idle_force(local);

> > Because you can't call into drivers after you stop them? Shouldn't this
> > be obvious?
> 
> That makes sense, but mac80211 is the one who tells the driver when
> its idle or non-idle, if we stop the device do we want it to handle an
> idle check itself? This change would only force out an idle change
> notification, nothing else.

I don't get what you're saying at all. Can you look at the code lines I
quoted again?

johannes

Re: [PATCH 5/5] mac80211: fix issuing idle calls when device open count is 0

[Luis R. Rodriguez]

On Tue, Dec 07, 2010 at 07:54:58AM -0800, Johannes Berg wrote:
> On Tue, 2010-12-07 at 07:51 -0800, Luis R. Rodriguez wrote:
> 
> > >> >>       drv_stop(local);
> > >> >> +     ieee80211_recalc_idle_force(local);
> 
> > > Because you can't call into drivers after you stop them? Shouldn't this
> > > be obvious?
> > 
> > That makes sense, but mac80211 is the one who tells the driver when
> > its idle or non-idle, if we stop the device do we want it to handle an
> > idle check itself? This change would only force out an idle change
> > notification, nothing else.
> 
> I don't get what you're saying at all. Can you look at the code lines I
> quoted again?

Yes, I did. Anyway here is a v2 with the changes you outlined incorporated.

>From d8983b423b24037e8e847fa45778d9313af3e057 Mon Sep 17 00:00:00 2001
From: Luis R. Rodriguez <lrodriguez@atheros.com>
Date: Mon, 6 Dec 2010 18:04:03 -0800
Subject: [PATCH v2] mac80211: fix issuing idle calls when device open count is 0

There are a few places where mac80211 may issue a hw config
but the hw config will be ignored and got into a black hole if
the device count is already 0. Further hw config calls will also
be discarded as the device is already marked as idle, in these
cases mac80211 assumes the radio is idle but the driver never
really got the notification. We need to ensure that places that
will call a hw reconfig with open_count set to 0 will send the
notification to the driver. This forces these checks in a few
other key missing places.

Without this suspend and resume is broken on devices which require
notifying the driver to become idle once the open_count already
has reached 0. This fixes suspend/resume when using new DBus APIs
are used which idle the device in places which had not yet gotten
widely tested.

Cc: stable@kernel.org
Cc: Paul Stewart <pstew@google.com>
Cc: Amod Bodas <amod.bodas@atheros.com>
Cc: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/mac80211/ieee80211_i.h |   16 +++++++++++++++-
 net/mac80211/iface.c       |    4 ++--
 net/mac80211/scan.c        |    2 +-
 net/mac80211/util.c        |    1 +
 4 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index ae32349..873ec88 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1132,8 +1132,21 @@ int ieee80211_if_change_type(struct ieee80211_sub_if_data *sdata,
 			     enum nl80211_iftype type);
 void ieee80211_if_remove(struct ieee80211_sub_if_data *sdata);
 void ieee80211_remove_interfaces(struct ieee80211_local *local);
+
 u32 __ieee80211_recalc_idle(struct ieee80211_local *local);
-void ieee80211_recalc_idle(struct ieee80211_local *local);
+void ieee80211_recalc_idle_check(struct ieee80211_local *local, bool force);
+
+static inline void ieee80211_recalc_idle(struct ieee80211_local *local)
+{
+	ieee80211_recalc_idle_check(local, false);
+}
+
+static inline void ieee80211_recalc_idle_force(struct ieee80211_local *local)
+
+{
+	ieee80211_recalc_idle_check(local, true);
+}
+
 void ieee80211_adjust_monitor_flags(struct ieee80211_sub_if_data *sdata,
 				    const int offset);
 
@@ -1211,6 +1224,7 @@ void ieee80211_process_measurement_req(struct ieee80211_sub_if_data *sdata,
 
 /* Suspend/resume and hw reconfiguration */
 int ieee80211_reconfig(struct ieee80211_local *local);
+void ieee80211_recalc_idle_force(struct ieee80211_local *local);
 void ieee80211_stop_device(struct ieee80211_local *local);
 
 #ifdef CONFIG_PM
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 36b7000..2803a7a 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1320,7 +1320,7 @@ u32 __ieee80211_recalc_idle(struct ieee80211_local *local)
 	return 0;
 }
 
-void ieee80211_recalc_idle(struct ieee80211_local *local)
+void ieee80211_recalc_idle_check(struct ieee80211_local *local, bool force)
 {
 	u32 chg;
 
@@ -1328,7 +1328,7 @@ void ieee80211_recalc_idle(struct ieee80211_local *local)
 	chg = __ieee80211_recalc_idle(local);
 	mutex_unlock(&local->iflist_mtx);
 	if (chg)
-		ieee80211_hw_config(local, chg);
+		__ieee80211_hw_config(local, chg, force);
 }
 
 static int netdev_notify(struct notifier_block *nb,
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index fb274db..30cd2a0 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -301,7 +301,7 @@ static void __ieee80211_scan_completed_finish(struct ieee80211_hw *hw,
 	}
 
 	mutex_lock(&local->mtx);
-	ieee80211_recalc_idle(local);
+	ieee80211_recalc_idle_force(local);
 	mutex_unlock(&local->mtx);
 
 	ieee80211_mlme_notify_scan_completed(local);
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index e497476..a4184eb 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -1120,6 +1120,7 @@ void ieee80211_stop_device(struct ieee80211_local *local)
 	cancel_work_sync(&local->reconfig_filter);
 
 	flush_workqueue(local->workqueue);
+	ieee80211_recalc_idle_force(local);
 	drv_stop(local);
 }
 
-- 
1.7.3.2.90.gd4c43

Re: [PATCH 5/5] mac80211: fix issuing idle calls when device open count is 0

[Johannes Berg]

On Tue, 2010-12-07 at 07:59 -0800, Luis R. Rodriguez wrote:

> @@ -301,7 +301,7 @@ static void __ieee80211_scan_completed_finish(struct ieee80211_hw *hw,
>  	}
>  
>  	mutex_lock(&local->mtx);
> -	ieee80211_recalc_idle(local);
> +	ieee80211_recalc_idle_force(local);
>  	mutex_unlock(&local->mtx);

This is the change that I don't think is necessary.

johannes

Re: [PATCH 5/5] mac80211: fix issuing idle calls when device open count is 0

[Luis R. Rodriguez]

On Tue, Dec 7, 2010 at 9:32 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Tue, 2010-12-07 at 07:59 -0800, Luis R. Rodriguez wrote:
>
>> @@ -301,7 +301,7 @@ static void __ieee80211_scan_completed_finish(struct ieee80211_hw *hw,
>>       }
>>
>>       mutex_lock(&local->mtx);
>> -     ieee80211_recalc_idle(local);
>> +     ieee80211_recalc_idle_force(local);
>>       mutex_unlock(&local->mtx);
>
> This is the change that I don't think is necessary.

Without this resume fails.

  Luis

Re: [PATCH 5/5] mac80211: fix issuing idle calls when device open count is 0

[Johannes Berg]

On Tue, 2010-12-07 at 10:04 -0800, Luis R. Rodriguez wrote:
> On Tue, Dec 7, 2010 at 9:32 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Tue, 2010-12-07 at 07:59 -0800, Luis R. Rodriguez wrote:
> >
> >> @@ -301,7 +301,7 @@ static void __ieee80211_scan_completed_finish(struct ieee80211_hw *hw,
> >>       }
> >>
> >>       mutex_lock(&local->mtx);
> >> -     ieee80211_recalc_idle(local);
> >> +     ieee80211_recalc_idle_force(local);
> >>       mutex_unlock(&local->mtx);
> >
> > This is the change that I don't think is necessary.
> 
> Without this resume fails.

In what situation? When you just suspend while things are up&running, or
if you suspend with interfaces down? It doesn't make sense anyway, so
something's going on in the rest of the scan code -- we should be
canceling scans properly when going down and when suspending, well
before any of this becomes relevant.

johannes

Re: [PATCH 5/5] mac80211: fix issuing idle calls when device open count is 0

[Luis R. Rodriguez]

On Tue, Dec 7, 2010 at 10:12 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Tue, 2010-12-07 at 10:04 -0800, Luis R. Rodriguez wrote:
>> On Tue, Dec 7, 2010 at 9:32 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
>> > On Tue, 2010-12-07 at 07:59 -0800, Luis R. Rodriguez wrote:
>> >
>> >> @@ -301,7 +301,7 @@ static void __ieee80211_scan_completed_finish(struct ieee80211_hw *hw,
>> >>       }
>> >>
>> >>       mutex_lock(&local->mtx);
>> >> -     ieee80211_recalc_idle(local);
>> >> +     ieee80211_recalc_idle_force(local);
>> >>       mutex_unlock(&local->mtx);
>> >
>> > This is the change that I don't think is necessary.
>>
>> Without this resume fails.
>
> In what situation? When you just suspend while things are up&running, or
> if you suspend with interfaces down?

I believe Paul was suspending when the interface is up and running.

> It doesn't make sense anyway, so
> something's going on in the rest of the scan code -- we should be
> canceling scans properly when going down and when suspending, well
> before any of this becomes relevant.

The issue might be we race to stop the device prior to canceling a
scan. Do you see that being possible?

  Luis

Re: [PATCH 5/5] mac80211: fix issuing idle calls when device open count is 0

[Johannes Berg]

On Tue, 2010-12-07 at 12:49 -0800, Luis R. Rodriguez wrote:

> > In what situation? When you just suspend while things are up&running, or
> > if you suspend with interfaces down?
> 
> I believe Paul was suspending when the interface is up and running.

Ok.

> > It doesn't make sense anyway, so
> > something's going on in the rest of the scan code -- we should be
> > canceling scans properly when going down and when suspending, well
> > before any of this becomes relevant.
> 
> The issue might be we race to stop the device prior to canceling a
> scan. Do you see that being possible?

Not really, since we do scan_cancel() within suspend()... I guess I'd
like to find out why that doesn't do what I think it does.

johannes

Re: [PATCH 5/5] mac80211: fix issuing idle calls when device open count is 0

[Luis R. Rodriguez]

On Tue, Dec 7, 2010 at 12:54 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Tue, 2010-12-07 at 12:49 -0800, Luis R. Rodriguez wrote:
>
>> > In what situation? When you just suspend while things are up&running, or
>> > if you suspend with interfaces down?
>>
>> I believe Paul was suspending when the interface is up and running.
>
> Ok.
>
>> > It doesn't make sense anyway, so
>> > something's going on in the rest of the scan code -- we should be
>> > canceling scans properly when going down and when suspending, well
>> > before any of this becomes relevant.
>>
>> The issue might be we race to stop the device prior to canceling a
>> scan. Do you see that being possible?
>
> Not really, since we do scan_cancel() within suspend()... I guess I'd
> like to find out why that doesn't do what I think it does.

Well lets see, this is only reproducible with the new DBUS APIs, and
frankly I am not sure what these things do. Is it possible we have a
race between how it brings down an interface for suspend and canceling
a scan?

  Luis

Re: [PATCH 5/5] mac80211: fix issuing idle calls when device open count is 0

[Johannes Berg]

On Tue, 2010-12-07 at 12:56 -0800, Luis R. Rodriguez wrote:

> > Not really, since we do scan_cancel() within suspend()... I guess I'd
> > like to find out why that doesn't do what I think it does.
> 
> Well lets see, this is only reproducible with the new DBUS APIs, and
> frankly I am not sure what these things do. Is it possible we have a
> race between how it brings down an interface for suspend and canceling
> a scan?

But both suspend and do_stop should run under RTNL.

johannes

Re: [PATCH 5/5] mac80211: fix issuing idle calls when device open count is 0

[Luis R. Rodriguez]

On Tue, Dec 07, 2010 at 12:57:52PM -0800, Johannes Berg wrote:
> On Tue, 2010-12-07 at 12:56 -0800, Luis R. Rodriguez wrote:
> 
> > > Not really, since we do scan_cancel() within suspend()... I guess I'd
> > > like to find out why that doesn't do what I think it does.
> > 
> > Well lets see, this is only reproducible with the new DBUS APIs, and
> > frankly I am not sure what these things do. Is it possible we have a
> > race between how it brings down an interface for suspend and canceling
> > a scan?
> 
> But both suspend and do_stop should run under RTNL.

Paul, can you try this instead of these two patches. What this
does is it assumes mac80211 will tell us that stop() implies
to disable the radio as explained in the documentation and that
any further operations will not happen, and that idle settings
prior to mac80211 issuing a start() callback are the responsibility
of the driver to ensure integrity on. Because of this we ensure
idle on stop() as no other interfaces are possible enabled and
we also ensure it to be idle on resume.

diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 97ddb32..eb98f7f 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1337,8 +1337,8 @@ static void ath9k_stop(struct ieee80211_hw *hw)
 
 	ath9k_ps_restore(sc);
 
-	/* Finally, put the chip in FULL SLEEP mode */
-	ath9k_setpower(sc, ATH9K_PM_FULL_SLEEP);
+	sc->ps_idle = true;
+	ath_radio_disable(sc, hw);
 
 	sc->sc_flags |= SC_OP_INVALID;
 
diff --git a/drivers/net/wireless/ath/ath9k/pci.c b/drivers/net/wireless/ath/ath9k/pci.c
index 09f69a9..47334f3 100644
--- a/drivers/net/wireless/ath/ath9k/pci.c
+++ b/drivers/net/wireless/ath/ath9k/pci.c
@@ -297,6 +297,9 @@ static int ath_pci_resume(struct device *device)
 			    AR_GPIO_OUTPUT_MUX_AS_OUTPUT);
 	ath9k_hw_set_gpio(sc->sc_ah, sc->sc_ah->led_pin, 1);
 
+	sc->ps_idle = true;
+	ath_radio_disable(sc, hw);
+
 	return 0;
 }