Powering-off a genpd that was on during boot, before all of its consumer
devices have been probed, is certainly prone to problems.
Let's fix these problems by preventing these genpds from being powered-off
until ->sync_state(). Note that, this only works for OF based platform as
->sync_state() are relying on fw_devlink.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/pmdomain/core.c | 12 +++++++++++-
include/linux/pm_domain.h | 1 +
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 695d7d9e5582..a8c56f7a7ba0 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -212,6 +212,12 @@ static inline bool irq_safe_dev_in_sleep_domain(struct device *dev,
return ret;
}
+#ifdef CONFIG_PM_GENERIC_DOMAINS_OF
+static bool genpd_may_stay_on(bool on) { return on; }
+#else
+static bool genpd_may_stay_on(bool on) { return false; }
+#endif
+
static int genpd_runtime_suspend(struct device *dev);
/*
@@ -933,11 +939,12 @@ static void genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
* The domain is already in the "power off" state.
* System suspend is in progress.
* The domain is configured as always on.
+ * The domain was on at boot and still need to stay on.
* The domain has a subdomain being powered on.
*/
if (!genpd_status_on(genpd) || genpd->prepared_count > 0 ||
genpd_is_always_on(genpd) || genpd_is_rpm_always_on(genpd) ||
- atomic_read(&genpd->sd_count) > 0)
+ genpd->stay_on || atomic_read(&genpd->sd_count) > 0)
return;
/*
@@ -2374,6 +2381,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
atomic_set(&genpd->sd_count, 0);
genpd->status = is_off ? GENPD_STATE_OFF : GENPD_STATE_ON;
+ genpd->stay_on = genpd_may_stay_on(!is_off);
genpd->sync_state = GENPD_SYNC_STATE_OFF;
genpd->device_count = 0;
genpd->provider = NULL;
@@ -2640,6 +2648,7 @@ void of_genpd_sync_state(struct device *dev)
list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
if (genpd->provider == &np->fwnode) {
genpd_lock(genpd);
+ genpd->stay_on = false;
genpd_power_off(genpd, false, 0);
genpd_unlock(genpd);
}
@@ -3486,6 +3495,7 @@ static void genpd_provider_sync_state(struct device *dev)
case GENPD_SYNC_STATE_SIMPLE:
genpd_lock(genpd);
+ genpd->stay_on = false;
genpd_power_off(genpd, false, 0);
genpd_unlock(genpd);
break;
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 2185ee9e4f7c..c5358cccacad 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -193,6 +193,7 @@ struct generic_pm_domain {
unsigned int performance_state; /* Aggregated max performance state */
cpumask_var_t cpus; /* A cpumask of the attached CPUs */
bool synced_poweroff; /* A consumer needs a synced poweroff */
+ bool stay_on; /* Stay powered-on during boot. */
enum genpd_sync_state sync_state; /* How sync_state is managed. */
int (*power_off)(struct generic_pm_domain *domain);
int (*power_on)(struct generic_pm_domain *domain);
--
2.43.0On Thu, Apr 17, 2025 at 7:25 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> Powering-off a genpd that was on during boot, before all of its consumer
> devices have been probed, is certainly prone to problems.
>
> Let's fix these problems by preventing these genpds from being powered-off
> until ->sync_state(). Note that, this only works for OF based platform as
> ->sync_state() are relying on fw_devlink.
For non-OF platforms, this will still wait until late_initcall(). So,
there's at least SOME protection. We could potentially even move that
to happen after deferred probe timeout expires.
-Saravana
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> drivers/pmdomain/core.c | 12 +++++++++++-
> include/linux/pm_domain.h | 1 +
> 2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 695d7d9e5582..a8c56f7a7ba0 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -212,6 +212,12 @@ static inline bool irq_safe_dev_in_sleep_domain(struct device *dev,
> return ret;
> }
>
> +#ifdef CONFIG_PM_GENERIC_DOMAINS_OF
> +static bool genpd_may_stay_on(bool on) { return on; }
> +#else
> +static bool genpd_may_stay_on(bool on) { return false; }
> +#endif
> +
> static int genpd_runtime_suspend(struct device *dev);
>
> /*
> @@ -933,11 +939,12 @@ static void genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
> * The domain is already in the "power off" state.
> * System suspend is in progress.
> * The domain is configured as always on.
> + * The domain was on at boot and still need to stay on.
> * The domain has a subdomain being powered on.
> */
> if (!genpd_status_on(genpd) || genpd->prepared_count > 0 ||
> genpd_is_always_on(genpd) || genpd_is_rpm_always_on(genpd) ||
> - atomic_read(&genpd->sd_count) > 0)
> + genpd->stay_on || atomic_read(&genpd->sd_count) > 0)
> return;
>
> /*
> @@ -2374,6 +2381,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
> INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
> atomic_set(&genpd->sd_count, 0);
> genpd->status = is_off ? GENPD_STATE_OFF : GENPD_STATE_ON;
> + genpd->stay_on = genpd_may_stay_on(!is_off);
> genpd->sync_state = GENPD_SYNC_STATE_OFF;
> genpd->device_count = 0;
> genpd->provider = NULL;
> @@ -2640,6 +2648,7 @@ void of_genpd_sync_state(struct device *dev)
> list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
> if (genpd->provider == &np->fwnode) {
> genpd_lock(genpd);
> + genpd->stay_on = false;
> genpd_power_off(genpd, false, 0);
> genpd_unlock(genpd);
> }
> @@ -3486,6 +3495,7 @@ static void genpd_provider_sync_state(struct device *dev)
>
> case GENPD_SYNC_STATE_SIMPLE:
> genpd_lock(genpd);
> + genpd->stay_on = false;
> genpd_power_off(genpd, false, 0);
> genpd_unlock(genpd);
> break;
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 2185ee9e4f7c..c5358cccacad 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -193,6 +193,7 @@ struct generic_pm_domain {
> unsigned int performance_state; /* Aggregated max performance state */
> cpumask_var_t cpus; /* A cpumask of the attached CPUs */
> bool synced_poweroff; /* A consumer needs a synced poweroff */
> + bool stay_on; /* Stay powered-on during boot. */
> enum genpd_sync_state sync_state; /* How sync_state is managed. */
> int (*power_off)(struct generic_pm_domain *domain);
> int (*power_on)(struct generic_pm_domain *domain);
> --
> 2.43.0
>
On Fri, 18 Apr 2025 at 02:51, Saravana Kannan <saravanak@google.com> wrote:
>
> On Thu, Apr 17, 2025 at 7:25 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > Powering-off a genpd that was on during boot, before all of its consumer
> > devices have been probed, is certainly prone to problems.
> >
> > Let's fix these problems by preventing these genpds from being powered-off
> > until ->sync_state(). Note that, this only works for OF based platform as
> > ->sync_state() are relying on fw_devlink.
>
> For non-OF platforms, this will still wait until late_initcall(). So,
> there's at least SOME protection. We could potentially even move that
> to happen after deferred probe timeout expires.
If I understand correctly, you are suggesting that we could update
genpd_power_off_unused() (late_initcall_sync) to clear genpd->stay_on
for the non-OF case? That should work I think.
>
> -Saravana
Kind regards
Uffe
>
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> > drivers/pmdomain/core.c | 12 +++++++++++-
> > include/linux/pm_domain.h | 1 +
> > 2 files changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> > index 695d7d9e5582..a8c56f7a7ba0 100644
> > --- a/drivers/pmdomain/core.c
> > +++ b/drivers/pmdomain/core.c
> > @@ -212,6 +212,12 @@ static inline bool irq_safe_dev_in_sleep_domain(struct device *dev,
> > return ret;
> > }
> >
> > +#ifdef CONFIG_PM_GENERIC_DOMAINS_OF
> > +static bool genpd_may_stay_on(bool on) { return on; }
> > +#else
> > +static bool genpd_may_stay_on(bool on) { return false; }
> > +#endif
> > +
> > static int genpd_runtime_suspend(struct device *dev);
> >
> > /*
> > @@ -933,11 +939,12 @@ static void genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
> > * The domain is already in the "power off" state.
> > * System suspend is in progress.
> > * The domain is configured as always on.
> > + * The domain was on at boot and still need to stay on.
> > * The domain has a subdomain being powered on.
> > */
> > if (!genpd_status_on(genpd) || genpd->prepared_count > 0 ||
> > genpd_is_always_on(genpd) || genpd_is_rpm_always_on(genpd) ||
> > - atomic_read(&genpd->sd_count) > 0)
> > + genpd->stay_on || atomic_read(&genpd->sd_count) > 0)
> > return;
> >
> > /*
> > @@ -2374,6 +2381,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
> > INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
> > atomic_set(&genpd->sd_count, 0);
> > genpd->status = is_off ? GENPD_STATE_OFF : GENPD_STATE_ON;
> > + genpd->stay_on = genpd_may_stay_on(!is_off);
> > genpd->sync_state = GENPD_SYNC_STATE_OFF;
> > genpd->device_count = 0;
> > genpd->provider = NULL;
> > @@ -2640,6 +2648,7 @@ void of_genpd_sync_state(struct device *dev)
> > list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
> > if (genpd->provider == &np->fwnode) {
> > genpd_lock(genpd);
> > + genpd->stay_on = false;
> > genpd_power_off(genpd, false, 0);
> > genpd_unlock(genpd);
> > }
> > @@ -3486,6 +3495,7 @@ static void genpd_provider_sync_state(struct device *dev)
> >
> > case GENPD_SYNC_STATE_SIMPLE:
> > genpd_lock(genpd);
> > + genpd->stay_on = false;
> > genpd_power_off(genpd, false, 0);
> > genpd_unlock(genpd);
> > break;
> > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> > index 2185ee9e4f7c..c5358cccacad 100644
> > --- a/include/linux/pm_domain.h
> > +++ b/include/linux/pm_domain.h
> > @@ -193,6 +193,7 @@ struct generic_pm_domain {
> > unsigned int performance_state; /* Aggregated max performance state */
> > cpumask_var_t cpus; /* A cpumask of the attached CPUs */
> > bool synced_poweroff; /* A consumer needs a synced poweroff */
> > + bool stay_on; /* Stay powered-on during boot. */
> > enum genpd_sync_state sync_state; /* How sync_state is managed. */
> > int (*power_off)(struct generic_pm_domain *domain);
> > int (*power_on)(struct generic_pm_domain *domain);
> > --
> > 2.43.0
> >
© 2016 - 2025 Red Hat, Inc.