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

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

[Chuan Liu via B4 Relay]

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);
 

---
base-commit: 7f98ab9da046865d57c102fd3ca9669a29845f67
change-id: 20260107-fix_error_setting_clk_rate_range-d928da67af90

Best regards,
-- 
Chuan Liu <chuan.liu@amlogic.com>

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

[Brian Masney]

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.

Brian

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

[Chuan Liu]

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
> 

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

[Brian Masney]

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.

Brian

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

[Chuan Liu]

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
> 

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

[Brian Masney]

[-- Attachment #1: Type: text/plain, Size: 4882 bytes --]

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

[-- Attachment #2: 0001-clk-test-introduce-test-to-test-the-rollback-of-cons.patch --]
[-- Type: text/plain, Size: 3005 bytes --]

From 2367a5e238f0d26ae38510fb0fa2eb33a43cc046 Mon Sep 17 00:00:00 2001
From: Brian Masney <bmasney@redhat.com>
Date: Fri, 16 Jan 2026 11:21:21 -0500
Subject: [PATCH] clk: test: introduce test to test the rollback of consumer's
 rate boundaries
Content-type: text/plain

Add a kunit test to ensure that when a second consumer requests an
invalid rate range that the aggregated range is rolled back correctly,
and the first consumer is not impacted.

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/clk_test.c | 53 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index a268d7b5d4cb..afffa182559f 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -1500,6 +1500,58 @@ static void clk_range_test_multiple_disjoints_range(struct kunit *test)
 	clk_put(user1);
 }
 
+static void clk_range_test_consumer_boundaries_rollback(struct kunit *test)
+{
+	struct clk_dummy_context *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	unsigned long rate, min, max;
+	struct clk *user1, *user2;
+	int ret;
+
+	user1 = clk_hw_get_clk(hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user1);
+
+	user2 = clk_hw_get_clk(hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user2);
+
+	/* user1 sets a range */
+	ret = clk_set_rate_range(user1, 1000, 2000);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	clk_hw_get_rate_range(hw, &min, &max);
+	KUNIT_ASSERT_EQ(test, min, 1000);
+	KUNIT_ASSERT_EQ(test, max, 2000);
+
+	/* Verify user1 can set a rate within its range */
+	ret = clk_set_rate(user1, 1500);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+	rate = clk_get_rate(user1);
+	KUNIT_EXPECT_EQ(test, rate, 1500);
+
+	/* user2 attempts to set a disjoint range [3000, 4000] - should fail */
+	ret = clk_set_rate_range(user2, 3000, 4000);
+	KUNIT_EXPECT_EQ(test, ret, -EINVAL);
+
+	/*
+	 * After the failed call, user1 should still be able to set rates
+	 * within its original range. This verifies that user2's boundaries
+	 * were correctly rolled back and didn't corrupt the aggregated
+	 * boundaries.
+	 */
+
+	clk_hw_get_rate_range(hw, &min, &max);
+	KUNIT_EXPECT_EQ(test, min, 1000);
+	KUNIT_EXPECT_EQ(test, max, 2000);
+
+	ret = clk_set_rate(user1, 1800);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+	rate = clk_get_rate(user1);
+	KUNIT_EXPECT_EQ(test, rate, 1800);
+
+	clk_put(user2);
+	clk_put(user1);
+}
+
 /*
  * Test that if our clock has some boundaries and we try to round a rate
  * lower than the minimum, the returned rate will be within range.
@@ -1738,6 +1790,7 @@ static struct kunit_case clk_range_test_cases[] = {
 	KUNIT_CASE(clk_range_test_set_range),
 	KUNIT_CASE(clk_range_test_set_range_invalid),
 	KUNIT_CASE(clk_range_test_multiple_disjoints_range),
+	KUNIT_CASE(clk_range_test_consumer_boundaries_rollback),
 	KUNIT_CASE(clk_range_test_set_range_round_rate_lower),
 	KUNIT_CASE(clk_range_test_set_range_set_rate_lower),
 	KUNIT_CASE(clk_range_test_set_range_set_round_rate_consistent_lower),
-- 
2.52.0

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

[Chuan Liu]

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