[PATCH 0/4] PM / Domains / Runtime: Optimize device system PM when using genpd

[PATCH 0/4] PM / Domains / Runtime: Optimize device system PM when using genpd

[Ulf Hansson]

The main goal of this series is to avoid unnecessary operations of devices
during the system PM suspend/resume path, especially when using genpd.

For example, we want to avoid to have genpd to unconditionally runtime resume
devices in the system PM prepare phase. That also means that in most cases a
runtime suspended device can remain in such state, during the system PM suspend
sequence.

Moreover, we don't want to unconditionally wake up all devices during the system
PM resume sequence, but instead defer that to be done when devices are actually
going to be used.

Note, this series is based upon another posted series (not queued) for genpd:
[PATCH v3 0/2] PM / Domains: Second step in improving system PM code in genpd
http://www.spinics.net/lists/dri-devel/msg107207.html

Ulf Hansson (4):
  PM / Domains: Remove redundant call to pm_request_idle() in genpd
  PM / Runtime: Prevent re-resuming devices in pm_runtime_force_resume()
  PM / Domains: Allow runtime PM during system PM phases
  PM / Runtime: Defer resuming of the device in
    pm_runtime_force_resume()

 drivers/base/power/domain.c  | 56 ++++++++++++++++++++++++--------------------
 drivers/base/power/runtime.c | 14 +++++++++++
 2 files changed, 44 insertions(+), 26 deletions(-)

-- 
1.9.1

[PATCH 1/4] PM / Domains: Remove redundant call to pm_request_idle() in genpd

[Ulf Hansson]

The PM core increases the runtime PM usage count at the system PM prepare
phase. Later when the system has resumed and the ->complete() callback has
been invoked, it drops the usage count. In this way, it intentionally
prevents runtime PM suspend for the device throughout this period.

For this reason, let's remove the call to pm_request_idle() from within
genpd's ->complete() calllback as it's redundant.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 658eb1b..60a9971 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -938,7 +938,6 @@ static void pm_genpd_complete(struct device *dev)
 	pm_generic_complete(dev);
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
-	pm_request_idle(dev);
 }
 
 /**
-- 
1.9.1

Re: [PATCH 1/4] PM / Domains: Remove redundant call to pm_request_idle() in genpd

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> The PM core increases the runtime PM usage count at the system PM prepare
> phase. Later when the system has resumed and the ->complete() callback has
> been invoked, it drops the usage count. In this way, it intentionally
> prevents runtime PM suspend for the device throughout this period.
>
> For this reason, let's remove the call to pm_request_idle() from within
> genpd's ->complete() calllback as it's redundant.

It's not obvious from this description why the pm_request_idle() is
redundant based just on usage count.

IIUC, I think what you mean is that it's redundant because
device_complete() does a pm_runtime_put(), which, in addition to
decrementing the usage count, will do the equivalent of
pm_request_idle()?

Kevin

Re: [PATCH 1/4] PM / Domains: Remove redundant call to pm_request_idle() in genpd

[Ulf Hansson]

On 23 May 2016 at 23:25, Kevin Hilman <khilman@baylibre.com> wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:
>
>> The PM core increases the runtime PM usage count at the system PM prepare
>> phase. Later when the system has resumed and the ->complete() callback has
>> been invoked, it drops the usage count. In this way, it intentionally
>> prevents runtime PM suspend for the device throughout this period.
>>
>> For this reason, let's remove the call to pm_request_idle() from within
>> genpd's ->complete() calllback as it's redundant.
>
> It's not obvious from this description why the pm_request_idle() is
> redundant based just on usage count.
>
> IIUC, I think what you mean is that it's redundant because
> device_complete() does a pm_runtime_put(), which, in addition to
> decrementing the usage count, will do the equivalent of
> pm_request_idle()?

That's correct, allow me to re-spin and clarify the change log.

Kind regards
Uffe

[PATCH 2/4] PM / Runtime: Prevent re-resuming devices in pm_runtime_force_resume()

[Ulf Hansson]

If the device has already been resumed before pm_runtime_force_resume() is
invoked, prevent calling the ->runtime_resume() callback for the device,
as it's not the expected behaviour from the subsystem/driver.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/runtime.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index b746904..09e4eb1 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1506,6 +1506,9 @@ int pm_runtime_force_resume(struct device *dev)
 		goto out;
 	}
 
+	if (!pm_runtime_status_suspended(dev))
+		goto out;
+
 	ret = pm_runtime_set_active(dev);
 	if (ret)
 		goto out;
-- 
1.9.1

Re: [PATCH 2/4] PM / Runtime: Prevent re-resuming devices in pm_runtime_force_resume()

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> If the device has already been resumed before pm_runtime_force_resume() is
> invoked, prevent calling the ->runtime_resume() callback for the device,
> as it's not the expected behaviour from the subsystem/driver.

I'm not following how this description matches the code.

Based on the code, it seems like it should say "If the device has not
been runtime suspended, don't runtime resume it."   IOW, "device has
already been resumed" is not what pm_runtime_status_suspended() is
checking for.

Kevin

> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/base/power/runtime.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index b746904..09e4eb1 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1506,6 +1506,9 @@ int pm_runtime_force_resume(struct device *dev)
>  		goto out;
>  	}
>  
> +	if (!pm_runtime_status_suspended(dev))
> +		goto out;
> +
>  	ret = pm_runtime_set_active(dev);
>  	if (ret)
>  		goto out;

Re: [PATCH 2/4] PM / Runtime: Prevent re-resuming devices in pm_runtime_force_resume()

[Ulf Hansson]

On 23 May 2016 at 23:30, Kevin Hilman <khilman@baylibre.com> wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:
>
>> If the device has already been resumed before pm_runtime_force_resume() is
>> invoked, prevent calling the ->runtime_resume() callback for the device,
>> as it's not the expected behaviour from the subsystem/driver.
>
> I'm not following how this description matches the code.
>
> Based on the code, it seems like it should say "If the device has not
> been runtime suspended, don't runtime resume it."   IOW, "device has
> already been resumed" is not what pm_runtime_status_suspended() is
> checking for.

Hmm, these helpers are executed during the system PM sequence to
re-use the ->runtime_suspend|resume() callbacks to suspend and resume
the device.
The message is the change log do become a bit confusing because of
that, what do you think about the below instead?

If the runtime PM status of the device isn't runtime suspended, prevent
pm_runtime_force_resume() to call the ->runtime_resume() callback for the
device, as it's not the expected behaviour from the subsystem/driver.

[...]

Kind regards
Uffe

Re: [PATCH 2/4] PM / Runtime: Prevent re-resuming devices in pm_runtime_force_resume()

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> On 23 May 2016 at 23:30, Kevin Hilman <khilman@baylibre.com> wrote:
>> Ulf Hansson <ulf.hansson@linaro.org> writes:
>>
>>> If the device has already been resumed before pm_runtime_force_resume() is
>>> invoked, prevent calling the ->runtime_resume() callback for the device,
>>> as it's not the expected behaviour from the subsystem/driver.
>>
>> I'm not following how this description matches the code.
>>
>> Based on the code, it seems like it should say "If the device has not
>> been runtime suspended, don't runtime resume it."   IOW, "device has
>> already been resumed" is not what pm_runtime_status_suspended() is
>> checking for.
>
> Hmm, these helpers are executed during the system PM sequence to
> re-use the ->runtime_suspend|resume() callbacks to suspend and resume
> the device.
> The message is the change log do become a bit confusing because of
> that, what do you think about the below instead?
>
> If the runtime PM status of the device isn't runtime suspended, prevent
> pm_runtime_force_resume() to call the ->runtime_resume() callback for the
> device, as it's not the expected behaviour from the subsystem/driver.

Yeah, that's clearer.  Thanks.

Kevin

[PATCH 3/4] PM / Domains: Allow runtime PM during system PM phases

[Ulf Hansson]

The PM core disables runtime PM at __device_suspend_late() before it calls
a system PM "late" callback for the device. When resuming the device,
after the corresponding "early" callback has been invoked, it re-enables
runtime PM.

By changing genpd to conform to this behaviour, the device no longer have
to be unconditionally runtime resumed from genpd's ->prepare() callback. In
most cases that avoids unnecessary operations.

As runtime PM then isn't disabled/enabled by genpd, the subsystem/driver
can rely on the generic behaviour from PM core. Consequentially runtime PM
is allowed in more phases of system PM than before.

Although, because of this change and due to that genpd powers on the PM
domain unconditionally in the system PM resume "noirq" phase, it could
potentially cause a PM domain to stay powered on even if it's unused after
the system has resumed. To avoid this, let's schedule a power off work
when genpd's system PM ->complete() callback has been invoked for the last
device in the PM domain.

Another issue that arises due to this change in genpd, concerns those
platforms/PM domains that makes use of genpd's device ->stop|start()
callbacks. In these scenarios, the corresponding subsystem/driver needs to
invoke pm_runtime_force_suspend() from a system PM suspend callback to
allow genpd's ->runtime_suspend() to be invoked for an active device, else
genpd can't "stop" a device that is "started".

The subsystem/driver also needs to invoke pm_runtime_force_resume()
in a system PM resume callback, to restore the runtime PM state for the
device and to re-enable runtime PM.

Currently not all involved subsystem/drivers makes use of
pm_runtime_force_suspend|resume() accordingly. Therefore, let's invoke
pm_runtime_force_suspend|resume() from genpd's "noirq" system PM
callbacks, in cases when the ->stop|start() callbacks are being used.

In this way, devices are "stoped" during suspend and "started" during
resume, even in those cases when the subsystem/driver don't call
pm_runtime_force_suspend|resume() themselves.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 55 ++++++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 25 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 60a9971..9193aac 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -739,21 +739,6 @@ static int pm_genpd_prepare(struct device *dev)
 
 	mutex_unlock(&genpd->lock);
 
-	/*
-	 * Even if the PM domain is powered off at this point, we can't expect
-	 * it to remain in that state during the entire system PM suspend
-	 * phase. Any subsystem/driver for a device in the PM domain, may still
-	 * need to serve a request which may require the device to be runtime
-	 * resumed and its PM domain to be powered.
-	 *
-	 * As we are disabling runtime PM at this point, we are preventing the
-	 * subsystem/driver to decide themselves. For that reason, we need to
-	 * make sure the device is operational as it may be required in some
-	 * cases.
-	 */
-	pm_runtime_resume(dev);
-	__pm_runtime_disable(dev, false);
-
 	ret = pm_generic_prepare(dev);
 	if (ret) {
 		mutex_lock(&genpd->lock);
@@ -761,7 +746,6 @@ static int pm_genpd_prepare(struct device *dev)
 		genpd->prepared_count--;
 
 		mutex_unlock(&genpd->lock);
-		pm_runtime_enable(dev);
 	}
 
 	return ret;
@@ -777,6 +761,7 @@ static int pm_genpd_prepare(struct device *dev)
 static int pm_genpd_suspend_noirq(struct device *dev)
 {
 	struct generic_pm_domain *genpd;
+	int ret;
 
 	dev_dbg(dev, "%s()\n", __func__);
 
@@ -787,7 +772,11 @@ static int pm_genpd_suspend_noirq(struct device *dev)
 	if (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev))
 		return 0;
 
-	genpd_stop_dev(genpd, dev);
+	if (genpd->dev_ops.stop && genpd->dev_ops.start) {
+		ret = pm_runtime_force_suspend(dev);
+		if (ret)
+			return ret;
+	}
 
 	/*
 	 * Since all of the "noirq" callbacks are executed sequentially, it is
@@ -809,6 +798,7 @@ static int pm_genpd_suspend_noirq(struct device *dev)
 static int pm_genpd_resume_noirq(struct device *dev)
 {
 	struct generic_pm_domain *genpd;
+	int ret = 0;
 
 	dev_dbg(dev, "%s()\n", __func__);
 
@@ -827,7 +817,10 @@ static int pm_genpd_resume_noirq(struct device *dev)
 	pm_genpd_sync_poweron(genpd, true);
 	genpd->suspended_count--;
 
-	return genpd_start_dev(genpd, dev);
+	if (genpd->dev_ops.stop && genpd->dev_ops.start)
+		ret = pm_runtime_force_resume(dev);
+
+	return ret;
 }
 
 /**
@@ -842,6 +835,7 @@ static int pm_genpd_resume_noirq(struct device *dev)
 static int pm_genpd_freeze_noirq(struct device *dev)
 {
 	struct generic_pm_domain *genpd;
+	int ret = 0;
 
 	dev_dbg(dev, "%s()\n", __func__);
 
@@ -849,7 +843,10 @@ static int pm_genpd_freeze_noirq(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	return genpd_stop_dev(genpd, dev);
+	if (genpd->dev_ops.stop && genpd->dev_ops.start)
+		ret = pm_runtime_force_suspend(dev);
+
+	return ret;
 }
 
 /**
@@ -862,6 +859,7 @@ static int pm_genpd_freeze_noirq(struct device *dev)
 static int pm_genpd_thaw_noirq(struct device *dev)
 {
 	struct generic_pm_domain *genpd;
+	int ret = 0;
 
 	dev_dbg(dev, "%s()\n", __func__);
 
@@ -869,7 +867,10 @@ static int pm_genpd_thaw_noirq(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	return genpd_start_dev(genpd, dev);
+	if (genpd->dev_ops.stop && genpd->dev_ops.start)
+		ret = pm_runtime_force_resume(dev);
+
+	return ret;
 }
 
 /**
@@ -882,6 +883,7 @@ static int pm_genpd_thaw_noirq(struct device *dev)
 static int pm_genpd_restore_noirq(struct device *dev)
 {
 	struct generic_pm_domain *genpd;
+	int ret = 0;
 
 	dev_dbg(dev, "%s()\n", __func__);
 
@@ -907,7 +909,10 @@ static int pm_genpd_restore_noirq(struct device *dev)
 
 	pm_genpd_sync_poweron(genpd, true);
 
-	return genpd_start_dev(genpd, dev);
+	if (genpd->dev_ops.stop && genpd->dev_ops.start)
+		ret = pm_runtime_force_resume(dev);
+
+	return ret;
 }
 
 /**
@@ -929,15 +934,15 @@ static void pm_genpd_complete(struct device *dev)
 	if (IS_ERR(genpd))
 		return;
 
+	pm_generic_complete(dev);
+
 	mutex_lock(&genpd->lock);
 
 	genpd->prepared_count--;
+	if (!genpd->prepared_count)
+		genpd_queue_power_off_work(genpd);
 
 	mutex_unlock(&genpd->lock);
-
-	pm_generic_complete(dev);
-	pm_runtime_set_active(dev);
-	pm_runtime_enable(dev);
 }
 
 /**
-- 
1.9.1

Re: [PATCH 3/4] PM / Domains: Allow runtime PM during system PM phases

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> The PM core disables runtime PM at __device_suspend_late() before it calls
> a system PM "late" callback for the device. When resuming the device,
> after the corresponding "early" callback has been invoked, it re-enables
> runtime PM.
>
> By changing genpd to conform to this behaviour, the device no longer have
> to be unconditionally runtime resumed from genpd's ->prepare() callback. In
> most cases that avoids unnecessary operations.

...avoids the unnecessary, and energy-wasting operaions of runtime
resuming devices that have nothing to do, only to runtime suspend them
shortly after.

> As runtime PM then isn't disabled/enabled by genpd, the subsystem/driver
> can rely on the generic behaviour from PM core. Consequentially runtime PM
> is allowed in more phases of system PM than before.

OK.  I like this change a lot, but it the changelog doesn't go into
describing how this gets around the problems that were being worked
around by having genpd disable runtime PM in the first place.  This
changelog should describe how this appraoch deals with those problems,
or why they don't exist anymore.

> Although, because of this change and due to that genpd powers on the PM
> domain unconditionally in the system PM resume "noirq" phase, it could
> potentially cause a PM domain to stay powered on even if it's unused after
> the system has resumed. To avoid this, let's schedule a power off work
> when genpd's system PM ->complete() callback has been invoked for the last
> device in the PM domain.

OK.

> Another issue that arises due to this change in genpd, concerns those
> platforms/PM domains that makes use of genpd's device ->stop|start()
> callbacks. In these scenarios, the corresponding subsystem/driver needs to
> invoke pm_runtime_force_suspend() from a system PM suspend callback to
> allow genpd's ->runtime_suspend() to be invoked for an active device, else
> genpd can't "stop" a device that is "started".
>
> The subsystem/driver also needs to invoke pm_runtime_force_resume()
> in a system PM resume callback, to restore the runtime PM state for the
> device and to re-enable runtime PM.
>
> Currently not all involved subsystem/drivers makes use of
> pm_runtime_force_suspend|resume() accordingly. Therefore, let's invoke
> pm_runtime_force_suspend|resume() from genpd's "noirq" system PM
> callbacks, in cases when the ->stop|start() callbacks are being used.
>
> In this way, devices are "stoped" during suspend and "started" during
> resume, even in those cases when the subsystem/driver don't call
> pm_runtime_force_suspend|resume() themselves.

IMO, this last part might be better dealt with as a separate patch, so
it's clear which parts are for the main genpd change, and which parts
fixup the fallout.

On a related note, are there many genpd drivers using dev_ops->start and
->stop out there?  A quick grep didn't find anything other than the
pm_clk stuff.

Kevin

Re: [PATCH 3/4] PM / Domains: Allow runtime PM during system PM phases

[Ulf Hansson]

On 24 May 2016 at 01:06, Kevin Hilman <khilman@baylibre.com> wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:
>
>> The PM core disables runtime PM at __device_suspend_late() before it calls
>> a system PM "late" callback for the device. When resuming the device,
>> after the corresponding "early" callback has been invoked, it re-enables
>> runtime PM.
>>
>> By changing genpd to conform to this behaviour, the device no longer have
>> to be unconditionally runtime resumed from genpd's ->prepare() callback. In
>> most cases that avoids unnecessary operations.
>
> ...avoids the unnecessary, and energy-wasting operaions of runtime
> resuming devices that have nothing to do, only to runtime suspend them
> shortly after.

Perfect, I will update.

>
>> As runtime PM then isn't disabled/enabled by genpd, the subsystem/driver
>> can rely on the generic behaviour from PM core. Consequentially runtime PM
>> is allowed in more phases of system PM than before.
>
> OK.  I like this change a lot, but it the changelog doesn't go into
> describing how this gets around the problems that were being worked
> around by having genpd disable runtime PM in the first place.  This
> changelog should describe how this appraoch deals with those problems,
> or why they don't exist anymore.

OK, I will try to elaborate a bit on this.

>
>> Although, because of this change and due to that genpd powers on the PM
>> domain unconditionally in the system PM resume "noirq" phase, it could
>> potentially cause a PM domain to stay powered on even if it's unused after
>> the system has resumed. To avoid this, let's schedule a power off work
>> when genpd's system PM ->complete() callback has been invoked for the last
>> device in the PM domain.
>
> OK.
>
>> Another issue that arises due to this change in genpd, concerns those
>> platforms/PM domains that makes use of genpd's device ->stop|start()
>> callbacks. In these scenarios, the corresponding subsystem/driver needs to
>> invoke pm_runtime_force_suspend() from a system PM suspend callback to
>> allow genpd's ->runtime_suspend() to be invoked for an active device, else
>> genpd can't "stop" a device that is "started".
>>
>> The subsystem/driver also needs to invoke pm_runtime_force_resume()
>> in a system PM resume callback, to restore the runtime PM state for the
>> device and to re-enable runtime PM.
>>
>> Currently not all involved subsystem/drivers makes use of
>> pm_runtime_force_suspend|resume() accordingly. Therefore, let's invoke
>> pm_runtime_force_suspend|resume() from genpd's "noirq" system PM
>> callbacks, in cases when the ->stop|start() callbacks are being used.
>>
>> In this way, devices are "stoped" during suspend and "started" during
>> resume, even in those cases when the subsystem/driver don't call
>> pm_runtime_force_suspend|resume() themselves.
>
> IMO, this last part might be better dealt with as a separate patch, so
> it's clear which parts are for the main genpd change, and which parts
> fixup the fallout.

I do that!

>
> On a related note, are there many genpd drivers using dev_ops->start and
> ->stop out there?  A quick grep didn't find anything other than the
> pm_clk stuff.
>
> Kevin

Currently it's only the pm_clk stuff. Although that is quite
extensively used among Renesas SoCs.

Thanks for reviewing!

Kind regards
Uffe

[PATCH 4/4] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Ulf Hansson]

When the pm_runtime_force_suspend|resume() helpers were invented, we still
had CONFIG_PM_RUNTIME and CONFIG_PM_SLEEP as separate Kconfig options.

To make sure these helpers worked for all combinations and without
introducing too much of complexity, the device was always resumed in
pm_runtime_force_resume().

More precisely, when CONFIG_PM_SLEEP was set and CONFIG_PM_RUNTIME was
unset, we needed to resume the device as the subsystem/driver couldn't
rely on using runtime PM to do it.

As the CONFIG_PM_RUNTIME option was merged into CONFIG_PM a while ago, it
removed this combination, of using CONFIG_PM_SLEEP without the earlier
CONFIG_PM_RUNTIME.

For this reason we can now rely on the subsystem/driver to use runtime PM
to resume the device, instead of forcing that to be done in all cases. In
other words, let's defer this to a later point when it's actually needed.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/runtime.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 09e4eb1..1db7b46 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1509,6 +1509,17 @@ int pm_runtime_force_resume(struct device *dev)
 	if (!pm_runtime_status_suspended(dev))
 		goto out;
 
+	/*
+	 * The PM core increases the runtime PM usage count in the system PM
+	 * prepare phase. If the count is greather than 1 at this point, someone
+	 * else has also increased it. In that case, invoke the runtime resume
+	 * callback for the device as that is likely what is expected. In other
+	 * case we trust the subsystem/driver to runtime resume the device when
+	 * it's actually needed.
+	 */
+	if (atomic_read(&dev->power.usage_count) < 2)
+		goto out;
+
 	ret = pm_runtime_set_active(dev);
 	if (ret)
 		goto out;
-- 
1.9.1

Re: [PATCH 4/4] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> When the pm_runtime_force_suspend|resume() helpers were invented, we still
> had CONFIG_PM_RUNTIME and CONFIG_PM_SLEEP as separate Kconfig options.
>
> To make sure these helpers worked for all combinations and without
> introducing too much of complexity, the device was always resumed in
> pm_runtime_force_resume().
>
> More precisely, when CONFIG_PM_SLEEP was set and CONFIG_PM_RUNTIME was
> unset, we needed to resume the device as the subsystem/driver couldn't
> rely on using runtime PM to do it.
>
> As the CONFIG_PM_RUNTIME option was merged into CONFIG_PM a while ago, it
> removed this combination, of using CONFIG_PM_SLEEP without the earlier
> CONFIG_PM_RUNTIME.
>
> For this reason we can now rely on the subsystem/driver to use runtime PM
> to resume the device, instead of forcing that to be done in all cases. In
> other words, let's defer this to a later point when it's actually needed.

s/this/the runtime resume/

>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/base/power/runtime.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 09e4eb1..1db7b46 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1509,6 +1509,17 @@ int pm_runtime_force_resume(struct device *dev)
>  	if (!pm_runtime_status_suspended(dev))
>  		goto out;
>  
> +	/*
> +	 * The PM core increases the runtime PM usage count in the system PM
> +	 * prepare phase. If the count is greather than 1 at this point, someone

s/greather/greater/

> +	 * else has also increased it. In that case, invoke the runtime resume
> +	 * callback for the device as that is likely what is expected. In other

Instead of "someone else..."

...a real user (such as a driver or subsystem) has also increased it,
indicating that the device was active (RPM_ACTIVE) when system suspend
was invoked.  Since the device was active on suspend, it's expected to
be used on resume as well, so invoke runtime resume for that device
ensuring that it is RPM_ACTIVE during system resume.

Kevin

> +	 * case we trust the subsystem/driver to runtime resume the device when
> +	 * it's actually needed.
> +	 */
> +	if (atomic_read(&dev->power.usage_count) < 2)
> +		goto out;
> +
>  	ret = pm_runtime_set_active(dev);
>  	if (ret)
>  		goto out;

Re: [PATCH 4/4] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Ulf Hansson]

On 24 May 2016 at 01:15, Kevin Hilman <khilman@baylibre.com> wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:
>
>> When the pm_runtime_force_suspend|resume() helpers were invented, we still
>> had CONFIG_PM_RUNTIME and CONFIG_PM_SLEEP as separate Kconfig options.
>>
>> To make sure these helpers worked for all combinations and without
>> introducing too much of complexity, the device was always resumed in
>> pm_runtime_force_resume().
>>
>> More precisely, when CONFIG_PM_SLEEP was set and CONFIG_PM_RUNTIME was
>> unset, we needed to resume the device as the subsystem/driver couldn't
>> rely on using runtime PM to do it.
>>
>> As the CONFIG_PM_RUNTIME option was merged into CONFIG_PM a while ago, it
>> removed this combination, of using CONFIG_PM_SLEEP without the earlier
>> CONFIG_PM_RUNTIME.
>>
>> For this reason we can now rely on the subsystem/driver to use runtime PM
>> to resume the device, instead of forcing that to be done in all cases. In
>> other words, let's defer this to a later point when it's actually needed.
>
> s/this/the runtime resume/
>
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/base/power/runtime.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>> index 09e4eb1..1db7b46 100644
>> --- a/drivers/base/power/runtime.c
>> +++ b/drivers/base/power/runtime.c
>> @@ -1509,6 +1509,17 @@ int pm_runtime_force_resume(struct device *dev)
>>       if (!pm_runtime_status_suspended(dev))
>>               goto out;
>>
>> +     /*
>> +      * The PM core increases the runtime PM usage count in the system PM
>> +      * prepare phase. If the count is greather than 1 at this point, someone
>
> s/greather/greater/
>
>> +      * else has also increased it. In that case, invoke the runtime resume
>> +      * callback for the device as that is likely what is expected. In other
>
> Instead of "someone else..."
>
> ...a real user (such as a driver or subsystem) has also increased it,
> indicating that the device was active (RPM_ACTIVE) when system suspend
> was invoked.  Since the device was active on suspend, it's expected to
> be used on resume as well, so invoke runtime resume for that device
> ensuring that it is RPM_ACTIVE during system resume.

"someone else" also takes user space, child devices, etc into account.
There are just quite many cases when the usage count could be
increased and I didn't want to be too specific about them.

Although, if you insists that this improves the understanding, I will
update the comment.

[...]

Kind regards
Uffe

Re: [PATCH 4/4] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> On 24 May 2016 at 01:15, Kevin Hilman <khilman@baylibre.com> wrote:
>> Ulf Hansson <ulf.hansson@linaro.org> writes:
>>
>>> When the pm_runtime_force_suspend|resume() helpers were invented, we still
>>> had CONFIG_PM_RUNTIME and CONFIG_PM_SLEEP as separate Kconfig options.
>>>
>>> To make sure these helpers worked for all combinations and without
>>> introducing too much of complexity, the device was always resumed in
>>> pm_runtime_force_resume().
>>>
>>> More precisely, when CONFIG_PM_SLEEP was set and CONFIG_PM_RUNTIME was
>>> unset, we needed to resume the device as the subsystem/driver couldn't
>>> rely on using runtime PM to do it.
>>>
>>> As the CONFIG_PM_RUNTIME option was merged into CONFIG_PM a while ago, it
>>> removed this combination, of using CONFIG_PM_SLEEP without the earlier
>>> CONFIG_PM_RUNTIME.
>>>
>>> For this reason we can now rely on the subsystem/driver to use runtime PM
>>> to resume the device, instead of forcing that to be done in all cases. In
>>> other words, let's defer this to a later point when it's actually needed.
>>
>> s/this/the runtime resume/
>>
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>  drivers/base/power/runtime.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>>> index 09e4eb1..1db7b46 100644
>>> --- a/drivers/base/power/runtime.c
>>> +++ b/drivers/base/power/runtime.c
>>> @@ -1509,6 +1509,17 @@ int pm_runtime_force_resume(struct device *dev)
>>>       if (!pm_runtime_status_suspended(dev))
>>>               goto out;
>>>
>>> +     /*
>>> +      * The PM core increases the runtime PM usage count in the system PM
>>> +      * prepare phase. If the count is greather than 1 at this point, someone
>>
>> s/greather/greater/
>>
>>> +      * else has also increased it. In that case, invoke the runtime resume
>>> +      * callback for the device as that is likely what is expected. In other
>>
>> Instead of "someone else..."
>>
>> ...a real user (such as a driver or subsystem) has also increased it,
>> indicating that the device was active (RPM_ACTIVE) when system suspend
>> was invoked.  Since the device was active on suspend, it's expected to
>> be used on resume as well, so invoke runtime resume for that device
>> ensuring that it is RPM_ACTIVE during system resume.
>
> "someone else" also takes user space, child devices, etc into account.
> There are just quite many cases when the usage count could be
> increased and I didn't want to be too specific about them.
>
> Although, if you insists that this improves the understanding, I will
> update the comment.

How about: a real user (such as a subsystem, driver, userspace, etc.)

Kevin

Re: [PATCH 4/4] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Geert Uytterhoeven]

Hi Ulf, Rafael,

On Tue, May 17, 2016 at 1:41 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> When the pm_runtime_force_suspend|resume() helpers were invented, we still
> had CONFIG_PM_RUNTIME and CONFIG_PM_SLEEP as separate Kconfig options.
>
> To make sure these helpers worked for all combinations and without
> introducing too much of complexity, the device was always resumed in
> pm_runtime_force_resume().
>
> More precisely, when CONFIG_PM_SLEEP was set and CONFIG_PM_RUNTIME was
> unset, we needed to resume the device as the subsystem/driver couldn't
> rely on using runtime PM to do it.
>
> As the CONFIG_PM_RUNTIME option was merged into CONFIG_PM a while ago, it
> removed this combination, of using CONFIG_PM_SLEEP without the earlier
> CONFIG_PM_RUNTIME.
>
> For this reason we can now rely on the subsystem/driver to use runtime PM
> to resume the device, instead of forcing that to be done in all cases. In
> other words, let's defer this to a later point when it's actually needed.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/base/power/runtime.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 09e4eb1..1db7b46 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1509,6 +1509,17 @@ int pm_runtime_force_resume(struct device *dev)
>         if (!pm_runtime_status_suspended(dev))
>                 goto out;
>
> +       /*
> +        * The PM core increases the runtime PM usage count in the system PM
> +        * prepare phase. If the count is greather than 1 at this point, someone
> +        * else has also increased it. In that case, invoke the runtime resume
> +        * callback for the device as that is likely what is expected. In other
> +        * case we trust the subsystem/driver to runtime resume the device when
> +        * it's actually needed.
> +        */
> +       if (atomic_read(&dev->power.usage_count) < 2)
> +               goto out;
> +
>         ret = pm_runtime_set_active(dev);
>         if (ret)
>                 goto out;

This patch (commit eb13a0a1b6d5d5c2 in pm/linux-next) breaks resume on
sh73a0/kzm9g and r8a73a4/ape6evm. On these boards, the Ethernet controller is a
child of a local bus (bsc), whose clock (zb) is controlled through pm_clk and
simple-pm-bus, cfr.

    arch/arm/boot/dts/r8a73a4-ape6evm.dts
    arch/arm/boot/dts/r8a73a4.dtsi
    arch/arm/boot/dts/sh73a0-kzm9g.dts
    arch/arm/boot/dts/sh73a0.dtsi

During resume, the bus clock is not enabled, causing an imprecise abort
when accessing the Ethernet controller's registers. With some debug code
added:

    PM: Syncing filesystems ... done.
    PM: Preparing system for sleep (mem)
    Freezing user space processes ... (elapsed 0.001 seconds) done.
    Double checking all user space processes after OOM killer
disable... (elapsed 0.000 seconds)
    Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
    PM: Suspending system (mem)
    smsc911x: smsc911x_suspend
    simple-pm-bus fec10000.bus: simple_pm_bus_suspend
    PM: suspend of devices complete after 21.484 msecs
    PM: suspend devices took 0.030 seconds
    PM: late suspend of devices complete after 0.488 msecs
    cpg_div6_clock_disable: zb
    rmobile_pd_power_down: a3sp
    PM: noirq suspend of devices complete after 8.300 msecs
    Disabling non-boot CPUs ...
    CPU1: shutdown

    PM: early resume of devices complete after 0.488 msecs
    simple-pm-bus fec10000.bus: simple_pm_bus_resume
    smsc911x: smsc911x_resume
    Unhandled fault: imprecise external abort (0x1406) at 0xb6f40068
    pgd = deedc000
    [b6f40068] *pgd=5f774831, *pte=4087659f, *ppte=40876e7e
    Internal error: : 1406 [#1] SMP ARM
    Modules linked in:
    CPU: 0 PID: 1062 Comm: s2ram Not tainted
4.7.0-rc5-kzm9g-00391-g87777f5105e9303a-dirty #765
    Hardware name: Generic SH73A0 (Flattened Device Tree)
    task: dedc0840 ti: deee6000 task.ti: deee6000
    PC is at __smsc911x_reg_read+0x1c/0x60
    LR is at smsc911x_resume+0x78/0xd0

After reverting this patch, resume succeeds again, as the zb clock is enabled
first:

    PM: Syncing filesystems ... done.
    PM: Preparing system for sleep (mem)
    Freezing user space processes ... (elapsed 0.001 seconds) done.
    Double checking all user space processes after OOM killer
disable... (elapsed 0.000 seconds)
    Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
    PM: Suspending system (mem)
    smsc911x: smsc911x_suspend
    simple-pm-bus fec10000.bus: simple_pm_bus_suspend
    PM: suspend of devices complete after 22.460 msecs
    PM: suspend devices took 0.030 seconds
    PM: late suspend of devices complete after 0.488 msecs
    cpg_div6_clock_disable: zb
    rmobile_pd_power_down: a3sp
    PM: noirq suspend of devices complete after 8.300 msecs
    Disabling non-boot CPUs ...
    CPU1: shutdown

    Enabling non-boot CPUs ...
    CPU1 is up
    rmobile_pd_power_up: a3sp
    rmobile_pd_power_up: a4mp
    a4mp: Power on, 0x00000100 -> PSTR = 0x007b3133
    cpg_div6_clock_enable: zb
    PM: noirq resume of devices complete after 92.773 msecs
    PM: early resume of devices complete after 0.488 msecs
    simple-pm-bus fec10000.bus: simple_pm_bus_resume
    smsc911x: smsc911x_resume
    PM: resume of devices complete after 28.564 msecs
    PM: resume devices took 0.040 seconds
    PM: Finishing wakeup.
    Restarting tasks ...
    rmobile_pd_power_down: a4mp
    a4mp: Power off, 0x00000100 -> PSTR = 0x007b3033
    done.

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

Re: [PATCH 4/4] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Rafael J. Wysocki]

On Tuesday, June 28, 2016 06:26:36 PM Geert Uytterhoeven wrote:
> Hi Ulf, Rafael,
> 
> On Tue, May 17, 2016 at 1:41 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > When the pm_runtime_force_suspend|resume() helpers were invented, we still
> > had CONFIG_PM_RUNTIME and CONFIG_PM_SLEEP as separate Kconfig options.
> >
> > To make sure these helpers worked for all combinations and without
> > introducing too much of complexity, the device was always resumed in
> > pm_runtime_force_resume().
> >
> > More precisely, when CONFIG_PM_SLEEP was set and CONFIG_PM_RUNTIME was
> > unset, we needed to resume the device as the subsystem/driver couldn't
> > rely on using runtime PM to do it.
> >
> > As the CONFIG_PM_RUNTIME option was merged into CONFIG_PM a while ago, it
> > removed this combination, of using CONFIG_PM_SLEEP without the earlier
> > CONFIG_PM_RUNTIME.
> >
> > For this reason we can now rely on the subsystem/driver to use runtime PM
> > to resume the device, instead of forcing that to be done in all cases. In
> > other words, let's defer this to a later point when it's actually needed.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/base/power/runtime.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index 09e4eb1..1db7b46 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -1509,6 +1509,17 @@ int pm_runtime_force_resume(struct device *dev)
> >         if (!pm_runtime_status_suspended(dev))
> >                 goto out;
> >
> > +       /*
> > +        * The PM core increases the runtime PM usage count in the system PM
> > +        * prepare phase. If the count is greather than 1 at this point, someone
> > +        * else has also increased it. In that case, invoke the runtime resume
> > +        * callback for the device as that is likely what is expected. In other
> > +        * case we trust the subsystem/driver to runtime resume the device when
> > +        * it's actually needed.
> > +        */
> > +       if (atomic_read(&dev->power.usage_count) < 2)
> > +               goto out;
> > +
> >         ret = pm_runtime_set_active(dev);
> >         if (ret)
> >                 goto out;
> 
> This patch (commit eb13a0a1b6d5d5c2 in pm/linux-next) breaks resume on
> sh73a0/kzm9g and r8a73a4/ape6evm. On these boards, the Ethernet controller is a
> child of a local bus (bsc), whose clock (zb) is controlled through pm_clk and
> simple-pm-bus, cfr.
> 
>     arch/arm/boot/dts/r8a73a4-ape6evm.dts
>     arch/arm/boot/dts/r8a73a4.dtsi
>     arch/arm/boot/dts/sh73a0-kzm9g.dts
>     arch/arm/boot/dts/sh73a0.dtsi
> 
> During resume, the bus clock is not enabled, causing an imprecise abort
> when accessing the Ethernet controller's registers. With some debug code
> added:
> 
>     PM: Syncing filesystems ... done.
>     PM: Preparing system for sleep (mem)
>     Freezing user space processes ... (elapsed 0.001 seconds) done.
>     Double checking all user space processes after OOM killer
> disable... (elapsed 0.000 seconds)
>     Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
>     PM: Suspending system (mem)
>     smsc911x: smsc911x_suspend
>     simple-pm-bus fec10000.bus: simple_pm_bus_suspend
>     PM: suspend of devices complete after 21.484 msecs
>     PM: suspend devices took 0.030 seconds
>     PM: late suspend of devices complete after 0.488 msecs
>     cpg_div6_clock_disable: zb
>     rmobile_pd_power_down: a3sp
>     PM: noirq suspend of devices complete after 8.300 msecs
>     Disabling non-boot CPUs ...
>     CPU1: shutdown
> 
>     PM: early resume of devices complete after 0.488 msecs
>     simple-pm-bus fec10000.bus: simple_pm_bus_resume
>     smsc911x: smsc911x_resume
>     Unhandled fault: imprecise external abort (0x1406) at 0xb6f40068
>     pgd = deedc000
>     [b6f40068] *pgd=5f774831, *pte=4087659f, *ppte=40876e7e
>     Internal error: : 1406 [#1] SMP ARM
>     Modules linked in:
>     CPU: 0 PID: 1062 Comm: s2ram Not tainted
> 4.7.0-rc5-kzm9g-00391-g87777f5105e9303a-dirty #765
>     Hardware name: Generic SH73A0 (Flattened Device Tree)
>     task: dedc0840 ti: deee6000 task.ti: deee6000
>     PC is at __smsc911x_reg_read+0x1c/0x60
>     LR is at smsc911x_resume+0x78/0xd0
> 
> After reverting this patch, resume succeeds again, as the zb clock is enabled
> first:
> 
>     PM: Syncing filesystems ... done.
>     PM: Preparing system for sleep (mem)
>     Freezing user space processes ... (elapsed 0.001 seconds) done.
>     Double checking all user space processes after OOM killer
> disable... (elapsed 0.000 seconds)
>     Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
>     PM: Suspending system (mem)
>     smsc911x: smsc911x_suspend
>     simple-pm-bus fec10000.bus: simple_pm_bus_suspend
>     PM: suspend of devices complete after 22.460 msecs
>     PM: suspend devices took 0.030 seconds
>     PM: late suspend of devices complete after 0.488 msecs
>     cpg_div6_clock_disable: zb
>     rmobile_pd_power_down: a3sp
>     PM: noirq suspend of devices complete after 8.300 msecs
>     Disabling non-boot CPUs ...
>     CPU1: shutdown
> 
>     Enabling non-boot CPUs ...
>     CPU1 is up
>     rmobile_pd_power_up: a3sp
>     rmobile_pd_power_up: a4mp
>     a4mp: Power on, 0x00000100 -> PSTR = 0x007b3133
>     cpg_div6_clock_enable: zb
>     PM: noirq resume of devices complete after 92.773 msecs
>     PM: early resume of devices complete after 0.488 msecs
>     simple-pm-bus fec10000.bus: simple_pm_bus_resume
>     smsc911x: smsc911x_resume
>     PM: resume of devices complete after 28.564 msecs
>     PM: resume devices took 0.040 seconds
>     PM: Finishing wakeup.
>     Restarting tasks ...
>     rmobile_pd_power_down: a4mp
>     a4mp: Power off, 0x00000100 -> PSTR = 0x007b3033
>     done.

OK, I've dropped the offending commit from my linux-next branch.

Thanks,
Rafael

Re: [PATCH 4/4] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Ulf Hansson]

On 28 June 2016 at 18:26, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf, Rafael,
>
> On Tue, May 17, 2016 at 1:41 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> When the pm_runtime_force_suspend|resume() helpers were invented, we still
>> had CONFIG_PM_RUNTIME and CONFIG_PM_SLEEP as separate Kconfig options.
>>
>> To make sure these helpers worked for all combinations and without
>> introducing too much of complexity, the device was always resumed in
>> pm_runtime_force_resume().
>>
>> More precisely, when CONFIG_PM_SLEEP was set and CONFIG_PM_RUNTIME was
>> unset, we needed to resume the device as the subsystem/driver couldn't
>> rely on using runtime PM to do it.
>>
>> As the CONFIG_PM_RUNTIME option was merged into CONFIG_PM a while ago, it
>> removed this combination, of using CONFIG_PM_SLEEP without the earlier
>> CONFIG_PM_RUNTIME.
>>
>> For this reason we can now rely on the subsystem/driver to use runtime PM
>> to resume the device, instead of forcing that to be done in all cases. In
>> other words, let's defer this to a later point when it's actually needed.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/base/power/runtime.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>> index 09e4eb1..1db7b46 100644
>> --- a/drivers/base/power/runtime.c
>> +++ b/drivers/base/power/runtime.c
>> @@ -1509,6 +1509,17 @@ int pm_runtime_force_resume(struct device *dev)
>>         if (!pm_runtime_status_suspended(dev))
>>                 goto out;
>>
>> +       /*
>> +        * The PM core increases the runtime PM usage count in the system PM
>> +        * prepare phase. If the count is greather than 1 at this point, someone
>> +        * else has also increased it. In that case, invoke the runtime resume
>> +        * callback for the device as that is likely what is expected. In other
>> +        * case we trust the subsystem/driver to runtime resume the device when
>> +        * it's actually needed.
>> +        */
>> +       if (atomic_read(&dev->power.usage_count) < 2)
>> +               goto out;
>> +
>>         ret = pm_runtime_set_active(dev);
>>         if (ret)
>>                 goto out;
>
> This patch (commit eb13a0a1b6d5d5c2 in pm/linux-next) breaks resume on

Thanks for reporting!

> sh73a0/kzm9g and r8a73a4/ape6evm. On these boards, the Ethernet controller is a
> child of a local bus (bsc), whose clock (zb) is controlled through pm_clk and
> simple-pm-bus, cfr.
>
>     arch/arm/boot/dts/r8a73a4-ape6evm.dts
>     arch/arm/boot/dts/r8a73a4.dtsi
>     arch/arm/boot/dts/sh73a0-kzm9g.dts
>     arch/arm/boot/dts/sh73a0.dtsi
>
> During resume, the bus clock is not enabled, causing an imprecise abort
> when accessing the Ethernet controller's registers. With some debug code
> added:

I was looking into this in some more detail.

As you are stating that Ethernet device is a child device to the local
bus device, I would have expected that the pm_runtime_get_sync()
invoked during ->probe() in the Ethernet driver should cause its
parent device (the bus device) to be "forced" resumed.

That's because the pm_runtime_get_sync() should trigger an increased
usage count of the parent device. Later, when
pm_runtime_force_resume() validates the count for the bus device, it
should be 2 and thus it should lead to that it decides to *not* defer
the resume of the device. Apparently this isn't happening for some
reason...

Perhaps there's a pm_suspend_ignore_children() set for the parent?
Or perhaps the parent/child relationship isn't set up correctly?

>
>     PM: Syncing filesystems ... done.
>     PM: Preparing system for sleep (mem)
>     Freezing user space processes ... (elapsed 0.001 seconds) done.
>     Double checking all user space processes after OOM killer
> disable... (elapsed 0.000 seconds)
>     Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
>     PM: Suspending system (mem)
>     smsc911x: smsc911x_suspend
>     simple-pm-bus fec10000.bus: simple_pm_bus_suspend
>     PM: suspend of devices complete after 21.484 msecs
>     PM: suspend devices took 0.030 seconds
>     PM: late suspend of devices complete after 0.488 msecs
>     cpg_div6_clock_disable: zb
>     rmobile_pd_power_down: a3sp
>     PM: noirq suspend of devices complete after 8.300 msecs
>     Disabling non-boot CPUs ...
>     CPU1: shutdown
>
>     PM: early resume of devices complete after 0.488 msecs
>     simple-pm-bus fec10000.bus: simple_pm_bus_resume
>     smsc911x: smsc911x_resume
>     Unhandled fault: imprecise external abort (0x1406) at 0xb6f40068
>     pgd = deedc000
>     [b6f40068] *pgd=5f774831, *pte=4087659f, *ppte=40876e7e
>     Internal error: : 1406 [#1] SMP ARM
>     Modules linked in:
>     CPU: 0 PID: 1062 Comm: s2ram Not tainted
> 4.7.0-rc5-kzm9g-00391-g87777f5105e9303a-dirty #765
>     Hardware name: Generic SH73A0 (Flattened Device Tree)
>     task: dedc0840 ti: deee6000 task.ti: deee6000
>     PC is at __smsc911x_reg_read+0x1c/0x60
>     LR is at smsc911x_resume+0x78/0xd0
>
> After reverting this patch, resume succeeds again, as the zb clock is enabled
> first:
>
>     PM: Syncing filesystems ... done.
>     PM: Preparing system for sleep (mem)
>     Freezing user space processes ... (elapsed 0.001 seconds) done.
>     Double checking all user space processes after OOM killer
> disable... (elapsed 0.000 seconds)
>     Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
>     PM: Suspending system (mem)
>     smsc911x: smsc911x_suspend
>     simple-pm-bus fec10000.bus: simple_pm_bus_suspend
>     PM: suspend of devices complete after 22.460 msecs
>     PM: suspend devices took 0.030 seconds
>     PM: late suspend of devices complete after 0.488 msecs
>     cpg_div6_clock_disable: zb
>     rmobile_pd_power_down: a3sp
>     PM: noirq suspend of devices complete after 8.300 msecs
>     Disabling non-boot CPUs ...
>     CPU1: shutdown
>
>     Enabling non-boot CPUs ...
>     CPU1 is up
>     rmobile_pd_power_up: a3sp
>     rmobile_pd_power_up: a4mp
>     a4mp: Power on, 0x00000100 -> PSTR = 0x007b3133
>     cpg_div6_clock_enable: zb
>     PM: noirq resume of devices complete after 92.773 msecs
>     PM: early resume of devices complete after 0.488 msecs
>     simple-pm-bus fec10000.bus: simple_pm_bus_resume
>     smsc911x: smsc911x_resume
>     PM: resume of devices complete after 28.564 msecs
>     PM: resume devices took 0.040 seconds
>     PM: Finishing wakeup.
>     Restarting tasks ...
>     rmobile_pd_power_down: a4mp
>     a4mp: Power off, 0x00000100 -> PSTR = 0x007b3033
>     done.

Thanks for the detailed explanation of what is happening!

Unless we can fix this in the Ethernet driver/local bus, I have some
ideas of how we could change genpd, to still allow $subject patch to
be applied. Although I will wait a day or two before I propose
something. In the meantime I will also try to collect some more
information.

Thanks!

Kind regards
Uffe

Re: [PATCH 4/4] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Geert Uytterhoeven]

Hi Ulf,

On Wed, Jun 29, 2016 at 7:30 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 28 June 2016 at 18:26, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> This patch (commit eb13a0a1b6d5d5c2 in pm/linux-next) breaks resume on
>
> Thanks for reporting!
>
>> sh73a0/kzm9g and r8a73a4/ape6evm. On these boards, the Ethernet controller is a
>> child of a local bus (bsc), whose clock (zb) is controlled through pm_clk and
>> simple-pm-bus, cfr.
>>
>>     arch/arm/boot/dts/r8a73a4-ape6evm.dts
>>     arch/arm/boot/dts/r8a73a4.dtsi
>>     arch/arm/boot/dts/sh73a0-kzm9g.dts
>>     arch/arm/boot/dts/sh73a0.dtsi
>>
>> During resume, the bus clock is not enabled, causing an imprecise abort
>> when accessing the Ethernet controller's registers. With some debug code
>> added:
>
> I was looking into this in some more detail.
>
> As you are stating that Ethernet device is a child device to the local
> bus device, I would have expected that the pm_runtime_get_sync()
> invoked during ->probe() in the Ethernet driver should cause its
> parent device (the bus device) to be "forced" resumed.
>
> That's because the pm_runtime_get_sync() should trigger an increased
> usage count of the parent device. Later, when
> pm_runtime_force_resume() validates the count for the bus device, it
> should be 2 and thus it should lead to that it decides to *not* defer
> the resume of the device. Apparently this isn't happening for some
> reason...
>
> Perhaps there's a pm_suspend_ignore_children() set for the parent?
> Or perhaps the parent/child relationship isn't set up correctly?

No, ignore_children = 0. I've dumped all fields from the bus' prepare
callback I had added for Kevin's test:

PM: Suspending system (mem)
simple-pm-bus fec10000.bus: simple_pm_bus_prepare
can_wakeup = 0
async_suspend = 0
is_prepared = 0
is_suspended = 0
is_noirq_suspended = 0
is_late_suspended = 0
early_init = 1
direct_complete = 0
wakeup_path = 0
syscore = 0
no_pm_callbacks = 0
timer_expires = 0
usage_count = 1
child_count = 1
disable_depth = 0
idle_notification = 0
request_pending = 0
deferred_resume = 0
run_wake = 0
runtime_auto = 1
ignore_children = 0
no_callbacks = 0
irq_safe = 0
use_autosuspend = 0
timer_autosuspends = 0
memalloc_noio = 1
request = 0
runtime_status = 0
runtime_error = 0
autosuspend_delay = 0
last_busy = 4294937777
active_jiffies = 0
suspended_jiffies = 430
accounting_timestamp = 4294937777
PM: suspend of devices complete after 4.394 msecs

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

Re: [PATCH 4/4] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Kevin Hilman]

Hi Geert,

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> Hi Ulf, Rafael,
>
> On Tue, May 17, 2016 at 1:41 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> When the pm_runtime_force_suspend|resume() helpers were invented, we still
>> had CONFIG_PM_RUNTIME and CONFIG_PM_SLEEP as separate Kconfig options.
>>
>> To make sure these helpers worked for all combinations and without
>> introducing too much of complexity, the device was always resumed in
>> pm_runtime_force_resume().
>>
>> More precisely, when CONFIG_PM_SLEEP was set and CONFIG_PM_RUNTIME was
>> unset, we needed to resume the device as the subsystem/driver couldn't
>> rely on using runtime PM to do it.
>>
>> As the CONFIG_PM_RUNTIME option was merged into CONFIG_PM a while ago, it
>> removed this combination, of using CONFIG_PM_SLEEP without the earlier
>> CONFIG_PM_RUNTIME.
>>
>> For this reason we can now rely on the subsystem/driver to use runtime PM
>> to resume the device, instead of forcing that to be done in all cases. In
>> other words, let's defer this to a later point when it's actually needed.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/base/power/runtime.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>> index 09e4eb1..1db7b46 100644
>> --- a/drivers/base/power/runtime.c
>> +++ b/drivers/base/power/runtime.c
>> @@ -1509,6 +1509,17 @@ int pm_runtime_force_resume(struct device *dev)
>>         if (!pm_runtime_status_suspended(dev))
>>                 goto out;
>>
>> +       /*
>> +        * The PM core increases the runtime PM usage count in the system PM
>> +        * prepare phase. If the count is greather than 1 at this point, someone
>> +        * else has also increased it. In that case, invoke the runtime resume
>> +        * callback for the device as that is likely what is expected. In other
>> +        * case we trust the subsystem/driver to runtime resume the device when
>> +        * it's actually needed.
>> +        */
>> +       if (atomic_read(&dev->power.usage_count) < 2)
>> +               goto out;
>> +
>>         ret = pm_runtime_set_active(dev);
>>         if (ret)
>>                 goto out;
>
> This patch (commit eb13a0a1b6d5d5c2 in pm/linux-next) breaks resume on
> sh73a0/kzm9g and r8a73a4/ape6evm. On these boards, the Ethernet controller is a
> child of a local bus (bsc), whose clock (zb) is controlled through pm_clk and
> simple-pm-bus, cfr.
>
>     arch/arm/boot/dts/r8a73a4-ape6evm.dts
>     arch/arm/boot/dts/r8a73a4.dtsi
>     arch/arm/boot/dts/sh73a0-kzm9g.dts
>     arch/arm/boot/dts/sh73a0.dtsi
>
> During resume, the bus clock is not enabled, causing an imprecise abort
> when accessing the Ethernet controller's registers.

I have a hunch (without too much digging) that this may be an odd
interaction with the direct_complete stuff since the simple-pm-bus has
no callbacks.

For kicks, could you add something like the hack below (untested) which
will avoid the direct_complete path, and at least help indicate if
that path is worth investigating further.

Thanks,

Kevin

diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
index c5eb46cbf388..63b95fb21510 100644
--- a/drivers/bus/simple-pm-bus.c
+++ b/drivers/bus/simple-pm-bus.c
@@ -36,6 +36,15 @@ static int simple_pm_bus_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int simple_pm_bus_prepare(struct device *dev)
+{
+	return pm_generic_prepare(dev);
+}
+
+static const struct dev_pm_ops simple_pm_bus_ops = {
+	.prepare = simple_pm_bus_prepare,
+};
+
 static const struct of_device_id simple_pm_bus_of_match[] = {
 	{ .compatible = "simple-pm-bus", },
 	{ /* sentinel */ }
@@ -48,6 +57,7 @@ static struct platform_driver simple_pm_bus_driver = {
 	.driver = {
 		.name = "simple-pm-bus",
 		.of_match_table = simple_pm_bus_of_match,
+		.pm = &simple_pm_bus_ops,
 	},
 };
 

Re: [PATCH 4/4] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Geert Uytterhoeven]

Hi Kevin,

On Fri, Jul 1, 2016 at 12:40 AM, Kevin Hilman <khilman@baylibre.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>> On Tue, May 17, 2016 at 1:41 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> When the pm_runtime_force_suspend|resume() helpers were invented, we still
>>> had CONFIG_PM_RUNTIME and CONFIG_PM_SLEEP as separate Kconfig options.
>>>
>>> To make sure these helpers worked for all combinations and without
>>> introducing too much of complexity, the device was always resumed in
>>> pm_runtime_force_resume().
>>>
>>> More precisely, when CONFIG_PM_SLEEP was set and CONFIG_PM_RUNTIME was
>>> unset, we needed to resume the device as the subsystem/driver couldn't
>>> rely on using runtime PM to do it.
>>>
>>> As the CONFIG_PM_RUNTIME option was merged into CONFIG_PM a while ago, it
>>> removed this combination, of using CONFIG_PM_SLEEP without the earlier
>>> CONFIG_PM_RUNTIME.
>>>
>>> For this reason we can now rely on the subsystem/driver to use runtime PM
>>> to resume the device, instead of forcing that to be done in all cases. In
>>> other words, let's defer this to a later point when it's actually needed.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>  drivers/base/power/runtime.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>>> index 09e4eb1..1db7b46 100644
>>> --- a/drivers/base/power/runtime.c
>>> +++ b/drivers/base/power/runtime.c
>>> @@ -1509,6 +1509,17 @@ int pm_runtime_force_resume(struct device *dev)
>>>         if (!pm_runtime_status_suspended(dev))
>>>                 goto out;
>>>
>>> +       /*
>>> +        * The PM core increases the runtime PM usage count in the system PM
>>> +        * prepare phase. If the count is greather than 1 at this point, someone
>>> +        * else has also increased it. In that case, invoke the runtime resume
>>> +        * callback for the device as that is likely what is expected. In other
>>> +        * case we trust the subsystem/driver to runtime resume the device when
>>> +        * it's actually needed.
>>> +        */
>>> +       if (atomic_read(&dev->power.usage_count) < 2)
>>> +               goto out;
>>> +
>>>         ret = pm_runtime_set_active(dev);
>>>         if (ret)
>>>                 goto out;
>>
>> This patch (commit eb13a0a1b6d5d5c2 in pm/linux-next) breaks resume on
>> sh73a0/kzm9g and r8a73a4/ape6evm. On these boards, the Ethernet controller is a
>> child of a local bus (bsc), whose clock (zb) is controlled through pm_clk and
>> simple-pm-bus, cfr.
>>
>>     arch/arm/boot/dts/r8a73a4-ape6evm.dts
>>     arch/arm/boot/dts/r8a73a4.dtsi
>>     arch/arm/boot/dts/sh73a0-kzm9g.dts
>>     arch/arm/boot/dts/sh73a0.dtsi
>>
>> During resume, the bus clock is not enabled, causing an imprecise abort
>> when accessing the Ethernet controller's registers.
>
> I have a hunch (without too much digging) that this may be an odd
> interaction with the direct_complete stuff since the simple-pm-bus has
> no callbacks.
>
> For kicks, could you add something like the hack below (untested) which
> will avoid the direct_complete path, and at least help indicate if
> that path is worth investigating further.

Thanks!
()
> diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
> index c5eb46cbf388..63b95fb21510 100644
> --- a/drivers/bus/simple-pm-bus.c
> +++ b/drivers/bus/simple-pm-bus.c
> @@ -36,6 +36,15 @@ static int simple_pm_bus_remove(struct platform_device *pdev)
>         return 0;
>  }
>
> +static int simple_pm_bus_prepare(struct device *dev)
> +{
> +       return pm_generic_prepare(dev);

This causes an infinite loop, as pm_generic_prepare just calls
dev->driver->pm->prepare(dev);

> +}
> +
> +static const struct dev_pm_ops simple_pm_bus_ops = {
> +       .prepare = simple_pm_bus_prepare,
> +};
> +
>  static const struct of_device_id simple_pm_bus_of_match[] = {
>         { .compatible = "simple-pm-bus", },
>         { /* sentinel */ }
> @@ -48,6 +57,7 @@ static struct platform_driver simple_pm_bus_driver = {
>         .driver = {
>                 .name = "simple-pm-bus",
>                 .of_match_table = simple_pm_bus_of_match,
> +               .pm = &simple_pm_bus_ops,
>         },
>  };

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