pm: device: Several fixes #34781

ceolin · 2021-05-03T18:42:14Z

Don't wake up devices unnecessarily
Fix concurrence issues with k_poll_signal. Using k_condvar instead.
Fix pre-kernel API usage
Some code guideline fixes
Use spin lock instead of semaphore to protect critical sessions, since it was the semaphore with K_FOREVER which is not allowed from ISR.
Change API signature for clarity sake

sjg20 · 2021-05-04T15:55:00Z

subsys/power/device.c

@@ -60,26 +60,63 @@ static device_idx_t num_pm;
 /* Number of devices successfully suspended. */
 static device_idx_t num_susp;

+static bool should_suspend(const struct device *dev, uint32_t state)


Are there tests for this behaviour?

yep:

./samples/drivers/spi_flash_at45/prj.conf:CONFIG_PM_DEVICE=y ./samples/subsys/power/device_pm/prj.conf:CONFIG_PM_DEVICE=y ./samples/boards/ti/cc13x2_cc26x2/system_off/prj.conf:CONFIG_PM_DEVICE=y ./samples/boards/nrf/system_off/prj.conf:CONFIG_PM_DEVICE=y ./samples/boards/stm32/power_mgmt/blinky/prj.conf:CONFIG_PM_DEVICE=y ./tests/net/pm/prj.conf:CONFIG_PM_DEVICE=y ./tests/subsys/power/power_mgmt/prj.conf:CONFIG_PM_DEVICE=y

sjg20 · 2021-05-04T15:56:31Z

drivers/display/display_st7789v.c

@@ -409,14 +409,14 @@ static void st7789v_enter_sleep(struct st7789v_data *data)
 }

 static int st7789v_pm_control(const struct device *dev, uint32_t ctrl_command,
-				 void *context, device_pm_cb cb, void *arg)
+				 uint32_t *state, device_pm_cb cb, void *arg)


What if we need to add something more in future...should this be a struct with an int inside?

the last parameter is supposed to be used as context to the callback. This variable that was being called void *context was being treated as an uint32_t * by the device_pm framework (and everywhere else), so it is not like you could just change it to be a different type in your use case. If you need to pass a different data you should use *arg.

The reason to change it is, as you can see, very confuse and with bad variable names this function signature.

Still unclear why it wasn't made into struct pm_context { uint32_t state }; to allow for seamless extensibility going forward, but ok...

sjg20 · 2021-05-04T15:57:46Z

subsys/power/device_pm.c

-	k_poll_signal_raise(&dev->pm->signal, pm_state);
+	/*
+	 * This function returns the number of woken threads on success. There
+	 * is nothing we can do with this information. Just ignoring it.


sjg20 · 2021-05-04T16:01:20Z

subsys/power/device_pm.c


-	return result == target_state ? 0 : -EIO;
+	return target_state == atomic_get(&dev->pm->fsm_state) ? 0 : -EIO;


This code is a bit hard to understand. How does it actually end up being in the target state? How does it know what to do, since target_state is not stored anywhere.

you request to device to change to target_state, as this operation may be asynchronous the device is responsible to call the given callback with its state after this operation. The state given by the device is then stored in dev->pm->fsm_state we then compare here to check if the state was properly set.

ceolin · 2021-05-04T16:28:25Z

@sjg20 is not clear for me what changes were requested, there is anything other than the typo that needs to be changed after my answers ?

sjg20

Looks good. I do think a comment on the operation (like you provided in the response) would be useful

ceolin · 2021-05-04T16:46:00Z

Looks good. I do think a comment on the operation (like you provided in the response) would be useful

will add it, thanks for the feedback

Currently when the system goes to sleep it asks *all* devices that support PM to suspend or go to a low power state, and the system wakes up it put *all* suspended devices in active state, even if a device was already suspended and not being used. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

The context parameter used across device power management is actually the power state. Just use it and avoid a lot of unnecessary casts. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

Don't use kernel services before in pre-kernel state. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

BIT macro properly handle bit shift operations. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

The sync API was using k_poll_signal and in certain conditions is possible multiple threads waiting on a signal leading to an undefined behavior. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

Add an explicit fallthrough in a case label. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

Device pm runtime was using semaphore to protect critical section but enable / disable functions were waiting on the semaphore. So, just replace it with a spin lock. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

Check if a device is busy before suspend it. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

Rename state variable. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

erwango · 2021-05-07T12:27:12Z

subsys/pm/device_runtime.c

@@ -111,6 +111,16 @@ static int pm_device_request(const struct device *dev,
 		}
 	}

+	if (k_is_pre_kernel()) {


I'm not convinced this is the correct way.
This means clock activation should be done on device side, on top of calling pm_device_get primitive (which at least, should be documented).
Instead, isn't it a way to run the worker routine code in a synchronous way at this stage ?

What about adding the following just here ?

if (dev->pm->usage == 1) { (void)device_set_power_state(dev, DEVICE_PM_ACTIVE_STATE, NULL, NULL); } else if (dev->pm->usage == 0) { (void)device_set_power_state(dev, DEVICE_PM_SUSPEND_STATE, NULL, NULL); }

yes, I was staring at the same code as well, this does not seem to be correct. Here we need to enable power directly and without having to worry about concurrency, along the lines of what @erwango is proposing.

This exactly code was part of my other pr
64dac71

The reason for what I have not included it in this one is because this is a workaround from my understanding. At this point devices should have being initialized and powered on accordingly and this activation unnecessary. This is fixing an existing problem not in the proper way imho.

But as we don't have it fixed, I can cherry-pick this commit and once we have it done properly I remove this path.

Btw, this is not about concurrence, but putting the device in an unexpected condition. From the device perspective is expected the init function be called before the power control function. The device may do some data initialization in the init that will be used in the power control ....

At this point devices should have being initialized and powered on accordingly and this activation unnecessary.

No, this is not the case. you need to be able to use those drivers and use them means powering them up if they are not active. The issue was originally exactly that, we are trying to power on a device and cant be cause kernel is still not up, i.e. we try to deal with code designed for multithreds and concurrency that in this stage can be skipped.

yep, but not in pre-kernel. Honestly I don't see why devices are not initialized in the right order at this point. Anyway, I've cherry-picked that commit

what does initialized mean exactly? init() AFAIK does not power up a device necessarily..

call init()

nashif · 2021-05-07T12:52:29Z

subsys/pm/device_runtime.c

@@ -111,6 +111,16 @@ static int pm_device_request(const struct device *dev,
 		}
 	}

+	if (k_is_pre_kernel()) {


yes, I was staring at the same code as well, this does not seem to be correct. Here we need to enable power directly and without having to worry about concurrency, along the lines of what @erwango is proposing.

Devices may be initialized but started powered down for this reason is necessary to power a device on if requested even if in pre-kernel state. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

ceolin requested review from ABOSTM, anangl, andyross, avisconti, bwitherspoon, ChiHuaL, dcpleung, erwango, FRASTM, jukkar, MaureenHelm, Mierunski, MulinChao, nashif, nvlsianpu, pfalcon, tbursztyka, vanwinkeljan and WealianLiao as code owners May 3, 2021 18:42

sjg20 requested changes May 4, 2021

View reviewed changes

sjg20 approved these changes May 4, 2021

View reviewed changes

ceolin force-pushed the pm/device-split branch from 2a14e11 to d5bd2ab Compare May 4, 2021 17:21

nashif assigned nashif and unassigned ioannisg May 4, 2021

pfalcon removed their request for review May 5, 2021 14:40

erwango mentioned this pull request May 5, 2021

STM32: Run time GPIO clocks gating #32788

Merged

galak added this to the v2.6.0 milestone May 5, 2021

ceolin force-pushed the pm/device-split branch from d5bd2ab to af42965 Compare May 6, 2021 21:04

ceolin requested review from carlescufi and galak as code owners May 6, 2021 21:04

Flavio Ceolin added 9 commits May 6, 2021 15:42

power: device: void *context -> uint32_t *state

f30909b

The context parameter used across device power management is actually the power state. Just use it and avoid a lot of unnecessary casts. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

power: device_pm: Check if is running in pre-kernel

cf1a04b

Don't use kernel services before in pre-kernel state. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

power: device_pm: Use BIT macro for code compliance

0dce30b

BIT macro properly handle bit shift operations. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

power: device_pm: Fix concurrence issues

6fd331c

The sync API was using k_poll_signal and in certain conditions is possible multiple threads waiting on a signal leading to an undefined behavior. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

power: device_pm: Add missing fallthrough

9ddff59

Add an explicit fallthrough in a case label. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

power: device_pm: Use spin lock instead of semaphore

21b9145

Device pm runtime was using semaphore to protect critical section but enable / disable functions were waiting on the semaphore. So, just replace it with a spin lock. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

power: device: Do not suspend a busy device

910bead

Check if a device is busy before suspend it. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

pm: device: fsm_state -> state

d5f9832

Rename state variable. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

ceolin force-pushed the pm/device-split branch from af42965 to d5f9832 Compare May 6, 2021 23:08

erwango reviewed May 7, 2021

View reviewed changes

nashif requested changes May 7, 2021

View reviewed changes

pm: device_runtime: Power device on/off in pre-kernel

64d0cfe

Devices may be initialized but started powered down for this reason is necessary to power a device on if requested even if in pre-kernel state. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

nashif approved these changes May 7, 2021

View reviewed changes

nashif merged commit e89d9d6 into zephyrproject-rtos:master May 7, 2021

This was referenced May 7, 2021

Power Management Overhaul #33265

Closed

power: device_pm_get_sync not thread-safe #31856

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pm: device: Several fixes #34781

pm: device: Several fixes #34781

ceolin commented May 3, 2021

sjg20 May 4, 2021

ceolin May 4, 2021

sjg20 May 4, 2021

ceolin May 4, 2021

pfalcon May 5, 2021

sjg20 May 4, 2021

sjg20 May 4, 2021

ceolin May 4, 2021

ceolin commented May 4, 2021

sjg20 left a comment

ceolin commented May 4, 2021

erwango May 7, 2021

erwango May 7, 2021

nashif May 7, 2021

ceolin May 7, 2021

ceolin May 7, 2021 •

edited

Loading

nashif May 7, 2021

ceolin May 7, 2021

nashif May 7, 2021

ceolin May 7, 2021

nashif May 7, 2021


		return result == target_state ? 0 : -EIO;
		return target_state == atomic_get(&dev->pm->fsm_state) ? 0 : -EIO;

pm: device: Several fixes #34781

pm: device: Several fixes #34781

Conversation

ceolin commented May 3, 2021

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

ceolin commented May 4, 2021

sjg20 left a comment

Choose a reason for hiding this comment

ceolin commented May 4, 2021

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

ceolin May 7, 2021 • edited Loading

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

ceolin May 7, 2021 •

edited

Loading