All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mmc: core: Do not pre-claim host in suspend
@ 2012-04-19  9:55 Ulf Hansson
  2012-04-19 10:02 ` Vitaly Wool
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Ulf Hansson @ 2012-04-19  9:55 UTC (permalink / raw)
  To: linux-mmc, Chris Ball
  Cc: Daniel Drake, Per Forlin, Ulf Hansson, Johan Rudholm, Lee Jones

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] mmc: core: Do not pre-claim host in suspend
  2012-04-19  9:55 [PATCH] mmc: core: Do not pre-claim host in suspend Ulf Hansson
@ 2012-04-19 10:02 ` Vitaly Wool
  2012-04-19 15:48   ` Daniel Drake
  2012-04-20 13:41 ` Chris Ball
  2012-04-20 17:17 ` Sujit Reddy Thumma
  2 siblings, 1 reply; 7+ messages in thread
From: Vitaly Wool @ 2012-04-19 10:02 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: linux-mmc, Chris Ball, Daniel Drake, Per Forlin, Johan Rudholm,
	Lee Jones

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] mmc: core: Do not pre-claim host in suspend
  2012-04-19 10:02 ` Vitaly Wool
@ 2012-04-19 15:48   ` Daniel Drake
  2012-04-20  8:55     ` Ulf Hansson
  0 siblings, 1 reply; 7+ messages in thread
From: Daniel Drake @ 2012-04-19 15:48 UTC (permalink / raw)
  To: Vitaly Wool
  Cc: Ulf Hansson, linux-mmc, Chris Ball, Per Forlin, Johan Rudholm, Lee Jones

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] mmc: core: Do not pre-claim host in suspend
  2012-04-19 15:48   ` Daniel Drake
@ 2012-04-20  8:55     ` Ulf Hansson
  2012-04-20 13:39       ` Daniel Drake
  0 siblings, 1 reply; 7+ messages in thread
From: Ulf Hansson @ 2012-04-20  8:55 UTC (permalink / raw)
  To: Daniel Drake
  Cc: Vitaly Wool, linux-mmc, Chris Ball, Per FORLIN, Johan RUDHOLM, Lee Jones

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] mmc: core: Do not pre-claim host in suspend
  2012-04-20  8:55     ` Ulf Hansson
@ 2012-04-20 13:39       ` Daniel Drake
  0 siblings, 0 replies; 7+ messages in thread
From: Daniel Drake @ 2012-04-20 13:39 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Vitaly Wool, linux-mmc, Chris Ball, Per FORLIN, Johan RUDHOLM, Lee Jones

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] mmc: core: Do not pre-claim host in suspend
  2012-04-19  9:55 [PATCH] mmc: core: Do not pre-claim host in suspend Ulf Hansson
  2012-04-19 10:02 ` Vitaly Wool
@ 2012-04-20 13:41 ` Chris Ball
  2012-04-20 17:17 ` Sujit Reddy Thumma
  2 siblings, 0 replies; 7+ messages in thread
From: Chris Ball @ 2012-04-20 13:41 UTC (permalink / raw)
  To: Ulf Hansson; +Cc: linux-mmc, Daniel Drake, Per Forlin, Johan Rudholm, Lee Jones

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] mmc: core: Do not pre-claim host in suspend
  2012-04-19  9:55 [PATCH] mmc: core: Do not pre-claim host in suspend Ulf Hansson
  2012-04-19 10:02 ` Vitaly Wool
  2012-04-20 13:41 ` Chris Ball
@ 2012-04-20 17:17 ` Sujit Reddy Thumma
  2 siblings, 0 replies; 7+ messages in thread
From: Sujit Reddy Thumma @ 2012-04-20 17:17 UTC (permalink / raw)
  Cc: linux-mmc, Chris Ball, Daniel Drake, Per Forlin, Ulf Hansson,
	Johan Rudholm, Lee Jones

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.


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2012-04-20 17:17 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-19  9:55 [PATCH] mmc: core: Do not pre-claim host in suspend Ulf Hansson
2012-04-19 10:02 ` Vitaly Wool
2012-04-19 15:48   ` Daniel Drake
2012-04-20  8:55     ` Ulf Hansson
2012-04-20 13:39       ` Daniel Drake
2012-04-20 13:41 ` Chris Ball
2012-04-20 17:17 ` Sujit Reddy Thumma

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.