[PATCH] wl12xx: Fix power control for CONFIG_RUNTIME_PM off configurations

[PATCH] wl12xx: Fix power control for CONFIG_RUNTIME_PM off configurations

[Pontus Fuchs]

On rmmod wl12xx will leave the sdio host powered off, regardless
off runtime PM being enabled or not. This leads to probe failure on
a second insmod, if runtime pm is disabled, because the sdio core
expects the host to be powered on.

Signed-off-by: Pontus Fuchs <pontus.fuchs@gmail.com>
---
 drivers/net/wireless/wl12xx/sdio.c |   49 ++++++++++++++++++++---------------
 1 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/sdio.c b/drivers/net/wireless/wl12xx/sdio.c
index 468a505..45115a3 100644
--- a/drivers/net/wireless/wl12xx/sdio.c
+++ b/drivers/net/wireless/wl12xx/sdio.c
@@ -117,14 +117,17 @@ static void wl12xx_sdio_raw_write(struct device *child, int addr, void *buf,
 		dev_err(child->parent, "sdio write failed (%d)\n", ret);
 }
 
-static int wl12xx_sdio_power_on(struct wl12xx_sdio_glue *glue)
+static int wl12xx_sdio_power_on(struct sdio_func *func, bool noresume)
 {
-	int ret;
-	struct sdio_func *func = dev_to_sdio_func(glue->dev);
+	int ret = 0;
 
 	/* If enabled, tell runtime PM not to power off the card */
 	if (pm_runtime_enabled(&func->dev)) {
-		ret = pm_runtime_get_sync(&func->dev);
+		if (noresume)
+			pm_runtime_get_noresume(&func->dev);
+		else
+			ret = pm_runtime_get_sync(&func->dev);
+
 		if (ret < 0)
 			goto out;
 	} else {
@@ -134,20 +137,13 @@ static int wl12xx_sdio_power_on(struct wl12xx_sdio_glue *glue)
 			goto out;
 	}
 
-	sdio_claim_host(func);
-	sdio_enable_func(func);
-
 out:
 	return ret;
 }
 
-static int wl12xx_sdio_power_off(struct wl12xx_sdio_glue *glue)
+static int wl12xx_sdio_power_off(struct sdio_func *func)
 {
 	int ret;
-	struct sdio_func *func = dev_to_sdio_func(glue->dev);
-
-	sdio_disable_func(func);
-	sdio_release_host(func);
 
 	/* Power off the card manually, even if runtime PM is enabled. */
 	ret = mmc_power_save_host(func->card->host);
@@ -164,11 +160,21 @@ static int wl12xx_sdio_power_off(struct wl12xx_sdio_glue *glue)
 static int wl12xx_sdio_set_power(struct device *child, bool enable)
 {
 	struct wl12xx_sdio_glue *glue = dev_get_drvdata(child->parent);
+	struct sdio_func *func = dev_to_sdio_func(glue->dev);
+	int ret;
 
-	if (enable)
-		return wl12xx_sdio_power_on(glue);
-	else
-		return wl12xx_sdio_power_off(glue);
+	if (enable) {
+		ret = wl12xx_sdio_power_on(func, false);
+		if (ret)
+			return ret;
+		sdio_claim_host(func);
+		sdio_enable_func(func);
+		return ret;
+	} else {
+		sdio_disable_func(func);
+		sdio_release_host(func);
+		return wl12xx_sdio_power_off(func);
+	}
 }
 
 static struct wl1271_if_operations sdio_ops = {
@@ -223,8 +229,8 @@ static int __devinit wl1271_probe(struct sdio_func *func,
 
 	sdio_set_drvdata(func, glue);
 
-	/* Tell PM core that we don't need the card to be powered now */
-	pm_runtime_put_noidle(&func->dev);
+	/* Power off as we don't need the card to be powered now */
+	wl12xx_sdio_power_off(func);
 
 	glue->core = platform_device_alloc("wl12xx", -1);
 	if (!glue->core) {
@@ -275,11 +281,12 @@ static void __devexit wl1271_remove(struct sdio_func *func)
 {
 	struct wl12xx_sdio_glue *glue = sdio_get_drvdata(func);
 
-	/* Undo decrement done above in wl1271_probe */
-	pm_runtime_get_noresume(&func->dev);
-
 	platform_device_del(glue->core);
 	platform_device_put(glue->core);
+
+	/* Leave the sdio host in the state it was in before probing */
+	wl12xx_sdio_power_on(func, true);
+
 	kfree(glue);
 }
 
-- 
1.7.5.4

Re: [PATCH] wl12xx: Fix power control for CONFIG_RUNTIME_PM off configurations

[Ohad Ben-Cohen]

Hi Pontus,

On Wed, Dec 21, 2011 at 1:41 PM, Pontus Fuchs <pontus.fuchs@gmail.com> wrote:
> On rmmod wl12xx will leave the sdio host powered off, regardless
> off runtime PM being enabled or not. This leads to probe failure on
> a second insmod, if runtime pm is disabled, because the sdio core
> expects the host to be powered on.

Just gave this patch a quick glance, and it looks rather nasty :)

> @@ -223,8 +229,8 @@ static int __devinit wl1271_probe(struct sdio_func *func,
>
>        sdio_set_drvdata(func, glue);
>
> -       /* Tell PM core that we don't need the card to be powered now */
> -       pm_runtime_put_noidle(&func->dev);
> +       /* Power off as we don't need the card to be powered now */
> +       wl12xx_sdio_power_off(func);

This in particular looks wrong.

You bypass runtime PM by manipulating the power state of the card
directly, and as a result you leave the runtime PM state out of sync
with the real power state of the device.

This is very unhealthy, and can lead to the system powering on the
card on certain events (e.g. on system resume) even when the wlan
interface is down.

Regards,
Ohad.

Re: [PATCH] wl12xx: Fix power control for CONFIG_RUNTIME_PM off configurations

[Pontus Fuchs]

Hi,

On 2011-12-22 07:39, Ohad Ben-Cohen wrote:
>> -       /* Tell PM core that we don't need the card to be powered now */
>> -       pm_runtime_put_noidle(&func->dev);
>> +       /* Power off as we don't need the card to be powered now */
>> +       wl12xx_sdio_power_off(func);
>
> This in particular looks wrong.
>
> You bypass runtime PM by manipulating the power state of the card
> directly, and as a result you leave the runtime PM state out of sync
> with the real power state of the device.

Even without my changes we will call wl12xx_sdio_power_on/off for every 
ifconfig up/down. Is that also a problem or is that case different?

//Pontus

Re: [PATCH] wl12xx: Fix power control for CONFIG_RUNTIME_PM off configurations

[Ohad Ben-Cohen]

On Thu, Dec 22, 2011 at 9:44 AM, Pontus Fuchs <pontus.fuchs@gmail.com> wrote:
> Even without my changes we will call wl12xx_sdio_power_on/off for every
> ifconfig up/down. Is that also a problem or is that case different?

That's ok: if runtime PM is enabled, we always call its API.