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

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.

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(-)

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@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.

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.

> 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
>

Hi,

On Thu, Feb 2, 2023 at 10:24 AM Matthias Kaehlcke <mka@chromium.org> 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@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.
>
> 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.

Just for completeness, there are at least a few other similar concepts
in the kernel where the kernel needs to decide that it's going to stop
waiting for modules to show up and it just shuts off anything that's
unused. The other one that jumps to the top of my head is related to
"driver_deferred_probe_timeout". There we give 10 seconds (by default)
for userspace to load modules. After that point in time we start
returning errors instead of waiting longer. You can even see that the
default depends on whether "CONFIG_MODULES" is set.

-Doug

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@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.
> 
> 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.

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

> 
>> 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
>>

Hi,

On Thu, Feb 2, 2023 at 11:53 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> 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@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.
> >
> > 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.
>
> So inherently one either has to provide drivers for all devices in
> question or disable unused devices in DT.

Hmm. I guess I haven't been involved too much in those discussions,
but overall I thought:

1. The device tree should ideally be describing the hardware. Thus if
the hardware is there / available to use on a given board then the
device should be marked enabled.

2. Users are not actually required to enable drivers for all hardware
on their board. Things should still function OK even if a driver is
disabled. For instance, if the SoC had a crypto accelerator you'd
describe it in the device tree but it would be OK for someone to build
a kernel that didn't enable the crypto accelerator driver.

Am I mistaken? Which point did I get wrong, #1, or #2?

-Doug

On 03/02/2023 03:20, Matthias Kaehlcke wrote:
> 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@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.
>>>
>>> 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).

The goal is to move the control down to individual drivers. Previously 
we had issues with clk_disable_unused() disabling mdss/mdp clocks 
incorrectly, which frequently led to broken display output. Other 
clock/genpd/regulator drivers might have other internal dependencies. 
Thus it is not really possible to handle resource shutdown in the common 
  (framework) code.

> 
> 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.

I think it would be nice to have some way of 'sync_pending' debug 
available (compare this to debugfs/devices_deferred).

Note, we are trying to make sure that all supported drivers are enabled 
at least as modules (if possible). If we fail, please send a patch 
fixing the defconfig.

> 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.

I might be missing a point somewhere, but for me it looks like a logical 
solution. Please send a proposal.

On Fri, Feb 03, 2023 at 10:00:27PM +0200, Dmitry Baryshkov wrote:
> On 03/02/2023 03:20, Matthias Kaehlcke wrote:
> > 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@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.
> > > > 
> > > > 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).
> 
> The goal is to move the control down to individual drivers. Previously we
> had issues with clk_disable_unused() disabling mdss/mdp clocks incorrectly,
> which frequently led to broken display output. Other clock/genpd/regulator
> drivers might have other internal dependencies. Thus it is not really
> possible to handle resource shutdown in the common  (framework) code.
> 
> > 
> > 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.
> 
> I think it would be nice to have some way of 'sync_pending' debug available
> (compare this to debugfs/devices_deferred).

Most folks are probably not even aware that they have a 'sync_state' issue and
wouldn't look in debugfs, so I think this would have to be something proactive,
like a warning log that is enabled by default (possibly with the option to
disable it). Something in debugfs could be a nice complement.

> Note, we are trying to make sure that all supported drivers are enabled at
> least as modules (if possible). If we fail, please send a patch fixing the
> defconfig.

That's great, however not everybody uses the defconfig, it's just a default.

> > 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.
> 
> I might be missing a point somewhere, but for me it looks like a logical
> solution. Please send a proposal.

I started working on a patch, I'll probably send it out next week if I don't
encounter any evident major issues.

On 23-02-03 22:00:27, Dmitry Baryshkov wrote:
> On 03/02/2023 03:20, Matthias Kaehlcke wrote:
> > 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@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.
> > > > 
> > > > 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).
> 
> The goal is to move the control down to individual drivers. Previously we
> had issues with clk_disable_unused() disabling mdss/mdp clocks incorrectly,
> which frequently led to broken display output. Other clock/genpd/regulator
> drivers might have other internal dependencies. Thus it is not really
> possible to handle resource shutdown in the common  (framework) code.
> 
> > 
> > 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.
> 
> I think it would be nice to have some way of 'sync_pending' debug available
> (compare this to debugfs/devices_deferred).

There is actually a 'state_synced' sysfs interface (per device) that
either shows 0, meaning it hasn't reach sync_state yet, or the file is
not available at all, meaning it has reached sync_state.

> 
> Note, we are trying to make sure that all supported drivers are enabled at
> least as modules (if possible). If we fail, please send a patch fixing the
> defconfig.
> 
> > 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.
> 
> I might be missing a point somewhere, but for me it looks like a logical
> solution. Please send a proposal.
> 
> -- 
> With best wishes
> Dmitry
>

On 23-02-02 18:24:15, 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@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.
> 
> 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.

If I get this correctly, one of the providers is missing (doesn't matter
the reason), in which case, your kernel needs that driver, period. There
is no reason why you would expect the consumer to work without the
provider. Or, you could just remove the property in the devicetree node,
the property that makes the consumer wait for that provider. Anyway, you
should never end up with a consumer provider relationship in devicetree
without providing the provider driver.

> 
> 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.

NACK, timeouts are just another hack in this case, specially when we
have a pretty reliable mechanism like sync_state.

> 
> > 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
> >

On Mon, Feb 06, 2023 at 06:31:21PM +0200, Abel Vesa wrote:
> On 23-02-02 18:24:15, 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@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.
> > 
> > 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.
> 
> If I get this correctly, one of the providers is missing (doesn't matter
> the reason), in which case, your kernel needs that driver, period. There
> is no reason why you would expect the consumer to work without the
> provider. Or, you could just remove the property in the devicetree node,
> the property that makes the consumer wait for that provider. Anyway, you
> should never end up with a consumer provider relationship in devicetree
> without providing the provider driver.

I would agree if it was actually a provider that's missing, however it's a
'missing' consumer that prevents the sync_state() call.

> > 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.
> 
> NACK, timeouts are just another hack in this case, specially when we
> have a pretty reliable mechanism like sync_state.

It does not work properly unless all consumers are probed successfully. It
makes sense to wait some time for the consumers to probe, but not eternally,
it's perfectly valid that a driver for a (potential) consumer is not enabled.

On Mon, Feb 06, 2023 at 06:24:30PM +0200, Abel Vesa wrote:
> On 23-02-02 14:20:56, Doug Anderson wrote:
> > Hi,
> > 
> > On Thu, Feb 2, 2023 at 11:53 AM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> 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@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.
> > > >
> > > > 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.
> > >
> > > So inherently one either has to provide drivers for all devices in
> > > question or disable unused devices in DT.
> > 
> > Hmm. I guess I haven't been involved too much in those discussions,
> > but overall I thought:
> > 
> > 1. The device tree should ideally be describing the hardware. Thus if
> > the hardware is there / available to use on a given board then the
> > device should be marked enabled.
> 
> That is correct.
> 
> > 
> > 2. Users are not actually required to enable drivers for all hardware
> > on their board. Things should still function OK even if a driver is
> > disabled. For instance, if the SoC had a crypto accelerator you'd
> > describe it in the device tree but it would be OK for someone to build
> > a kernel that didn't enable the crypto accelerator driver.
> 
> Right, but sync state is relying on fw_devlinks to decide if there are
> any consumers left that still need to probe. So if one of the consumer
> devicetree nodes needs some provider, the consumer will simply not work
> without the provider. In theory at least.

It is correct that a device which actually depends on a provider won't
work without that provider, however that isn't the case here. In the
scenario above the consumer is the crypto accelerator. It doesn't probe
because it's driver is not enabled, not because it's waiting for a
provider (clk, interconnect, ...).

CC'ed Saravana

On 23-02-06 17:22:23, Matthias Kaehlcke wrote:
> On Mon, Feb 06, 2023 at 06:31:21PM +0200, Abel Vesa wrote:
> > On 23-02-02 18:24:15, 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@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.
> > > 
> > > 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.
> > 
> > If I get this correctly, one of the providers is missing (doesn't matter
> > the reason), in which case, your kernel needs that driver, period. There
> > is no reason why you would expect the consumer to work without the
> > provider. Or, you could just remove the property in the devicetree node,
> > the property that makes the consumer wait for that provider. Anyway, you
> > should never end up with a consumer provider relationship in devicetree
> > without providing the provider driver.
> 
> I would agree if it was actually a provider that's missing, however it's a
> 'missing' consumer that prevents the sync_state() call.

Oh, my bad.

Still, why would you keep the consumer node enabled in devicetree if you don't
intend to allow its driver to ever probe?

> 
> > > 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.
> > 
> > NACK, timeouts are just another hack in this case, specially when we
> > have a pretty reliable mechanism like sync_state.
> 
> It does not work properly unless all consumers are probed successfully. It
> makes sense to wait some time for the consumers to probe, but not eternally,
> it's perfectly valid that a driver for a (potential) consumer is not enabled.

Usually, if you have a consumer devicetree node that you consider it
should not probe, you should consider disabling that node in your board
dts, specially if you don't intend to provide its driver.

Again, timeouts are bad all-around. What happens if rootfs doesn't get
mounted in time? Will 30 seconds be enough for every scenario? What
happens if I want to load the driver (module) for a consumer a day after boot?

IMHO, I think even the regulator_init_complete should be switched to some sync
state approach.

On Mon, Feb 06, 2023 at 07:48:23PM +0200, Abel Vesa wrote:
> 
> CC'ed Saravana
> 
> On 23-02-06 17:22:23, Matthias Kaehlcke wrote:
> > On Mon, Feb 06, 2023 at 06:31:21PM +0200, Abel Vesa wrote:
> > > On 23-02-02 18:24:15, 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@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.
> > > > 
> > > > 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.
> > > 
> > > If I get this correctly, one of the providers is missing (doesn't matter
> > > the reason), in which case, your kernel needs that driver, period. There
> > > is no reason why you would expect the consumer to work without the
> > > provider. Or, you could just remove the property in the devicetree node,
> > > the property that makes the consumer wait for that provider. Anyway, you
> > > should never end up with a consumer provider relationship in devicetree
> > > without providing the provider driver.
> > 
> > I would agree if it was actually a provider that's missing, however it's a
> > 'missing' consumer that prevents the sync_state() call.
> 
> Oh, my bad.
> 
> Still, why would you keep the consumer node enabled in devicetree if you don't
> intend to allow its driver to ever probe?

As Doug pointed out, the device tree is supposed to describe the hardware, but
that shouldn't impose a user/admin/downstream maintainer to use every single
existing piece of hardware with the potential power implications on battery
powererd devices.

I someone uses an off the shelf board like a Raspberry Pi for a project in
which they only use a subset of the functionality, they would be forced to
use a downstream device tree if they can't just disable the drivers they
are not interested in. Supposedly we want people/companies to use upstream
kernels as much as possible, however you suggest to adapt the device tree
in a way that does not describe the hardware, which effectively forces folks
to user downstream kernels (or at least device trees).

> > > > 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.
> > > 
> > > NACK, timeouts are just another hack in this case, specially when we
> > > have a pretty reliable mechanism like sync_state.
> > 
> > It does not work properly unless all consumers are probed successfully. It
> > makes sense to wait some time for the consumers to probe, but not eternally,
> > it's perfectly valid that a driver for a (potential) consumer is not enabled.
> 
> Usually, if you have a consumer devicetree node that you consider it
> should not probe, you should consider disabling that node in your board
> dts, specially if you don't intend to provide its driver.

Nope, the device tree is supposed to describe the hardware and the hardware
doesn't change because a particular use case doesn't require/want the use of
all existing parts.

> Again, timeouts are bad all-around. What happens if rootfs doesn't get
> mounted in time? Will 30 seconds be enough for every scenario? What
> happens if I want to load the driver (module) for a consumer a day after boot?

I am not sure if I have a complete/correct picture here. My understanding is
that sync_state is above all used for handing over critical hardware from the
bootloader to the kernel. For example the CPUs may require clocks and
interconnects to run at a certain speed during boot, before they are 'probed'
and can ask the provider to run the resource (at least) at a certain speed.

I would expect that drivers on the roots aren't critical for the system to
enter the rootfs, all these should be either built-in or as modules on an
initramfs. Let's say we have an audio driver that uses a clock (provider)
and this audio driver lives as a modules on the rootfs. For some reason our
rootfs takes a long time to mount and sync_state() of the clock provider is
called, due to the timeout we introduced. The provider determines that it
hasn't received any requests for the audio clock and disables it. Now
finally the rootfs is mounted and out audio module is loaded. The audio
device probes, gets the clock and asks the provider to run it at certain
speed. The clock provider puts the clock at the requested speed and audio
works as if the sync_state timeout never happened.

Am I missing/misunderstanding anything important?

> IMHO, I think even the regulator_init_complete should be switched to some sync
> state approach.

Maybe, with a timeout :)

On Mon, Feb 6, 2023 at 9:48 AM Abel Vesa <abel.vesa@linaro.org> wrote:
>
>
> CC'ed Saravana

Thanks. Please do cc me for stuff like this from the start. I skimmed
the series and I think it's doing one of my TODO items. So, thanks for
the patch!

I'll take a closer look within a few days -- trying to get through
some existing fw_devlink stuff.

But long story short, it is the right thing to keep a supplier on
indefinitely if there's a consumer device (that's not disabled in DT)
that never gets probed. It's a pretty common scenario -- for example,
say a display backlight. The default case should be functional
correctness. And then we can add stuff that allows changing this
behavior with command line args or something else that can be done
from userspace.

+1 to what Doug said elsewhere in this thread too. I'm trying to
consolidate the "when do we give up" decision at the driver core level
independent of what framework is being used.

-Saravana

>
> On 23-02-06 17:22:23, Matthias Kaehlcke wrote:
> > On Mon, Feb 06, 2023 at 06:31:21PM +0200, Abel Vesa wrote:
> > > On 23-02-02 18:24:15, 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@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.
> > > >
> > > > 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.
> > >
> > > If I get this correctly, one of the providers is missing (doesn't matter
> > > the reason), in which case, your kernel needs that driver, period. There
> > > is no reason why you would expect the consumer to work without the
> > > provider. Or, you could just remove the property in the devicetree node,
> > > the property that makes the consumer wait for that provider. Anyway, you
> > > should never end up with a consumer provider relationship in devicetree
> > > without providing the provider driver.
> >
> > I would agree if it was actually a provider that's missing, however it's a
> > 'missing' consumer that prevents the sync_state() call.
>
> Oh, my bad.
>
> Still, why would you keep the consumer node enabled in devicetree if you don't
> intend to allow its driver to ever probe?
>
> >
> > > > 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.
> > >
> > > NACK, timeouts are just another hack in this case, specially when we
> > > have a pretty reliable mechanism like sync_state.
> >
> > It does not work properly unless all consumers are probed successfully. It
> > makes sense to wait some time for the consumers to probe, but not eternally,
> > it's perfectly valid that a driver for a (potential) consumer is not enabled.
>
> Usually, if you have a consumer devicetree node that you consider it
> should not probe, you should consider disabling that node in your board
> dts, specially if you don't intend to provide its driver.
>
> Again, timeouts are bad all-around. What happens if rootfs doesn't get
> mounted in time? Will 30 seconds be enough for every scenario? What
> happens if I want to load the driver (module) for a consumer a day after boot?
>
> IMHO, I think even the regulator_init_complete should be switched to some sync
> state approach.

Hi,

On Mon, Feb 6, 2023 at 11:33 AM Saravana Kannan <saravanak@google.com> wrote:
>
> On Mon, Feb 6, 2023 at 9:48 AM Abel Vesa <abel.vesa@linaro.org> wrote:
> >
> >
> > CC'ed Saravana
>
> Thanks. Please do cc me for stuff like this from the start. I skimmed
> the series and I think it's doing one of my TODO items. So, thanks for
> the patch!
>
> I'll take a closer look within a few days -- trying to get through
> some existing fw_devlink stuff.
>
> But long story short, it is the right thing to keep a supplier on
> indefinitely if there's a consumer device (that's not disabled in DT)
> that never gets probed. It's a pretty common scenario -- for example,
> say a display backlight. The default case should be functional
> correctness. And then we can add stuff that allows changing this
> behavior with command line args or something else that can be done
> from userspace.
>
> +1 to what Doug said elsewhere in this thread too. I'm trying to
> consolidate the "when do we give up" decision at the driver core level
> independent of what framework is being used.

I'm not really sure I agree with the above, at least not without lots
of discussion in the community. It really goes against what the kernel
has been doing for years and years in the regulator and clock
frameworks. Those frameworks both eventually give up and power down
resources that no active drivers are using. Either changing the
regulator/clock frameworks or saying that other frameworks should work
in an opposite way seems like a recipe for confusion.

Now, certainly I won't say that the way that the regulator and clock
frameworks function is perfect nor will I say that they don't cause
any problems. However, going the opposite way where resources are kept
at full power indefinitely will _also_ cause problems.

Specifically, let's look at the case you mentioned of a display
backlight. I think you're saying that if there is no backlight driver
enabled in the kernel that you'd expect the backlight to just be on at
full brightness. Would you expect this even if the firmware didn't
leave the backlight on? In any case, why do you say it's more correct?
I suppose you'd say that the screen is at least usable like this.
...except that you've broken a different feature: suspend/resume.
Without being able to turn the backlight off at suspend time the
device would drain tons of power. It could also overheat when you
stuffed it in your backpack and damage the battery or start a fire.
Even if you argue that in the case of the display backlight you're
better off, what about a keyboard backlight? It's pretty easy to use a
laptop without the keyboard backlight and if you didn't have a driver
for it you'd be in better shape leaving it off instead of leaving it
on 100% of the time, even when the device is suspended.

Overall: if a kernel isn't configured for a given driver we shouldn't
be expecting the hardware controlled by that driver to work. The best
we can hope for is that it's at least in a low power state.

In general I think that having a well-defined way to know it's time to
give up and power off anything for which a driver didn't probe needs
to be an important part of any designs here.


> -Saravana
>
> >
> > On 23-02-06 17:22:23, Matthias Kaehlcke wrote:
> > > On Mon, Feb 06, 2023 at 06:31:21PM +0200, Abel Vesa wrote:
> > > > On 23-02-02 18:24:15, 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@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.
> > > > >
> > > > > 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.
> > > >
> > > > If I get this correctly, one of the providers is missing (doesn't matter
> > > > the reason), in which case, your kernel needs that driver, period. There
> > > > is no reason why you would expect the consumer to work without the
> > > > provider. Or, you could just remove the property in the devicetree node,
> > > > the property that makes the consumer wait for that provider. Anyway, you
> > > > should never end up with a consumer provider relationship in devicetree
> > > > without providing the provider driver.
> > >
> > > I would agree if it was actually a provider that's missing, however it's a
> > > 'missing' consumer that prevents the sync_state() call.
> >
> > Oh, my bad.
> >
> > Still, why would you keep the consumer node enabled in devicetree if you don't
> > intend to allow its driver to ever probe?
> >
> > >
> > > > > 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.
> > > >
> > > > NACK, timeouts are just another hack in this case, specially when we
> > > > have a pretty reliable mechanism like sync_state.
> > >
> > > It does not work properly unless all consumers are probed successfully. It
> > > makes sense to wait some time for the consumers to probe, but not eternally,
> > > it's perfectly valid that a driver for a (potential) consumer is not enabled.
> >
> > Usually, if you have a consumer devicetree node that you consider it
> > should not probe, you should consider disabling that node in your board
> > dts, specially if you don't intend to provide its driver.
> >
> > Again, timeouts are bad all-around. What happens if rootfs doesn't get
> > mounted in time? Will 30 seconds be enough for every scenario? What
> > happens if I want to load the driver (module) for a consumer a day after boot?
> >
> > IMHO, I think even the regulator_init_complete should be switched to some sync
> > state approach.

On Mon, Feb 6, 2023 at 1:10 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Feb 6, 2023 at 11:33 AM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Mon, Feb 6, 2023 at 9:48 AM Abel Vesa <abel.vesa@linaro.org> wrote:
> > >
> > >
> > > CC'ed Saravana
> >
> > Thanks. Please do cc me for stuff like this from the start. I skimmed
> > the series and I think it's doing one of my TODO items. So, thanks for
> > the patch!
> >
> > I'll take a closer look within a few days -- trying to get through
> > some existing fw_devlink stuff.
> >
> > But long story short, it is the right thing to keep a supplier on
> > indefinitely if there's a consumer device (that's not disabled in DT)
> > that never gets probed. It's a pretty common scenario -- for example,
> > say a display backlight. The default case should be functional
> > correctness. And then we can add stuff that allows changing this
> > behavior with command line args or something else that can be done
> > from userspace.
> >
> > +1 to what Doug said elsewhere in this thread too. I'm trying to
> > consolidate the "when do we give up" decision at the driver core level
> > independent of what framework is being used.
>
> I'm not really sure I agree with the above, at least not without lots
> of discussion in the community. It really goes against what the kernel
> has been doing for years and years in the regulator and clock
> frameworks. Those frameworks both eventually give up and power down
> resources that no active drivers are using. Either changing the
> regulator/clock frameworks or saying that other frameworks should work
> in an opposite way seems like a recipe for confusion.
>
> Now, certainly I won't say that the way that the regulator and clock
> frameworks function is perfect nor will I say that they don't cause
> any problems. However, going the opposite way where resources are kept
> at full power indefinitely will _also_ cause problems.
>
> Specifically, let's look at the case you mentioned of a display
> backlight. I think you're saying that if there is no backlight driver
> enabled in the kernel that you'd expect the backlight to just be on at
> full brightness.

No, I'm not saying that.

> Would you expect this even if the firmware didn't
> leave the backlight on?

sync_state() never turns on anything that wasn't already on at boot.
So in your example, if the firmware didn't turn on the backlight, then
it'll remain off.

> In any case, why do you say it's more correct?

Because if you turn off the display, the device is unusable. In other
circumstances, it can crash a device because the firmware powered it
on left it in a "good enough" state, but we'd go turn it off and crash
the system.

> I suppose you'd say that the screen is at least usable like this.
> ...except that you've broken a different feature: suspend/resume.

If the display is off and the laptop is unusable, then we have bigger
problems than suspend/resume?

> Without being able to turn the backlight off at suspend time the
> device would drain tons of power. It could also overheat when you
> stuffed it in your backpack and damage the battery or start a fire.
> Even if you argue that in the case of the display backlight you're
> better off, what about a keyboard backlight? It's pretty easy to use a
> laptop without the keyboard backlight and if you didn't have a driver
> for it you'd be in better shape leaving it off instead of leaving it
> on 100% of the time, even when the device is suspended.

I think you are again assuming sync_state() will cause stuff to be
turned on if the firmware didn't leave it on before booting the
kernel. This is not the case.

But let's assume you had the same understanding, then I'd argue that
between the default kernel configuration crashing some systems vs
having power impact on others, I'd prefer the former. The firmware
shouldn't have left the keyboard backlight on if it cared about
suspend/resume.

> Overall: if a kernel isn't configured for a given driver we shouldn't
> be expecting the hardware controlled by that driver to work. The best
> we can hope for is that it's at least in a low power state.
>
> In general I think that having a well-defined way to know it's time to
> give up and power off anything for which a driver didn't probe needs
> to be an important part of any designs here.

Btw, the current compromise for deferred probes/optional suppliers is
"keep extending the timeout by 10 seconds as long as modules are being
loaded".

As I said in my earlier email, this is just what I think it should be
like and there's still stuff to figure out before I send out a patch
like that. For example, we could have a sysfs file to write to to
release sync_state() for a device. Then you'd just echo to that file
in your example and go about your day.

-Saravana

Hi,

On Mon, Feb 6, 2023 at 1:35 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Mon, Feb 6, 2023 at 1:10 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Mon, Feb 6, 2023 at 11:33 AM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > On Mon, Feb 6, 2023 at 9:48 AM Abel Vesa <abel.vesa@linaro.org> wrote:
> > > >
> > > >
> > > > CC'ed Saravana
> > >
> > > Thanks. Please do cc me for stuff like this from the start. I skimmed
> > > the series and I think it's doing one of my TODO items. So, thanks for
> > > the patch!
> > >
> > > I'll take a closer look within a few days -- trying to get through
> > > some existing fw_devlink stuff.
> > >
> > > But long story short, it is the right thing to keep a supplier on
> > > indefinitely if there's a consumer device (that's not disabled in DT)
> > > that never gets probed. It's a pretty common scenario -- for example,
> > > say a display backlight. The default case should be functional
> > > correctness. And then we can add stuff that allows changing this
> > > behavior with command line args or something else that can be done
> > > from userspace.
> > >
> > > +1 to what Doug said elsewhere in this thread too. I'm trying to
> > > consolidate the "when do we give up" decision at the driver core level
> > > independent of what framework is being used.
> >
> > I'm not really sure I agree with the above, at least not without lots
> > of discussion in the community. It really goes against what the kernel
> > has been doing for years and years in the regulator and clock
> > frameworks. Those frameworks both eventually give up and power down
> > resources that no active drivers are using. Either changing the
> > regulator/clock frameworks or saying that other frameworks should work
> > in an opposite way seems like a recipe for confusion.
> >
> > Now, certainly I won't say that the way that the regulator and clock
> > frameworks function is perfect nor will I say that they don't cause
> > any problems. However, going the opposite way where resources are kept
> > at full power indefinitely will _also_ cause problems.
> >
> > Specifically, let's look at the case you mentioned of a display
> > backlight. I think you're saying that if there is no backlight driver
> > enabled in the kernel that you'd expect the backlight to just be on at
> > full brightness.
>
> No, I'm not saying that.
>
> > Would you expect this even if the firmware didn't
> > leave the backlight on?
>
> sync_state() never turns on anything that wasn't already on at boot.
> So in your example, if the firmware didn't turn on the backlight, then
> it'll remain off.

As per offline discussion, part of the problems are that today this
_isn't_ true for a few Qualcomm things (like interconnect). The
interconnect frameway specifically maxes things out for early boot.


> > In any case, why do you say it's more correct?
>
> Because if you turn off the display, the device is unusable. In other
> circumstances, it can crash a device because the firmware powered it
> on left it in a "good enough" state, but we'd go turn it off and crash
> the system.
>
> > I suppose you'd say that the screen is at least usable like this.
> > ...except that you've broken a different feature: suspend/resume.
>
> If the display is off and the laptop is unusable, then we have bigger
> problems than suspend/resume?

I suspect that here we'll have to agree to disagree. IMO it's a
non-goal to expect hardware to work for which there is no driver. So
making the backlight work without a backlight driver isn't really
something we should strive for.


> > Without being able to turn the backlight off at suspend time the
> > device would drain tons of power. It could also overheat when you
> > stuffed it in your backpack and damage the battery or start a fire.
> > Even if you argue that in the case of the display backlight you're
> > better off, what about a keyboard backlight? It's pretty easy to use a
> > laptop without the keyboard backlight and if you didn't have a driver
> > for it you'd be in better shape leaving it off instead of leaving it
> > on 100% of the time, even when the device is suspended.
>
> I think you are again assuming sync_state() will cause stuff to be
> turned on if the firmware didn't leave it on before booting the
> kernel. This is not the case.
>
> But let's assume you had the same understanding, then I'd argue that
> between the default kernel configuration crashing some systems vs
> having power impact on others, I'd prefer the former. The firmware
> shouldn't have left the keyboard backlight on if it cared about
> suspend/resume.

The keylight is a bit of a contrived example, of course. ...but not
that contrived. It's entirely possible that the keyboard backlight is
controlled by a GPIO and that the default state of that GPIO at bootup
enables the backlight regulator. That would mean that the firmware
"left" the keyboard backlight on. The firmware's job is not to init
all hardware. It's to init whatever hardware was needed to boot the
kernel and then get out of the way and boot the kernel. Ideally the
kernel should assume as little about the firmware as possible except
in cases where the firmware actually needs to hand something off to
the kernel (serial console, boot splash, etc).


> > Overall: if a kernel isn't configured for a given driver we shouldn't
> > be expecting the hardware controlled by that driver to work. The best
> > we can hope for is that it's at least in a low power state.
> >
> > In general I think that having a well-defined way to know it's time to
> > give up and power off anything for which a driver didn't probe needs
> > to be an important part of any designs here.
>
> Btw, the current compromise for deferred probes/optional suppliers is
> "keep extending the timeout by 10 seconds as long as modules are being
> loaded".
>
> As I said in my earlier email, this is just what I think it should be
> like and there's still stuff to figure out before I send out a patch
> like that. For example, we could have a sysfs file to write to to
> release sync_state() for a device. Then you'd just echo to that file
> in your example and go about your day.

We don't need to get into a centi-thread here, but I'll at least say
that it's my opinion that we need some way to get the same type of
behavior that the existing regulator / clock frameworks have. That is:
if there are resources that no driver has enabled that there should be
some way to get them to shut off eventually.

-Doug

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.

> +                       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?

> +
>  #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

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

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@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?
> 

Yes, I agree.

Regards,
Bjorn

On Tue, Feb 07, 2023 at 03:45:35PM -0800, Doug Anderson wrote:
> Hi,
> 
> On Mon, Feb 6, 2023 at 1:35 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Mon, Feb 6, 2023 at 1:10 PM Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, Feb 6, 2023 at 11:33 AM Saravana Kannan <saravanak@google.com> wrote:
> > > >
> > > > On Mon, Feb 6, 2023 at 9:48 AM Abel Vesa <abel.vesa@linaro.org> wrote:
> > > > >
> > > > >
> > > > > CC'ed Saravana
> > > >
> > > > Thanks. Please do cc me for stuff like this from the start. I skimmed
> > > > the series and I think it's doing one of my TODO items. So, thanks for
> > > > the patch!
> > > >
> > > > I'll take a closer look within a few days -- trying to get through
> > > > some existing fw_devlink stuff.
> > > >
> > > > But long story short, it is the right thing to keep a supplier on
> > > > indefinitely if there's a consumer device (that's not disabled in DT)
> > > > that never gets probed. It's a pretty common scenario -- for example,
> > > > say a display backlight. The default case should be functional
> > > > correctness. And then we can add stuff that allows changing this
> > > > behavior with command line args or something else that can be done
> > > > from userspace.
> > > >
> > > > +1 to what Doug said elsewhere in this thread too. I'm trying to
> > > > consolidate the "when do we give up" decision at the driver core level
> > > > independent of what framework is being used.
> > >
> > > I'm not really sure I agree with the above, at least not without lots
> > > of discussion in the community. It really goes against what the kernel
> > > has been doing for years and years in the regulator and clock
> > > frameworks. Those frameworks both eventually give up and power down
> > > resources that no active drivers are using. Either changing the
> > > regulator/clock frameworks or saying that other frameworks should work
> > > in an opposite way seems like a recipe for confusion.
> > >
> > > Now, certainly I won't say that the way that the regulator and clock
> > > frameworks function is perfect nor will I say that they don't cause
> > > any problems. However, going the opposite way where resources are kept
> > > at full power indefinitely will _also_ cause problems.
> > >
> > > Specifically, let's look at the case you mentioned of a display
> > > backlight. I think you're saying that if there is no backlight driver
> > > enabled in the kernel that you'd expect the backlight to just be on at
> > > full brightness.
> >
> > No, I'm not saying that.
> >
> > > Would you expect this even if the firmware didn't
> > > leave the backlight on?
> >
> > sync_state() never turns on anything that wasn't already on at boot.
> > So in your example, if the firmware didn't turn on the backlight, then
> > it'll remain off.
> 
> As per offline discussion, part of the problems are that today this
> _isn't_ true for a few Qualcomm things (like interconnect). The
> interconnect frameway specifically maxes things out for early boot.
> 

The problem being solved here is that the bootloader leaves some vote at
1GB/s, as needed by hardware related to driver B.

Driver A is loaded first and votes for 1kb/s; what should the kernel do
now, without knowledge of the needs from the hardware associated with B,
or the ability to read back the bootloader's votes.

This was the behavior of the initial implementation, and the practical
implications was seen as the UART would typically come along really
early, cast a low vote on the various buses and it would take forever to
get to the probing of the drivers that actually gave us reasonable
votes.


Also consider the case where driver A probes, votes for bandwidth, does
it's initialization and then votes for 0. Without making assumptions
about the needs of B (or a potential B even), we'd turn off critical
resources - possible preventing us from ever attempting to probe B.



As such, the only safe solution is to assume that there might be a later
loaded/probed client that has a large vote and preemptively vote for
some higher bandwidth until then.

> 
> > > In any case, why do you say it's more correct?
> >
> > Because if you turn off the display, the device is unusable. In other
> > circumstances, it can crash a device because the firmware powered it
> > on left it in a "good enough" state, but we'd go turn it off and crash
> > the system.
> >
> > > I suppose you'd say that the screen is at least usable like this.
> > > ...except that you've broken a different feature: suspend/resume.
> >
> > If the display is off and the laptop is unusable, then we have bigger
> > problems than suspend/resume?
> 
> I suspect that here we'll have to agree to disagree. IMO it's a
> non-goal to expect hardware to work for which there is no driver. So
> making the backlight work without a backlight driver isn't really
> something we should strive for.
> 

Without trying to make you agree ;)

How can you differentiate between "the driver wasn't built" and "the
driver isn't yet available"?

Consider the case where I boot my laptop, I have some set of builtin
drivers, some set of drivers in the ramdisk and some set of drivers in
the root filesystem.

In the event that something goes wrong mounting the rootfs, I will now
be in the ramdisk console. Given the current timer-based disabling of
regulators, I have ~25 seconds to solve my problem before the backlight
goes blank.


Obviously this isn't a typical scenario in a consumer device, but it
seems conceivable that your ramdisk would run fsck for some amount of
time before mounting the rootfs and picking up the last tier of drivers.

Regards,
Bjorn

On Mon, Feb 20, 2023 at 09:15:50AM -0800, Bjorn Andersson wrote:
> On Tue, Feb 07, 2023 at 03:45:35PM -0800, Doug Anderson wrote:
> > Hi,
> > 
> > On Mon, Feb 6, 2023 at 1:35 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > On Mon, Feb 6, 2023 at 1:10 PM Doug Anderson <dianders@chromium.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Mon, Feb 6, 2023 at 11:33 AM Saravana Kannan <saravanak@google.com> wrote:
> > > > >
> > > > > On Mon, Feb 6, 2023 at 9:48 AM Abel Vesa <abel.vesa@linaro.org> wrote:
> > > > > >
> > > > > >
> > > > > > CC'ed Saravana
> > > > >
> > > > > Thanks. Please do cc me for stuff like this from the start. I skimmed
> > > > > the series and I think it's doing one of my TODO items. So, thanks for
> > > > > the patch!
> > > > >
> > > > > I'll take a closer look within a few days -- trying to get through
> > > > > some existing fw_devlink stuff.
> > > > >
> > > > > But long story short, it is the right thing to keep a supplier on
> > > > > indefinitely if there's a consumer device (that's not disabled in DT)
> > > > > that never gets probed. It's a pretty common scenario -- for example,
> > > > > say a display backlight. The default case should be functional
> > > > > correctness. And then we can add stuff that allows changing this
> > > > > behavior with command line args or something else that can be done
> > > > > from userspace.
> > > > >
> > > > > +1 to what Doug said elsewhere in this thread too. I'm trying to
> > > > > consolidate the "when do we give up" decision at the driver core level
> > > > > independent of what framework is being used.
> > > >
> > > > I'm not really sure I agree with the above, at least not without lots
> > > > of discussion in the community. It really goes against what the kernel
> > > > has been doing for years and years in the regulator and clock
> > > > frameworks. Those frameworks both eventually give up and power down
> > > > resources that no active drivers are using. Either changing the
> > > > regulator/clock frameworks or saying that other frameworks should work
> > > > in an opposite way seems like a recipe for confusion.
> > > >
> > > > Now, certainly I won't say that the way that the regulator and clock
> > > > frameworks function is perfect nor will I say that they don't cause
> > > > any problems. However, going the opposite way where resources are kept
> > > > at full power indefinitely will _also_ cause problems.
> > > >
> > > > Specifically, let's look at the case you mentioned of a display
> > > > backlight. I think you're saying that if there is no backlight driver
> > > > enabled in the kernel that you'd expect the backlight to just be on at
> > > > full brightness.
> > >
> > > No, I'm not saying that.
> > >
> > > > Would you expect this even if the firmware didn't
> > > > leave the backlight on?
> > >
> > > sync_state() never turns on anything that wasn't already on at boot.
> > > So in your example, if the firmware didn't turn on the backlight, then
> > > it'll remain off.
> > 
> > As per offline discussion, part of the problems are that today this
> > _isn't_ true for a few Qualcomm things (like interconnect). The
> > interconnect frameway specifically maxes things out for early boot.
> > 
> 
> The problem being solved here is that the bootloader leaves some vote at
> 1GB/s, as needed by hardware related to driver B.
> 
> Driver A is loaded first and votes for 1kb/s; what should the kernel do
> now, without knowledge of the needs from the hardware associated with B,
> or the ability to read back the bootloader's votes.
> 
> This was the behavior of the initial implementation, and the practical
> implications was seen as the UART would typically come along really
> early, cast a low vote on the various buses and it would take forever to
> get to the probing of the drivers that actually gave us reasonable
> votes.

I generally understand this problem and agree that it makes sense to bump
the resources *initially*. Doug and I primarily question the 'wait forever'
part of it.

> Also consider the case where driver A probes, votes for bandwidth, does
> it's initialization and then votes for 0. Without making assumptions
> about the needs of B (or a potential B even), we'd turn off critical
> resources - possible preventing us from ever attempting to probe B.

For the most critical devices that are probed during early boot this
would still work if the resources are initially bumped and then turned
off after some timeout.

Could you provide an example for some other type of device that is/would
be probed later? Except for auto-probing buses like USB or PCI the device
should probe regardless of the resources being enabled and then vote
during probe for the required bandwidth, voltage, etc., which should put
the resources into the required state. Am I missing something here?

> As such, the only safe solution is to assume that there might be a later
> loaded/probed client that has a large vote and preemptively vote for
> some higher bandwidth until then.

> > > > In any case, why do you say it's more correct?
> > >
> > > Because if you turn off the display, the device is unusable. In other
> > > circumstances, it can crash a device because the firmware powered it
> > > on left it in a "good enough" state, but we'd go turn it off and crash
> > > the system.
> > >
> > > > I suppose you'd say that the screen is at least usable like this.
> > > > ...except that you've broken a different feature: suspend/resume.
> > >
> > > If the display is off and the laptop is unusable, then we have bigger
> > > problems than suspend/resume?
> > 
> > I suspect that here we'll have to agree to disagree. IMO it's a
> > non-goal to expect hardware to work for which there is no driver. So
> > making the backlight work without a backlight driver isn't really
> > something we should strive for.
> > 
> 
> Without trying to make you agree ;)
> 
> How can you differentiate between "the driver wasn't built" and "the
> driver isn't yet available"?

Unfortunately you can't AFAIK.

> Consider the case where I boot my laptop, I have some set of builtin
> drivers, some set of drivers in the ramdisk and some set of drivers in
> the root filesystem.
> 
> In the event that something goes wrong mounting the rootfs, I will now
> be in the ramdisk console. Given the current timer-based disabling of
> regulators, I have ~25 seconds to solve my problem before the backlight
> goes blank.
> 
> 
> Obviously this isn't a typical scenario in a consumer device, but it
> seems conceivable that your ramdisk would run fsck for some amount of
> time before mounting the rootfs and picking up the last tier of drivers.

If the laptop is running a kernel that is tailored for this device I'd
say the most practial solution would be to either build the backlight
driver into the kernel or have it on the ramdisk as a module.

However the laptop might be running a distribution like Debian or Red Hat
with (I assume) a single kernel+ramdisk for all systems of a given
architecture (e.g. aarch64). In that case it might not be desirable to
have all possible backlight drivers in the kernel image or ramdisk. For
this kind of system 'wait forever' might be a suitable solution.

I have the impression we might want both options, a timeout after which
resources are turned off unless they have been voted for, and 'wait
forever', with a Kconfig option to select the desired behavior.

For most tailored systems the timout is probably a more suitable solution.
The maintainer of the kernel/system can decide to not enable certain
drivers because they aren't needed and include essential drivers into
the kernel image or ramdisk. The timeout ensures that the system doesn't
burn extra power for reasons that aren't evident for the maintainer (who
might not even be aware of the whole sync_state story).

On the other hand general purpose distributions might want to wait
forever, since they have to support a wide range of hardware and enable
most available drivers anyway.

If we end up with such an option I think the timeout should be the
default. Why? There are hundreds of maintainers of tailored kernels
who might run into hard to detect/debug power issues with 'wait
forever'. On the other hand there is a limited number of general purpose
distributions, with kernel teams that probably already know about
'sync_state'. They only have to enable 'wait forever' once and then
carry it forward to future versions.