Re: [PATCH 11/11] pmdomain: core: Leave powered-on genpds on until ->sync_state()

Ulf Hansson
2025-04-23T07:47:13Z

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
> >