linux-kernel - Re: [PATCH 2/3] 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] [day] [month] [year] [list]

Message-ID: <DCX03UL17R3K.1MRUGNR4PUIDL@ti.com>
Date: Fri, 19 Sep 2025 13:50:31 -0500
From: Randolph Sapp <rs@...com>
To: Michael Walle <mwalle@...nel.org>, 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>, Nishanth
 Menon <nm@...com>,
        Vignesh Raghavendra <vigneshr@...com>,
        Tero Kristo
	<kristo@...nel.org>,
        Santosh Shilimkar <ssantosh@...nel.org>,
        Michael
 Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>
CC: Andrew Davis <afd@...com>, <dri-devel@...ts.freedesktop.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <linux-clk@...r.kernel.org>
Subject: Re: [PATCH 2/3] clk: keystone: don't cache clock rate

On Mon Sep 15, 2025 at 9:34 AM CDT, 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>
> ---
> I guess to make it work correctly with the caching of the linux
> subsystem a new flag to query the real clock rate is needed. That
> way, one could also query the default value without having to turn
> the clock and consumer on first. That can be retrofitted later and
> the driver could query the firmware capabilities.
>
> 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.
> ---
>  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 c5894fc9395e..d73858b5ca7a 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);


Thanks for looking into this Michael. I'm still convinced that it's unusual to
report 0 for a clock rate when the device is powered down. In most cases it's
not actually 0 and is actually just in bypass mode.

I was told it's a way to indicate clock status and probably won't be changing
any time soon though. Ignore the fact that we also already have a separate way
to query clock status. :)

This series looks good, but won't quite result in a functional GPU without the
following patch: https://lore.kernel.org/all/20250808232522.1296240-1-rs@ti.com/

I suppose I'll submit that again on it's own.

Reviewed-by: Randolph Sapp <rs@...com>