On 2010-11-18, 23:36 +0000, Seibel, Eric wrote:
> From 5d9f15f00ea02f46385857ba7fb5a6db8741fca0 Mon Sep 17 00:00:00 2001
> From: Pierre Tardy <[email protected]>
> Date: Fri, 12 Nov 2010 17:37:48 +0100
> Subject: [PATCH 1/6] sdhci: allow platform to trigger regulator detection
>
> One some platforms, the regulators associated to the mmc are available
> late in the boot.
> There might be race condition between the time gpio expander controlling the
> mmc regulator is probed and sdhci is probed.
>
> sdhci_pci_request_regulators() is called by platform code at a time
> where it is known that the regulator is available to retry getting
> the regulators associated to pci sdhcis
Hi,
This patch is bringing race condition for mmc_power_up() and
mmc_power_off()... i.e.:
[ 8.027030] ------------[ cut here ]------------
[ 8.027030] kernel BUG at drivers/mmc/host/sdhci.c:1121!
[ 8.027030] invalid opcode: 0000 [#1] PREEMPT SMP
[ 8.027030] last sysfs file:
[ 8.027030] Modules linked in:
[ 8.027030]
[ 8.027030] Pid: 26, comm: kmmcd Not tainted 2.6.35.3+ #6 To be filled by
O.E.M./OakTrail platform
[ 8.027030] EIP: 0060:[<c1377d8b>] EFLAGS: 00010083 CPU: 1
[ 8.027030] EIP is at sdhci_set_power+0x4b/0xb9
[ 8.027030] EAX: 00000001 EBX: f6f03620 ECX: 00000000 EDX: 00000000
[ 8.027030] ESI: f6f03620 EDI: f6f036d8 EBP: f75cbef0 ESP: f75cbee8
[ 8.027030] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
[ 8.027030] Process kmmcd (pid: 26, ti=f75ca000 task=f75c86b0
task.ti=f75ca000)
[ 8.027030] Stack:
[ 8.027030] f6f03620 f6f034e4 f75cbf08 c1377e7f 00000282 f6f034e4 f6f032a0
f75cbf30
[ 8.027030] <0> f75cbf18 c136ef51 f6f032a0 00000000 f75cbf40 c136fafc
00061a80 000493e0
[ 8.027030] <0> 00030d40 000186a0 f75cbf8c f75cbf8c f75c86b0 f75cbf98
f75cbfa4 c104a382
[ 8.027030] Call Trace:
[ 8.027030] [<c1377e7f>] ? sdhci_set_ios+0x86/0x10d
[ 8.027030] [<c136ef51>] ? mmc_power_up+0xa9/0xb7
[ 8.027030] [<c136fafc>] ? mmc_rescan+0x1e2/0x2b3
[ 8.027030] [<c104a382>] ? worker_thread+0x211/0x30a
[ 8.027030] [<c104a338>] ? worker_thread+0x1c7/0x30a
[ 8.027030] [<c136f91a>] ? mmc_rescan+0x0/0x2b3
[ 8.027030] [<c104db27>] ? autoremove_wake_function+0x0/0x33
[ 8.027030] [<c104a171>] ? worker_thread+0x0/0x30a
[ 8.027030] [<c104d6d5>] ? kthread+0x61/0x66
[ 8.027030] [<c104d674>] ? kthread+0x0/0x66
[ 8.027030] [<c1002d7a>] ? kernel_thread_helper+0x6/0x1a
[ 8.027030] Code: 80 00 00 00 74 1d 3d 00 00 02 00 75 1e eb 10 3d 00 00 10
00 74 11 3d 00 00 20 00 75 0e eb 08 b1 0c eb 0e b1 0a eb 0a b1 0e eb 06 <0f> 0b
eb fe 31 c9 38 8e e8 00 00 00 74 5c 84 c9 88 8e e8 00 00
[ 8.027030] EIP: [<c1377d8b>] sdhci_set_power+0x4b/0xb9 SS:ESP 0068:f75cbee8
[ 8.027030] ---[ end trace 79d81671aeab5327 ]---
> +void sdhci_try_get_regulator(struct sdhci_host *host)
> +{
> + struct regulator *vmmc;
> + unsigned long flags;
> + if (!host->vmmc) {
> + vmmc = regulator_get(mmc_dev(host->mmc), "vmmc");
> + if (!IS_ERR(vmmc)) {
> + spin_lock_irqsave(&host->lock, flags);
> + if (!host->vmmc) {
> + host->vmmc = vmmc;
> + spin_unlock_irqrestore(&host->lock, flags);
> + regulator_enable(host->vmmc);
> + mmc_detect_change(host->mmc, 0);
> + } else { /* race! we got the regulator twice */
> + spin_unlock_irqrestore(&host->lock, flags);
> + regulator_put(vmmc);
> + }
> + }
> + }
> +}
> +EXPORT_SYMBOL_GPL(sdhci_try_get_regulator);
> int sdhci_add_host(struct sdhci_host *host)
> {
> struct mmc_host *mmc;
> @@ -2049,14 +2070,7 @@ int sdhci_add_host(struct sdhci_host *host)
> mmc_hostname(mmc), host);
> if (ret)
> goto untasklet;
> -
> - host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");
> - if (IS_ERR(host->vmmc)) {
> - printk(KERN_INFO "%s: no vmmc regulator found\n",
> mmc_hostname(mmc));
> - host->vmmc = NULL;
> - } else {
> - regulator_enable(host->vmmc);
> - }
> + sdhci_try_get_regulator(host);
My analysis is that when you try to do sdhci_try_get_regulator(host)
in sdhci_add_host(), it will trigger mmc_detect_change(), and then
mmc_rescan(), then mmc_power_up()...
At the same time, driver initialise code continues run into
mmc_start_host(), and then mmc_power_off().
These two paths is in a race condition. Once you do a mmc_power_off()
while mmc_power_up() is running, the BUG() as showed above will be
triggered.
About the patch itself, I don't understand why the change chunk in
sdhci_add_host() is needed?
A good news is that this patch had already been removed from MeeGo
devel:kernel by Arjan. Sorry Seibel :-p
Kangkai
_______________________________________________
MeeGo-kernel mailing list
[email protected]
http://lists.meego.com/listinfo/meego-kernel
