Re: [RFC PATCH v2 1/2] PM: domains: Skip disabling unused domains if provider has sync_state

[Bjorn Andersson]

On Wed, Feb 15, 2023 at 02:21:53PM +0200, Abel Vesa wrote:
> On 23-02-15 12:57:54, Ulf Hansson wrote:
> > On Fri, 27 Jan 2023 at 11:40, Abel Vesa <abel.vesa@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > >
> > > 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@xxxxxxxxxx/
> > >
> > > 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?
> 

Yes, I agree.

Regards,
Bjorn