BUG/PATCH? mmcblk devices don't suspend properly.

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 6021 bytes --]


I've been experimenting with removing the call to sys_sync() in
enter_state().  For me (with verbose debugging and syslog running)
this causes a noticeable delay when entering suspend.

Removing it should not affect correctness as there is no locking the ensure
that no other process writes out data immediately after the 'sync'
completed.  But it could make existing bugs more obvious - in fact it does :-)

The device I am doing this on is using a micro-SD card for storage.
The card cannot be removed without removing the battery, so
MMC_CAP_NONREMOVABLE is set for the mmc slot (which seems necessary for
safely having '/' there).

Since removing the sys_sync() call I've notices a number of problems with
suspend/resume, the most obvious being that resume blocks in mmc_claim_host:

[  263.585754] susman          D c046455c  5604  1086   1084 0x00000000
[  263.592498] [<c046455c>] (__schedule+0x584/0x614) from [<c0302630>] (__mmc_claim_host+0xb8/0x154)
[  263.601806] [<c0302630>] (__mmc_claim_host+0xb8/0x154) from [<c0308640>] (mmc_sd_resume+0x34/0x5c)
[  263.611206] [<c0308640>] (mmc_sd_resume+0x34/0x5c) from [<c0301ce4>] (mmc_resume_host+0xc8/0x15c)
[  263.620513] [<c0301ce4>] (mmc_resume_host+0xc8/0x15c) from [<c0317bdc>] (omap_hsmmc_resume+0xbc/0x104)
[  263.630279] [<c0317bdc>] (omap_hsmmc_resume+0xbc/0x104) from [<c025b41c>] (platform_pm_resume+0x44/0x54)
[  263.640197] [<c025b41c>] (platform_pm_resume+0x44/0x54) from [<c025f88c>] (dpm_run_callback+0x48/0x8c)
[  263.649963] [<c025f88c>] (dpm_run_callback+0x48/0x8c) from [<c0260348>] (device_resume+0x204/0x268)
[  263.659454] [<c0260348>] (device_resume+0x204/0x268) from [<c02604b8>] (dpm_resume+0x10c/0x244)
[  263.668579] [<c02604b8>] (dpm_resume+0x10c/0x244) from [<c02605fc>] (dpm_resume_end+0xc/0x18)
[  263.677490] [<c02605fc>] (dpm_resume_end+0xc/0x18) from [<c0061784>] (suspend_devices_and_enter+0x1d0/0x22c)
[  263.687805] [<c0061784>] (suspend_devices_and_enter+0x1d0/0x22c) from [<c00618f8>] (enter_state+0x118/0x170)
[  263.698089] [<c00618f8>] (enter_state+0x118/0x170) from [<c00605b4>] (state_store+0x94/0x118)
[  263.707061] [<c00605b4>] (state_store+0x94/0x118) from [<c01df898>] (kobj_attr_store+0x1c/0x24)
[  263.716186] [<c01df898>] (kobj_attr_store+0x1c/0x24) from [<c011dcf4>] (sysfs_write_file+0x108/0x13c)
[  263.725860] [<c011dcf4>] (sysfs_write_file+0x108/0x13c) from [<c00c5748>] (vfs_write+0xac/0x180)
[  263.735076] [<c00c5748>] (vfs_write+0xac/0x180) from [<c00c58d4>] (sys_write+0x40/0x6c)
[  263.743469] [<c00c58d4>] (sys_write+0x40/0x6c) from [<c000ea00>] (ret_fast_syscall+0x0/0x3c)


while mmcdq/0 is owning the device and waiting for something that will
apparently never happen:

[  262.566680] mmcqd/0         D c046455c  5828    53      2 0x00000000
[  262.573425] [<c046455c>] (__schedule+0x584/0x614) from [<c04620f0>] (schedule_timeout+0x1c/0x1d0)
[  262.582733] [<c04620f0>] (schedule_timeout+0x1c/0x1d0) from [<c0463eb4>] (wait_for_common+0xd8/0x150)
[  262.592407] [<c0463eb4>] (wait_for_common+0xd8/0x150) from [<c0303488>] (mmc_wait_for_req_done+0x24/0xbc)
[  262.602447] [<c0303488>] (mmc_wait_for_req_done+0x24/0xbc) from [<c0303f20>] (mmc_start_req+0x50/0x11c)
[  262.612304] [<c0303f20>] (mmc_start_req+0x50/0x11c) from [<c030df2c>] (mmc_blk_issue_rw_rq+0x78/0x500)
[  262.622070] [<c030df2c>] (mmc_blk_issue_rw_rq+0x78/0x500) from [<c030e7a4>] (mmc_blk_issue_rq+0x3f0/0x420)
[  262.632171] [<c030e7a4>] (mmc_blk_issue_rq+0x3f0/0x420) from [<c030f884>] (mmc_queue_thread+0x98/0x100)
[  262.642028] [<c030f884>] (mmc_queue_thread+0x98/0x100) from [<c004fcc8>] (kthread+0x84/0x90)
[  262.650878] [<c004fcc8>] (kthread+0x84/0x90) from [<c000f398>] (kernel_thread_exit+0x0/0x8)

I traced this to the fact that mmc_bus_suspend / mmc_bus_resume are *never*
being called.  So mmc_blk_suspend isn't called and the queue isn't suspended.

The problem appears to be that mmc_bus_suspend is defined as a 'legacy'
suspend function. i.e. it is assigned to mmc_bus_type.suspend rather than
mmc_bus_type.pm.suspend.
In __device_suspend() I see that the legacy suspend function is only called if
the bus has *no* dev_pm_ops at all.
However when CONFIG_PM_RUNTIME is defined, mmc_bustype does have a dev_pm_ops
which contains runtime_{suspend,resume,idle},but no suspend or resume.

The net effect is that - as I observed - mmc_bus_suspend is never called.

I added lines:

	.suspend		= mmc_bus_suspend,
	.resume			= mmc_bus_resume,

to mmc_bus_pm_ops and can no longer reproduce the problem.  So maybe this
patch is appropriate:

diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index 5d011a3..80c1e46 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -169,6 +169,8 @@ static const struct dev_pm_ops mmc_bus_pm_ops = {
 	.runtime_suspend	= mmc_runtime_suspend,
 	.runtime_resume		= mmc_runtime_resume,
 	.runtime_idle		= mmc_runtime_idle,
+	.suspend		= mmc_bus_suspend,
+	.resume			= mmc_bus_resume,
 };
 
 #define MMC_PM_OPS_PTR	(&mmc_bus_pm_ops)

however I suspect we should remove the 'legacy' pointers at the same time.(?).

Also, while exploring this problem I could not find anything that would cause
mmc_bus_suspend() to wait for an async request to complete.  Maybe  it is
there somewhere that I don't understand yet, and I cannot be sure that any of
my symptoms could be explained by an async request continuing while the
hardware was powered off, but I wonder if something like this might be needed
too:

diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index 2517547..cd36c30 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -354,6 +354,8 @@ void mmc_queue_suspend(struct mmc_queue *mq)
 		spin_unlock_irqrestore(q->queue_lock, flags);
 
 		down(&mq->thread_sem);
+		/* wait for current request to complete */
+		mmc_start_req(mq->card->host, NULL, NULL);
 	}
 }
 

Thanks for any help you can provide,

NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]