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

[Randolph Sapp <rs@ti.com>]

On Fri Oct 17, 2025 at 1:09 PM CDT, Randolph Sapp wrote:
> 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

Any follow up to this Michael? I wanted to be sure this was something the
subsystem should allow before I look into further workarounds.

- Randolph

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