[PATCH] mac80211: Push idle state to driver before stop

[PATCH] mac80211: Push idle state to driver before stop

[Paul Stewart]

Sometimes mac80211 doesn't push idle state downwards to the
driver.  Specifically, there are times in some functions where
the "local->hw.conf.flags & IEEE80211_CONF_IDLE" may change but
the equivalent of "drv_config(local, IEEE80211_CONF_CHANGE_IDLE)"
does not end up being called.  This is usually not all that
problematic except, for example, suspending and resuming an idle
ath9k device.  If the device isn't marked idle when
ieee80211_stop_device() is called, the device never gets put to
sleep, and will end up in an unresponsive state on resume.

As a precaution, explicitly call drv_config() before
ieee80211_stop_device(), which should be a no-op under normal
circumstances, but where this problem arises, it will shut down
the ath9k where necessary.

One example where this problem occurs is when I down an interface
while a scan is in progress on ath9k.  If the device was not shut
down correctly and the system suspends and resumes repeatedly with
ath9k, I end up with a fatal register read error (0x7000/deadbeef)
when trying to bring the interface back up.

Signed-off-by: Paul Stewart <pstew@google.com>
---
 net/mac80211/iface.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index b6db237..5af5a89 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -536,6 +536,7 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 		if (local->ops->napi_poll)
 			napi_disable(&local->napi);
 		ieee80211_clear_tx_pending(local);
+		drv_config(local, IEEE80211_CONF_CHANGE_IDLE);
 		ieee80211_stop_device(local);
 
 		/* no reconfiguring after stop! */
-- 
1.7.3.1

Re: [PATCH] mac80211: Push idle state to driver before stop

[Johannes Berg]

On Wed, 2010-12-15 at 10:54 -0800, Paul Stewart wrote:
> Sometimes mac80211 doesn't push idle state downwards to the
> driver.  Specifically, there are times in some functions where
> the "local->hw.conf.flags & IEEE80211_CONF_IDLE" may change but
> the equivalent of "drv_config(local, IEEE80211_CONF_CHANGE_IDLE)"
> does not end up being called.  This is usually not all that
> problematic except, for example, suspending and resuming an idle
> ath9k device.  If the device isn't marked idle when
> ieee80211_stop_device() is called, the device never gets put to
> sleep, and will end up in an unresponsive state on resume.

I thought this was solved in ath9k differently? Was that somehow
inadequate?

> As a precaution, explicitly call drv_config() before
> ieee80211_stop_device(), which should be a no-op under normal
> circumstances, but where this problem arises, it will shut down
> the ath9k where necessary.

I'm not convinced that this makes a lot of sense. I'm sure you can come
up with other scenarios in which we stop a device and expect it to come
back. I'm not sure why mac80211 should be responsible for all this?

> One example where this problem occurs is when I down an interface
> while a scan is in progress on ath9k.  If the device was not shut
> down correctly and the system suspends and resumes repeatedly with
> ath9k, I end up with a fatal register read error (0x7000/deadbeef)
> when trying to bring the interface back up.
> 
> Signed-off-by: Paul Stewart <pstew@google.com>
> ---
>  net/mac80211/iface.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
> index b6db237..5af5a89 100644
> --- a/net/mac80211/iface.c
> +++ b/net/mac80211/iface.c
> @@ -536,6 +536,7 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
>  		if (local->ops->napi_poll)
>  			napi_disable(&local->napi);
>  		ieee80211_clear_tx_pending(local);
> +		drv_config(local, IEEE80211_CONF_CHANGE_IDLE);
>  		ieee80211_stop_device(local);
>  
>  		/* no reconfiguring after stop! */

I don't like this anyway -- you're calling a driver callback directly,
and there's no guarantee that just that particular change will be used
by the driver -- it's free to look at other state given to it in each
callback as well. That seems problematic because of all the other logic
that we have in ieee80211_hw_config().

At the very least I think you need to provide a better rationale for
this patch and explain why the ath9k change isn't sufficient?

johannes

Re: [PATCH] mac80211: Push idle state to driver before stop

[Paul Stewart]

On Wed, Dec 15, 2010 at 10:59 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2010-12-15 at 10:54 -0800, Paul Stewart wrote:
>> Sometimes mac80211 doesn't push idle state downwards to the
>> driver.  Specifically, there are times in some functions where
>> the "local->hw.conf.flags & IEEE80211_CONF_IDLE" may change but
>> the equivalent of "drv_config(local, IEEE80211_CONF_CHANGE_IDLE)"
>> does not end up being called.  This is usually not all that
>> problematic except, for example, suspending and resuming an idle
>> ath9k device.  If the device isn't marked idle when
>> ieee80211_stop_device() is called, the device never gets put to
>> sleep, and will end up in an unresponsive state on resume.
>
> I thought this was solved in ath9k differently? Was that somehow
> inadequate?

Unfortunately previous attempts to solve this problem failed.

>> As a precaution, explicitly call drv_config() before
>> ieee80211_stop_device(), which should be a no-op under normal
>> circumstances, but where this problem arises, it will shut down
>> the ath9k where necessary.
>
> I'm not convinced that this makes a lot of sense. I'm sure you can come
> up with other scenarios in which we stop a device and expect it to come
> back. I'm not sure why mac80211 should be responsible for all this?
>
>> One example where this problem occurs is when I down an interface
>> while a scan is in progress on ath9k.  If the device was not shut
>> down correctly and the system suspends and resumes repeatedly with
>> ath9k, I end up with a fatal register read error (0x7000/deadbeef)
>> when trying to bring the interface back up.
>>
>> Signed-off-by: Paul Stewart <pstew@google.com>
>> ---
>>  net/mac80211/iface.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
>> index b6db237..5af5a89 100644
>> --- a/net/mac80211/iface.c
>> +++ b/net/mac80211/iface.c
>> @@ -536,6 +536,7 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
>>               if (local->ops->napi_poll)
>>                       napi_disable(&local->napi);
>>               ieee80211_clear_tx_pending(local);
>> +             drv_config(local, IEEE80211_CONF_CHANGE_IDLE);
>>               ieee80211_stop_device(local);
>>
>>               /* no reconfiguring after stop! */
>
> I don't like this anyway -- you're calling a driver callback directly,
> and there's no guarantee that just that particular change will be used
> by the driver -- it's free to look at other state given to it in each
> callback as well. That seems problematic because of all the other logic
> that we have in ieee80211_hw_config().
>
> At the very least I think you need to provide a better rationale for
> this patch and explain why the ath9k change isn't sufficient?

Here's a slightly better rationale, as I'm getting to understand the problem.
ieee80211_do_stop() decrements "open_count" so early during the function
that if the device is not idle at this point (e.g, due to scanning),
drv_config()
will never be called on the the lower-level driver, since ieee80211_hw_config()
tests for "open_count > 0" and silently returns no-error.

--
Paul

Re: [PATCH] mac80211: Push idle state to driver before stop

[Johannes Berg]

On Thu, 2010-12-16 at 03:17 -0800, Paul Stewart wrote:

> > I thought this was solved in ath9k differently? Was that somehow
> > inadequate?
> 
> Unfortunately previous attempts to solve this problem failed.

I thought ath9k was fixed to simply go idle when stopped? Why didn't
that help?

> Here's a slightly better rationale, as I'm getting to understand the problem.
> ieee80211_do_stop() decrements "open_count" so early during the function
> that if the device is not idle at this point (e.g, due to scanning),
> drv_config()
> will never be called on the the lower-level driver, since ieee80211_hw_config()
> tests for "open_count > 0" and silently returns no-error.

Yes, I realise that -- but also Luis said this change wasn't sufficient
in his tests, he also needed something in scan code itself. Also, this
is really quite intentional if you read the comments around the area
you're modifying -- mac80211 assumes that after it shuts down the device
with stop(), the device will be powered off under control of the driver
until start().

I just don't think it'll be easy to actually make sure that we always go
IDLE before calling stop(). This patch might solve part of the problem,
but what about hardware scan, that might have to be stopped by the
driver during stop()? Similar things might come up with other features.

Therefore I'm not sure we should even try. If we stop() the hardware,
what's preventing the driver from doing whatever it needs to put the
device into a low power state?

johannes

Re: [PATCH] mac80211: Push idle state to driver before stop

[Paul Stewart]

On Thu, Dec 16, 2010 at 3:30 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2010-12-16 at 03:17 -0800, Paul Stewart wrote:
>
>> > I thought this was solved in ath9k differently? Was that somehow
>> > inadequate?
>>
>> Unfortunately previous attempts to solve this problem failed.
>
> I thought ath9k was fixed to simply go idle when stopped? Why didn't
> that help?

So, I'm trying to balance two fairly nasty problems here.  On one end
we have the problem before any changes where the device is not shut
down when it legitimately should be -- we ifdown the device while scanning
but the device in ath9k is not set to "idle".  Fixing the problem in ath9k
alone causes a separate problem.  If you stop the driver from mac80211's
pm.c, ath9k would set the device ps_idle, when mac80211 does not
consider the device to to be idle.  Thus, when we resume from suspend,
the device stays quiescent (mac80211 has no reason to believe it needs
to do anything special to be able to tx, receive beacons, etc.)

>> Here's a slightly better rationale, as I'm getting to understand the problem.
>> ieee80211_do_stop() decrements "open_count" so early during the function
>> that if the device is not idle at this point (e.g, due to scanning),
>> drv_config()
>> will never be called on the the lower-level driver, since ieee80211_hw_config()
>> tests for "open_count > 0" and silently returns no-error.
>
> Yes, I realise that -- but also Luis said this change wasn't sufficient
> in his tests, he also needed something in scan code itself. Also, this
> is really quite intentional if you read the comments around the area
> you're modifying -- mac80211 assumes that after it shuts down the device
> with stop(), the device will be powered off under control of the driver
> until start().
>
> I just don't think it'll be easy to actually make sure that we always go
> IDLE before calling stop(). This patch might solve part of the problem,
> but what about hardware scan, that might have to be stopped by the
> driver during stop()? Similar things might come up with other features.
>
> Therefore I'm not sure we should even try. If we stop() the hardware,
> what's preventing the driver from doing whatever it needs to put the
> device into a low power state?
>
> johannes
>
>

Re: [PATCH] mac80211: Push idle state to driver before stop

[Johannes Berg]

On Thu, 2010-12-16 at 03:51 -0800, Paul Stewart wrote:

> So, I'm trying to balance two fairly nasty problems here.  On one end
> we have the problem before any changes where the device is not shut
> down when it legitimately should be -- we ifdown the device while scanning
> but the device in ath9k is not set to "idle".

See, I contend that statement -- we actually *turn off* the device with
stop(), so it shouldn't matter.

> Fixing the problem in ath9k
> alone causes a separate problem.  If you stop the driver from mac80211's
> pm.c, ath9k would set the device ps_idle, when mac80211 does not
> consider the device to to be idle.  Thus, when we resume from suspend,
> the device stays quiescent (mac80211 has no reason to believe it needs
> to do anything special to be able to tx, receive beacons, etc.)

How so? After resume, mac80211 will invoke start(), add interfaces and
stations back, and then invoke drv_config with all flags in "changed"
set. Therefore, at this point, the device should be reset. Where's this
not working?

I think the problem might be that ath9k is expecting mac80211 to
perfectly nest calls to idle/non-idle, even across suspend/resume and
device shutdown. I don't think that's feasible at all.

johannes

Re: [PATCH] mac80211: Push idle state to driver before stop

[Paul Stewart]

On Thu, Dec 16, 2010 at 4:04 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2010-12-16 at 03:51 -0800, Paul Stewart wrote:
>> Fixing the problem in ath9k
>> alone causes a separate problem.  If you stop the driver from mac80211's
>> pm.c, ath9k would set the device ps_idle, when mac80211 does not
>> consider the device to to be idle.  Thus, when we resume from suspend,
>> the device stays quiescent (mac80211 has no reason to believe it needs
>> to do anything special to be able to tx, receive beacons, etc.)
>
> How so? After resume, mac80211 will invoke start(), add interfaces and
> stations back, and then invoke drv_config with all flags in "changed"
> set. Therefore, at this point, the device should be reset. Where's this
> not working?

I haven't dug deep into it, but I can guess at a reason -- ath9k stores idle
state in two different places -- one is meant to mirror mac80211's state,
and the other one is internal, periodically computed from the state of all
wiphys. The ath9k version of the fix modified the
internal state without the mac80211 mirror. The ath9k config() routine
probably does nothing when called with "all changed" on resume because
in fact, if we suspend and resume when non-idle, nothing _has_ changed
from that perspective.  I fear that unless ath9k gets changed more
substantially, it really does need to be informed of IDLE changes.

> I think the problem might be that ath9k is expecting mac80211 to
> perfectly nest calls to idle/non-idle, even across suspend/resume and
> device shutdown. I don't think that's feasible at all.

--
Paul

Re: [PATCH] mac80211: Push idle state to driver before stop

[Johannes Berg]

On Thu, 2010-12-16 at 04:27 -0800, Paul Stewart wrote:

> > How so? After resume, mac80211 will invoke start(), add interfaces and
> > stations back, and then invoke drv_config with all flags in "changed"
> > set. Therefore, at this point, the device should be reset. Where's this
> > not working?
> 
> I haven't dug deep into it, but I can guess at a reason -- ath9k stores idle
> state in two different places -- one is meant to mirror mac80211's state,
> and the other one is internal, periodically computed from the state of all
> wiphys. The ath9k version of the fix modified the
> internal state without the mac80211 mirror. 

But at this point mac80211 doesn't care what the state is any more.

> The ath9k config() routine
> probably does nothing when called with "all changed" on resume because
> in fact, if we suspend and resume when non-idle, nothing _has_ changed
> from that perspective.

Hmm, I still don't get it. The "idle just changed" flag is set -- and
idle will actually be what it was at suspend time. If ath9k was simply
setting its own idle state on stop(), and returning to what mac80211
says when it first configures the device after start(), what would the
problem be?

>   I fear that unless ath9k gets changed more
> substantially, it really does need to be informed of IDLE changes.

I'd rather have ath9k change more than try to enforce this perfectly in
mac80211. I still think that's brittle, and every little bug in a corner
case will cause severe problems since it causes suspend/resume to fail.

johannes

Re: [PATCH] mac80211: Push idle state to driver before stop

[Paul Stewart]

On Thu, Dec 16, 2010 at 4:35 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2010-12-16 at 04:27 -0800, Paul Stewart wrote:
>
>> > How so? After resume, mac80211 will invoke start(), add interfaces and
>> > stations back, and then invoke drv_config with all flags in "changed"
>> > set. Therefore, at this point, the device should be reset. Where's this
>> > not working?
>>
>> I haven't dug deep into it, but I can guess at a reason -- ath9k stores idle
>> state in two different places -- one is meant to mirror mac80211's state,
>> and the other one is internal, periodically computed from the state of all
>> wiphys. The ath9k version of the fix modified the
>> internal state without the mac80211 mirror.
>
> But at this point mac80211 doesn't care what the state is any more.

Huh?  I'm not sure I understand this statement.  If mac80211 suspends
and resumes with open_count > 1 (e.g, with an associated STA), I argue
mac80211 _does_ care what the state is.  In fact, although we called
stop() on the driver, there's every expectation that on resume any state
stored on suspend is recovered.  Are you not assuming this?

>> The ath9k config() routine
>> probably does nothing when called with "all changed" on resume because
>> in fact, if we suspend and resume when non-idle, nothing _has_ changed
>> from that perspective.
>
> Hmm, I still don't get it. The "idle just changed" flag is set -- and
> idle will actually be what it was at suspend time. If ath9k was simply
> setting its own idle state on stop(), and returning to what mac80211
> says when it first configures the device after start(), what would the
> problem be?

I don't have the code in front of me (in transit at the moment), but I'm
pretty sure the config routine in ath9k only checks to see whether the
idle state mac80211 passes in is different from the previous passed-in
value. It doesn't care what its internal state is because it believed it was
computed from mac80211 passed-in values.  It may be possible to
rework that code.

>>   I fear that unless ath9k gets changed more
>> substantially, it really does need to be informed of IDLE changes.
>
> I'd rather have ath9k change more than try to enforce this perfectly in
> mac80211. I still think that's brittle, and every little bug in a corner
> case will cause severe problems since it causes suspend/resume to fail.
>
> johannes
>
>

Re: [PATCH] mac80211: Push idle state to driver before stop

[Johannes Berg]

On Thu, 2010-12-16 at 04:46 -0800, Paul Stewart wrote:

> >> I haven't dug deep into it, but I can guess at a reason -- ath9k stores idle
> >> state in two different places -- one is meant to mirror mac80211's state,
> >> and the other one is internal, periodically computed from the state of all
> >> wiphys. The ath9k version of the fix modified the
> >> internal state without the mac80211 mirror.
> >
> > But at this point mac80211 doesn't care what the state is any more.
> 
> Huh?  I'm not sure I understand this statement.  If mac80211 suspends
> and resumes with open_count > 1 (e.g, with an associated STA), I argue
> mac80211 _does_ care what the state is.  In fact, although we called
> stop() on the driver, there's every expectation that on resume any state
> stored on suspend is recovered.  Are you not assuming this?

Not really -- the driver may throw away all internal state, mac80211
will (attempt to) restore it all through drv_config() with changed = ~0.
Evidently ath9k has some magic that makes this fail?

johannes

Re: [PATCH] mac80211: Push idle state to driver before stop

[Paul Stewart]

On Thu, Dec 16, 2010 at 4:53 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2010-12-16 at 04:46 -0800, Paul Stewart wrote:
> Not really -- the driver may throw away all internal state, mac80211
> will (attempt to) restore it all through drv_config() with changed = ~0.
> Evidently ath9k has some magic that makes this fail?

The magic in question is as follows (ath9k_config()):

    if (changed & IEEE80211_CONF_CHANGE_IDLE) {
        bool enable_radio;
        bool all_wiphys_idle;
        bool idle = !!(conf->flags & IEEE80211_CONF_IDLE);

        spin_lock_bh(&sc->wiphy_lock);
        all_wiphys_idle =  ath9k_all_wiphys_idle(sc);
       ath9k_set_wiphy_idle(aphy, idle);

        enable_radio = (!idle && all_wiphys_idle);

        /*
         * After we unlock here its possible another wiphy
         * can be re-renabled so to account for that we will
         * only disable the radio toward the end of this routine
         * if by then all wiphys are still idle.
         */
        spin_unlock_bh(&sc->wiphy_lock);

        if (enable_radio) {
            sc->ps_idle = false;
            ath_radio_enable(sc, hw);
            ath_dbg(common, ATH_DBG_CONFIG,
                    "not-idle: enabling radio\n");
        }
    }

So, ath9k enables and disables the radio based on ps_idle (the
internal state), but that's not re-evaluated unless the wiphys
actually change.  In this case they do not change because ath9k was
never informed that the wiphy went idle (all_wiphys_idle evaluates to
false).  One solution would be to do an ah9k_set_wiphy_idle(aphy,
true) in ath9k_stop() instead of Luis' previous suggestion of setting
sc->ps_idle directly.  Perhaps in January when I return from vacation
I'll test this to make sure that performs as intended in the
suspend-resume-while-associated case.  It's a little bit of a lie to
do it in this case since in fact as far as mac80211 is concerned, the
wiphy is not idle in this case.

--
Paul

Re: [PATCH] mac80211: Push idle state to driver before stop

[Luis R. Rodriguez]

On Thu, Dec 16, 2010 at 08:02:27AM -0800, Paul Stewart wrote:
> On Thu, Dec 16, 2010 at 4:53 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Thu, 2010-12-16 at 04:46 -0800, Paul Stewart wrote:
> > Not really -- the driver may throw away all internal state, mac80211
> > will (attempt to) restore it all through drv_config() with changed = ~0.
> > Evidently ath9k has some magic that makes this fail?
> 
> The magic in question is as follows (ath9k_config()):
> 
>     if (changed & IEEE80211_CONF_CHANGE_IDLE) {
>         bool enable_radio;
>         bool all_wiphys_idle;
>         bool idle = !!(conf->flags & IEEE80211_CONF_IDLE);
> 
>         spin_lock_bh(&sc->wiphy_lock);
>         all_wiphys_idle =  ath9k_all_wiphys_idle(sc);
>        ath9k_set_wiphy_idle(aphy, idle);
> 
>         enable_radio = (!idle && all_wiphys_idle);
> 
>         /*
>          * After we unlock here its possible another wiphy
>          * can be re-renabled so to account for that we will
>          * only disable the radio toward the end of this routine
>          * if by then all wiphys are still idle.
>          */
>         spin_unlock_bh(&sc->wiphy_lock);
> 
>         if (enable_radio) {
>             sc->ps_idle = false;
>             ath_radio_enable(sc, hw);
>             ath_dbg(common, ATH_DBG_CONFIG,
>                     "not-idle: enabling radio\n");
>         }
>     }
> 
> So, ath9k enables and disables the radio based on ps_idle (the
> internal state), but that's not re-evaluated unless the wiphys
> actually change.  In this case they do not change because ath9k was
> never informed that the wiphy went idle (all_wiphys_idle evaluates to
> false).  One solution would be to do an ah9k_set_wiphy_idle(aphy,
> true) in ath9k_stop() instead of Luis' previous suggestion of setting
> sc->ps_idle directly.  Perhaps in January when I return from vacation
> I'll test this to make sure that performs as intended in the
> suspend-resume-while-associated case.  It's a little bit of a lie to
> do it in this case since in fact as far as mac80211 is concerned, the
> wiphy is not idle in this case.

Try this:

diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index c68205d..3de3dbf 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1350,6 +1350,7 @@ static void ath9k_stop(struct ieee80211_hw *hw)
 	ath9k_ps_restore(sc);
 
 	sc->ps_idle = true;
+	ath9k_set_wiphy_idle(aphy, 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 7ca8499..4538283 100644
--- a/drivers/net/wireless/ath/ath9k/pci.c
+++ b/drivers/net/wireless/ath/ath9k/pci.c
@@ -310,6 +310,7 @@ static int ath_pci_resume(struct device *device)
 	ath9k_hw_set_gpio(sc->sc_ah, sc->sc_ah->led_pin, 1);
 
 	sc->ps_idle = true;
+	ath9k_set_wiphy_idle(aphy, true);
 	ath_radio_disable(sc, hw);
 
 	return 0;

Re: [PATCH] mac80211: Push idle state to driver before stop

[Luis R. Rodriguez]

On Thu, Dec 16, 2010 at 9:44 AM, Luis R. Rodriguez
<lrodriguez@atheros.com> wrote:
> On Thu, Dec 16, 2010 at 08:02:27AM -0800, Paul Stewart wrote:
>> On Thu, Dec 16, 2010 at 4:53 AM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> > On Thu, 2010-12-16 at 04:46 -0800, Paul Stewart wrote:
>> > Not really -- the driver may throw away all internal state, mac80211
>> > will (attempt to) restore it all through drv_config() with changed = ~0.
>> > Evidently ath9k has some magic that makes this fail?
>>
>> The magic in question is as follows (ath9k_config()):
>>
>>     if (changed & IEEE80211_CONF_CHANGE_IDLE) {
>>         bool enable_radio;
>>         bool all_wiphys_idle;
>>         bool idle = !!(conf->flags & IEEE80211_CONF_IDLE);
>>
>>         spin_lock_bh(&sc->wiphy_lock);
>>         all_wiphys_idle =  ath9k_all_wiphys_idle(sc);
>>        ath9k_set_wiphy_idle(aphy, idle);
>>
>>         enable_radio = (!idle && all_wiphys_idle);
>>
>>         /*
>>          * After we unlock here its possible another wiphy
>>          * can be re-renabled so to account for that we will
>>          * only disable the radio toward the end of this routine
>>          * if by then all wiphys are still idle.
>>          */
>>         spin_unlock_bh(&sc->wiphy_lock);
>>
>>         if (enable_radio) {
>>             sc->ps_idle = false;
>>             ath_radio_enable(sc, hw);
>>             ath_dbg(common, ATH_DBG_CONFIG,
>>                     "not-idle: enabling radio\n");
>>         }
>>     }
>>
>> So, ath9k enables and disables the radio based on ps_idle (the
>> internal state), but that's not re-evaluated unless the wiphys
>> actually change.  In this case they do not change because ath9k was
>> never informed that the wiphy went idle (all_wiphys_idle evaluates to
>> false).  One solution would be to do an ah9k_set_wiphy_idle(aphy,
>> true) in ath9k_stop() instead of Luis' previous suggestion of setting
>> sc->ps_idle directly.  Perhaps in January when I return from vacation
>> I'll test this to make sure that performs as intended in the
>> suspend-resume-while-associated case.  It's a little bit of a lie to
>> do it in this case since in fact as far as mac80211 is concerned, the
>> wiphy is not idle in this case.
>
> Try this:
>
> diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
> index c68205d..3de3dbf 100644
> --- a/drivers/net/wireless/ath/ath9k/main.c
> +++ b/drivers/net/wireless/ath/ath9k/main.c
> @@ -1350,6 +1350,7 @@ static void ath9k_stop(struct ieee80211_hw *hw)
>        ath9k_ps_restore(sc);
>
>        sc->ps_idle = true;
> +       ath9k_set_wiphy_idle(aphy, 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 7ca8499..4538283 100644
> --- a/drivers/net/wireless/ath/ath9k/pci.c
> +++ b/drivers/net/wireless/ath/ath9k/pci.c
> @@ -310,6 +310,7 @@ static int ath_pci_resume(struct device *device)
>        ath9k_hw_set_gpio(sc->sc_ah, sc->sc_ah->led_pin, 1);
>
>        sc->ps_idle = true;
> +       ath9k_set_wiphy_idle(aphy, true);
>        ath_radio_disable(sc, hw);
>
>        return 0;

If this does not work then yes, I will argue that mac80211 is not
turning us on when it needs to. I was concered about the resume case
given that mac80211 only calls drv_start() when open_count is > 0. So
we'll see.

  Luis

Re: [PATCH] mac80211: Push idle state to driver before stop

[Paul Stewart]

On Thu, Dec 16, 2010 at 9:44 AM, Luis R. Rodriguez
<lrodriguez@atheros.com> wrote:
> Try this:

I'm now on vacation and thousands of miles (and more importantly a few
firewalls) away from my build system.  If you want to know if this
works before the end of the end of the year, you can try to replicate
the conditions (scan active during ieee80211_do_stop()), otherwise we
can go back to this next year.

--
Paul

Re: [PATCH] mac80211: Push idle state to driver before stop

[Luis R. Rodriguez]

On Thu, Dec 16, 2010 at 11:08 AM, Paul Stewart <pstew@google.com> wrote:
> On Thu, Dec 16, 2010 at 9:44 AM, Luis R. Rodriguez
> <lrodriguez@atheros.com> wrote:
>> Try this:
>
> I'm now on vacation and thousands of miles (and more importantly a few
> firewalls) away from my build system.  If you want to know if this
> works before the end of the end of the year, you can try to replicate
> the conditions (scan active during ieee80211_do_stop()), otherwise we
> can go back to this next year.

2011 looks so futuristic, but yeah I'm sure this can wait, if I get a
chance maybe I'll try to hack my CR48 with my own kernel to test this.
We'll see I guess.

  Luis

Re: [PATCH] mac80211: Push idle state to driver before stop

[Paul Stewart]

On Thu, Dec 16, 2010 at 9:44 AM, Luis R. Rodriguez
<lrodriguez@atheros.com> wrote:
> Try this:

Happy new year.  I've tried this patch, and the system continues to
suspend and resume successfully (i.e, the fix from the earlier patches
continues to alleviate the original problem), however the system
continues to be "deaf" to beacons at resume time if the system
suspends and resumes while associated.  You don't need a ChromeOS
system to reproduce this issue.  Just associate to the network and
suspend/resume.  On resume the system believes it should be
associated, but then the beacon loss timer kicks in and we
disassociate, since we are never successful in receiving a response.

--
Paul

Re: [PATCH] mac80211: Push idle state to driver before stop

[Paul Stewart]

On Tue, Jan 4, 2011 at 10:19 AM, Paul Stewart <pstew@google.com> wrote:
> On Thu, Dec 16, 2010 at 9:44 AM, Luis R. Rodriguez
> <lrodriguez@atheros.com> wrote:
>> Try this:
>
> Happy new year.  I've tried this patch, and the system continues to
> suspend and resume successfully (i.e, the fix from the earlier patches
> continues to alleviate the original problem), however the system
> continues to be "deaf" to beacons at resume time if the system
> suspends and resumes while associated.  You don't need a ChromeOS
> system to reproduce this issue.  Just associate to the network and
> suspend/resume.  On resume the system believes it should be
> associated, but then the beacon loss timer kicks in and we
> disassociate, since we are never successful in receiving a response.

Whoops.  I didn't apply that change correctly.  Actually, I think this
change works.  I think I may have spotted a couple of unrelated
anomalies I'll trace down and address separately.

--
Paul

Re: [PATCH] mac80211: Push idle state to driver before stop

[Luis R. Rodriguez]

On Tue, Jan 04, 2011 at 02:29:53PM -0800, Paul Stewart wrote:
> On Tue, Jan 4, 2011 at 10:19 AM, Paul Stewart <pstew@google.com> wrote:
> > On Thu, Dec 16, 2010 at 9:44 AM, Luis R. Rodriguez
> > <lrodriguez@atheros.com> wrote:
> >> Try this:
> >
> > Happy new year.  I've tried this patch, and the system continues to
> > suspend and resume successfully (i.e, the fix from the earlier patches
> > continues to alleviate the original problem), however the system
> > continues to be "deaf" to beacons at resume time if the system
> > suspends and resumes while associated.  You don't need a ChromeOS
> > system to reproduce this issue.  Just associate to the network and
> > suspend/resume.  On resume the system believes it should be
> > associated, but then the beacon loss timer kicks in and we
> > disassociate, since we are never successful in receiving a response.
> 
> Whoops.  I didn't apply that change correctly.  Actually, I think this
> change works.  I think I may have spotted a couple of unrelated
> anomalies I'll trace down and address separately.

Great, I think this patch got sucked up already, I'll check.

  Luis

Re: [PATCH] mac80211: Push idle state to driver before stop

[Luis R. Rodriguez]

On Wed, Jan 5, 2011 at 10:26 AM, Luis R. Rodriguez
<lrodriguez@atheros.com> wrote:
> On Tue, Jan 04, 2011 at 02:29:53PM -0800, Paul Stewart wrote:
>> On Tue, Jan 4, 2011 at 10:19 AM, Paul Stewart <pstew@google.com> wrote:
>> > On Thu, Dec 16, 2010 at 9:44 AM, Luis R. Rodriguez
>> > <lrodriguez@atheros.com> wrote:
>> >> Try this:
>> >
>> > Happy new year.  I've tried this patch, and the system continues to
>> > suspend and resume successfully (i.e, the fix from the earlier patches
>> > continues to alleviate the original problem), however the system
>> > continues to be "deaf" to beacons at resume time if the system
>> > suspends and resumes while associated.  You don't need a ChromeOS
>> > system to reproduce this issue.  Just associate to the network and
>> > suspend/resume.  On resume the system believes it should be
>> > associated, but then the beacon loss timer kicks in and we
>> > disassociate, since we are never successful in receiving a response.
>>
>> Whoops.  I didn't apply that change correctly.  Actually, I think this
>> change works.  I think I may have spotted a couple of unrelated
>> anomalies I'll trace down and address separately.
>
> Great, I think this patch got sucked up already, I'll check.

Indeed, John just sent this to David as part of the queued up patches
for 2.6.38.

  Luis