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

[Ulf Hansson]

On Fri, 18 Apr 2025 at 02:51, Saravana Kannan <saravanak at google.com> wrote:
>
> On Thu, Apr 17, 2025 at 7:25 AM Ulf Hansson <ulf.hansson at 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 at 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
> >