Hi,
I get the following errors when I choose devices in /sys/power/pm_test. It also shows that if 'dpm_suspend_start' fails somewhere down the line then the kernel does not reset the 'transition_started' variable and we can get warning similar to below. IMHO 'dpm_resume_noirq' is not the right place for setting this variable. Please see the patch below which fixes the issue.
Thanks,
-Romit
# [ 113.713958] mmcblk0: mmc1:0001 STM16G 14.8 GiB
[ 113.718902] mmcblk0: unknown partition table
[ 113.745574] ------------[ cut here ]------------
[ 113.751525] WARNING: at drivers/base/power/main.c:98 device_pm_add+0x94/0xdc()
[ 113.758819] Device: bdi
[ 113.758819] Parentless device registered during a PM transaction
[ 113.773071] Modules linked in:
[ 113.776214] [<c002b87c>] (unwind_backtrace+0x0/0xdc) from [<c004ff58>] (warn_slowpath_common+0x48/0x60)
[ 113.785705] [<c004ff58>] (warn_slowpath_common+0x48/0x60) from [<c004ffa8>] (warn_slowpath_fmt+0x24/0x30)
[ 113.799896] [<c004ffa8>] (warn_slowpath_fmt+0x24/0x30) from [<c01b36b8>] (device_pm_add+0x94/0xdc)
[ 113.808990] [<c01b36b8>] (device_pm_add+0x94/0xdc) from [<c01ad9bc>] (device_add+0x3a8/0x534)
[ 113.817565] [<c01ad9bc>] (device_add+0x3a8/0x534) from [<c01adbd8>] (device_create_vargs+0x78/0xa8)
[ 113.829650] [<c01adbd8>] (device_create_vargs+0x78/0xa8) from [<c0090a9c>] (bdi_register+0x3c/0x154)
[ 113.839263] [<c0090a9c>] (bdi_register+0x3c/0x154) from [<c0090bd4>] (bdi_register_dev+0x20/0x28)
[ 113.851348] [<c0090bd4>] (bdi_register_dev+0x20/0x28) from [<c0173b98>] (add_disk+0xe4/0x124)
[ 113.859954] [<c0173b98>] (add_disk+0xe4/0x124) from [<c01e6b54>] (mmc_blk_probe+0x250/0x288)
[ 113.870513] [<c01e6b54>] (mmc_blk_probe+0x250/0x288) from [<c01e14d4>] (mmc_bus_probe+0x18/0x1c)
[ 113.879394] [<c01e14d4>] (mmc_bus_probe+0x18/0x1c) from [<c01af97c>] (driver_probe_device+0xa0/0x14c)
[ 113.893890] [<c01af97c>] (driver_probe_device+0xa0/0x14c) from [<c01aef44>] (bus_for_each_drv+0x44/0x80)
[ 113.905609] [<c01aef44>] (bus_for_each_drv+0x44/0x80) from [<c01afb60>] (device_attach+0x50/0x68)
[ 113.914550] [<c01afb60>] (device_attach+0x50/0x68) from [<c01aed94>] (bus_probe_device+0x24/0x40)
[ 113.925170] [<c01aed94>] (bus_probe_device+0x24/0x40) from [<c01ad9f0>] (device_add+0x3dc/0x534)
[ 113.934082] [<c01ad9f0>] (device_add+0x3dc/0x534) from [<c01e1688>] (mmc_add_card+0xcc/0x118)
[ 113.945251] [<c01e1688>] (mmc_add_card+0xcc/0x118) from [<c01e2408>] (mmc_attach_mmc+0xdc/0x13c)
[ 113.954132] [<c01e2408>] (mmc_attach_mmc+0xdc/0x13c) from [<c01e12f0>] (mmc_rescan+0x2b0/0x2e8)
[ 113.964874] [<c01e12f0>] (mmc_rescan+0x2b0/0x2e8) from [<c0060840>] (worker_thread+0x16c/0x218)
[ 113.973693] [<c0060840>] (worker_thread+0x16c/0x218) from [<c0063d88>] (kthread+0x7c/0x84)
[ 113.986694] [<c0063d88>] (kthread+0x7c/0x84) from [<c0026ea0>] (kernel_thread_exit+0x0/0x8)
[ 113.995147] ---[ end trace aaa7bf5becc572f5 ]---
Fixes the location where we decide that our power transition is complete.
Signed-off-by: Romit Dasgupta <[email protected]>
---
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index e0dc407..6b9e991 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -365,7 +365,6 @@ void dpm_resume_noirq(pm_message_t state)
struct device *dev;
mutex_lock(&dpm_list_mtx);
- transition_started = false;
list_for_each_entry(dev, &dpm_list, power.entry)
if (dev->power.status > DPM_OFF) {
int error;
@@ -529,6 +528,7 @@ static void dpm_complete(pm_message_t state)
put_device(dev);
}
list_splice(&list, &dpm_list);
+ transition_started = false;
mutex_unlock(&dpm_list_mtx);
}
On Tue, 27 Oct 2009, Dasgupta, Romit wrote:
> Hi,
> I get the following errors when I choose devices in /sys/power/pm_test. It also shows that if 'dpm_suspend_start' fails somewhere down the line then the kernel does not reset the 'transition_started' variable and we can get warning similar to below. IMHO 'dpm_resume_noirq' is not the right place for setting this variable. Please see the patch below which fixes the issue.
> Fixes the location where we decide that our power transition is complete.
> Signed-off-by: Romit Dasgupta <[email protected]>
> ---
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index e0dc407..6b9e991 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -365,7 +365,6 @@ void dpm_resume_noirq(pm_message_t state)
> struct device *dev;
>
> mutex_lock(&dpm_list_mtx);
> - transition_started = false;
> list_for_each_entry(dev, &dpm_list, power.entry)
> if (dev->power.status > DPM_OFF) {
> int error;
> @@ -529,6 +528,7 @@ static void dpm_complete(pm_message_t state)
> put_device(dev);
> }
> list_splice(&list, &dpm_list);
> + transition_started = false;
> mutex_unlock(&dpm_list_mtx);
> }
The second hunk is okay, but the first hunk isn't. We want to set
transition_started to false as soon as devices start resuming, because
once a resume has begun it's okay to add new devices.
Alan Stern
Ok. Then the following is the refined and probably more appropriate one.
This fixes the point where we need to complete the power transition when
device suspend fails.
Signed-off-by: Romit Dasgupta <[email protected]>
---
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index e0dc407..3d0b26a 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -444,6 +444,7 @@ static void dpm_resume(pm_message_t state)
INIT_LIST_HEAD(&list);
mutex_lock(&dpm_list_mtx);
+ transition_started = false;
while (!list_empty(&dpm_list)) {
struct device *dev = to_device(dpm_list.next);
> -----Original Message-----
> From: [email protected] [mailto:linux-omap-
> [email protected]] On Behalf Of Alan Stern
> Sent: Tuesday, October 27, 2009 7:08 PM
> To: Dasgupta, Romit
> Cc: Hilman, Kevin; [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [linux-pm] [PATCH] PM: Fixes warning on suspend errors
>
> On Tue, 27 Oct 2009, Dasgupta, Romit wrote:
>
> > Hi,
> > I get the following errors when I choose devices in /sys/power/pm_test. It
> also shows that if 'dpm_suspend_start' fails somewhere down the line then
> the kernel does not reset the 'transition_started' variable and we can get
> warning similar to below. IMHO 'dpm_resume_noirq' is not the right place for
> setting this variable. Please see the patch below which fixes the issue.
>
> > Fixes the location where we decide that our power transition is complete.
> > Signed-off-by: Romit Dasgupta <[email protected]>
> > ---
> > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> > index e0dc407..6b9e991 100644
> > --- a/drivers/base/power/main.c
> > +++ b/drivers/base/power/main.c
> > @@ -365,7 +365,6 @@ void dpm_resume_noirq(pm_message_t state)
> > struct device *dev;
> >
> > mutex_lock(&dpm_list_mtx);
> > - transition_started = false;
> > list_for_each_entry(dev, &dpm_list, power.entry)
> > if (dev->power.status > DPM_OFF) {
> > int error;
> > @@ -529,6 +528,7 @@ static void dpm_complete(pm_message_t state)
> > put_device(dev);
> > }
> > list_splice(&list, &dpm_list);
> > + transition_started = false;
> > mutex_unlock(&dpm_list_mtx);
> > }
>
> The second hunk is okay, but the first hunk isn't. We want to set
> transition_started to false as soon as devices start resuming, because
> once a resume has begun it's okay to add new devices.
>
> Alan Stern
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 27 October 2009, Dasgupta, Romit wrote:
> Ok. Then the following is the refined and probably more appropriate one.
>
>
>
> This fixes the point where we need to complete the power transition when
> device suspend fails.
>
> Signed-off-by: Romit Dasgupta <[email protected]>
> ---
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index e0dc407..3d0b26a 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -444,6 +444,7 @@ static void dpm_resume(pm_message_t state)
>
> INIT_LIST_HEAD(&list);
> mutex_lock(&dpm_list_mtx);
> + transition_started = false;
> while (!list_empty(&dpm_list)) {
> struct device *dev = to_device(dpm_list.next);
>
Applied to suspend-2.6/linux-next.
Thanks,
Rafael