[PATCH 0/5] PM: Enable option of re-use runtime PM callbacks at system suspend

[PATCH 0/5] PM: Enable option of re-use runtime PM callbacks at system suspend

[Ulf Hansson]

To put devices into low power state during system suspend, it may be convenient
for runtime PM supported subsystems, power domains and drivers to have the
option of re-using the runtime PM callbacks.

At the moment, quite complex solutions exist for power domains that tries to
handle the above, like for OMAP2. The idea here, is to make it possible for
drivers, who should know best, how to easiest put their devices into low power
state. The intent is thus not only to simplify drivers but also power domains.

Additionally, some drivers seems to have messed up things when combining
runtime PM with system PM. While we enable the option of re-using the runtime
PM callbacks during system PM, we also intend to clarify the way forward for
how these scenarios could be resolved.

Some new helper macros for defining PM callbacks and two new pm_generic*
functions has been implemented in this patch set. These are provided to make it
easier for those who wants to enable the option of re-using the runtime PM
callbacks during system suspend.

A minor fix was needed for the platform bus, which runtime PM callbacks are set
to the pm_genric_runtime_suspend|resume functions. These were implemented only
for CONFIG_PM_RUNTIME and thus the platform bus prevented driver's runtime PM
callbacks to be invoked when only CONFIG_PM_SLEEP was used. We move them into
CONFIG_PM to resolve the problem and then let drivers decide how to handle this
themselves.

Patch 5 is folded into this patch set, to visualize how a driver that uses both
runtime PM and system PM, can benefit from re-using the runtime PM callbacks
during system suspend.

Also note that, patch 1 to 4 has been submitted to linux-pm as standalone
patches recently. I thought it make sense to collect them into a patchset
since those are closely related.

Ulf Hansson (5):
  PM / Sleep: Add pm_generic functions to re-use runtime PM callbacks
  PM / Runtime: Implement the pm_generic_runtime functions for
    CONFIG_PM
  PM / Runtime: Add second macro for definition of runtime PM callbacks
  PM / Sleep: Add macro to define common late/early system PM callbacks
  drm/exynos: Convert to suspend_late/resume_early callbacks for fimc

 drivers/base/power/generic_ops.c         |   90 +++++++++++++++++++++++++++++-
 drivers/gpu/drm/exynos/exynos_drm_fimc.c |   33 ++---------
 include/linux/pm.h                       |   23 ++++++++
 include/linux/pm_runtime.h               |   12 ++--
 4 files changed, 123 insertions(+), 35 deletions(-)

-- 
1.7.9.5

[PATCH 1/5] PM / Sleep: Add pm_generic functions to re-use runtime PM callbacks

[Ulf Hansson]

To put devices into low power state during sleep, it sometimes makes
sense at subsystem-level to re-use the runtime PM callbacks.

The PM core will at device_suspend_late disable runtime PM, after that
we can safely operate on these callbacks. At suspend_late the device
will be put into low power state by invoking the runtime_suspend
callbacks, unless the runtime status is already suspended. At
resume_early the state is restored by invoking the runtime_resume
callbacks. Soon after the PM core will re-enable runtime PM before
returning from device_resume_early.

The new pm_generic functions are named pm_generic_suspend_late_runtime
and pm_generic_resume_early_runtime, they are supposed to be used in
pairs.

Do note that these new generic callbacks will work smothly even with
and without CONFIG_PM_RUNTIME, as long as the runtime PM callbacks are
implemented inside CONFIG_PM instead of CONFIG_PM_RUNTIME.

A special thanks to Alan Stern who came up with this idea.

Cc: Kevin Hilman <khilman@linaro.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/generic_ops.c |   86 ++++++++++++++++++++++++++++++++++++++
 include/linux/pm.h               |    2 +
 2 files changed, 88 insertions(+)

diff --git a/drivers/base/power/generic_ops.c b/drivers/base/power/generic_ops.c
index 5ee030a..8e78ad1 100644
--- a/drivers/base/power/generic_ops.c
+++ b/drivers/base/power/generic_ops.c
@@ -93,6 +93,49 @@ int pm_generic_suspend_late(struct device *dev)
 EXPORT_SYMBOL_GPL(pm_generic_suspend_late);
 
 /**
+ * pm_generic_suspend_late_runtime - Generic suspend_late callback for drivers
+ * @dev: Device to suspend.
+ * Use to invoke runtime_suspend callbacks at suspend_late.
+ */
+int pm_generic_suspend_late_runtime(struct device *dev)
+{
+	int (*callback)(struct device *);
+	int ret = 0;
+
+	/*
+	 * PM core has disabled runtime PM in device_suspend_late, thus we can
+	 * safely check the device's runtime status and decide whether
+	 * additional actions are needed to put the device into low power state.
+	 * If so, we invoke the runtime_suspend callbacks.
+	 * For the !CONFIG_PM_RUNTIME case, pm_runtime_status_suspended() always
+	 * returns false and therefore the runtime_suspend callback will be
+	 * invoked.
+	 */
+	if (pm_runtime_status_suspended(dev))
+		return 0;
+
+	if (dev->pm_domain)
+		callback = dev->pm_domain->ops.runtime_suspend;
+	else if (dev->type && dev->type->pm)
+		callback = dev->type->pm->runtime_suspend;
+	else if (dev->class && dev->class->pm)
+		callback = dev->class->pm->runtime_suspend;
+	else if (dev->bus && dev->bus->pm)
+		callback = dev->bus->pm->runtime_suspend;
+	else
+		callback = NULL;
+
+	if (!callback && dev->driver && dev->driver->pm)
+		callback = dev->driver->pm->runtime_suspend;
+
+	if (callback)
+		ret = callback(dev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pm_generic_suspend_late_runtime);
+
+/**
  * pm_generic_suspend - Generic suspend callback for subsystems.
  * @dev: Device to suspend.
  */
@@ -237,6 +280,49 @@ int pm_generic_resume_early(struct device *dev)
 EXPORT_SYMBOL_GPL(pm_generic_resume_early);
 
 /**
+ * pm_generic_resume_early_runtime - Generic resume_early callback for drivers
+ * @dev: Device to resume.
+ * Use to invoke runtime_resume callbacks at resume_early.
+ */
+int pm_generic_resume_early_runtime(struct device *dev)
+{
+	int (*callback)(struct device *);
+	int ret = 0;
+
+	/*
+	 * PM core has not yet enabled runtime PM in device_resume_early,
+	 * thus we can safely check the device's runtime status and restore the
+	 * previous state we had in device_suspend_late. If restore is needed
+	 * we invoke the runtime_resume callbacks.
+	 * For the !CONFIG_PM_RUNTIME case, pm_runtime_status_suspended() always
+	 * returns false and therefore the runtime_resume callback will be
+	 * invoked.
+	 */
+	if (pm_runtime_status_suspended(dev))
+		return 0;
+
+	if (dev->pm_domain)
+		callback = dev->pm_domain->ops.runtime_resume;
+	else if (dev->type && dev->type->pm)
+		callback = dev->type->pm->runtime_resume;
+	else if (dev->class && dev->class->pm)
+		callback = dev->class->pm->runtime_resume;
+	else if (dev->bus && dev->bus->pm)
+		callback = dev->bus->pm->runtime_resume;
+	else
+		callback = NULL;
+
+	if (!callback && dev->driver && dev->driver->pm)
+		callback = dev->driver->pm->runtime_resume;
+
+	if (callback)
+		ret = callback(dev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pm_generic_resume_early_runtime);
+
+/**
  * pm_generic_resume - Generic resume callback for subsystems.
  * @dev: Device to resume.
  */
diff --git a/include/linux/pm.h b/include/linux/pm.h
index a224c7f..5bce0d4 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -656,9 +656,11 @@ extern void dpm_for_each_dev(void *data, void (*fn)(struct device *, void *));
 
 extern int pm_generic_prepare(struct device *dev);
 extern int pm_generic_suspend_late(struct device *dev);
+extern int pm_generic_suspend_late_runtime(struct device *dev);
 extern int pm_generic_suspend_noirq(struct device *dev);
 extern int pm_generic_suspend(struct device *dev);
 extern int pm_generic_resume_early(struct device *dev);
+extern int pm_generic_resume_early_runtime(struct device *dev);
 extern int pm_generic_resume_noirq(struct device *dev);
 extern int pm_generic_resume(struct device *dev);
 extern int pm_generic_freeze_noirq(struct device *dev);
-- 
1.7.9.5

[PATCH 2/5] PM / Runtime: Implement the pm_generic_runtime functions for CONFIG_PM

[Ulf Hansson]

The pm_generic_runtime_suspend|resume functions were implemented within
CONFIG_PM_RUNTIME.

As we also may use runtime PM callbacks during system suspend, to put
devices into low power state, we need to move the implementation of
pm_generic_runtime_suspend|resume to CONFIG_PM.

This change prevents the platform bus from by-passing driver's runtime
PM callbacks while only CONFIG_PM_SLEEP and not CONFIG_PM_RUNTIME is
being used.

Cc: Kevin Hilman <khilman@linaro.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/generic_ops.c |    4 ++--
 include/linux/pm_runtime.h       |   12 ++++++++----
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/base/power/generic_ops.c b/drivers/base/power/generic_ops.c
index 8e78ad1..a4c6cb0 100644
--- a/drivers/base/power/generic_ops.c
+++ b/drivers/base/power/generic_ops.c
@@ -10,7 +10,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/export.h>
 
-#ifdef CONFIG_PM_RUNTIME
+#ifdef CONFIG_PM
 /**
  * pm_generic_runtime_suspend - Generic runtime suspend callback for subsystems.
  * @dev: Device to suspend.
@@ -48,7 +48,7 @@ int pm_generic_runtime_resume(struct device *dev)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(pm_generic_runtime_resume);
-#endif /* CONFIG_PM_RUNTIME */
+#endif /* CONFIG_PM */
 
 #ifdef CONFIG_PM_SLEEP
 /**
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 6fa7cea..16c9a62 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -23,6 +23,14 @@
 					    usage_count */
 #define RPM_AUTO		0x08	/* Use autosuspend_delay */
 
+#ifdef CONFIG_PM
+extern int pm_generic_runtime_suspend(struct device *dev);
+extern int pm_generic_runtime_resume(struct device *dev);
+#else
+static inline int pm_generic_runtime_suspend(struct device *dev) { return 0; }
+static inline int pm_generic_runtime_resume(struct device *dev) { return 0; }
+#endif
+
 #ifdef CONFIG_PM_RUNTIME
 
 extern struct workqueue_struct *pm_wq;
@@ -37,8 +45,6 @@ extern void pm_runtime_enable(struct device *dev);
 extern void __pm_runtime_disable(struct device *dev, bool check_resume);
 extern void pm_runtime_allow(struct device *dev);
 extern void pm_runtime_forbid(struct device *dev);
-extern int pm_generic_runtime_suspend(struct device *dev);
-extern int pm_generic_runtime_resume(struct device *dev);
 extern void pm_runtime_no_callbacks(struct device *dev);
 extern void pm_runtime_irq_safe(struct device *dev);
 extern void __pm_runtime_use_autosuspend(struct device *dev, bool use);
@@ -142,8 +148,6 @@ static inline bool pm_runtime_active(struct device *dev) { return true; }
 static inline bool pm_runtime_status_suspended(struct device *dev) { return false; }
 static inline bool pm_runtime_enabled(struct device *dev) { return false; }
 
-static inline int pm_generic_runtime_suspend(struct device *dev) { return 0; }
-static inline int pm_generic_runtime_resume(struct device *dev) { return 0; }
 static inline void pm_runtime_no_callbacks(struct device *dev) {}
 static inline void pm_runtime_irq_safe(struct device *dev) {}
 
-- 
1.7.9.5

[PATCH 3/5] PM / Runtime: Add second macro for definition of runtime PM callbacks

[Ulf Hansson]

It is allowed and will in many cases make sense to have the runtime PM
callbacks to be defined for CONFIG_PM instead of CONFIG_PM_RUNTIME.

Since the PM core disables runtime PM during system suspend, before the
.suspend_late callbacks are invoked, drivers could at this point
directly invoke it's runtime PM callbacks or even walk through it's bus
or power domain, to put it's device into low power state.

Use the new macro SET_PM_RUNTIME_PM_OPS in cases were the above makes
sense. Make sure the callbacks are encapsulated within CONFIG_PM
instead of CONFIG_PM_RUNTIME.

Do note that the old macro SET_RUNTIME_PM_OPS, which is being quite
widely used right now, requires the callbacks to be defined for
CONFIG_PM_RUNTIME. In many cases it will certainly be convenient to
convert to the new macro above, but still some cases are optimal for
only CONFIG_PM_RUNTIME.

Cc: Kevin Hilman <khilman@linaro.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 include/linux/pm.h |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/pm.h b/include/linux/pm.h
index 5bce0d4..529657c 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -320,6 +320,15 @@ struct dev_pm_ops {
 #define SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn)
 #endif
 
+#ifdef CONFIG_PM
+#define SET_PM_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn) \
+	.runtime_suspend = suspend_fn, \
+	.runtime_resume = resume_fn, \
+	.runtime_idle = idle_fn,
+#else
+#define SET_PM_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn)
+#endif
+
 /*
  * Use this if you want to use the same suspend and resume callbacks for suspend
  * to RAM and hibernation.
-- 
1.7.9.5

[PATCH 4/5] PM / Sleep: Add macro to define common late/early system PM callbacks

[Ulf Hansson]

We use the same approach as for the existing SET_SYSTEM_SLEEP_PM_OPS,
but for the late and early callbacks instead.

The new SET_LATE_SYSTEM_SLEEP_PM_OPS, defined for CONFIG_PM_SLEEP, will
point ->suspend_late, ->freeze_late and ->poweroff_late to the same
function. Vice verse happens for ->resume_early, ->thaw_early and
->restore_early.

Cc: Kevin Hilman <khilman@linaro.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 include/linux/pm.h |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/pm.h b/include/linux/pm.h
index 529657c..fb0f1db 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -311,6 +311,18 @@ struct dev_pm_ops {
 #define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
 #endif
 
+#ifdef CONFIG_PM_SLEEP
+#define SET_LATE_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
+	.suspend_late = suspend_fn, \
+	.resume_early = resume_fn, \
+	.freeze_late = suspend_fn, \
+	.thaw_early = resume_fn, \
+	.poweroff_late = suspend_fn, \
+	.restore_early = resume_fn,
+#else
+#define SET_LATE_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
+#endif
+
 #ifdef CONFIG_PM_RUNTIME
 #define SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn) \
 	.runtime_suspend = suspend_fn, \
-- 
1.7.9.5

[PATCH 5/5] drm/exynos: Convert to suspend_late/resume_early callbacks for fimc

[Ulf Hansson]

To simplify code for system suspend, convert the .suspend|resume
callbacks into .suspend_late|resume_early. In general this could be
convenient for any driver that supports both system PM and runtime PM.

Move the runtime PM callbacks to be implemented within CONFIG_PM, to
make them available for system PM.

Use pm_generic_suspend_late_runtime and pm_generic_resume_early_runtime
as the new callbacks.

Cc: David Airlie <airlied@linux.ie>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: linux-samsung-soc@vger.kernel.org
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Len Brown <len.brown@intel.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/gpu/drm/exynos/exynos_drm_fimc.c |   33 ++++--------------------------
 1 file changed, 4 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
index 8adfc8f..7aa17a3 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
@@ -1888,33 +1888,7 @@ static int fimc_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int fimc_suspend(struct device *dev)
-{
-	struct fimc_context *ctx = get_fimc_context(dev);
-
-	DRM_DEBUG_KMS("id[%d]\n", ctx->id);
-
-	if (pm_runtime_suspended(dev))
-		return 0;
-
-	return fimc_clk_ctrl(ctx, false);
-}
-
-static int fimc_resume(struct device *dev)
-{
-	struct fimc_context *ctx = get_fimc_context(dev);
-
-	DRM_DEBUG_KMS("id[%d]\n", ctx->id);
-
-	if (!pm_runtime_suspended(dev))
-		return fimc_clk_ctrl(ctx, true);
-
-	return 0;
-}
-#endif
-
-#ifdef CONFIG_PM_RUNTIME
+#ifdef CONFIG_PM
 static int fimc_runtime_suspend(struct device *dev)
 {
 	struct fimc_context *ctx = get_fimc_context(dev);
@@ -1935,8 +1909,9 @@ static int fimc_runtime_resume(struct device *dev)
 #endif
 
 static const struct dev_pm_ops fimc_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(fimc_suspend, fimc_resume)
-	SET_RUNTIME_PM_OPS(fimc_runtime_suspend, fimc_runtime_resume, NULL)
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_generic_suspend_late_runtime,
+				pm_generic_resume_early_runtime)
+	SET_PM_RUNTIME_PM_OPS(fimc_runtime_suspend, fimc_runtime_resume, NULL)
 };
 
 static const struct of_device_id fimc_of_match[] = {
-- 
1.7.9.5

Re: [PATCH 0/5] PM: Enable option of re-use runtime PM callbacks at system suspend

[Rafael J. Wysocki]

On Wednesday, November 27, 2013 04:34:55 PM Ulf Hansson wrote:
> To put devices into low power state during system suspend, it may be convenient
> for runtime PM supported subsystems, power domains and drivers to have the
> option of re-using the runtime PM callbacks.
> 
> At the moment, quite complex solutions exist for power domains that tries to
> handle the above, like for OMAP2. The idea here, is to make it possible for
> drivers, who should know best, how to easiest put their devices into low power
> state. The intent is thus not only to simplify drivers but also power domains.

So this is not entirely correct and stems from the fact that you are only
considering one platform.

On some other platforms, like x86 PC for one example, device drivers have no
idea how to put their devices into low power states at all, because that
depends on what's there in the ACPI tables.

This becomes clearly visible when you try to use the same driver on two
different platforms that have different board layouts and power configurations.
And if one of them uses ACPI by chance, the driver shouldn't really fiddle with
its little knobs for clocks and power rails directly.

> Additionally, some drivers seems to have messed up things when combining
> runtime PM with system PM. While we enable the option of re-using the runtime
> PM callbacks during system PM, we also intend to clarify the way forward for
> how these scenarios could be resolved.
> 
> Some new helper macros for defining PM callbacks and two new pm_generic*
> functions has been implemented in this patch set. These are provided to make it
> easier for those who wants to enable the option of re-using the runtime PM
> callbacks during system suspend.

I'm generally opposed to re-using callbacks like this, because it adds confusion
to the picture.  It may seem to be clever, but in fact it leads to bad design
choices in the drivers in my opinion.

Let's talk about specific examples, though.

Why exactly do you need what patch [5/5] does in the exynos_drm_fimc driver?

Rafael

Re: [PATCH 0/5] PM: Enable option of re-use runtime PM callbacks at system suspend

[Ulf Hansson]

On 27 November 2013 21:42, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, November 27, 2013 04:34:55 PM Ulf Hansson wrote:
>> To put devices into low power state during system suspend, it may be convenient
>> for runtime PM supported subsystems, power domains and drivers to have the
>> option of re-using the runtime PM callbacks.
>>
>> At the moment, quite complex solutions exist for power domains that tries to
>> handle the above, like for OMAP2. The idea here, is to make it possible for
>> drivers, who should know best, how to easiest put their devices into low power
>> state. The intent is thus not only to simplify drivers but also power domains.
>
> So this is not entirely correct and stems from the fact that you are only
> considering one platform.

Bad wording from my side, sorry.

I guess you realize, that am mostly working on ARM SoCs, but that
involve many platforms and drivers. In these cases it is quite common
that drivers are the best "decision makers" for PM. It is also very
common to have power domain regulators.

This combination is almost impossible to support without "hacks" as of
today, especially if you have a more complex situation were the driver
is attached to both a bus and a power domain, and were those also are
handling runtime PM resources.

>
> On some other platforms, like x86 PC for one example, device drivers have no
> idea how to put their devices into low power states at all, because that
> depends on what's there in the ACPI tables.
>
> This becomes clearly visible when you try to use the same driver on two
> different platforms that have different board layouts and power configurations.
> And if one of them uses ACPI by chance, the driver shouldn't really fiddle with
> its little knobs for clocks and power rails directly.

Again, this patch set will only provide the option of being able to
re-use runtime PM callbacks during system suspend; could also be
considered as fundamental "building blocks" for those SoCs who needs
this.

Additionally and important, it wont break nor interfere with any other
platforms like x86, which of course is very important. On the other
hand, if it makes sense for other platforms to use these new building
blocks they have the opportunity to do so.

>
>> Additionally, some drivers seems to have messed up things when combining
>> runtime PM with system PM. While we enable the option of re-using the runtime
>> PM callbacks during system PM, we also intend to clarify the way forward for
>> how these scenarios could be resolved.
>>
>> Some new helper macros for defining PM callbacks and two new pm_generic*
>> functions has been implemented in this patch set. These are provided to make it
>> easier for those who wants to enable the option of re-using the runtime PM
>> callbacks during system suspend.
>
> I'm generally opposed to re-using callbacks like this, because it adds confusion
> to the picture.  It may seem to be clever, but in fact it leads to bad design
> choices in the drivers in my opinion.

In my world of the kernel, it will clearly resolve confusions and
simplify a significant amount of code in power domains, buses and
drivers. So I guess it depends on from what point you look at this.

And, as you stated previously during these discussions, we have the
opportunity to update the documentation around this topic, I will
happily do it, if needed.

>
> Let's talk about specific examples, though.
>
> Why exactly do you need what patch [5/5] does in the exynos_drm_fimc driver?

This was a simple example, I wanted to visualize how the new building
blocks were going to be used. Anyway, this we achieve with the patch:

1.
The PM part in the driver becomes simplified, we don't need the
wrapper functions for the runtime PM callbacks any more.

2.
Previously the driver did not make sure runtime PM was disabled,
before it put the device into low power state at .suspend. From a
runtime PM point of view, this is not a "nice" behaviour and opens up
for confusions, even if it likely would work in most cases.

3.
The power domain runtime PM callbacks were by-passed during system
suspend, my patch fixes this. Why do I want this? Because the power
domain can have runtime PM resources it need to handle at this phase.
Potentially, it could handle that from it's .suspend_late callback
instead, but then it gets unnecessary complicated, which is what
clearly happened to the power domain for OMAP2, for example.


If you want additional proof of concepts, we can have a look at more
complex example.

Typically I am thinking of cases were a cross SoC driver is attached
to a bus and for some SoCs a power domain as well. Then, the bus, the
power domain and the driver - all have runtime PM resources to handle.

In these cases using the new building blocks will not only
significantly simplify code, but also fix immediate bugs. One example
are drivers attached to the AMBA bus, at drivers/amba/bus.c.

Kind regards
Ulf Hansson

>
> Rafael
>

Re: [PATCH 0/5] PM: Enable option of re-use runtime PM callbacks at system suspend

[Rafael J. Wysocki]

On Thursday, November 28, 2013 10:58:48 AM Ulf Hansson wrote:
> On 27 November 2013 21:42, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Wednesday, November 27, 2013 04:34:55 PM Ulf Hansson wrote:
> >> To put devices into low power state during system suspend, it may be convenient
> >> for runtime PM supported subsystems, power domains and drivers to have the
> >> option of re-using the runtime PM callbacks.
> >>
> >> At the moment, quite complex solutions exist for power domains that tries to
> >> handle the above, like for OMAP2. The idea here, is to make it possible for
> >> drivers, who should know best, how to easiest put their devices into low power
> >> state. The intent is thus not only to simplify drivers but also power domains.
> >
> > So this is not entirely correct and stems from the fact that you are only
> > considering one platform.
> 
> Bad wording from my side, sorry.
> 
> I guess you realize, that am mostly working on ARM SoCs, but that
> involve many platforms and drivers. In these cases it is quite common
> that drivers are the best "decision makers" for PM. It is also very
> common to have power domain regulators.
> 
> This combination is almost impossible to support without "hacks" as of
> today, especially if you have a more complex situation were the driver
> is attached to both a bus and a power domain, and were those also are
> handling runtime PM resources.
> 
> >
> > On some other platforms, like x86 PC for one example, device drivers have no
> > idea how to put their devices into low power states at all, because that
> > depends on what's there in the ACPI tables.
> >
> > This becomes clearly visible when you try to use the same driver on two
> > different platforms that have different board layouts and power configurations.
> > And if one of them uses ACPI by chance, the driver shouldn't really fiddle with
> > its little knobs for clocks and power rails directly.
> 
> Again, this patch set will only provide the option of being able to
> re-use runtime PM callbacks during system suspend; could also be
> considered as fundamental "building blocks" for those SoCs who needs
> this.
> 
> Additionally and important, it wont break nor interfere with any other
> platforms like x86, which of course is very important. On the other
> hand, if it makes sense for other platforms to use these new building
> blocks they have the opportunity to do so.

The lack of specificity here doesn't make the discussion any easier.

It usually is better to talk about specific problems to address than
using general terms which may mean slightly different things for different
people.

> >> Additionally, some drivers seems to have messed up things when combining
> >> runtime PM with system PM. While we enable the option of re-using the runtime
> >> PM callbacks during system PM, we also intend to clarify the way forward for
> >> how these scenarios could be resolved.
> >>
> >> Some new helper macros for defining PM callbacks and two new pm_generic*
> >> functions has been implemented in this patch set. These are provided to make it
> >> easier for those who wants to enable the option of re-using the runtime PM
> >> callbacks during system suspend.
> >
> > I'm generally opposed to re-using callbacks like this, because it adds confusion
> > to the picture.  It may seem to be clever, but in fact it leads to bad design
> > choices in the drivers in my opinion.
> 
> In my world of the kernel, it will clearly resolve confusions and
> simplify a significant amount of code in power domains, buses and
> drivers. So I guess it depends on from what point you look at this.

This is so vague that I don't even know how to respond. :-)

So let me say instead that what you did in patch [5/5] is a layering violation
which always is a bug, even if it doesn't break things outright.

After that patch the driver would call into a layer that is supposed to call
into it under normal conditions.  Moreover, it would expect that layer to
call back into it again in a specific way, which may or may not happen depending
on how exactly that layer is implemented.  So even if it works today, it will
add constraints on how that other layer may be implmented which *is* confusing
and wrong in my not so humble opinion.

I'll never apply any patches that lead to situations like that, don't even
bother to send them to me.  Pretty please.

> And, as you stated previously during these discussions, we have the
> opportunity to update the documentation around this topic, I will
> happily do it, if needed.

That's always welcome. :-)

> >
> > Let's talk about specific examples, though.
> >
> > Why exactly do you need what patch [5/5] does in the exynos_drm_fimc driver?
> 
> This was a simple example, I wanted to visualize how the new building
> blocks were going to be used. Anyway, this we achieve with the patch:
> 
> 1.
> The PM part in the driver becomes simplified, we don't need the
> wrapper functions for the runtime PM callbacks any more.

No, it is not simplified.  It becomes *far* more complicated conceptually
instead, although that is hidden by moving the complexity into the functions
added by patch [1/5].  So whoever doesn't look into those functions will
not actually realize how complicated the code really is.

> 2.
> Previously the driver did not make sure runtime PM was disabled,
> before it put the device into low power state at .suspend. From a
> runtime PM point of view, this is not a "nice" behaviour and opens up
> for confusions, even if it likely would work in most cases.

So the proper fix, in my opinion, would be to point .suspend_late and
.resume_early in that driver to fimc_suspend() and fimc_resume(),
respectively, and leave the .suspend and .resume pointers unpopulated.

Wouldn't that actually work?

> 3.
> The power domain runtime PM callbacks were by-passed during system
> suspend, my patch fixes this.

I don't exactly understand this.  Why would they be bypassed?

> Why do I want this? Because the power
> domain can have runtime PM resources it need to handle at this phase.
> Potentially, it could handle that from it's .suspend_late callback
> instead, but then it gets unnecessary complicated, which is what
> clearly happened to the power domain for OMAP2, for example.

I'd like to understand this.  What exactly do you mean by "unnecessary
complicated"?

> 
> If you want additional proof of concepts, we can have a look at more
> complex example.
> 
> Typically I am thinking of cases were a cross SoC driver is attached
> to a bus and for some SoCs a power domain as well. Then, the bus, the
> power domain and the driver - all have runtime PM resources to handle.

Sure.

> In these cases using the new building blocks will not only
> significantly simplify code, but also fix immediate bugs. One example
> are drivers attached to the AMBA bus, at drivers/amba/bus.c.

Again, what drivers and what's the bug you're talking about?

Rafael

Re: [PATCH 0/5] PM: Enable option of re-use runtime PM callbacks at system suspend

[Ulf Hansson]

>
> The lack of specificity here doesn't make the discussion any easier.
>
> It usually is better to talk about specific problems to address than
> using general terms which may mean slightly different things for different
> people.

During these discussions, I have tried to point at existing code for
drivers and existing code for power domains. Those which at the moment
either have got things wrong or are unnecessary complicated, in
regards to their PM implementation.

I suppose I could provide some more patches for proof of concept, will
that be a way forward?

>
>> >> Additionally, some drivers seems to have messed up things when combining
>> >> runtime PM with system PM. While we enable the option of re-using the runtime
>> >> PM callbacks during system PM, we also intend to clarify the way forward for
>> >> how these scenarios could be resolved.
>> >>
>> >> Some new helper macros for defining PM callbacks and two new pm_generic*
>> >> functions has been implemented in this patch set. These are provided to make it
>> >> easier for those who wants to enable the option of re-using the runtime PM
>> >> callbacks during system suspend.
>> >
>> > I'm generally opposed to re-using callbacks like this, because it adds confusion
>> > to the picture.  It may seem to be clever, but in fact it leads to bad design
>> > choices in the drivers in my opinion.
>>
>> In my world of the kernel, it will clearly resolve confusions and
>> simplify a significant amount of code in power domains, buses and
>> drivers. So I guess it depends on from what point you look at this.
>
> This is so vague that I don't even know how to respond. :-)
>
> So let me say instead that what you did in patch [5/5] is a layering violation
> which always is a bug, even if it doesn't break things outright.
>
> After that patch the driver would call into a layer that is supposed to call
> into it under normal conditions.  Moreover, it would expect that layer to
> call back into it again in a specific way, which may or may not happen depending
> on how exactly that layer is implemented.  So even if it works today, it will
> add constraints on how that other layer may be implmented which *is* confusing
> and wrong in my not so humble opinion.
>
> I'll never apply any patches that lead to situations like that, don't even
> bother to send them to me.  Pretty please.
>

After all these good discussions which clearly pointed the solution
into this direction, you decide to bring up this argument now? It
makes me wonder.

Indirectly what you are saying is that, the PM core should at
device_prepare, do pm_runtime_disable() instead of
pm_runtime_get_noresume(), to prevent drivers/subsystems from
triggering "layering violations" by invoking pm_runtime_get_sync().

Because, this is exactly the same kind of layering violation you refer
to while neglecting my approach, which at the moment
drivers/subsystems not only are allowed to, but also encouraged to do
during system suspend.

Now, obviously I don't think we shall change the behaviour of PM core,
that would just not be convenient for subsystems and drivers, right?

So, the PM core allows layering violations for the .runtime_resume
callbacks to be invoked during system suspend. It can do so, because
it trust drivers/subsystems to act responsibly and to what suites them
best.

For the same reasons, I believe we should trust drivers/subsystems, to
understand when it makes sense for them to re-use all of the runtime
PM callbacks during system suspend and not just the .runtime_suspend
callback.

That is in principle what I and Alan, who came up with this idea, are
suggesting.

>> And, as you stated previously during these discussions, we have the
>> opportunity to update the documentation around this topic, I will
>> happily do it, if needed.
>
> That's always welcome. :-)
>
>> >
>> > Let's talk about specific examples, though.
>> >
>> > Why exactly do you need what patch [5/5] does in the exynos_drm_fimc driver?
>>
>> This was a simple example, I wanted to visualize how the new building
>> blocks were going to be used. Anyway, this we achieve with the patch:
>>
>> 1.
>> The PM part in the driver becomes simplified, we don't need the
>> wrapper functions for the runtime PM callbacks any more.
>
> No, it is not simplified.  It becomes *far* more complicated conceptually
> instead, although that is hidden by moving the complexity into the functions
> added by patch [1/5].  So whoever doesn't look into those functions will
> not actually realize how complicated the code really is.
>
>> 2.
>> Previously the driver did not make sure runtime PM was disabled,
>> before it put the device into low power state at .suspend. From a
>> runtime PM point of view, this is not a "nice" behaviour and opens up
>> for confusions, even if it likely would work in most cases.
>
> So the proper fix, in my opinion, would be to point .suspend_late and
> .resume_early in that driver to fimc_suspend() and fimc_resume(),
> respectively, and leave the .suspend and .resume pointers unpopulated.
>
> Wouldn't that actually work?

If we decide to ignore the power domain issue below, yes.

>
>> 3.
>> The power domain runtime PM callbacks were by-passed during system
>> suspend, my patch fixes this.
>
> I don't exactly understand this.  Why would they be bypassed?
>
>> Why do I want this? Because the power
>> domain can have runtime PM resources it need to handle at this phase.
>> Potentially, it could handle that from it's .suspend_late callback
>> instead, but then it gets unnecessary complicated, which is what
>> clearly happened to the power domain for OMAP2, for example.
>
> I'd like to understand this.  What exactly do you mean by "unnecessary
> complicated"?

Please, have a deeper look into the OMAP power domain implementation.

If each SoC (for those that have power domain regulators) needs their
own version of such a power domain, then I certainly think it is more
complicated that in needs to be.

>
>>
>> If you want additional proof of concepts, we can have a look at more
>> complex example.
>>
>> Typically I am thinking of cases were a cross SoC driver is attached
>> to a bus and for some SoCs a power domain as well. Then, the bus, the
>> power domain and the driver - all have runtime PM resources to handle.
>
> Sure.

OK, I consider resending the patch set, including some additional
proof of concept patches.

>
>> In these cases using the new building blocks will not only
>> significantly simplify code, but also fix immediate bugs. One example
>> are drivers attached to the AMBA bus, at drivers/amba/bus.c.
>
> Again, what drivers and what's the bug you're talking about?

I will use some of these as examples, it will be more visible to you then.

Kind regards
Uffe

>
> Rafael
>

Re: [PATCH 0/5] PM: Enable option of re-use runtime PM callbacks at system suspend

[Ulf Hansson]

On 29 November 2013 10:32, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>
>> The lack of specificity here doesn't make the discussion any easier.
>>
>> It usually is better to talk about specific problems to address than
>> using general terms which may mean slightly different things for different
>> people.
>
> During these discussions, I have tried to point at existing code for
> drivers and existing code for power domains. Those which at the moment
> either have got things wrong or are unnecessary complicated, in
> regards to their PM implementation.
>
> I suppose I could provide some more patches for proof of concept, will
> that be a way forward?
>
>>
>>> >> Additionally, some drivers seems to have messed up things when combining
>>> >> runtime PM with system PM. While we enable the option of re-using the runtime
>>> >> PM callbacks during system PM, we also intend to clarify the way forward for
>>> >> how these scenarios could be resolved.
>>> >>
>>> >> Some new helper macros for defining PM callbacks and two new pm_generic*
>>> >> functions has been implemented in this patch set. These are provided to make it
>>> >> easier for those who wants to enable the option of re-using the runtime PM
>>> >> callbacks during system suspend.
>>> >
>>> > I'm generally opposed to re-using callbacks like this, because it adds confusion
>>> > to the picture.  It may seem to be clever, but in fact it leads to bad design
>>> > choices in the drivers in my opinion.
>>>
>>> In my world of the kernel, it will clearly resolve confusions and
>>> simplify a significant amount of code in power domains, buses and
>>> drivers. So I guess it depends on from what point you look at this.
>>
>> This is so vague that I don't even know how to respond. :-)
>>
>> So let me say instead that what you did in patch [5/5] is a layering violation
>> which always is a bug, even if it doesn't break things outright.
>>
>> After that patch the driver would call into a layer that is supposed to call
>> into it under normal conditions.  Moreover, it would expect that layer to
>> call back into it again in a specific way, which may or may not happen depending
>> on how exactly that layer is implemented.  So even if it works today, it will
>> add constraints on how that other layer may be implmented which *is* confusing
>> and wrong in my not so humble opinion.
>>
>> I'll never apply any patches that lead to situations like that, don't even
>> bother to send them to me.  Pretty please.
>>
>
> After all these good discussions which clearly pointed the solution
> into this direction, you decide to bring up this argument now? It
> makes me wonder.
>
> Indirectly what you are saying is that, the PM core should at
> device_prepare, do pm_runtime_disable() instead of
> pm_runtime_get_noresume(), to prevent drivers/subsystems from
> triggering "layering violations" by invoking pm_runtime_get_sync().
>
> Because, this is exactly the same kind of layering violation you refer
> to while neglecting my approach, which at the moment
> drivers/subsystems not only are allowed to, but also encouraged to do
> during system suspend.
>
> Now, obviously I don't think we shall change the behaviour of PM core,
> that would just not be convenient for subsystems and drivers, right?
>
> So, the PM core allows layering violations for the .runtime_resume
> callbacks to be invoked during system suspend. It can do so, because
> it trust drivers/subsystems to act responsibly and to what suites them
> best.
>
> For the same reasons, I believe we should trust drivers/subsystems, to
> understand when it makes sense for them to re-use all of the runtime
> PM callbacks during system suspend and not just the .runtime_suspend
> callback.

Sorry, another typo:

"not just the .runtime_suspend" -> "not just the .runtime_resume".

>
> That is in principle what I and Alan, who came up with this idea, are
> suggesting.
>
>>> And, as you stated previously during these discussions, we have the
>>> opportunity to update the documentation around this topic, I will
>>> happily do it, if needed.
>>
>> That's always welcome. :-)
>>
>>> >
>>> > Let's talk about specific examples, though.
>>> >
>>> > Why exactly do you need what patch [5/5] does in the exynos_drm_fimc driver?
>>>
>>> This was a simple example, I wanted to visualize how the new building
>>> blocks were going to be used. Anyway, this we achieve with the patch:
>>>
>>> 1.
>>> The PM part in the driver becomes simplified, we don't need the
>>> wrapper functions for the runtime PM callbacks any more.
>>
>> No, it is not simplified.  It becomes *far* more complicated conceptually
>> instead, although that is hidden by moving the complexity into the functions
>> added by patch [1/5].  So whoever doesn't look into those functions will
>> not actually realize how complicated the code really is.
>>
>>> 2.
>>> Previously the driver did not make sure runtime PM was disabled,
>>> before it put the device into low power state at .suspend. From a
>>> runtime PM point of view, this is not a "nice" behaviour and opens up
>>> for confusions, even if it likely would work in most cases.
>>
>> So the proper fix, in my opinion, would be to point .suspend_late and
>> .resume_early in that driver to fimc_suspend() and fimc_resume(),
>> respectively, and leave the .suspend and .resume pointers unpopulated.
>>
>> Wouldn't that actually work?
>
> If we decide to ignore the power domain issue below, yes.
>
>>
>>> 3.
>>> The power domain runtime PM callbacks were by-passed during system
>>> suspend, my patch fixes this.
>>
>> I don't exactly understand this.  Why would they be bypassed?
>>
>>> Why do I want this? Because the power
>>> domain can have runtime PM resources it need to handle at this phase.
>>> Potentially, it could handle that from it's .suspend_late callback
>>> instead, but then it gets unnecessary complicated, which is what
>>> clearly happened to the power domain for OMAP2, for example.
>>
>> I'd like to understand this.  What exactly do you mean by "unnecessary
>> complicated"?
>
> Please, have a deeper look into the OMAP power domain implementation.
>
> If each SoC (for those that have power domain regulators) needs their
> own version of such a power domain, then I certainly think it is more
> complicated that in needs to be.
>
>>
>>>
>>> If you want additional proof of concepts, we can have a look at more
>>> complex example.
>>>
>>> Typically I am thinking of cases were a cross SoC driver is attached
>>> to a bus and for some SoCs a power domain as well. Then, the bus, the
>>> power domain and the driver - all have runtime PM resources to handle.
>>
>> Sure.
>
> OK, I consider resending the patch set, including some additional
> proof of concept patches.
>
>>
>>> In these cases using the new building blocks will not only
>>> significantly simplify code, but also fix immediate bugs. One example
>>> are drivers attached to the AMBA bus, at drivers/amba/bus.c.
>>
>> Again, what drivers and what's the bug you're talking about?
>
> I will use some of these as examples, it will be more visible to you then.
>
> Kind regards
> Uffe
>
>>
>> Rafael
>>

Re: [PATCH 0/5] PM: Enable option of re-use runtime PM callbacks at system suspend

[Rafael J. Wysocki]

On Friday, November 29, 2013 10:32:06 AM Ulf Hansson wrote:
> >
> > The lack of specificity here doesn't make the discussion any easier.
> >
> > It usually is better to talk about specific problems to address than
> > using general terms which may mean slightly different things for different
> > people.
> 
> During these discussions, I have tried to point at existing code for
> drivers and existing code for power domains. Those which at the moment
> either have got things wrong or are unnecessary complicated, in
> regards to their PM implementation.
> 
> I suppose I could provide some more patches for proof of concept, will
> that be a way forward?

I'd like to understand the problems you're trying to address first, to
be honest.

> >> >> Additionally, some drivers seems to have messed up things when combining
> >> >> runtime PM with system PM. While we enable the option of re-using the runtime
> >> >> PM callbacks during system PM, we also intend to clarify the way forward for
> >> >> how these scenarios could be resolved.
> >> >>
> >> >> Some new helper macros for defining PM callbacks and two new pm_generic*
> >> >> functions has been implemented in this patch set. These are provided to make it
> >> >> easier for those who wants to enable the option of re-using the runtime PM
> >> >> callbacks during system suspend.
> >> >
> >> > I'm generally opposed to re-using callbacks like this, because it adds confusion
> >> > to the picture.  It may seem to be clever, but in fact it leads to bad design
> >> > choices in the drivers in my opinion.
> >>
> >> In my world of the kernel, it will clearly resolve confusions and
> >> simplify a significant amount of code in power domains, buses and
> >> drivers. So I guess it depends on from what point you look at this.
> >
> > This is so vague that I don't even know how to respond. :-)
> >
> > So let me say instead that what you did in patch [5/5] is a layering violation
> > which always is a bug, even if it doesn't break things outright.
> >
> > After that patch the driver would call into a layer that is supposed to call
> > into it under normal conditions.  Moreover, it would expect that layer to
> > call back into it again in a specific way, which may or may not happen depending
> > on how exactly that layer is implemented.  So even if it works today, it will
> > add constraints on how that other layer may be implmented which *is* confusing
> > and wrong in my not so humble opinion.
> >
> > I'll never apply any patches that lead to situations like that, don't even
> > bother to send them to me.  Pretty please.
> >
> 
> After all these good discussions which clearly pointed the solution
> into this direction, you decide to bring up this argument now? It
> makes me wonder.
> 
> Indirectly what you are saying is that, the PM core should at
> device_prepare, do pm_runtime_disable() instead of
> pm_runtime_get_noresume(), to prevent drivers/subsystems from
> triggering "layering violations" by invoking pm_runtime_get_sync().
> 
> Because, this is exactly the same kind of layering violation you refer
> to while neglecting my approach, which at the moment
> drivers/subsystems not only are allowed to, but also encouraged to do
> during system suspend.

Well, to be honest, I'd never put a call to pm_runtime_get_sync() into
a driver's system suspend callback.

Subsystem callbacks are a different matter (see below).

> Now, obviously I don't think we shall change the behaviour of PM core,
> that would just not be convenient for subsystems and drivers, right?
> 
> So, the PM core allows layering violations for the .runtime_resume
> callbacks to be invoked during system suspend. It can do so, because
> it trust drivers/subsystems to act responsibly and to what suites them
> best.

Actually, there's different reason for that.  Some subsystems (not necessarily
drivers) resume devices during the prepare phase of system suspend in case
those devices need to be reprogrammed before entering system sleep (for
example, to change their wakeup settings).  That's done by the PCI bus type
and ACPI PM domain.

If that wasn't necessary, then yes, I would just disable the runtime PM
framework for all devices during the entire system suspend/resume.

> For the same reasons, I believe we should trust drivers/subsystems, to
> understand when it makes sense for them to re-use all of the runtime
> PM callbacks during system suspend and not just the .runtime_suspend
> callback.
> 
> That is in principle what I and Alan, who came up with this idea, are
> suggesting.

The problem with it is, as I said, the subsystem-level code you're calling
back through pm_generic_suspend_late_runtime() (and the other resume function)
has to be implemented in a specific way for things to work.  So it goes like
this: "OK, now I'm not runtime-suspended, so I need to do something about that.
Why don't I call back to the layer above me that has just called me (and that
surely won't do anything after I return, right?), so that it does the right
thing (which it surely will do, of course?) and calls my runtime PM callback
as expected".

And now suppose that your subsystem-level callbacks look like this (pseudo code):

a_suspend_late(dev)
{
	if (successful(pm_generic_suspend_late(dev)))
		do_X(dev);
}

a_runtime_suspend(dev)
{
	if (successful(pm_generic_runtime_suspend(dev)))
		do_Y(dev);
}

Then, if the driver uses your pm_generic_suspend_late_runtime(), the actually
executed code will be (assuming dev is not runtime-suspended):

	a_suspend_late(dev)
		driver->suspend_late(dev)
			a_runtime_suspend(dev)
				driver->runtime_suspend(dev)
				do_Y(dev)
	do_X(dev)

So what if do_X(dev) after do_Y(dev) doesn't actually work?

And what you actually want is

	driver->runtime_suspend(dev)
	do_Y(dev)

> >> And, as you stated previously during these discussions, we have the
> >> opportunity to update the documentation around this topic, I will
> >> happily do it, if needed.
> >
> > That's always welcome. :-)
> >
> >> >
> >> > Let's talk about specific examples, though.
> >> >
> >> > Why exactly do you need what patch [5/5] does in the exynos_drm_fimc driver?
> >>
> >> This was a simple example, I wanted to visualize how the new building
> >> blocks were going to be used. Anyway, this we achieve with the patch:
> >>
> >> 1.
> >> The PM part in the driver becomes simplified, we don't need the
> >> wrapper functions for the runtime PM callbacks any more.
> >
> > No, it is not simplified.  It becomes *far* more complicated conceptually
> > instead, although that is hidden by moving the complexity into the functions
> > added by patch [1/5].  So whoever doesn't look into those functions will
> > not actually realize how complicated the code really is.
> >
> >> 2.
> >> Previously the driver did not make sure runtime PM was disabled,
> >> before it put the device into low power state at .suspend. From a
> >> runtime PM point of view, this is not a "nice" behaviour and opens up
> >> for confusions, even if it likely would work in most cases.
> >
> > So the proper fix, in my opinion, would be to point .suspend_late and
> > .resume_early in that driver to fimc_suspend() and fimc_resume(),
> > respectively, and leave the .suspend and .resume pointers unpopulated.
> >
> > Wouldn't that actually work?
> 
> If we decide to ignore the power domain issue below, yes.
> 
> >
> >> 3.
> >> The power domain runtime PM callbacks were by-passed during system
> >> suspend, my patch fixes this.
> >
> > I don't exactly understand this.  Why would they be bypassed?
> >
> >> Why do I want this? Because the power
> >> domain can have runtime PM resources it need to handle at this phase.
> >> Potentially, it could handle that from it's .suspend_late callback
> >> instead, but then it gets unnecessary complicated, which is what
> >> clearly happened to the power domain for OMAP2, for example.
> >
> > I'd like to understand this.  What exactly do you mean by "unnecessary
> > complicated"?
> 
> Please, have a deeper look into the OMAP power domain implementation.

What files exactly do I need to look at?

> If each SoC (for those that have power domain regulators) needs their
> own version of such a power domain, then I certainly think it is more
> complicated that in needs to be.

They surely can share implementations.

> >
> >>
> >> If you want additional proof of concepts, we can have a look at more
> >> complex example.
> >>
> >> Typically I am thinking of cases were a cross SoC driver is attached
> >> to a bus and for some SoCs a power domain as well. Then, the bus, the
> >> power domain and the driver - all have runtime PM resources to handle.
> >
> > Sure.
> 
> OK, I consider resending the patch set, including some additional
> proof of concept patches.
> 
> >
> >> In these cases using the new building blocks will not only
> >> significantly simplify code, but also fix immediate bugs. One example
> >> are drivers attached to the AMBA bus, at drivers/amba/bus.c.
> >
> > Again, what drivers and what's the bug you're talking about?
> 
> I will use some of these as examples, it will be more visible to you then.

Well, I think I know what you're trying to do.  The problem is that in my
opinion the way you're trying to achieve that is not the right one.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/5] PM: Enable option of re-use runtime PM callbacks at system suspend

[Rafael J. Wysocki]

On Friday, November 29, 2013 02:52:20 PM Rafael J. Wysocki wrote:
> On Friday, November 29, 2013 10:32:06 AM Ulf Hansson wrote:

[...]

> > For the same reasons, I believe we should trust drivers/subsystems, to
> > understand when it makes sense for them to re-use all of the runtime
> > PM callbacks during system suspend and not just the .runtime_suspend
> > callback.
> > 
> > That is in principle what I and Alan, who came up with this idea, are
> > suggesting.
> 
> The problem with it is, as I said, the subsystem-level code you're calling
> back through pm_generic_suspend_late_runtime() (and the other resume function)
> has to be implemented in a specific way for things to work.  So it goes like
> this: "OK, now I'm not runtime-suspended, so I need to do something about that.
> Why don't I call back to the layer above me that has just called me (and that
> surely won't do anything after I return, right?), so that it does the right
> thing (which it surely will do, of course?) and calls my runtime PM callback
> as expected".
> 
> And now suppose that your subsystem-level callbacks look like this (pseudo code):
> 
> a_suspend_late(dev)
> {
> 	if (successful(pm_generic_suspend_late(dev)))
> 		do_X(dev);
> }
> 
> a_runtime_suspend(dev)
> {
> 	if (successful(pm_generic_runtime_suspend(dev)))
> 		do_Y(dev);
> }
> 
> Then, if the driver uses your pm_generic_suspend_late_runtime(), the actually
> executed code will be (assuming dev is not runtime-suspended):
> 
> 	a_suspend_late(dev)
> 		driver->suspend_late(dev)
> 			a_runtime_suspend(dev)
> 				driver->runtime_suspend(dev)
> 				do_Y(dev)
> 	do_X(dev)
> 
> So what if do_X(dev) after do_Y(dev) doesn't actually work?
> 
> And what you actually want is
> 
> 	driver->runtime_suspend(dev)
> 	do_Y(dev)

That should have been

 	driver->runtime_suspend(dev)
 	do_X(dev)

because do_Y(dev) is for runtime suspend.  Sorry.

And of course, the subsystem-level code you're developing the driver for may not
do the do_X(dev) thing at all, in which case all will work.  But what if someone
tries to use the driver with a different subsystem-level code (like a new PM
domain)?

Rafael

Re: [PATCH 0/5] PM: Enable option of re-use runtime PM callbacks at system suspend

[Alan Stern]

On Fri, 29 Nov 2013, Rafael J. Wysocki wrote:

> That should have been
> 
>  	driver->runtime_suspend(dev)
>  	do_X(dev)
> 
> because do_Y(dev) is for runtime suspend.  Sorry.
> 
> And of course, the subsystem-level code you're developing the driver for may not
> do the do_X(dev) thing at all, in which case all will work.  But what if someone
> tries to use the driver with a different subsystem-level code (like a new PM
> domain)?

It sounds like the problem is one of division of responsibilities.  
What part of the work is done by the subsystem, and what part is done 
by the driver?  Obviously the answer will vary from one subsystem to 
another.

As I understand it, in Ulf's situation the subsystem knows that the
operations needed for the suspend_late phase of system suspend are the
same as those carried out by the driver's runtime_suspend callback (if
the device isn't already in a runtime-PM low-power state).  Or perhaps
it knows they are the same as those carried out by the subsystem's
runtime_suspend callback.  Or perhaps the _driver_ knows that the
operations it needs for suspend_late are the same as those carried out
by its own runtime_suspend callback (if the device isn't already in a
low-power state).

Regardless, under those conditions the suspend_late routine merely has
to invoke the appropriate runtime_suspend routine, after checking the
device's runtime PM status.  We can't do this by calling
pm_runtime_suspend, because runtime PM is disabled during suspend_late.  
Instead, we have to call the proper runtime-suspend routine directly.

The amount of code needed is quite small; basically nothing more than:

	if (!pm_runtime_status_suspended(dev))
		dev->driver->pm->runtime_suspend(dev);

The problems are:

     1. Which callback does this code belong in?

     2. Which runtime_suspend callback should be invoked?  The example 
	above uses dev->driver->pm->runtime_suspend, but this may not 
	always be the right one.

Alan Stern

Re: [PATCH 0/5] PM: Enable option of re-use runtime PM callbacks at system suspend

[Rafael J. Wysocki]

On Friday, November 29, 2013 10:30:57 AM Alan Stern wrote:
> On Fri, 29 Nov 2013, Rafael J. Wysocki wrote:
> 
> > That should have been
> > 
> >  	driver->runtime_suspend(dev)
> >  	do_X(dev)
> > 
> > because do_Y(dev) is for runtime suspend.  Sorry.
> > 
> > And of course, the subsystem-level code you're developing the driver for may not
> > do the do_X(dev) thing at all, in which case all will work.  But what if someone
> > tries to use the driver with a different subsystem-level code (like a new PM
> > domain)?
> 
> It sounds like the problem is one of division of responsibilities.  
> What part of the work is done by the subsystem, and what part is done 
> by the driver?  Obviously the answer will vary from one subsystem to 
> another.
> 
> As I understand it, in Ulf's situation the subsystem knows that the
> operations needed for the suspend_late phase of system suspend are the
> same as those carried out by the driver's runtime_suspend callback (if
> the device isn't already in a runtime-PM low-power state).  Or perhaps
> it knows they are the same as those carried out by the subsystem's
> runtime_suspend callback.  Or perhaps the _driver_ knows that the
> operations it needs for suspend_late are the same as those carried out
> by its own runtime_suspend callback (if the device isn't already in a
> low-power state).
> 
> Regardless, under those conditions the suspend_late routine merely has
> to invoke the appropriate runtime_suspend routine, after checking the
> device's runtime PM status.  We can't do this by calling
> pm_runtime_suspend, because runtime PM is disabled during suspend_late.  
> Instead, we have to call the proper runtime-suspend routine directly.

That would be kind of OK, if the driver's .suspend_late() only invoked its own
.runtime_suspend(), what you did below.

But, in the Ulf's approach the driver calls .runtime_suspend() from
its *subsystem* in the hope that it will all work out properly (or
perhaps based on the knowledge about this particular subsystem).

As I said, that may lead to problems when the same driver is attempted to
be used with a different subsystem-level code (for example, a different PM
domain).

> The amount of code needed is quite small; basically nothing more than:
> 
> 	if (!pm_runtime_status_suspended(dev))
> 		dev->driver->pm->runtime_suspend(dev);
> 
> The problems are:
> 
>      1. Which callback does this code belong in?
> 
>      2. Which runtime_suspend callback should be invoked?  The example 
> 	above uses dev->driver->pm->runtime_suspend, but this may not 
> 	always be the right one.

Well, none of them may always be the right one, which is my point.

I don't think we can come up with a really generic set of routines that will
be generally useful regardless of the subsystem/driver combination in question,
so in my opinion the PM core is not the right place to put such routines into.

I wouldn't have problems with them being defined in the OMAP PM code.  As part
of that code they are free to do whatever they like if that code's maintainers
are fine with that.  But I don't want to see such confusing stuff in the core.

Anyway, I'd like to understand what problems in particular those things would
be useful to solve, as I suspect that they may be addressed differently and
more cleanly.

Thanks,
Rafael

Re: [PATCH 0/5] PM: Enable option of re-use runtime PM callbacks at system suspend

[Ulf Hansson]

>
> Well, to be honest, I'd never put a call to pm_runtime_get_sync() into
> a driver's system suspend callback.

Nevertheless, the PM core allows it to happen and there are not only
subsystem-level code but also drivers that use it, for whatever
reasons.

>
> Subsystem callbacks are a different matter (see below).
>
>> Now, obviously I don't think we shall change the behaviour of PM core,
>> that would just not be convenient for subsystems and drivers, right?
>>
>> So, the PM core allows layering violations for the .runtime_resume
>> callbacks to be invoked during system suspend. It can do so, because
>> it trust drivers/subsystems to act responsibly and to what suites them
>> best.
>
> Actually, there's different reason for that.  Some subsystems (not necessarily
> drivers) resume devices during the prepare phase of system suspend in case
> those devices need to be reprogrammed before entering system sleep (for
> example, to change their wakeup settings).  That's done by the PCI bus type
> and ACPI PM domain.
>
> If that wasn't necessary, then yes, I would just disable the runtime PM
> framework for all devices during the entire system suspend/resume.
>
>> For the same reasons, I believe we should trust drivers/subsystems, to
>> understand when it makes sense for them to re-use all of the runtime
>> PM callbacks during system suspend and not just the .runtime_suspend
>> callback.
>>
>> That is in principle what I and Alan, who came up with this idea, are
>> suggesting.
>
> The problem with it is, as I said, the subsystem-level code you're calling
> back through pm_generic_suspend_late_runtime() (and the other resume function)
> has to be implemented in a specific way for things to work.  So it goes like
> this: "OK, now I'm not runtime-suspended, so I need to do something about that.
> Why don't I call back to the layer above me that has just called me (and that
> surely won't do anything after I return, right?), so that it does the right
> thing (which it surely will do, of course?) and calls my runtime PM callback
> as expected".

Let me try elaborate some more on the scenario I am trying to improve
for, in hope to make it more clear.

These are some typical runtime PM resources a driver may control:

- When clocks can be gated/ungated.
- When pins can be put in sleep/default state.
- When to drop/fetch a reference to a power domain regulator.
- When register context need to be saved/restored.

It still does not make sense for the driver itself to handle each
resource. For example, the PM domain code needs to control the actual
PM domain regulator. At subsystem-level, a bus could for example
control a bus clock, as in the AMBA case (drivers/amba/bus.c).

Before we continue the discussion about where, how and if the new
pm_generic functions should be implemented, I want to highlight the
following important facts while we consider the options.

1.
The order of handling the runtime PM resources are important,
including during the system suspend sequence. In these scenarios, the
driver has by default the knowledge of how to put it's device into low
power state and then obviously also how to restore it to full power.

Since historically, drivers could not re-use the runtime PM callbacks
to handle system suspend; in the OMAP2 case, this knowledge were
transferred to the PM domain. Quickly you then realize the PM domain
will become complex, due to the fact that it needs to know details
about every device. In principle parts of the drivers were moved to
the PM domain.

2.
Drivers (especially Cross SoC drivers) shall be able to put it's
device into low power state during system suspend, both with and
without a PM domain. Obviously because the existence of the PM domain
is SoC specific.

To make it more clear, drivers must not rely on that the PM domain
re-uses and invokes the runtime PM callbacks during system suspend to
put it's device into low power state. This can only be initiated by
the driver.

>
> And now suppose that your subsystem-level callbacks look like this (pseudo code):
>
> a_suspend_late(dev)
> {
>         if (successful(pm_generic_suspend_late(dev)))
>                 do_X(dev);
> }
>
> a_runtime_suspend(dev)
> {
>         if (successful(pm_generic_runtime_suspend(dev)))
>                 do_Y(dev);
> }
>
> Then, if the driver uses your pm_generic_suspend_late_runtime(), the actually
> executed code will be (assuming dev is not runtime-suspended):
>
>         a_suspend_late(dev)
>                 driver->suspend_late(dev)
>                         a_runtime_suspend(dev)
>                                 driver->runtime_suspend(dev)
>                                 do_Y(dev)
>         do_X(dev)
>
> So what if do_X(dev) after do_Y(dev) doesn't actually work?
>
> And what you actually want is
>
>         driver->runtime_suspend(dev)
>         do_Y(dev)
>
>> >> And, as you stated previously during these discussions, we have the
>> >> opportunity to update the documentation around this topic, I will
>> >> happily do it, if needed.
>> >
>> > That's always welcome. :-)
>> >
>> >> >
>> >> > Let's talk about specific examples, though.
>> >> >
>> >> > Why exactly do you need what patch [5/5] does in the exynos_drm_fimc driver?
>> >>
>> >> This was a simple example, I wanted to visualize how the new building
>> >> blocks were going to be used. Anyway, this we achieve with the patch:
>> >>
>> >> 1.
>> >> The PM part in the driver becomes simplified, we don't need the
>> >> wrapper functions for the runtime PM callbacks any more.
>> >
>> > No, it is not simplified.  It becomes *far* more complicated conceptually
>> > instead, although that is hidden by moving the complexity into the functions
>> > added by patch [1/5].  So whoever doesn't look into those functions will
>> > not actually realize how complicated the code really is.
>> >
>> >> 2.
>> >> Previously the driver did not make sure runtime PM was disabled,
>> >> before it put the device into low power state at .suspend. From a
>> >> runtime PM point of view, this is not a "nice" behaviour and opens up
>> >> for confusions, even if it likely would work in most cases.
>> >
>> > So the proper fix, in my opinion, would be to point .suspend_late and
>> > .resume_early in that driver to fimc_suspend() and fimc_resume(),
>> > respectively, and leave the .suspend and .resume pointers unpopulated.
>> >
>> > Wouldn't that actually work?
>>
>> If we decide to ignore the power domain issue below, yes.
>>
>> >
>> >> 3.
>> >> The power domain runtime PM callbacks were by-passed during system
>> >> suspend, my patch fixes this.
>> >
>> > I don't exactly understand this.  Why would they be bypassed?
>> >
>> >> Why do I want this? Because the power
>> >> domain can have runtime PM resources it need to handle at this phase.
>> >> Potentially, it could handle that from it's .suspend_late callback
>> >> instead, but then it gets unnecessary complicated, which is what
>> >> clearly happened to the power domain for OMAP2, for example.
>> >
>> > I'd like to understand this.  What exactly do you mean by "unnecessary
>> > complicated"?
>>
>> Please, have a deeper look into the OMAP power domain implementation.
>
> What files exactly do I need to look at?

Start with this:

arch/arm/mach-omap2/omap_device.c

>
>> If each SoC (for those that have power domain regulators) needs their
>> own version of such a power domain, then I certainly think it is more
>> complicated that in needs to be.
>
> They surely can share implementations.
>
>> >
>> >>
>> >> If you want additional proof of concepts, we can have a look at more
>> >> complex example.
>> >>
>> >> Typically I am thinking of cases were a cross SoC driver is attached
>> >> to a bus and for some SoCs a power domain as well. Then, the bus, the
>> >> power domain and the driver - all have runtime PM resources to handle.
>> >
>> > Sure.
>>
>> OK, I consider resending the patch set, including some additional
>> proof of concept patches.
>>
>> >
>> >> In these cases using the new building blocks will not only
>> >> significantly simplify code, but also fix immediate bugs. One example
>> >> are drivers attached to the AMBA bus, at drivers/amba/bus.c.
>> >
>> > Again, what drivers and what's the bug you're talking about?
>>
>> I will use some of these as examples, it will be more visible to you then.
>
> Well, I think I know what you're trying to do.  The problem is that in my
> opinion the way you're trying to achieve that is not the right one.

I am not so sure I have managed to describe the scenarios in detail
which I am trying to solve. Hopefully the information in this reply
adds some more insight though.

It sounds like you think the direction of where my solution is heading
is totally wrong. I would certainly appreciate some advise from you at
this point, because I am kind of out ideas at the moment.

Kind regards
Ulf Hansson

>
> Thanks!
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/5] PM: Enable option of re-use runtime PM callbacks at system suspend

[Ulf Hansson]

>
> That would be kind of OK, if the driver's .suspend_late() only invoked its own
> .runtime_suspend(), what you did below.
>
> But, in the Ulf's approach the driver calls .runtime_suspend() from
> its *subsystem* in the hope that it will all work out properly (or
> perhaps based on the knowledge about this particular subsystem).

Correct assumption.

>
> As I said, that may lead to problems when the same driver is attempted to
> be used with a different subsystem-level code (for example, a different PM
> domain).

I don't think this is an issue we need to consider.

I assume the reason for having a PM domain is typically because you
have a PM domain regulator to handle. Certainly the PM domain,
controls it by using the runtime PM callbacks.

Thus it does not make sense to bypass the PM domain callbacks, and
just letting the driver call it's own runtime PM callbacks during
system suspend. It will only do parts of the work then.

>
>> The amount of code needed is quite small; basically nothing more than:
>>
>>       if (!pm_runtime_status_suspended(dev))
>>               dev->driver->pm->runtime_suspend(dev);
>>
>> The problems are:
>>
>>      1. Which callback does this code belong in?
>>
>>      2. Which runtime_suspend callback should be invoked?  The example
>>       above uses dev->driver->pm->runtime_suspend, but this may not
>>       always be the right one.
>
> Well, none of them may always be the right one, which is my point.
>
> I don't think we can come up with a really generic set of routines that will
> be generally useful regardless of the subsystem/driver combination in question,
> so in my opinion the PM core is not the right place to put such routines into.
>
> I wouldn't have problems with them being defined in the OMAP PM code.  As part
> of that code they are free to do whatever they like if that code's maintainers
> are fine with that.  But I don't want to see such confusing stuff in the core.

As I just replied in my other email, having the code to be available
and used in the PM domain code is not an option that we will gain
from. The OMAP PM domain already does this, though in a slightly other
form  - and side effects is not what we want.

The drivers need to re-use and invoke the runtime PM callbacks, that
is to me the only option that makes sense.

At the same time, I somewhat agree with you that the new functions I
propose to be added is not so "generic". What if we think of them as
pm_runtime helper functions instead, would that make more sense?

Kind regards
Uffe

>
> Anyway, I'd like to understand what problems in particular those things would
> be useful to solve, as I suspect that they may be addressed differently and
> more cleanly.
>
> Thanks,
> Rafael
>

Re: [PATCH 1/5] PM / Sleep: Add pm_generic functions to re-use runtime PM callbacks

[Rafael J. Wysocki]

On Wednesday, November 27, 2013 04:34:56 PM Ulf Hansson wrote:
> To put devices into low power state during sleep, it sometimes makes
> sense at subsystem-level to re-use the runtime PM callbacks.
> 
> The PM core will at device_suspend_late disable runtime PM, after that
> we can safely operate on these callbacks. At suspend_late the device
> will be put into low power state by invoking the runtime_suspend
> callbacks, unless the runtime status is already suspended. At
> resume_early the state is restored by invoking the runtime_resume
> callbacks. Soon after the PM core will re-enable runtime PM before
> returning from device_resume_early.
> 
> The new pm_generic functions are named pm_generic_suspend_late_runtime
> and pm_generic_resume_early_runtime, they are supposed to be used in
> pairs.
> 
> Do note that these new generic callbacks will work smothly even with
> and without CONFIG_PM_RUNTIME, as long as the runtime PM callbacks are
> implemented inside CONFIG_PM instead of CONFIG_PM_RUNTIME.
> 
> A special thanks to Alan Stern who came up with this idea.
> 
> Cc: Kevin Hilman <khilman@linaro.org>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/base/power/generic_ops.c |   86 ++++++++++++++++++++++++++++++++++++++
>  include/linux/pm.h               |    2 +
>  2 files changed, 88 insertions(+)
> 
> diff --git a/drivers/base/power/generic_ops.c b/drivers/base/power/generic_ops.c
> index 5ee030a..8e78ad1 100644
> --- a/drivers/base/power/generic_ops.c
> +++ b/drivers/base/power/generic_ops.c
> @@ -93,6 +93,49 @@ int pm_generic_suspend_late(struct device *dev)
>  EXPORT_SYMBOL_GPL(pm_generic_suspend_late);

After a bit of reconsideration it appears to me that the only benefit of that
would be to make it possible for drivers to work around incomplete PM domains
implementations.  Which would be of questionable value.

>  /**
> + * pm_generic_suspend_late_runtime - Generic suspend_late callback for drivers
> + * @dev: Device to suspend.
> + * Use to invoke runtime_suspend callbacks at suspend_late.
> + */
> +int pm_generic_suspend_late_runtime(struct device *dev)
> +{
> +	int (*callback)(struct device *);
> +	int ret = 0;
> +
> +	/*
> +	 * PM core has disabled runtime PM in device_suspend_late, thus we can
> +	 * safely check the device's runtime status and decide whether
> +	 * additional actions are needed to put the device into low power state.
> +	 * If so, we invoke the runtime_suspend callbacks.
> +	 * For the !CONFIG_PM_RUNTIME case, pm_runtime_status_suspended() always
> +	 * returns false and therefore the runtime_suspend callback will be
> +	 * invoked.
> +	 */
> +	if (pm_runtime_status_suspended(dev))
> +		return 0;
> +
> +	if (dev->pm_domain)
> +		callback = dev->pm_domain->ops.runtime_suspend;

First of all, for this to work you need to assume that the type/bus/class will
not exercise hardware in any way by itself (or you can look at the PCI bus type
for an example why it won't generally work otherwise), so you could simply skip
the rest of this conditional.

For the bus types you are concerned about (platform and AMBA) that actually is
the case as far as I can say.

> +	else if (dev->type && dev->type->pm)
> +		callback = dev->type->pm->runtime_suspend;
> +	else if (dev->class && dev->class->pm)
> +		callback = dev->class->pm->runtime_suspend;
> +	else if (dev->bus && dev->bus->pm)
> +		callback = dev->bus->pm->runtime_suspend;
> +	else
> +		callback = NULL;
> +
> +	if (!callback && dev->driver && dev->driver->pm)
> +		callback = dev->driver->pm->runtime_suspend;
> +
> +	if (callback)
> +		ret = callback(dev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(pm_generic_suspend_late_runtime);

After that you're left with something like this:

int prototype_suspend_late(struct device *dev)
{
	int (*callback)(struct device *);

	if (pm_runtime_status_suspended(dev))
		return 0;

	if (dev->pm_domain)
		callback = dev->pm_domain->ops.runtime_suspend;
	else if (dev->driver && dev->driver->pm)
		callback = dev->driver->pm->runtime_suspend;
	else
		callback = NULL;

	return callback ? callback(dev) : 0;
}

Moreover, if a driver's .suspend_late is pointed to the above, it can be called
in two ways.  The first way is via type/bus/class or the PM core itself which
means that all PM handling at this stage is going to be done by
prototype_suspend_late().  The other way is via a PM domain, in which case
there may be some additional PM handling in the domain code in principle
(before or after calling the driver's .suspend_late()).  However, in the
latter case it generally wouldn't be OK to call PM domain's .runtime_suspend(),
because that may interfere with the PM handling in its .suspend_late().  So
again, this pretty much requires that the PM domain's .suspend_late pointer be
NULL or point to a routine that simply executes the driver's callback and does
noting in addition to that.

Now, I suspect that if the driver's runtime_suspend callback is
driver_runtime_suspend() and the PM domain's runtime_suspend callback is
pm_domain_runtime_suspend(), the code you actually want to be executed at the
"late suspend" stage will be

	if (!pm_runtime_status_suspended(dev))
		pm_domain_runtime_suspend()
			driver_runtime_suspend()

(where the assumption is that pm_domain_runtime_suspend() will call
driver_runtime_suspend() for you).  Clearly, prototype_suspend_late() will lead
to that effective result in the conditions in which it makes sense to use it.

So, instead of pointing the driver's .suspend_late() to prototype_suspend_late(),
why don't you point the .suspend_late of the *PM* *domain* to the following
slightly modified version of that function:

int pm_domain_simplified_suspend_late(struct device *dev)
{
	int (*callback)(struct device *) = NULL;

	if (pm_runtime_status_suspended(dev))
		return 0;

	/* Try to execute our own .runtime_suspend() callback. */
	if (dev->pm_domain)
		callback = dev->pm_domain->ops.runtime_suspend;

	if (WARN_ON(!callback) && dev->driver && dev->driver->pm)
		callback = dev->driver->pm->runtime_suspend;

	return callback ? callback(dev) : 0;
}

This will cause exactly the code you need to be executed too (with
a fallback to direct execution of the driver's callback in broken
cases where the PM domain's own .runtime_suspend() is not implemented),
but in a much cleaner way (no going back to the code layer that has just
called you etc.).  And you *can* point the PM domain's .suspend_late
to this, because it has to be either empty of trivial if the
prototype_suspend_late() above is supposed to work as the driver's
.suspend_late() callback.

So in my opinion, if you point the .suspend_late callback pointers of the
PM domains in question to the pm_domain_simplified_suspend_late() above
and their .resume_early callback pointers to the following function:

int pm_domain_simplified_resume_early(struct device *dev)
{
	int (*callback)(struct device *) = NULL;

	if (pm_runtime_status_suspended(dev))
		return 0;

	if (dev->pm_domain)
		callback = dev->pm_domain->ops.runtime_resume;

	if (WARN_ON(!callback) && dev->driver && dev->driver->pm)
		callback = dev->driver->pm->runtime_resume;

	return callback ? callback(dev) : 0;
}

it will solve your problems without that horrible jumping between code
layers.

Thanks,
Rafael

Re: [PATCH 1/5] PM / Sleep: Add pm_generic functions to re-use runtime PM callbacks

[Rafael J. Wysocki]

On Wednesday, December 04, 2013 12:15:13 AM Rafael J. Wysocki wrote:
> On Wednesday, November 27, 2013 04:34:56 PM Ulf Hansson wrote:
> > To put devices into low power state during sleep, it sometimes makes
> > sense at subsystem-level to re-use the runtime PM callbacks.
> > 
> > The PM core will at device_suspend_late disable runtime PM, after that
> > we can safely operate on these callbacks. At suspend_late the device
> > will be put into low power state by invoking the runtime_suspend
> > callbacks, unless the runtime status is already suspended. At
> > resume_early the state is restored by invoking the runtime_resume
> > callbacks. Soon after the PM core will re-enable runtime PM before
> > returning from device_resume_early.
> > 
> > The new pm_generic functions are named pm_generic_suspend_late_runtime
> > and pm_generic_resume_early_runtime, they are supposed to be used in
> > pairs.
> > 
> > Do note that these new generic callbacks will work smothly even with
> > and without CONFIG_PM_RUNTIME, as long as the runtime PM callbacks are
> > implemented inside CONFIG_PM instead of CONFIG_PM_RUNTIME.
> > 
> > A special thanks to Alan Stern who came up with this idea.
> > 
> > Cc: Kevin Hilman <khilman@linaro.org>
> > Cc: Alan Stern <stern@rowland.harvard.edu>
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/base/power/generic_ops.c |   86 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/pm.h               |    2 +
> >  2 files changed, 88 insertions(+)
> > 
> > diff --git a/drivers/base/power/generic_ops.c b/drivers/base/power/generic_ops.c
> > index 5ee030a..8e78ad1 100644
> > --- a/drivers/base/power/generic_ops.c
> > +++ b/drivers/base/power/generic_ops.c
> > @@ -93,6 +93,49 @@ int pm_generic_suspend_late(struct device *dev)
> >  EXPORT_SYMBOL_GPL(pm_generic_suspend_late);
> 
> After a bit of reconsideration it appears to me that the only benefit of that
> would be to make it possible for drivers to work around incomplete PM domains
> implementations.  Which would be of questionable value.
> 
> >  /**
> > + * pm_generic_suspend_late_runtime - Generic suspend_late callback for drivers
> > + * @dev: Device to suspend.
> > + * Use to invoke runtime_suspend callbacks at suspend_late.
> > + */
> > +int pm_generic_suspend_late_runtime(struct device *dev)
> > +{
> > +	int (*callback)(struct device *);
> > +	int ret = 0;
> > +
> > +	/*
> > +	 * PM core has disabled runtime PM in device_suspend_late, thus we can
> > +	 * safely check the device's runtime status and decide whether
> > +	 * additional actions are needed to put the device into low power state.
> > +	 * If so, we invoke the runtime_suspend callbacks.
> > +	 * For the !CONFIG_PM_RUNTIME case, pm_runtime_status_suspended() always
> > +	 * returns false and therefore the runtime_suspend callback will be
> > +	 * invoked.
> > +	 */
> > +	if (pm_runtime_status_suspended(dev))
> > +		return 0;
> > +
> > +	if (dev->pm_domain)
> > +		callback = dev->pm_domain->ops.runtime_suspend;
> 
> First of all, for this to work you need to assume that the type/bus/class will
> not exercise hardware in any way by itself (or you can look at the PCI bus type
> for an example why it won't generally work otherwise), so you could simply skip
> the rest of this conditional.
> 
> For the bus types you are concerned about (platform and AMBA) that actually is
> the case as far as I can say.
> 
> > +	else if (dev->type && dev->type->pm)
> > +		callback = dev->type->pm->runtime_suspend;
> > +	else if (dev->class && dev->class->pm)
> > +		callback = dev->class->pm->runtime_suspend;
> > +	else if (dev->bus && dev->bus->pm)
> > +		callback = dev->bus->pm->runtime_suspend;
> > +	else
> > +		callback = NULL;
> > +
> > +	if (!callback && dev->driver && dev->driver->pm)
> > +		callback = dev->driver->pm->runtime_suspend;
> > +
> > +	if (callback)
> > +		ret = callback(dev);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(pm_generic_suspend_late_runtime);
> 
> After that you're left with something like this:
> 
> int prototype_suspend_late(struct device *dev)
> {
> 	int (*callback)(struct device *);
> 
> 	if (pm_runtime_status_suspended(dev))
> 		return 0;
> 
> 	if (dev->pm_domain)
> 		callback = dev->pm_domain->ops.runtime_suspend;
> 	else if (dev->driver && dev->driver->pm)
> 		callback = dev->driver->pm->runtime_suspend;
> 	else
> 		callback = NULL;
> 
> 	return callback ? callback(dev) : 0;
> }
> 
> Moreover, if a driver's .suspend_late is pointed to the above, it can be called
> in two ways.  The first way is via type/bus/class or the PM core itself which
> means that all PM handling at this stage is going to be done by
> prototype_suspend_late().  The other way is via a PM domain, in which case
> there may be some additional PM handling in the domain code in principle
> (before or after calling the driver's .suspend_late()).  However, in the
> latter case it generally wouldn't be OK to call PM domain's .runtime_suspend(),
> because that may interfere with the PM handling in its .suspend_late().  So
> again, this pretty much requires that the PM domain's .suspend_late pointer be
> NULL or point to a routine that simply executes the driver's callback and does
> noting in addition to that.
> 
> Now, I suspect that if the driver's runtime_suspend callback is
> driver_runtime_suspend() and the PM domain's runtime_suspend callback is
> pm_domain_runtime_suspend(), the code you actually want to be executed at the
> "late suspend" stage will be
> 
> 	if (!pm_runtime_status_suspended(dev))
> 		pm_domain_runtime_suspend()
> 			driver_runtime_suspend()
> 
> (where the assumption is that pm_domain_runtime_suspend() will call
> driver_runtime_suspend() for you).  Clearly, prototype_suspend_late() will lead
> to that effective result in the conditions in which it makes sense to use it.
> 
> So, instead of pointing the driver's .suspend_late() to prototype_suspend_late(),
> why don't you point the .suspend_late of the *PM* *domain* to the following
> slightly modified version of that function:
> 
> int pm_domain_simplified_suspend_late(struct device *dev)
> {
> 	int (*callback)(struct device *) = NULL;
> 
> 	if (pm_runtime_status_suspended(dev))
> 		return 0;
> 
> 	/* Try to execute our own .runtime_suspend() callback. */
> 	if (dev->pm_domain)
> 		callback = dev->pm_domain->ops.runtime_suspend;
> 
> 	if (WARN_ON(!callback) && dev->driver && dev->driver->pm)
> 		callback = dev->driver->pm->runtime_suspend;
> 
> 	return callback ? callback(dev) : 0;
> }
> 
> This will cause exactly the code you need to be executed too (with
> a fallback to direct execution of the driver's callback in broken
> cases where the PM domain's own .runtime_suspend() is not implemented),
> but in a much cleaner way (no going back to the code layer that has just
> called you etc.).  And you *can* point the PM domain's .suspend_late
> to this, because it has to be either empty of trivial if the
> prototype_suspend_late() above is supposed to work as the driver's
> .suspend_late() callback.
> 
> So in my opinion, if you point the .suspend_late callback pointers of the
> PM domains in question to the pm_domain_simplified_suspend_late() above
> and their .resume_early callback pointers to the following function:
> 
> int pm_domain_simplified_resume_early(struct device *dev)
> {
> 	int (*callback)(struct device *) = NULL;
> 
> 	if (pm_runtime_status_suspended(dev))
> 		return 0;
> 
> 	if (dev->pm_domain)
> 		callback = dev->pm_domain->ops.runtime_resume;
> 
> 	if (WARN_ON(!callback) && dev->driver && dev->driver->pm)
> 		callback = dev->driver->pm->runtime_resume;
> 
> 	return callback ? callback(dev) : 0;
> }
> 
> it will solve your problems without that horrible jumping between code
> layers.

And in addition to that you can point the drivers' .suspend_late callbacks to
something like:

int pm_simplified_suspend_late(struct device *dev)
{ 
 	if (pm_runtime_status_suspended(dev))
 		return 0;

 	return dev->driver->pm->runtime_suspend ?
 		dev->driver->pm->runtime_suspend(dev) : 0;
}

(and analogously for the "early resume") which will cause their .runtime_suspend()
to be executed even if the PM domain doesn't have a .suspend_late (or there's
no PM domain at all).

Thanks,
Rafael

Re: [PATCH 1/5] PM / Sleep: Add pm_generic functions to re-use runtime PM callbacks

[Ulf Hansson]

On 4 December 2013 00:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, December 04, 2013 12:15:13 AM Rafael J. Wysocki wrote:
>> On Wednesday, November 27, 2013 04:34:56 PM Ulf Hansson wrote:
>> > To put devices into low power state during sleep, it sometimes makes
>> > sense at subsystem-level to re-use the runtime PM callbacks.
>> >
>> > The PM core will at device_suspend_late disable runtime PM, after that
>> > we can safely operate on these callbacks. At suspend_late the device
>> > will be put into low power state by invoking the runtime_suspend
>> > callbacks, unless the runtime status is already suspended. At
>> > resume_early the state is restored by invoking the runtime_resume
>> > callbacks. Soon after the PM core will re-enable runtime PM before
>> > returning from device_resume_early.
>> >
>> > The new pm_generic functions are named pm_generic_suspend_late_runtime
>> > and pm_generic_resume_early_runtime, they are supposed to be used in
>> > pairs.
>> >
>> > Do note that these new generic callbacks will work smothly even with
>> > and without CONFIG_PM_RUNTIME, as long as the runtime PM callbacks are
>> > implemented inside CONFIG_PM instead of CONFIG_PM_RUNTIME.
>> >
>> > A special thanks to Alan Stern who came up with this idea.
>> >
>> > Cc: Kevin Hilman <khilman@linaro.org>
>> > Cc: Alan Stern <stern@rowland.harvard.edu>
>> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> > ---
>> >  drivers/base/power/generic_ops.c |   86 ++++++++++++++++++++++++++++++++++++++
>> >  include/linux/pm.h               |    2 +
>> >  2 files changed, 88 insertions(+)
>> >
>> > diff --git a/drivers/base/power/generic_ops.c b/drivers/base/power/generic_ops.c
>> > index 5ee030a..8e78ad1 100644
>> > --- a/drivers/base/power/generic_ops.c
>> > +++ b/drivers/base/power/generic_ops.c
>> > @@ -93,6 +93,49 @@ int pm_generic_suspend_late(struct device *dev)
>> >  EXPORT_SYMBOL_GPL(pm_generic_suspend_late);
>>
>> After a bit of reconsideration it appears to me that the only benefit of that
>> would be to make it possible for drivers to work around incomplete PM domains
>> implementations.  Which would be of questionable value.
>>
>> >  /**
>> > + * pm_generic_suspend_late_runtime - Generic suspend_late callback for drivers
>> > + * @dev: Device to suspend.
>> > + * Use to invoke runtime_suspend callbacks at suspend_late.
>> > + */
>> > +int pm_generic_suspend_late_runtime(struct device *dev)
>> > +{
>> > +   int (*callback)(struct device *);
>> > +   int ret = 0;
>> > +
>> > +   /*
>> > +    * PM core has disabled runtime PM in device_suspend_late, thus we can
>> > +    * safely check the device's runtime status and decide whether
>> > +    * additional actions are needed to put the device into low power state.
>> > +    * If so, we invoke the runtime_suspend callbacks.
>> > +    * For the !CONFIG_PM_RUNTIME case, pm_runtime_status_suspended() always
>> > +    * returns false and therefore the runtime_suspend callback will be
>> > +    * invoked.
>> > +    */
>> > +   if (pm_runtime_status_suspended(dev))
>> > +           return 0;
>> > +
>> > +   if (dev->pm_domain)
>> > +           callback = dev->pm_domain->ops.runtime_suspend;
>>
>> First of all, for this to work you need to assume that the type/bus/class will
>> not exercise hardware in any way by itself (or you can look at the PCI bus type
>> for an example why it won't generally work otherwise), so you could simply skip
>> the rest of this conditional.
>>
>> For the bus types you are concerned about (platform and AMBA) that actually is
>> the case as far as I can say.
>>
>> > +   else if (dev->type && dev->type->pm)
>> > +           callback = dev->type->pm->runtime_suspend;
>> > +   else if (dev->class && dev->class->pm)
>> > +           callback = dev->class->pm->runtime_suspend;
>> > +   else if (dev->bus && dev->bus->pm)
>> > +           callback = dev->bus->pm->runtime_suspend;
>> > +   else
>> > +           callback = NULL;
>> > +
>> > +   if (!callback && dev->driver && dev->driver->pm)
>> > +           callback = dev->driver->pm->runtime_suspend;
>> > +
>> > +   if (callback)
>> > +           ret = callback(dev);
>> > +
>> > +   return ret;
>> > +}
>> > +EXPORT_SYMBOL_GPL(pm_generic_suspend_late_runtime);
>>
>> After that you're left with something like this:
>>
>> int prototype_suspend_late(struct device *dev)
>> {
>>       int (*callback)(struct device *);
>>
>>       if (pm_runtime_status_suspended(dev))
>>               return 0;
>>
>>       if (dev->pm_domain)
>>               callback = dev->pm_domain->ops.runtime_suspend;
>>       else if (dev->driver && dev->driver->pm)
>>               callback = dev->driver->pm->runtime_suspend;
>>       else
>>               callback = NULL;
>>
>>       return callback ? callback(dev) : 0;
>> }
>>
>> Moreover, if a driver's .suspend_late is pointed to the above, it can be called
>> in two ways.  The first way is via type/bus/class or the PM core itself which
>> means that all PM handling at this stage is going to be done by
>> prototype_suspend_late().  The other way is via a PM domain, in which case
>> there may be some additional PM handling in the domain code in principle
>> (before or after calling the driver's .suspend_late()).  However, in the
>> latter case it generally wouldn't be OK to call PM domain's .runtime_suspend(),
>> because that may interfere with the PM handling in its .suspend_late().  So
>> again, this pretty much requires that the PM domain's .suspend_late pointer be
>> NULL or point to a routine that simply executes the driver's callback and does
>> noting in addition to that.
>>
>> Now, I suspect that if the driver's runtime_suspend callback is
>> driver_runtime_suspend() and the PM domain's runtime_suspend callback is
>> pm_domain_runtime_suspend(), the code you actually want to be executed at the
>> "late suspend" stage will be
>>
>>       if (!pm_runtime_status_suspended(dev))
>>               pm_domain_runtime_suspend()
>>                       driver_runtime_suspend()
>>
>> (where the assumption is that pm_domain_runtime_suspend() will call
>> driver_runtime_suspend() for you).  Clearly, prototype_suspend_late() will lead
>> to that effective result in the conditions in which it makes sense to use it.
>>
>> So, instead of pointing the driver's .suspend_late() to prototype_suspend_late(),
>> why don't you point the .suspend_late of the *PM* *domain* to the following
>> slightly modified version of that function:
>>
>> int pm_domain_simplified_suspend_late(struct device *dev)
>> {
>>       int (*callback)(struct device *) = NULL;
>>
>>       if (pm_runtime_status_suspended(dev))
>>               return 0;
>>
>>       /* Try to execute our own .runtime_suspend() callback. */
>>       if (dev->pm_domain)
>>               callback = dev->pm_domain->ops.runtime_suspend;
>>
>>       if (WARN_ON(!callback) && dev->driver && dev->driver->pm)
>>               callback = dev->driver->pm->runtime_suspend;
>>
>>       return callback ? callback(dev) : 0;
>> }
>>
>> This will cause exactly the code you need to be executed too (with
>> a fallback to direct execution of the driver's callback in broken
>> cases where the PM domain's own .runtime_suspend() is not implemented),
>> but in a much cleaner way (no going back to the code layer that has just
>> called you etc.).  And you *can* point the PM domain's .suspend_late
>> to this, because it has to be either empty of trivial if the
>> prototype_suspend_late() above is supposed to work as the driver's
>> .suspend_late() callback.
>>
>> So in my opinion, if you point the .suspend_late callback pointers of the
>> PM domains in question to the pm_domain_simplified_suspend_late() above
>> and their .resume_early callback pointers to the following function:
>>
>> int pm_domain_simplified_resume_early(struct device *dev)
>> {
>>       int (*callback)(struct device *) = NULL;
>>
>>       if (pm_runtime_status_suspended(dev))
>>               return 0;
>>
>>       if (dev->pm_domain)
>>               callback = dev->pm_domain->ops.runtime_resume;
>>
>>       if (WARN_ON(!callback) && dev->driver && dev->driver->pm)
>>               callback = dev->driver->pm->runtime_resume;
>>
>>       return callback ? callback(dev) : 0;
>> }
>>
>> it will solve your problems without that horrible jumping between code
>> layers.
>
> And in addition to that you can point the drivers' .suspend_late callbacks to
> something like:
>
> int pm_simplified_suspend_late(struct device *dev)
> {
>         if (pm_runtime_status_suspended(dev))
>                 return 0;
>
>         return dev->driver->pm->runtime_suspend ?
>                 dev->driver->pm->runtime_suspend(dev) : 0;
> }
>
> (and analogously for the "early resume") which will cause their .runtime_suspend()
> to be executed even if the PM domain doesn't have a .suspend_late (or there's
> no PM domain at all).

Rafael, I highly appreciate your detailed reply!

Let' me try to summarize my picture to try to wrap up the discussion:

1.
I interpret your answer as we are aligned on that re-using the runtime
PM callbacks during system suspend, would in some cases be useful and
accepted. That is great news to hear and definitely the most important
part! :-)

I suppose that this also means we should go forward with the following
patches, right?

[PATCH 2/5] PM / Runtime: Implement the pm_generic_runtime functions
for CONFIG_PM

[PATCH 3/5] PM / Runtime: Add second macro for definition of runtime
PM callbacks

[PATCH 4/5] PM / Sleep: Add macro to define common late/early system
PM callbacks

2.
With your suggestions for how the PM domain shall implement it's
.suspend_late, I do in some cases see a problem for the driver.
Particularly in those scenarios were the driver needs full control of
the sequence of putting it's device into low power state during system
suspend. That is because the sequence is not initiated by the driver
but instead from the PM domain.

To be more clear, I don't think defining driver's .suspend_late
callbacks directly to the pm_generic_suspend_late_runtime function
would be enough in all cases.

Sometimes the driver have additional actions to complete during system
suspend - and the order of how things are done are important. Thus I
see the "pm_generic_suspend_late_runtime" also being invoked as a
function call from drivers .suspend_late callbacks to be able to
handle these cases.

I mentioned earlier that it could be better to view the
pm_generic_suspend_late_runtime more as "helper function" instead of
only as a callback.

To move things one step further, I think it would make sense to add a
pm_runtime_disable() in the beginning of
pm_generic_suspend_late_runtime(), to clarify that it requires runtime
PM to be disabled (runtime PM will then be re-enabled in the end of
pm_generic_resume_early_runtime()).

This would provide the option of using them as a "standalone" helper
functions, since those would no more rely on the PM core to disable
runtime PM from device_suspend_late.

3.
Finally, I understand you rejects to have the
pm_generic_suspend_late_runtime and pm_generic_resume_early_runtime
functions as a part of the PM core, at least in it's current form.

Considering my reply here, were I suggest to move them out from the
file with the generic callbacks and instead have them somewhere were
we can consider them as "helper functions", would that matter to your
opinion? Not sure were those would belong though, maybe closer to
runtime PM but still available for CONFIG_PM. :-)

An option would obviously be to not provide these kind of functions at
all and instead leave it to be implemented wherever it may be needed.
I just fear it would be duplicated code around, but it might be more
clear of what goes on then. :-)

Kind regards
Uffe

>
> Thanks,
> Rafael
>

Re: [PATCH 0/5] PM: Enable option of re-use runtime PM callbacks at system suspend

[Len Brown]

This thread raises the question...

Do we still need to have PM_RUNTIME apart from PM_SLEEP?

What is the benefit of being able to build-in one one without the other?
If that benefit is not significant, perhaps the time has come to
replace them both with CONFIG_PM...

cheers,
-Len Brown, Intel Open Source Technology Center

Re: [PATCH 0/5] PM: Enable option of re-use runtime PM callbacks at system suspend

[Alan Stern]

On Thu, 5 Dec 2013, Len Brown wrote:

> This thread raises the question...
> 
> Do we still need to have PM_RUNTIME apart from PM_SLEEP?
> 
> What is the benefit of being able to build-in one one without the other?
> If that benefit is not significant, perhaps the time has come to
> replace them both with CONFIG_PM...

There are lots of embedded/SoC platforms that implement PM_RUNTIME but
not PM_SLEEP.

Alan Stern

Re: [PATCH 0/5] PM: Enable option of re-use runtime PM callbacks at system suspend

[Pavel Machek]

On Thu 2013-12-05 17:21:50, Alan Stern wrote:
> On Thu, 5 Dec 2013, Len Brown wrote:
> 
> > This thread raises the question...
> > 
> > Do we still need to have PM_RUNTIME apart from PM_SLEEP?
> > 
> > What is the benefit of being able to build-in one one without the other?
> > If that benefit is not significant, perhaps the time has come to
> > replace them both with CONFIG_PM...
> 
> There are lots of embedded/SoC platforms that implement PM_RUNTIME but
> not PM_SLEEP.

And lots of PCs where PM_SLEEP is useful but PM_RUNTIME is not :-).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html