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
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Maxim Levitsky <[email protected]>
Acked-by: Philip Langdale <[email protected]>
---
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
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
On Mon, 21 Jun 2010 22:21:44 +0300
Maxim Levitsky <[email protected]> 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.
On Mon, 2010-06-21 at 12:39 -0700, Andrew Morton wrote:
> On Mon, 21 Jun 2010 22:21:44 +0300
> Maxim Levitsky <[email protected]> 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
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 <[email protected]>
> ---
> 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)
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > > ---
> > > 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
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 <[email protected]>
> > > > ---
> > > > 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
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 <[email protected]>
> > > > > ---
> > > > > 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
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 <[email protected]>
> > > > > > ---
> > > > > > 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
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
On Tue, 22 Jun 2010 23:53:21 +0200
"Rafael J. Wysocki" <[email protected]> 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.
On Wednesday, June 23, 2010, Andrew Morton wrote:
> On Tue, 22 Jun 2010 23:53:21 +0200
> "Rafael J. Wysocki" <[email protected]> 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.
Hi Andrew,
On Tue, 22 Jun 2010 15:20:06 -0700 Andrew Morton <[email protected]> wrote:
>
> On Tue, 22 Jun 2010 23:53:21 +0200 "Rafael J. Wysocki" <[email protected]> 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 [email protected]
http://www.canb.auug.org.au/~sfr/
On Wed, 23 Jun 2010 13:08:37 +1000 Stephen Rothwell <[email protected]> wrote:
> Hi Andrew,
>
> On Tue, 22 Jun 2010 15:20:06 -0700 Andrew Morton <[email protected]> wrote:
> >
> > On Tue, 22 Jun 2010 23:53:21 +0200 "Rafael J. Wysocki" <[email protected]> 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.
This fixes a build breakage introduced by
4c2ef25 (mmc: fix all hangs related to mmc/sd card insert/removal during suspend/resume)
Cc: Maxim Levitsky <[email protected]>
Cc: David Brownell <[email protected]>
Cc: Alan Stern <[email protected]>
Cc: [email protected]
Cc: Andrew Morton <[email protected]>
Signed-off-by: Uwe Kleine-König <[email protected]>
---
drivers/mmc/core/host.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 0efe631..d80cfdc 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -86,7 +86,9 @@ 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);
+#ifdef CONFIG_PM
host->pm_notify.notifier_call = mmc_pm_notify;
+#endif
/*
* By default, hosts do not support SGIO or large requests.
--
1.7.1
On Fri, 2010-08-13 at 11:24 +0200, Uwe Kleine-König wrote:
> This fixes a build breakage introduced by
>
> 4c2ef25 (mmc: fix all hangs related to mmc/sd card insert/removal during suspend/resume)
>
> Cc: Maxim Levitsky <[email protected]>
> Cc: David Brownell <[email protected]>
> Cc: Alan Stern <[email protected]>
> Cc: [email protected]
> Cc: Andrew Morton <[email protected]>
> Signed-off-by: Uwe Kleine-König <[email protected]>
> ---
> drivers/mmc/core/host.c | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 0efe631..d80cfdc 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -86,7 +86,9 @@ 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);
> +#ifdef CONFIG_PM
> host->pm_notify.notifier_call = mmc_pm_notify;
> +#endif
>
> /*
> * By default, hosts do not support SGIO or large requests.
Hi,
Sorry about that!
Best regards,
Maxim Levitsky
Uwe Kleine-Konig wrote:
>
> This fixes a build breakage introduced by
>
> 4c2ef25 (mmc: fix all hangs related to mmc/sd card insert/removal during
> suspend/resume)
>
> Cc: Maxim Levitsky <[email protected]>
> Cc: David Brownell <[email protected]>
> Cc: Alan Stern <[email protected]>
> Cc: [email protected]
> Cc: Andrew Morton <[email protected]>
> Signed-off-by: Uwe Kleine-König <[email protected]>
Acked-by: Kukjin Kim <[email protected]>
> ---
> drivers/mmc/core/host.c | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 0efe631..d80cfdc 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -86,7 +86,9 @@ 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);
> +#ifdef CONFIG_PM
> host->pm_notify.notifier_call = mmc_pm_notify;
> +#endif
>
> /*
> * By default, hosts do not support SGIO or large requests.
> --
> 1.7.1
>
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <[email protected]>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
On Fri, 2010-08-13 at 13:01 +0300, Maxim Levitsky wrote:
> On Fri, 2010-08-13 at 11:24 +0200, Uwe Kleine-König wrote:
> > This fixes a build breakage introduced by
> >
> > 4c2ef25 (mmc: fix all hangs related to mmc/sd card insert/removal during suspend/resume)
> >
> > Cc: Maxim Levitsky <[email protected]>
> > Cc: David Brownell <[email protected]>
> > Cc: Alan Stern <[email protected]>
> > Cc: [email protected]
> > Cc: Andrew Morton <[email protected]>
> > Signed-off-by: Uwe Kleine-König <[email protected]>
> > ---
> > drivers/mmc/core/host.c | 2 ++
> > 1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> > index 0efe631..d80cfdc 100644
> > --- a/drivers/mmc/core/host.c
> > +++ b/drivers/mmc/core/host.c
> > @@ -86,7 +86,9 @@ 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);
> > +#ifdef CONFIG_PM
> > host->pm_notify.notifier_call = mmc_pm_notify;
> > +#endif
> >
> > /*
> > * By default, hosts do not support SGIO or large requests.
>
> Hi,
>
> Sorry about that!
Hi folks,
Sorry again for this bug.
>From now on I will test compilation rigorously.
If you need it,
Acked-by: Maxim Levitsky <[email protected]>
Best regards,
Maxim Levitsky