Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230621144019.3219858-1-abel.vesa@linaro.org> <20230621144019.3219858-4-abel.vesa@linaro.org>
In-Reply-To: <20230621144019.3219858-4-abel.vesa@linaro.org>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Tue, 4 Jul 2023 15:24:17 +0200
Message-ID: <CAPDyKFqczOR0GjdCpe=C5r8=RWQ9nkhMNP_Nok5_Jkw+LaSahw@mail.gmail.com>
Subject: Re: [RFC PATCH v5 3/4] PM: domains: Ignore power off request for
 enabled unused domains
To:     Abel Vesa <abel.vesa@linaro.org>
Cc:     "Rafael J . Wysocki" <rafael@kernel.org>,
        Kevin Hilman <khilman@kernel.org>,
        Len Brown <len.brown@intel.com>, Pavel Machek <pavel@ucw.cz>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Saravana Kannan <saravanak@google.com>,
        Bjorn Andersson <andersson@kernel.org>,
        linux-pm@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-arm-msm@vger.kernel.org,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, 21 Jun 2023 at 16:40, Abel Vesa <abel.vesa@linaro.org> wrote:
>
> First of, safekeep the boot state that is provided on init, then use this
> boot state to make decisions whether a power off request should be
> ignored or not. In case a domain was left enabled before boot, most
> likely such domain is needed and should not be disabled on the 'disable
> unused' late initcall, but rather needs to stay powered on until the
> consumer driver gets a chance to probe. In order to keep such domain
> powered on until the consumer handles it correctly, the domain needs to
> be registered by a provider that has a sync_state callback registered
> and said provider has state synced.
>
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
>  drivers/base/power/domain.c | 49 +++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h   |  1 +
>  2 files changed, 50 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 51b9d4eaab5e..5967ade160e2 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -654,6 +654,43 @@ static void genpd_queue_power_off_work(struct generic_pm_domain *genpd)
>         queue_work(pm_wq, &genpd->power_off_work);
>  }
>
> +/**
> + * genpd_keep_on - Tells if the domain should skip the power 'off' request
> + * @genpd: PM domain to be checked.
> + *
> + * If the domain's current state meets the following conditions:
> + *  - marked for being kept as enabled
> + *  - has a provider with a sync state callback registered
> + *  - the provider hasn't state synced yet
> + * then the power 'off' request should be skipped.
> + *
> + * This function should only be called from genpd_power_off and with
> + * the lock held.
> + */
> +static inline bool genpd_keep_on(struct generic_pm_domain *genpd)
> +{
> +       bool ret = false;
> +
> +       if (!(genpd->boot_keep_on))
> +               return false;
> +
> +       if (!genpd->has_provider)
> +               goto out;

Hmm, resetting the boot_keep_on flag based on the above condition
isn't really working, I think.

genpd_power_off() may be called before/after there is an OF provider
assigned/removed for the genpd. With the current genpd APIs
(pm_genpd_init() and of_genpd_add_provider_*()), we have at least two
separate function calls to complete the initialization of the genpd
provider(s). Theoretically, we can't know when genpd_power_off() may
be called, especially if there are child-domains being used too.

It looks to me that we should not clear the boot_keep_on flag at all
in this path. Instead, we should rather bail out and return false, to
prevent the genpd from being powered off. Although this should be fine
for most cases, we have some genpd providers, which don't use OF
providers at all (pm-s3c64xx, amdgpu_acp). To deal with these cases,
we seem to need an opt-out solution (maybe a new genpd configuration
bit) that they can set, before calling pm_genpd_init().

That said, it looks like the genpd->has_provider seems not to be
entirely protected by the genpd lock (not in this path, but in other
paths in genpd). I think we need to fix that too, in some way or the
other.

> +
> +       if (!dev_has_sync_state(genpd->provider->dev))
> +               goto out;
> +
> +       if (dev_is_drv_state_synced(genpd->provider->dev))
> +               goto out;
> +
> +       return true;
> +
> +out:
> +       genpd->boot_keep_on = false;
> +
> +       return ret;
> +}
> +
>  /**
>   * genpd_power_off - Remove power from a given PM domain.
>   * @genpd: PM domain to power down.
> @@ -682,6 +719,13 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
>         if (!genpd_status_on(genpd) || genpd->prepared_count > 0)
>                 return 0;
>
> +       /*
> +        * If the domain is enabled and unused, bail out and ignore
> +        * the 'off' request until the provider has state synced.
> +        */
> +       if (genpd_keep_on(genpd))
> +               return -EBUSY;
> +
>         /*
>          * Abort power off for the PM domain in the following situations:
>          * (1) The domain is configured as always on.
> @@ -2065,6 +2109,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>         atomic_set(&genpd->sd_count, 0);
>         genpd->status = is_off ? GENPD_STATE_OFF : GENPD_STATE_ON;
>         genpd->device_count = 0;
> +       genpd->boot_keep_on = !is_off;
>         genpd->provider = NULL;
>         genpd->has_provider = false;
>         genpd->accounting_time = ktime_get_mono_fast_ns();
> @@ -2718,6 +2763,10 @@ static void genpd_dev_pm_sync(struct device *dev)
>         if (IS_ERR(pd))
>                 return;
>
> +       genpd_lock(pd);
> +       pd->boot_keep_on = false;

This should not be needed. I think you can drop this.

> +       genpd_unlock(pd);
> +
>         genpd_queue_power_off_work(pd);
>  }
>
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index f776fb93eaa0..3eb32c4b6d4f 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -131,6 +131,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 */
> +       bool boot_keep_on;      /* Keep enabled during 'disable unused' late initcall */
>         unsigned int device_count;      /* Number of devices */
>         unsigned int suspended_count;   /* System suspend device counter */
>         unsigned int prepared_count;    /* Suspend counter of prepared devices */

Kind regards
Uffe