Re: [PATCH] clk: do not trust cached rates for disabled clocks

["Michael Walle" <mwalle@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2179 bytes --]

Hi,

On Sat Oct 4, 2025 at 12:29 AM CEST, rs wrote:
> From: Randolph Sapp <rs@ti.com>
>
> Recalculate the clock rate for unprepared clocks. This cached value can
> vary depending on the clocking architecture. On platforms with clocks
> that have shared management it's possible that:
>
>  - Previously disabled clocks have been enabled by other entities
>  - Rates calculated during clock tree initialization could have changed
>
> Signed-off-by: Randolph Sapp <rs@ti.com>
> ---
>
> I'm hoping this will start a bit of a discussion. I'm still curious why people
> would want to read the rate of an unprepared clock, but there were so many
> logged operations on my test platforms that I assumed it must have some purpose.

> Either way, I don't believe cached values should ever be trusted in this
> scenario.
>
>  drivers/clk/clk.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 85d2f2481acf..9c8b9036b6f6 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1971,8 +1971,16 @@ static void __clk_recalc_rates(struct clk_core *core, bool update_req,
>  
>  static unsigned long clk_core_get_rate_recalc(struct clk_core *core)
>  {
> -	if (core && (core->flags & CLK_GET_RATE_NOCACHE))
> -		__clk_recalc_rates(core, false, 0);
> +	if (core) {
> +		bool prepared = clk_core_is_prepared(core);
> +
> +		if (core->flags & CLK_GET_RATE_NOCACHE || !prepared) {
> +			if (!prepared)
> +				pr_debug("%s: rate requested for unprepared clock %s\n",
> +					 __func__, core->name);
> +			__clk_recalc_rates(core, false, 0);
> +		}
> +	}

I'm not sure this patch is correct. In case the clock is not
prepared, the rate is still cached in __clk_recalc_rates(). Thus,
I'd expect the following sequence to return a wrong rate:

  # assuming clock is unprepared and will return 0
  clk_get_rate()       // this will fetch the (wrong) rate and cache it
  clk_prepare_enable()
  clk_get_rate()       // this will then return the cached rate

Or do I miss something here?

-michael

>  
>  	return clk_core_get_rate_nolock(core);
>  }


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]