* [PATCH 0/4] PM / Runtime: Improvements for parent/child relations
@ 2016-10-17 18:16 Ulf Hansson
2016-10-17 18:16 ` [PATCH 1/4] PM / Runtime: Remove the exported function pm_children_suspended() Ulf Hansson
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Ulf Hansson @ 2016-10-17 18:16 UTC (permalink / raw)
To: Rafael J. Wysocki, Alan Stern, Ulf Hansson, linux-pm
Cc: Len Brown, Pavel Machek, Kevin Hilman, Geert Uytterhoeven,
Lina Iyer, Jon Hunter, Marek Szyprowski, Linus Walleij
While working on improving the pm_runtime_force_suspend|resume() helpers [1], I
bumped into some code in the runtime PM core, which I think deserves some
improvement. Particularly these changes concerns parent-child management.
[1]
https://marc.info/?l=linux-pm&m=147637080702518&w=2
Ulf Hansson (4):
PM / Runtime: Remove the exported function pm_children_suspended()
PM / Runtime: Clarify comment in rpm_resume() when resuming the parent
PM / Runtime: Convert pm_runtime_set_suspended() to return an int
PM / Runtime: Don't allow to suspend a device with an active child
drivers/base/power/runtime.c | 19 +++++++++++++++----
include/linux/pm_runtime.h | 11 ++---------
2 files changed, 17 insertions(+), 13 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/4] PM / Runtime: Remove the exported function pm_children_suspended()
2016-10-17 18:16 [PATCH 0/4] PM / Runtime: Improvements for parent/child relations Ulf Hansson
@ 2016-10-17 18:16 ` Ulf Hansson
2016-10-21 12:20 ` Linus Walleij
2016-10-17 18:16 ` [PATCH 2/4] PM / Runtime: Clarify comment in rpm_resume() when resuming the parent Ulf Hansson
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Ulf Hansson @ 2016-10-17 18:16 UTC (permalink / raw)
To: Rafael J. Wysocki, Alan Stern, Ulf Hansson, linux-pm
Cc: Len Brown, Pavel Machek, Kevin Hilman, Geert Uytterhoeven,
Lina Iyer, Jon Hunter, Marek Szyprowski, Linus Walleij
The exported function pm_children_suspended() has only one caller, which is
the runtime PM internal function, rpm_check_suspend_allowed().
Let's clean-up this code, by removing pm_children_suspended() altogether
and instead do the one-liner check directly in rpm_check_suspend_allowed().
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/base/power/runtime.c | 3 ++-
include/linux/pm_runtime.h | 7 -------
2 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index f662267..5e4e5ec 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -241,7 +241,8 @@ static int rpm_check_suspend_allowed(struct device *dev)
retval = -EACCES;
else if (atomic_read(&dev->power.usage_count) > 0)
retval = -EAGAIN;
- else if (!pm_children_suspended(dev))
+ else if (!dev->power.ignore_children &&
+ atomic_read(&dev->power.child_count))
retval = -EBUSY;
/* Pending resume requests take precedence over suspends. */
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 2e14d26..61ea566 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -61,12 +61,6 @@ static inline void pm_suspend_ignore_children(struct device *dev, bool enable)
dev->power.ignore_children = enable;
}
-static inline bool pm_children_suspended(struct device *dev)
-{
- return dev->power.ignore_children
- || !atomic_read(&dev->power.child_count);
-}
-
static inline void pm_runtime_get_noresume(struct device *dev)
{
atomic_inc(&dev->power.usage_count);
@@ -162,7 +156,6 @@ static inline void pm_runtime_allow(struct device *dev) {}
static inline void pm_runtime_forbid(struct device *dev) {}
static inline void pm_suspend_ignore_children(struct device *dev, bool enable) {}
-static inline bool pm_children_suspended(struct device *dev) { return false; }
static inline void pm_runtime_get_noresume(struct device *dev) {}
static inline void pm_runtime_put_noidle(struct device *dev) {}
static inline bool device_run_wake(struct device *dev) { return false; }
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/4] PM / Runtime: Clarify comment in rpm_resume() when resuming the parent
2016-10-17 18:16 [PATCH 0/4] PM / Runtime: Improvements for parent/child relations Ulf Hansson
2016-10-17 18:16 ` [PATCH 1/4] PM / Runtime: Remove the exported function pm_children_suspended() Ulf Hansson
@ 2016-10-17 18:16 ` Ulf Hansson
2016-10-21 12:21 ` Linus Walleij
2016-10-17 18:17 ` [PATCH 3/4] PM / Runtime: Convert pm_runtime_set_suspended() to return an int Ulf Hansson
2016-10-17 18:17 ` [PATCH 4/4] PM / Runtime: Don't allow to suspend a device with an active child Ulf Hansson
3 siblings, 1 reply; 13+ messages in thread
From: Ulf Hansson @ 2016-10-17 18:16 UTC (permalink / raw)
To: Rafael J. Wysocki, Alan Stern, Ulf Hansson, linux-pm
Cc: Len Brown, Pavel Machek, Kevin Hilman, Geert Uytterhoeven,
Lina Iyer, Jon Hunter, Marek Szyprowski, Linus Walleij
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/base/power/runtime.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 5e4e5ec..f47a345 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -713,8 +713,8 @@ static int rpm_resume(struct device *dev, int rpmflags)
spin_lock(&parent->power.lock);
/*
- * We can resume if the parent's runtime PM is disabled or it
- * is set to ignore children.
+ * Resume the parent if it has runtime PM enabled and not been
+ * set to ignore its children.
*/
if (!parent->power.disable_depth
&& !parent->power.ignore_children) {
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/4] PM / Runtime: Convert pm_runtime_set_suspended() to return an int
2016-10-17 18:16 [PATCH 0/4] PM / Runtime: Improvements for parent/child relations Ulf Hansson
2016-10-17 18:16 ` [PATCH 1/4] PM / Runtime: Remove the exported function pm_children_suspended() Ulf Hansson
2016-10-17 18:16 ` [PATCH 2/4] PM / Runtime: Clarify comment in rpm_resume() when resuming the parent Ulf Hansson
@ 2016-10-17 18:17 ` Ulf Hansson
2016-10-21 12:21 ` Linus Walleij
2016-10-17 18:17 ` [PATCH 4/4] PM / Runtime: Don't allow to suspend a device with an active child Ulf Hansson
3 siblings, 1 reply; 13+ messages in thread
From: Ulf Hansson @ 2016-10-17 18:17 UTC (permalink / raw)
To: Rafael J. Wysocki, Alan Stern, Ulf Hansson, linux-pm
Cc: Len Brown, Pavel Machek, Kevin Hilman, Geert Uytterhoeven,
Lina Iyer, Jon Hunter, Marek Szyprowski, Linus Walleij
Because pm_runtime_set_suspended() invokes __pm_runtime_set_status(), which
can fail, pm_runtime_set_suspended() can also fail.
Instead of hiding a potential error, let's propagate it by converting
pm_runtime_set_suspended() from a void to return an int. In this way users
are able to check the error code and act accordingly.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
include/linux/pm_runtime.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 61ea566..4957fc1 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -258,9 +258,9 @@ static inline int pm_runtime_set_active(struct device *dev)
return __pm_runtime_set_status(dev, RPM_ACTIVE);
}
-static inline void pm_runtime_set_suspended(struct device *dev)
+static inline int pm_runtime_set_suspended(struct device *dev)
{
- __pm_runtime_set_status(dev, RPM_SUSPENDED);
+ return __pm_runtime_set_status(dev, RPM_SUSPENDED);
}
static inline void pm_runtime_disable(struct device *dev)
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/4] PM / Runtime: Don't allow to suspend a device with an active child
2016-10-17 18:16 [PATCH 0/4] PM / Runtime: Improvements for parent/child relations Ulf Hansson
` (2 preceding siblings ...)
2016-10-17 18:17 ` [PATCH 3/4] PM / Runtime: Convert pm_runtime_set_suspended() to return an int Ulf Hansson
@ 2016-10-17 18:17 ` Ulf Hansson
2016-10-21 12:23 ` Linus Walleij
2016-10-25 13:59 ` Geert Uytterhoeven
3 siblings, 2 replies; 13+ messages in thread
From: Ulf Hansson @ 2016-10-17 18:17 UTC (permalink / raw)
To: Rafael J. Wysocki, Alan Stern, Ulf Hansson, linux-pm
Cc: Len Brown, Pavel Machek, Kevin Hilman, Geert Uytterhoeven,
Lina Iyer, Jon Hunter, Marek Szyprowski, Linus Walleij
When resuming a device in __pm_runtime_set_status(), the prerequisite is
that its parent must already be active, else an error code is returned and
the device's status remains suspended.
When suspending a device there is no similar constraints being validated.
Let's change this to make the behaviour consistent, by not allowing to
suspend a device with an active child, unless it has been explicitly set to
ignore its children.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/base/power/runtime.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index f47a345..6f946a3 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1028,7 +1028,17 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status)
goto out_set;
if (status == RPM_SUSPENDED) {
- /* It always is possible to set the status to 'suspended'. */
+ /*
+ * It is invalid to suspend a device with an active child,
+ * unless it has been set to ignore its children.
+ */
+ if (!dev->power.ignore_children &&
+ atomic_read(&dev->power.child_count)) {
+ dev_err(dev, "runtime PM trying to suspend device but active child\n");
+ error = -EBUSY;
+ goto out;
+ }
+
if (parent) {
atomic_add_unless(&parent->power.child_count, -1, 0);
notify_parent = !parent->power.ignore_children;
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] PM / Runtime: Remove the exported function pm_children_suspended()
2016-10-17 18:16 ` [PATCH 1/4] PM / Runtime: Remove the exported function pm_children_suspended() Ulf Hansson
@ 2016-10-21 12:20 ` Linus Walleij
0 siblings, 0 replies; 13+ messages in thread
From: Linus Walleij @ 2016-10-21 12:20 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rafael J. Wysocki, Alan Stern, linux-pm, Len Brown, Pavel Machek,
Kevin Hilman, Geert Uytterhoeven, Lina Iyer, Jon Hunter,
Marek Szyprowski
On Mon, Oct 17, 2016 at 8:16 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> The exported function pm_children_suspended() has only one caller, which is
> the runtime PM internal function, rpm_check_suspend_allowed().
>
> Let's clean-up this code, by removing pm_children_suspended() altogether
> and instead do the one-liner check directly in rpm_check_suspend_allowed().
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] PM / Runtime: Clarify comment in rpm_resume() when resuming the parent
2016-10-17 18:16 ` [PATCH 2/4] PM / Runtime: Clarify comment in rpm_resume() when resuming the parent Ulf Hansson
@ 2016-10-21 12:21 ` Linus Walleij
0 siblings, 0 replies; 13+ messages in thread
From: Linus Walleij @ 2016-10-21 12:21 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rafael J. Wysocki, Alan Stern, linux-pm, Len Brown, Pavel Machek,
Kevin Hilman, Geert Uytterhoeven, Lina Iyer, Jon Hunter,
Marek Szyprowski
On Mon, Oct 17, 2016 at 8:16 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/4] PM / Runtime: Convert pm_runtime_set_suspended() to return an int
2016-10-17 18:17 ` [PATCH 3/4] PM / Runtime: Convert pm_runtime_set_suspended() to return an int Ulf Hansson
@ 2016-10-21 12:21 ` Linus Walleij
0 siblings, 0 replies; 13+ messages in thread
From: Linus Walleij @ 2016-10-21 12:21 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rafael J. Wysocki, Alan Stern, linux-pm, Len Brown, Pavel Machek,
Kevin Hilman, Geert Uytterhoeven, Lina Iyer, Jon Hunter,
Marek Szyprowski
On Mon, Oct 17, 2016 at 8:17 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> Because pm_runtime_set_suspended() invokes __pm_runtime_set_status(), which
> can fail, pm_runtime_set_suspended() can also fail.
>
> Instead of hiding a potential error, let's propagate it by converting
> pm_runtime_set_suspended() from a void to return an int. In this way users
> are able to check the error code and act accordingly.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] PM / Runtime: Don't allow to suspend a device with an active child
2016-10-17 18:17 ` [PATCH 4/4] PM / Runtime: Don't allow to suspend a device with an active child Ulf Hansson
@ 2016-10-21 12:23 ` Linus Walleij
2016-10-25 13:59 ` Geert Uytterhoeven
1 sibling, 0 replies; 13+ messages in thread
From: Linus Walleij @ 2016-10-21 12:23 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rafael J. Wysocki, Alan Stern, linux-pm, Len Brown, Pavel Machek,
Kevin Hilman, Geert Uytterhoeven, Lina Iyer, Jon Hunter,
Marek Szyprowski
On Mon, Oct 17, 2016 at 8:17 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> When resuming a device in __pm_runtime_set_status(), the prerequisite is
> that its parent must already be active, else an error code is returned and
> the device's status remains suspended.
>
> When suspending a device there is no similar constraints being validated.
> Let's change this to make the behaviour consistent, by not allowing to
> suspend a device with an active child, unless it has been explicitly set to
> ignore its children.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Looks correct. If there are drivers/buses relying on this
erroneous semantic we will learn about it pretty quickly I think :D
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] PM / Runtime: Don't allow to suspend a device with an active child
2016-10-17 18:17 ` [PATCH 4/4] PM / Runtime: Don't allow to suspend a device with an active child Ulf Hansson
2016-10-21 12:23 ` Linus Walleij
@ 2016-10-25 13:59 ` Geert Uytterhoeven
2016-10-25 21:31 ` Ulf Hansson
1 sibling, 1 reply; 13+ messages in thread
From: Geert Uytterhoeven @ 2016-10-25 13:59 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rafael J. Wysocki, Alan Stern, Linux PM list, Len Brown,
Pavel Machek, Kevin Hilman, Lina Iyer, Jon Hunter,
Marek Szyprowski, Linus Walleij, Linux-Renesas
Hi Ulf,
On Mon, Oct 17, 2016 at 8:17 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> When resuming a device in __pm_runtime_set_status(), the prerequisite is
> that its parent must already be active, else an error code is returned and
> the device's status remains suspended.
>
> When suspending a device there is no similar constraints being validated.
> Let's change this to make the behaviour consistent, by not allowing to
> suspend a device with an active child, unless it has been explicitly set to
> ignore its children.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
This is now commit 8b1107b85efd78c in pm/linux-next.
This breaks resume from s2ram on r8a73a4/ape6evm and sh73a0/kzm9g, where the
smsc911x Ethernet is connected to an external bus, whose clock is controlled
through Runtime PM and the simple-pm-bus driver.
Reverting this commit fixes the issue.
> ---
> drivers/base/power/runtime.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index f47a345..6f946a3 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1028,7 +1028,17 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status)
> goto out_set;
>
> if (status == RPM_SUSPENDED) {
> - /* It always is possible to set the status to 'suspended'. */
> + /*
> + * It is invalid to suspend a device with an active child,
> + * unless it has been set to ignore its children.
> + */
> + if (!dev->power.ignore_children &&
> + atomic_read(&dev->power.child_count)) {
> + dev_err(dev, "runtime PM trying to suspend device but active child\n");
> + error = -EBUSY;
> + goto out;
> + }
> +
> if (parent) {
> atomic_add_unless(&parent->power.child_count, -1, 0);
> notify_parent = !parent->power.ignore_children;
Kernel output difference, with some additional debug info:
| --- GOOD 2016-10-25 15:46:19.669597124 +0200
| +++ BAD 2016-10-25 15:48:15.201596869 +0200
| @@ -5,24 +5,71 @@ Freezing remaining freezable tasks ... (
| PM: Suspending system (mem)
| cpg_div6_clock_disable: sdhi1ck
| cpg_div6_clock_disable: sdhi0ck
| -PM: suspend of devices complete after 22.777 msecs
| -PM: late suspend of devices complete after 7.302 msecs
| +PM: suspend of devices complete after 22.932 msecs
| +PM: late suspend of devices complete after 7.311 msecs
| cpg_div6_clock_disable: mmc0
| cpg_div6_clock_disable: zb
pm_clk disables the clock of fec10000.bus.
| +simple-pm-bus fec10000.bus: runtime PM trying to suspend device but
active child
Suspend is aborted with -EBUSY, but zb stays disabled.
| rmobile_pd_power_down: a3sp
| -PM: noirq suspend of devices complete after 18.424 msecs
| +PM: noirq suspend of devices complete after 26.937 msecs
| Disabling non-boot CPUs ...
| -Suspended for 7.196 seconds
| +Suspended for 2.579 seconds
| rmobile_pd_power_up: a3sp
| -cpg_div6_clock_enable: zb
pm_clk doesn't enable the clock of fec10000.bus.
(since it thinks it's still suspended?)
| cpg_div6_clock_enable: sdhi0ck
| cpg_div6_clock_enable: sdhi1ck
| cpg_div6_clock_enable: mmc0
| -PM: noirq resume of devices complete after 134.580 msecs
| -PM: early resume of devices complete after 6.084 msecs
| -PM: resume of devices complete after 21.846 msecs
| -PM: Finishing wakeup.
| -Restarting tasks ... cpg_div6_clock_disable: mmc0
| -done.
| +PM: noirq resume of devices complete after 128.014 msecs
| +PM: early resume of devices complete after 6.121 msecs
| +Unhandled fault: imprecise external abort (0x1406) at 0x00000000
| +pgd = ee748000
| +[00000000] *pgd=7fc42835
| +Internal error: : 1406 [#1] SMP ARM
| +CPU: 0 PID: 1087 Comm: s2ram Not tainted
4.9.0-rc2-ape6evm-01959-g90550a414a6a6cd3-dirty #505
| +Hardware name: Generic R8A73A4 (Flattened Device Tree)
| +task: ee47a340 task.stack: ee686000
| +PC is at smsc911x_resume+0x6c/0xbc
| +LR is at smsc911x_resume+0x6c/0xbc
smsc911x_resume() tries to access the smsc911x while the bus clock zb is
not enabled.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] PM / Runtime: Don't allow to suspend a device with an active child
2016-10-25 13:59 ` Geert Uytterhoeven
@ 2016-10-25 21:31 ` Ulf Hansson
2016-10-26 9:47 ` Geert Uytterhoeven
0 siblings, 1 reply; 13+ messages in thread
From: Ulf Hansson @ 2016-10-25 21:31 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Rafael J. Wysocki, Alan Stern, Linux PM list, Len Brown,
Pavel Machek, Kevin Hilman, Lina Iyer, Jon Hunter,
Marek Szyprowski, Linus Walleij, Linux-Renesas
On 25 October 2016 at 15:59, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Mon, Oct 17, 2016 at 8:17 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> When resuming a device in __pm_runtime_set_status(), the prerequisite is
>> that its parent must already be active, else an error code is returned and
>> the device's status remains suspended.
>>
>> When suspending a device there is no similar constraints being validated.
>> Let's change this to make the behaviour consistent, by not allowing to
>> suspend a device with an active child, unless it has been explicitly set to
>> ignore its children.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> This is now commit 8b1107b85efd78c in pm/linux-next.
>
> This breaks resume from s2ram on r8a73a4/ape6evm and sh73a0/kzm9g, where the
> smsc911x Ethernet is connected to an external bus, whose clock is controlled
> through Runtime PM and the simple-pm-bus driver.
>
> Reverting this commit fixes the issue.
Geert, thanks again for testing and reporting. I believe this problem
is directly related to what you reported for another patch [1] as
well.
Can you please try out this rather trivial patch to the Ethernet
driver (smsc911x), to see if that solves the problem(s).
From: Ulf Hansson <ulf.hansson@linaro.org>
Date: Tue, 25 Oct 2016 23:11:51 +0200
Subject: [PATCH] net: smsc911x: Synchronize the runtime PM status during
system suspend
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/net/ethernet/smsc/smsc911x.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/ethernet/smsc/smsc911x.c
b/drivers/net/ethernet/smsc/smsc911x.c
index e9b8579..65fca9c 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -2584,6 +2584,9 @@ static int smsc911x_suspend(struct device *dev)
PMT_CTRL_PM_MODE_D1_ | PMT_CTRL_WOL_EN_ |
PMT_CTRL_ED_EN_ | PMT_CTRL_PME_EN_);
+ pm_runtime_disable(dev);
+ pm_runtime_set_suspended(dev);
+
return 0;
}
@@ -2593,6 +2596,9 @@ static int smsc911x_resume(struct device *dev)
struct smsc911x_data *pdata = netdev_priv(ndev);
unsigned int to = 100;
+ pm_runtime_enable(dev);
+ pm_runtime_resume(dev);
+
/* Note 3.11 from the datasheet:
* "When the LAN9220 is in a power saving state, a write of any
* data to the BYTE_TEST register will wake-up the device."
--
1.9.1
Kind regards
Uffe
[1]
https://patchwork.kernel.org/patch/9375061/
>
>> ---
>> drivers/base/power/runtime.c | 12 +++++++++++-
>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>> index f47a345..6f946a3 100644
>> --- a/drivers/base/power/runtime.c
>> +++ b/drivers/base/power/runtime.c
>> @@ -1028,7 +1028,17 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status)
>> goto out_set;
>>
>> if (status == RPM_SUSPENDED) {
>> - /* It always is possible to set the status to 'suspended'. */
>> + /*
>> + * It is invalid to suspend a device with an active child,
>> + * unless it has been set to ignore its children.
>> + */
>> + if (!dev->power.ignore_children &&
>> + atomic_read(&dev->power.child_count)) {
>> + dev_err(dev, "runtime PM trying to suspend device but active child\n");
>> + error = -EBUSY;
>> + goto out;
>> + }
>> +
>> if (parent) {
>> atomic_add_unless(&parent->power.child_count, -1, 0);
>> notify_parent = !parent->power.ignore_children;
>
> Kernel output difference, with some additional debug info:
>
> | --- GOOD 2016-10-25 15:46:19.669597124 +0200
> | +++ BAD 2016-10-25 15:48:15.201596869 +0200
> | @@ -5,24 +5,71 @@ Freezing remaining freezable tasks ... (
> | PM: Suspending system (mem)
> | cpg_div6_clock_disable: sdhi1ck
> | cpg_div6_clock_disable: sdhi0ck
> | -PM: suspend of devices complete after 22.777 msecs
> | -PM: late suspend of devices complete after 7.302 msecs
> | +PM: suspend of devices complete after 22.932 msecs
> | +PM: late suspend of devices complete after 7.311 msecs
> | cpg_div6_clock_disable: mmc0
> | cpg_div6_clock_disable: zb
>
> pm_clk disables the clock of fec10000.bus.
>
> | +simple-pm-bus fec10000.bus: runtime PM trying to suspend device but
> active child
>
> Suspend is aborted with -EBUSY, but zb stays disabled.
>
> | rmobile_pd_power_down: a3sp
> | -PM: noirq suspend of devices complete after 18.424 msecs
> | +PM: noirq suspend of devices complete after 26.937 msecs
> | Disabling non-boot CPUs ...
> | -Suspended for 7.196 seconds
> | +Suspended for 2.579 seconds
> | rmobile_pd_power_up: a3sp
> | -cpg_div6_clock_enable: zb
>
> pm_clk doesn't enable the clock of fec10000.bus.
> (since it thinks it's still suspended?)
>
> | cpg_div6_clock_enable: sdhi0ck
> | cpg_div6_clock_enable: sdhi1ck
> | cpg_div6_clock_enable: mmc0
> | -PM: noirq resume of devices complete after 134.580 msecs
> | -PM: early resume of devices complete after 6.084 msecs
> | -PM: resume of devices complete after 21.846 msecs
> | -PM: Finishing wakeup.
> | -Restarting tasks ... cpg_div6_clock_disable: mmc0
> | -done.
> | +PM: noirq resume of devices complete after 128.014 msecs
> | +PM: early resume of devices complete after 6.121 msecs
> | +Unhandled fault: imprecise external abort (0x1406) at 0x00000000
> | +pgd = ee748000
> | +[00000000] *pgd=7fc42835
> | +Internal error: : 1406 [#1] SMP ARM
> | +CPU: 0 PID: 1087 Comm: s2ram Not tainted
> 4.9.0-rc2-ape6evm-01959-g90550a414a6a6cd3-dirty #505
> | +Hardware name: Generic R8A73A4 (Flattened Device Tree)
> | +task: ee47a340 task.stack: ee686000
> | +PC is at smsc911x_resume+0x6c/0xbc
> | +LR is at smsc911x_resume+0x6c/0xbc
>
> smsc911x_resume() tries to access the smsc911x while the bus clock zb is
> not enabled.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] PM / Runtime: Don't allow to suspend a device with an active child
2016-10-25 21:31 ` Ulf Hansson
@ 2016-10-26 9:47 ` Geert Uytterhoeven
2016-10-26 11:30 ` Ulf Hansson
0 siblings, 1 reply; 13+ messages in thread
From: Geert Uytterhoeven @ 2016-10-26 9:47 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rafael J. Wysocki, Alan Stern, Linux PM list, Len Brown,
Pavel Machek, Kevin Hilman, Lina Iyer, Jon Hunter,
Marek Szyprowski, Linus Walleij, Linux-Renesas
Hi Ulf,
On Tue, Oct 25, 2016 at 11:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 25 October 2016 at 15:59, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Mon, Oct 17, 2016 at 8:17 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> When resuming a device in __pm_runtime_set_status(), the prerequisite is
>>> that its parent must already be active, else an error code is returned and
>>> the device's status remains suspended.
>>>
>>> When suspending a device there is no similar constraints being validated.
>>> Let's change this to make the behaviour consistent, by not allowing to
>>> suspend a device with an active child, unless it has been explicitly set to
>>> ignore its children.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> This is now commit 8b1107b85efd78c in pm/linux-next.
>>
>> This breaks resume from s2ram on r8a73a4/ape6evm and sh73a0/kzm9g, where the
>> smsc911x Ethernet is connected to an external bus, whose clock is controlled
>> through Runtime PM and the simple-pm-bus driver.
>>
>> Reverting this commit fixes the issue.
>
> Geert, thanks again for testing and reporting. I believe this problem
> is directly related to what you reported for another patch [1] as
> well.
Indeed. At first I thought that patch got applied, but I couldn't find it ;-)
> Can you please try out this rather trivial patch to the Ethernet
> driver (smsc911x), to see if that solves the problem(s).
Thanks, it does!
Difference between before and after is:
PM: Suspending system (mem)
cpg_div6_clock_disable: sdhi1ck
cpg_div6_clock_disable: sdhi0ck
-PM: suspend of devices complete after 22.932 msecs
-PM: late suspend of devices complete after 7.311 msecs
+PM: suspend of devices complete after 23.131 msecs
+PM: late suspend of devices complete after 7.300 msecs
cpg_div6_clock_disable: mmc0
cpg_div6_clock_disable: zb
-simple-pm-bus fec10000.bus: runtime PM trying to suspend device but
active child
rmobile_pd_power_down: a3sp
-PM: noirq suspend of devices complete after 26.937 msecs
+PM: noirq suspend of devices complete after 18.467 msecs
Disabling non-boot CPUs ...
-Suspended for 2.579 seconds
+Suspended for 2.949 seconds
rmobile_pd_power_up: a3sp
+cpg_div6_clock_enable: zb
cpg_div6_clock_enable: sdhi0ck
cpg_div6_clock_enable: sdhi1ck
cpg_div6_clock_enable: mmc0
-PM: noirq resume of devices complete after 128.014 msecs
-PM: early resume of devices complete after 6.121 msecs
-Unhandled fault: imprecise external abort (0x1406) at 0x00000000
The zb clock is enabled again.
BTW, shouldn't "runtime PM trying to suspend device but active child"
become a WARN()? If this happens, something is really wrong, and
the device will be left in half-suspended state (clock disabled).
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Date: Tue, 25 Oct 2016 23:11:51 +0200
> Subject: [PATCH] net: smsc911x: Synchronize the runtime PM status during
> system suspend
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] PM / Runtime: Don't allow to suspend a device with an active child
2016-10-26 9:47 ` Geert Uytterhoeven
@ 2016-10-26 11:30 ` Ulf Hansson
0 siblings, 0 replies; 13+ messages in thread
From: Ulf Hansson @ 2016-10-26 11:30 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Rafael J. Wysocki, Alan Stern, Linux PM list, Len Brown,
Pavel Machek, Kevin Hilman, Lina Iyer, Jon Hunter,
Marek Szyprowski, Linus Walleij, Linux-Renesas
On 26 October 2016 at 11:47, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Tue, Oct 25, 2016 at 11:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 25 October 2016 at 15:59, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Mon, Oct 17, 2016 at 8:17 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> When resuming a device in __pm_runtime_set_status(), the prerequisite is
>>>> that its parent must already be active, else an error code is returned and
>>>> the device's status remains suspended.
>>>>
>>>> When suspending a device there is no similar constraints being validated.
>>>> Let's change this to make the behaviour consistent, by not allowing to
>>>> suspend a device with an active child, unless it has been explicitly set to
>>>> ignore its children.
>>>>
>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>
>>> This is now commit 8b1107b85efd78c in pm/linux-next.
>>>
>>> This breaks resume from s2ram on r8a73a4/ape6evm and sh73a0/kzm9g, where the
>>> smsc911x Ethernet is connected to an external bus, whose clock is controlled
>>> through Runtime PM and the simple-pm-bus driver.
>>>
>>> Reverting this commit fixes the issue.
>>
>> Geert, thanks again for testing and reporting. I believe this problem
>> is directly related to what you reported for another patch [1] as
>> well.
>
> Indeed. At first I thought that patch got applied, but I couldn't find it ;-)
>
>> Can you please try out this rather trivial patch to the Ethernet
>> driver (smsc911x), to see if that solves the problem(s).
>
> Thanks, it does!
Great, thanks for testing!
I will re-post the patch with a proper change-log.
>
> Difference between before and after is:
>
> PM: Suspending system (mem)
> cpg_div6_clock_disable: sdhi1ck
> cpg_div6_clock_disable: sdhi0ck
> -PM: suspend of devices complete after 22.932 msecs
> -PM: late suspend of devices complete after 7.311 msecs
> +PM: suspend of devices complete after 23.131 msecs
> +PM: late suspend of devices complete after 7.300 msecs
> cpg_div6_clock_disable: mmc0
> cpg_div6_clock_disable: zb
> -simple-pm-bus fec10000.bus: runtime PM trying to suspend device but
> active child
> rmobile_pd_power_down: a3sp
> -PM: noirq suspend of devices complete after 26.937 msecs
> +PM: noirq suspend of devices complete after 18.467 msecs
> Disabling non-boot CPUs ...
> -Suspended for 2.579 seconds
> +Suspended for 2.949 seconds
> rmobile_pd_power_up: a3sp
> +cpg_div6_clock_enable: zb
> cpg_div6_clock_enable: sdhi0ck
> cpg_div6_clock_enable: sdhi1ck
> cpg_div6_clock_enable: mmc0
> -PM: noirq resume of devices complete after 128.014 msecs
> -PM: early resume of devices complete after 6.121 msecs
> -Unhandled fault: imprecise external abort (0x1406) at 0x00000000
>
> The zb clock is enabled again.
>
> BTW, shouldn't "runtime PM trying to suspend device but active child"
> become a WARN()? If this happens, something is really wrong, and
> the device will be left in half-suspended state (clock disabled).
That would make sense to me!
Although if we decide to change that, we should also convert from
dev_err() to WARN() when failing to set RPM_ACTIVE in
__pm_runtime_set_status(), as to be consistent.
Do you want to send a patch?
>
>> From: Ulf Hansson <ulf.hansson@linaro.org>
>> Date: Tue, 25 Oct 2016 23:11:51 +0200
>> Subject: [PATCH] net: smsc911x: Synchronize the runtime PM status during
>> system suspend
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Gr{oetje,eeting}s,
>
> Geert
>
Kind regards
Uffe
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-10-26 11:30 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-17 18:16 [PATCH 0/4] PM / Runtime: Improvements for parent/child relations Ulf Hansson
2016-10-17 18:16 ` [PATCH 1/4] PM / Runtime: Remove the exported function pm_children_suspended() Ulf Hansson
2016-10-21 12:20 ` Linus Walleij
2016-10-17 18:16 ` [PATCH 2/4] PM / Runtime: Clarify comment in rpm_resume() when resuming the parent Ulf Hansson
2016-10-21 12:21 ` Linus Walleij
2016-10-17 18:17 ` [PATCH 3/4] PM / Runtime: Convert pm_runtime_set_suspended() to return an int Ulf Hansson
2016-10-21 12:21 ` Linus Walleij
2016-10-17 18:17 ` [PATCH 4/4] PM / Runtime: Don't allow to suspend a device with an active child Ulf Hansson
2016-10-21 12:23 ` Linus Walleij
2016-10-25 13:59 ` Geert Uytterhoeven
2016-10-25 21:31 ` Ulf Hansson
2016-10-26 9:47 ` Geert Uytterhoeven
2016-10-26 11:30 ` Ulf Hansson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.