subsys: pm: add check for device busy in policy #61726

leifuzhao · 2023-08-22T08:49:55Z

Should add check for device busy here because one or more devices may still in busy, otherwise problem may happen due to system go into low power mode with some devices still in busy. Issue is observed in Intel ISH platform.

gmarull

check is already done here

zephyr/subsys/pm/pm.c

Line 72 in 63bb74c

if (!device_is_ready(dev) || pm_device_is_busy(dev) ||

gmarull · 2023-08-22T08:54:41Z

subsys/pm/policy.c

@@ -136,6 +137,12 @@ const struct pm_state_info *pm_policy_next_state(uint8_t cpu, int32_t ticks)
 	uint8_t num_cpu_states;
 	const struct pm_state_info *cpu_states;

+#if CONFIG_PM_DEVICE
+	if (pm_device_is_any_busy()) {


Also, take a look at the state locking API. It's the device driver code that needs to prevent system sleep if it's busy doing something.

Out Intel ISH platform requires all devices are not busy to enter into system low power. Add state locking in every device driver may not be a suitable solution. Is it possible to add a Kconfig option in pm that enable such property, for example config NEED_ALL_DEVICES_UNBUSY, if this config is y then runs this pm_device_is_any_busy() checking in policy.c?

here is a bad case:

a driver sets device busy, starts one transaction, then waits the completion semaphore with a long timeout or even FOREVER.

Zephyr runs to idle thread and gets a long idle time, so PM policy returns a deep sleep state.

PM suspends all other devices, then SoC goes into very low PM state, maybe suspend to disk.

SoC detects there's device interrupt pending, so wakes up immediately.

do all resume works.

device interrupt gets served.
in such a case, low-power states should be blocked. Any driver should avoid it. It's like a common thing. It seems not good for all drivers to call pm state lock APIs.

If your device is waiting on something, and needs to prevent deep sleep, simply lock deep sleep states while waiting.

It seems to me that most drivers have similar working model:

set register to trigger a transaction.

wait complete semaphore which will be given in ISR.

complete the transaction.
all those drivers should call PM state lock APIs to lock all low power states after step 1. as a common driver, I've afraid they should call PM state lock API multi times to lock PM_STATE_SUSPEND_TO_IDLE, PM_STATE_STANDBY, PM_STATE_SUSPEND_TO_RAM and PM_STATE_SUSPEND_TO_DISK, then call the API multi times to unlock them after transaction done.
this seems not good at all.

current solution doesn't solve the problems I mentioned.

It is fine for me having this option wrapped around a Kconfig option if that is a common needed for devices in a platform. If it is particular to one device, I do stay with the state lock solution, but if that is common thing for the platform, I think this solution is reasonable, it less error prone.

all ISH devices need it, except three on AON domain, HPET/RTC/IPC.

If ISH has more specific cases in term of pm_state selection, You can always define platform-specific policy CONFIG_PM_POLICY_CUSTOM.

If ISH has more specific cases in term of pm_state selection, You can always define platform-specific policy CONFIG_PM_POLICY_CUSTOM.

agree, why not use a custom policy instead of change the default policy. PM_NEED_ALL_DEVICES_IDLE seems to be very specialized and soon enough we will be wondering what is the usecase behind it without too much context and details in the documentation. See https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/subsys/pm/power_mgmt/src/main.c#L222C29-L222C49 for an example of a custom policy.

leifuzhao · 2023-08-23T02:18:44Z

check is already done here

zephyr/subsys/pm/pm.c

Line 72 in 63bb74c

if (!device_is_ready(dev) || pm_device_is_busy(dev) ||

This check is not enough for system level suspend. As can be seen from this code below, when one device is busy, the code runs continue statement, it means this device busy doesn't prevent system suspend which is wrong because system level suspend should happen when no device is busy, otherwise will surely create problem which has already been seen in Intel ISH platform.

	if (!device_is_ready(dev) || pm_device_is_busy(dev) ||
	    pm_device_state_is_locked(dev) ||
	    pm_device_wakeup_is_enabled(dev) ||
	    pm_device_runtime_is_enabled(dev)) {
		continue;
	}

gmarull · 2023-08-23T07:41:19Z

check is already done here

zephyr/subsys/pm/pm.c

Line 72 in 63bb74c

if (!device_is_ready(dev) || pm_device_is_busy(dev) ||

This check is not enough for system level suspend. As can be seen from this code below, when one device is busy, the code runs continue statement, it means this device busy doesn't prevent system suspend which is wrong because system level suspend should happen when no device is busy, otherwise will surely create problem which has already been seen in Intel ISH platform.
	if (!device_is_ready(dev) || pm_device_is_busy(dev) ||
	    pm_device_state_is_locked(dev) ||
	    pm_device_wakeup_is_enabled(dev) ||
	    pm_device_runtime_is_enabled(dev)) {
		continue;
	}

Then just use the state locking APIs as suggested above.

ceolin · 2023-08-23T15:31:15Z

check is already done here

zephyr/subsys/pm/pm.c

Line 72 in 63bb74c

if (!device_is_ready(dev) || pm_device_is_busy(dev) ||

This check is not enough for system level suspend. As can be seen from this code below, when one device is busy, the code runs continue statement, it means this device busy doesn't prevent system suspend which is wrong because system level suspend should happen when no device is busy, otherwise will surely create problem which has already been seen in Intel ISH platform.
	if (!device_is_ready(dev) || pm_device_is_busy(dev) ||
	    pm_device_state_is_locked(dev) ||
	    pm_device_wakeup_is_enabled(dev) ||
	    pm_device_runtime_is_enabled(dev)) {
		continue;
	}

Then just use the state locking APIs as suggested above.

Agree, if that is the case, the right way to prevent the system to sleep is putting a constraint.

subsys/pm/Kconfig

gmarull · 2023-09-06T13:41:20Z

subsys/pm/policy.c

@@ -136,6 +137,12 @@ const struct pm_state_info *pm_policy_next_state(uint8_t cpu, int32_t ticks)
 	uint8_t num_cpu_states;
 	const struct pm_state_info *cpu_states;

+#if defined(CONFIG_PM_DEVICE) && defined(CONFIG_PM_NEED_ALL_DEVICES_IDLE)


This pollutes CONFIG_PM code with PM_DEVICE related stuff, so to me this is a -1. As explained, there are other more precise means to solve this issue, that is, with state locking (maybe refining its implementation to accommodate new requirements). There's also no need to enable CONFIG_PM_DEVICE to modify the behavior of system PM, this is why we have policy APIs.

@gmarull I have no knowledge of other SoCs/platforms, but Intel ISH's SoC core low-power mode and device/peripheral power do have relationship. When CPU goes into deep sleep, devices' power will be gated.
So ISH does need this change, appreciate your understanding.

@gmarull Hi Gerald, we do need this patch for production. could you approve it? or if you have any more concern, let me know. Thanks.

I expressed my concerns, and they have not been addressed. Let me know if this patch has further updates.

@gmarull Hi Gerald, just get your point. Previously I thought CONFIG_PM_DEVICE is a sub-config of CONFIG_PM because CONFIG_PM_DEVICE has prefix CONFIG_PM, but it seems they're independent from subsystem/pm/Kconfig.
but this seems weird to me. Shouldn't Device PM be part of PM (Power Management)? @ceolin @nashif
Do you know there is any SoC that needs only CONFIG_PM_DEVICE but no CONFIG_PM? I wonder if there's any such need.

@gmarull I am not following what you are objecting to exactly, the code has changed since this:

This pollutes CONFIG_PM code with PM_DEVICE related stuff, so to me this is a -1

tmleman

Not blocking since it's hidden behind #ifdef.

tmleman · 2023-09-11T10:23:39Z

subsys/pm/policy.c

@@ -136,6 +137,12 @@ const struct pm_state_info *pm_policy_next_state(uint8_t cpu, int32_t ticks)
 	uint8_t num_cpu_states;
 	const struct pm_state_info *cpu_states;

+#if CONFIG_PM_DEVICE
+	if (pm_device_is_any_busy()) {


If ISH has more specific cases in term of pm_state selection, You can always define platform-specific policy CONFIG_PM_POLICY_CUSTOM.

ceolin

I think that is an acceptable constraint / behavior for the policy. Devices and SoC are not completely disconnect things in some cases, so having it protected by a build option is ok for me.

subsys/pm/Kconfig

nashif · 2023-09-11T23:10:11Z

subsys/pm/policy.c

@@ -136,6 +137,12 @@ const struct pm_state_info *pm_policy_next_state(uint8_t cpu, int32_t ticks)
 	uint8_t num_cpu_states;
 	const struct pm_state_info *cpu_states;

+#if CONFIG_PM_DEVICE
+	if (pm_device_is_any_busy()) {


If ISH has more specific cases in term of pm_state selection, You can always define platform-specific policy CONFIG_PM_POLICY_CUSTOM.

agree, why not use a custom policy instead of change the default policy. PM_NEED_ALL_DEVICES_IDLE seems to be very specialized and soon enough we will be wondering what is the usecase behind it without too much context and details in the documentation. See https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/subsys/pm/power_mgmt/src/main.c#L222C29-L222C49 for an example of a custom policy.

Add check for device busy when CONFIG_PM_NEED_ALL_DEVICES_IDLE is set to y because one or more devices may still in busy and causes problem when system go into low power in Intel ISH platform. Signed-off-by: Leifu Zhao <leifu.zhao@intel.com>

kwd-doodling · 2023-09-12T04:46:23Z

We've considered CONFIG_PM_POLICY_CUSTOM. That patch is just a copy of current policy function with changes in this PR. Leifu wondered weather it's acceptable by community for duplicated code.
@ceolin @nashif either way we're OK. Please give your point which is better.

ceolin · 2023-09-12T16:27:56Z

We've considered CONFIG_PM_POLICY_CUSTOM. That patch is just a copy of current policy function with changes in this PR. Leifu wondered weather it's acceptable by community for duplicated code. @ceolin @nashif either way we're OK. Please give your point which is better.

I recommend using custom policy when you have very unique needs, like actions depending on specific cores, specialized constraints, ... This one didn't look a super unique case for me. The drawback for custom policy is that you have to account state and latency constraints in your policy for those APIs work correctly.
That's said, since it looks controversial and you are ok in have a custom policy, that is probably the easiest/best approach now.

leifuzhao · 2023-09-13T08:36:10Z

We've considered CONFIG_PM_POLICY_CUSTOM. That patch is just a copy of current policy function with changes in this PR. Leifu wondered weather it's acceptable by community for duplicated code. @ceolin @nashif either way we're OK. Please give your point which is better.

I recommend using custom policy when you have very unique needs, like actions depending on specific cores, specialized constraints, ... This one didn't look a super unique case for me. The drawback for custom policy is that you have to account state and latency constraints in your policy for those APIs work correctly. That's said, since it looks controversial and you are ok in have a custom policy, that is probably the easiest/best approach now.

Agree. POLICY_CUSTOM is not feasible solution at least right now. It is not simple as just copy and modify as imagined. Just copy pm_policy_next_state() and add check_all_busy() doesn't work since there are several variables defined as static outside the pm_policy_next_state() function in subsys/pm/policy.c, such as next_event_cyc, max_latency_cyc. If copy whole subsys/pm/policy.c file and add check_all_busy(), it also doesn't work. This is becasue subsys/pm/policy.c defines several API functions such as pm_policy_latency_request_update() etc, then there will be multiple definitions of same function. But if remove them in the new copied file, for example remove pm_policy_latency_request_update(), then pm_policy_latency_request_update() in subsys/pm/policy.c will call static function update_max_latency() which manipulates and updates local copy of max_latency_cyc. The max_latency_cyc variable used by pm_policy_next_state() in new copied file doesn't get updated which is wrong. So we have to change and add code to deal with and this brings risk to already working well ish PM.

kwd-doodling · 2023-09-13T12:30:44Z

could #62550 be merged? I think it's good to solve the issue Leifu mentioned when implementing POLICY_CUSTOM.

kwd-doodling · 2023-09-14T12:45:43Z

could #62550 be merged? I think it's good to solve the issue Leifu mentioned when implementing POLICY_CUSTOM.
@ceolin @nashif is it possible to get #62550 merged quickly? ISH really needs it.

nashif · 2023-09-15T14:00:46Z

could #62550 be merged? I think it's good to solve the issue Leifu mentioned when implementing POLICY_CUSTOM.

was trying to show possible solutions, I know it is not great, and I think we can do better than that introducing hooks rather than reimplementation of complete functions.

nashif

will have a followup proposal to improve use of custom policies without duplicating much code...

no response, please revisit.

ceolin · 2023-09-15T16:59:47Z

Hi, I'll re-enforce the +1 here. I think this is a reasonable constraint and it is protected under a build option, it is nothing crazy. The custom policy is supposed (or at least documented) to be used by the application, so doing it here would not allow applications to override its behavior. I was thinking about it ... much of the logic inside the default policy should be available to be used by custom policies ... the way it is now is, at least. ... painful

gmarull · 2023-09-18T11:15:51Z

So yet another abuse of powers mergings PRs, and then we are opening proposals like this #61358, I guess they're a joke.

I disagree on the way the subsystem is maintained. For more context refer to zephyrproject-rtos#61726 or https://discord.com/channels/720317445772017664/997527108844798012/ 1153288380231208960 Signed-off-by: Gerard Marull-Paretas <gerard@teslabs.com>

I disagree on the way the subsystem is maintained. For more context refer to #61726 or https://discord.com/channels/720317445772017664/997527108844798012/ 1153288380231208960 Signed-off-by: Gerard Marull-Paretas <gerard@teslabs.com>

I disagree on the way the subsystem is maintained. For more context refer to zephyrproject-rtos/zephyr#61726 or https://discord.com/channels/720317445772017664/997527108844798012/ 1153288380231208960 (cherry picked from commit 4ba97c2) Original-Signed-off-by: Gerard Marull-Paretas <gerard@teslabs.com> GitOrigin-RevId: 4ba97c2 Change-Id: I2cf897da2b65b4068a111ec0cd3016ad2d351f5b Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/4875292 Reviewed-by: Al Semjonovs <asemjonovs@google.com> Tested-by: ChromeOS Prod (Robot) <chromeos-ci-prod@chromeos-bot.iam.gserviceaccount.com> Commit-Queue: Al Semjonovs <asemjonovs@google.com> Tested-by: Al Semjonovs <asemjonovs@google.com>

I disagree on the way the subsystem is maintained. For more context refer to zephyrproject-rtos#61726 or https://discord.com/channels/720317445772017664/997527108844798012/ 1153288380231208960 Signed-off-by: Gerard Marull-Paretas <gerard@teslabs.com>

leifuzhao requested review from nashif and ceolin as code owners August 22, 2023 08:49

zephyrbot added the area: Power Management label Aug 22, 2023

zephyrbot requested review from gmarull, JordanYates, teburd and tmleman August 22, 2023 08:50

zephyrbot assigned ceolin Aug 22, 2023

gmarull previously requested changes Aug 22, 2023

View reviewed changes

gmarull reviewed Aug 22, 2023

View reviewed changes

leifuzhao force-pushed the mainout1 branch from efeeefb to 26ad466 Compare September 5, 2023 09:25

leifuzhao requested a review from dcpleung as a code owner September 5, 2023 09:25

leifuzhao force-pushed the mainout1 branch from 26ad466 to 6194a06 Compare September 5, 2023 09:26

zephyrbot added platform: X86 x86 and x86-64 platform: Intel ISH Intel Corporation, Integrated Sensor Hub labels Sep 5, 2023

zephyrbot requested review from jhedberg, laurenmurphyx64 and tbursztyka September 5, 2023 09:26

leifuzhao force-pushed the mainout1 branch from 6194a06 to d03e2cf Compare September 6, 2023 07:23

ceolin reviewed Sep 6, 2023

View reviewed changes

subsys/pm/Kconfig Outdated Show resolved Hide resolved

gmarull reviewed Sep 6, 2023

View reviewed changes

leifuzhao force-pushed the mainout1 branch 2 times, most recently from 32d275e to 54a51bf Compare September 7, 2023 06:04

kwd-doodling previously approved these changes Sep 7, 2023

View reviewed changes

kwd-doodling requested review from ceolin and gmarull September 7, 2023 08:01

tmleman reviewed Sep 11, 2023

View reviewed changes

ceolin previously approved these changes Sep 11, 2023

View reviewed changes

nashif requested changes Sep 11, 2023

View reviewed changes

leifuzhao dismissed stale reviews from ceolin and kwd-doodling via 29bd945 September 12, 2023 01:38

leifuzhao force-pushed the mainout1 branch from 54a51bf to 29bd945 Compare September 12, 2023 01:38

ceolin mentioned this pull request Sep 12, 2023

PM: allow custom policy to use default policy code without duplication #62550

Closed

nashif approved these changes Sep 15, 2023

View reviewed changes

ceolin approved these changes Sep 15, 2023

View reviewed changes

nashif merged commit 61ab3a8 into zephyrproject-rtos:main Sep 15, 2023
17 checks passed

gmarull mentioned this pull request Sep 18, 2023

Define guidelines for improved four eyes principle #61358

Closed

gmarull mentioned this pull request Sep 18, 2023

maintainers: remove gmarull as a PM subsystem collaborator #62781

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

subsys: pm: add check for device busy in policy #61726

subsys: pm: add check for device busy in policy #61726

leifuzhao commented Aug 22, 2023

gmarull left a comment

gmarull Aug 22, 2023

leifuzhao Sep 4, 2023

kwd-doodling Sep 4, 2023

gmarull Sep 4, 2023

kwd-doodling Sep 4, 2023

gmarull Sep 6, 2023

ceolin Sep 6, 2023

kwd-doodling Sep 7, 2023 •

edited

Loading

tmleman Sep 11, 2023

nashif Sep 11, 2023

leifuzhao commented Aug 23, 2023

gmarull commented Aug 23, 2023

ceolin commented Aug 23, 2023

gmarull Sep 6, 2023

kwd-doodling Sep 7, 2023

kwd-doodling Sep 7, 2023 •

edited

Loading

gmarull Sep 7, 2023

kwd-doodling Sep 8, 2023 •

edited

Loading

nashif Sep 11, 2023

tmleman left a comment

tmleman Sep 11, 2023

ceolin left a comment

nashif Sep 11, 2023

kwd-doodling commented Sep 12, 2023

ceolin commented Sep 12, 2023

leifuzhao commented Sep 13, 2023

kwd-doodling commented Sep 13, 2023

kwd-doodling commented Sep 14, 2023

nashif commented Sep 15, 2023

nashif left a comment

ceolin commented Sep 15, 2023 •

edited

Loading

gmarull commented Sep 18, 2023

subsys: pm: add check for device busy in policy #61726

subsys: pm: add check for device busy in policy #61726

Conversation

leifuzhao commented Aug 22, 2023

gmarull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwd-doodling Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leifuzhao commented Aug 23, 2023

gmarull commented Aug 23, 2023

ceolin commented Aug 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwd-doodling Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwd-doodling Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmleman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceolin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwd-doodling commented Sep 12, 2023

ceolin commented Sep 12, 2023

leifuzhao commented Sep 13, 2023

kwd-doodling commented Sep 13, 2023

kwd-doodling commented Sep 14, 2023

nashif commented Sep 15, 2023

nashif left a comment

Choose a reason for hiding this comment

ceolin commented Sep 15, 2023 • edited Loading

gmarull commented Sep 18, 2023

kwd-doodling Sep 7, 2023 •

edited

Loading

kwd-doodling Sep 7, 2023 •

edited

Loading

kwd-doodling Sep 8, 2023 •

edited

Loading

ceolin commented Sep 15, 2023 •

edited

Loading