[PATCH 0/7] wl12xx: add initial wowlan support

[PATCH 0/7] wl12xx: add initial wowlan support

[Eliad Peller]

Add initial support for Wake-On-Wireless.

Currently, we support only the ANY trigger, which basically means
the device stays awake while the host is being suspended, and will
wake up the host by any irq it will trigger (but since the wl12xx
fw offloads a lot of its work, it will probably wake up the host
because of rx frame/beacon loss)

Note that this patchset needs some additional omap/mmc patches in 
order to support the MMC_PM_KEEP_POWER flag and wake-irq correctly
(however, if the MMC_PM_KEEP_POWER flag is not being supported we 
don't advertise support for wowlan triggers, so this patchset alone
shouldn't harm).

Eliad Peller (7):
  wl12xx_sdio: set interrupt as wake_up interrupt
  wl12xx: declare suspend/resume callbacks (for wowlan)
  wl12xx_sdio: set MMC_PM_KEEP_POWER flag on suspend
  wl12xx: prevent scheduling while suspending (WoW enabled)
  wl12xx_sdio: declare support for NL80211_WOW_TRIGGER_ANYTHING trigger
  wl12xx: add ps completion event
  wl12xx: enter/exit psm on wowlan suspend/resume

 drivers/net/wireless/wl12xx/event.c  |    7 ++
 drivers/net/wireless/wl12xx/main.c   |  146 ++++++++++++++++++++++++++++++++++
 drivers/net/wireless/wl12xx/sdio.c   |   66 +++++++++++++++
 drivers/net/wireless/wl12xx/wl12xx.h |    9 ++
 4 files changed, 228 insertions(+), 0 deletions(-)

[PATCH 1/7] wl12xx_sdio: set interrupt as wake_up interrupt

[Eliad Peller]

set the sdio interrupt as wake_up interrupt, so we will be able
to wake up the suspended system (Wake-On-Wireless)

Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 drivers/net/wireless/wl12xx/sdio.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/sdio.c b/drivers/net/wireless/wl12xx/sdio.c
index bcd4ad7..1298461 100644
--- a/drivers/net/wireless/wl12xx/sdio.c
+++ b/drivers/net/wireless/wl12xx/sdio.c
@@ -267,6 +267,8 @@ static int __devinit wl1271_probe(struct sdio_func *func,
 		goto out_free;
 	}
 
+	enable_irq_wake(wl->irq);
+
 	disable_irq(wl->irq);
 
 	ret = wl1271_init_ieee80211(wl);
@@ -303,6 +305,7 @@ static void __devexit wl1271_remove(struct sdio_func *func)
 	pm_runtime_get_noresume(&func->dev);
 
 	wl1271_unregister_hw(wl);
+	disable_irq_wake(wl->irq);
 	free_irq(wl->irq, wl);
 	wl1271_free_hw(wl);
 }
-- 
1.7.0.4

[PATCH 2/7] wl12xx: declare suspend/resume callbacks (for wowlan)

[Eliad Peller]

Additionally, add wow_enabled field to wl, to indicate
whether wowlan was configured.

Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 drivers/net/wireless/wl12xx/main.c   |   19 +++++++++++++++++++
 drivers/net/wireless/wl12xx/wl12xx.h |    6 ++++++
 2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index f82e736..9ca71ce 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -1332,6 +1332,23 @@ static struct notifier_block wl1271_dev_notifier = {
 	.notifier_call = wl1271_dev_notify,
 };
 
+static int wl1271_op_suspend(struct ieee80211_hw *hw,
+			    struct cfg80211_wowlan *wow)
+{
+	struct wl1271 *wl = hw->priv;
+	wl1271_debug(DEBUG_MAC80211, "mac80211 suspend wow=%d", !!wow);
+	wl->wow_enabled = !!wow;
+	return 0;
+}
+
+static int wl1271_op_resume(struct ieee80211_hw *hw)
+{
+	struct wl1271 *wl = hw->priv;
+	wl1271_debug(DEBUG_MAC80211, "mac80211 resume wow=%d",
+		     wl->wow_enabled);
+	return 0;
+}
+
 static int wl1271_op_start(struct ieee80211_hw *hw)
 {
 	wl1271_debug(DEBUG_MAC80211, "mac80211 start");
@@ -3426,6 +3443,8 @@ static const struct ieee80211_ops wl1271_ops = {
 	.stop = wl1271_op_stop,
 	.add_interface = wl1271_op_add_interface,
 	.remove_interface = wl1271_op_remove_interface,
+	.suspend = wl1271_op_suspend,
+	.resume = wl1271_op_resume,
 	.config = wl1271_op_config,
 	.prepare_multicast = wl1271_op_prepare_multicast,
 	.configure_filter = wl1271_op_configure_filter,
diff --git a/drivers/net/wireless/wl12xx/wl12xx.h b/drivers/net/wireless/wl12xx/wl12xx.h
index b760143..f9d0a14 100644
--- a/drivers/net/wireless/wl12xx/wl12xx.h
+++ b/drivers/net/wireless/wl12xx/wl12xx.h
@@ -564,6 +564,12 @@ struct wl1271 {
 	int tcxo_clock;
 
 	/*
+	 * wowlan trigger was configured during suspend.
+	 * (currently, only "ANY" trigger is supported)
+	 */
+	bool wow_enabled;
+
+	/*
 	 * AP-mode - links indexed by HLID. The global and broadcast links
 	 * are always active.
 	 */
-- 
1.7.0.4

[PATCH 3/7] wl12xx_sdio: set MMC_PM_KEEP_POWER flag on suspend

[Eliad Peller]

if a wow trigger was configured, set the MMC_PM_KEEP_POWER flag
on suspend, so our power will be kept while the system is suspended.

We needed to set this flag on each suspend attempt (when we want
to keep power)

Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 drivers/net/wireless/wl12xx/sdio.c |   29 ++++++++++++++++++++++++++++-
 1 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/sdio.c b/drivers/net/wireless/wl12xx/sdio.c
index 1298461..5b03fd5 100644
--- a/drivers/net/wireless/wl12xx/sdio.c
+++ b/drivers/net/wireless/wl12xx/sdio.c
@@ -314,7 +314,34 @@ static int wl1271_suspend(struct device *dev)
 {
 	/* Tell MMC/SDIO core it's OK to power down the card
 	 * (if it isn't already), but not to remove it completely */
-	return 0;
+	struct sdio_func *func = dev_to_sdio_func(dev);
+	struct wl1271 *wl = sdio_get_drvdata(func);
+	mmc_pm_flag_t sdio_flags;
+	int ret = 0;
+
+	wl1271_debug(DEBUG_MAC80211, "wl1271 suspend. wow_enabled: %d",
+		     wl->wow_enabled);
+
+	/* check whether sdio should keep power */
+	if (wl->wow_enabled) {
+		sdio_flags = sdio_get_host_pm_caps(func);
+
+		if (!(sdio_flags & MMC_PM_KEEP_POWER)) {
+			wl1271_error("can't keep power while host "
+				     "is suspended");
+			ret = -EINVAL;
+			goto out;
+		}
+
+		/* keep power while host suspended */
+		ret = sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
+		if (ret) {
+			wl1271_error("error while trying to keep power");
+			goto out;
+		}
+	}
+out:
+	return ret;
 }
 
 static int wl1271_resume(struct device *dev)
-- 
1.7.0.4

[PATCH 4/7] wl12xx: prevent scheduling while suspending (WoW enabled)

[Eliad Peller]

When WoW is enabled, the interface will stay up and the chip will
be powered on, so we have to flush/cancel any remaining work, and
prevent the irq handler from scheduling a new work until the system
is resumed.

Add 2 new flags:
* WL1271_FLAG_SUSPENDED - the system is (about to be) suspended.
* WL1271_FLAG_PENDING_WORK - there is a pending irq work which
   should be scheduled when the system is being resumed.

In order to wake-up the system while getting an irq, we initialize
the device as wakeup device, and calling pm_wakeup_event() upon
getting the interrupt (while the system is about to be suspended)

Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 drivers/net/wireless/wl12xx/main.c   |   46 ++++++++++++++++++++++++++++++++++
 drivers/net/wireless/wl12xx/sdio.c   |   27 ++++++++++++++++++++
 drivers/net/wireless/wl12xx/wl12xx.h |    2 +
 3 files changed, 75 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index 9ca71ce..308855a 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -1338,6 +1338,28 @@ static int wl1271_op_suspend(struct ieee80211_hw *hw,
 	struct wl1271 *wl = hw->priv;
 	wl1271_debug(DEBUG_MAC80211, "mac80211 suspend wow=%d", !!wow);
 	wl->wow_enabled = !!wow;
+	if (wl->wow_enabled) {
+		/* flush any remaining work */
+		wl1271_debug(DEBUG_MAC80211, "flushing remaining works");
+		flush_delayed_work(&wl->scan_complete_work);
+
+		/*
+		 * disable and re-enable interrupts in order to flush
+		 * the threaded_irq
+		 */
+		wl1271_disable_interrupts(wl);
+
+		/*
+		 * set suspended flag to avoid triggering a new threaded_irq
+		 * work. no need for spinlock as interrupts are disabled.
+		 */
+		set_bit(WL1271_FLAG_SUSPENDED, &wl->flags);
+
+		wl1271_enable_interrupts(wl);
+		flush_work(&wl->tx_work);
+		flush_delayed_work(&wl->pspoll_work);
+		flush_delayed_work(&wl->elp_work);
+	}
 	return 0;
 }
 
@@ -1346,6 +1368,30 @@ static int wl1271_op_resume(struct ieee80211_hw *hw)
 	struct wl1271 *wl = hw->priv;
 	wl1271_debug(DEBUG_MAC80211, "mac80211 resume wow=%d",
 		     wl->wow_enabled);
+
+	/*
+	 * re-enable irq_work enqueuing, and call irq_work directly if
+	 * there is a pending work.
+	 */
+	if (wl->wow_enabled) {
+		struct wl1271 *wl = hw->priv;
+		unsigned long flags;
+		bool run_irq_work = false;
+
+		spin_lock_irqsave(&wl->wl_lock, flags);
+		clear_bit(WL1271_FLAG_SUSPENDED, &wl->flags);
+		if (test_and_clear_bit(WL1271_FLAG_PENDING_WORK, &wl->flags))
+			run_irq_work = true;
+		spin_unlock_irqrestore(&wl->wl_lock, flags);
+
+		if (run_irq_work) {
+			wl1271_debug(DEBUG_MAC80211,
+				     "run postponed irq_work directly");
+			wl1271_irq(0, wl);
+			wl1271_enable_interrupts(wl);
+		}
+	}
+
 	return 0;
 }
 
diff --git a/drivers/net/wireless/wl12xx/sdio.c b/drivers/net/wireless/wl12xx/sdio.c
index 5b03fd5..bf2a6ad 100644
--- a/drivers/net/wireless/wl12xx/sdio.c
+++ b/drivers/net/wireless/wl12xx/sdio.c
@@ -72,6 +72,7 @@ static irqreturn_t wl1271_hardirq(int irq, void *cookie)
 {
 	struct wl1271 *wl = cookie;
 	unsigned long flags;
+	bool skip = false;
 
 	wl1271_debug(DEBUG_IRQ, "IRQ");
 
@@ -82,8 +83,20 @@ static irqreturn_t wl1271_hardirq(int irq, void *cookie)
 		complete(wl->elp_compl);
 		wl->elp_compl = NULL;
 	}
+
+	if (test_bit(WL1271_FLAG_SUSPENDED, &wl->flags)) {
+		/* don't enqueue a work right now. mark it as pending */
+		set_bit(WL1271_FLAG_PENDING_WORK, &wl->flags);
+		wl1271_debug(DEBUG_IRQ, "should not enqueue work");
+		disable_irq_nosync(wl->irq);
+		pm_wakeup_event(wl1271_sdio_wl_to_dev(wl), 0);
+		skip = true;
+	}
 	spin_unlock_irqrestore(&wl->wl_lock, flags);
 
+	if (skip)
+		return IRQ_HANDLED;
+
 	return IRQ_WAKE_THREAD;
 }
 
@@ -268,6 +281,7 @@ static int __devinit wl1271_probe(struct sdio_func *func,
 	}
 
 	enable_irq_wake(wl->irq);
+	device_init_wakeup(wl1271_sdio_wl_to_dev(wl), 1);
 
 	disable_irq(wl->irq);
 
@@ -305,6 +319,7 @@ static void __devexit wl1271_remove(struct sdio_func *func)
 	pm_runtime_get_noresume(&func->dev);
 
 	wl1271_unregister_hw(wl);
+	device_init_wakeup(wl1271_sdio_wl_to_dev(wl), 0);
 	disable_irq_wake(wl->irq);
 	free_irq(wl->irq, wl);
 	wl1271_free_hw(wl);
@@ -339,6 +354,9 @@ static int wl1271_suspend(struct device *dev)
 			wl1271_error("error while trying to keep power");
 			goto out;
 		}
+
+		/* release host */
+		sdio_release_host(func);
 	}
 out:
 	return ret;
@@ -346,6 +364,15 @@ out:
 
 static int wl1271_resume(struct device *dev)
 {
+	struct sdio_func *func = dev_to_sdio_func(dev);
+	struct wl1271 *wl = sdio_get_drvdata(func);
+
+	wl1271_debug(DEBUG_MAC80211, "wl1271 resume");
+	if (wl->wow_enabled) {
+		/* claim back host */
+		sdio_claim_host(func);
+	}
+
 	return 0;
 }
 
diff --git a/drivers/net/wireless/wl12xx/wl12xx.h b/drivers/net/wireless/wl12xx/wl12xx.h
index f9d0a14..daf941d 100644
--- a/drivers/net/wireless/wl12xx/wl12xx.h
+++ b/drivers/net/wireless/wl12xx/wl12xx.h
@@ -358,6 +358,8 @@ enum wl12xx_flags {
 	WL1271_FLAG_AP_STARTED,
 	WL1271_FLAG_IF_INITIALIZED,
 	WL1271_FLAG_DUMMY_PACKET_PENDING,
+	WL1271_FLAG_SUSPENDED,
+	WL1271_FLAG_PENDING_WORK,
 };
 
 struct wl1271_link {
-- 
1.7.0.4

Re: [PATCH 4/7] wl12xx: prevent scheduling while suspending (WoW enabled)

[Luciano Coelho]

On Wed, 2011-05-11 at 11:54 +0300, Eliad Peller wrote:
> When WoW is enabled, the interface will stay up and the chip will
> be powered on, so we have to flush/cancel any remaining work, and
> prevent the irq handler from scheduling a new work until the system
> is resumed.
> 
> Add 2 new flags:
> * WL1271_FLAG_SUSPENDED - the system is (about to be) suspended.
> * WL1271_FLAG_PENDING_WORK - there is a pending irq work which
>    should be scheduled when the system is being resumed.
> 
> In order to wake-up the system while getting an irq, we initialize
> the device as wakeup device, and calling pm_wakeup_event() upon
> getting the interrupt (while the system is about to be suspended)
> 
> Signed-off-by: Eliad Peller <eliad@wizery.com>
> ---

[...]

> diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
> index 9ca71ce..308855a 100644
> --- a/drivers/net/wireless/wl12xx/main.c
> +++ b/drivers/net/wireless/wl12xx/main.c
> @@ -1338,6 +1338,28 @@ static int wl1271_op_suspend(struct ieee80211_hw *hw,
>  	struct wl1271 *wl = hw->priv;
>  	wl1271_debug(DEBUG_MAC80211, "mac80211 suspend wow=%d", !!wow);
>  	wl->wow_enabled = !!wow;
> +	if (wl->wow_enabled) {
> +		/* flush any remaining work */
> +		wl1271_debug(DEBUG_MAC80211, "flushing remaining works");
> +		flush_delayed_work(&wl->scan_complete_work);
> +
> +		/*
> +		 * disable and re-enable interrupts in order to flush
> +		 * the threaded_irq
> +		 */
> +		wl1271_disable_interrupts(wl);
> +
> +		/*
> +		 * set suspended flag to avoid triggering a new threaded_irq
> +		 * work. no need for spinlock as interrupts are disabled.
> +		 */
> +		set_bit(WL1271_FLAG_SUSPENDED, &wl->flags);

The set_bit() function is atomic, so you wouldn't need the spinlock here
anyway.  Couldn't you use __set_bit() instead, to avoid the expense of
using an atomic function when it's not necessary?


> +
> +		wl1271_enable_interrupts(wl);
> +		flush_work(&wl->tx_work);
> +		flush_delayed_work(&wl->pspoll_work);
> +		flush_delayed_work(&wl->elp_work);
> +	}
>  	return 0;
>  }
>  
> @@ -1346,6 +1368,30 @@ static int wl1271_op_resume(struct ieee80211_hw *hw)
>  	struct wl1271 *wl = hw->priv;
>  	wl1271_debug(DEBUG_MAC80211, "mac80211 resume wow=%d",
>  		     wl->wow_enabled);
> +
> +	/*
> +	 * re-enable irq_work enqueuing, and call irq_work directly if
> +	 * there is a pending work.
> +	 */
> +	if (wl->wow_enabled) {
> +		struct wl1271 *wl = hw->priv;
> +		unsigned long flags;
> +		bool run_irq_work = false;
> +
> +		spin_lock_irqsave(&wl->wl_lock, flags);
> +		clear_bit(WL1271_FLAG_SUSPENDED, &wl->flags);
> +		if (test_and_clear_bit(WL1271_FLAG_PENDING_WORK, &wl->flags))
> +			run_irq_work = true;
> +		spin_unlock_irqrestore(&wl->wl_lock, flags);

You could use __clear_bit() and __test_and_clear_bit() here too, since
you're spinning.


> +		if (run_irq_work) {
> +			wl1271_debug(DEBUG_MAC80211,
> +				     "run postponed irq_work directly");
> +			wl1271_irq(0, wl);
> +			wl1271_enable_interrupts(wl);

Shouldn't this wl1271_enable_interrupts() be outside the if? Don't you
want to enable interrupts again when there was no pending work?


> diff --git a/drivers/net/wireless/wl12xx/sdio.c b/drivers/net/wireless/wl12xx/sdio.c
> index 5b03fd5..bf2a6ad 100644
> --- a/drivers/net/wireless/wl12xx/sdio.c
> +++ b/drivers/net/wireless/wl12xx/sdio.c
> @@ -72,6 +72,7 @@ static irqreturn_t wl1271_hardirq(int irq, void *cookie)
>  {
>  	struct wl1271 *wl = cookie;
>  	unsigned long flags;
> +	bool skip = false;
>  
>  	wl1271_debug(DEBUG_IRQ, "IRQ");
>  
> @@ -82,8 +83,20 @@ static irqreturn_t wl1271_hardirq(int irq, void *cookie)
>  		complete(wl->elp_compl);
>  		wl->elp_compl = NULL;
>  	}
> +
> +	if (test_bit(WL1271_FLAG_SUSPENDED, &wl->flags)) {
> +		/* don't enqueue a work right now. mark it as pending */
> +		set_bit(WL1271_FLAG_PENDING_WORK, &wl->flags);
> +		wl1271_debug(DEBUG_IRQ, "should not enqueue work");
> +		disable_irq_nosync(wl->irq);
> +		pm_wakeup_event(wl1271_sdio_wl_to_dev(wl), 0);
> +		skip = true;
> +	}
>  	spin_unlock_irqrestore(&wl->wl_lock, flags);
>  
> +	if (skip)
> +		return IRQ_HANDLED;
> +
>  	return IRQ_WAKE_THREAD;
>  }

Could be nicer to just skip the skip variable (unintended pun
intended ;).  You can do it like this:

if (test_bit...) {
	...
	spin_unlock_irqrestore();
	return IRQ_HANDLED;
}
spin_unlock_irqrestore();
return IRQ_WAKE_THREAD;

Looks a bit nicer to me, but I don't mind if you prefer not skipping the
skip. :)


-- 
Cheers,
Luca.

Re: [PATCH 4/7] wl12xx: prevent scheduling while suspending (WoW enabled)

[Johannes Berg]

On Thu, 2011-05-12 at 22:48 +0300, Luciano Coelho wrote:

> > diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
> > index 9ca71ce..308855a 100644
> > --- a/drivers/net/wireless/wl12xx/main.c
> > +++ b/drivers/net/wireless/wl12xx/main.c
> > @@ -1338,6 +1338,28 @@ static int wl1271_op_suspend(struct ieee80211_hw *hw,
> >  	struct wl1271 *wl = hw->priv;
> >  	wl1271_debug(DEBUG_MAC80211, "mac80211 suspend wow=%d", !!wow);
> >  	wl->wow_enabled = !!wow;
> > +	if (wl->wow_enabled) {
> > +		/* flush any remaining work */
> > +		wl1271_debug(DEBUG_MAC80211, "flushing remaining works");
> > +		flush_delayed_work(&wl->scan_complete_work);
> > +
> > +		/*
> > +		 * disable and re-enable interrupts in order to flush
> > +		 * the threaded_irq
> > +		 */
> > +		wl1271_disable_interrupts(wl);
> > +
> > +		/*
> > +		 * set suspended flag to avoid triggering a new threaded_irq
> > +		 * work. no need for spinlock as interrupts are disabled.
> > +		 */
> > +		set_bit(WL1271_FLAG_SUSPENDED, &wl->flags);
> 
> The set_bit() function is atomic, so you wouldn't need the spinlock here
> anyway.  Couldn't you use __set_bit() instead, to avoid the expense of
> using an atomic function when it's not necessary?

That's only possible if the spinlock is held for all modifications of
the variable. If there are any modifications that don't hold it, then
you need the atomic version.

johannes

Re: [PATCH 4/7] wl12xx: prevent scheduling while suspending (WoW enabled)

[Luciano Coelho]

On Thu, 2011-05-12 at 21:52 +0200, Johannes Berg wrote:
> On Thu, 2011-05-12 at 22:48 +0300, Luciano Coelho wrote:
> 
> > > diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
> > > index 9ca71ce..308855a 100644
> > > --- a/drivers/net/wireless/wl12xx/main.c
> > > +++ b/drivers/net/wireless/wl12xx/main.c
> > > @@ -1338,6 +1338,28 @@ static int wl1271_op_suspend(struct ieee80211_hw *hw,
> > >  	struct wl1271 *wl = hw->priv;
> > >  	wl1271_debug(DEBUG_MAC80211, "mac80211 suspend wow=%d", !!wow);
> > >  	wl->wow_enabled = !!wow;
> > > +	if (wl->wow_enabled) {
> > > +		/* flush any remaining work */
> > > +		wl1271_debug(DEBUG_MAC80211, "flushing remaining works");
> > > +		flush_delayed_work(&wl->scan_complete_work);
> > > +
> > > +		/*
> > > +		 * disable and re-enable interrupts in order to flush
> > > +		 * the threaded_irq
> > > +		 */
> > > +		wl1271_disable_interrupts(wl);
> > > +
> > > +		/*
> > > +		 * set suspended flag to avoid triggering a new threaded_irq
> > > +		 * work. no need for spinlock as interrupts are disabled.
> > > +		 */
> > > +		set_bit(WL1271_FLAG_SUSPENDED, &wl->flags);
> > 
> > The set_bit() function is atomic, so you wouldn't need the spinlock here
> > anyway.  Couldn't you use __set_bit() instead, to avoid the expense of
> > using an atomic function when it's not necessary?
> 
> That's only possible if the spinlock is held for all modifications of
> the variable. If there are any modifications that don't hold it, then
> you need the atomic version.

Ah, you're right.   wl->flags is being modified in other places too (ie.
other bits are being set/cleared in other parts of the code), so there
could be some conflicts.  I had just considered the flag as an
independent boolean, which it obviously isn't.

My next comment about changing to __set_bit() can also be disregarded.

-- 
Cheers,
Luca.

Re: [PATCH 4/7] wl12xx: prevent scheduling while suspending (WoW enabled)

[Eliad Peller]

On Thu, May 12, 2011 at 10:48 PM, Luciano Coelho <coelho@ti.com> wrote:
> On Wed, 2011-05-11 at 11:54 +0300, Eliad Peller wrote:
>> When WoW is enabled, the interface will stay up and the chip will
>> be powered on, so we have to flush/cancel any remaining work, and
>> prevent the irq handler from scheduling a new work until the system
>> is resumed.
>>
>> Add 2 new flags:
>> * WL1271_FLAG_SUSPENDED - the system is (about to be) suspended.
>> * WL1271_FLAG_PENDING_WORK - there is a pending irq work which
>>    should be scheduled when the system is being resumed.
>>
>> In order to wake-up the system while getting an irq, we initialize
>> the device as wakeup device, and calling pm_wakeup_event() upon
>> getting the interrupt (while the system is about to be suspended)
>>
>> Signed-off-by: Eliad Peller <eliad@wizery.com>
>> ---
>
> [...]
>
>> +             if (run_irq_work) {
>> +                     wl1271_debug(DEBUG_MAC80211,
>> +                                  "run postponed irq_work directly");
>> +                     wl1271_irq(0, wl);
>> +                     wl1271_enable_interrupts(wl);
>
> Shouldn't this wl1271_enable_interrupts() be outside the if? Don't you
> want to enable interrupts again when there was no pending work?

by default, the interrupts are enabled (we need them in order to wake
up the device).
we disable them only after getting an interrupt and setting the
pending_work bit (see wl1271_hardirq())

>
>
>> diff --git a/drivers/net/wireless/wl12xx/sdio.c b/drivers/net/wireless/wl12xx/sdio.c
>> index 5b03fd5..bf2a6ad 100644
>> --- a/drivers/net/wireless/wl12xx/sdio.c
>> +++ b/drivers/net/wireless/wl12xx/sdio.c
>> @@ -72,6 +72,7 @@ static irqreturn_t wl1271_hardirq(int irq, void *cookie)
>>  {
>>       struct wl1271 *wl = cookie;
>>       unsigned long flags;
>> +     bool skip = false;
>>
>>       wl1271_debug(DEBUG_IRQ, "IRQ");
>>
>> @@ -82,8 +83,20 @@ static irqreturn_t wl1271_hardirq(int irq, void *cookie)
>>               complete(wl->elp_compl);
>>               wl->elp_compl = NULL;
>>       }
>> +
>> +     if (test_bit(WL1271_FLAG_SUSPENDED, &wl->flags)) {
>> +             /* don't enqueue a work right now. mark it as pending */
>> +             set_bit(WL1271_FLAG_PENDING_WORK, &wl->flags);
>> +             wl1271_debug(DEBUG_IRQ, "should not enqueue work");
>> +             disable_irq_nosync(wl->irq);
>> +             pm_wakeup_event(wl1271_sdio_wl_to_dev(wl), 0);
>> +             skip = true;
>> +     }
>>       spin_unlock_irqrestore(&wl->wl_lock, flags);
>>
>> +     if (skip)
>> +             return IRQ_HANDLED;
>> +
>>       return IRQ_WAKE_THREAD;
>>  }
>
> Could be nicer to just skip the skip variable (unintended pun
> intended ;).  You can do it like this:
>
> if (test_bit...) {
>        ...
>        spin_unlock_irqrestore();
>        return IRQ_HANDLED;
> }
> spin_unlock_irqrestore();
> return IRQ_WAKE_THREAD;
>
> Looks a bit nicer to me, but I don't mind if you prefer not skipping the
> skip. :)
>

well, i don't have a preference here how to handle the HANDLED case. ;)
so i'll change it if you prefer it that way.

thanks,
Eliad.

[PATCH 5/7] wl12xx_sdio: declare support for NL80211_WOW_TRIGGER_ANYTHING trigger

[Eliad Peller]

Since wowlan requires the ability to stay awake while the host
is suspended, declare support for NL80211_WOW_TRIGGER_ANYTHING
if the MMC_PM_KEEP_POWER capability is being supported.

Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 drivers/net/wireless/wl12xx/sdio.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/sdio.c b/drivers/net/wireless/wl12xx/sdio.c
index bf2a6ad..dacd00c 100644
--- a/drivers/net/wireless/wl12xx/sdio.c
+++ b/drivers/net/wireless/wl12xx/sdio.c
@@ -234,6 +234,7 @@ static int __devinit wl1271_probe(struct sdio_func *func,
 	const struct wl12xx_platform_data *wlan_data;
 	struct wl1271 *wl;
 	unsigned long irqflags;
+	mmc_pm_flag_t mmcflags;
 	int ret;
 
 	/* We are only able to handle the wlan function */
@@ -285,6 +286,13 @@ static int __devinit wl1271_probe(struct sdio_func *func,
 
 	disable_irq(wl->irq);
 
+	/* if sdio can keep power while host is suspended, enable wow */
+	mmcflags = sdio_get_host_pm_caps(func);
+	wl1271_debug(DEBUG_SDIO, "sdio PM caps = 0x%x", mmcflags);
+
+	if (mmcflags & MMC_PM_KEEP_POWER)
+		hw->wiphy->wowlan.flags = WIPHY_WOWLAN_ANY;
+
 	ret = wl1271_init_ieee80211(wl);
 	if (ret)
 		goto out_irq;
-- 
1.7.0.4

[PATCH 6/7] wl12xx: add ps completion event

[Eliad Peller]

Signal when psm was entered successfully, if the completion
variable is being set.

Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 drivers/net/wireless/wl12xx/event.c  |    7 +++++++
 drivers/net/wireless/wl12xx/wl12xx.h |    1 +
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/event.c b/drivers/net/wireless/wl12xx/event.c
index ae69330..21e52ed 100644
--- a/drivers/net/wireless/wl12xx/event.c
+++ b/drivers/net/wireless/wl12xx/event.c
@@ -135,6 +135,13 @@ static int wl1271_event_ps_report(struct wl1271 *wl,
 
 		/* enable beacon early termination */
 		ret = wl1271_acx_bet_enable(wl, true);
+		if (ret < 0)
+			break;
+
+		if (wl->ps_compl) {
+			complete(wl->ps_compl);
+			wl->ps_compl = NULL;
+		}
 		break;
 	default:
 		break;
diff --git a/drivers/net/wireless/wl12xx/wl12xx.h b/drivers/net/wireless/wl12xx/wl12xx.h
index daf941d..cf08a9d 100644
--- a/drivers/net/wireless/wl12xx/wl12xx.h
+++ b/drivers/net/wireless/wl12xx/wl12xx.h
@@ -512,6 +512,7 @@ struct wl1271 {
 	unsigned int rx_filter;
 
 	struct completion *elp_compl;
+	struct completion *ps_compl;
 	struct delayed_work elp_work;
 	struct delayed_work pspoll_work;
 
-- 
1.7.0.4

Re: [PATCH 6/7] wl12xx: add ps completion event

[Luciano Coelho]

On Wed, 2011-05-11 at 11:54 +0300, Eliad Peller wrote:
> Signal when psm was entered successfully, if the completion
> variable is being set.
> 
> Signed-off-by: Eliad Peller <eliad@wizery.com>
> ---
>  drivers/net/wireless/wl12xx/event.c  |    7 +++++++
>  drivers/net/wireless/wl12xx/wl12xx.h |    1 +
>  2 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/wireless/wl12xx/event.c b/drivers/net/wireless/wl12xx/event.c
> index ae69330..21e52ed 100644
> --- a/drivers/net/wireless/wl12xx/event.c
> +++ b/drivers/net/wireless/wl12xx/event.c
> @@ -135,6 +135,13 @@ static int wl1271_event_ps_report(struct wl1271 *wl,
>  
>  		/* enable beacon early termination */
>  		ret = wl1271_acx_bet_enable(wl, true);
> +		if (ret < 0)
> +			break;
> +
> +		if (wl->ps_compl) {
> +			complete(wl->ps_compl);
> +			wl->ps_compl = NULL;
> +		}
>  		break;
>  	default:
>  		break;
> diff --git a/drivers/net/wireless/wl12xx/wl12xx.h b/drivers/net/wireless/wl12xx/wl12xx.h
> index daf941d..cf08a9d 100644
> --- a/drivers/net/wireless/wl12xx/wl12xx.h
> +++ b/drivers/net/wireless/wl12xx/wl12xx.h
> @@ -512,6 +512,7 @@ struct wl1271 {
>  	unsigned int rx_filter;
>  
>  	struct completion *elp_compl;
> +	struct completion *ps_compl;
>  	struct delayed_work elp_work;
>  	struct delayed_work pspoll_work;
>  

This patch doesn't make sense by itself, without the next one.  Can you
squash them?

-- 
Cheers,
Luca.

Re: [PATCH 6/7] wl12xx: add ps completion event

[Eliad Peller]

On Thu, May 12, 2011 at 11:10 PM, Luciano Coelho <coelho@ti.com> wrote:
> On Wed, 2011-05-11 at 11:54 +0300, Eliad Peller wrote:
>> Signal when psm was entered successfully, if the completion
>> variable is being set.
>>
>> Signed-off-by: Eliad Peller <eliad@wizery.com>
>> ---
>>  drivers/net/wireless/wl12xx/event.c  |    7 +++++++
>>  drivers/net/wireless/wl12xx/wl12xx.h |    1 +
>>  2 files changed, 8 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/wireless/wl12xx/event.c b/drivers/net/wireless/wl12xx/event.c
>> index ae69330..21e52ed 100644
>> --- a/drivers/net/wireless/wl12xx/event.c
>> +++ b/drivers/net/wireless/wl12xx/event.c
>> @@ -135,6 +135,13 @@ static int wl1271_event_ps_report(struct wl1271 *wl,
>>
>>               /* enable beacon early termination */
>>               ret = wl1271_acx_bet_enable(wl, true);
>> +             if (ret < 0)
>> +                     break;
>> +
>> +             if (wl->ps_compl) {
>> +                     complete(wl->ps_compl);
>> +                     wl->ps_compl = NULL;
>> +             }
>>               break;
>>       default:
>>               break;
>> diff --git a/drivers/net/wireless/wl12xx/wl12xx.h b/drivers/net/wireless/wl12xx/wl12xx.h
>> index daf941d..cf08a9d 100644
>> --- a/drivers/net/wireless/wl12xx/wl12xx.h
>> +++ b/drivers/net/wireless/wl12xx/wl12xx.h
>> @@ -512,6 +512,7 @@ struct wl1271 {
>>       unsigned int rx_filter;
>>
>>       struct completion *elp_compl;
>> +     struct completion *ps_compl;
>>       struct delayed_work elp_work;
>>       struct delayed_work pspoll_work;
>>
>
> This patch doesn't make sense by itself, without the next one.  Can you
> squash them?
>
sure.

Eliad.

[PATCH 7/7] wl12xx: enter/exit psm on wowlan suspend/resume

[Eliad Peller]

When operating as station, enter psm before suspending
the device into wowlan state.

Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 drivers/net/wireless/wl12xx/main.c |   81 ++++++++++++++++++++++++++++++++++++
 1 files changed, 81 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index 308855a..b4dd1d9 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -1332,6 +1332,79 @@ static struct notifier_block wl1271_dev_notifier = {
 	.notifier_call = wl1271_dev_notify,
 };
 
+static int wl1271_configure_suspend(struct wl1271 *wl)
+{
+	int ret;
+
+	if (wl->bss_type != BSS_TYPE_STA_BSS)
+		return 0;
+
+	mutex_lock(&wl->mutex);
+
+	ret = wl1271_ps_elp_wakeup(wl);
+	if (ret < 0)
+		goto out_unlock;
+
+	/* enter psm if needed*/
+	if (!test_bit(WL1271_FLAG_PSM, &wl->flags)) {
+		DECLARE_COMPLETION_ONSTACK(compl);
+
+		wl->ps_compl = &compl;
+		ret = wl1271_ps_set_mode(wl, STATION_POWER_SAVE_MODE,
+				   wl->basic_rate, true);
+		if (ret < 0)
+			goto out_sleep;
+
+		/* we must unlock here so we will be able to get events */
+		wl1271_ps_elp_sleep(wl);
+		mutex_unlock(&wl->mutex);
+
+		ret = wait_for_completion_timeout(
+			&compl, msecs_to_jiffies(500));
+		if (ret <= 0) {
+			wl1271_warning("couldn't enter ps mode!");
+			ret = -EBUSY;
+			goto out;
+		}
+
+		/* take mutex again, and wakeup */
+		mutex_lock(&wl->mutex);
+
+		ret = wl1271_ps_elp_wakeup(wl);
+		if (ret < 0)
+			goto out_unlock;
+	}
+out_sleep:
+	wl1271_ps_elp_sleep(wl);
+out_unlock:
+	mutex_unlock(&wl->mutex);
+out:
+	return ret;
+
+}
+
+static void wl1271_configure_resume(struct wl1271 *wl)
+{
+	int ret;
+
+	if (wl->bss_type != BSS_TYPE_STA_BSS)
+		return;
+
+	mutex_lock(&wl->mutex);
+	ret = wl1271_ps_elp_wakeup(wl);
+	if (ret < 0)
+		goto out;
+
+	/* exit psm if it wasn't configured */
+	if (!test_bit(WL1271_FLAG_PSM_REQUESTED, &wl->flags))
+		wl1271_ps_set_mode(wl, STATION_ACTIVE_MODE,
+				   wl->basic_rate, true);
+
+	wl1271_ps_elp_sleep(wl);
+out:
+	mutex_unlock(&wl->mutex);
+}
+
 static int wl1271_op_suspend(struct ieee80211_hw *hw,
 			    struct cfg80211_wowlan *wow)
 {
@@ -1339,6 +1412,12 @@ static int wl1271_op_suspend(struct ieee80211_hw *hw,
 	wl1271_debug(DEBUG_MAC80211, "mac80211 suspend wow=%d", !!wow);
 	wl->wow_enabled = !!wow;
 	if (wl->wow_enabled) {
+		int ret;
+		ret = wl1271_configure_suspend(wl);
+		if (ret < 0) {
+			wl1271_warning("couldn't prepare device to suspend");
+			return ret;
+		}
 		/* flush any remaining work */
 		wl1271_debug(DEBUG_MAC80211, "flushing remaining works");
 		flush_delayed_work(&wl->scan_complete_work);
@@ -1390,6 +1469,8 @@ static int wl1271_op_resume(struct ieee80211_hw *hw)
 			wl1271_irq(0, wl);
 			wl1271_enable_interrupts(wl);
 		}
+
+		wl1271_configure_resume(wl);
 	}
 
 	return 0;
-- 
1.7.0.4

Re: [PATCH 7/7] wl12xx: enter/exit psm on wowlan suspend/resume

[Luciano Coelho]

On Wed, 2011-05-11 at 11:54 +0300, Eliad Peller wrote:
> When operating as station, enter psm before suspending
> the device into wowlan state.
> 
> Signed-off-by: Eliad Peller <eliad@wizery.com>
> ---

[...]

> diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
> index 308855a..b4dd1d9 100644
> --- a/drivers/net/wireless/wl12xx/main.c
> +++ b/drivers/net/wireless/wl12xx/main.c
> @@ -1332,6 +1332,79 @@ static struct notifier_block wl1271_dev_notifier = {
>  	.notifier_call = wl1271_dev_notify,
>  };
>  
> +static int wl1271_configure_suspend(struct wl1271 *wl)
> +{
> +	int ret;
> +
> +	if (wl->bss_type != BSS_TYPE_STA_BSS)
> +		return 0;
> +
> +	mutex_lock(&wl->mutex);
> +
> +	ret = wl1271_ps_elp_wakeup(wl);
> +	if (ret < 0)
> +		goto out_unlock;
> +
> +	/* enter psm if needed*/
> +	if (!test_bit(WL1271_FLAG_PSM, &wl->flags)) {
> +		DECLARE_COMPLETION_ONSTACK(compl);
> +
> +		wl->ps_compl = &compl;
> +		ret = wl1271_ps_set_mode(wl, STATION_POWER_SAVE_MODE,
> +				   wl->basic_rate, true);
> +		if (ret < 0)
> +			goto out_sleep;
> +
> +		/* we must unlock here so we will be able to get events */
> +		wl1271_ps_elp_sleep(wl);
> +		mutex_unlock(&wl->mutex);
> +
> +		ret = wait_for_completion_timeout(
> +			&compl, msecs_to_jiffies(500));

We have this for the ELP wakeup:

#define WL1271_WAKEUP_TIMEOUT 500

We should have a similar definition PS timeout.  And is this value just
a guess or is there a real meaning to it? Can these timeouts be combined
instead of having separate defines?


-- 
Cheers,
Luca.

Re: [PATCH 7/7] wl12xx: enter/exit psm on wowlan suspend/resume

[Eliad Peller]

On Thu, May 12, 2011 at 11:24 PM, Luciano Coelho <coelho@ti.com> wrote:
> On Wed, 2011-05-11 at 11:54 +0300, Eliad Peller wrote:
>> When operating as station, enter psm before suspending
>> the device into wowlan state.
>>
>> Signed-off-by: Eliad Peller <eliad@wizery.com>
>> ---
>
> [...]
>
>> diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
>> index 308855a..b4dd1d9 100644
>> --- a/drivers/net/wireless/wl12xx/main.c
>> +++ b/drivers/net/wireless/wl12xx/main.c
>> @@ -1332,6 +1332,79 @@ static struct notifier_block wl1271_dev_notifier = {
>>       .notifier_call = wl1271_dev_notify,
>>  };
>>
>> +static int wl1271_configure_suspend(struct wl1271 *wl)
>> +{
>> +     int ret;
>> +
>> +     if (wl->bss_type != BSS_TYPE_STA_BSS)
>> +             return 0;
>> +
>> +     mutex_lock(&wl->mutex);
>> +
>> +     ret = wl1271_ps_elp_wakeup(wl);
>> +     if (ret < 0)
>> +             goto out_unlock;
>> +
>> +     /* enter psm if needed*/
>> +     if (!test_bit(WL1271_FLAG_PSM, &wl->flags)) {
>> +             DECLARE_COMPLETION_ONSTACK(compl);
>> +
>> +             wl->ps_compl = &compl;
>> +             ret = wl1271_ps_set_mode(wl, STATION_POWER_SAVE_MODE,
>> +                                wl->basic_rate, true);
>> +             if (ret < 0)
>> +                     goto out_sleep;
>> +
>> +             /* we must unlock here so we will be able to get events */
>> +             wl1271_ps_elp_sleep(wl);
>> +             mutex_unlock(&wl->mutex);
>> +
>> +             ret = wait_for_completion_timeout(
>> +                     &compl, msecs_to_jiffies(500));
>
> We have this for the ELP wakeup:
>
> #define WL1271_WAKEUP_TIMEOUT 500
>
> We should have a similar definition PS timeout.  And is this value just
> a guess or is there a real meaning to it? Can these timeouts be combined
> instead of having separate defines?
>
this number is arbitrary.
i don't think we should combine it with the elp timeout, as they
represent different arbitrary values :)
but i'll #define it.

thanks,
Eliad.

Re: [PATCH 0/7] wl12xx: add initial wowlan support

[Luciano Coelho]

Hi Eliad,

On Wed, 2011-05-11 at 11:54 +0300, Eliad Peller wrote:
> Note that this patchset needs some additional omap/mmc patches in 
> order to support the MMC_PM_KEEP_POWER flag and wake-irq correctly
> (however, if the MMC_PM_KEEP_POWER flag is not being supported we 
> don't advertise support for wowlan triggers, so this patchset alone
> shouldn't harm). 

Do you know if these patches are queued in linux-omap for 2.6.40?

-- 
Cheers,
Luca.

Re: [PATCH 0/7] wl12xx: add initial wowlan support

[Ohad Ben-Cohen]

Hi Luca,

On Wed, May 11, 2011 at 1:57 AM, Luciano Coelho <coelho@ti.com> wrote:
> On Wed, 2011-05-11 at 11:54 +0300, Eliad Peller wrote:
>> Note that this patchset needs some additional omap/mmc patches in
>> order to support the MMC_PM_KEEP_POWER flag and wake-irq correctly
>> (however, if the MMC_PM_KEEP_POWER flag is not being supported we
>> don't advertise support for wowlan triggers, so this patchset alone
>> shouldn't harm).
>
> Do you know if these patches are queued in linux-omap for 2.6.40?

Not yet, we wanted first to get driver support for this, and then we'd
start adding the required platform support (for several platforms btw,
not only omap. so we'll start adding them one by one now).

Thanks,
Ohad.