Date:   Wed, 15 Feb 2023 14:21:53 +0200
From:   Abel Vesa <abel.vesa@linaro.org>
To:     Ulf Hansson <ulf.hansson@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>,
        Bjorn Andersson <andersson@kernel.org>,
        Andy Gross <agross@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.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>,
        Stephen Boyd <sboyd@kernel.org>
Subject: Re: [RFC PATCH v2 1/2] PM: domains: Skip disabling unused domains if
 provider has sync_state
Message-ID: <Y+zOYfSI7Zq5sll1@linaro.org>
References: <20230127104054.895129-1-abel.vesa@linaro.org>
 <CAPDyKFon35wcQ+5kx3QZb-awN_S_q8y1Sir-G+GoxkCvpN=iiA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAPDyKFon35wcQ+5kx3QZb-awN_S_q8y1Sir-G+GoxkCvpN=iiA@mail.gmail.com>
Precedence: bulk

On 23-02-15 12:57:54, Ulf Hansson wrote:
> On Fri, 27 Jan 2023 at 11:40, Abel Vesa <abel.vesa@linaro.org> wrote:
> >
> > Currently, there are cases when a domain needs to remain enabled until
> > the consumer driver probes. Sometimes such consumer drivers may be built
> > as modules. Since the genpd_power_off_unused is called too early for
> > such consumer driver modules to get a chance to probe, the domain, since
> > it is unused, will get disabled. On the other hand, the best time for
> > an unused domain to be disabled is on the provider's sync_state
> > callback. So, if the provider has registered a sync_state callback,
> > assume the unused domains for that provider will be disabled on its
> > sync_state callback. Also provide a generic sync_state callback which
> > disables all the domains unused for the provider that registers it.
> >
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > ---
> >
> > This approach has been applied for unused clocks as well.
> > With this patch merged in, all the providers that have sync_state
> > callback registered will leave the domains enabled unless the provider's
> > sync_state callback explicitly disables them. So those providers will
> > need to add the disabling part to their sync_state callback. On the
> > other hand, the platforms that have cases where domains need to remain
> > enabled (even if unused) until the consumer driver probes, will be able,
> > with this patch in, to run without the pd_ignore_unused kernel argument,
> > which seems to be the case for most Qualcomm platforms, at this moment.
> 
> My apologies for the somewhat late reply. Please see my comments below.
> 
> >
> > The v1 is here:
> > https://lore.kernel.org/all/20230126234013.3638425-1-abel.vesa@linaro.org/
> >
> > Changes since v1:
> >  * added a generic sync state callback to be registered by providers in
> >    order to disable the unused domains on their sync state. Also
> >    mentioned this in the commit message.
> >
> >  drivers/base/power/domain.c | 17 ++++++++++++++++-
> >  include/linux/pm_domain.h   |  3 +++
> >  2 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index 84662d338188..c2a5f77c01f3 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -1099,7 +1099,8 @@ static int __init genpd_power_off_unused(void)
> >         mutex_lock(&gpd_list_lock);
> >
> >         list_for_each_entry(genpd, &gpd_list, gpd_list_node)
> > -               genpd_queue_power_off_work(genpd);
> > +               if (!dev_has_sync_state(genpd->provider->dev))
> 
> Unfortunately, this doesn't really help, due to the fact that a
> genpd's ->power_off() callback may get called anyway. At power off,
> the genpd core only cares about those consumers that are currently
> attached, not those that might get attached at some point later in
> time.
> 
> In other words, it's the responsibility for each specific genpd
> provider to cope with the condition that its ->sync_state() callback
> may *not* have been called, while its ->power_off() callback is being
> called.
> 
> In these cases, the genpd provider should probably make the
> ->power_off() callback to return -EBUSY. This is what we do in
> psci_pd_power_off(), for example.
> 

Hmm, this might actually be a better idea. Bjorn, do you agree?

> > +                       genpd_queue_power_off_work(genpd);
> >
> >         mutex_unlock(&gpd_list_lock);
> >
> > @@ -1107,6 +1108,20 @@ static int __init genpd_power_off_unused(void)
> >  }
> >  late_initcall(genpd_power_off_unused);
> >
> > +void 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)
> > +               if (genpd->provider->dev == dev)
> > +                       genpd_queue_power_off_work(genpd);
> > +
> > +       mutex_unlock(&gpd_list_lock);
> > +}
> > +EXPORT_SYMBOL_GPL(genpd_power_off_unused_sync_state);
> 
> I don't think this function is needed at all.
> 
> In fact, this part of the problem that you are trying to solve should
> already be managed by the driver core, as it calls
> dev->pm_domain->sync() (which is assigned to genpd_dev_pm_sync()) , in
> really_probe(). Or isn't that taking care of the problem for you?

Hmm, I missed the genpd_dev_pm_sync scenario entirely. Yes, that is
actually what is needed, and yes, this function I added here is useless
in this case.

> 
> > +
> >  #ifdef CONFIG_PM_SLEEP
> >
> >  /**
> > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> > index f776fb93eaa0..1fd5aa500c81 100644
> > --- a/include/linux/pm_domain.h
> > +++ b/include/linux/pm_domain.h
> > @@ -351,6 +351,7 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev,
> >                                          unsigned int index);
> >  struct device *genpd_dev_pm_attach_by_name(struct device *dev,
> >                                            const char *name);
> > +void genpd_power_off_unused_sync_state(struct device *dev);
> >  #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
> >  static inline int of_genpd_add_provider_simple(struct device_node *np,
> >                                         struct generic_pm_domain *genpd)
> > @@ -419,6 +420,8 @@ struct generic_pm_domain *of_genpd_remove_last(struct device_node *np)
> >  {
> >         return ERR_PTR(-EOPNOTSUPP);
> >  }
> > +
> > +static inline genpd_power_off_unused_sync_state(struct device *dev) {}
> >  #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
> >
> >  #ifdef CONFIG_PM
> > --
> > 2.34.1
> >
> 
> Kind regards
> Uffe