On Fri, 5 May 2023 at 17:02, Abel Vesa <abel.vesa@xxxxxxxxxx> wrote:
>
> By storing the status of the domain at boot, specified by the provider,
> we can decide to skip powering 'off' the domain on the late initcall,
> strictly based on the status boot being 'on' or 'unknown', and then
> assume the provider will disable it from its sync state callback.
> Also, provide a generic genpd sync state callback for those providers
> that only need that when they state synced.
If I understand correctly, this means that providers that don't use
the sync state callback, will be kept powered-on until the late
initcall, even if those could be turned off at an earlier point. Is
this really what we want?
>
> Signed-off-by: Abel Vesa <abel.vesa@xxxxxxxxxx>
> ---
> drivers/base/power/domain.c | 51 +++++++++++++++++++++++++++++++++++--
> include/linux/pm_domain.h | 5 ++++
> 2 files changed, 54 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 33a3945c023e..9cc0ce43b47b 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -125,6 +125,7 @@ static const struct genpd_lock_ops genpd_spin_ops = {
> #define genpd_unlock(p) p->lock_ops->unlock(p)
>
> #define genpd_status_on(genpd) (genpd->status == GENPD_STATE_ON)
> +#define genpd_boot_keep_on(genpd) (!(genpd->boot_status == GENPD_STATE_OFF))
This seems a bit unnecessarily complicated. Can't we just use bool in
the genpd struct, to track whether we should allow powering off or
not, based upon the boot condition.
> #define genpd_is_irq_safe(genpd) (genpd->flags & GENPD_FLAG_IRQ_SAFE)
> #define genpd_is_always_on(genpd) (genpd->flags & GENPD_FLAG_ALWAYS_ON)
> #define genpd_is_active_wakeup(genpd) (genpd->flags & GENPD_FLAG_ACTIVE_WAKEUP)
> @@ -654,6 +655,29 @@ static void genpd_queue_power_off_work(struct generic_pm_domain *genpd)
> queue_work(pm_wq, &genpd->power_off_work);
> }
>
> +/**
> + * pm_genpd_power_off_unused_sync_state - Power off all domains for provider.
> + * @dev: Provider's device.
> + *
> + * Request power off for all unused domains of the provider.
> + * This should be used exclusively as sync state callback for genpd providers.
> + */
> +void pm_genpd_power_off_unused_sync_state(struct device *dev)
> +{
> + struct generic_pm_domain *genpd;
> +
> + mutex_lock(&gpd_list_lock);
> +
> + list_for_each_entry(genpd, &gpd_list, gpd_list_node)
It looks like we need the of_genpd_mutex here as well, as it's really
the list of providers that we want to protect too.
> + if (genpd->provider->dev == dev && genpd_boot_keep_on(genpd)) {
> + genpd->boot_status = GENPD_STATE_OFF;
This needs to be done while holding the genpd's lock.
> + genpd_queue_power_off_work(genpd);
> + }
> +
> + mutex_unlock(&gpd_list_lock);
> +}
> +EXPORT_SYMBOL_GPL(pm_genpd_power_off_unused_sync_state);
> +
> /**
> * genpd_power_off - Remove power from a given PM domain.
> * @genpd: PM domain to power down.
> @@ -674,6 +698,12 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
> unsigned int not_suspended = 0;
> int ret;
>
> + /*
> + * If the domain was left enabled at boot stage,
> + * abort power off until sync state is reached.
> + */
> + if (genpd_boot_keep_on(genpd))
> + return -EBUSY;
> /*
> * Do not try to power off the domain in the following situations:
> * (1) The domain is already in the "power off" state.
> @@ -763,6 +793,12 @@ static int genpd_power_on(struct generic_pm_domain *genpd, unsigned int depth)
> struct gpd_link *link;
> int ret = 0;
>
> + /*
> + * Just in case this is the first power on request, mark the boot
> + * status of it as 'off'.
> + */
> + genpd->boot_status = GENPD_STATE_OFF;
> +
> if (genpd_status_on(genpd))
> return 0;
>
> @@ -1095,8 +1131,16 @@ static int __init genpd_power_off_unused(void)
>
> mutex_lock(&gpd_list_lock);
>
> + /*
> + * If the provider has registered a 'sync state' callback,
> + * assume that callback will power off its registered unused domains,
> + * otherwise we power them off from here.
> + */
> list_for_each_entry(genpd, &gpd_list, gpd_list_node)
> - genpd_queue_power_off_work(genpd);
> + if (!dev_has_sync_state(&genpd->dev)) {
The genpd->dev isn't really the one that is used by the genpd provider
driver, hence this doesn't work.
In the code you introduced above, you used the "genpd->provider->dev",
which is probably what we want here too, right?
> + genpd->boot_status = GENPD_STATE_OFF;
Updating this needs to be protected by the genpd's lock.
> + genpd_queue_power_off_work(genpd);
> + }
The above said - I am concerned about having to hold each genpd's lock
in this path.
I realize that we need to update the genpd->boot_status at some point,
but let me think a bit if we can do that in a slightly better way,
without holding all the locks.
>
> mutex_unlock(&gpd_list_lock);
>
> @@ -1124,6 +1168,9 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock,
> {
> struct gpd_link *link;
>
> + if (genpd_boot_keep_on(genpd))
> + return;
> +
> if (!genpd_status_on(genpd) || genpd_is_always_on(genpd))
> return;
>
> @@ -2064,7 +2111,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
> genpd->gov = gov;
> INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
> atomic_set(&genpd->sd_count, 0);
> - genpd->status = boot_status;
> + genpd->status = genpd->boot_status = boot_status;
> genpd->device_count = 0;
> genpd->provider = NULL;
> genpd->has_provider = false;
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index c545e44ee52b..86bb531a319c 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -132,6 +132,7 @@ struct generic_pm_domain {
> const char *name;
> atomic_t sd_count; /* Number of subdomains with power "on" */
> enum gpd_status status; /* Current state of the domain */
> + enum gpd_status boot_status; /* Boot state of the domain */
> unsigned int device_count; /* Number of devices */
> unsigned int suspended_count; /* System suspend device counter */
> unsigned int prepared_count; /* Suspend counter of prepared devices */
> @@ -233,6 +234,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
> struct dev_power_governor *gov,
> enum gpd_status boot_status);
> int pm_genpd_remove(struct generic_pm_domain *genpd);
> +void pm_genpd_power_off_unused_sync_state(struct device *dev);
> int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state);
> int dev_pm_genpd_add_notifier(struct device *dev, struct notifier_block *nb);
> int dev_pm_genpd_remove_notifier(struct device *dev);
> @@ -281,6 +283,9 @@ static inline int pm_genpd_remove(struct generic_pm_domain *genpd)
> return -EOPNOTSUPP;
> }
>
> +static inline void pm_genpd_power_off_unused_sync_state(struct device *dev)
> +{ }
> +
> static inline int dev_pm_genpd_set_performance_state(struct device *dev,
> unsigned int state)
> {
Kind regards
Uffe
