[PATCH 2/2] ath9k_htc: Fix suspend/resume

[PATCH 2/2] ath9k_htc: Fix suspend/resume

[Sujith]

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

The HW has to be set to FULLSLEEP mode during suspend,
when no interface has been brought up. Not doing this would
break resume, as the chip won't be powered up at all.

Signed-off-by: Sujith Manoharan <Sujith.Manoharan@atheros.com>
---
 drivers/net/wireless/ath/ath9k/hif_usb.c      |    7 +++++++
 drivers/net/wireless/ath/ath9k/htc.h          |    3 +++
 drivers/net/wireless/ath/ath9k/htc_drv_init.c |    6 ++++++
 drivers/net/wireless/ath/ath9k/htc_drv_main.c |    4 ++--
 4 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c
index ae842db..6372c64 100644
--- a/drivers/net/wireless/ath/ath9k/hif_usb.c
+++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
@@ -1019,6 +1019,13 @@ static int ath9k_hif_usb_suspend(struct usb_interface *interface,
 	struct hif_device_usb *hif_dev =
 		(struct hif_device_usb *) usb_get_intfdata(interface);
 
+	/*
+	 * The device has to be set to FULLSLEEP mode in case no
+	 * interface is up.
+	 */
+	if (!(hif_dev->flags & HIF_USB_START))
+		ath9k_htc_suspend(hif_dev->htc_handle);
+
 	ath9k_hif_usb_dealloc_urbs(hif_dev);
 
 	return 0;
diff --git a/drivers/net/wireless/ath/ath9k/htc.h b/drivers/net/wireless/ath/ath9k/htc.h
index afe39a9..fdf9d5f 100644
--- a/drivers/net/wireless/ath/ath9k/htc.h
+++ b/drivers/net/wireless/ath/ath9k/htc.h
@@ -455,6 +455,8 @@ u32 ath9k_htc_calcrxfilter(struct ath9k_htc_priv *priv);
 void ath9k_htc_ps_wakeup(struct ath9k_htc_priv *priv);
 void ath9k_htc_ps_restore(struct ath9k_htc_priv *priv);
 void ath9k_ps_work(struct work_struct *work);
+bool ath9k_htc_setpower(struct ath9k_htc_priv *priv,
+			enum ath9k_power_mode mode);
 
 void ath9k_start_rfkill_poll(struct ath9k_htc_priv *priv);
 void ath9k_init_leds(struct ath9k_htc_priv *priv);
@@ -464,6 +466,7 @@ int ath9k_htc_probe_device(struct htc_target *htc_handle, struct device *dev,
 			   u16 devid, char *product, u32 drv_info);
 void ath9k_htc_disconnect_device(struct htc_target *htc_handle, bool hotunplug);
 #ifdef CONFIG_PM
+void ath9k_htc_suspend(struct htc_target *htc_handle);
 int ath9k_htc_resume(struct htc_target *htc_handle);
 #endif
 #ifdef CONFIG_ATH9K_HTC_DEBUGFS
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_init.c b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
index 071d0c9..3b172ea 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_init.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
@@ -888,6 +888,12 @@ void ath9k_htc_disconnect_device(struct htc_target *htc_handle, bool hotunplug)
 }
 
 #ifdef CONFIG_PM
+
+void ath9k_htc_suspend(struct htc_target *htc_handle)
+{
+	ath9k_htc_setpower(htc_handle->drv_priv, ATH9K_PM_FULL_SLEEP);
+}
+
 int ath9k_htc_resume(struct htc_target *htc_handle)
 {
 	struct ath9k_htc_priv *priv = htc_handle->drv_priv;
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_main.c b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
index 8266ce1..1f62155 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_main.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
@@ -63,8 +63,8 @@ static enum htc_phymode ath9k_htc_get_curmode(struct ath9k_htc_priv *priv,
 	return mode;
 }
 
-static bool ath9k_htc_setpower(struct ath9k_htc_priv *priv,
-			       enum ath9k_power_mode mode)
+bool ath9k_htc_setpower(struct ath9k_htc_priv *priv,
+			enum ath9k_power_mode mode)
 {
 	bool ret;
 
-- 
1.7.3.2

Re: [PATCH 2/2] ath9k_htc: Fix suspend/resume

[John W. Linville]

On Wed, Dec 01, 2010 at 12:30:27PM +0530, Sujith wrote:
> From: Sujith Manoharan <Sujith.Manoharan@atheros.com>
> 
> The HW has to be set to FULLSLEEP mode during suspend,
> when no interface has been brought up. Not doing this would
> break resume, as the chip won't be powered up at all.
> 
> Signed-off-by: Sujith Manoharan <Sujith.Manoharan@atheros.com>

Is this intended for 2.6.37?  It looks like it will apply there.

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/2] ath9k_htc: Fix suspend/resume

[Sujith]

John W. Linville wrote:
> On Wed, Dec 01, 2010 at 12:30:27PM +0530, Sujith wrote:
> > From: Sujith Manoharan <Sujith.Manoharan@atheros.com>
> > 
> > The HW has to be set to FULLSLEEP mode during suspend,
> > when no interface has been brought up. Not doing this would
> > break resume, as the chip won't be powered up at all.
> > 
> > Signed-off-by: Sujith Manoharan <Sujith.Manoharan@atheros.com>
> 
> Is this intended for 2.6.37?  It looks like it will apply there.

Thanks for pointing this out and yes, this would be a candidate for 2.6.37

Sujith

Re: [PATCH 2/2] ath9k_htc: Fix suspend/resume

[Luis R. Rodriguez]

On Thu, Dec 2, 2010 at 6:48 PM, Sujith <m.sujith@gmail.com> wrote:
> John W. Linville wrote:
>> On Wed, Dec 01, 2010 at 12:30:27PM +0530, Sujith wrote:
>> > From: Sujith Manoharan <Sujith.Manoharan@atheros.com>
>> >
>> > The HW has to be set to FULLSLEEP mode during suspend,
>> > when no interface has been brought up. Not doing this would
>> > break resume, as the chip won't be powered up at all.
>> >
>> > Signed-off-by: Sujith Manoharan <Sujith.Manoharan@atheros.com>
>>
>> Is this intended for 2.6.37?  It looks like it will apply there.
>
> Thanks for pointing this out and yes, this would be a candidate for 2.6.37

Just curious, if you revert this patch and apply the ones I just
posted, does it fix it, or is this patch required either way?

  Luis

Re: [PATCH 2/2] ath9k_htc: Fix suspend/resume

[Sujith]

Luis R. Rodriguez wrote:
> Just curious, if you revert this patch and apply the ones I just
> posted, does it fix it, or is this patch required either way?

Well, I don't see this patch in -testing, -next-2.6 or -2.6.
Maybe Linville overlooked this patch.

Applying 4/5 and 5/5 from your series seems to fix the issue though.
I am able to suspend/resume without hiccups with UB95.

But, I think this is purely an ath9k issue, mac80211 is probably right in
assuming that only devices that have been issued ~IEEE80211_CONF_IDLE
would have their radios enabled anyway.

ath9k doesn't differentiate between init/start and enables the radio
(setting it to AWAKE mode) in the initialization sequence.
Why should mac80211 issue a CONF_IDLE notification when the device
was never powered up .. ?

But maybe I am wrong and confused.
Anyway, your patches fix the problem.

I did see this warning once while testing, but I couldn't reproduce it again.

WARNING: at /home/sujith/dev/wireless-testing/net/mac80211/iface.c:1267 __ieee80211_recalc_idle+0x22c/0x310 [mac80211]()
Hardware name: 7661GN4
Modules linked in: arc4 ecb ath9k_htc(-) ath9k_common ath9k_hw ath mac80211
cfg80211 ext2 mct_u232 usbserial snd_hda_codec_analog i915 snd_hda_intel snd_hda_codec snd_pcm_oss snd_pcm dd
[  154.022734] Pid: 3110, comm: rmmod Tainted: G        W   2.6.37-rc4-wl #11
[  154.022736] Call Trace:
[<ffffffff81057e9a>] warn_slowpath_common+0x7a/0xb0
[<ffffffff81057ee5>] warn_slowpath_null+0x15/0x20
[<ffffffffa05d4d3c>] __ieee80211_recalc_idle+0x22c/0x310 [mac80211]
[<ffffffffa05d4e5a>] ieee80211_recalc_idle_check+0x3a/0x70 [mac80211]
[<ffffffffa05d639e>] ieee80211_recalc_idle_force+0xe/0x10 [mac80211]
[<ffffffffa05e5ad0>] ieee80211_stop_device+0xa0/0x160 [mac80211]
[<ffffffffa05d510f>] ieee80211_do_stop+0x27f/0x610 [mac80211]
[<ffffffff8130e217>] ? dev_deactivate+0x1d7/0x200
[<ffffffffa05d54b5>] ieee80211_stop+0x15/0x20 [mac80211]
[<ffffffff812f1b46>] __dev_close+0x56/0x90
[<ffffffff812f1b9a>] dev_close+0x1a/0x50
[<ffffffff812f1f0a>] rollback_registered_many+0x6a/0x250
[<ffffffff812f2106>] unregister_netdevice_many+0x16/0x80
[<ffffffffa05d4ad4>] ieee80211_remove_interfaces+0xb4/0xf0 [mac80211]
[<ffffffffa05c10be>] ieee80211_unregister_hw+0x4e/0x110 [mac80211]
[<ffffffffa028d113>] ath9k_htc_disconnect_device+0x43/0x90 [ath9k_htc]
[<ffffffffa0284a62>] ath9k_htc_hw_deinit+0x12/0x20 [ath9k_htc]
[<ffffffffa02864bf>] ath9k_hif_usb_disconnect+0x4f/0x140 [ath9k_htc]
[<ffffffffa03e70cb>] usb_unbind_interface+0x5b/0x1a0 [usbcore]
[<ffffffff812ae400>] __device_release_driver+0x70/0xe0
[<ffffffff812aeb28>] driver_detach+0xc8/0xd0
[<ffffffff812ae278>] bus_remove_driver+0x88/0xe0
[<ffffffff812aebba>] driver_unregister+0x5a/0x90
[<ffffffffa03e61ba>] usb_deregister+0xba/0xd0 [usbcore]
[<ffffffffa0286970>] ath9k_hif_usb_exit+0x10/0x20 [ath9k_htc]
[<ffffffffa028d595>] ath9k_htc_exit+0x9/0x1e [ath9k_htc]
[<ffffffff8109d226>] sys_delete_module+0x196/0x290
[<ffffffff813a8bb9>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[<ffffffff8100bf52>] system_call_fastpath+0x16/0x1b

Sujith

Re: [PATCH 2/2] ath9k_htc: Fix suspend/resume

[Luis R. Rodriguez]

On Mon, Dec 6, 2010 at 8:09 PM, Sujith <m.sujith@gmail.com> wrote:
> Luis R. Rodriguez wrote:
>> Just curious, if you revert this patch and apply the ones I just
>> posted, does it fix it, or is this patch required either way?
>
> Well, I don't see this patch in -testing, -next-2.6 or -2.6.
> Maybe Linville overlooked this patch.
>
> Applying 4/5 and 5/5 from your series seems to fix the issue though.
> I am able to suspend/resume without hiccups with UB95.
>
> But, I think this is purely an ath9k issue, mac80211 is probably right in
> assuming that only devices that have been issued ~IEEE80211_CONF_IDLE
> would have their radios enabled anyway.
>
> ath9k doesn't differentiate between init/start and enables the radio
> (setting it to AWAKE mode) in the initialization sequence.
> Why should mac80211 issue a CONF_IDLE notification when the device
> was never powered up .. ?

That's the thing, the device was powered up, lifted from idle, but we
never got notified we are back in idle, since we stop the device but
never issue the idle call since the open_count was already 0 after a
device stop.

  Luis

Re: [PATCH 2/2] ath9k_htc: Fix suspend/resume

[Sujith]

Luis R. Rodriguez wrote:
> That's the thing, the device was powered up, lifted from idle, but we
                    ^^^^^^^^^^^^^^^^^^^^^^^^^
Not according to mac80211 - start() wasn't issued, config() with ~CONF_IDLE
wasn't issued.

This is the issue in ath9k_htc:

* modprobe ath9k_htc
* pm-suspend
---
* resume

In this case, as I said above, neither start() nor CONF_IDLE have been notified
to the driver by mac80211, since the interface was never brought up.

But ath9k_htc powers up the device while initialization, immediately when the driver
is loaded, not waiting for start() or ~CONF_IDLE. And since the driver maintains
internal state with respect to the radio's power status, I think it is up to the
driver to clean it properly ...

Sujith

Re: [PATCH 2/2] ath9k_htc: Fix suspend/resume

[Luis R. Rodriguez]

On Mon, Dec 6, 2010 at 8:22 PM, Sujith <m.sujith@gmail.com> wrote:
> Luis R. Rodriguez wrote:
>> That's the thing, the device was powered up, lifted from idle, but we
>                    ^^^^^^^^^^^^^^^^^^^^^^^^^
> Not according to mac80211 - start() wasn't issued, config() with ~CONF_IDLE
> wasn't issued.
>
> This is the issue in ath9k_htc:
>
> * modprobe ath9k_htc
> * pm-suspend
> ---
> * resume
>
> In this case, as I said above, neither start() nor CONF_IDLE have been notified
> to the driver by mac80211, since the interface was never brought up.
>
> But ath9k_htc powers up the device while initialization, immediately when the driver
> is loaded, not waiting for start() or ~CONF_IDLE. And since the driver maintains
> internal state with respect to the radio's power status, I think it is up to the
> driver to clean it properly ...

Oh right, I see. Should we not wait to turn the radio until we get a
non-idle config() call?

  Luis

Re: [PATCH 2/2] ath9k_htc: Fix suspend/resume

[Sujith]

Luis R. Rodriguez wrote:
> Oh right, I see. Should we not wait to turn the radio until we get a
> non-idle config() call?

Ideally, yes. But that would require extensive changes in ath9k_hw.
Initialization has to be pruned, powering up the chip has to be deferred ...

Which is why I decided to settle for a simple band-aid for now. :)

Sujith

Re: [PATCH 2/2] ath9k_htc: Fix suspend/resume

[Luis R. Rodriguez]

On Tue, Dec 7, 2010 at 12:01 AM, Sujith <m.sujith@gmail.com> wrote:
> Luis R. Rodriguez wrote:
>> Oh right, I see. Should we not wait to turn the radio until we get a
>> non-idle config() call?
>
> Ideally, yes. But that would require extensive changes in ath9k_hw.
> Initialization has to be pruned, powering up the chip has to be deferred ...
>
> Which is why I decided to settle for a simple band-aid for now. :)

Hm, well I was thinking more along the lines of what
ath9k_htc_config() already does with IEEE80211_CONF_CHANGE_IDLE when
enabling the radio. That seems right.

  Luis

Re: [PATCH 2/2] ath9k_htc: Fix suspend/resume

[Sujith]

Luis R. Rodriguez wrote:
> Hm, well I was thinking more along the lines of what
> ath9k_htc_config() already does with IEEE80211_CONF_CHANGE_IDLE when
> enabling the radio. That seems right.

Not sure, maybe ath9k has issues handling CONF_IDLE.

Sujith

Re: [PATCH 2/2] ath9k_htc: Fix suspend/resume

[Luis R. Rodriguez]

On Tue, Dec 7, 2010 at 12:48 AM, Sujith <m.sujith@gmail.com> wrote:
> Luis R. Rodriguez wrote:
>> Hm, well I was thinking more along the lines of what
>> ath9k_htc_config() already does with IEEE80211_CONF_CHANGE_IDLE when
>> enabling the radio. That seems right.
>
> Not sure, maybe ath9k has issues handling CONF_IDLE.

Well ath9k was fine with enabling the radio, the issue was with it not
getting a now we're idle notification later on in certain cases.

  Luis

Re: [PATCH 2/2] ath9k_htc: Fix suspend/resume

[John W. Linville]

On Tue, Dec 07, 2010 at 09:39:10AM +0530, Sujith wrote:
> Luis R. Rodriguez wrote:
> > Just curious, if you revert this patch and apply the ones I just
> > posted, does it fix it, or is this patch required either way?
> 
> Well, I don't see this patch in -testing, -next-2.6 or -2.6.
> Maybe Linville overlooked this patch.

Obviously I didn't overlook it, or I wouldn't have asked about whether
or not it should go to 2.6.37...

> Applying 4/5 and 5/5 from your series seems to fix the issue though.
> I am able to suspend/resume without hiccups with UB95.

But now this begs the question of whether this should be applied at all?

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/2] ath9k_htc: Fix suspend/resume

[Luis R. Rodriguez]

On Tue, Dec 7, 2010 at 11:46 AM, John W. Linville
<linville@tuxdriver.com> wrote:
> On Tue, Dec 07, 2010 at 09:39:10AM +0530, Sujith wrote:
>> Luis R. Rodriguez wrote:
>> > Just curious, if you revert this patch and apply the ones I just
>> > posted, does it fix it, or is this patch required either way?
>>
>> Well, I don't see this patch in -testing, -next-2.6 or -2.6.
>> Maybe Linville overlooked this patch.
>
> Obviously I didn't overlook it, or I wouldn't have asked about whether
> or not it should go to 2.6.37...
>
>> Applying 4/5 and 5/5 from your series seems to fix the issue though.
>> I am able to suspend/resume without hiccups with UB95.
>
> But now this begs the question of whether this should be applied at all?

As I reviewed with Johannes the driver must ensure the device remains
idle after we get the last stop() call. So this patch seems fine, I'd
even consider removing the check for the if (!(hif_dev->flags &
HIF_USB_START)) and also call  ath9k_htc_suspend(hif_dev->htc_handle);
upon resume to ensure it is kept in full sleep then as well.

  Luis

Re: [PATCH 2/2] ath9k_htc: Fix suspend/resume

[Sujith]

John W. Linville wrote:
> Obviously I didn't overlook it, or I wouldn't have asked about whether
> or not it should go to 2.6.37...

Sorry, I saw the other patch [1/2] merged and thought that you missed this.

> > Applying 4/5 and 5/5 from your series seems to fix the issue though.
> > I am able to suspend/resume without hiccups with UB95.
> 
> But now this begs the question of whether this should be applied at all?

Yes, this is needed for ath9k_htc.

Sujith