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

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

Hi Brian,

On 1/15/2026 9:01 PM, Brian Masney wrote:
> [ EXTERNAL EMAIL ]
> 
> Hi Chuan,
> 
> On Thu, Jan 15, 2026 at 10:37:55AM +0800, Chuan Liu wrote:
>> On 1/15/2026 9:30 AM, Brian Masney wrote:
>>> 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;
> 
> Yes, that's what I had in mind.
> 
>> 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.
> 
> It's ultimately up to Stephen what he wants to take. I personally have a
> slight preference to the approach above, however I don't have a strong
> opinion about it. I'm just calling this out to help with reviews.
> 
> The one thing that Stephen will want though is kunit tests for this
> since it changes the clk core. There's already a bunch of kunit tests in
> drivers/clk/clk_test.c. Feel free to reach out to me if you need help
> writing a new test.
> 

Thank you for the reminder. Stephen previously pointed out the need to 
improve the kunit tests, and I will take this into account in future 
submissions.

I would greatly appreciate it if you could help enhance the kunit tests 
for this particular test case.:)

> Brian
>