linux-kernel - Re: [PATCH v2 2/4] clk: keystone: don't cache clock rate

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <DFDXXBFG13CK.385K2HM9FOWS6@kernel.org>
Date: Fri, 02 Jan 2026 08:55:13 +0100
From: "Michael Walle" <mwalle@...nel.org>
To: "Nishanth Menon" <nm@...com>
Cc: "Frank Binns" <frank.binns@...tec.com>, "Matt Coster"
 <matt.coster@...tec.com>, "Maarten Lankhorst"
 <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard" <mripard@...nel.org>,
 "Thomas Zimmermann" <tzimmermann@...e.de>, "David Airlie"
 <airlied@...il.com>, "Simona Vetter" <simona@...ll.ch>, "Rob Herring"
 <robh@...nel.org>, "Krzysztof Kozlowski" <krzk+dt@...nel.org>, "Conor
 Dooley" <conor+dt@...nel.org>, "Vignesh Raghavendra" <vigneshr@...com>,
 "Tero Kristo" <kristo@...nel.org>, "Andrew Davis" <afd@...com>, "Santosh
 Shilimkar" <ssantosh@...nel.org>, "Michael Turquette"
 <mturquette@...libre.com>, "Stephen Boyd" <sboyd@...nel.org>, "Kevin
 Hilman" <khilman@...libre.com>, "Randolph Sapp" <rs@...com>,
 <linux-clk@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
 <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2 2/4] clk: keystone: don't cache clock rate

On Tue Dec 30, 2025 at 9:12 PM CET, Nishanth Menon wrote:
> On 13:47-20251223, Michael Walle wrote:
>> The TISCI firmware will return 0 if the clock or consumer is not
>> enabled although there is a stored value in the firmware. IOW a call to
>> set rate will work but at get rate will always return 0 if the clock is
>> disabled.
>> The clk framework will try to cache the clock rate when it's requested
>> by a consumer. If the clock or consumer is not enabled at that point,
>> the cached value is 0, which is wrong. Thus, disable the cache
>> altogether.
>> 
>> Signed-off-by: Michael Walle <mwalle@...nel.org>
>> Reviewed-by: Kevin Hilman <khilman@...libre.com>
>> Reviewed-by: Randolph Sapp <rs@...com>
>> ---
>>  drivers/clk/keystone/sci-clk.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>> 
>> diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
>> index 9d5071223f4c..0a1565fdbb3b 100644
>> --- a/drivers/clk/keystone/sci-clk.c
>> +++ b/drivers/clk/keystone/sci-clk.c
>> @@ -333,6 +333,14 @@ static int _sci_clk_build(struct sci_clk_provider *provider,
>>  
>>  	init.ops = &sci_clk_ops;
>>  	init.num_parents = sci_clk->num_parents;
>> +
>> +	/*
>> +	 * A clock rate query to the SCI firmware will return 0 if either the
>> +	 * clock itself is disabled or the attached device/consumer is disabled.
>> +	 * This makes it inherently unsuitable for the caching of the clk
>> +	 * framework.
>> +	 */
>> +	init.flags = CLK_GET_RATE_NOCACHE;
>>  	sci_clk->hw.init = &init;
>>  
>>  	ret = devm_clk_hw_register(provider->dev, &sci_clk->hw);
>> -- 
>> 2.47.3
>> 
>
> Reviewed-by: Nishanth Menon <nm@...com>
>
> I wish there was a better scheme, but inherently, just like SCMI and
> other systems where power management co-processor controls clocks, there
> is no real feasible caching scheme I can think of. I wonder if Stephen
> or others have a thought on this?
>
> That said, I wonder if we need fixes tag to this? I am sure there are
> other clocks susceptible to this as well. I wonder if
> commit 3c13933c6033 ("clk: keystone: sci-clk: add support for
> dynamically probing clocks") is the appropriate tag?

>From my previous versions of this patch:

> Regarding a Fixes: tag. I didn't include one because it might have a
> slight performance impact because the firmware has to be queried
> every time now and it doesn't have been a problem for now. OTOH I've
> enabled tracing during boot and there were just a handful
> clock_{get/set}_rate() calls.

I'm still undecided if this needs a Fixes tag or not. Strictly
speaking it would need one. Although, I'm not sure it's the one
you mentioned, because the culprit is the "we return 0 if the clock
or it's consumer is disabled", which then caches the wrong value.
So it is probably the very first commit b745c0794e2f ("clk:
keystone: Add sci-clk driver support").

-michael

Download attachment "signature.asc" of type "application/pgp-signature" (298 bytes)