Skip to contentSkip to conversation

Lets not expect domains marked as `RPM_ALWAYS_ON`/`ALWAYS_ON` to be ON at boot

DISCUSSION Ajay Agarwal 2 participants 2 messages Dec 1, 2023

lore
Share & tools
lore
#0 lore raw

Hello Linux PM experts

For a given power domain, if the genpd->flags has the GENPD_FLAG_RPM_ALWAYS_ON flag set, then pm_genpd_init expects that domain to be ON:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/power/domain.c?h=v6.7-rc3#n2102

I have a usecase where the power domain is not ON by default on kernel boot, but I also want it to stay ON during RPM. But the current logic fails the pm_genpd_init for me.

IMO, the kernel should not force a PD which needs to stay ON during RPM to be ON at boot also. It is quite possible that the PD will be turned ON for the very first time by a client driver in the kernel itself.

Additionally, I think the kernel as well should not enforce this for PDs which do not want to turn OFF ever, i.e., the ones which have GENPD_FLAG_ALWAYS_ON flag set. Let the kernel turn ON these PDs when it wants and only then prevent the RPM or system suspend from turning these domains OFF.

Proposed patch in pm_genpd_init:

```
-       /* Always-on domains must be powered on at initialization. */
-       if ((genpd_is_always_on(genpd) || genpd_is_rpm_always_on(genpd)) &&
-                       !genpd_status_on(genpd)) {
-               pr_err("always-on PM domain %s is not on\n", genpd->name);
-               return -EINVAL;
-       }
-
```

Looking for your views here.

Thanks
Ajay

#1 lore raw
Quoted 7 lines · Ajay Agarwal

Hello Linux PM experts

For a given power domain, if the genpd->flags has the GENPD_FLAG_RPM_ALWAYS_ON flag set, then pm_genpd_init expects that domain to be ON:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/power/domain.c?h=v6.7-rc3#n2102

Quoted 7 lines

I have a usecase where the power domain is not ON by default on kernel boot, but I also want it to stay ON during RPM. But the current logic fails the pm_genpd_init for me.

IMO, the kernel should not force a PD which needs to stay ON during RPM to be ON at boot also. It is quite possible that the PD will be turned ON for the very first time by a client driver in the kernel itself.

Right, you have a point. However, there is a problem with allowing this.

A consumer driver for an "irq_safe" device that is attached to a "non-irq-safe" genpd, may call pm_runtime_get_sync() to turn on its device/PM domain. This wouldn't work unless the PM domain has already been powered-on by genpd.

This problem is only present when a device has multiple PM domains to attach to, as it's only in this case when genpd leaves the PM domain in it's current state (which could be powered-off). In the single PM domain case, we are always powering-on the genpd, upfront, when attaching.

The simple solution would be to always always power-on the PM domain during attach. Perhaps the upsides of doing this are greater than the downsides. For example, the consumer driver will get consistent behaviour, no matter whether its device is attached to a single or multiple PM domain(s).

What do you think?

Quoted 5 lines

Additionally, I think the kernel as well should not enforce this for PDs which do not want to turn OFF ever, i.e., the ones which have GENPD_FLAG_ALWAYS_ON flag set. Let the kernel turn ON these PDs when it wants and only then prevent the RPM or system suspend from turning these domains OFF.

No, I don't agree with this.

The whole purpose of GENPD_FLAG_ALWAYS_ON is to allow representing PM domains that are really supposed to be powered-on always. Like always on power-islands/rails.

Moreover genpd makes assumptions about these PM domains during initializations and in other paths too, which would then break.

Quoted 12 lines

Proposed patch in pm_genpd_init:
```

(quoted diff) +0 -7
-       /* Always-on domains must be powered on at initialization. */
-       if ((genpd_is_always_on(genpd) || genpd_is_rpm_always_on(genpd)) &&
-                       !genpd_status_on(genpd)) {
-               pr_err("always-on PM domain %s is not on\n", genpd->name);
-               return -EINVAL;
-       }
-

```

Quoted 4 lines

Looking for your views here.

Thanks
Ajay

Kind regards
Uffe