Date:   Fri, 3 Feb 2023 01:20:46 +0000
From:   Matthias Kaehlcke <mka@...omium.org>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc:     Abel Vesa <abel.vesa@...aro.org>,
        Bjorn Andersson <andersson@...nel.org>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Kevin Hilman <khilman@...nel.org>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andy Gross <agross@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        linux-pm@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arm-msm@...r.kernel.org, Stephen Boyd <sboyd@...nel.org>,
        Doug Anderson <dianders@...omium.org>
Subject: Re: [RFC PATCH v2 1/2] PM: domains: Skip disabling unused domains if
 provider has sync_state

Hi Dmitry,

On Thu, Feb 02, 2023 at 09:53:41PM +0200, Dmitry Baryshkov wrote:
> On 02/02/2023 20:24, Matthias Kaehlcke wrote:
> > Hi Abel,
> > 
> > On Fri, Jan 27, 2023 at 12:40:53PM +0200, Abel Vesa 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@...aro.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.
> > 
> > I recently encountered a related issue on a Qualcomm platform with a
> > v6.2-rc kernel, which includes 3a39049f88e4 ("soc: qcom: rpmhpd: Use
> > highest corner until sync_state"). The issue involves a DT node with a
> > rpmhpd, the DT node is enabled, however the corresponding device driver
> > is not enabled in the kernel. In such a scenario the sync_state callback
> > is never called, because the genpd consumer never probes. As a result
> > the Always-on subsystem (AOSS) of the SoC doesn't enter sleep mode during
> > system suspend, which results in a substantially higher power consumption
> > in S3.
> > 
> > I wonder if genpd (and some other frameworks) needs something like
> > regulator_init_complete(), which turns off unused regulators 30s after
> > system boot. That's conceptually similar to the current
> > genpd_power_off_unused(), but would provide time for modules being loaded.
> 
> I think the overall goal is to move away from ad-hoc implementations like
> clk_disable_unused/genpd_power_off_unused/regulator_init_complete towards
> the sync_state.

I generally agree with the goal of using common mechanisms whenever possible.

> So inherently one either has to provide drivers for all devices in question
> or disable unused devices in DT.

I don't think that's a great solution, it essentially hands the issue down to
the users or downstream maintainers of the kernel, who might not be aware that
there is an issue, nor know about the specifics of genpd (or interconnects and
clocks which have similar problems).

In general symptoms are probably subtle, like a (potentially substantially)
increased power consumption during system suspend. The issue might have been
introduced by an update to a newer kernel, which now includes a DT node for a
new SoC feature which wasn't supported by the 'old' kernel. It's common
practice to use the 'old' .config, at least as a starting point, which
obviously doesn't enable the new driver. That happend to me with [1] when
testing v6.1. It took me quite some time to track the 'culprit' commit down
and then some debugging to understand what's going on. Shortly after that I
ran into a related issue involving genpds when testing v6.2-rc, which again
took a non-trivial amount of time to track down (and I'm familiar with the SoC
platform and the general nature of the issue). I don't think it's reasonable
to expect every user/downstream maintainer of an impacted system to go through
this, one person at a time.

Maybe there could be a generic solution for drivers with a 'sync_state'
callback, e.g. a the driver (or framework) could have a 'sync_state_timeout'
callback (or similar), which is called by the driver framework if 'sync_state'
wasn't called (for example) 30s after the device was probed. Then the provider
can power off or throttle unclaimed resources.

m.

[1] https://lore.kernel.org/lkml/20220902043511.17130-5-quic_rjendra@quicinc.com/


> > > 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))
> > > +			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);
> > > +
> > >   #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
> > > 
> 
> -- 
> With best wishes
> Dmitry
> 

Powered by blists - more mailing lists