[PATCH 0/3] SDIO suspend/resume optimizations

[PATCH 0/3] SDIO suspend/resume optimizations

[Ohad Ben-Cohen]

Remove redundant power and card reinitialization cycles involved with
SDIO suspend/resume transitions, especially when interacting with SDIO
runtime PM.

As a result, suspend/resume latencies are reduced and battery life is
improved.

I have not done any latencies or power measurements, but I am pretty
certain the numbers will not be very big.

Having said that, common mobile platform (such as Android and OLPC) are
aggressively utilizing full system suspend in order to save as much energy
as possible.

In those systems, suspend/resume transitions happen very frequently,
and even small enhancements, as presented in this patchset, may eventually
deem substantial.

Please note that these changes should be completely safe on all environment.

Patches have passed overnight stress with the script mentioned at:
http://www.mail-archive.com/linux-mmc@vger.kernel.org/msg03923.html

PS - I'm going on a plane and will not have Internet access so apologies
for belated replies.

Ohad Ben-Cohen (3):
  mmc: skip detection of nonremovable cards on rescan
  mmc: sdio: don't reinitialize nonremovable powered-resumed cards
  mmc: sdio: don't power up cards on system suspend

 drivers/mmc/core/core.c     |   21 +++++++++++++++++++--
 drivers/mmc/core/sdio.c     |   16 ++++++++++++++--
 drivers/mmc/core/sdio_bus.c |   32 --------------------------------
 include/linux/mmc/host.h    |    5 +++++
 4 files changed, 38 insertions(+), 36 deletions(-)

[PATCH 1/3] mmc: skip detection of nonremovable cards on rescan

[Ohad Ben-Cohen]

mmc_rescan checks whether registered cards are still present before
skipping them, by calling the bus-specific ->detect() handler.

With buses that support runtime PM, the card may be powered off at
this point, so they need to be powered on and fully reinitialized before
->detect() executes.

This whole process is redundant with nonremovable cards; in those cases,
we can safely skip calling ->detect() and implicitly assume its success.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 drivers/mmc/core/core.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 6286898..e8332d7 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1506,8 +1506,12 @@ void mmc_rescan(struct work_struct *work)
 
 	mmc_bus_get(host);
 
-	/* if there is a card registered, check whether it is still present */
-	if ((host->bus_ops != NULL) && host->bus_ops->detect && !host->bus_dead)
+	/*
+	 * if there is a _removable_ card registered, check whether it is
+	 * still present
+	 */
+	if ((host->bus_ops != NULL) && host->bus_ops->detect && !host->bus_dead
+					&& mmc_card_is_removable(host))
 		host->bus_ops->detect(host);
 
 	mmc_bus_put(host);
-- 
1.7.0.4

[PATCH 2/3] mmc: sdio: don't reinitialize nonremovable powered-resumed cards

[Ohad Ben-Cohen]

Upon system resume, SDIO core must reinitialize cards that were
powered off during suspend.

If the card had its power kept during suspend (and thus it
is 'powered-resumed'), SDIO core performs only a limited reinitializing,
mainly needed to make sure that the card wasn't removed/replaced.

If a __nonremovable__ card is powered-resumed, we can safely skip the
reinitializing phase.

Note: 9b966aa removed the bus width reconfiguration since
mmc_sdio_init_card already does it. It is brought back now in case
mmc_sdio_init_card is skipped.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---

I have not switched all existing code to mmc_card_is_powered_resumed(),
I can do that as a subsequent clean up patch.

 drivers/mmc/core/sdio.c  |   16 ++++++++++++++--
 include/linux/mmc/host.h |    5 +++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index efef5f9..b424fbe 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -627,15 +627,27 @@ static int mmc_sdio_suspend(struct mmc_host *host)
 
 static int mmc_sdio_resume(struct mmc_host *host)
 {
-	int i, err;
+	int i, err = 0;
 
 	BUG_ON(!host);
 	BUG_ON(!host->card);
 
 	/* Basic card reinitialization. */
 	mmc_claim_host(host);
-	err = mmc_sdio_init_card(host, host->ocr, host->card,
+
+	/* No need to reinitialize powered-resumed nonremovable cards */
+	if (mmc_card_is_removable(host) || !mmc_card_is_powered_resumed(host))
+		err = mmc_sdio_init_card(host, host->ocr, host->card,
 				 (host->pm_flags & MMC_PM_KEEP_POWER));
+	else if (mmc_card_is_powered_resumed(host)) {
+		/* We may have switched to 1-bit mode during suspend */
+		err = sdio_enable_4bit_bus(host->card);
+		if (err > 0) {
+			mmc_set_bus_width(host, MMC_BUS_WIDTH_4);
+			err = 0;
+		}
+	}
+
 	if (!err && host->sdio_irqs)
 		mmc_signal_sdio_irq(host);
 	mmc_release_host(host);
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 381c77f..30b6fb9 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -317,5 +317,10 @@ static inline int mmc_card_is_removable(struct mmc_host *host)
 	return !(host->caps & MMC_CAP_NONREMOVABLE) && mmc_assume_removable;
 }
 
+static inline int mmc_card_is_powered_resumed(struct mmc_host *host)
+{
+	return host->pm_flags & MMC_PM_KEEP_POWER;
+}
+
 #endif
 
-- 
1.7.0.4

[PATCH 3/3] mmc: sdio: don't power up cards on system suspend

[Ohad Ben-Cohen]

Initial SDIO runtime PM implementation took a conservative approach
of powering up cards (and fully reinitializing them) on system suspend,
just before the suspend handlers of the relevant drivers were executed.

To avoid redundant power and reinitialization cycles, this patch removes
this behavior: if a card is already powered off when system suspend kicks
in, it is left at that state.

If a card is active when a system sleep starts, everything is
straightforward and works exactly like before. But if the card was
already suspended before the sleep began, then when MMC core power it up
back on resume, its run-time PM status has to be updated to reflect the
actual post-system sleep status.

The technique to do that is borrowed from the I2C runtime PM
implementation (for more info also see Documentation/power/runtime_pm.txt).

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 drivers/mmc/core/core.c     |   13 +++++++++++++
 drivers/mmc/core/sdio_bus.c |   32 --------------------------------
 2 files changed, 13 insertions(+), 32 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index e8332d7..2e29faf 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -22,6 +22,7 @@
 #include <linux/scatterlist.h>
 #include <linux/log2.h>
 #include <linux/regulator/consumer.h>
+#include <linux/pm_runtime.h>
 
 #include <linux/mmc/card.h>
 #include <linux/mmc/host.h>
@@ -1785,6 +1786,18 @@ int mmc_resume_host(struct mmc_host *host)
 		if (!(host->pm_flags & MMC_PM_KEEP_POWER)) {
 			mmc_power_up(host);
 			mmc_select_voltage(host, host->ocr);
+			/*
+			 * Tell runtime PM core we just powered up the card,
+			 * since it still believes the card is powered off.
+			 * Note that currently runtime PM is only enabled
+			 * for SDIO cards that are MMC_CAP_POWER_OFF_CARD
+			 */
+			if (mmc_card_sdio(host->card) &&
+				(host->caps & MMC_CAP_POWER_OFF_CARD)) {
+				pm_runtime_disable(&host->card->dev);
+				pm_runtime_set_active(&host->card->dev);
+				pm_runtime_enable(&host->card->dev);
+			}
 		}
 		BUG_ON(!host->bus_ops->resume);
 		err = host->bus_ops->resume(host);
diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
index 203da44..d29b9c3 100644
--- a/drivers/mmc/core/sdio_bus.c
+++ b/drivers/mmc/core/sdio_bus.c
@@ -197,44 +197,12 @@ out:
 
 #ifdef CONFIG_PM_RUNTIME
 
-static int sdio_bus_pm_prepare(struct device *dev)
-{
-	struct sdio_func *func = dev_to_sdio_func(dev);
-
-	/*
-	 * Resume an SDIO device which was suspended at run time at this
-	 * point, in order to allow standard SDIO suspend/resume paths
-	 * to keep working as usual.
-	 *
-	 * Ultimately, the SDIO driver itself will decide (in its
-	 * suspend handler, or lack thereof) whether the card should be
-	 * removed or kept, and if kept, at what power state.
-	 *
-	 * At this point, PM core have increased our use count, so it's
-	 * safe to directly resume the device. After system is resumed
-	 * again, PM core will drop back its runtime PM use count, and if
-	 * needed device will be suspended again.
-	 *
-	 * The end result is guaranteed to be a power state that is
-	 * coherent with the device's runtime PM use count.
-	 *
-	 * The return value of pm_runtime_resume is deliberately unchecked
-	 * since there is little point in failing system suspend if a
-	 * device can't be resumed.
-	 */
-	if (func->card->host->caps & MMC_CAP_POWER_OFF_CARD)
-		pm_runtime_resume(dev);
-
-	return 0;
-}
-
 static const struct dev_pm_ops sdio_bus_pm_ops = {
 	SET_RUNTIME_PM_OPS(
 		pm_generic_runtime_suspend,
 		pm_generic_runtime_resume,
 		pm_generic_runtime_idle
 	)
-	.prepare = sdio_bus_pm_prepare,
 };
 
 #define SDIO_PM_OPS_PTR	(&sdio_bus_pm_ops)
-- 
1.7.0.4

Re: [PATCH 2/3] mmc: sdio: don't reinitialize nonremovable powered-resumed cards

[Michał Mirosław]

2010/11/28 Ohad Ben-Cohen <ohad@wizery.com>:
> Upon system resume, SDIO core must reinitialize cards that were
> powered off during suspend.
>
> If the card had its power kept during suspend (and thus it
> is 'powered-resumed'), SDIO core performs only a limited reinitializing,
> mainly needed to make sure that the card wasn't removed/replaced.
>
> If a __nonremovable__ card is powered-resumed, we can safely skip the
> reinitializing phase.
>
> Note: 9b966aa removed the bus width reconfiguration since
> mmc_sdio_init_card already does it. It is brought back now in case
> mmc_sdio_init_card is skipped.
>
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
> ---
>
> I have not switched all existing code to mmc_card_is_powered_resumed(),
> I can do that as a subsequent clean up patch.
>
>  drivers/mmc/core/sdio.c  |   16 ++++++++++++++--
>  include/linux/mmc/host.h |    5 +++++
>  2 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index efef5f9..b424fbe 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -627,15 +627,27 @@ static int mmc_sdio_suspend(struct mmc_host *host)
>
>  static int mmc_sdio_resume(struct mmc_host *host)
>  {
> -       int i, err;
> +       int i, err = 0;
>
>        BUG_ON(!host);
>        BUG_ON(!host->card);
>
>        /* Basic card reinitialization. */
>        mmc_claim_host(host);
> -       err = mmc_sdio_init_card(host, host->ocr, host->card,
> +
> +       /* No need to reinitialize powered-resumed nonremovable cards */
> +       if (mmc_card_is_removable(host) || !mmc_card_is_powered_resumed(host))
> +               err = mmc_sdio_init_card(host, host->ocr, host->card,
>                                 (host->pm_flags & MMC_PM_KEEP_POWER));

You can use mmc_card_is_powered_resumed() here, too.

> +       else if (mmc_card_is_powered_resumed(host)) {
> +               /* We may have switched to 1-bit mode during suspend */
> +               err = sdio_enable_4bit_bus(host->card);
> +               if (err > 0) {
> +                       mmc_set_bus_width(host, MMC_BUS_WIDTH_4);
> +                       err = 0;
> +               }
> +       }
> +
>        if (!err && host->sdio_irqs)
>                mmc_signal_sdio_irq(host);
>        mmc_release_host(host);
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 381c77f..30b6fb9 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -317,5 +317,10 @@ static inline int mmc_card_is_removable(struct mmc_host *host)
>        return !(host->caps & MMC_CAP_NONREMOVABLE) && mmc_assume_removable;
>  }
>
> +static inline int mmc_card_is_powered_resumed(struct mmc_host *host)
> +{
> +       return host->pm_flags & MMC_PM_KEEP_POWER;
> +}
> +
>  #endif
>

Best Regards,
Michał Mirosław

Re: [PATCH 2/3] mmc: sdio: don't reinitialize nonremovable powered-resumed cards

[Ohad Ben-Cohen]

2010/11/28 Michał Mirosław <mirqus@gmail.com>:

<snip>

>> +
>> +       /* No need to reinitialize powered-resumed nonremovable cards */
>> +       if (mmc_card_is_removable(host) || !mmc_card_is_powered_resumed(host))
>> +               err = mmc_sdio_init_card(host, host->ocr, host->card,
>>                                 (host->pm_flags & MMC_PM_KEEP_POWER));
>
> You can use mmc_card_is_powered_resumed() here, too.

Yeah. there are additional incarnations, too. that's why I noted that:

>> I have not switched all existing code to mmc_card_is_powered_resumed(),
>> I can do that as a subsequent clean up patch.

Re: [PATCH 2/3] mmc: sdio: don't reinitialize nonremovable powered-resumed cards

[Ohad Ben-Cohen]

2010/11/28 Ohad Ben-Cohen <ohad@wizery.com>:
>>> I have not switched all existing code to mmc_card_is_powered_resumed(),
>>> I can do that as a subsequent clean up patch.

(i just prefer not to mix functional changes with cleanups in a single patch)


>

Re: [PATCH 1/3] mmc: skip detection of nonremovable cards on rescan

[Chris Ball]

Hi Ohad,

On Sun, Nov 28, 2010 at 07:21:28AM +0200, Ohad Ben-Cohen wrote:
> mmc_rescan checks whether registered cards are still present before
> skipping them, by calling the bus-specific ->detect() handler.
> 
> With buses that support runtime PM, the card may be powered off at
> this point, so they need to be powered on and fully reinitialized before
> ->detect() executes.
> 
> This whole process is redundant with nonremovable cards; in those cases,
> we can safely skip calling ->detect() and implicitly assume its success.
> 
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
> ---
>  drivers/mmc/core/core.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 6286898..e8332d7 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1506,8 +1506,12 @@ void mmc_rescan(struct work_struct *work)
>  
>  	mmc_bus_get(host);
>  
> -	/* if there is a card registered, check whether it is still present */
> -	if ((host->bus_ops != NULL) && host->bus_ops->detect && !host->bus_dead)
> +	/*
> +	 * if there is a _removable_ card registered, check whether it is
> +	 * still present
> +	 */
> +	if ((host->bus_ops != NULL) && host->bus_ops->detect && !host->bus_dead
> +					&& mmc_card_is_removable(host))
>  		host->bus_ops->detect(host);
>  
>  	mmc_bus_put(host);

Thanks, pushed to mmc-next with trivial cleanup as below:

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index e7c0c78..d9c98ee 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1506,8 +1506,12 @@ void mmc_rescan(struct work_struct *work)
 
 	mmc_bus_get(host);
 
-	/* if there is a card registered, check whether it is still present */
-	if ((host->bus_ops != NULL) && host->bus_ops->detect && !host->bus_dead)
+	/*
+	 * if there is a _removable_ card registered, check whether it is
+	 * still present
+	 */
+	if (host->bus_ops && host->bus_ops->detect && !host->bus_dead
+	    && mmc_card_is_removable(host))
 		host->bus_ops->detect(host);
 
 	mmc_bus_put(host);
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 2/3] mmc: sdio: don't reinitialize nonremovable powered-resumed cards

[Chris Ball]

Hi Ohad,

On Sun, Nov 28, 2010 at 07:21:29AM +0200, Ohad Ben-Cohen wrote:
> Upon system resume, SDIO core must reinitialize cards that were
> powered off during suspend.
> 
> If the card had its power kept during suspend (and thus it
> is 'powered-resumed'), SDIO core performs only a limited reinitializing,
> mainly needed to make sure that the card wasn't removed/replaced.
> 
> If a __nonremovable__ card is powered-resumed, we can safely skip the
> reinitializing phase.
> 
> Note: 9b966aa removed the bus width reconfiguration since
> mmc_sdio_init_card already does it. It is brought back now in case
> mmc_sdio_init_card is skipped.
> 
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>

Thanks, pushed to mmc-next.

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

Re: [PATCH 3/3] mmc: sdio: don't power up cards on system suspend

[Chris Ball]

Hi Ohad,

On Sun, Nov 28, 2010 at 07:21:30AM +0200, Ohad Ben-Cohen wrote:
> Initial SDIO runtime PM implementation took a conservative approach
> of powering up cards (and fully reinitializing them) on system suspend,
> just before the suspend handlers of the relevant drivers were executed.
> 
> To avoid redundant power and reinitialization cycles, this patch removes
> this behavior: if a card is already powered off when system suspend kicks
> in, it is left at that state.
> 
> If a card is active when a system sleep starts, everything is
> straightforward and works exactly like before. But if the card was
> already suspended before the sleep began, then when MMC core power it up
> back on resume, its run-time PM status has to be updated to reflect the
> actual post-system sleep status.
> 
> The technique to do that is borrowed from the I2C runtime PM
> implementation (for more info also see Documentation/power/runtime_pm.txt).
> 
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>

Thanks, pushed to mmc-next.

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

Re: [PATCH 1/3] mmc: skip detection of nonremovable cards on rescan

[Ohad Ben-Cohen]

Hi Chris,

On Fri, Dec 17, 2010 at 2:51 AM, Chris Ball <cjb@laptop.org> wrote:
...
> Thanks, pushed to mmc-next with trivial cleanup as below:

Looks good, thanks.

I just briefly tested mmc-next and it looks fine.

Thanks,
Ohad.

Re: [PATCH 1/3] mmc: skip detection of nonremovable cards on rescan - card removal detection regression

[Maxim Levitsky]

On Mon, 2010-12-20 at 00:11 +0200, Ohad Ben-Cohen wrote:
> Hi Chris,
> 
> On Fri, Dec 17, 2010 at 2:51 AM, Chris Ball <cjb@laptop.org> wrote:
> ...
> > Thanks, pushed to mmc-next with trivial cleanup as below:
> 
> Looks good, thanks.
> 
> I just briefly tested mmc-next and it looks fine.

This patch breaks the CONFIG_MMC_UNSAFE_RESUME, because it sets fake
non-removeable flag, but the intended use of this option is to assume
that card is not removed _during_ suspend,  and it can still be removed
during normal use.
With this commit, card removal isn't detected.

Best regards,
	Maxim Levitsky

Re: [PATCH 1/3] mmc: skip detection of nonremovable cards on rescan - card removal detection regression

[Chris Ball]

Hi,

On Thu, Mar 03 2011, Maxim Levitsky wrote:
> This patch breaks the CONFIG_MMC_UNSAFE_RESUME, because it sets fake
> non-removeable flag, but the intended use of this option is to assume
> that card is not removed _during_ suspend,  and it can still be removed
> during normal use.
> With this commit, card removal isn't detected.

I guess we disagree about the semantics of CONFIG_MMC_UNSAFE_RESUME.
The Kconfig text says:

config MMC_UNSAFE_RESUME
        bool "Assume MMC/SD cards are non-removable (DANGEROUS)"

which I interpret as meaning that you should set MMC_UNSAFE_RESUME
*if you're using a card that cannot be removed*.  If your card *can* be
removed, then it would be extremely foolish to turn on MMC_UNSAFE_RESUME,
because then if someone removed your removable card during suspend and
modified it in another machine before resuming, you'd get massive
filesystem corruption.

What am I missing?

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

Re: [PATCH 1/3] mmc: skip detection of nonremovable cards on rescan - card removal detection regression

[Maxim Levitsky]

On Thu, 2011-03-03 at 21:28 -0500, Chris Ball wrote:
> Hi,
> 
> On Thu, Mar 03 2011, Maxim Levitsky wrote:
> > This patch breaks the CONFIG_MMC_UNSAFE_RESUME, because it sets fake
> > non-removeable flag, but the intended use of this option is to assume
> > that card is not removed _during_ suspend,  and it can still be removed
> > during normal use.
> > With this commit, card removal isn't detected.
> 
> I guess we disagree about the semantics of CONFIG_MMC_UNSAFE_RESUME.
> The Kconfig text says:
> 
> config MMC_UNSAFE_RESUME
>         bool "Assume MMC/SD cards are non-removable (DANGEROUS)"
> 
> which I interpret as meaning that you should set MMC_UNSAFE_RESUME
> *if you're using a card that cannot be removed*.  If your card *can* be
> removed, then it would be extremely foolish to turn on MMC_UNSAFE_RESUME,
> because then if someone removed your removable card during suspend and
> modified it in another machine before resuming, you'd get massive
> filesystem corruption.
That is assuming that someone kindly inserted the card back, while
system still was suspended, which is really unlikely, and besides, I
know who uses my computer (me mostly) and I know what I am doing.


The description also says:

"If you say Y here, the MMC layer will assume that all cards
stayed in their respective slots during the suspend. The
normal behaviour is to remove them at suspend and
redetecting them at resume. Breaking this assumption will
in most cases result in data corruption."


I own my computer, and I don't need the OS to take the decisions for me.

I don't want to have FS corruption if I accidentally suspend the system
with the card in the slot which could happen if that option is disabled.


Best regards,
	Maxim Levitsky

Re: [PATCH 1/3] mmc: skip detection of nonremovable cards on rescan - card removal detection regression

[Jaehoon Chung]

Hi..

I agreed Chris's opinion..If use CONFIG_MMC_UNSAFE_RESUME, you can know to
occur some problem..so mentions to "DANGEROUS"
If card can be removed during suspend, i think we have too many cases.
So this configuration...i think good that use only non-removable card.
If you considered every case, you should not use UNSAFE_RESUME..

Regards,
Jaehoon Chung

Maxim Levitsky wrote:
> On Thu, 2011-03-03 at 21:28 -0500, Chris Ball wrote:
>> Hi,
>>
>> On Thu, Mar 03 2011, Maxim Levitsky wrote:
>>> This patch breaks the CONFIG_MMC_UNSAFE_RESUME, because it sets fake
>>> non-removeable flag, but the intended use of this option is to assume
>>> that card is not removed _during_ suspend,  and it can still be removed
>>> during normal use.
>>> With this commit, card removal isn't detected.
>> I guess we disagree about the semantics of CONFIG_MMC_UNSAFE_RESUME.
>> The Kconfig text says:
>>
>> config MMC_UNSAFE_RESUME
>>         bool "Assume MMC/SD cards are non-removable (DANGEROUS)"
>>
>> which I interpret as meaning that you should set MMC_UNSAFE_RESUME
>> *if you're using a card that cannot be removed*.  If your card *can* be
>> removed, then it would be extremely foolish to turn on MMC_UNSAFE_RESUME,
>> because then if someone removed your removable card during suspend and
>> modified it in another machine before resuming, you'd get massive
>> filesystem corruption.
> That is assuming that someone kindly inserted the card back, while
> system still was suspended, which is really unlikely, and besides, I
> know who uses my computer (me mostly) and I know what I am doing.
> 
> 
> The description also says:
> 
> "If you say Y here, the MMC layer will assume that all cards
> stayed in their respective slots during the suspend. The
> normal behaviour is to remove them at suspend and
> redetecting them at resume. Breaking this assumption will
> in most cases result in data corruption."
> 
> 
> I own my computer, and I don't need the OS to take the decisions for me.
> 
> I don't want to have FS corruption if I accidentally suspend the system
> with the card in the slot which could happen if that option is disabled.
> 
> 
> Best regards,
> 	Maxim Levitsky
> 
> 
> 
> --
> 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
> 

Re: [PATCH 1/3] mmc: skip detection of nonremovable cards on rescan - card removal detection regression

[Maxim Levitsky]

On Fri, 2011-03-04 at 14:25 +0900, Jaehoon Chung wrote:
> Hi..
> 
> I agreed Chris's opinion..If use CONFIG_MMC_UNSAFE_RESUME, you can know to
> occur some problem..so mentions to "DANGEROUS"
> If card can be removed during suspend, i think we have too many cases.
> So this configuration...i think good that use only non-removable card.
> If you considered every case, you should not use UNSAFE_RESUME..
Thanks for breaking suspend of MMC cards.

You do know that _by default_, upon Linus request, _all_ USB sticks are
treated as CONFIG_MMC_UNSAFE_RESUME?
And in the wild there are much more USB sticks that MMC/SD cards.


And now if I remove the MMC card while system is running, system still
thinks its not.


Best regards,
	Maxim Levitsky





> 
> Regards,
> Jaehoon Chung
> 
> Maxim Levitsky wrote:
> > On Thu, 2011-03-03 at 21:28 -0500, Chris Ball wrote:
> >> Hi,
> >>
> >> On Thu, Mar 03 2011, Maxim Levitsky wrote:
> >>> This patch breaks the CONFIG_MMC_UNSAFE_RESUME, because it sets fake
> >>> non-removeable flag, but the intended use of this option is to assume
> >>> that card is not removed _during_ suspend,  and it can still be removed
> >>> during normal use.
> >>> With this commit, card removal isn't detected.
> >> I guess we disagree about the semantics of CONFIG_MMC_UNSAFE_RESUME.
> >> The Kconfig text says:
> >>
> >> config MMC_UNSAFE_RESUME
> >>         bool "Assume MMC/SD cards are non-removable (DANGEROUS)"
> >>
> >> which I interpret as meaning that you should set MMC_UNSAFE_RESUME
> >> *if you're using a card that cannot be removed*.  If your card *can* be
> >> removed, then it would be extremely foolish to turn on MMC_UNSAFE_RESUME,
> >> because then if someone removed your removable card during suspend and
> >> modified it in another machine before resuming, you'd get massive
> >> filesystem corruption.
> > That is assuming that someone kindly inserted the card back, while
> > system still was suspended, which is really unlikely, and besides, I
> > know who uses my computer (me mostly) and I know what I am doing.
> > 
> > 
> > The description also says:
> > 
> > "If you say Y here, the MMC layer will assume that all cards
> > stayed in their respective slots during the suspend. The
> > normal behaviour is to remove them at suspend and
> > redetecting them at resume. Breaking this assumption will
> > in most cases result in data corruption."
> > 
> > 
> > I own my computer, and I don't need the OS to take the decisions for me.
> > 
> > I don't want to have FS corruption if I accidentally suspend the system
> > with the card in the slot which could happen if that option is disabled.
> > 
> > 
> > Best regards,
> > 	Maxim Levitsky
> > 
> > 
> > 
> > --
> > 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
> > 
> 
> --
> 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

Re: [PATCH 1/3] mmc: skip detection of nonremovable cards on rescan - card removal detection regression

[Ohad Ben-Cohen]

On Fri, Mar 4, 2011 at 4:22 AM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> On Mon, 2010-12-20 at 00:11 +0200, Ohad Ben-Cohen wrote:
>> Hi Chris,
>>
>> On Fri, Dec 17, 2010 at 2:51 AM, Chris Ball <cjb@laptop.org> wrote:
>> ...
>> > Thanks, pushed to mmc-next with trivial cleanup as below:
>>
>> Looks good, thanks.
>>
>> I just briefly tested mmc-next and it looks fine.
>
> This patch breaks the CONFIG_MMC_UNSAFE_RESUME

Does the below work for you (untested) ?

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 6625c05..150b5f3 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1529,7 +1529,7 @@ void mmc_rescan(struct work_struct *work)
 	 * still present
 	 */
 	if (host->bus_ops && host->bus_ops->detect && !host->bus_dead
-	    && mmc_card_is_removable(host))
+	    && !(host->caps & MMC_CAP_NONREMOVABLE))
 		host->bus_ops->detect(host);

 	/*

Re: [PATCH 1/3] mmc: skip detection of nonremovable cards on rescan - card removal detection regression

[Dmitry Shmidt]

On Sat, Mar 5, 2011 at 7:03 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Fri, Mar 4, 2011 at 4:22 AM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
>> On Mon, 2010-12-20 at 00:11 +0200, Ohad Ben-Cohen wrote:
>>> Hi Chris,
>>>
>>> On Fri, Dec 17, 2010 at 2:51 AM, Chris Ball <cjb@laptop.org> wrote:
>>> ...
>>> > Thanks, pushed to mmc-next with trivial cleanup as below:
>>>
>>> Looks good, thanks.
>>>
>>> I just briefly tested mmc-next and it looks fine.
>>
>> This patch breaks the CONFIG_MMC_UNSAFE_RESUME
>
> Does the below work for you (untested) ?
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 6625c05..150b5f3 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1529,7 +1529,7 @@ void mmc_rescan(struct work_struct *work)
>         * still present
>         */
>        if (host->bus_ops && host->bus_ops->detect && !host->bus_dead
> -           && mmc_card_is_removable(host))
> +           && !(host->caps & MMC_CAP_NONREMOVABLE))
>                host->bus_ops->detect(host);
>
>        /*
>
This works for me,
Thanks,

Dmitry

Re: [PATCH 1/3] mmc: skip detection of nonremovable cards on rescan - card removal detection regression

[Ohad Ben-Cohen]

On Mon, Mar 7, 2011 at 9:01 PM, Dmitry Shmidt <dimitrysh@google.com> wrote:
> On Sat, Mar 5, 2011 at 7:03 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> On Fri, Mar 4, 2011 at 4:22 AM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
>>> This patch breaks the CONFIG_MMC_UNSAFE_RESUME
>>
>> Does the below work for you (untested) ?
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 6625c05..150b5f3 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -1529,7 +1529,7 @@ void mmc_rescan(struct work_struct *work)
>>         * still present
>>         */
>>        if (host->bus_ops && host->bus_ops->detect && !host->bus_dead
>> -           && mmc_card_is_removable(host))
>> +           && !(host->caps & MMC_CAP_NONREMOVABLE))
>>                host->bus_ops->detect(host);
>>
>>        /*
>>
> This works for me,

Thanks.

Maxim, does this work for you too ?

Re: [PATCH 1/3] mmc: skip detection of nonremovable cards on rescan - card removal detection regression

[Maxim Levitsky]

On Tue, 2011-03-08 at 18:44 +0200, Ohad Ben-Cohen wrote:
> On Mon, Mar 7, 2011 at 9:01 PM, Dmitry Shmidt <dimitrysh@google.com> wrote:
> > On Sat, Mar 5, 2011 at 7:03 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> >> On Fri, Mar 4, 2011 at 4:22 AM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> >>> This patch breaks the CONFIG_MMC_UNSAFE_RESUME
> >>
> >> Does the below work for you (untested) ?
> >>
> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> >> index 6625c05..150b5f3 100644
> >> --- a/drivers/mmc/core/core.c
> >> +++ b/drivers/mmc/core/core.c
> >> @@ -1529,7 +1529,7 @@ void mmc_rescan(struct work_struct *work)
> >>         * still present
> >>         */
> >>        if (host->bus_ops && host->bus_ops->detect && !host->bus_dead
> >> -           && mmc_card_is_removable(host))
> >> +           && !(host->caps & MMC_CAP_NONREMOVABLE))
> >>                host->bus_ops->detect(host);
> >>
> >>        /*
> >>
> > This works for me,
> 
> Thanks.
> 
> Maxim, does this work for you too ?

Yep, just tested.

Best regards,
	Maxim Levitsky

Re: [PATCH 1/3] mmc: skip detection of nonremovable cards on rescan - card removal detection regression

[Ohad Ben-Cohen]

On Tue, Mar 8, 2011 at 7:02 PM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
>> Maxim, does this work for you too ?
>
> Yep, just tested.

Thanks