[Lucid] pull-request: mmc: fix all hangs related to mmc/sd card insert/removal during suspend/resume
Lee Jones
lee.jones at canonical.com
Fri Sep 3 14:40:02 UTC 2010
On 03/09/10 13:48, Stefan Bader wrote:
> There are several places that do not really feel like related to the issue at hand.
>
> On 09/03/2010 02:13 PM, Lee Jones wrote:
>> This patch fixes:
>>
>> BugLink: http://bugs.launchpad.net/bugs/477106
>>
>> The following changes since commit 5470af5fd104192214b496a2736fd26200f4650d:
>> Steve Conklin (1):
>> UBUNTU: [Config] updateconfigs
>>
>> are available in the git repository at:
>>
>> git://kernel.ubuntu.com/lag/ubuntu-lucid.git lp477106
>>
>> Lee Jones (1):
>> mmc: fix all hangs related to mmc/sd card insert/removal during suspend/resume
>>
>> drivers/mmc/core/core.c | 87 ++++++++++++++++++++++++++++++++--------------
>> drivers/mmc/core/host.c | 5 +++
>> include/linux/mmc/host.h | 6 +++
>> include/linux/mmc/pm.h | 30 ++++++++++++++++
>> 4 files changed, 102 insertions(+), 26 deletions(-)
>> create mode 100644 include/linux/mmc/pm.h
>>
>> ------------------------------------------------------------------------
>>
>> commit f8a01342978bcf0749b4ca27268deb47d3559261
>> Author: Lee Jones <lee.jones at canonical.com>
>> Date: Fri Sep 3 12:42:15 2010 +0100
>>
>> mmc: fix all hangs related to mmc/sd card insert/removal during suspend/resume
>>
>> Based on a patch-set originally written by 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 disabled while userspace is frozen, 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.
>>
>> [akpm at linux-foundation.org: clean up function prototype]
>> [akpm at linux-foundation.org: fix CONFIG_PM-n linkage, small cleanups]
>> [akpm at linux-foundation.org: coding-style fixes]
>> Signed-off-by: Maxim Levitsky <maximlevitsky at gmail.com>
>> Cc: David Brownell <david-b at pacbell.net>
>> Cc: Alan Stern <stern at rowland.harvard.edu>
>> Cc: <linux-mmc at vger.kernel.org>
>> Signed-off-by: Andrew Morton <akpm at linux-foundation.org>
>> Signed-off-by: Linus Torvalds <torvalds at linux-foundation.org>
>>
>> [Backported to Ubuntu-2.6.32-25.43 by Lee Jones]
>> Signed-off-by: Lee Jones <lee.jones at canonical.com>
>>
>> BugLink: http://bugs.launchpad.net/bugs/477106
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 30acd52..2479abe 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -106,8 +106,9 @@ void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
>> cmd->error = 0;
>> host->ops->request(host, mrq);
>> } else {
>> +#ifdef CONFIG_LEDS_TRIGGERS
>> led_trigger_event(host->led, LED_OFF);
>> -
>> +#endif
>
> These #ifdefs maybe beneficial but it feels like those would address some other
> issue...
In include/linux/mmc/host.h these lines appear:
#ifdef CONFIG_LEDS_TRIGGERS
struct led_trigger *led; /* activity led */
#endif
It seems silly to protect the declaration and not the use of the variable.
>> pr_debug("%s: req done (CMD%u): %d: %08x %08x %08x %08x\n",
>> mmc_hostname(host), cmd->opcode, err,
>> cmd->resp[0], cmd->resp[1],
>> @@ -163,7 +164,9 @@ mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
>>
>> WARN_ON(!host->claimed);
>>
>> +#ifdef CONFIG_LEDS_TRIGGERS
>> led_trigger_event(host->led, LED_FULL);
>> +#endif
>>
>> mrq->cmd->error = 0;
>> mrq->cmd->mrq = mrq;
>> @@ -1057,6 +1060,17 @@ void mmc_rescan(struct work_struct *work)
>> container_of(work, struct mmc_host, detect.work);
>> u32 ocr;
>> int err;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&host->lock, flags);
>> +
>> + if (host->rescan_disable) {
>> + spin_unlock_irqrestore(&host->lock, flags);
>> + return;
>> + }
>> +
>> + spin_unlock_irqrestore(&host->lock, flags);
>> +
>>
>> mmc_bus_get(host);
>>
>> @@ -1263,18 +1277,6 @@ int mmc_suspend_host(struct mmc_host *host, pm_message_t state)
>> 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);
>> - err = 0;
>> - }
>> }
>> mmc_bus_put(host);
>>
>> @@ -1304,28 +1306,61 @@ 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;
>> }
>> }
>> mmc_bus_put(host);
>>
>> - /*
>> - * We add a slight delay here so that resume can progress
>> - * in parallel.
>> - */
>> - mmc_detect_change(host, 1);
>> -
>> return err;
>> }
>> -
>> EXPORT_SYMBOL(mmc_resume_host);
>>
>> +/* 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);
>> + unsigned long flags;
>> +
>> +
>> + switch (mode) {
>> + case PM_HIBERNATION_PREPARE:
>> + case PM_SUSPEND_PREPARE:
>> +
>> + spin_lock_irqsave(&host->lock, flags);
>> + host->rescan_disable = 1;
>> + spin_unlock_irqrestore(&host->lock, flags);
>> + cancel_delayed_work_sync(&host->detect);
>> +
>> + if (!host->bus_ops || host->bus_ops->suspend)
>> + break;
>> +
>> + mmc_claim_host(host);
>> +
>> + if (host->bus_ops->remove)
>> + host->bus_ops->remove(host);
>> +
>> + mmc_detach_bus(host);
>> + mmc_release_host(host);
>> + host->pm_flags = 0;
>
> The line above seems to be the only reason you pulled in the new header file.
> But then this variable is not used at all and maybe it would be better just to
> drop that from the backport.
I am going to redo the pull-request.
The new pull-request will contain bring this patch-set more in line with the original code.
Although this will mean more code rather than less, it will be semantically closer to the most up-to-date code.
>
>> + break;
>> +
>> + case PM_POST_SUSPEND:
>> + case PM_POST_HIBERNATION:
>> +
>> + spin_lock_irqsave(&host->lock, flags);
>> + host->rescan_disable = 0;
>> + spin_unlock_irqrestore(&host->lock, flags);
>> + mmc_detect_change(host, 0);
>> +
>> + }
>> +
>> + return 0;
>> +}
>> #endif
>>
>> static int __init mmc_init(void)
>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
>> index a268d12..0efe631 100644
>> --- a/drivers/mmc/core/host.c
>> +++ b/drivers/mmc/core/host.c
>> @@ -16,6 +16,8 @@
>> #include <linux/idr.h>
>> #include <linux/pagemap.h>
>> #include <linux/leds.h>
>> +#include <linux/slab.h>
>> +#include <linux/suspend.h>
>>
>> #include <linux/mmc/host.h>
>>
>> @@ -84,6 +86,7 @@ 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.
>> @@ -132,6 +135,7 @@ int mmc_add_host(struct mmc_host *host)
>> #endif
>>
>> mmc_start_host(host);
>> + register_pm_notifier(&host->pm_notify);
>>
>> return 0;
>> }
>> @@ -148,6 +152,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
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index eaf3636..c43bc21 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -14,6 +14,7 @@
>> #include <linux/sched.h>
>>
>> #include <linux/mmc/core.h>
>> +#include <linux/mmc/pm.h>
>
> see above.
>
>>
>> struct mmc_ios {
>> unsigned int clock; /* clock rate */
>> @@ -120,6 +121,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 */
>> @@ -177,6 +179,7 @@ struct mmc_host {
>>
>> /* Only used with MMC_CAP_DISABLE */
>> int enabled; /* host is enabled */
>> + int rescan_disable; /* disable card detection */
>> int nesting_cnt; /* "enable" nesting count */
>> int en_dis_recurs; /* detect recursion */
>> unsigned int disable_delay; /* disable delay in msecs */
>> @@ -197,6 +200,8 @@ struct mmc_host {
>> struct task_struct *sdio_irq_thread;
>> atomic_t sdio_irq_thread_abort;
>>
>> + mmc_pm_flag_t pm_flags; /* requested pm features */
>> +
>
> see above.
>
>> #ifdef CONFIG_LEDS_TRIGGERS
>> struct led_trigger *led; /* activity led */
>> #endif
>> @@ -249,6 +254,7 @@ 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)
>
> This whole include file does not seem to be really required.
>
>> diff --git a/include/linux/mmc/pm.h b/include/linux/mmc/pm.h
>> new file mode 100644
>> index 0000000..d37aac4
>> --- /dev/null
>> +++ b/include/linux/mmc/pm.h
>> @@ -0,0 +1,30 @@
>> +/*
>> + * linux/include/linux/mmc/pm.h
>> + *
>> + * Author: Nicolas Pitre
>> + * Copyright: (C) 2009 Marvell Technology Group Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#ifndef LINUX_MMC_PM_H
>> +#define LINUX_MMC_PM_H
>> +
>> +/*
>> + * These flags are used to describe power management features that
>> + * some cards (typically SDIO cards) might wish to benefit from when
>> + * the host system is being suspended. There are several layers of
>> + * abstractions involved, from the host controller driver, to the MMC core
>> + * code, to the SDIO core code, to finally get to the actual SDIO function
>> + * driver. This file is therefore used for common definitions shared across
>> + * all those layers.
>> + */
>> +
>> +typedef unsigned int mmc_pm_flag_t;
>> +
>> +#define MMC_PM_KEEP_POWER (1 << 0) /* preserve card power during suspend */
>> +#define MMC_PM_WAKE_SDIO_IRQ (1 << 1) /* wake up host system on SDIO IRQ assertion */
>> +
>> +#endif
>>
>
More information about the kernel-team
mailing list