Re: [PATCH] clk: Ensure correct consumer's rate boundaries

[Chuan Liu <chuan.liu@amlogic.com>]

Hi Brian,

On 1/15/2026 9:30 AM, Brian Masney wrote:
> [ EXTERNAL EMAIL ]
> 
> Hi Chuan,
> 
> On Fri, Jan 09, 2026 at 11:24:22AM +0800, Chuan Liu via B4 Relay wrote:
>> From: Chuan Liu <chuan.liu@amlogic.com>
>>
>> If we were to have two users of the same clock, doing something like:
>>
>> clk_set_rate_range(user1, 1000, 2000);
>> clk_set_rate_range(user2, 3000, 4000);
>>
>> Even when user2's call returns -EINVAL, the min_rate and max_rate of
>> user2 are still incorrectly updated. This causes subsequent calls by
>> user1 to fail when setting the clock rate, as clk_core_get_boundaries()
>> returns corrupted boundaries (min_rate = 3000, max_rate = 2000).
>>
>> To prevent this, clk_core_check_boundaries() now rollback to the old
>> boundaries when the check fails.
>>
>> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
>> ---
>>   drivers/clk/clk.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 85d2f2481acf..0dfb16bf3f31 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -2710,13 +2710,17 @@ static int clk_set_rate_range_nolock(struct clk *clk,
>>         */
>>        rate = clamp(rate, min, max);
>>        ret = clk_core_set_rate_nolock(clk->core, rate);
>> +
>> +out:
>>        if (ret) {
>> -             /* rollback the changes */
>> +             /*
>> +              * Rollback the consumer’s old boundaries if check_boundaries or
>> +              * set_rate fails.
>> +              */
>>                clk->min_rate = old_min;
>>                clk->max_rate = old_max;
>>        }
>>
>> -out:
>>        if (clk->exclusive_count)
>>                clk_core_rate_protect(clk->core);
> 
> This looks correct to me. Just a quick question though to possibly
> simplify this further. Currently clk_set_rate_range_nolock() has the
> following code:
> 
>          /* Save the current values in case we need to rollback the change */
>          old_min = clk->min_rate;
>          old_max = clk->max_rate;
>          clk->min_rate = min;
>          clk->max_rate = max;
> 
>          if (!clk_core_check_boundaries(clk->core, min, max)) {
>                  ret = -EINVAL;
>                  goto out;
>          }
> 
> Since clk_core_check_boundaries() is a readonly operation, what do you
> think about moving clk_core_check_boundaries above the code that saves the
> previous values? That way we only need to rollback in the case where
> set_rate() fails.
> 

Perhaps it would be more appropriate to move the 
clk_core_check_boundaries() check before saving the previous values, 
like this:

	if (!clk_core_check_boundaries(clk->core, min, max)) {
		ret = -EINVAL;
		goto out;
	}

	/* Save the current values in case we need to rollback the change */
	old_min = clk->min_rate;
	old_max = clk->max_rate;
	clk->min_rate = min;
	clk->max_rate = max;

The changes in this patch are intended to avoid altering the original 
driver execution flow, while making the minimal modification to fix the 
issue where the range is incorrectly assigned.

> Brian
>