PM / Domains: Fix initial default state of the need_restore flag

PATCH New Ulf Hansson 7 participants 21 messages Nov 7, 2014 – Nov 14, 2014

Subsystems (6)

Arm Samsung Exynos Driver core Suspend to RAM Hibernation +2 more subsystems

lore

Share & tools

lore

Ulf Hansson

author

2014-11-07 13:27 UTC #0 lore raw

PATCH 1/1 PM / Domains: Fix initial default state of the need_restore flag

The initial state of the device's need_restore flag should'nt depend on the current state of the PM domain. For example it should be perfectly valid to attach an inactive device to a powered PM domain.

The pm_genpd_dev_need_restore() API allow us to update the need_restore flag to somewhat cope with such scenarios. Typically that should have been done from drivers/buses ->probe() since it's those that put the requirements on the value of the need_restore flag.

Until recently, the Exynos SOCs were the only user of the pm_genpd_dev_need_restore() API, though invoking it from a centralized location while adding devices to their PM domains.

Due to that Exynos now have swithed to the generic OF-based PM domain look-up, it's no longer possible to invoke the API from a centralized location. The reason is because devices are now added to their PM domains during the probe sequence.

Commit "ARM: exynos: Move to generic PM domain DT bindings" did the switch for Exynos to the generic OF-based PM domain look-up, but it also removed the call to pm_genpd_dev_need_restore(). This caused a regression for some of the Exynos drivers.

To handle things more properly in the generic PM domain, let's change the default initial value of the need_restore flag to reflect that the state is unknown. As soon as some of the runtime PM callbacks gets invoked, update the initial value accordingly.

Moreover, since the generic PM domain is verifying that all device's are both runtime PM enabled and suspended, using pm_runtime_suspended() while pm_genpd_poweroff() is invoked from the scheduled work, we can be sure of that the PM domain won't be powering off while having active devices.

Do note that, the generic PM domain can still only know about active devices which has been activated through invoking its runtime PM resume callback. In other words, buses/drivers using pm_runtime_set_active() during ->probe() will still suffer from a race condition, potentially probing a device without having its PM domain being powered. That issue will have to be solved using a different approach.

This a log from the boot regression for Exynos5, which is being fixed in this patch.

------------[ cut here ]------------
WARNING: CPU: 0 PID: 308 at ../drivers/clk/clk.c:851 clk_disable+0x24/0x30()
Modules linked in:
CPU: 0 PID: 308 Comm: kworker/0:1 Not tainted 3.18.0-rc3-00569-gbd9449f-dirty #10
Workqueue: pm pm_runtime_work
[<c0013c64>] (unwind_backtrace) from [<c0010dec>] (show_stack+0x10/0x14)
[<c0010dec>] (show_stack) from [<c03ee4cc>] (dump_stack+0x70/0xbc)
[<c03ee4cc>] (dump_stack) from [<c0020d34>] (warn_slowpath_common+0x64/0x88)
[<c0020d34>] (warn_slowpath_common) from [<c0020d74>] (warn_slowpath_null+0x1c/0x24)
[<c0020d74>] (warn_slowpath_null) from [<c03107b0>] (clk_disable+0x24/0x30)
[<c03107b0>] (clk_disable) from [<c02cc834>] (gsc_runtime_suspend+0x128/0x160)
[<c02cc834>] (gsc_runtime_suspend) from [<c0249024>] (pm_generic_runtime_suspend+0x2c/0x38)
[<c0249024>] (pm_generic_runtime_suspend) from [<c024f44c>] (pm_genpd_default_save_state+0x2c/0x8c)
[<c024f44c>] (pm_genpd_default_save_state) from [<c024ff2c>] (pm_genpd_poweroff+0x224/0x3ec)
[<c024ff2c>] (pm_genpd_poweroff) from [<c02501b4>] (pm_genpd_runtime_suspend+0x9c/0xcc)
[<c02501b4>] (pm_genpd_runtime_suspend) from [<c024a4f8>] (__rpm_callback+0x2c/0x60)
[<c024a4f8>] (__rpm_callback) from [<c024a54c>] (rpm_callback+0x20/0x74)
[<c024a54c>] (rpm_callback) from [<c024a930>] (rpm_suspend+0xd4/0x43c)
[<c024a930>] (rpm_suspend) from [<c024bbcc>] (pm_runtime_work+0x80/0x90)
[<c024bbcc>] (pm_runtime_work) from [<c0032a9c>] (process_one_work+0x12c/0x314)
[<c0032a9c>] (process_one_work) from [<c0032cf4>] (worker_thread+0x3c/0x4b0)
[<c0032cf4>] (worker_thread) from [<c003747c>] (kthread+0xcc/0xe8)
[<c003747c>] (kthread) from [<c000e738>] (ret_from_fork+0x14/0x3c)
---[ end trace 40cd58bcd6988f12 ]---

Fixes a4a8c2c4962bb655 (ARM: exynos: Move to generic PM domain DT bindings)

Reported-by Sylwester Nawrocki <[email protected]>

Signed-off-by Ulf Hansson <[email protected]>

---

This patch is intended as fix for 3.18 rc[n] due to the regression for Exynos SOCs.

I would also like to call for help in getting this thoroughly tested.

I have tested this on Arndale Dual, Exynos 5250. According the log attached in the commit message as well.

I have tested this on UX500, which support for the generic PM domain is about to be queued for 3.19. Since UX500 uses the AMBA bus/drivers, which uses pm_runtime_set_active() instead of pm_runtime_get_sync() during ->probe(), I could also verify that this behavior is maintained.

Finally I have also tested this patchset on UX500 and using the below patchset to make sure the approach is suitable long term wise as well. [PATCH v3 0/9] PM / Domains: Fix race conditions during boot
http://www.spinics.net/lists/arm-kernel/msg369409.html

---
 drivers/base/power/domain.c | 38 +++++++++++++++++++++++++++++++-------
 include/linux/pm_domain.h   |  2 +-
 2 files changed, 32 insertions(+), 8 deletions(-)

drivers/base/power/domain.c +31 -7

index b520687..a9523a3 100644
@@ -361,9 +361,19 @@ static int __pm_genpd_save_device(struct pm_domain_data *pdd,
 	struct device *dev = pdd->dev;
 	int ret = 0;
 
-	if (gpd_data->need_restore)
+	if (gpd_data->need_restore == 1)
 		return 0;
 
+	/*
+	 * If the value of the need_restore flag is still unknown at this point,
+	 * we trust that pm_genpd_poweroff() has verified that the device is
+	 * already runtime PM suspended.
+	 */
+	if (gpd_data->need_restore == -1) {
+		gpd_data->need_restore = 1;
+		return 0;
+	}
+
 	mutex_unlock(&genpd->lock);
 
 	genpd_start_dev(genpd, dev);

@@ -373,7 +383,7 @@ static int __pm_genpd_save_device(struct pm_domain_data *pdd,
 	mutex_lock(&genpd->lock);
 
 	if (!ret)
-		gpd_data->need_restore = true;
+		gpd_data->need_restore = 1;
 
 	return ret;
 }

@@ -389,13 +399,17 @@ static void __pm_genpd_restore_device(struct pm_domain_data *pdd,
 {
 	struct generic_pm_domain_data *gpd_data = to_gpd_data(pdd);
 	struct device *dev = pdd->dev;
-	bool need_restore = gpd_data->need_restore;
+	int need_restore = gpd_data->need_restore;
 
-	gpd_data->need_restore = false;
+	gpd_data->need_restore = 0;
 	mutex_unlock(&genpd->lock);
 
 	genpd_start_dev(genpd, dev);
-	if (need_restore)
+	/*
+	 * Make sure to also restore those devices which we until now, didn't
+	 * know the state for.
+	 */
+	if (need_restore != 0)
 		genpd_restore_dev(genpd, dev);
 
 	mutex_lock(&genpd->lock);

@@ -603,6 +617,7 @@ static void genpd_power_off_work_fn(struct work_struct *work)
 static int pm_genpd_runtime_suspend(struct device *dev)
 {
 	struct generic_pm_domain *genpd;
+	struct generic_pm_domain_data *gpd_data;
 	bool (*stop_ok)(struct device *__dev);
 	int ret;

@@ -628,6 +643,15 @@ static int pm_genpd_runtime_suspend(struct device *dev)
 		return 0;
 
 	mutex_lock(&genpd->lock);
+	/*
+	 * If we have an unknown state of the need_restore flag, it means none
+	 * of the runtime PM callbacks has been invoked yet. Let's update the
+	 * flag to reflect that the current state is active.
+	 */
+	gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
+	if (gpd_data->need_restore == -1)
+		gpd_data->need_restore = 0;
+
 	genpd->in_progress++;
 	pm_genpd_poweroff(genpd);
 	genpd->in_progress--;

@@ -1442,7 +1466,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	mutex_lock(&gpd_data->lock);
 	gpd_data->base.dev = dev;
 	list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
-	gpd_data->need_restore = genpd->status == GPD_STATE_POWER_OFF;
+	gpd_data->need_restore = -1;
 	gpd_data->td.constraint_changed = true;
 	gpd_data->td.effective_constraint_ns = -1;
 	mutex_unlock(&gpd_data->lock);

@@ -1546,7 +1570,7 @@ void pm_genpd_dev_need_restore(struct device *dev, bool val)
 
 	psd = dev_to_psd(dev);
 	if (psd && psd->domain_data)
-		to_gpd_data(psd->domain_data)->need_restore = val;
+		to_gpd_data(psd->domain_data)->need_restore = val ? 1 : 0;
 
 	spin_unlock_irqrestore(&dev->power.lock, flags);
 }

include/linux/pm_domain.h +1 -1

index b3ed776..2e0e06d 100644
@@ -106,7 +106,7 @@ struct generic_pm_domain_data {
 	struct notifier_block nb;
 	struct mutex lock;
 	unsigned int refcount;
-	bool need_restore;
+	int need_restore;
 };
 
 #ifdef CONFIG_PM_GENERIC_DOMAINS

Signature · 1 line

1.9.1

Sylwester Nawrocki

2014-11-07 18:52 UTC #1 lore raw

PATCH 1/1 PM / Domains: Fix initial default state of the need_restore flag

On 07/11/14 14:27, Ulf Hansson wrote:
[...]

Quoted 51 lines

To handle things more properly in the generic PM domain, let's change the default initial value of the need_restore flag to reflect that the state is unknown. As soon as some of the runtime PM callbacks gets invoked, update the initial value accordingly.
Moreover, since the generic PM domain is verifying that all device's are both runtime PM enabled and suspended, using pm_runtime_suspended() while pm_genpd_poweroff() is invoked from the scheduled work, we can be sure of that the PM domain won't be powering off while having active devices.
Do note that, the generic PM domain can still only know about active devices which has been activated through invoking its runtime PM resume callback. In other words, buses/drivers using pm_runtime_set_active() during ->probe() will still suffer from a race condition, potentially probing a device without having its PM domain being powered. That issue will have to be solved using a different approach.
This a log from the boot regression for Exynos5, which is being fixed in this patch.
------------[ cut here ]------------
WARNING: CPU: 0 PID: 308 at ../drivers/clk/clk.c:851 clk_disable+0x24/0x30()
Modules linked in:
CPU: 0 PID: 308 Comm: kworker/0:1 Not tainted 3.18.0-rc3-00569-gbd9449f-dirty #10
Workqueue: pm pm_runtime_work
[<c0013c64>] (unwind_backtrace) from [<c0010dec>] (show_stack+0x10/0x14)
[<c0010dec>] (show_stack) from [<c03ee4cc>] (dump_stack+0x70/0xbc)
[<c03ee4cc>] (dump_stack) from [<c0020d34>] (warn_slowpath_common+0x64/0x88)
[<c0020d34>] (warn_slowpath_common) from [<c0020d74>] (warn_slowpath_null+0x1c/0x24)
[<c0020d74>] (warn_slowpath_null) from [<c03107b0>] (clk_disable+0x24/0x30)
[<c03107b0>] (clk_disable) from [<c02cc834>] (gsc_runtime_suspend+0x128/0x160)
[<c02cc834>] (gsc_runtime_suspend) from [<c0249024>] (pm_generic_runtime_suspend+0x2c/0x38)
[<c0249024>] (pm_generic_runtime_suspend) from [<c024f44c>] (pm_genpd_default_save_state+0x2c/0x8c)
[<c024f44c>] (pm_genpd_default_save_state) from [<c024ff2c>] (pm_genpd_poweroff+0x224/0x3ec)
[<c024ff2c>] (pm_genpd_poweroff) from [<c02501b4>] (pm_genpd_runtime_suspend+0x9c/0xcc)
[<c02501b4>] (pm_genpd_runtime_suspend) from [<c024a4f8>] (__rpm_callback+0x2c/0x60)
[<c024a4f8>] (__rpm_callback) from [<c024a54c>] (rpm_callback+0x20/0x74)
[<c024a54c>] (rpm_callback) from [<c024a930>] (rpm_suspend+0xd4/0x43c)
[<c024a930>] (rpm_suspend) from [<c024bbcc>] (pm_runtime_work+0x80/0x90)
[<c024bbcc>] (pm_runtime_work) from [<c0032a9c>] (process_one_work+0x12c/0x314)
[<c0032a9c>] (process_one_work) from [<c0032cf4>] (worker_thread+0x3c/0x4b0)
[<c0032cf4>] (worker_thread) from [<c003747c>] (kthread+0xcc/0xe8)
[<c003747c>] (kthread) from [<c000e738>] (ret_from_fork+0x14/0x3c)
---[ end trace 40cd58bcd6988f12 ]---
Fixes a4a8c2c4962bb655 (ARM: exynos: Move to generic PM domain DT bindings)
Reported-by Sylwester Nawrocki <[email protected]>
Signed-off-by Ulf Hansson <[email protected]>
---

Quoted 7 lines

This patch is intended as fix for 3.18 rc[n] due to the regression for Exynos SOCs.
I would also like to call for help in getting this thoroughly tested.
I have tested this on Arndale Dual, Exynos 5250. According the log attached in the commit message as well.

Thanks Ulf, I've tested this patch on top of v3.18-rc3 on Exynos4412 Trats2 board and it seems the unbalanced runtime_suspend() call issue is gone.

Quoted 4 lines

I have tested this on UX500, which support for the generic PM domain is about to be queued for 3.19. Since UX500 uses the AMBA bus/drivers, which uses pm_runtime_set_active() instead of pm_runtime_get_sync() during ->probe(), I could also verify that this behavior is maintained.

I had some issues with such configuration, after modifying the drivers to call pm_runtime_set_active() (instead of pm_runtime_get_sync()) in probe(). It looked like there is some access to the hardware with corresponding power domain disabled. Couldn't debug it in reasonable time due to system hanging, will try to get back to this on Wednesday.

Quoted 4 lines

Finally I have also tested this patchset on UX500 and using the below patchset to make sure the approach is suitable long term wise as well. [PATCH v3 0/9] PM / Domains: Fix race conditions during boot
http://www.spinics.net/lists/arm-kernel/msg369409.html

And with that additional patch series and the drivers modified everything seemed to work too.

The patch looks good to me as a fix for v3.18, if others don't report regressions feel free to add my:

Reviewed-by Sylwester Nawrocki <[email protected]>

(On Exynos4412 Trats2 board)

Tested-by Sylwester Nawrocki <[email protected]>

Signature · 2 lines

Regards,
Sylwester

Kevin Hilman

2014-11-07 19:47 UTC #2 lore raw

PATCH 1/1 PM / Domains: Fix initial default state of the need_restore flag

In reply to Ulf Hansson

Quoted 69 lines · Ulf Hansson

The initial state of the device's need_restore flag should'nt depend on the current state of the PM domain. For example it should be perfectly valid to attach an inactive device to a powered PM domain.
The pm_genpd_dev_need_restore() API allow us to update the need_restore flag to somewhat cope with such scenarios. Typically that should have been done from drivers/buses ->probe() since it's those that put the requirements on the value of the need_restore flag.
Until recently, the Exynos SOCs were the only user of the pm_genpd_dev_need_restore() API, though invoking it from a centralized location while adding devices to their PM domains.
Due to that Exynos now have swithed to the generic OF-based PM domain look-up, it's no longer possible to invoke the API from a centralized location. The reason is because devices are now added to their PM domains during the probe sequence.
Commit "ARM: exynos: Move to generic PM domain DT bindings" did the switch for Exynos to the generic OF-based PM domain look-up, but it also removed the call to pm_genpd_dev_need_restore(). This caused a regression for some of the Exynos drivers.
To handle things more properly in the generic PM domain, let's change the default initial value of the need_restore flag to reflect that the state is unknown. As soon as some of the runtime PM callbacks gets invoked, update the initial value accordingly.
Moreover, since the generic PM domain is verifying that all device's are both runtime PM enabled and suspended, using pm_runtime_suspended() while pm_genpd_poweroff() is invoked from the scheduled work, we can be sure of that the PM domain won't be powering off while having active devices.
Do note that, the generic PM domain can still only know about active devices which has been activated through invoking its runtime PM resume callback. In other words, buses/drivers using pm_runtime_set_active() during ->probe() will still suffer from a race condition, potentially probing a device without having its PM domain being powered. That issue will have to be solved using a different approach.
This a log from the boot regression for Exynos5, which is being fixed in this patch.
------------[ cut here ]------------
WARNING: CPU: 0 PID: 308 at ../drivers/clk/clk.c:851 clk_disable+0x24/0x30()
Modules linked in:
CPU: 0 PID: 308 Comm: kworker/0:1 Not tainted 3.18.0-rc3-00569-gbd9449f-dirty #10
Workqueue: pm pm_runtime_work
[<c0013c64>] (unwind_backtrace) from [<c0010dec>] (show_stack+0x10/0x14)
[<c0010dec>] (show_stack) from [<c03ee4cc>] (dump_stack+0x70/0xbc)
[<c03ee4cc>] (dump_stack) from [<c0020d34>] (warn_slowpath_common+0x64/0x88)
[<c0020d34>] (warn_slowpath_common) from [<c0020d74>] (warn_slowpath_null+0x1c/0x24)
[<c0020d74>] (warn_slowpath_null) from [<c03107b0>] (clk_disable+0x24/0x30)
[<c03107b0>] (clk_disable) from [<c02cc834>] (gsc_runtime_suspend+0x128/0x160)
[<c02cc834>] (gsc_runtime_suspend) from [<c0249024>] (pm_generic_runtime_suspend+0x2c/0x38)
[<c0249024>] (pm_generic_runtime_suspend) from [<c024f44c>] (pm_genpd_default_save_state+0x2c/0x8c)
[<c024f44c>] (pm_genpd_default_save_state) from [<c024ff2c>] (pm_genpd_poweroff+0x224/0x3ec)
[<c024ff2c>] (pm_genpd_poweroff) from [<c02501b4>] (pm_genpd_runtime_suspend+0x9c/0xcc)
[<c02501b4>] (pm_genpd_runtime_suspend) from [<c024a4f8>] (__rpm_callback+0x2c/0x60)
[<c024a4f8>] (__rpm_callback) from [<c024a54c>] (rpm_callback+0x20/0x74)
[<c024a54c>] (rpm_callback) from [<c024a930>] (rpm_suspend+0xd4/0x43c)
[<c024a930>] (rpm_suspend) from [<c024bbcc>] (pm_runtime_work+0x80/0x90)
[<c024bbcc>] (pm_runtime_work) from [<c0032a9c>] (process_one_work+0x12c/0x314)
[<c0032a9c>] (process_one_work) from [<c0032cf4>] (worker_thread+0x3c/0x4b0)
[<c0032cf4>] (worker_thread) from [<c003747c>] (kthread+0xcc/0xe8)
[<c003747c>] (kthread) from [<c000e738>] (ret_from_fork+0x14/0x3c)
---[ end trace 40cd58bcd6988f12 ]---

Quoted 3 lines

Fixes a4a8c2c4962bb655 (ARM: exynos: Move to generic PM domain DT bindings)
Reported-by Sylwester Nawrocki <[email protected]>
Signed-off-by Ulf Hansson <[email protected]>

Reviewed-by Kevin Hilman <[email protected]>

And looks like we need this for v3.18-rc since it does fix the regression.

A minor nit: you add a few new multi-line comment blocks which should have a new empty line before them, but currently they're right up against the previous code. Please add a blank line for separation and to be consisitent with the rest of the file.

Thanks,

Kevin

Dmitry Torokhov

2014-11-07 21:57 UTC #3 lore raw

PATCH 1/1 PM / Domains: Fix initial default state of the need_restore flag

Quoted 71 lines · Kevin Hilman

Quoted 69 lines · Ulf Hansson
The initial state of the device's need_restore flag should'nt depend on the current state of the PM domain. For example it should be perfectly valid to attach an inactive device to a powered PM domain.
The pm_genpd_dev_need_restore() API allow us to update the need_restore flag to somewhat cope with such scenarios. Typically that should have been done from drivers/buses ->probe() since it's those that put the requirements on the value of the need_restore flag.
Until recently, the Exynos SOCs were the only user of the pm_genpd_dev_need_restore() API, though invoking it from a centralized location while adding devices to their PM domains.
Due to that Exynos now have swithed to the generic OF-based PM domain look-up, it's no longer possible to invoke the API from a centralized location. The reason is because devices are now added to their PM domains during the probe sequence.
Commit "ARM: exynos: Move to generic PM domain DT bindings" did the switch for Exynos to the generic OF-based PM domain look-up, but it also removed the call to pm_genpd_dev_need_restore(). This caused a regression for some of the Exynos drivers.
To handle things more properly in the generic PM domain, let's change the default initial value of the need_restore flag to reflect that the state is unknown. As soon as some of the runtime PM callbacks gets invoked, update the initial value accordingly.
Moreover, since the generic PM domain is verifying that all device's are both runtime PM enabled and suspended, using pm_runtime_suspended() while pm_genpd_poweroff() is invoked from the scheduled work, we can be sure of that the PM domain won't be powering off while having active devices.
Do note that, the generic PM domain can still only know about active devices which has been activated through invoking its runtime PM resume callback. In other words, buses/drivers using pm_runtime_set_active() during ->probe() will still suffer from a race condition, potentially probing a device without having its PM domain being powered. That issue will have to be solved using a different approach.
This a log from the boot regression for Exynos5, which is being fixed in this patch.
------------[ cut here ]------------
WARNING: CPU: 0 PID: 308 at ../drivers/clk/clk.c:851 clk_disable+0x24/0x30()
Modules linked in:
CPU: 0 PID: 308 Comm: kworker/0:1 Not tainted 3.18.0-rc3-00569-gbd9449f-dirty #10
Workqueue: pm pm_runtime_work
[<c0013c64>] (unwind_backtrace) from [<c0010dec>] (show_stack+0x10/0x14)
[<c0010dec>] (show_stack) from [<c03ee4cc>] (dump_stack+0x70/0xbc)
[<c03ee4cc>] (dump_stack) from [<c0020d34>] (warn_slowpath_common+0x64/0x88)
[<c0020d34>] (warn_slowpath_common) from [<c0020d74>] (warn_slowpath_null+0x1c/0x24)
[<c0020d74>] (warn_slowpath_null) from [<c03107b0>] (clk_disable+0x24/0x30)
[<c03107b0>] (clk_disable) from [<c02cc834>] (gsc_runtime_suspend+0x128/0x160)
[<c02cc834>] (gsc_runtime_suspend) from [<c0249024>] (pm_generic_runtime_suspend+0x2c/0x38)
[<c0249024>] (pm_generic_runtime_suspend) from [<c024f44c>] (pm_genpd_default_save_state+0x2c/0x8c)
[<c024f44c>] (pm_genpd_default_save_state) from [<c024ff2c>] (pm_genpd_poweroff+0x224/0x3ec)
[<c024ff2c>] (pm_genpd_poweroff) from [<c02501b4>] (pm_genpd_runtime_suspend+0x9c/0xcc)
[<c02501b4>] (pm_genpd_runtime_suspend) from [<c024a4f8>] (__rpm_callback+0x2c/0x60)
[<c024a4f8>] (__rpm_callback) from [<c024a54c>] (rpm_callback+0x20/0x74)
[<c024a54c>] (rpm_callback) from [<c024a930>] (rpm_suspend+0xd4/0x43c)
[<c024a930>] (rpm_suspend) from [<c024bbcc>] (pm_runtime_work+0x80/0x90)
[<c024bbcc>] (pm_runtime_work) from [<c0032a9c>] (process_one_work+0x12c/0x314)
[<c0032a9c>] (process_one_work) from [<c0032cf4>] (worker_thread+0x3c/0x4b0)
[<c0032cf4>] (worker_thread) from [<c003747c>] (kthread+0xcc/0xe8)
[<c003747c>] (kthread) from [<c000e738>] (ret_from_fork+0x14/0x3c)
---[ end trace 40cd58bcd6988f12 ]---

Quoted 8 lines

Quoted 3 lines
Fixes a4a8c2c4962bb655 (ARM: exynos: Move to generic PM domain DT bindings)
Reported-by Sylwester Nawrocki <[email protected]>
Signed-off-by Ulf Hansson <[email protected]>
Reviewed-by Kevin Hilman <[email protected]>
And looks like we need this for v3.18-rc since it does fix the regression.

I guess we do need it for 3.18, but when we are talking about 3.19, before we make any more changes can we outline how power domains are supposed to work?

1. How do we attach a device to power domain? Right now it seems that individual buses are responsible for attaching their devices to power domains. Should we move it into driver core (device_pm_add?) so that device starts its life in its proper power domain?

2. When do we power up the devices (and the domains)? Right now devices in ACPI power domain are powered when they are attached to the power domain (which coincides with probing), but generic power domains do not do that. Can we add a separate API to explicitly power up the device (and its domain if it is powered down) and do it again, either in device core or in individual buses. This way, regardless of runtime PM or not, we will have devices powered on when driver tries to bind to them. If binding fails we can power down the device.

I've heard there is a concern about power domain bouncing up and down during probing. Is it really a concern? Would not we run into the same issues past booting up with aggressive PM policy? Anyway, we could probably work around this by refusing to actually power down the domains until we are done booting, similarly to who genpd_poweroff_unused works.

Thanks.

Signature · 1 line

Dmitry

Kevin Hilman

2014-11-07 22:26 UTC #4 lore raw

PATCH 1/1 PM / Domains: Fix initial default state of the need_restore flag

Quoted 76 lines · Dmitry Torokhov

Quoted 75 lines · Kevin Hilman
Quoted 72 lines · Ulf Hansson
The initial state of the device's need_restore flag should'nt depend on the current state of the PM domain. For example it should be perfectly valid to attach an inactive device to a powered PM domain.
The pm_genpd_dev_need_restore() API allow us to update the need_restore flag to somewhat cope with such scenarios. Typically that should have been done from drivers/buses ->probe() since it's those that put the requirements on the value of the need_restore flag.
Until recently, the Exynos SOCs were the only user of the pm_genpd_dev_need_restore() API, though invoking it from a centralized location while adding devices to their PM domains.
Due to that Exynos now have swithed to the generic OF-based PM domain look-up, it's no longer possible to invoke the API from a centralized location. The reason is because devices are now added to their PM domains during the probe sequence.
Commit "ARM: exynos: Move to generic PM domain DT bindings" did the switch for Exynos to the generic OF-based PM domain look-up, but it also removed the call to pm_genpd_dev_need_restore(). This caused a regression for some of the Exynos drivers.
To handle things more properly in the generic PM domain, let's change the default initial value of the need_restore flag to reflect that the state is unknown. As soon as some of the runtime PM callbacks gets invoked, update the initial value accordingly.
Moreover, since the generic PM domain is verifying that all device's are both runtime PM enabled and suspended, using pm_runtime_suspended() while pm_genpd_poweroff() is invoked from the scheduled work, we can be sure of that the PM domain won't be powering off while having active devices.
Do note that, the generic PM domain can still only know about active devices which has been activated through invoking its runtime PM resume callback. In other words, buses/drivers using pm_runtime_set_active() during ->probe() will still suffer from a race condition, potentially probing a device without having its PM domain being powered. That issue will have to be solved using a different approach.
This a log from the boot regression for Exynos5, which is being fixed in this patch.
------------[ cut here ]------------
WARNING: CPU: 0 PID: 308 at ../drivers/clk/clk.c:851 clk_disable+0x24/0x30()
Modules linked in:
CPU: 0 PID: 308 Comm: kworker/0:1 Not tainted 3.18.0-rc3-00569-gbd9449f-dirty #10
Workqueue: pm pm_runtime_work
[<c0013c64>] (unwind_backtrace) from [<c0010dec>] (show_stack+0x10/0x14)
[<c0010dec>] (show_stack) from [<c03ee4cc>] (dump_stack+0x70/0xbc)
[<c03ee4cc>] (dump_stack) from [<c0020d34>] (warn_slowpath_common+0x64/0x88)
[<c0020d34>] (warn_slowpath_common) from [<c0020d74>] (warn_slowpath_null+0x1c/0x24)
[<c0020d74>] (warn_slowpath_null) from [<c03107b0>] (clk_disable+0x24/0x30)
[<c03107b0>] (clk_disable) from [<c02cc834>] (gsc_runtime_suspend+0x128/0x160)
[<c02cc834>] (gsc_runtime_suspend) from [<c0249024>] (pm_generic_runtime_suspend+0x2c/0x38)
[<c0249024>] (pm_generic_runtime_suspend) from [<c024f44c>] (pm_genpd_default_save_state+0x2c/0x8c)
[<c024f44c>] (pm_genpd_default_save_state) from [<c024ff2c>] (pm_genpd_poweroff+0x224/0x3ec)
[<c024ff2c>] (pm_genpd_poweroff) from [<c02501b4>] (pm_genpd_runtime_suspend+0x9c/0xcc)
[<c02501b4>] (pm_genpd_runtime_suspend) from [<c024a4f8>] (__rpm_callback+0x2c/0x60)
[<c024a4f8>] (__rpm_callback) from [<c024a54c>] (rpm_callback+0x20/0x74)
[<c024a54c>] (rpm_callback) from [<c024a930>] (rpm_suspend+0xd4/0x43c)
[<c024a930>] (rpm_suspend) from [<c024bbcc>] (pm_runtime_work+0x80/0x90)
[<c024bbcc>] (pm_runtime_work) from [<c0032a9c>] (process_one_work+0x12c/0x314)
[<c0032a9c>] (process_one_work) from [<c0032cf4>] (worker_thread+0x3c/0x4b0)
[<c0032cf4>] (worker_thread) from [<c003747c>] (kthread+0xcc/0xe8)
[<c003747c>] (kthread) from [<c000e738>] (ret_from_fork+0x14/0x3c)
---[ end trace 40cd58bcd6988f12 ]---
Fixes a4a8c2c4962bb655 (ARM: exynos: Move to generic PM domain DT bindings)
Reported-by Sylwester Nawrocki <[email protected]>
Signed-off-by Ulf Hansson <[email protected]>

Quoted 8 lines

Quoted 4 lines
Reviewed-by Kevin Hilman <[email protected]>
And looks like we need this for v3.18-rc since it does fix the regression.
I guess we do need it for 3.18, but when we are talking about 3.19, before we make any more changes can we outline how power domains are supposed to work?

Yes, I agree, we have some things to sort out for v3.19, but as this one is a regression fix, I think it needs to be in v3.18-rc.

Kevin

Quoted 21 lines

1. How do we attach a device to power domain? Right now it seems that individual buses are responsible for attaching their devices to power domains. Should we move it into driver core (device_pm_add?) so that device starts its life in its proper power domain?
2. When do we power up the devices (and the domains)? Right now devices in ACPI power domain are powered when they are attached to the power domain (which coincides with probing), but generic power domains do not do that. Can we add a separate API to explicitly power up the device (and its domain if it is powered down) and do it again, either in device core or in individual buses. This way, regardless of runtime PM or not, we will have devices powered on when driver tries to bind to them. If binding fails we can power down the device.
I've heard there is a concern about power domain bouncing up and down during probing. Is it really a concern? Would not we run into the same issues past booting up with aggressive PM policy? Anyway, we could probably work around this by refusing to actually power down the domains until we are done booting, similarly to who genpd_poweroff_unused works.
Thanks.

Ulf Hansson

author

2014-11-10 15:18 UTC #5 lore raw

PATCH 1/1 PM / Domains: Fix initial default state of the need_restore flag

In reply to Dmitry Torokhov

[...]

Quoted 8 lines

I guess we do need it for 3.18, but when we are talking about 3.19, before we make any more changes can we outline how power domains are supposed to work?
1. How do we attach a device to power domain? Right now it seems that individual buses are responsible for attaching their devices to power domains. Should we move it into driver core (device_pm_add?) so that device starts its life in its proper power domain?

That was the initial approach.

To my understanding, Rafael's primary reason for not accepting that was that it's not common, but it's platform-specific.
http://marc.info/?l=linux-pm&m=140243462304669&w=2

Now, even if we would reconsider doing as you propose, what would the actual benefit be? Obviously, we would get less amount of code to maintain, which is one reason, but are there more?

Quoted 9 lines

2. When do we power up the devices (and the domains)? Right now devices in ACPI power domain are powered when they are attached to the power domain (which coincides with probing), but generic power domains do not do that. Can we add a separate API to explicitly power up the device (and its domain if it is powered down) and do it again, either in device core or in individual buses. This way, regardless of runtime PM or not, we will have devices powered on when driver tries to bind to them. If binding fails we can power down the device.

Isn't that exactly what I implemented in [1], what am I missing?

Quoted 6 lines

I've heard there is a concern about power domain bouncing up and down during probing. Is it really a concern? Would not we run into the same issues past booting up with aggressive PM policy? Anyway, we could probably work around this by refusing to actually power down the domains until we are done booting, similarly to who genpd_poweroff_unused works.

I haven't heard about this kind of issues, but I think your approach seems reasonable.

Currently, I think we should focus on solving the problem in [1]. Those drivers/subsystems that use pm_runtime_set_active() during ->probe() are broken in this regards.

Now, if we move the attachment of devices to their PM domains from buses into driver core, that could simplify [1]. Still, only if we also decide to power on PM domains from driver core, else it wouldn't matter much I think.

[1]
[PATCH v3 0/9] PM / Domains: Fix race conditions during boot
http://marc.info/?t=141320907000003&r=1&w=2

Kind regards
Uffe

Dmitry Torokhov

2014-11-10 18:32 UTC #6 lore raw

PATCH 1/1 PM / Domains: Fix initial default state of the need_restore flag

Quoted 11 lines · Ulf Hansson

[...]
Quoted 8 lines
I guess we do need it for 3.18, but when we are talking about 3.19, before we make any more changes can we outline how power domains are supposed to work?
1. How do we attach a device to power domain? Right now it seems that individual buses are responsible for attaching their devices to power domains. Should we move it into driver core (device_pm_add?) so that device starts its life in its proper power domain?

Quoted 5 lines

That was the initial approach.
To my understanding, Rafael's primary reason for not accepting that was that it's not common, but it's platform-specific.
http://marc.info/?l=linux-pm&m=140243462304669&w=2

I am not sure I agree with Rafael there:

- when we are talking about the latest incarnation of power domains it is not really platform specific anymore (as it was when we were dealing with ACPI only case);

- I do not see why sirincling platform specific code around i2c, spi, etc bus code (which is not platform-specific) is OK, but a no-no for the driver ocre.

Quoted 4 lines

Now, even if we would reconsider doing as you propose, what would the actual benefit be? Obviously, we would get less amount of code to maintain, which is one reason, but are there more?

I think so - you would have complete picture of your power domain, including data exposed in debugfs, etc.

Quoted 11 lines

Quoted 10 lines
2. When do we power up the devices (and the domains)? Right now devices in ACPI power domain are powered when they are attached to the power domain (which coincides with probing), but generic power domains do not do that. Can we add a separate API to explicitly power up the device (and its domain if it is powered down) and do it again, either in device core or in individual buses. This way, regardless of runtime PM or not, we will have devices powered on when driver tries to bind to them. If binding fails we can power down the device.

Quoted 1 line

Isn't that exactly what I implemented in [1], what am I missing?

Hm, OK. I guess the title of the patch series thrown me off because as far as I am concerned it is not a race, we simply were not powering the domain properly for probing.

Thanks.

Signature · 1 line

Dmitry

Mark Brown

2014-11-10 19:39 UTC #7 lore raw

PATCH 1/1 PM / Domains: Fix initial default state of the need_restore flag

Quoted 3 lines · Dmitry Torokhov

- I do not see why sirincling platform specific code around i2c, spi, etc bus code (which is not platform-specific) is OK, but a no-no for the driver ocre.

sirincling? In the case of I2C and SPI the buses don't define any sort
of power management or power domain so any management is going to be
device or system specific.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141110/a6b1c0cc/attachment.sig>

Dmitry Torokhov

2014-11-10 20:33 UTC #8 lore raw

PATCH 1/1 PM / Domains: Fix initial default state of the need_restore flag

Quoted 8 lines · Mark Brown

Quoted 2 lines · Dmitry Torokhov
- I do not see why sirincling platform specific code around i2c, spi, etc bus code (which is not platform-specific) is OK, but a no-no
for
Quoted 1 line
the driver ocre.
sirincling?

Sorry, I was going for "sprinkling" but failed spectacularly.

Thanks.

Signature · 1 line

Dmitry

Rafael J. Wysocki

2014-11-13 02:31 UTC #9 lore raw

In reply to Ulf Hansson

[CC list trimmed]

Quoted 11 lines · Ulf Hansson

[...]
Quoted 8 lines
I guess we do need it for 3.18, but when we are talking about 3.19, before we make any more changes can we outline how power domains are supposed to work?
1. How do we attach a device to power domain? Right now it seems that individual buses are responsible for attaching their devices to power domains. Should we move it into driver core (device_pm_add?) so that device starts its life in its proper power domain?

Quoted 5 lines

That was the initial approach.
To my understanding, Rafael's primary reason for not accepting that was that it's not common, but it's platform-specific.
http://marc.info/?l=linux-pm&m=140243462304669&w=2

For the record, I still believe this is platform-specific.

I also think that the knowledge about what power (or generally PM) domain a device should belong to is not in a bus type or in the driver core. That knowledge is in the code that enumerates devices.

I wonder, then, if we could set the PM domain pointer at about the time when we set the bus type pointer? Things will be consistent all the way through the entire device life cycle then.

Quoted 3 lines

Now, even if we would reconsider doing as you propose, what would the actual benefit be? Obviously, we would get less amount of code to maintain, which is one reason, but are there more?

The same set of subsystem-level PM callbacks will be present for the device throughout its life cycle.

Quoted 10 lines

Quoted 8 lines
2. When do we power up the devices (and the domains)? Right now devices in ACPI power domain are powered when they are attached to the power domain (which coincides with probing), but generic power domains do not do that. Can we add a separate API to explicitly power up the device (and its domain if it is powered down) and do it again, either in device core or in individual buses. This way, regardless of runtime PM or not, we will have devices powered on when driver tries to bind to them. If binding fails we can power down the device.
Isn't that exactly what I implemented in [1], what am I missing?

Not really. Dmitry is talking about a generic interface independent of PM domains.

If we have pm_power_up(dev)/pm_power_down(dev), then the PM core could use it around really_probe() for all devices. In theory. But that's what we started with when we were working on the runtime PM framework and we ended up with what we have today.

Problem is, pm_power_up() would probably end up being pretty much the same as pm_runtime_resume() without executing driver callbacks and similarly for pm_power_down(). That's why I was thinking about running pm_runtime_resume() at the time we know that driver callbacks are not present, just for the purpose of powering up the device. [That has a problem of working with CONFIG_PM_RUNTIME unset, but let me set this one aside for a while.]

Now, Grygorii seems to be claiming that some drivers *require* their .runtime_resume() callbacks to be executed during .probe() pretty much before anything else which won't happen if pm_runtime_resume() is done before really_probe(). I'm wondering, then, which drivers those are.

Essentially, the "power up" operation will depend on the PM domain and bus type, so they'll need to provide callbacks that will most likely duplicate runtime PM callbacks. And those callbacks need to be available when we do the "power up" thing.

How can we address that?

We seem to be dealing with a case in which PM is needed on some systems just to make them work at the basic level and not for energy saving, which was what runtime PM was designed for.

Rafael

Ulf Hansson

author

2014-11-13 16:40 UTC #10 lore raw

[...]

Quoted 7 lines

Quoted 4 lines
To my understanding, Rafael's primary reason for not accepting that was that it's not common, but it's platform-specific.
http://marc.info/?l=linux-pm&m=140243462304669&w=2
For the record, I still believe this is platform-specific.

Quoted 7 lines

I also think that the knowledge about what power (or generally PM) domain a device should belong to is not in a bus type or in the driver core. That knowledge is in the code that enumerates devices.
I wonder, then, if we could set the PM domain pointer at about the time when we set the bus type pointer? Things will be consistent all the way through the entire device life cycle then.

Could you maybe give some examples of where such code should be invoked from then?

I do see some difficulties in your suggestion, primarily since we would need all PM domains to be initialized prior "device enumeration". That in combination with doing PM domain initialization from SOC specific code, could be a bit tricky to sort out.

Quoted 7 lines

Quoted 4 lines
Now, even if we would reconsider doing as you propose, what would the actual benefit be? Obviously, we would get less amount of code to maintain, which is one reason, but are there more?
The same set of subsystem-level PM callbacks will be present for the device throughout its life cycle.

I also do like the consistency this would bring us.

Quoted 20 lines

Quoted 11 lines
Quoted 9 lines
2. When do we power up the devices (and the domains)? Right now devices in ACPI power domain are powered when they are attached to the power domain (which coincides with probing), but generic power domains do not do that. Can we add a separate API to explicitly power up the device (and its domain if it is powered down) and do it again, either in device core or in individual buses. This way, regardless of runtime PM or not, we will have devices powered on when driver tries to bind to them. If binding fails we can power down the device.
Isn't that exactly what I implemented in [1], what am I missing?
Not really. Dmitry is talking about a generic interface independent of PM domains.
If we have pm_power_up(dev)/pm_power_down(dev), then the PM core could use it around really_probe() for all devices. In theory. But that's what we started with when we were working on the runtime PM framework and we ended up with what we have today.

Quoted 6 lines

Problem is, pm_power_up() would probably end up being pretty much the same as pm_runtime_resume() without executing driver callbacks and similarly for pm_power_down(). That's why I was thinking about running pm_runtime_resume() at the time we know that driver callbacks are not present, just for the purpose of powering up the device. [That has a problem of working with CONFIG_PM_RUNTIME unset, but let me set this one aside for a while.]

I take this as we are aligned on using the runtime PM API from the driver core isn't a solution to the problem, right?

I think Alan in the other thread [1] also had some valuable points to why we shouldn't use runtime PM from the driver core to power on PM domains.

To that I would also like to add, that I really don't think the driver core should enable runtime PM, that's up to each an every subsystem/driver to decide when/if to do. Using pm_runtime_resume() or any of the pm_runtime_get*() API from the driver core would have required that as well.

Quoted 5 lines

Now, Grygorii seems to be claiming that some drivers *require* their .runtime_resume() callbacks to be executed during .probe() pretty much before anything else which won't happen if pm_runtime_resume() is done before really_probe(). I'm wondering, then, which drivers those are.

I have looked around and there are certainly hole bunch of such drivers.

I guess the most important reason to why these drivers behave as stated, is because that's been the common solution to make sure the PM domain stays powered during probe. It's unfortunate that no one cared about this until now.

Still, I am optimistic. :-) We should be able to cope with these drivers as long as we don't use runtime PM to power on the PM domain during probe.

Quoted 5 lines

Essentially, the "power up" operation will depend on the PM domain and bus type, so they'll need to provide callbacks that will most likely duplicate runtime PM callbacks. And those callbacks need to be available when we do the "power up" thing.

In principle what you suggest me to do is this, right?

1. Re-design how the device gets attached to its PM domain. That will also change the ACPI PM domain, which I have limited knowledge about. :-) In principle we want the PM domain pointer to be assigned to the device at device enumeration. This is needed since you think we should do the "power up" thing from the driver core.

2. Add "pm_power_up|down()" APIs with corresponding ->"power up|down"() callbacks in the struct dev_pm_domain. Similar as done in patch "[PATCH v3 1/9] PM / Domains: Add dev_pm_domain_get|put() APIs" [2]. Since the callbacks will be unique per PM domain those will be assigned at PM domain initialization.

3. Invoke the new APIs, pm_power_up|down() around really_probe() in driver core.

So to summarize, without the requirement of doing "pm_power_up|down()" from driver core, and instead leave that to be done by the buses, we would be back to the solution I proposed in "[PATCH v3 0/9] PM /
Domains: Fix race conditions during boot" [3].

Quoted 9 lines

How can we address that?
We seem to be dealing with a case in which PM is needed on some systems just to make them work at the basic level and not for energy saving, which was what runtime PM was designed for.
Rafael

Kind regards
Uffe

[1]
http://marc.info/?l=linux-pm&m=141511953314249&w=2
[2]
http://marc.info/?l=linux-pm&m=141320895322708&w=2
[3]
http://marc.info/?t=141320907000003&r=1&w=2

Grygorii Strashko

2014-11-13 19:15 UTC #11 lore raw

Quoted 81 lines · Ulf Hansson

[...]
Quoted 7 lines
Quoted 4 lines
To my understanding, Rafael's primary reason for not accepting that was that it's not common, but it's platform-specific.
http://marc.info/?l=linux-pm&m=140243462304669&w=2
For the record, I still believe this is platform-specific.
Quoted 7 lines
I also think that the knowledge about what power (or generally PM) domain a device should belong to is not in a bus type or in the driver core. That knowledge is in the code that enumerates devices.
I wonder, then, if we could set the PM domain pointer at about the time when we set the bus type pointer? Things will be consistent all the way through the entire device life cycle then.
Could you maybe give some examples of where such code should be invoked from then?
I do see some difficulties in your suggestion, primarily since we would need all PM domains to be initialized prior "device enumeration". That in combination with doing PM domain initialization from SOC specific code, could be a bit tricky to sort out.
Quoted 7 lines
Quoted 4 lines
Now, even if we would reconsider doing as you propose, what would the actual benefit be? Obviously, we would get less amount of code to maintain, which is one reason, but are there more?
The same set of subsystem-level PM callbacks will be present for the device throughout its life cycle.
I also do like the consistency this would bring us.
Quoted 20 lines
Quoted 11 lines
Quoted 9 lines
2. When do we power up the devices (and the domains)? Right now devices in ACPI power domain are powered when they are attached to the power domain (which coincides with probing), but generic power domains do not do that. Can we add a separate API to explicitly power up the device (and its domain if it is powered down) and do it again, either in device core or in individual buses. This way, regardless of runtime PM or not, we will have devices powered on when driver tries to bind to them. If binding fails we can power down the device.
Isn't that exactly what I implemented in [1], what am I missing?
Not really. Dmitry is talking about a generic interface independent of PM domains.
If we have pm_power_up(dev)/pm_power_down(dev), then the PM core could use it around really_probe() for all devices. In theory. But that's what we started with when we were working on the runtime PM framework and we ended up with what we have today.
Quoted 6 lines
Problem is, pm_power_up() would probably end up being pretty much the same as pm_runtime_resume() without executing driver callbacks and similarly for pm_power_down(). That's why I was thinking about running pm_runtime_resume() at the time we know that driver callbacks are not present, just for the purpose of powering up the device. [That has a problem of working with CONFIG_PM_RUNTIME unset, but let me set this one aside for a while.]
I take this as we are aligned on using the runtime PM API from the driver core isn't a solution to the problem, right?
I think Alan in the other thread [1] also had some valuable points to why we shouldn't use runtime PM from the driver core to power on PM domains.
To that I would also like to add, that I really don't think the driver core should enable runtime PM, that's up to each an every subsystem/driver to decide when/if to do. Using pm_runtime_resume() or any of the pm_runtime_get*() API from the driver core would have required that as well.
Quoted 5 lines
Now, Grygorii seems to be claiming that some drivers *require* their .runtime_resume() callbacks to be executed during .probe() pretty much before anything else which won't happen if pm_runtime_resume() is done before really_probe(). I'm wondering, then, which drivers those are.

Quoted 6 lines

I have looked around and there are certainly hole bunch of such drivers.
I guess the most important reason to why these drivers behave as stated, is because that's been the common solution to make sure the PM domain stays powered during probe. It's unfortunate that no one cared about this until now.

PM domain is smth. relatively new :) Initial intention is to execute Bus's PM callbacks and wake up parent devices.

regards,
-grygorii

Ulf Hansson

author

2014-11-13 21:59 UTC #12 lore raw

In reply to Ulf Hansson

[...]

Quoted 13 lines

Quoted 4 lines
Essentially, the "power up" operation will depend on the PM domain and bus type, so they'll need to provide callbacks that will most likely duplicate runtime PM callbacks. And those callbacks need to be available when we do the "power up" thing.
In principle what you suggest me to do is this, right?
1. Re-design how the device gets attached to its PM domain. That will also change the ACPI PM domain, which I have limited knowledge about. :-) In principle we want the PM domain pointer to be assigned to the device at device enumeration. This is needed since you think we should do the "power up" thing from the driver core.

Quoted 8 lines

2. Add "pm_power_up|down()" APIs with corresponding ->"power up|down"() callbacks in the struct dev_pm_domain. Similar as done in patch "[PATCH v3 1/9] PM / Domains: Add dev_pm_domain_get|put() APIs" [2]. Since the callbacks will be unique per PM domain those will be assigned at PM domain initialization.
3. Invoke the new APIs, pm_power_up|down() around really_probe() in driver core.

Answering my own email, I guess am starting to be paranoid. :-)

I assume that the above solution is close to what you had in mind. If not, please just ignore this response.

As I stated earlier, the 1) could be a bit tricky to sort out. Especially since there are SOC specific patchsets that's been queued for 3.19 already which adds genpd support. I am not saying that we can't do it, but I am bit concerned that we wont solve the _real_ problem before the issues really starts to show up for several SOCs.

Therefore, I wonder if you could consider the following somewhat intermediate step, instead of going directly towards 1):

- Move the calls to dev_pm_domain_attach/detach() from the various buses ->probe(), into the driver core (really_probe()). In that way, we will be able to power up the PM domain from the driver core, since the device's PM domain pointer would be assigned.

I do realize that you rejected this approach earlier, but I guess we have some new "data" to consider!? :-)

Kind regards
Uffe

Grygorii Strashko

2014-11-13 17:51 UTC #13 lore raw

In reply to Rafael J. Wysocki

Quoted 63 lines · Rafael J. Wysocki

[CC list trimmed]
Quoted 11 lines · Ulf Hansson
[...]
Quoted 8 lines
I guess we do need it for 3.18, but when we are talking about 3.19, before we make any more changes can we outline how power domains are supposed to work?
1. How do we attach a device to power domain? Right now it seems that individual buses are responsible for attaching their devices to power domains. Should we move it into driver core (device_pm_add?) so that device starts its life in its proper power domain?
Quoted 5 lines
That was the initial approach.
To my understanding, Rafael's primary reason for not accepting that was that it's not common, but it's platform-specific.
http://marc.info/?l=linux-pm&m=140243462304669&w=2
For the record, I still believe this is platform-specific.
I also think that the knowledge about what power (or generally PM) domain a device should belong to is not in a bus type or in the driver core. That knowledge is in the code that enumerates devices.
I wonder, then, if we could set the PM domain pointer at about the time when we set the bus type pointer? Things will be consistent all the way through the entire device life cycle then.
Quoted 3 lines
Now, even if we would reconsider doing as you propose, what would the actual benefit be? Obviously, we would get less amount of code to maintain, which is one reason, but are there more?
The same set of subsystem-level PM callbacks will be present for the device throughout its life cycle.
Quoted 10 lines
Quoted 8 lines
2. When do we power up the devices (and the domains)? Right now devices in ACPI power domain are powered when they are attached to the power domain (which coincides with probing), but generic power domains do not do that. Can we add a separate API to explicitly power up the device (and its domain if it is powered down) and do it again, either in device core or in individual buses. This way, regardless of runtime PM or not, we will have devices powered on when driver tries to bind to them. If binding fails we can power down the device.
Isn't that exactly what I implemented in [1], what am I missing?
Not really. Dmitry is talking about a generic interface independent of PM domains.
If we have pm_power_up(dev)/pm_power_down(dev), then the PM core could use it around really_probe() for all devices. In theory. But that's what we started with when we were working on the runtime PM framework and we ended up with what we have today.
Problem is, pm_power_up() would probably end up being pretty much the same as pm_runtime_resume() without executing driver callbacks and similarly for pm_power_down(). That's why I was thinking about running pm_runtime_resume() at the time we know that driver callbacks are not present, just for the purpose of powering up the device. [That has a problem of working with CONFIG_PM_RUNTIME unset, but let me set this one aside for a while.]

Quoted 4 lines

Now, Grygorii seems to be claiming that some drivers *require* their .runtime_resume() callbacks to be executed during .probe() pretty much before anything else which won't happen if pm_runtime_resume() is done before really_probe(). I'm wondering, then, which drivers those are.

I've checked few folders and below few drivers i found which rely or may rely on their .runtime_resume callback to be executed (It very hard to identify dependency for some of drivers):
drivers/mfd/omap-usb-host.c
arizona-core.c

gpio/gpio-omap.c

video/fbdev/s3c-fb.c sh_mobile_lcdcfb.c

omap2/dss/dss.c dsi.c dispc.c rfbi.c venc.c

Quoted 6 lines

Essentially, the "power up" operation will depend on the PM domain and bus type, so they'll need to provide callbacks that will most likely duplicate runtime PM callbacks. And those callbacks need to be available when we do the "power up" thing.

Quoted 6 lines

How can we address that?
We seem to be dealing with a case in which PM is needed on some systems just to make them work at the basic level and not for energy saving, which was what runtime PM was designed for.

Regards,
-grygorii

Mark Brown

2014-11-13 17:56 UTC #14 lore raw

Quoted 4 lines · Grygorii Strashko

I've checked few folders and below few drivers i found which rely or may rely on their .runtime_resume callback to be executed (It very hard to identify dependency for some of drivers):
arizona-core.c

No, it doesn't - probe() leaves the device powered (you can see the driver interacting with the device before it enables runtime PM).

Grygorii Strashko

2014-11-13 19:08 UTC #15 lore raw

Quoted 10 lines · Mark Brown

Quoted 4 lines · Grygorii Strashko
I've checked few folders and below few drivers i found which rely or may rely on their .runtime_resume callback to be executed (It very hard to identify dependency for some of drivers):
arizona-core.c
No, it doesn't - probe() leaves the device powered (you can see the driver interacting with the device before it enables runtime PM).

What I've found there is:
arizona_dev_init()
|- arizona_clk32k_enable()

    |-pm_runtime_get_sync() if arizona->pdata.clk32k_src == ARIZONA_32KZ_MCLK1

so it's added as "may rely"

regards,
-grygorii

Mark Brown

2014-11-13 19:13 UTC #16 lore raw

Quoted 11 lines · Grygorii Strashko

Quoted 2 lines · Mark Brown
No, it doesn't - probe() leaves the device powered (you can see the driver interacting with the device before it enables runtime PM).
What I've found there is:
arizona_dev_init()
|- arizona_clk32k_enable()
    |-pm_runtime_get_sync() if arizona->pdata.clk32k_src == ARIZONA_32KZ_MCLK1
so it's added as "may rely"

No, that's adding a reference to stop it power down under runtime PM - if runtime PM is inactive it will have no effect.

Grygorii Strashko

2014-11-13 20:23 UTC #17 lore raw

Hi Mark,

Quoted 10 lines · Mark Brown

Quoted 9 lines · Grygorii Strashko
Quoted 2 lines · Mark Brown
No, it doesn't - probe() leaves the device powered (you can see the driver interacting with the device before it enables runtime PM).
What I've found there is:
arizona_dev_init()
|- arizona_clk32k_enable()
     |-pm_runtime_get_sync() if arizona->pdata.clk32k_src == ARIZONA_32KZ_MCLK1

            |-arizona_runtime_resume() if dev->driver.pm == arizona_pm_ops

Quoted 6 lines

Quoted 2 lines
so it's added as "may rely"
No, that's adding a reference to stop it power down under runtime PM - if runtime PM is inactive it will have no effect.

NP. You know better - it's not about something wrong. It's just small list of drivers where .runtime_resume() is called or may be called from .probe() (just as an example) :)

regards,
-grygorii

Kevin Hilman

2014-11-14 19:16 UTC #18 lore raw

In reply to Rafael J. Wysocki

Quoted 48 lines · Rafael J. Wysocki

[CC list trimmed]
Quoted 11 lines · Ulf Hansson
[...]
Quoted 8 lines
I guess we do need it for 3.18, but when we are talking about 3.19, before we make any more changes can we outline how power domains are supposed to work?
1. How do we attach a device to power domain? Right now it seems that individual buses are responsible for attaching their devices to power domains. Should we move it into driver core (device_pm_add?) so that device starts its life in its proper power domain?
Quoted 5 lines
That was the initial approach.
To my understanding, Rafael's primary reason for not accepting that was that it's not common, but it's platform-specific.
http://marc.info/?l=linux-pm&m=140243462304669&w=2
For the record, I still believe this is platform-specific.
I also think that the knowledge about what power (or generally PM) domain a device should belong to is not in a bus type or in the driver core. That knowledge is in the code that enumerates devices.
I wonder, then, if we could set the PM domain pointer at about the time when we set the bus type pointer? Things will be consistent all the way through the entire device life cycle then.
Quoted 3 lines
Now, even if we would reconsider doing as you propose, what would the actual benefit be? Obviously, we would get less amount of code to maintain, which is one reason, but are there more?
The same set of subsystem-level PM callbacks will be present for the device throughout its life cycle.
Quoted 10 lines
Quoted 8 lines
2. When do we power up the devices (and the domains)? Right now devices in ACPI power domain are powered when they are attached to the power domain (which coincides with probing), but generic power domains do not do that. Can we add a separate API to explicitly power up the device (and its domain if it is powered down) and do it again, either in device core or in individual buses. This way, regardless of runtime PM or not, we will have devices powered on when driver tries to bind to them. If binding fails we can power down the device.
Isn't that exactly what I implemented in [1], what am I missing?

Quoted 7 lines

Not really. Dmitry is talking about a generic interface independent of PM domains.
If we have pm_power_up(dev)/pm_power_down(dev), then the PM core could use it around really_probe() for all devices. In theory. But that's what we started with when we were working on the runtime PM framework and we ended up with what we have today.

OK, I'll admit it. I'm getting confused.

In the context of PM domains, what does it mean for a device to be "powered"? Isn't it the domain that is powered up or down? Devices within the domain can be runtime resumed or suspended but the domain is what is powered.

That being the case, I'm not seing the point of a per-device power_up/power_down API. As you said, it seems to basically duplicate the runtime PM API.

Quoted 6 lines

Problem is, pm_power_up() would probably end up being pretty much the same as pm_runtime_resume() without executing driver callbacks and similarly for pm_power_down(). That's why I was thinking about running pm_runtime_resume() at the time we know that driver callbacks are not present, just for the purpose of powering up the device. [That has a problem of working with CONFIG_PM_RUNTIME unset, but let me set this one aside for a while.]

I think I'm tending o agree with Alan[1], that this is really best handled by the sybsystem or bus-type, not the core.

Quoted 4 lines

Now, Grygorii seems to be claiming that some drivers *require* their .runtime_resume() callbacks to be executed during .probe() pretty much before anything else which won't happen if pm_runtime_resume() is done before really_probe(). I'm wondering, then, which drivers those are.

Well, it's not technically a requirement, but an assumption that when a _get_sync() is done in a driver's ->probe, it's ->runtime_resume() will be called.

Quoted 4 lines

Essentially, the "power up" operation will depend on the PM domain and bus type, so they'll need to provide callbacks that will most likely duplicate runtime PM callbacks. And those callbacks need to be available when we do the "power up" thing.

Yuck. More callbacks. Just what we need. ;)

I think I'm still not quite getting the fundamental problem being addressed. Why isn't a pm_runtime_get_sync() by the first device in a PM domain sufficient for powering on the domain?

Yes, I understand there are some issues around *keeping* the domain powered, but I think that's a separate problem.

Quoted 5 lines

How can we address that?
We seem to be dealing with a case in which PM is needed on some systems just to make them work at the basic level and not for energy saving, which was what runtime PM was designed for.

I think they're the same thing. Power is needed for devices to work at a basic level, and cutting power is needed for energy savings. Runtime PM is designed for that. :)

Kevin

[1] http://marc.info/?l=linux-pm&m=141511953314249&w=2

Rafael J. Wysocki

2014-11-14 23:24 UTC #19 lore raw

Quoted 1 line · Kevin Hilman

"Rafael J. Wysocki" <[email protected]> writes:

[cut]

Quoted 6 lines

Quoted 4 lines
Essentially, the "power up" operation will depend on the PM domain and bus type, so they'll need to provide callbacks that will most likely duplicate runtime PM callbacks. And those callbacks need to be available when we do the "power up" thing.
Yuck. More callbacks. Just what we need. ;)

Precisely.

Quoted 9 lines

I think I'm still not quite getting the fundamental problem being addressed. Why isn't a pm_runtime_get_sync() by the first device in a PM domain sufficient for powering on the domain?
Yes, I understand there are some issues around *keeping* the domain powered, but I think that's a separate problem.
Quoted 2 lines
How can we address that?

Quoted 7 lines

Quoted 3 lines
We seem to be dealing with a case in which PM is needed on some systems just to make them work at the basic level and not for energy saving, which was what runtime PM was designed for.
I think they're the same thing. Power is needed for devices to work at a basic level, and cutting power is needed for energy savings. Runtime PM is designed for that. :)

Yes, that pretty much is my conclusion.

Consequently, the best way to go in my view is to require PM_RUNTIME to be set when we need to do runtime PM. Which is when we need to dynamically power up and power down things as needed, basically.

Rafael

Rafael J. Wysocki

2014-11-08 00:37 UTC #20 lore raw

PATCH 1/1 PM / Domains: Fix initial default state of the need_restore flag

In reply to Ulf Hansson

Quoted 87 lines · Ulf Hansson

The initial state of the device's need_restore flag should'nt depend on the current state of the PM domain. For example it should be perfectly valid to attach an inactive device to a powered PM domain.
The pm_genpd_dev_need_restore() API allow us to update the need_restore flag to somewhat cope with such scenarios. Typically that should have been done from drivers/buses ->probe() since it's those that put the requirements on the value of the need_restore flag.
Until recently, the Exynos SOCs were the only user of the pm_genpd_dev_need_restore() API, though invoking it from a centralized location while adding devices to their PM domains.
Due to that Exynos now have swithed to the generic OF-based PM domain look-up, it's no longer possible to invoke the API from a centralized location. The reason is because devices are now added to their PM domains during the probe sequence.
Commit "ARM: exynos: Move to generic PM domain DT bindings" did the switch for Exynos to the generic OF-based PM domain look-up, but it also removed the call to pm_genpd_dev_need_restore(). This caused a regression for some of the Exynos drivers.
To handle things more properly in the generic PM domain, let's change the default initial value of the need_restore flag to reflect that the state is unknown. As soon as some of the runtime PM callbacks gets invoked, update the initial value accordingly.
Moreover, since the generic PM domain is verifying that all device's are both runtime PM enabled and suspended, using pm_runtime_suspended() while pm_genpd_poweroff() is invoked from the scheduled work, we can be sure of that the PM domain won't be powering off while having active devices.
Do note that, the generic PM domain can still only know about active devices which has been activated through invoking its runtime PM resume callback. In other words, buses/drivers using pm_runtime_set_active() during ->probe() will still suffer from a race condition, potentially probing a device without having its PM domain being powered. That issue will have to be solved using a different approach.
This a log from the boot regression for Exynos5, which is being fixed in this patch.
------------[ cut here ]------------
WARNING: CPU: 0 PID: 308 at ../drivers/clk/clk.c:851 clk_disable+0x24/0x30()
Modules linked in:
CPU: 0 PID: 308 Comm: kworker/0:1 Not tainted 3.18.0-rc3-00569-gbd9449f-dirty #10
Workqueue: pm pm_runtime_work
[<c0013c64>] (unwind_backtrace) from [<c0010dec>] (show_stack+0x10/0x14)
[<c0010dec>] (show_stack) from [<c03ee4cc>] (dump_stack+0x70/0xbc)
[<c03ee4cc>] (dump_stack) from [<c0020d34>] (warn_slowpath_common+0x64/0x88)
[<c0020d34>] (warn_slowpath_common) from [<c0020d74>] (warn_slowpath_null+0x1c/0x24)
[<c0020d74>] (warn_slowpath_null) from [<c03107b0>] (clk_disable+0x24/0x30)
[<c03107b0>] (clk_disable) from [<c02cc834>] (gsc_runtime_suspend+0x128/0x160)
[<c02cc834>] (gsc_runtime_suspend) from [<c0249024>] (pm_generic_runtime_suspend+0x2c/0x38)
[<c0249024>] (pm_generic_runtime_suspend) from [<c024f44c>] (pm_genpd_default_save_state+0x2c/0x8c)
[<c024f44c>] (pm_genpd_default_save_state) from [<c024ff2c>] (pm_genpd_poweroff+0x224/0x3ec)
[<c024ff2c>] (pm_genpd_poweroff) from [<c02501b4>] (pm_genpd_runtime_suspend+0x9c/0xcc)
[<c02501b4>] (pm_genpd_runtime_suspend) from [<c024a4f8>] (__rpm_callback+0x2c/0x60)
[<c024a4f8>] (__rpm_callback) from [<c024a54c>] (rpm_callback+0x20/0x74)
[<c024a54c>] (rpm_callback) from [<c024a930>] (rpm_suspend+0xd4/0x43c)
[<c024a930>] (rpm_suspend) from [<c024bbcc>] (pm_runtime_work+0x80/0x90)
[<c024bbcc>] (pm_runtime_work) from [<c0032a9c>] (process_one_work+0x12c/0x314)
[<c0032a9c>] (process_one_work) from [<c0032cf4>] (worker_thread+0x3c/0x4b0)
[<c0032cf4>] (worker_thread) from [<c003747c>] (kthread+0xcc/0xe8)
[<c003747c>] (kthread) from [<c000e738>] (ret_from_fork+0x14/0x3c)
---[ end trace 40cd58bcd6988f12 ]---
Fixes a4a8c2c4962bb655 (ARM: exynos: Move to generic PM domain DT bindings)
Reported-by Sylwester Nawrocki <[email protected]>
Signed-off-by Ulf Hansson <[email protected]>
---
This patch is intended as fix for 3.18 rc[n] due to the regression for Exynos SOCs.
I would also like to call for help in getting this thoroughly tested.
I have tested this on Arndale Dual, Exynos 5250. According the log attached in the commit message as well.
I have tested this on UX500, which support for the generic PM domain is about to be queued for 3.19. Since UX500 uses the AMBA bus/drivers, which uses pm_runtime_set_active() instead of pm_runtime_get_sync() during ->probe(), I could also verify that this behavior is maintained.

Quoted 4 lines

Finally I have also tested this patchset on UX500 and using the below patchset to make sure the approach is suitable long term wise as well. [PATCH v3 0/9] PM / Domains: Fix race conditions during boot
http://www.spinics.net/lists/arm-kernel/msg369409.html

I'm generally fine with the approach, but ->

Quoted 10 lines

---
 drivers/base/power/domain.c | 38 +++++++++++++++++++++++++++++++-------
 include/linux/pm_domain.h   |  2 +-
 2 files changed, 32 insertions(+), 8 deletions(-)

drivers/base/power/domain.c +0 -0

index b520687..a9523a3 100644

drivers/base/power/domain.c static int __pm_genpd_save_device(struct pm_domain_data *pdd, · line 364

Quoted 5 lines

drivers/base/power/domain.c +1 -1

@@ -361,9 +361,19 @@ static int __pm_genpd_save_device(struct pm_domain_data *pdd,
 	struct device *dev = pdd->dev;
 	int ret = 0;
 
-	if (gpd_data->need_restore)
+	if (gpd_data->need_restore == 1)

-> I'd prefer checks for the sign rather than for a specific value, like

	if (gpd_data->need_restore > 0)

drivers/base/power/domain.c static int __pm_genpd_save_device(struct pm_domain_data *pdd, · line 366

Quoted 2 lines

drivers/base/power/domain.c +0 -0
 		return 0;
 

and so on.

Quoted 30 lines

drivers/base/power/domain.c +12 -3

+	/*
+	 * If the value of the need_restore flag is still unknown at this point,
+	 * we trust that pm_genpd_poweroff() has verified that the device is
+	 * already runtime PM suspended.
+	 */
+	if (gpd_data->need_restore == -1) {
+		gpd_data->need_restore = 1;
+		return 0;
+	}
+
 	mutex_unlock(&genpd->lock);
 
 	genpd_start_dev(genpd, dev);

@@ -373,7 +383,7 @@ static int __pm_genpd_save_device(struct pm_domain_data *pdd,
 	mutex_lock(&genpd->lock);
 
 	if (!ret)
-		gpd_data->need_restore = true;
+		gpd_data->need_restore = 1;
 
 	return ret;
 }

@@ -389,13 +399,17 @@ static void __pm_genpd_restore_device(struct pm_domain_data *pdd,
 {
 	struct generic_pm_domain_data *gpd_data = to_gpd_data(pdd);
 	struct device *dev = pdd->dev;
-	bool need_restore = gpd_data->need_restore;
+	int need_restore = gpd_data->need_restore;
 
-	gpd_data->need_restore = false;

drivers/base/power/domain.c static void __pm_genpd_restore_device(struct pm_domain_data *pdd, · line 412

Quoted 10 lines

drivers/base/power/domain.c +6 -1

+	gpd_data->need_restore = 0;
 	mutex_unlock(&genpd->lock);
 
 	genpd_start_dev(genpd, dev);
-	if (need_restore)
+	/*
+	 * Make sure to also restore those devices which we until now, didn't
+	 * know the state for.
+	 */
+	if (need_restore != 0)

and here you can do

if (need_restore)

Quoted 59 lines

drivers/base/power/domain.c +12 -2

 		genpd_restore_dev(genpd, dev);
 
 	mutex_lock(&genpd->lock);

@@ -603,6 +617,7 @@ static void genpd_power_off_work_fn(struct work_struct *work)
 static int pm_genpd_runtime_suspend(struct device *dev)
 {
 	struct generic_pm_domain *genpd;
+	struct generic_pm_domain_data *gpd_data;
 	bool (*stop_ok)(struct device *__dev);
 	int ret;

@@ -628,6 +643,15 @@ static int pm_genpd_runtime_suspend(struct device *dev)
 		return 0;
 
 	mutex_lock(&genpd->lock);
+	/*
+	 * If we have an unknown state of the need_restore flag, it means none
+	 * of the runtime PM callbacks has been invoked yet. Let's update the
+	 * flag to reflect that the current state is active.
+	 */
+	gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
+	if (gpd_data->need_restore == -1)
+		gpd_data->need_restore = 0;
+
 	genpd->in_progress++;
 	pm_genpd_poweroff(genpd);
 	genpd->in_progress--;

@@ -1442,7 +1466,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	mutex_lock(&gpd_data->lock);
 	gpd_data->base.dev = dev;
 	list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
-	gpd_data->need_restore = genpd->status == GPD_STATE_POWER_OFF;
+	gpd_data->need_restore = -1;
 	gpd_data->td.constraint_changed = true;
 	gpd_data->td.effective_constraint_ns = -1;
 	mutex_unlock(&gpd_data->lock);

@@ -1546,7 +1570,7 @@ void pm_genpd_dev_need_restore(struct device *dev, bool val)
 
 	psd = dev_to_psd(dev);
 	if (psd && psd->domain_data)
-		to_gpd_data(psd->domain_data)->need_restore = val;
+		to_gpd_data(psd->domain_data)->need_restore = val ? 1 : 0;
 
 	spin_unlock_irqrestore(&dev->power.lock, flags);
 }

include/linux/pm_domain.h +1 -1

index b3ed776..2e0e06d 100644
@@ -106,7 +106,7 @@ struct generic_pm_domain_data {
 	struct notifier_block nb;
 	struct mutex lock;
 	unsigned int refcount;
-	bool need_restore;
+	int need_restore;
 };
 
 #ifdef CONFIG_PM_GENERIC_DOMAINS

Signature · 2 lines

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