[PATCH] clk: Warn (and therefore taint the kernel) on clk_ignore_unused

[PATCH] clk: Warn (and therefore taint the kernel) on clk_ignore_unused

[Konrad Dybcio]

From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

If any sort of ignore_unused is in place, it means one of:

* power is going to waste
* the platform description is incomplete (missing consumer-provider
  relationships)
* the platform description is just broken

Many people will happily declare their job done when a platform
magically happens to work as they make use of bootloader-enabled
resources, which then leads to double or triple the amount of work
of another person, as they attempt to reduce the unnecessary power
drainage and/or ensure stabiility throughout a suspend-resume cycle.

Issue a good ol' warning (and taint the kernel) to make such cases
obvious and hopefully draw more attention to it. This way, it'll be
easier to avoid effectively untested code or DT description getting
merged into the kernel, or worse, going into production.

The clock subsystem plays a crucial part in this quest, as even if
the clock controllers themselves don't draw a lot of power when on
(comparatively), improper description of clock requirements has been
the #1 cause of incomplete/incorrect devicetree bindings in my
experience.

Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 drivers/clk/clk.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index cf7720b9172ff223d86227aad144e15375ddfd86..9e2e240efc31f02e4880542370ba773037b733a0 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1527,7 +1527,8 @@ static int __init clk_disable_unused(void)
 	struct clk_core *core;
 	int ret;
 
-	if (clk_ignore_unused) {
+	/* If you need ignore_unused, your platform description is broken / incomplete */
+	if (WARN_ON(clk_ignore_unused)) {
 		pr_warn("clk: Not disabling unused clocks\n");
 		return 0;
 	}

---
base-commit: df4b2bbff898227db0c14264ac7edd634e79f755
change-id: 20250201-topic-ignore_unused_warn-254a966f627a

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Re: [PATCH] clk: Warn (and therefore taint the kernel) on clk_ignore_unused

[Stephen Boyd]

Quoting Konrad Dybcio (2025-02-01 08:52:30)
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> If any sort of ignore_unused is in place, it means one of:
> 
> * power is going to waste
> * the platform description is incomplete (missing consumer-provider
>   relationships)
> * the platform description is just broken
> 
> Many people will happily declare their job done when a platform
> magically happens to work as they make use of bootloader-enabled
> resources, which then leads to double or triple the amount of work
> of another person, as they attempt to reduce the unnecessary power
> drainage and/or ensure stabiility throughout a suspend-resume cycle.
> 
> Issue a good ol' warning (and taint the kernel) to make such cases
> obvious and hopefully draw more attention to it. This way, it'll be
> easier to avoid effectively untested code or DT description getting
> merged into the kernel, or worse, going into production.
> 
> The clock subsystem plays a crucial part in this quest, as even if
> the clock controllers themselves don't draw a lot of power when on
> (comparatively), improper description of clock requirements has been
> the #1 cause of incomplete/incorrect devicetree bindings in my
> experience.

What is a user supposed to do about this warning stack? We already print
a warning. I don't see us dumping the stack when a driver is unfinished
and doesn't implement runtime PM to save power.

Re: [PATCH] clk: Warn (and therefore taint the kernel) on clk_ignore_unused

[Florian Fainelli]

On 3/3/25 14:48, Stephen Boyd wrote:
> Quoting Konrad Dybcio (2025-02-01 08:52:30)
>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>
>> If any sort of ignore_unused is in place, it means one of:
>>
>> * power is going to waste
>> * the platform description is incomplete (missing consumer-provider
>>    relationships)
>> * the platform description is just broken
>>
>> Many people will happily declare their job done when a platform
>> magically happens to work as they make use of bootloader-enabled
>> resources, which then leads to double or triple the amount of work
>> of another person, as they attempt to reduce the unnecessary power
>> drainage and/or ensure stabiility throughout a suspend-resume cycle.
>>
>> Issue a good ol' warning (and taint the kernel) to make such cases
>> obvious and hopefully draw more attention to it. This way, it'll be
>> easier to avoid effectively untested code or DT description getting
>> merged into the kernel, or worse, going into production.
>>
>> The clock subsystem plays a crucial part in this quest, as even if
>> the clock controllers themselves don't draw a lot of power when on
>> (comparatively), improper description of clock requirements has been
>> the #1 cause of incomplete/incorrect devicetree bindings in my
>> experience.
> 
> What is a user supposed to do about this warning stack? We already print
> a warning. I don't see us dumping the stack when a driver is unfinished
> and doesn't implement runtime PM to save power.
> 

Agreed, I don't think this is tremendously helpful given that it does 
not even tell you what part is incomplete, it's just a broad warning for 
the entire system.

Assuming you have a clock provided that can be used to turn clocks off, 
and you did not boot with 'clk_ignore_unused' set on the kernel command 
line, then you should discover pretty quickly which driver is not 
managing the clocks as it should no?
-- 
Florian

Re: [PATCH] clk: Warn (and therefore taint the kernel) on clk_ignore_unused

[Dmitry Baryshkov]

On Tue, 4 Mar 2025 at 00:16, Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
>
> On 3/3/25 14:48, Stephen Boyd wrote:
> > Quoting Konrad Dybcio (2025-02-01 08:52:30)
> >> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >>
> >> If any sort of ignore_unused is in place, it means one of:
> >>
> >> * power is going to waste
> >> * the platform description is incomplete (missing consumer-provider
> >>    relationships)
> >> * the platform description is just broken
> >>
> >> Many people will happily declare their job done when a platform
> >> magically happens to work as they make use of bootloader-enabled
> >> resources, which then leads to double or triple the amount of work
> >> of another person, as they attempt to reduce the unnecessary power
> >> drainage and/or ensure stabiility throughout a suspend-resume cycle.
> >>
> >> Issue a good ol' warning (and taint the kernel) to make such cases
> >> obvious and hopefully draw more attention to it. This way, it'll be
> >> easier to avoid effectively untested code or DT description getting
> >> merged into the kernel, or worse, going into production.
> >>
> >> The clock subsystem plays a crucial part in this quest, as even if
> >> the clock controllers themselves don't draw a lot of power when on
> >> (comparatively), improper description of clock requirements has been
> >> the #1 cause of incomplete/incorrect devicetree bindings in my
> >> experience.
> >
> > What is a user supposed to do about this warning stack? We already print
> > a warning. I don't see us dumping the stack when a driver is unfinished
> > and doesn't implement runtime PM to save power.
> >
>
> Agreed, I don't think this is tremendously helpful given that it does
> not even tell you what part is incomplete, it's just a broad warning for
> the entire system.
>
> Assuming you have a clock provided that can be used to turn clocks off,
> and you did not boot with 'clk_ignore_unused' set on the kernel command
> line, then you should discover pretty quickly which driver is not
> managing the clocks as it should no?

Unfortunately it's sometimes not that easy. And some developers
pretend that 'clk_ignore_unused' is a viable way to run the system.

-- 
With best wishes
Dmitry

Re: [PATCH] clk: Warn (and therefore taint the kernel) on clk_ignore_unused

[Stephen Boyd]

Quoting Dmitry Baryshkov (2025-03-03 15:17:21)
> On Tue, 4 Mar 2025 at 00:16, Florian Fainelli
> <florian.fainelli@broadcom.com> wrote:
> >
> > On 3/3/25 14:48, Stephen Boyd wrote:
> > > Quoting Konrad Dybcio (2025-02-01 08:52:30)
[...]
> > >>
> > >> The clock subsystem plays a crucial part in this quest, as even if
> > >> the clock controllers themselves don't draw a lot of power when on
> > >> (comparatively), improper description of clock requirements has been
> > >> the #1 cause of incomplete/incorrect devicetree bindings in my
> > >> experience.
> > >
> > > What is a user supposed to do about this warning stack? We already print
> > > a warning. I don't see us dumping the stack when a driver is unfinished
> > > and doesn't implement runtime PM to save power.
> > >
> >
> > Agreed, I don't think this is tremendously helpful given that it does
> > not even tell you what part is incomplete, it's just a broad warning for
> > the entire system.
> >
> > Assuming you have a clock provided that can be used to turn clocks off,
> > and you did not boot with 'clk_ignore_unused' set on the kernel command
> > line, then you should discover pretty quickly which driver is not
> > managing the clocks as it should no?
> 
> Unfortunately it's sometimes not that easy. And some developers
> pretend that 'clk_ignore_unused' is a viable way to run the system.
> 

Maybe we would be better off with a config option that removes the clk
ignore unused ability entirely. Then you can have a kernel config check
somewhere in the build process that verifies that a user can't even set
the kernel commandline to change the behavior.

Re: [PATCH] clk: Warn (and therefore taint the kernel) on clk_ignore_unused

[Konrad Dybcio]

On 3/4/25 8:34 PM, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2025-03-03 15:17:21)
>> On Tue, 4 Mar 2025 at 00:16, Florian Fainelli
>> <florian.fainelli@broadcom.com> wrote:
>>>
>>> On 3/3/25 14:48, Stephen Boyd wrote:
>>>> Quoting Konrad Dybcio (2025-02-01 08:52:30)
> [...]
>>>>>
>>>>> The clock subsystem plays a crucial part in this quest, as even if
>>>>> the clock controllers themselves don't draw a lot of power when on
>>>>> (comparatively), improper description of clock requirements has been
>>>>> the #1 cause of incomplete/incorrect devicetree bindings in my
>>>>> experience.
>>>>
>>>> What is a user supposed to do about this warning stack? We already print
>>>> a warning. I don't see us dumping the stack when a driver is unfinished
>>>> and doesn't implement runtime PM to save power.
>>>>
>>>
>>> Agreed, I don't think this is tremendously helpful given that it does
>>> not even tell you what part is incomplete, it's just a broad warning for
>>> the entire system.
>>>
>>> Assuming you have a clock provided that can be used to turn clocks off,
>>> and you did not boot with 'clk_ignore_unused' set on the kernel command
>>> line, then you should discover pretty quickly which driver is not
>>> managing the clocks as it should no?
>>
>> Unfortunately it's sometimes not that easy. And some developers
>> pretend that 'clk_ignore_unused' is a viable way to run the system.
>>
> 
> Maybe we would be better off with a config option that removes the clk
> ignore unused ability entirely. Then you can have a kernel config check
> somewhere in the build process that verifies that a user can't even set
> the kernel commandline to change the behavior.

I used WARN specifically to taint the kernel (which would in turn throw off
any reasonable CI checks). Perhaps we could add a Kconfig entry (unless
there already is one) that would do the same, and clk_ignore_unused could
be gated behind it.

But then, it would make it harder to debug production kernels with that
parameter, which could potentially come in handy too

Konrad

Re: [PATCH] clk: Warn (and therefore taint the kernel) on clk_ignore_unused

[Brian Masney]

On Thu, May 22, 2025 at 09:47:35PM +0200, Konrad Dybcio wrote:
> On 3/4/25 8:34 PM, Stephen Boyd wrote:
> > Maybe we would be better off with a config option that removes the clk
> > ignore unused ability entirely. Then you can have a kernel config check
> > somewhere in the build process that verifies that a user can't even set
> > the kernel commandline to change the behavior.
> 
> I used WARN specifically to taint the kernel (which would in turn throw off
> any reasonable CI checks). Perhaps we could add a Kconfig entry (unless
> there already is one) that would do the same, and clk_ignore_unused could
> be gated behind it.
> 
> But then, it would make it harder to debug production kernels with that
> parameter, which could potentially come in handy too

In addition to clk_ignore_unused, there's also regulator_ignore_unused
and pd_ignore_unused that should also be considered.

From a power-management perspective, a userspace tool like powertop will
warn about various things that could be improved. For example, on my
Thinkpad x13s laptop, one of the warnings given is:

    Bad           Runtime PM for PCI Device Qualcomm Technologies,
                    Inc QCNFA765 Wireless Network Adapter

I think it would be useful to add a check to powertop for the presence
of any of these three kernel parameters.

I know that power management isn't the only reason for the original
patch, and this won't cover the case to give some kind of warning where
the various core parts of the system controlled by Linux are not
described in device tree, and the system is relying on a resource setup
by the bootloader.

Brian

Re: [PATCH] clk: Warn (and therefore taint the kernel) on clk_ignore_unused

[Bjorn Andersson]

On Mon, Mar 3, 2025 at 5:17 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Tue, 4 Mar 2025 at 00:16, Florian Fainelli
> <florian.fainelli@broadcom.com> wrote:
> >
> > On 3/3/25 14:48, Stephen Boyd wrote:
> > > Quoting Konrad Dybcio (2025-02-01 08:52:30)
> > >> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
[..]
> > >
> > > What is a user supposed to do about this warning stack? We already print
> > > a warning. I don't see us dumping the stack when a driver is unfinished
> > > and doesn't implement runtime PM to save power.
> > >
> >
> > Agreed, I don't think this is tremendously helpful given that it does
> > not even tell you what part is incomplete, it's just a broad warning for
> > the entire system.
> >
> > Assuming you have a clock provided that can be used to turn clocks off,
> > and you did not boot with 'clk_ignore_unused' set on the kernel command
> > line, then you should discover pretty quickly which driver is not
> > managing the clocks as it should no?
>
> Unfortunately it's sometimes not that easy. And some developers
> pretend that 'clk_ignore_unused' is a viable way to run the system.
>

A bit late to the discussion, but I think you got that "pretend" part backwards.
Some folks pretend that you can run the Linux kernel on a platform
with clock provider or consumer drivers built as modules without
clk_ignore_unused and have a reliable outcome.

Regards,
Bjorn

Re: [PATCH] clk: Warn (and therefore taint the kernel) on clk_ignore_unused

[Bjorn Andersson]

On Mon, Mar 3, 2025 at 5:16 PM Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
> Assuming you have a clock provided that can be used to turn clocks off,
> and you did not boot with 'clk_ignore_unused' set on the kernel command
> line, then you should discover pretty quickly which driver is not
> managing the clocks as it should no?

clk_ignore_unused affects the behavior of clk_disable_unused(), which
is called at late_initcall() so if either your clock provider or
consumer shows up after this point the disabling of unused clocks will
either not happen (because the clock doesn't yet exist) or it will act
on incomplete data (because the client isn't there to reference it
yet).

Regards,
Bjorn