[PATCH] libertas: fix return codes in if_sdio_suspend()

[PATCH] libertas: fix return codes in if_sdio_suspend()

[Daniel Mack]

Prior to commit 573185cc7e64 ("mmc: core: Invoke sdio func driver's PM
callbacks from the sdio bus"), the MMC core used to call into the power
management functions of SDIO clients itself and removed the card if the
return code was non-zero. IOW, the mmc handled errors gracefully and didn't
upchain them to the pm core.

Since this change, the mmc core relies on generic power management
functions which treat all errors as a reason to cancel the suspend
immediately. This causes suspend attempts to fail when the libertas
driver is loaded.

To fix this, power down the card explicitly in if_sdio_suspend() when we
know we're about to lose power and return success. Also set a flag in these
cases, and power up the card again in if_sdio_resume().

Signed-off-by: Daniel Mack <daniel@zonque.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Chris Ball <chris@printf.net>
---
 drivers/net/wireless/marvell/libertas/dev.h   |  1 +
 .../net/wireless/marvell/libertas/if_sdio.c   | 28 +++++++++++++++----
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas/dev.h b/drivers/net/wireless/marvell/libertas/dev.h
index dd1ee1f0af48..469134930026 100644
--- a/drivers/net/wireless/marvell/libertas/dev.h
+++ b/drivers/net/wireless/marvell/libertas/dev.h
@@ -104,6 +104,7 @@ struct lbs_private {
 	u8 fw_ready;
 	u8 surpriseremoved;
 	u8 setup_fw_on_resume;
+	u8 power_up_on_resume;
 	int (*hw_host_to_card) (struct lbs_private *priv, u8 type, u8 *payload, u16 nb);
 	void (*reset_card) (struct lbs_private *priv);
 	int (*power_save) (struct lbs_private *priv);
diff --git a/drivers/net/wireless/marvell/libertas/if_sdio.c b/drivers/net/wireless/marvell/libertas/if_sdio.c
index 2300e796c6ab..f43807663b1b 100644
--- a/drivers/net/wireless/marvell/libertas/if_sdio.c
+++ b/drivers/net/wireless/marvell/libertas/if_sdio.c
@@ -1290,15 +1290,23 @@ static void if_sdio_remove(struct sdio_func *func)
 static int if_sdio_suspend(struct device *dev)
 {
 	struct sdio_func *func = dev_to_sdio_func(dev);
-	int ret;
 	struct if_sdio_card *card = sdio_get_drvdata(func);
+	struct lbs_private *priv = card->priv;
+	int ret;
 
 	mmc_pm_flag_t flags = sdio_get_host_pm_caps(func);
+	priv->power_up_on_resume = false;
 
 	/* If we're powered off anyway, just let the mmc layer remove the
 	 * card. */
-	if (!lbs_iface_active(card->priv))
-		return -ENOSYS;
+	if (!lbs_iface_active(priv)) {
+		if (priv->fw_ready) {
+			priv->power_up_on_resume = true;
+			if_sdio_power_off(card);
+		}
+
+		return 0;
+	}
 
 	dev_info(dev, "%s: suspend: PM flags = 0x%x\n",
 		 sdio_func_id(func), flags);
@@ -1306,9 +1314,14 @@ static int if_sdio_suspend(struct device *dev)
 	/* If we aren't being asked to wake on anything, we should bail out
 	 * and let the SD stack power down the card.
 	 */
-	if (card->priv->wol_criteria == EHS_REMOVE_WAKEUP) {
+	if (priv->wol_criteria == EHS_REMOVE_WAKEUP) {
 		dev_info(dev, "Suspend without wake params -- powering down card\n");
-		return -ENOSYS;
+		if (priv->fw_ready) {
+			priv->power_up_on_resume = true;
+			if_sdio_power_off(card);
+		}
+
+		return 0;
 	}
 
 	if (!(flags & MMC_PM_KEEP_POWER)) {
@@ -1321,7 +1334,7 @@ static int if_sdio_suspend(struct device *dev)
 	if (ret)
 		return ret;
 
-	ret = lbs_suspend(card->priv);
+	ret = lbs_suspend(priv);
 	if (ret)
 		return ret;
 
@@ -1336,6 +1349,9 @@ static int if_sdio_resume(struct device *dev)
 
 	dev_info(dev, "%s: resume: we're back\n", sdio_func_id(func));
 
+	if (card->priv->power_up_on_resume)
+		if_sdio_power_on(card);
+
 	ret = lbs_resume(card->priv);
 
 	return ret;
-- 
2.17.1

[PATCH] libertas: fix return codes in if_sdio_suspend()

[Daniel Mack]

Prior to commit 573185cc7e64 ("mmc: core: Invoke sdio func driver's PM
callbacks from the sdio bus"), the MMC core used to call into the power
management functions of SDIO clients itself and removed the card if the
return code was non-zero. IOW, the mmc handled errors gracefully and didn't
upchain them to the pm core.

Since this change, the mmc core relies on generic power management
functions which treat all errors as a reason to cancel the suspend
immediately. This causes suspend attempts to fail when the libertas
driver is loaded.

To fix this, power down the card explicitly in if_sdio_suspend() when we
know we're about to lose power and return success. Also set a flag in these
cases, and power up the card again in if_sdio_resume().

Signed-off-by: Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
Cc: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Chris Ball <chris-OsFVWbfNK3isTnJN9+BGXg@public.gmane.org>
---
 drivers/net/wireless/marvell/libertas/dev.h   |  1 +
 .../net/wireless/marvell/libertas/if_sdio.c   | 28 +++++++++++++++----
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas/dev.h b/drivers/net/wireless/marvell/libertas/dev.h
index dd1ee1f0af48..469134930026 100644
--- a/drivers/net/wireless/marvell/libertas/dev.h
+++ b/drivers/net/wireless/marvell/libertas/dev.h
@@ -104,6 +104,7 @@ struct lbs_private {
 	u8 fw_ready;
 	u8 surpriseremoved;
 	u8 setup_fw_on_resume;
+	u8 power_up_on_resume;
 	int (*hw_host_to_card) (struct lbs_private *priv, u8 type, u8 *payload, u16 nb);
 	void (*reset_card) (struct lbs_private *priv);
 	int (*power_save) (struct lbs_private *priv);
diff --git a/drivers/net/wireless/marvell/libertas/if_sdio.c b/drivers/net/wireless/marvell/libertas/if_sdio.c
index 2300e796c6ab..f43807663b1b 100644
--- a/drivers/net/wireless/marvell/libertas/if_sdio.c
+++ b/drivers/net/wireless/marvell/libertas/if_sdio.c
@@ -1290,15 +1290,23 @@ static void if_sdio_remove(struct sdio_func *func)
 static int if_sdio_suspend(struct device *dev)
 {
 	struct sdio_func *func = dev_to_sdio_func(dev);
-	int ret;
 	struct if_sdio_card *card = sdio_get_drvdata(func);
+	struct lbs_private *priv = card->priv;
+	int ret;
 
 	mmc_pm_flag_t flags = sdio_get_host_pm_caps(func);
+	priv->power_up_on_resume = false;
 
 	/* If we're powered off anyway, just let the mmc layer remove the
 	 * card. */
-	if (!lbs_iface_active(card->priv))
-		return -ENOSYS;
+	if (!lbs_iface_active(priv)) {
+		if (priv->fw_ready) {
+			priv->power_up_on_resume = true;
+			if_sdio_power_off(card);
+		}
+
+		return 0;
+	}
 
 	dev_info(dev, "%s: suspend: PM flags = 0x%x\n",
 		 sdio_func_id(func), flags);
@@ -1306,9 +1314,14 @@ static int if_sdio_suspend(struct device *dev)
 	/* If we aren't being asked to wake on anything, we should bail out
 	 * and let the SD stack power down the card.
 	 */
-	if (card->priv->wol_criteria == EHS_REMOVE_WAKEUP) {
+	if (priv->wol_criteria == EHS_REMOVE_WAKEUP) {
 		dev_info(dev, "Suspend without wake params -- powering down card\n");
-		return -ENOSYS;
+		if (priv->fw_ready) {
+			priv->power_up_on_resume = true;
+			if_sdio_power_off(card);
+		}
+
+		return 0;
 	}
 
 	if (!(flags & MMC_PM_KEEP_POWER)) {
@@ -1321,7 +1334,7 @@ static int if_sdio_suspend(struct device *dev)
 	if (ret)
 		return ret;
 
-	ret = lbs_suspend(card->priv);
+	ret = lbs_suspend(priv);
 	if (ret)
 		return ret;
 
@@ -1336,6 +1349,9 @@ static int if_sdio_resume(struct device *dev)
 
 	dev_info(dev, "%s: resume: we're back\n", sdio_func_id(func));
 
+	if (card->priv->power_up_on_resume)
+		if_sdio_power_on(card);
+
 	ret = lbs_resume(card->priv);
 
 	return ret;
-- 
2.17.1

Re: [PATCH] libertas: fix return codes in if_sdio_suspend()

[Daniel Mack]

Ah, the subject line of this patch could actually be improved. Let me 
know if you're happy with the contents of this patch, so I can resend.

Thanks,
Daniel


On Tuesday, June 26, 2018 10:48 PM, Daniel Mack wrote:
> Prior to commit 573185cc7e64 ("mmc: core: Invoke sdio func driver's PM
> callbacks from the sdio bus"), the MMC core used to call into the power
> management functions of SDIO clients itself and removed the card if the
> return code was non-zero. IOW, the mmc handled errors gracefully and didn't
> upchain them to the pm core.
> 
> Since this change, the mmc core relies on generic power management
> functions which treat all errors as a reason to cancel the suspend
> immediately. This causes suspend attempts to fail when the libertas
> driver is loaded.
> 
> To fix this, power down the card explicitly in if_sdio_suspend() when we
> know we're about to lose power and return success. Also set a flag in these
> cases, and power up the card again in if_sdio_resume().
> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Chris Ball <chris@printf.net>
> ---
>   drivers/net/wireless/marvell/libertas/dev.h   |  1 +
>   .../net/wireless/marvell/libertas/if_sdio.c   | 28 +++++++++++++++----
>   2 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/libertas/dev.h b/drivers/net/wireless/marvell/libertas/dev.h
> index dd1ee1f0af48..469134930026 100644
> --- a/drivers/net/wireless/marvell/libertas/dev.h
> +++ b/drivers/net/wireless/marvell/libertas/dev.h
> @@ -104,6 +104,7 @@ struct lbs_private {
>   	u8 fw_ready;
>   	u8 surpriseremoved;
>   	u8 setup_fw_on_resume;
> +	u8 power_up_on_resume;
>   	int (*hw_host_to_card) (struct lbs_private *priv, u8 type, u8 *payload, u16 nb);
>   	void (*reset_card) (struct lbs_private *priv);
>   	int (*power_save) (struct lbs_private *priv);
> diff --git a/drivers/net/wireless/marvell/libertas/if_sdio.c b/drivers/net/wireless/marvell/libertas/if_sdio.c
> index 2300e796c6ab..f43807663b1b 100644
> --- a/drivers/net/wireless/marvell/libertas/if_sdio.c
> +++ b/drivers/net/wireless/marvell/libertas/if_sdio.c
> @@ -1290,15 +1290,23 @@ static void if_sdio_remove(struct sdio_func *func)
>   static int if_sdio_suspend(struct device *dev)
>   {
>   	struct sdio_func *func = dev_to_sdio_func(dev);
> -	int ret;
>   	struct if_sdio_card *card = sdio_get_drvdata(func);
> +	struct lbs_private *priv = card->priv;
> +	int ret;
>   
>   	mmc_pm_flag_t flags = sdio_get_host_pm_caps(func);
> +	priv->power_up_on_resume = false;
>   
>   	/* If we're powered off anyway, just let the mmc layer remove the
>   	 * card. */
> -	if (!lbs_iface_active(card->priv))
> -		return -ENOSYS;
> +	if (!lbs_iface_active(priv)) {
> +		if (priv->fw_ready) {
> +			priv->power_up_on_resume = true;
> +			if_sdio_power_off(card);
> +		}
> +
> +		return 0;
> +	}
>   
>   	dev_info(dev, "%s: suspend: PM flags = 0x%x\n",
>   		 sdio_func_id(func), flags);
> @@ -1306,9 +1314,14 @@ static int if_sdio_suspend(struct device *dev)
>   	/* If we aren't being asked to wake on anything, we should bail out
>   	 * and let the SD stack power down the card.
>   	 */
> -	if (card->priv->wol_criteria == EHS_REMOVE_WAKEUP) {
> +	if (priv->wol_criteria == EHS_REMOVE_WAKEUP) {
>   		dev_info(dev, "Suspend without wake params -- powering down card\n");
> -		return -ENOSYS;
> +		if (priv->fw_ready) {
> +			priv->power_up_on_resume = true;
> +			if_sdio_power_off(card);
> +		}
> +
> +		return 0;
>   	}
>   
>   	if (!(flags & MMC_PM_KEEP_POWER)) {
> @@ -1321,7 +1334,7 @@ static int if_sdio_suspend(struct device *dev)
>   	if (ret)
>   		return ret;
>   
> -	ret = lbs_suspend(card->priv);
> +	ret = lbs_suspend(priv);
>   	if (ret)
>   		return ret;
>   
> @@ -1336,6 +1349,9 @@ static int if_sdio_resume(struct device *dev)
>   
>   	dev_info(dev, "%s: resume: we're back\n", sdio_func_id(func));
>   
> +	if (card->priv->power_up_on_resume)
> +		if_sdio_power_on(card);
> +
>   	ret = lbs_resume(card->priv);
>   
>   	return ret;
> 

Re: [PATCH] libertas: fix return codes in if_sdio_suspend()

[Daniel Mack]

Ah, the subject line of this patch could actually be improved. Let me 
know if you're happy with the contents of this patch, so I can resend.

Thanks,
Daniel


On Tuesday, June 26, 2018 10:48 PM, Daniel Mack wrote:
> Prior to commit 573185cc7e64 ("mmc: core: Invoke sdio func driver's PM
> callbacks from the sdio bus"), the MMC core used to call into the power
> management functions of SDIO clients itself and removed the card if the
> return code was non-zero. IOW, the mmc handled errors gracefully and didn't
> upchain them to the pm core.
> 
> Since this change, the mmc core relies on generic power management
> functions which treat all errors as a reason to cancel the suspend
> immediately. This causes suspend attempts to fail when the libertas
> driver is loaded.
> 
> To fix this, power down the card explicitly in if_sdio_suspend() when we
> know we're about to lose power and return success. Also set a flag in these
> cases, and power up the card again in if_sdio_resume().
> 
> Signed-off-by: Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
> Cc: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Chris Ball <chris-OsFVWbfNK3isTnJN9+BGXg@public.gmane.org>
> ---
>   drivers/net/wireless/marvell/libertas/dev.h   |  1 +
>   .../net/wireless/marvell/libertas/if_sdio.c   | 28 +++++++++++++++----
>   2 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/libertas/dev.h b/drivers/net/wireless/marvell/libertas/dev.h
> index dd1ee1f0af48..469134930026 100644
> --- a/drivers/net/wireless/marvell/libertas/dev.h
> +++ b/drivers/net/wireless/marvell/libertas/dev.h
> @@ -104,6 +104,7 @@ struct lbs_private {
>   	u8 fw_ready;
>   	u8 surpriseremoved;
>   	u8 setup_fw_on_resume;
> +	u8 power_up_on_resume;
>   	int (*hw_host_to_card) (struct lbs_private *priv, u8 type, u8 *payload, u16 nb);
>   	void (*reset_card) (struct lbs_private *priv);
>   	int (*power_save) (struct lbs_private *priv);
> diff --git a/drivers/net/wireless/marvell/libertas/if_sdio.c b/drivers/net/wireless/marvell/libertas/if_sdio.c
> index 2300e796c6ab..f43807663b1b 100644
> --- a/drivers/net/wireless/marvell/libertas/if_sdio.c
> +++ b/drivers/net/wireless/marvell/libertas/if_sdio.c
> @@ -1290,15 +1290,23 @@ static void if_sdio_remove(struct sdio_func *func)
>   static int if_sdio_suspend(struct device *dev)
>   {
>   	struct sdio_func *func = dev_to_sdio_func(dev);
> -	int ret;
>   	struct if_sdio_card *card = sdio_get_drvdata(func);
> +	struct lbs_private *priv = card->priv;
> +	int ret;
>   
>   	mmc_pm_flag_t flags = sdio_get_host_pm_caps(func);
> +	priv->power_up_on_resume = false;
>   
>   	/* If we're powered off anyway, just let the mmc layer remove the
>   	 * card. */
> -	if (!lbs_iface_active(card->priv))
> -		return -ENOSYS;
> +	if (!lbs_iface_active(priv)) {
> +		if (priv->fw_ready) {
> +			priv->power_up_on_resume = true;
> +			if_sdio_power_off(card);
> +		}
> +
> +		return 0;
> +	}
>   
>   	dev_info(dev, "%s: suspend: PM flags = 0x%x\n",
>   		 sdio_func_id(func), flags);
> @@ -1306,9 +1314,14 @@ static int if_sdio_suspend(struct device *dev)
>   	/* If we aren't being asked to wake on anything, we should bail out
>   	 * and let the SD stack power down the card.
>   	 */
> -	if (card->priv->wol_criteria == EHS_REMOVE_WAKEUP) {
> +	if (priv->wol_criteria == EHS_REMOVE_WAKEUP) {
>   		dev_info(dev, "Suspend without wake params -- powering down card\n");
> -		return -ENOSYS;
> +		if (priv->fw_ready) {
> +			priv->power_up_on_resume = true;
> +			if_sdio_power_off(card);
> +		}
> +
> +		return 0;
>   	}
>   
>   	if (!(flags & MMC_PM_KEEP_POWER)) {
> @@ -1321,7 +1334,7 @@ static int if_sdio_suspend(struct device *dev)
>   	if (ret)
>   		return ret;
>   
> -	ret = lbs_suspend(card->priv);
> +	ret = lbs_suspend(priv);
>   	if (ret)
>   		return ret;
>   
> @@ -1336,6 +1349,9 @@ static int if_sdio_resume(struct device *dev)
>   
>   	dev_info(dev, "%s: resume: we're back\n", sdio_func_id(func));
>   
> +	if (card->priv->power_up_on_resume)
> +		if_sdio_power_on(card);
> +
>   	ret = lbs_resume(card->priv);
>   
>   	return ret;
> 

Re: [PATCH] libertas: fix return codes in if_sdio_suspend()

[Ulf Hansson]

On 26 June 2018 at 22:50, Daniel Mack <daniel@zonque.org> wrote:
> Ah, the subject line of this patch could actually be improved. Let me know
> if you're happy with the contents of this patch, so I can resend.
>
> Thanks,
> Daniel
>
>
>
> On Tuesday, June 26, 2018 10:48 PM, Daniel Mack wrote:
>>
>> Prior to commit 573185cc7e64 ("mmc: core: Invoke sdio func driver's PM
>> callbacks from the sdio bus"), the MMC core used to call into the power
>> management functions of SDIO clients itself and removed the card if the
>> return code was non-zero. IOW, the mmc handled errors gracefully and
>> didn't
>> upchain them to the pm core.
>>
>> Since this change, the mmc core relies on generic power management
>> functions which treat all errors as a reason to cancel the suspend
>> immediately. This causes suspend attempts to fail when the libertas
>> driver is loaded.
>>
>> To fix this, power down the card explicitly in if_sdio_suspend() when we
>> know we're about to lose power and return success. Also set a flag in
>> these
>> cases, and power up the card again in if_sdio_resume().
>>
>> Signed-off-by: Daniel Mack <daniel@zonque.org>
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> Cc: Chris Ball <chris@printf.net>
>> ---
>>   drivers/net/wireless/marvell/libertas/dev.h   |  1 +
>>   .../net/wireless/marvell/libertas/if_sdio.c   | 28 +++++++++++++++----
>>   2 files changed, 23 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/wireless/marvell/libertas/dev.h
>> b/drivers/net/wireless/marvell/libertas/dev.h
>> index dd1ee1f0af48..469134930026 100644
>> --- a/drivers/net/wireless/marvell/libertas/dev.h
>> +++ b/drivers/net/wireless/marvell/libertas/dev.h
>> @@ -104,6 +104,7 @@ struct lbs_private {
>>         u8 fw_ready;
>>         u8 surpriseremoved;
>>         u8 setup_fw_on_resume;
>> +       u8 power_up_on_resume;
>>         int (*hw_host_to_card) (struct lbs_private *priv, u8 type, u8
>> *payload, u16 nb);
>>         void (*reset_card) (struct lbs_private *priv);
>>         int (*power_save) (struct lbs_private *priv);
>> diff --git a/drivers/net/wireless/marvell/libertas/if_sdio.c
>> b/drivers/net/wireless/marvell/libertas/if_sdio.c
>> index 2300e796c6ab..f43807663b1b 100644
>> --- a/drivers/net/wireless/marvell/libertas/if_sdio.c
>> +++ b/drivers/net/wireless/marvell/libertas/if_sdio.c
>> @@ -1290,15 +1290,23 @@ static void if_sdio_remove(struct sdio_func *func)
>>   static int if_sdio_suspend(struct device *dev)
>>   {
>>         struct sdio_func *func = dev_to_sdio_func(dev);
>> -       int ret;
>>         struct if_sdio_card *card = sdio_get_drvdata(func);
>> +       struct lbs_private *priv = card->priv;
>> +       int ret;
>>         mmc_pm_flag_t flags = sdio_get_host_pm_caps(func);
>> +       priv->power_up_on_resume = false;
>>         /* If we're powered off anyway, just let the mmc layer remove the
>>          * card. */
>> -       if (!lbs_iface_active(card->priv))
>> -               return -ENOSYS;
>> +       if (!lbs_iface_active(priv)) {
>> +               if (priv->fw_ready) {
>> +                       priv->power_up_on_resume = true;
>> +                       if_sdio_power_off(card);
>> +               }
>> +
>> +               return 0;
>> +       }
>>         dev_info(dev, "%s: suspend: PM flags = 0x%x\n",
>>                  sdio_func_id(func), flags);
>> @@ -1306,9 +1314,14 @@ static int if_sdio_suspend(struct device *dev)
>>         /* If we aren't being asked to wake on anything, we should bail
>> out
>>          * and let the SD stack power down the card.
>>          */
>> -       if (card->priv->wol_criteria == EHS_REMOVE_WAKEUP) {
>> +       if (priv->wol_criteria == EHS_REMOVE_WAKEUP) {
>>                 dev_info(dev, "Suspend without wake params -- powering
>> down card\n");
>> -               return -ENOSYS;
>> +               if (priv->fw_ready) {
>> +                       priv->power_up_on_resume = true;
>> +                       if_sdio_power_off(card);
>> +               }
>> +
>> +               return 0;
>>         }
>>         if (!(flags & MMC_PM_KEEP_POWER)) {
>> @@ -1321,7 +1334,7 @@ static int if_sdio_suspend(struct device *dev)
>>         if (ret)
>>                 return ret;
>>   -     ret = lbs_suspend(card->priv);
>> +       ret = lbs_suspend(priv);
>>         if (ret)
>>                 return ret;
>>   @@ -1336,6 +1349,9 @@ static int if_sdio_resume(struct device *dev)
>>         dev_info(dev, "%s: resume: we're back\n", sdio_func_id(func));
>>   +     if (card->priv->power_up_on_resume)
>> +               if_sdio_power_on(card);
>> +

To guarantee firmware is loaded, don't you need the below as well?

wait_event(card->pwron_waitq, priv->fw_ready);

>>         ret = lbs_resume(card->priv);
>>         return ret;
>>
>

Kind regards
Uffe

Re: [PATCH] libertas: fix return codes in if_sdio_suspend()

[Ulf Hansson]

On 26 June 2018 at 22:50, Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org> wrote:
> Ah, the subject line of this patch could actually be improved. Let me know
> if you're happy with the contents of this patch, so I can resend.
>
> Thanks,
> Daniel
>
>
>
> On Tuesday, June 26, 2018 10:48 PM, Daniel Mack wrote:
>>
>> Prior to commit 573185cc7e64 ("mmc: core: Invoke sdio func driver's PM
>> callbacks from the sdio bus"), the MMC core used to call into the power
>> management functions of SDIO clients itself and removed the card if the
>> return code was non-zero. IOW, the mmc handled errors gracefully and
>> didn't
>> upchain them to the pm core.
>>
>> Since this change, the mmc core relies on generic power management
>> functions which treat all errors as a reason to cancel the suspend
>> immediately. This causes suspend attempts to fail when the libertas
>> driver is loaded.
>>
>> To fix this, power down the card explicitly in if_sdio_suspend() when we
>> know we're about to lose power and return success. Also set a flag in
>> these
>> cases, and power up the card again in if_sdio_resume().
>>
>> Signed-off-by: Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
>> Cc: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Cc: Chris Ball <chris-OsFVWbfNK3isTnJN9+BGXg@public.gmane.org>
>> ---
>>   drivers/net/wireless/marvell/libertas/dev.h   |  1 +
>>   .../net/wireless/marvell/libertas/if_sdio.c   | 28 +++++++++++++++----
>>   2 files changed, 23 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/wireless/marvell/libertas/dev.h
>> b/drivers/net/wireless/marvell/libertas/dev.h
>> index dd1ee1f0af48..469134930026 100644
>> --- a/drivers/net/wireless/marvell/libertas/dev.h
>> +++ b/drivers/net/wireless/marvell/libertas/dev.h
>> @@ -104,6 +104,7 @@ struct lbs_private {
>>         u8 fw_ready;
>>         u8 surpriseremoved;
>>         u8 setup_fw_on_resume;
>> +       u8 power_up_on_resume;
>>         int (*hw_host_to_card) (struct lbs_private *priv, u8 type, u8
>> *payload, u16 nb);
>>         void (*reset_card) (struct lbs_private *priv);
>>         int (*power_save) (struct lbs_private *priv);
>> diff --git a/drivers/net/wireless/marvell/libertas/if_sdio.c
>> b/drivers/net/wireless/marvell/libertas/if_sdio.c
>> index 2300e796c6ab..f43807663b1b 100644
>> --- a/drivers/net/wireless/marvell/libertas/if_sdio.c
>> +++ b/drivers/net/wireless/marvell/libertas/if_sdio.c
>> @@ -1290,15 +1290,23 @@ static void if_sdio_remove(struct sdio_func *func)
>>   static int if_sdio_suspend(struct device *dev)
>>   {
>>         struct sdio_func *func = dev_to_sdio_func(dev);
>> -       int ret;
>>         struct if_sdio_card *card = sdio_get_drvdata(func);
>> +       struct lbs_private *priv = card->priv;
>> +       int ret;
>>         mmc_pm_flag_t flags = sdio_get_host_pm_caps(func);
>> +       priv->power_up_on_resume = false;
>>         /* If we're powered off anyway, just let the mmc layer remove the
>>          * card. */
>> -       if (!lbs_iface_active(card->priv))
>> -               return -ENOSYS;
>> +       if (!lbs_iface_active(priv)) {
>> +               if (priv->fw_ready) {
>> +                       priv->power_up_on_resume = true;
>> +                       if_sdio_power_off(card);
>> +               }
>> +
>> +               return 0;
>> +       }
>>         dev_info(dev, "%s: suspend: PM flags = 0x%x\n",
>>                  sdio_func_id(func), flags);
>> @@ -1306,9 +1314,14 @@ static int if_sdio_suspend(struct device *dev)
>>         /* If we aren't being asked to wake on anything, we should bail
>> out
>>          * and let the SD stack power down the card.
>>          */
>> -       if (card->priv->wol_criteria == EHS_REMOVE_WAKEUP) {
>> +       if (priv->wol_criteria == EHS_REMOVE_WAKEUP) {
>>                 dev_info(dev, "Suspend without wake params -- powering
>> down card\n");
>> -               return -ENOSYS;
>> +               if (priv->fw_ready) {
>> +                       priv->power_up_on_resume = true;
>> +                       if_sdio_power_off(card);
>> +               }
>> +
>> +               return 0;
>>         }
>>         if (!(flags & MMC_PM_KEEP_POWER)) {
>> @@ -1321,7 +1334,7 @@ static int if_sdio_suspend(struct device *dev)
>>         if (ret)
>>                 return ret;
>>   -     ret = lbs_suspend(card->priv);
>> +       ret = lbs_suspend(priv);
>>         if (ret)
>>                 return ret;
>>   @@ -1336,6 +1349,9 @@ static int if_sdio_resume(struct device *dev)
>>         dev_info(dev, "%s: resume: we're back\n", sdio_func_id(func));
>>   +     if (card->priv->power_up_on_resume)
>> +               if_sdio_power_on(card);
>> +

To guarantee firmware is loaded, don't you need the below as well?

wait_event(card->pwron_waitq, priv->fw_ready);

>>         ret = lbs_resume(card->priv);
>>         return ret;
>>
>

Kind regards
Uffe

Re: [PATCH] libertas: fix return codes in if_sdio_suspend()

[Daniel Mack]

On Wednesday, June 27, 2018 09:54 AM, Ulf Hansson wrote:
> On 26 June 2018 at 22:50, Daniel Mack <daniel@zonque.org> wrote:
>> On Tuesday, June 26, 2018 10:48 PM, Daniel Mack wrote:

>>> @@ -1321,7 +1334,7 @@ static int if_sdio_suspend(struct device *dev)
>>>          if (ret)
>>>                  return ret;
>>>    -     ret = lbs_suspend(card->priv);
>>> +       ret = lbs_suspend(priv);
>>>          if (ret)
>>>                  return ret;
>>>    @@ -1336,6 +1349,9 @@ static int if_sdio_resume(struct device *dev)
>>>          dev_info(dev, "%s: resume: we're back\n", sdio_func_id(func));
>>>    +     if (card->priv->power_up_on_resume)
>>> +               if_sdio_power_on(card);
>>> +
> 
> To guarantee firmware is loaded, don't you need the below as well?
> 
> wait_event(card->pwron_waitq, priv->fw_ready);

Hmm, yes. I should probably just be calling into if_sdio_power_restore().

I'll test that and resend the patch.


Thanks,
Daniel

Re: [PATCH] libertas: fix return codes in if_sdio_suspend()

[Daniel Mack]

On Wednesday, June 27, 2018 09:54 AM, Ulf Hansson wrote:
> On 26 June 2018 at 22:50, Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org> wrote:
>> On Tuesday, June 26, 2018 10:48 PM, Daniel Mack wrote:

>>> @@ -1321,7 +1334,7 @@ static int if_sdio_suspend(struct device *dev)
>>>          if (ret)
>>>                  return ret;
>>>    -     ret = lbs_suspend(card->priv);
>>> +       ret = lbs_suspend(priv);
>>>          if (ret)
>>>                  return ret;
>>>    @@ -1336,6 +1349,9 @@ static int if_sdio_resume(struct device *dev)
>>>          dev_info(dev, "%s: resume: we're back\n", sdio_func_id(func));
>>>    +     if (card->priv->power_up_on_resume)
>>> +               if_sdio_power_on(card);
>>> +
> 
> To guarantee firmware is loaded, don't you need the below as well?
> 
> wait_event(card->pwron_waitq, priv->fw_ready);

Hmm, yes. I should probably just be calling into if_sdio_power_restore().

I'll test that and resend the patch.


Thanks,
Daniel

Re: [PATCH] libertas: fix return codes in if_sdio_suspend()

[Ulf Hansson]

On 27 June 2018 at 10:31, Daniel Mack <daniel@zonque.org> wrote:
> On Wednesday, June 27, 2018 09:54 AM, Ulf Hansson wrote:
>>
>> On 26 June 2018 at 22:50, Daniel Mack <daniel@zonque.org> wrote:
>>>
>>> On Tuesday, June 26, 2018 10:48 PM, Daniel Mack wrote:
>
>
>>>> @@ -1321,7 +1334,7 @@ static int if_sdio_suspend(struct device *dev)
>>>>          if (ret)
>>>>                  return ret;
>>>>    -     ret = lbs_suspend(card->priv);
>>>> +       ret = lbs_suspend(priv);
>>>>          if (ret)
>>>>                  return ret;
>>>>    @@ -1336,6 +1349,9 @@ static int if_sdio_resume(struct device *dev)
>>>>          dev_info(dev, "%s: resume: we're back\n", sdio_func_id(func));
>>>>    +     if (card->priv->power_up_on_resume)
>>>> +               if_sdio_power_on(card);
>>>> +
>>
>>
>> To guarantee firmware is loaded, don't you need the below as well?
>>
>> wait_event(card->pwron_waitq, priv->fw_ready);
>
>
> Hmm, yes. I should probably just be calling into if_sdio_power_restore().

You don't want to mess up the runtime PM counter though.

>
> I'll test that and resend the patch.

Great!

Kind regards
Uffe

Re: [PATCH] libertas: fix return codes in if_sdio_suspend()

[Ulf Hansson]

On 27 June 2018 at 10:31, Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org> wrote:
> On Wednesday, June 27, 2018 09:54 AM, Ulf Hansson wrote:
>>
>> On 26 June 2018 at 22:50, Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org> wrote:
>>>
>>> On Tuesday, June 26, 2018 10:48 PM, Daniel Mack wrote:
>
>
>>>> @@ -1321,7 +1334,7 @@ static int if_sdio_suspend(struct device *dev)
>>>>          if (ret)
>>>>                  return ret;
>>>>    -     ret = lbs_suspend(card->priv);
>>>> +       ret = lbs_suspend(priv);
>>>>          if (ret)
>>>>                  return ret;
>>>>    @@ -1336,6 +1349,9 @@ static int if_sdio_resume(struct device *dev)
>>>>          dev_info(dev, "%s: resume: we're back\n", sdio_func_id(func));
>>>>    +     if (card->priv->power_up_on_resume)
>>>> +               if_sdio_power_on(card);
>>>> +
>>
>>
>> To guarantee firmware is loaded, don't you need the below as well?
>>
>> wait_event(card->pwron_waitq, priv->fw_ready);
>
>
> Hmm, yes. I should probably just be calling into if_sdio_power_restore().

You don't want to mess up the runtime PM counter though.

>
> I'll test that and resend the patch.

Great!

Kind regards
Uffe