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

[Randolph Sapp <rs@ti.com>]

On Thu Oct 16, 2025 at 6:23 AM CDT, Michael Walle wrote:
> 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

Ah, that's a really good point. I guess since my test is only looking at
instances where people are doing a clk_get_rate (occurring outside of driver),
clk_prepare_enable, and clk_set_rate it works right now. Need to adjust that.

Am I correct in my assumption about running clk_get_rate on unprepared clocks
though? (That it shouldn't be allowed or, if it is, that the result shouldn't be
cached.)

- Randolph

>>  
>>  	return clk_core_get_rate_nolock(core);
>>  }