[PATCH] mmc: core: Do not pre-claim host in suspend

[PATCH] mmc: core: Do not pre-claim host in suspend

[Ulf Hansson]

Since SDIO drivers may want to do some SDIO operations
in their suspend callback functions, we must not keep
the host claimed when calling them.

Daniel Drake reported that libertas_sdio encountered
a deadlock in it's suspend function.

Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
---
 drivers/mmc/core/core.c |   55 +++++++++++++++++-----------------------------
 1 files changed, 20 insertions(+), 35 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index e541efb..ba821fe 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2238,6 +2238,7 @@ int mmc_cache_ctrl(struct mmc_host *host, u8 enable)
 			mmc_card_is_removable(host))
 		return err;
 
+	mmc_claim_host(host);
 	if (card && mmc_card_mmc(card) &&
 			(card->ext_csd.cache_size > 0)) {
 		enable = !!enable;
@@ -2255,6 +2256,7 @@ int mmc_cache_ctrl(struct mmc_host *host, u8 enable)
 				card->ext_csd.cache_ctrl = enable;
 		}
 	}
+	mmc_release_host(host);
 
 	return err;
 }
@@ -2272,49 +2274,32 @@ int mmc_suspend_host(struct mmc_host *host)
 
 	cancel_delayed_work(&host->detect);
 	mmc_flush_scheduled_work();
-	if (mmc_try_claim_host(host)) {
-		err = mmc_cache_ctrl(host, 0);
-		mmc_release_host(host);
-	} else {
-		err = -EBUSY;
-	}
 
+	err = mmc_cache_ctrl(host, 0);
 	if (err)
 		goto out;
 
 	mmc_bus_get(host);
 	if (host->bus_ops && !host->bus_dead) {
 
-		/*
-		 * A long response time is not acceptable for device drivers
-		 * when doing suspend. Prevent mmc_claim_host in the suspend
-		 * sequence, to potentially wait "forever" by trying to
-		 * pre-claim the host.
-		 */
-		if (mmc_try_claim_host(host)) {
-			if (host->bus_ops->suspend) {
-				err = host->bus_ops->suspend(host);
-			}
-			mmc_release_host(host);
+		if (host->bus_ops->suspend)
+			err = host->bus_ops->suspend(host);
 
-			if (err == -ENOSYS || !host->bus_ops->resume) {
-				/*
-				 * We simply "remove" the card in this case.
-				 * It will be redetected on resume.  (Calling
-				 * bus_ops->remove() with a claimed host can
-				 * deadlock.)
-				 */
-				if (host->bus_ops->remove)
-					host->bus_ops->remove(host);
-				mmc_claim_host(host);
-				mmc_detach_bus(host);
-				mmc_power_off(host);
-				mmc_release_host(host);
-				host->pm_flags = 0;
-				err = 0;
-			}
-		} else {
-			err = -EBUSY;
+		if (err == -ENOSYS || !host->bus_ops->resume) {
+			/*
+			 * We simply "remove" the card in this case.
+			 * It will be redetected on resume.  (Calling
+			 * bus_ops->remove() with a claimed host can
+			 * deadlock.)
+			 */
+			if (host->bus_ops->remove)
+				host->bus_ops->remove(host);
+			mmc_claim_host(host);
+			mmc_detach_bus(host);
+			mmc_power_off(host);
+			mmc_release_host(host);
+			host->pm_flags = 0;
+			err = 0;
 		}
 	}
 	mmc_bus_put(host);
-- 
1.7.9

Re: [PATCH] mmc: core: Do not pre-claim host in suspend

[Vitaly Wool]

Hi Ulf,

On Thu, Apr 19, 2012 at 11:55 AM, Ulf Hansson
<ulf.hansson@stericsson.com> wrote:
> Since SDIO drivers may want to do some SDIO operations
> in their suspend callback functions, we must not keep
> the host claimed when calling them.
>
> Daniel Drake reported that libertas_sdio encountered
> a deadlock in it's suspend function.

I think it looks like a hack. Can you provide a better description of
where this deadlock actually happens?

If libertas_sdio claims host as a part of its suspend operation from a
context different from the suspend context, isn't it libertas_sdio
that has to be fixed?

Vitaly

Re: [PATCH] mmc: core: Do not pre-claim host in suspend

[Daniel Drake]

On Thu, Apr 19, 2012 at 4:02 AM, Vitaly Wool <vitalywool@gmail.com> wrote:
> I think it looks like a hack. Can you provide a better description of
> where this deadlock actually happens?
>
> If libertas_sdio claims host as a part of its suspend operation from a
> context different from the suspend context, isn't it libertas_sdio
> that has to be fixed?

libertas_sdio performs work in a workqueue during the suspend routine.
It doesn't take long, but it is essential.

Technically it would be possible to do the same work from the suspend
thread without changing context. However, implementation-wise this is
quite difficult. We cannot simply blast commands off to the card
directly, we have to obey various rules and keep things in sync. This
is done with an abstraction layer in the driver which is also shared
with equivalent devices that connect over USB, SPI, etc. Sometimes we
send commands from atomic context, or asynchronously, so thats why we
do things in a workqueue.

Breaking this abstraction for the suspend corner-case would be a pain,
and I don't agree with the strange requirement that SDIO drivers can't
communicate from other contexts in the suspend routine. My thoughts
are here: http://article.gmane.org/gmane.linux.kernel.mmc/13899

Ulf, you have understood the problem correctly and your patch solves
the issue. Thanks for the fast response.

Daniel

Re: [PATCH] mmc: core: Do not pre-claim host in suspend

[Ulf Hansson]

On 04/19/2012 05:48 PM, Daniel Drake wrote:
> On Thu, Apr 19, 2012 at 4:02 AM, Vitaly Wool<vitalywool@gmail.com>  wrote:
>> I think it looks like a hack. Can you provide a better description of
>> where this deadlock actually happens?
>>
>> If libertas_sdio claims host as a part of its suspend operation from a
>> context different from the suspend context, isn't it libertas_sdio
>> that has to be fixed?
>
> libertas_sdio performs work in a workqueue during the suspend routine.
> It doesn't take long, but it is essential.
>
> Technically it would be possible to do the same work from the suspend
> thread without changing context. However, implementation-wise this is
> quite difficult. We cannot simply blast commands off to the card
> directly, we have to obey various rules and keep things in sync. This
> is done with an abstraction layer in the driver which is also shared
> with equivalent devices that connect over USB, SPI, etc. Sometimes we
> send commands from atomic context, or asynchronously, so thats why we
> do things in a workqueue.
>
> Breaking this abstraction for the suspend corner-case would be a pain,
> and I don't agree with the strange requirement that SDIO drivers can't
> communicate from other contexts in the suspend routine. My thoughts
> are here: http://article.gmane.org/gmane.linux.kernel.mmc/13899
>
> Ulf, you have understood the problem correctly and your patch solves
> the issue. Thanks for the fast response.
>
> Daniel

Thanks Daniel for testing, could we add your "Tested-by" to this patch then?

Myself, would like to do some more testing for MMC/SD, before adding 
mine Tested-by tag... get back to this soon..

Kind regards
Uffe

Re: [PATCH] mmc: core: Do not pre-claim host in suspend

[Daniel Drake]

On Fri, Apr 20, 2012 at 2:55 AM, Ulf Hansson <ulf.hansson@stericsson.com> wrote:
> Thanks Daniel for testing, could we add your "Tested-by" to this patch then?

Tested-by: Daniel Drake <dsd@laptop.org>

Thanks

Re: [PATCH] mmc: core: Do not pre-claim host in suspend

[Chris Ball]

Hi Ulf,

On Thu, Apr 19 2012, Ulf Hansson wrote:
> Since SDIO drivers may want to do some SDIO operations
> in their suspend callback functions, we must not keep
> the host claimed when calling them.
>
> Daniel Drake reported that libertas_sdio encountered
> a deadlock in it's suspend function.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>

Thanks for taking a look at this, pushed to mmc-next for 3.4
with Dan's Tested-by.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: core: Do not pre-claim host in suspend

[Sujit Reddy Thumma]

Hi Ulf,

> Since SDIO drivers may want to do some SDIO operations
> in their suspend callback functions, we must not keep
> the host claimed when calling them.
>
> Daniel Drake reported that libertas_sdio encountered
> a deadlock in it's suspend function.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
> ---

Sorry for jumping in late, can we have waiver just for SDIO cards? Will
something like below acceptable?

@@ -2421,12 +2421,24 @@ int mmc_suspend_host(struct mmc_host *host)
                 * when doing suspend. Prevent mmc_claim_host in the suspend
                 * sequence, to potentially wait "forever" by trying to
                 * pre-claim the host.
+                *
+                * Skip try claim host for SDIO cards, doing so fixes
deadlock
+                * conditions. The function driver suspend may again call
into
+                * SDIO driver within a different context for enabling power
+                * save mode in the card and hence wait in mmc_claim_host
+                * causing deadlock.
                 */
-               if (mmc_try_claim_host(host)) {
+
+               if (!(host->card && mmc_card_sdio(host->card)))
+                       if (!mmc_try_claim_host(host))
+                               err = -EBUSY;
+
+               if (!err) {
                        if (host->bus_ops->suspend) {
                                err = host->bus_ops->suspend(host);
                        }
-                       mmc_do_release_host(host);
+                       if (!(host->card && mmc_card_sdio(host->card)))
+                               mmc_do_release_host(host);

                        if (err == -ENOSYS || !host->bus_ops->resume) {
                                /*
@@ -2444,8 +2456,6 @@ int mmc_suspend_host(struct mmc_host *host)
                                host->pm_flags = 0;
                                err = 0;
                        }
-               } else {
-                       err = -EBUSY;
                }
        }

>  drivers/mmc/core/core.c |   55
> +++++++++++++++++-----------------------------
>  1 files changed, 20 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index e541efb..ba821fe 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2238,6 +2238,7 @@ int mmc_cache_ctrl(struct mmc_host *host, u8 enable)
>  			mmc_card_is_removable(host))
>  		return err;
>
> +	mmc_claim_host(host);
>  	if (card && mmc_card_mmc(card) &&
>  			(card->ext_csd.cache_size > 0)) {
>  		enable = !!enable;
> @@ -2255,6 +2256,7 @@ int mmc_cache_ctrl(struct mmc_host *host, u8 enable)
>  				card->ext_csd.cache_ctrl = enable;
>  		}
>  	}
> +	mmc_release_host(host);
>
>  	return err;
>  }
> @@ -2272,49 +2274,32 @@ int mmc_suspend_host(struct mmc_host *host)
>
>  	cancel_delayed_work(&host->detect);
>  	mmc_flush_scheduled_work();
> -	if (mmc_try_claim_host(host)) {
> -		err = mmc_cache_ctrl(host, 0);
> -		mmc_release_host(host);
> -	} else {
> -		err = -EBUSY;
> -	}
>
> +	err = mmc_cache_ctrl(host, 0);
>  	if (err)
>  		goto out;
>
>  	mmc_bus_get(host);
>  	if (host->bus_ops && !host->bus_dead) {
>
> -		/*
> -		 * A long response time is not acceptable for device drivers
> -		 * when doing suspend. Prevent mmc_claim_host in the suspend
> -		 * sequence, to potentially wait "forever" by trying to
> -		 * pre-claim the host.
> -		 */
> -		if (mmc_try_claim_host(host)) {
> -			if (host->bus_ops->suspend) {
> -				err = host->bus_ops->suspend(host);
> -			}
> -			mmc_release_host(host);
> +		if (host->bus_ops->suspend)
> +			err = host->bus_ops->suspend(host);
>
> -			if (err == -ENOSYS || !host->bus_ops->resume) {
> -				/*
> -				 * We simply "remove" the card in this case.
> -				 * It will be redetected on resume.  (Calling
> -				 * bus_ops->remove() with a claimed host can
> -				 * deadlock.)
> -				 */
> -				if (host->bus_ops->remove)
> -					host->bus_ops->remove(host);
> -				mmc_claim_host(host);
> -				mmc_detach_bus(host);
> -				mmc_power_off(host);
> -				mmc_release_host(host);
> -				host->pm_flags = 0;
> -				err = 0;
> -			}
> -		} else {
> -			err = -EBUSY;
> +		if (err == -ENOSYS || !host->bus_ops->resume) {
> +			/*
> +			 * We simply "remove" the card in this case.
> +			 * It will be redetected on resume.  (Calling
> +			 * bus_ops->remove() with a claimed host can
> +			 * deadlock.)
> +			 */
> +			if (host->bus_ops->remove)
> +				host->bus_ops->remove(host);
> +			mmc_claim_host(host);
> +			mmc_detach_bus(host);
> +			mmc_power_off(host);
> +			mmc_release_host(host);
> +			host->pm_flags = 0;
> +			err = 0;
>  		}
>  	}
>  	mmc_bus_put(host);
> --
> 1.7.9
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
Thanks & Regards,
Sujit Reddy Thumma

Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum.