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

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

Hi Brian,
Thank you very much for your help. I will wait and collect feedback from 
Stephen and others, and include your kunit test case together in V2.

On 1/17/2026 12:44 AM, Brian Masney wrote:
> [ EXTERNAL EMAIL ]
> 
> Hi Chuan,
> i
> On Fri, Jan 16, 2026 at 05:32:39PM +0800, Chuan Liu wrote:
>> On 1/15/2026 9:01 PM, Brian Masney wrote:
>>> 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.:)
> 
> I attached a patch that adds a kunit test that you can apply with 'git
> am'. Try it with, and without your patch to see how it operates with
> and without your fix. If you like it, then feel free to include it in
> the next version of this patch series.
> 
> Brian