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

Saravana Kannan
2025-04-18T00:51:25Z

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.

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