[PATCH 1/2] MMC: fix all hangs related to mmc/sd card insert/removal during suspend/resume.

[PATCH 1/2] MMC: fix all hangs related to mmc/sd card insert/removal during suspend/resume.

[Maxim Levitsky]

If you don't use CONFIG_MMC_UNSAFE_RESUME, as soon as you attempt to
suspend, the card will be removed, therefore this patch doesn't change
the behavior of this option.

However the removal will be done by pm notifier, which runs while
userspace is still not frozen and thus can freely use del_gendisk,
without the risk of deadlock which would happen otherwise.


Card detect workqueue is now freezeable,
therefore if you do use CONFIG_MMC_UNSAFE_RESUME,
and remove the card during suspend, the removal will be
detected as soon as userspace is unfrozen, again at the moment
it is safe to call del_gendisk.

Tested with and without CONFIG_MMC_UNSAFE_RESUME with suspend and hibernate.

Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
---
 drivers/mmc/core/core.c  |   54 +++++++++++++++++++++++++++------------------
 drivers/mmc/core/host.c  |    6 +++++
 include/linux/mmc/host.h |    3 ++
 3 files changed, 41 insertions(+), 22 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 569e94d..0cba53a 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1259,26 +1259,11 @@ int mmc_suspend_host(struct mmc_host *host)
 
 	if (host->caps & MMC_CAP_DISABLE)
 		cancel_delayed_work(&host->disable);
-	cancel_delayed_work(&host->detect);
-	mmc_flush_scheduled_work();
 
 	mmc_bus_get(host);
 	if (host->bus_ops && !host->bus_dead) {
 		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.
-			 */
-			if (host->bus_ops->remove)
-				host->bus_ops->remove(host);
-			mmc_claim_host(host);
-			mmc_detach_bus(host);
-			mmc_release_host(host);
-			host->pm_flags = 0;
-			err = 0;
-		}
 	}
 	mmc_bus_put(host);
 
@@ -1310,12 +1295,6 @@ int mmc_resume_host(struct mmc_host *host)
 			printk(KERN_WARNING "%s: error %d during resume "
 					    "(card was removed?)\n",
 					    mmc_hostname(host), err);
-			if (host->bus_ops->remove)
-				host->bus_ops->remove(host);
-			mmc_claim_host(host);
-			mmc_detach_bus(host);
-			mmc_release_host(host);
-			/* no need to bother upper layers */
 			err = 0;
 		}
 	}
@@ -1330,6 +1309,37 @@ int mmc_resume_host(struct mmc_host *host)
 	return err;
 }
 
+/* Do the card removal on suspend if card is assumed removeable
+ * Do that in pm notifier while userspace isn't yet frozen, so we will be able
+   to sync the card.
+*/
+int mmc_pm_notify(struct notifier_block *notify_block,
+					unsigned long mode, void *unused)
+{
+	struct mmc_host *host = container_of(
+		notify_block, struct mmc_host, pm_notify);
+
+
+	switch (mode) {
+	case PM_HIBERNATION_PREPARE:
+	case PM_SUSPEND_PREPARE:
+
+		if (!host->bus_ops || host->bus_ops->suspend)
+			break;
+
+		if (host->bus_ops->remove)
+			host->bus_ops->remove(host);
+		mmc_claim_host(host);
+		mmc_detach_bus(host);
+		mmc_release_host(host);
+		host->pm_flags = 0;
+		break;
+
+	}
+
+	return 0;
+}
+
 EXPORT_SYMBOL(mmc_resume_host);
 
 #endif
@@ -1338,7 +1348,7 @@ static int __init mmc_init(void)
 {
 	int ret;
 
-	workqueue = create_singlethread_workqueue("kmmcd");
+	workqueue = create_freezeable_workqueue("kmmcd");
 	if (!workqueue)
 		return -ENOMEM;
 
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 4735390..8a631c8 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -17,6 +17,7 @@
 #include <linux/pagemap.h>
 #include <linux/leds.h>
 #include <linux/slab.h>
+#include <linux/suspend.h>
 
 #include <linux/mmc/host.h>
 
@@ -85,6 +86,8 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
 	init_waitqueue_head(&host->wq);
 	INIT_DELAYED_WORK(&host->detect, mmc_rescan);
 	INIT_DELAYED_WORK_DEFERRABLE(&host->disable, mmc_host_deeper_disable);
+	host->pm_notify.notifier_call = mmc_pm_notify;
+
 
 	/*
 	 * By default, hosts do not support SGIO or large requests.
@@ -133,6 +136,7 @@ int mmc_add_host(struct mmc_host *host)
 #endif
 
 	mmc_start_host(host);
+	register_pm_notifier(&host->pm_notify);
 
 	return 0;
 }
@@ -149,6 +153,7 @@ EXPORT_SYMBOL(mmc_add_host);
  */
 void mmc_remove_host(struct mmc_host *host)
 {
+	unregister_pm_notifier(&host->pm_notify);
 	mmc_stop_host(host);
 
 #ifdef CONFIG_DEBUG_FS
@@ -158,6 +163,7 @@ void mmc_remove_host(struct mmc_host *host)
 	device_del(&host->class_dev);
 
 	led_trigger_unregister_simple(host->led);
+
 }
 
 EXPORT_SYMBOL(mmc_remove_host);
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index f65913c..b45812d 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -124,6 +124,7 @@ struct mmc_host {
 	unsigned int		f_min;
 	unsigned int		f_max;
 	u32			ocr_avail;
+	struct notifier_block	pm_notify;
 
 #define MMC_VDD_165_195		0x00000080	/* VDD voltage 1.65 - 1.95 */
 #define MMC_VDD_20_21		0x00000100	/* VDD voltage 2.0 ~ 2.1 */
@@ -257,6 +258,8 @@ int mmc_card_can_sleep(struct mmc_host *host);
 int mmc_host_enable(struct mmc_host *host);
 int mmc_host_disable(struct mmc_host *host);
 int mmc_host_lazy_disable(struct mmc_host *host);
+int mmc_pm_notify(struct notifier_block *notify_block, unsigned long, void *);
+
 
 static inline void mmc_set_disable_delay(struct mmc_host *host,
 					 unsigned int disable_delay)
-- 
1.7.0.4

[PATCH 2/2] mmc: make sdhci work with ricoh mmc controller

[Maxim Levitsky]

The current way of disabling it is not well tested by vendor
and has all kinds of bugs that show up on resume from ram/disk.
A very good example is a dead SDHCI controller.

Old way of disabling is still supported by
continuing to use CONFIG_MMC_RICOH_MMC.

Based on 'http://list.drzeus.cx/pipermail/sdhci-devel/2007-December/002085.html'
Therefore most of the credit for this goes to Andrew de Quincey

CC: Andrew de Quincey <adq_dvb@lidskialf.net>

Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
Acked-by: Philip Langdale <philipl@overt.org>
---
 drivers/mmc/host/sdhci-pci.c |   41 +++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.c     |    3 ++-
 drivers/mmc/host/sdhci.h     |    4 ++++
 3 files changed, 47 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 65483fd..e021431 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -17,6 +17,7 @@
 #include <linux/pci.h>
 #include <linux/dma-mapping.h>
 #include <linux/slab.h>
+#include <linux/device.h>
 
 #include <linux/mmc/host.h>
 
@@ -84,7 +85,30 @@ static int ricoh_probe(struct sdhci_pci_chip *chip)
 	if (chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SAMSUNG ||
 	    chip->pdev->subsystem_vendor == PCI_VENDOR_ID_SONY)
 		chip->quirks |= SDHCI_QUIRK_NO_CARD_NO_RESET;
+	return 0;
+}
+
+static int ricoh_mmc_probe_slot(struct sdhci_pci_slot *slot)
+{
+	slot->host->caps =
+		((0x21 << SDHCI_TIMEOUT_CLK_SHIFT)
+			& SDHCI_TIMEOUT_CLK_MASK) |
+
+		((0x21 << SDHCI_CLOCK_BASE_SHIFT)
+			& SDHCI_CLOCK_BASE_MASK) |
 
+		SDHCI_TIMEOUT_CLK_UNIT |
+		SDHCI_CAN_VDD_330 |
+		SDHCI_CAN_DO_SDMA;
+	return 0;
+}
+
+static int ricoh_mmc_resume(struct sdhci_pci_chip *chip)
+{
+	/* Apply a delay to allow controller to settle */
+	/* Otherwise it becomes confused if card state changed
+		during suspend */
+	msleep(500);
 	return 0;
 }
 
@@ -95,6 +119,15 @@ static const struct sdhci_pci_fixes sdhci_ricoh = {
 			  SDHCI_QUIRK_CLOCK_BEFORE_RESET,
 };
 
+static const struct sdhci_pci_fixes sdhci_ricoh_mmc = {
+	.probe_slot	= ricoh_mmc_probe_slot,
+	.resume		= ricoh_mmc_resume,
+	.quirks		= SDHCI_QUIRK_32BIT_DMA_ADDR |
+			  SDHCI_QUIRK_CLOCK_BEFORE_RESET |
+			  SDHCI_QUIRK_NO_CARD_NO_RESET |
+			  SDHCI_QUIRK_MISSING_CAPS
+};
+
 static const struct sdhci_pci_fixes sdhci_ene_712 = {
 	.quirks		= SDHCI_QUIRK_SINGLE_POWER_WRITE |
 			  SDHCI_QUIRK_BROKEN_DMA,
@@ -374,6 +407,14 @@ static const struct pci_device_id pci_ids[] __devinitdata = {
 	},
 
 	{
+		.vendor         = PCI_VENDOR_ID_RICOH,
+		.device         = 0x843,
+		.subvendor      = PCI_ANY_ID,
+		.subdevice      = PCI_ANY_ID,
+		.driver_data    = (kernel_ulong_t)&sdhci_ricoh_mmc,
+	},
+
+	{
 		.vendor		= PCI_VENDOR_ID_ENE,
 		.device		= PCI_DEVICE_ID_ENE_CB712_SD,
 		.subvendor	= PCI_ANY_ID,
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c6d1bd8..483b78e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1687,7 +1687,8 @@ int sdhci_add_host(struct sdhci_host *host)
 			host->version);
 	}
 
-	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
+	caps = (host->quirks & SDHCI_QUIRK_MISSING_CAPS) ? host->caps :
+		sdhci_readl(host, SDHCI_CAPABILITIES);
 
 	if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
 		host->flags |= SDHCI_USE_SDMA;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index c846813..b1839a3 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -240,6 +240,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN		(1<<25)
 /* Controller cannot support End Attribute in NOP ADMA descriptor */
 #define SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC		(1<<26)
+/* Controller is missing device caps. Use caps provided by host */
+#define SDHCI_QUIRK_MISSING_CAPS			(1<<27)
 
 	int			irq;		/* Device IRQ */
 	void __iomem *		ioaddr;		/* Mapped address */
@@ -292,6 +294,8 @@ struct sdhci_host {
 
 	struct timer_list	timer;		/* Timer for timeouts */
 
+	unsigned int		caps;		/* Alternative capabilities */
+
 	unsigned long		private[0] ____cacheline_aligned;
 };
 
-- 
1.7.0.4

Re: [PATCH] Two fixes for my mmc/sd cardreader

[Maxim Levitsky]

On Fri, 2010-06-18 at 00:21 +0300, Maxim Levitsky wrote: 
> Hi,
> 
> These are 2 fixes for my card reader.
> 
> 
> First patch fixes old issue with system hand on suspend to disk/ram with
> mmc card inserted.
> I updated description, and pm notification registration order.
> I think this patch can an should go to 2.6.35, because it fixes long
> standing and nasty regression.
> 
> The second patch is a result of my work trying to understand why my card
> reader sometimes dies on resume.
> This reader has a special MMC function which steals MMC cards, and until
> now had no driver. A way to disable it was found, and while it works, it
> has (at least here) a side effect of killing the controller on resume
> from ram/disk (and it happens often, and doesn't depend of whether card
> was in slot or not during suspend).
> 
> Fortunately it turned out that MMC part is _almost_ standard SDHCI
> controller.
> This patch adds support for this device to standard sdhci driver.
> Unfortunately, this support still contais small hack.
> It waits 1/2 of a second on resume before initializing the controller.
> Not doing so, and resuming with MMC card present results in confused
> controller. It is not dead though. A card reinsert makes it work again
> with all cards.
> Yet the 1st patch is must for this because otherwise mmc core seeing
> that controller doesn't respond, removes the card, therefore hangs the
> system.
> It doesn't happen when I wait these 1/2 of second though.
> 
> I think that this patch is also ok for 2.6.35, because it only adds new
> functionality.
> You are free to disable MMC controller using the same
> CONFIG_MMC_RICOH_MMC.
> 
> If you don't disable it though, instead of full lack of functionality
> you will get full featured MMC controller.
> 
> Best regards,
> Maxim Levitsky
> 
> 
> 
ping

Re: [PATCH] Two fixes for my mmc/sd cardreader

[Andrew Morton]

On Mon, 21 Jun 2010 22:21:44 +0300
Maxim Levitsky <maximlevitsky@gmail.com> wrote:

> On Fri, 2010-06-18 at 00:21 +0300, Maxim Levitsky wrote: 
> > Hi,
> > 
> > These are 2 fixes for my card reader.
> > 
> > 
> > First patch fixes old issue with system hand on suspend to disk/ram with
> > mmc card inserted.
> > I updated description, and pm notification registration order.
> > I think this patch can an should go to 2.6.35, because it fixes long
> > standing and nasty regression.
> > 
> > The second patch is a result of my work trying to understand why my card
> > reader sometimes dies on resume.
> > This reader has a special MMC function which steals MMC cards, and until
> > now had no driver. A way to disable it was found, and while it works, it
> > has (at least here) a side effect of killing the controller on resume
> > from ram/disk (and it happens often, and doesn't depend of whether card
> > was in slot or not during suspend).
> > 
> > Fortunately it turned out that MMC part is _almost_ standard SDHCI
> > controller.
> > This patch adds support for this device to standard sdhci driver.
> > Unfortunately, this support still contais small hack.
> > It waits 1/2 of a second on resume before initializing the controller.
> > Not doing so, and resuming with MMC card present results in confused
> > controller. It is not dead though. A card reinsert makes it work again
> > with all cards.
> > Yet the 1st patch is must for this because otherwise mmc core seeing
> > that controller doesn't respond, removes the card, therefore hangs the
> > system.
> > It doesn't happen when I wait these 1/2 of second though.
> > 
> > I think that this patch is also ok for 2.6.35, because it only adds new
> > functionality.
> > You are free to disable MMC controller using the same
> > CONFIG_MMC_RICOH_MMC.
> > 
> > If you don't disable it though, instead of full lack of functionality
> > you will get full featured MMC controller.
> > 
> > Best regards,
> > Maxim Levitsky
> > 
> > 
> > 
> ping

hey, that was only three days.  I commonly leave things to bake on the
mailing list for a while, see what people have to say about it. 
Particularly with subsystems like MMC.

Re: [PATCH] Two fixes for my mmc/sd cardreader

[Maxim Levitsky]

On Mon, 2010-06-21 at 12:39 -0700, Andrew Morton wrote: 
> On Mon, 21 Jun 2010 22:21:44 +0300
> Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> 
> > On Fri, 2010-06-18 at 00:21 +0300, Maxim Levitsky wrote: 
> > > Hi,
> > > 
> > > These are 2 fixes for my card reader.
> > > 
> > > 
> > > First patch fixes old issue with system hand on suspend to disk/ram with
> > > mmc card inserted.
> > > I updated description, and pm notification registration order.
> > > I think this patch can an should go to 2.6.35, because it fixes long
> > > standing and nasty regression.
> > > 
> > > The second patch is a result of my work trying to understand why my card
> > > reader sometimes dies on resume.
> > > This reader has a special MMC function which steals MMC cards, and until
> > > now had no driver. A way to disable it was found, and while it works, it
> > > has (at least here) a side effect of killing the controller on resume
> > > from ram/disk (and it happens often, and doesn't depend of whether card
> > > was in slot or not during suspend).
> > > 
> > > Fortunately it turned out that MMC part is _almost_ standard SDHCI
> > > controller.
> > > This patch adds support for this device to standard sdhci driver.
> > > Unfortunately, this support still contais small hack.
> > > It waits 1/2 of a second on resume before initializing the controller.
> > > Not doing so, and resuming with MMC card present results in confused
> > > controller. It is not dead though. A card reinsert makes it work again
> > > with all cards.
> > > Yet the 1st patch is must for this because otherwise mmc core seeing
> > > that controller doesn't respond, removes the card, therefore hangs the
> > > system.
> > > It doesn't happen when I wait these 1/2 of second though.
> > > 
> > > I think that this patch is also ok for 2.6.35, because it only adds new
> > > functionality.
> > > You are free to disable MMC controller using the same
> > > CONFIG_MMC_RICOH_MMC.
> > > 
> > > If you don't disable it though, instead of full lack of functionality
> > > you will get full featured MMC controller.
> > > 
> > > Best regards,
> > > Maxim Levitsky
> > > 
> > > 
> > > 
> > ping
> 
> hey, that was only three days.  I commonly leave things to bake on the
> mailing list for a while, see what people have to say about it. 
> Particularly with subsystems like MMC.

Sure.

I just posted the patches on weekend, thought they weren't noticed...
Anyway confirm again that I didn't yet seen any problem with my card
reader.
(I test it regularly)

Best regards,
Maxim Levitsky

Re: [PATCH 1/2] MMC: fix all hangs related to mmc/sd card insert/removal during suspend/resume.

[Adrian Hunter]

ext Maxim Levitsky wrote:
> If you don't use CONFIG_MMC_UNSAFE_RESUME, as soon as you attempt to
> suspend, the card will be removed, therefore this patch doesn't change
> the behavior of this option.
> 
> However the removal will be done by pm notifier, which runs while
> userspace is still not frozen and thus can freely use del_gendisk,
> without the risk of deadlock which would happen otherwise.
> 
> 
> Card detect workqueue is now freezeable,
> therefore if you do use CONFIG_MMC_UNSAFE_RESUME,
> and remove the card during suspend, the removal will be
> detected as soon as userspace is unfrozen, again at the moment
> it is safe to call del_gendisk.
> 
> Tested with and without CONFIG_MMC_UNSAFE_RESUME with suspend and hibernate.
> 
> Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> ---
>  drivers/mmc/core/core.c  |   54 +++++++++++++++++++++++++++------------------
>  drivers/mmc/core/host.c  |    6 +++++
>  include/linux/mmc/host.h |    3 ++
>  3 files changed, 41 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 569e94d..0cba53a 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1259,26 +1259,11 @@ int mmc_suspend_host(struct mmc_host *host)
>  
>  	if (host->caps & MMC_CAP_DISABLE)
>  		cancel_delayed_work(&host->disable);
> -	cancel_delayed_work(&host->detect);
> -	mmc_flush_scheduled_work();
>  
>  	mmc_bus_get(host);
>  	if (host->bus_ops && !host->bus_dead) {
>  		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.
> -			 */
> -			if (host->bus_ops->remove)
> -				host->bus_ops->remove(host);
> -			mmc_claim_host(host);
> -			mmc_detach_bus(host);
> -			mmc_release_host(host);
> -			host->pm_flags = 0;
> -			err = 0;
> -		}
>  	}
>  	mmc_bus_put(host);
>  
> @@ -1310,12 +1295,6 @@ int mmc_resume_host(struct mmc_host *host)
>  			printk(KERN_WARNING "%s: error %d during resume "
>  					    "(card was removed?)\n",
>  					    mmc_hostname(host), err);
> -			if (host->bus_ops->remove)
> -				host->bus_ops->remove(host);
> -			mmc_claim_host(host);
> -			mmc_detach_bus(host);
> -			mmc_release_host(host);
> -			/* no need to bother upper layers */
>  			err = 0;
>  		}
>  	}
> @@ -1330,6 +1309,37 @@ int mmc_resume_host(struct mmc_host *host)
>  	return err;
>  }
>  
> +/* Do the card removal on suspend if card is assumed removeable
> + * Do that in pm notifier while userspace isn't yet frozen, so we will be able
> +   to sync the card.
> +*/
> +int mmc_pm_notify(struct notifier_block *notify_block,
> +					unsigned long mode, void *unused)
> +{
> +	struct mmc_host *host = container_of(
> +		notify_block, struct mmc_host, pm_notify);
> +
> +
> +	switch (mode) {
> +	case PM_HIBERNATION_PREPARE:
> +	case PM_SUSPEND_PREPARE:
> +
> +		if (!host->bus_ops || host->bus_ops->suspend)
> +			break;
> +
> +		if (host->bus_ops->remove)
> +			host->bus_ops->remove(host);
> +		mmc_claim_host(host);
> +		mmc_detach_bus(host);
> +		mmc_release_host(host);
> +		host->pm_flags = 0;
> +		break;

Is it possible that you receive PM_SUSPEND_PREPARE
but there is no suspend and therefore no resume
and therefore the card is removed but not detected
again?

Is it possible that you are racing with kmmcd and the
card is added after you receive PM_SUSPEND_PREPARE but
before kmmcd is frozen?

> +
> +	}
> +
> +	return 0;
> +}
> +
>  EXPORT_SYMBOL(mmc_resume_host);
>  
>  #endif
> @@ -1338,7 +1348,7 @@ static int __init mmc_init(void)
>  {
>  	int ret;
>  
> -	workqueue = create_singlethread_workqueue("kmmcd");
> +	workqueue = create_freezeable_workqueue("kmmcd");
>  	if (!workqueue)
>  		return -ENOMEM;
>  
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 4735390..8a631c8 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -17,6 +17,7 @@
>  #include <linux/pagemap.h>
>  #include <linux/leds.h>
>  #include <linux/slab.h>
> +#include <linux/suspend.h>
>  
>  #include <linux/mmc/host.h>
>  
> @@ -85,6 +86,8 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
>  	init_waitqueue_head(&host->wq);
>  	INIT_DELAYED_WORK(&host->detect, mmc_rescan);
>  	INIT_DELAYED_WORK_DEFERRABLE(&host->disable, mmc_host_deeper_disable);
> +	host->pm_notify.notifier_call = mmc_pm_notify;
> +
>  
>  	/*
>  	 * By default, hosts do not support SGIO or large requests.
> @@ -133,6 +136,7 @@ int mmc_add_host(struct mmc_host *host)
>  #endif
>  
>  	mmc_start_host(host);
> +	register_pm_notifier(&host->pm_notify);
>  
>  	return 0;
>  }
> @@ -149,6 +153,7 @@ EXPORT_SYMBOL(mmc_add_host);
>   */
>  void mmc_remove_host(struct mmc_host *host)
>  {
> +	unregister_pm_notifier(&host->pm_notify);
>  	mmc_stop_host(host);
>  
>  #ifdef CONFIG_DEBUG_FS
> @@ -158,6 +163,7 @@ void mmc_remove_host(struct mmc_host *host)
>  	device_del(&host->class_dev);
>  
>  	led_trigger_unregister_simple(host->led);
> +
>  }
>  
>  EXPORT_SYMBOL(mmc_remove_host);
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index f65913c..b45812d 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -124,6 +124,7 @@ struct mmc_host {
>  	unsigned int		f_min;
>  	unsigned int		f_max;
>  	u32			ocr_avail;
> +	struct notifier_block	pm_notify;
>  
>  #define MMC_VDD_165_195		0x00000080	/* VDD voltage 1.65 - 1.95 */
>  #define MMC_VDD_20_21		0x00000100	/* VDD voltage 2.0 ~ 2.1 */
> @@ -257,6 +258,8 @@ int mmc_card_can_sleep(struct mmc_host *host);
>  int mmc_host_enable(struct mmc_host *host);
>  int mmc_host_disable(struct mmc_host *host);
>  int mmc_host_lazy_disable(struct mmc_host *host);
> +int mmc_pm_notify(struct notifier_block *notify_block, unsigned long, void *);
> +
>  
>  static inline void mmc_set_disable_delay(struct mmc_host *host,
>  					 unsigned int disable_delay)

Re: [PATCH 1/2] MMC: fix all hangs related to mmc/sd card insert/removal during suspend/resume.

[Maxim Levitsky]

On Mon, 2010-06-21 at 23:04 +0300, Adrian Hunter wrote: 
> ext Maxim Levitsky wrote:
> > If you don't use CONFIG_MMC_UNSAFE_RESUME, as soon as you attempt to
> > suspend, the card will be removed, therefore this patch doesn't change
> > the behavior of this option.
> > 
> > However the removal will be done by pm notifier, which runs while
> > userspace is still not frozen and thus can freely use del_gendisk,
> > without the risk of deadlock which would happen otherwise.
> > 
> > 
> > Card detect workqueue is now freezeable,
> > therefore if you do use CONFIG_MMC_UNSAFE_RESUME,
> > and remove the card during suspend, the removal will be
> > detected as soon as userspace is unfrozen, again at the moment
> > it is safe to call del_gendisk.
> > 
> > Tested with and without CONFIG_MMC_UNSAFE_RESUME with suspend and hibernate.
> > 
> > Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> > ---
> >  drivers/mmc/core/core.c  |   54 +++++++++++++++++++++++++++------------------
> >  drivers/mmc/core/host.c  |    6 +++++
> >  include/linux/mmc/host.h |    3 ++
> >  3 files changed, 41 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index 569e94d..0cba53a 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -1259,26 +1259,11 @@ int mmc_suspend_host(struct mmc_host *host)
> >  
> >  	if (host->caps & MMC_CAP_DISABLE)
> >  		cancel_delayed_work(&host->disable);
> > -	cancel_delayed_work(&host->detect);
> > -	mmc_flush_scheduled_work();
> >  
> >  	mmc_bus_get(host);
> >  	if (host->bus_ops && !host->bus_dead) {
> >  		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.
> > -			 */
> > -			if (host->bus_ops->remove)
> > -				host->bus_ops->remove(host);
> > -			mmc_claim_host(host);
> > -			mmc_detach_bus(host);
> > -			mmc_release_host(host);
> > -			host->pm_flags = 0;
> > -			err = 0;
> > -		}
> >  	}
> >  	mmc_bus_put(host);
> >  
> > @@ -1310,12 +1295,6 @@ int mmc_resume_host(struct mmc_host *host)
> >  			printk(KERN_WARNING "%s: error %d during resume "
> >  					    "(card was removed?)\n",
> >  					    mmc_hostname(host), err);
> > -			if (host->bus_ops->remove)
> > -				host->bus_ops->remove(host);
> > -			mmc_claim_host(host);
> > -			mmc_detach_bus(host);
> > -			mmc_release_host(host);
> > -			/* no need to bother upper layers */
> >  			err = 0;
> >  		}
> >  	}
> > @@ -1330,6 +1309,37 @@ int mmc_resume_host(struct mmc_host *host)
> >  	return err;
> >  }
> >  
> > +/* Do the card removal on suspend if card is assumed removeable
> > + * Do that in pm notifier while userspace isn't yet frozen, so we will be able
> > +   to sync the card.
> > +*/
> > +int mmc_pm_notify(struct notifier_block *notify_block,
> > +					unsigned long mode, void *unused)
> > +{
> > +	struct mmc_host *host = container_of(
> > +		notify_block, struct mmc_host, pm_notify);
> > +
> > +
> > +	switch (mode) {
> > +	case PM_HIBERNATION_PREPARE:
> > +	case PM_SUSPEND_PREPARE:
> > +
> > +		if (!host->bus_ops || host->bus_ops->suspend)
> > +			break;
> > +
> > +		if (host->bus_ops->remove)
> > +			host->bus_ops->remove(host);
> > +		mmc_claim_host(host);
> > +		mmc_detach_bus(host);
> > +		mmc_release_host(host);
> > +		host->pm_flags = 0;
> > +		break;
> 
> Is it possible that you receive PM_SUSPEND_PREPARE
> but there is no suspend and therefore no resume
> and therefore the card is removed but not detected
> again?
This is very good point.
The solution is to kick mmc detection thread from this notifier.
on resume.
I update the patch.

> 
> Is it possible that you are racing with kmmcd and the
> card is added after you receive PM_SUSPEND_PREPARE but
> before kmmcd is frozen?
This is unlikely but valid race.
I afraid I don't know nice way to solve it right now.
I can add some ad-hoc variable to tell interrupt handler not to kick the
detection workqueue after suspend notifier was called.

I wish there was a generic freeze_workqueue function.


Best regards,
Maxim Levitsky

Re: [PATCH 1/2] MMC: fix all hangs related to mmc/sd card insert/removal during suspend/resume.

[Rafael J. Wysocki]

On Monday, June 21, 2010, Maxim Levitsky wrote:
> On Mon, 2010-06-21 at 23:04 +0300, Adrian Hunter wrote: 
> > ext Maxim Levitsky wrote:
> > > If you don't use CONFIG_MMC_UNSAFE_RESUME, as soon as you attempt to
> > > suspend, the card will be removed, therefore this patch doesn't change
> > > the behavior of this option.
> > > 
> > > However the removal will be done by pm notifier, which runs while
> > > userspace is still not frozen and thus can freely use del_gendisk,
> > > without the risk of deadlock which would happen otherwise.
> > > 
> > > 
> > > Card detect workqueue is now freezeable,
> > > therefore if you do use CONFIG_MMC_UNSAFE_RESUME,
> > > and remove the card during suspend, the removal will be
> > > detected as soon as userspace is unfrozen, again at the moment
> > > it is safe to call del_gendisk.
> > > 
> > > Tested with and without CONFIG_MMC_UNSAFE_RESUME with suspend and hibernate.
> > > 
> > > Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> > > ---
> > >  drivers/mmc/core/core.c  |   54 +++++++++++++++++++++++++++------------------
> > >  drivers/mmc/core/host.c  |    6 +++++
> > >  include/linux/mmc/host.h |    3 ++
> > >  3 files changed, 41 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > > index 569e94d..0cba53a 100644
> > > --- a/drivers/mmc/core/core.c
> > > +++ b/drivers/mmc/core/core.c
> > > @@ -1259,26 +1259,11 @@ int mmc_suspend_host(struct mmc_host *host)
> > >  
> > >  	if (host->caps & MMC_CAP_DISABLE)
> > >  		cancel_delayed_work(&host->disable);
> > > -	cancel_delayed_work(&host->detect);
> > > -	mmc_flush_scheduled_work();
> > >  
> > >  	mmc_bus_get(host);
> > >  	if (host->bus_ops && !host->bus_dead) {
> > >  		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.
> > > -			 */
> > > -			if (host->bus_ops->remove)
> > > -				host->bus_ops->remove(host);
> > > -			mmc_claim_host(host);
> > > -			mmc_detach_bus(host);
> > > -			mmc_release_host(host);
> > > -			host->pm_flags = 0;
> > > -			err = 0;
> > > -		}
> > >  	}
> > >  	mmc_bus_put(host);
> > >  
> > > @@ -1310,12 +1295,6 @@ int mmc_resume_host(struct mmc_host *host)
> > >  			printk(KERN_WARNING "%s: error %d during resume "
> > >  					    "(card was removed?)\n",
> > >  					    mmc_hostname(host), err);
> > > -			if (host->bus_ops->remove)
> > > -				host->bus_ops->remove(host);
> > > -			mmc_claim_host(host);
> > > -			mmc_detach_bus(host);
> > > -			mmc_release_host(host);
> > > -			/* no need to bother upper layers */
> > >  			err = 0;
> > >  		}
> > >  	}
> > > @@ -1330,6 +1309,37 @@ int mmc_resume_host(struct mmc_host *host)
> > >  	return err;
> > >  }
> > >  
> > > +/* Do the card removal on suspend if card is assumed removeable
> > > + * Do that in pm notifier while userspace isn't yet frozen, so we will be able
> > > +   to sync the card.
> > > +*/
> > > +int mmc_pm_notify(struct notifier_block *notify_block,
> > > +					unsigned long mode, void *unused)
> > > +{
> > > +	struct mmc_host *host = container_of(
> > > +		notify_block, struct mmc_host, pm_notify);
> > > +
> > > +
> > > +	switch (mode) {
> > > +	case PM_HIBERNATION_PREPARE:
> > > +	case PM_SUSPEND_PREPARE:
> > > +
> > > +		if (!host->bus_ops || host->bus_ops->suspend)
> > > +			break;
> > > +
> > > +		if (host->bus_ops->remove)
> > > +			host->bus_ops->remove(host);
> > > +		mmc_claim_host(host);
> > > +		mmc_detach_bus(host);
> > > +		mmc_release_host(host);
> > > +		host->pm_flags = 0;
> > > +		break;
> > 
> > Is it possible that you receive PM_SUSPEND_PREPARE
> > but there is no suspend and therefore no resume
> > and therefore the card is removed but not detected
> > again?
> This is very good point.
> The solution is to kick mmc detection thread from this notifier.
> on resume.
> I update the patch.
> 
> > 
> > Is it possible that you are racing with kmmcd and the
> > card is added after you receive PM_SUSPEND_PREPARE but
> > before kmmcd is frozen?
> This is unlikely but valid race.
> I afraid I don't know nice way to solve it right now.
> I can add some ad-hoc variable to tell interrupt handler not to kick the
> detection workqueue after suspend notifier was called.
> 
> I wish there was a generic freeze_workqueue function.

There are freezable workqueues that are automatically frozen during suspend
by the process freezer.  However, at the moment they need to be singlethread
and I'm not sure if using one in this particular case is appropriate.

Rafael

Re: [PATCH 1/2] MMC: fix all hangs related to mmc/sd card insert/removal during suspend/resume.

[Maxim Levitsky]

On Mon, 2010-06-21 at 22:26 +0200, Rafael J. Wysocki wrote: 
> On Monday, June 21, 2010, Maxim Levitsky wrote:
> > On Mon, 2010-06-21 at 23:04 +0300, Adrian Hunter wrote: 
> > > ext Maxim Levitsky wrote:
> > > > If you don't use CONFIG_MMC_UNSAFE_RESUME, as soon as you attempt to
> > > > suspend, the card will be removed, therefore this patch doesn't change
> > > > the behavior of this option.
> > > > 
> > > > However the removal will be done by pm notifier, which runs while
> > > > userspace is still not frozen and thus can freely use del_gendisk,
> > > > without the risk of deadlock which would happen otherwise.
> > > > 
> > > > 
> > > > Card detect workqueue is now freezeable,
> > > > therefore if you do use CONFIG_MMC_UNSAFE_RESUME,
> > > > and remove the card during suspend, the removal will be
> > > > detected as soon as userspace is unfrozen, again at the moment
> > > > it is safe to call del_gendisk.
> > > > 
> > > > Tested with and without CONFIG_MMC_UNSAFE_RESUME with suspend and hibernate.
> > > > 
> > > > Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> > > > ---
> > > >  drivers/mmc/core/core.c  |   54 +++++++++++++++++++++++++++------------------
> > > >  drivers/mmc/core/host.c  |    6 +++++
> > > >  include/linux/mmc/host.h |    3 ++
> > > >  3 files changed, 41 insertions(+), 22 deletions(-)
> > > > 
> > > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > > > index 569e94d..0cba53a 100644
> > > > --- a/drivers/mmc/core/core.c
> > > > +++ b/drivers/mmc/core/core.c
> > > > @@ -1259,26 +1259,11 @@ int mmc_suspend_host(struct mmc_host *host)
> > > >  
> > > >  	if (host->caps & MMC_CAP_DISABLE)
> > > >  		cancel_delayed_work(&host->disable);
> > > > -	cancel_delayed_work(&host->detect);
> > > > -	mmc_flush_scheduled_work();
> > > >  
> > > >  	mmc_bus_get(host);
> > > >  	if (host->bus_ops && !host->bus_dead) {
> > > >  		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.
> > > > -			 */
> > > > -			if (host->bus_ops->remove)
> > > > -				host->bus_ops->remove(host);
> > > > -			mmc_claim_host(host);
> > > > -			mmc_detach_bus(host);
> > > > -			mmc_release_host(host);
> > > > -			host->pm_flags = 0;
> > > > -			err = 0;
> > > > -		}
> > > >  	}
> > > >  	mmc_bus_put(host);
> > > >  
> > > > @@ -1310,12 +1295,6 @@ int mmc_resume_host(struct mmc_host *host)
> > > >  			printk(KERN_WARNING "%s: error %d during resume "
> > > >  					    "(card was removed?)\n",
> > > >  					    mmc_hostname(host), err);
> > > > -			if (host->bus_ops->remove)
> > > > -				host->bus_ops->remove(host);
> > > > -			mmc_claim_host(host);
> > > > -			mmc_detach_bus(host);
> > > > -			mmc_release_host(host);
> > > > -			/* no need to bother upper layers */
> > > >  			err = 0;
> > > >  		}
> > > >  	}
> > > > @@ -1330,6 +1309,37 @@ int mmc_resume_host(struct mmc_host *host)
> > > >  	return err;
> > > >  }
> > > >  
> > > > +/* Do the card removal on suspend if card is assumed removeable
> > > > + * Do that in pm notifier while userspace isn't yet frozen, so we will be able
> > > > +   to sync the card.
> > > > +*/
> > > > +int mmc_pm_notify(struct notifier_block *notify_block,
> > > > +					unsigned long mode, void *unused)
> > > > +{
> > > > +	struct mmc_host *host = container_of(
> > > > +		notify_block, struct mmc_host, pm_notify);
> > > > +
> > > > +
> > > > +	switch (mode) {
> > > > +	case PM_HIBERNATION_PREPARE:
> > > > +	case PM_SUSPEND_PREPARE:
> > > > +
> > > > +		if (!host->bus_ops || host->bus_ops->suspend)
> > > > +			break;
> > > > +
> > > > +		if (host->bus_ops->remove)
> > > > +			host->bus_ops->remove(host);
> > > > +		mmc_claim_host(host);
> > > > +		mmc_detach_bus(host);
> > > > +		mmc_release_host(host);
> > > > +		host->pm_flags = 0;
> > > > +		break;
> > > 
> > > Is it possible that you receive PM_SUSPEND_PREPARE
> > > but there is no suspend and therefore no resume
> > > and therefore the card is removed but not detected
> > > again?
> > This is very good point.
> > The solution is to kick mmc detection thread from this notifier.
> > on resume.
> > I update the patch.
> > 
> > > 
> > > Is it possible that you are racing with kmmcd and the
> > > card is added after you receive PM_SUSPEND_PREPARE but
> > > before kmmcd is frozen?
> > This is unlikely but valid race.
> > I afraid I don't know nice way to solve it right now.
> > I can add some ad-hoc variable to tell interrupt handler not to kick the
> > detection workqueue after suspend notifier was called.
> > 
> > I wish there was a generic freeze_workqueue function.
> 
> There are freezable workqueues that are automatically frozen during suspend
> by the process freezer.  However, at the moment they need to be singlethread
> and I'm not sure if using one in this particular case is appropriate.

I *do* use freezable  work-queue.
However since this is pm notifier, it is called before userspace and the
workqueue is frozen.
Therefore I would like manually to freeze the workqueue from the pm
notifier.


Best regards,
Maxim Levitsky

Re: [PATCH 1/2] MMC: fix all hangs related to mmc/sd card insert/removal during suspend/resume.

[Rafael J. Wysocki]

On Tuesday, June 22, 2010, Maxim Levitsky wrote:
> On Mon, 2010-06-21 at 22:26 +0200, Rafael J. Wysocki wrote: 
> > On Monday, June 21, 2010, Maxim Levitsky wrote:
> > > On Mon, 2010-06-21 at 23:04 +0300, Adrian Hunter wrote: 
> > > > ext Maxim Levitsky wrote:
> > > > > If you don't use CONFIG_MMC_UNSAFE_RESUME, as soon as you attempt to
> > > > > suspend, the card will be removed, therefore this patch doesn't change
> > > > > the behavior of this option.
> > > > > 
> > > > > However the removal will be done by pm notifier, which runs while
> > > > > userspace is still not frozen and thus can freely use del_gendisk,
> > > > > without the risk of deadlock which would happen otherwise.
> > > > > 
> > > > > 
> > > > > Card detect workqueue is now freezeable,
> > > > > therefore if you do use CONFIG_MMC_UNSAFE_RESUME,
> > > > > and remove the card during suspend, the removal will be
> > > > > detected as soon as userspace is unfrozen, again at the moment
> > > > > it is safe to call del_gendisk.
> > > > > 
> > > > > Tested with and without CONFIG_MMC_UNSAFE_RESUME with suspend and hibernate.
> > > > > 
> > > > > Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> > > > > ---
> > > > >  drivers/mmc/core/core.c  |   54 +++++++++++++++++++++++++++------------------
> > > > >  drivers/mmc/core/host.c  |    6 +++++
> > > > >  include/linux/mmc/host.h |    3 ++
> > > > >  3 files changed, 41 insertions(+), 22 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > > > > index 569e94d..0cba53a 100644
> > > > > --- a/drivers/mmc/core/core.c
> > > > > +++ b/drivers/mmc/core/core.c
> > > > > @@ -1259,26 +1259,11 @@ int mmc_suspend_host(struct mmc_host *host)
> > > > >  
> > > > >  	if (host->caps & MMC_CAP_DISABLE)
> > > > >  		cancel_delayed_work(&host->disable);
> > > > > -	cancel_delayed_work(&host->detect);
> > > > > -	mmc_flush_scheduled_work();
> > > > >  
> > > > >  	mmc_bus_get(host);
> > > > >  	if (host->bus_ops && !host->bus_dead) {
> > > > >  		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.
> > > > > -			 */
> > > > > -			if (host->bus_ops->remove)
> > > > > -				host->bus_ops->remove(host);
> > > > > -			mmc_claim_host(host);
> > > > > -			mmc_detach_bus(host);
> > > > > -			mmc_release_host(host);
> > > > > -			host->pm_flags = 0;
> > > > > -			err = 0;
> > > > > -		}
> > > > >  	}
> > > > >  	mmc_bus_put(host);
> > > > >  
> > > > > @@ -1310,12 +1295,6 @@ int mmc_resume_host(struct mmc_host *host)
> > > > >  			printk(KERN_WARNING "%s: error %d during resume "
> > > > >  					    "(card was removed?)\n",
> > > > >  					    mmc_hostname(host), err);
> > > > > -			if (host->bus_ops->remove)
> > > > > -				host->bus_ops->remove(host);
> > > > > -			mmc_claim_host(host);
> > > > > -			mmc_detach_bus(host);
> > > > > -			mmc_release_host(host);
> > > > > -			/* no need to bother upper layers */
> > > > >  			err = 0;
> > > > >  		}
> > > > >  	}
> > > > > @@ -1330,6 +1309,37 @@ int mmc_resume_host(struct mmc_host *host)
> > > > >  	return err;
> > > > >  }
> > > > >  
> > > > > +/* Do the card removal on suspend if card is assumed removeable
> > > > > + * Do that in pm notifier while userspace isn't yet frozen, so we will be able
> > > > > +   to sync the card.
> > > > > +*/
> > > > > +int mmc_pm_notify(struct notifier_block *notify_block,
> > > > > +					unsigned long mode, void *unused)
> > > > > +{
> > > > > +	struct mmc_host *host = container_of(
> > > > > +		notify_block, struct mmc_host, pm_notify);
> > > > > +
> > > > > +
> > > > > +	switch (mode) {
> > > > > +	case PM_HIBERNATION_PREPARE:
> > > > > +	case PM_SUSPEND_PREPARE:
> > > > > +
> > > > > +		if (!host->bus_ops || host->bus_ops->suspend)
> > > > > +			break;
> > > > > +
> > > > > +		if (host->bus_ops->remove)
> > > > > +			host->bus_ops->remove(host);
> > > > > +		mmc_claim_host(host);
> > > > > +		mmc_detach_bus(host);
> > > > > +		mmc_release_host(host);
> > > > > +		host->pm_flags = 0;
> > > > > +		break;
> > > > 
> > > > Is it possible that you receive PM_SUSPEND_PREPARE
> > > > but there is no suspend and therefore no resume
> > > > and therefore the card is removed but not detected
> > > > again?
> > > This is very good point.
> > > The solution is to kick mmc detection thread from this notifier.
> > > on resume.
> > > I update the patch.
> > > 
> > > > 
> > > > Is it possible that you are racing with kmmcd and the
> > > > card is added after you receive PM_SUSPEND_PREPARE but
> > > > before kmmcd is frozen?
> > > This is unlikely but valid race.
> > > I afraid I don't know nice way to solve it right now.
> > > I can add some ad-hoc variable to tell interrupt handler not to kick the
> > > detection workqueue after suspend notifier was called.
> > > 
> > > I wish there was a generic freeze_workqueue function.
> > 
> > There are freezable workqueues that are automatically frozen during suspend
> > by the process freezer.  However, at the moment they need to be singlethread
> > and I'm not sure if using one in this particular case is appropriate.
> 
> I *do* use freezable  work-queue.

I overlooked that, sorry.

> However since this is pm notifier, it is called before userspace and the
> workqueue is frozen.
> Therefore I would like manually to freeze the workqueue from the pm
> notifier.

No, that won't work.  You need to find an alternative solution.  I guess you
may insert a work item that's going to sleep until a condition is
satisfied (analogous to a workqueue barrier) and wait for it to run.

Rafael

Re: [PATCH 1/2] MMC: fix all hangs related to mmc/sd card insert/removal during suspend/resume.

[Maxim Levitsky]

On Tue, 2010-06-22 at 11:19 +0200, Rafael J. Wysocki wrote: 
> On Tuesday, June 22, 2010, Maxim Levitsky wrote:
> > On Mon, 2010-06-21 at 22:26 +0200, Rafael J. Wysocki wrote: 
> > > On Monday, June 21, 2010, Maxim Levitsky wrote:
> > > > On Mon, 2010-06-21 at 23:04 +0300, Adrian Hunter wrote: 
> > > > > ext Maxim Levitsky wrote:
> > > > > > If you don't use CONFIG_MMC_UNSAFE_RESUME, as soon as you attempt to
> > > > > > suspend, the card will be removed, therefore this patch doesn't change
> > > > > > the behavior of this option.
> > > > > > 
> > > > > > However the removal will be done by pm notifier, which runs while
> > > > > > userspace is still not frozen and thus can freely use del_gendisk,
> > > > > > without the risk of deadlock which would happen otherwise.
> > > > > > 
> > > > > > 
> > > > > > Card detect workqueue is now freezeable,
> > > > > > therefore if you do use CONFIG_MMC_UNSAFE_RESUME,
> > > > > > and remove the card during suspend, the removal will be
> > > > > > detected as soon as userspace is unfrozen, again at the moment
> > > > > > it is safe to call del_gendisk.
> > > > > > 
> > > > > > Tested with and without CONFIG_MMC_UNSAFE_RESUME with suspend and hibernate.
> > > > > > 
> > > > > > Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> > > > > > ---
> > > > > >  drivers/mmc/core/core.c  |   54 +++++++++++++++++++++++++++------------------
> > > > > >  drivers/mmc/core/host.c  |    6 +++++
> > > > > >  include/linux/mmc/host.h |    3 ++
> > > > > >  3 files changed, 41 insertions(+), 22 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > > > > > index 569e94d..0cba53a 100644
> > > > > > --- a/drivers/mmc/core/core.c
> > > > > > +++ b/drivers/mmc/core/core.c
> > > > > > @@ -1259,26 +1259,11 @@ int mmc_suspend_host(struct mmc_host *host)
> > > > > >  
> > > > > >  	if (host->caps & MMC_CAP_DISABLE)
> > > > > >  		cancel_delayed_work(&host->disable);
> > > > > > -	cancel_delayed_work(&host->detect);
> > > > > > -	mmc_flush_scheduled_work();
> > > > > >  
> > > > > >  	mmc_bus_get(host);
> > > > > >  	if (host->bus_ops && !host->bus_dead) {
> > > > > >  		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.
> > > > > > -			 */
> > > > > > -			if (host->bus_ops->remove)
> > > > > > -				host->bus_ops->remove(host);
> > > > > > -			mmc_claim_host(host);
> > > > > > -			mmc_detach_bus(host);
> > > > > > -			mmc_release_host(host);
> > > > > > -			host->pm_flags = 0;
> > > > > > -			err = 0;
> > > > > > -		}
> > > > > >  	}
> > > > > >  	mmc_bus_put(host);
> > > > > >  
> > > > > > @@ -1310,12 +1295,6 @@ int mmc_resume_host(struct mmc_host *host)
> > > > > >  			printk(KERN_WARNING "%s: error %d during resume "
> > > > > >  					    "(card was removed?)\n",
> > > > > >  					    mmc_hostname(host), err);
> > > > > > -			if (host->bus_ops->remove)
> > > > > > -				host->bus_ops->remove(host);
> > > > > > -			mmc_claim_host(host);
> > > > > > -			mmc_detach_bus(host);
> > > > > > -			mmc_release_host(host);
> > > > > > -			/* no need to bother upper layers */
> > > > > >  			err = 0;
> > > > > >  		}
> > > > > >  	}
> > > > > > @@ -1330,6 +1309,37 @@ int mmc_resume_host(struct mmc_host *host)
> > > > > >  	return err;
> > > > > >  }
> > > > > >  
> > > > > > +/* Do the card removal on suspend if card is assumed removeable
> > > > > > + * Do that in pm notifier while userspace isn't yet frozen, so we will be able
> > > > > > +   to sync the card.
> > > > > > +*/
> > > > > > +int mmc_pm_notify(struct notifier_block *notify_block,
> > > > > > +					unsigned long mode, void *unused)
> > > > > > +{
> > > > > > +	struct mmc_host *host = container_of(
> > > > > > +		notify_block, struct mmc_host, pm_notify);
> > > > > > +
> > > > > > +
> > > > > > +	switch (mode) {
> > > > > > +	case PM_HIBERNATION_PREPARE:
> > > > > > +	case PM_SUSPEND_PREPARE:
> > > > > > +
> > > > > > +		if (!host->bus_ops || host->bus_ops->suspend)
> > > > > > +			break;
> > > > > > +
> > > > > > +		if (host->bus_ops->remove)
> > > > > > +			host->bus_ops->remove(host);
> > > > > > +		mmc_claim_host(host);
> > > > > > +		mmc_detach_bus(host);
> > > > > > +		mmc_release_host(host);
> > > > > > +		host->pm_flags = 0;
> > > > > > +		break;
> > > > > 
> > > > > Is it possible that you receive PM_SUSPEND_PREPARE
> > > > > but there is no suspend and therefore no resume
> > > > > and therefore the card is removed but not detected
> > > > > again?
> > > > This is very good point.
> > > > The solution is to kick mmc detection thread from this notifier.
> > > > on resume.
> > > > I update the patch.
> > > > 
> > > > > 
> > > > > Is it possible that you are racing with kmmcd and the
> > > > > card is added after you receive PM_SUSPEND_PREPARE but
> > > > > before kmmcd is frozen?
> > > > This is unlikely but valid race.
> > > > I afraid I don't know nice way to solve it right now.
> > > > I can add some ad-hoc variable to tell interrupt handler not to kick the
> > > > detection workqueue after suspend notifier was called.
> > > > 
> > > > I wish there was a generic freeze_workqueue function.
> > > 
> > > There are freezable workqueues that are automatically frozen during suspend
> > > by the process freezer.  However, at the moment they need to be singlethread
> > > and I'm not sure if using one in this particular case is appropriate.
> > 
> > I *do* use freezable  work-queue.
> 
> I overlooked that, sorry.
> 
> > However since this is pm notifier, it is called before userspace and the
> > workqueue is frozen.
> > Therefore I would like manually to freeze the workqueue from the pm
> > notifier.
> 
> No, that won't work.  You need to find an alternative solution.  I guess you
> may insert a work item that's going to sleep until a condition is
> satisfied (analogous to a workqueue barrier) and wait for it to 
This screams to be done in generic way.
Something like suspend_workqueue() and resume_workqueue();

In addition to that I just found that .suspend function sometimes can
return -ENOSYS, which triggers card removal. I wrongly remove that chunk
of code.

To make the thing picture perfect I would have to invest more time to
it, I will do so as soon as I finish my exams.

Meanwhile the current patch already fixes all but corner cases or rather
nasty hang on suspend with any MMC/SD card inserted.


Best regards,
Maxim Levitsky

Re: [PATCH 1/2] MMC: fix all hangs related to mmc/sd card insert/removal during suspend/resume.

[Rafael J. Wysocki]

On Tuesday, June 22, 2010, Maxim Levitsky wrote:
> On Tue, 2010-06-22 at 11:19 +0200, Rafael J. Wysocki wrote: 
> > On Tuesday, June 22, 2010, Maxim Levitsky wrote:
> > > On Mon, 2010-06-21 at 22:26 +0200, Rafael J. Wysocki wrote: 
...
> > > I *do* use freezable  work-queue.
> > 
> > I overlooked that, sorry.
> > 
> > > However since this is pm notifier, it is called before userspace and the
> > > workqueue is frozen.
> > > Therefore I would like manually to freeze the workqueue from the pm
> > > notifier.
> > 
> > No, that won't work.  You need to find an alternative solution.  I guess you
> > may insert a work item that's going to sleep until a condition is
> > satisfied (analogous to a workqueue barrier) and wait for it to 
> This screams to be done in generic way.
> Something like suspend_workqueue() and resume_workqueue();

Well, there was no need for that until now. :-)

> In addition to that I just found that .suspend function sometimes can
> return -ENOSYS, which triggers card removal. I wrongly remove that chunk
> of code.
> 
> To make the thing picture perfect I would have to invest more time to
> it, I will do so as soon as I finish my exams.
> 
> Meanwhile the current patch already fixes all but corner cases or rather
> nasty hang on suspend with any MMC/SD card inserted.

OK

I think Andrew has already taken [2/2].

Andrew, who's maintaining MMC now?

Rafael

Re: [PATCH 1/2] MMC: fix all hangs related to mmc/sd card insert/removal during suspend/resume.

[Andrew Morton]

On Tue, 22 Jun 2010 23:53:21 +0200
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> On Tuesday, June 22, 2010, Maxim Levitsky wrote:
> > On Tue, 2010-06-22 at 11:19 +0200, Rafael J. Wysocki wrote: 
> > > On Tuesday, June 22, 2010, Maxim Levitsky wrote:
> > > > On Mon, 2010-06-21 at 22:26 +0200, Rafael J. Wysocki wrote: 
> ...
> > > > I *do* use freezable  work-queue.
> > > 
> > > I overlooked that, sorry.
> > > 
> > > > However since this is pm notifier, it is called before userspace and the
> > > > workqueue is frozen.
> > > > Therefore I would like manually to freeze the workqueue from the pm
> > > > notifier.
> > > 
> > > No, that won't work.  You need to find an alternative solution.  I guess you
> > > may insert a work item that's going to sleep until a condition is
> > > satisfied (analogous to a workqueue barrier) and wait for it to 
> > This screams to be done in generic way.
> > Something like suspend_workqueue() and resume_workqueue();
> 
> Well, there was no need for that until now. :-)
> 
> > In addition to that I just found that .suspend function sometimes can
> > return -ENOSYS, which triggers card removal. I wrongly remove that chunk
> > of code.
> > 
> > To make the thing picture perfect I would have to invest more time to
> > it, I will do so as soon as I finish my exams.
> > 
> > Meanwhile the current patch already fixes all but corner cases or rather
> > nasty hang on suspend with any MMC/SD card inserted.
> 
> OK
> 
> I think Andrew has already taken [2/2].

I took them both, but I need to come back to this discussion to work
out what to do with them now.

> Andrew, who's maintaining MMC now?

Pierre stopped doing it, so I'm now pretending to.

I actually pretend to maintain a huge number of subsystems and should
sprinkle akpms all over MAINTAINERS so stuff doesn't get lost.

Re: [PATCH 1/2] MMC: fix all hangs related to mmc/sd card insert/removal during suspend/resume.

[Rafael J. Wysocki]

On Wednesday, June 23, 2010, Andrew Morton wrote:
> On Tue, 22 Jun 2010 23:53:21 +0200
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> > On Tuesday, June 22, 2010, Maxim Levitsky wrote:
> > > On Tue, 2010-06-22 at 11:19 +0200, Rafael J. Wysocki wrote: 
> > > > On Tuesday, June 22, 2010, Maxim Levitsky wrote:
> > > > > On Mon, 2010-06-21 at 22:26 +0200, Rafael J. Wysocki wrote: 
> > ...
> > > > > I *do* use freezable  work-queue.
> > > > 
> > > > I overlooked that, sorry.
> > > > 
> > > > > However since this is pm notifier, it is called before userspace and the
> > > > > workqueue is frozen.
> > > > > Therefore I would like manually to freeze the workqueue from the pm
> > > > > notifier.
> > > > 
> > > > No, that won't work.  You need to find an alternative solution.  I guess you
> > > > may insert a work item that's going to sleep until a condition is
> > > > satisfied (analogous to a workqueue barrier) and wait for it to 
> > > This screams to be done in generic way.
> > > Something like suspend_workqueue() and resume_workqueue();
> > 
> > Well, there was no need for that until now. :-)
> > 
> > > In addition to that I just found that .suspend function sometimes can
> > > return -ENOSYS, which triggers card removal. I wrongly remove that chunk
> > > of code.
> > > 
> > > To make the thing picture perfect I would have to invest more time to
> > > it, I will do so as soon as I finish my exams.
> > > 
> > > Meanwhile the current patch already fixes all but corner cases or rather
> > > nasty hang on suspend with any MMC/SD card inserted.
> > 
> > OK
> > 
> > I think Andrew has already taken [2/2].
> 
> I took them both, but I need to come back to this discussion to work
> out what to do with them now.

I think they are worth merging.  At least they shouldn't break things and
Maxim has already promised to clean up that code in future.

> > Andrew, who's maintaining MMC now?
> 
> Pierre stopped doing it, so I'm now pretending to.
> 
> I actually pretend to maintain a huge number of subsystems and should
> sprinkle akpms all over MAINTAINERS so stuff doesn't get lost.

I see.

If you have anything PM-related, I can handle that too.

MMC tree (Was: Re: [PATCH 1/2] MMC: fix all hangs related to mmc/sd card insert/removal during suspend/resume.)

[Stephen Rothwell]

[-- Attachment #1.1: Type: text/plain, Size: 702 bytes --]

Hi Andrew,

On Tue, 22 Jun 2010 15:20:06 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Tue, 22 Jun 2010 23:53:21 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> >
> > Andrew, who's maintaining MMC now?
> 
> Pierre stopped doing it, so I'm now pretending to.

So I guess I should remove the mmc tree (currently empty) from
linux-next, then?

> I actually pretend to maintain a huge number of subsystems and should
> sprinkle akpms all over MAINTAINERS so stuff doesn't get lost.

How about splitting these subsystems out of -mm and adding them to
linux-next?

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #1.2: Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: MMC tree (Was: Re: [PATCH 1/2] MMC: fix all hangs related to mmc/sd card insert/removal during suspend/resume.)

[Andrew Morton]

On Wed, 23 Jun 2010 13:08:37 +1000 Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> Hi Andrew,
> 
> On Tue, 22 Jun 2010 15:20:06 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Tue, 22 Jun 2010 23:53:21 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > >
> > > Andrew, who's maintaining MMC now?
> > 
> > Pierre stopped doing it, so I'm now pretending to.
> 
> So I guess I should remove the mmc tree (currently empty) from
> linux-next, then?

yup.

> > I actually pretend to maintain a huge number of subsystems and should
> > sprinkle akpms all over MAINTAINERS so stuff doesn't get lost.
> 
> How about splitting these subsystems out of -mm and adding them to
> linux-next?

Sigh.  Need to get onto that.  I suppose you'd prefer something that
actually compiles a bit, too.