diff mbox series

[RESEND] clk: set initial best mux parent to current parent when determining rate

Message ID 20240224-mux-v1-1-608cc704ef43@outlook.com
State New
Headers
Series [RESEND] clk: set initial best mux parent to current parent when determining rate |

Commit Message

Yang Xiwen via B4 Relay Feb. 23, 2024, 5:18 p.m. UTC 
  From: Yang Xiwen <forbidden405@outlook.com>

Originally, the initial clock rate is hardcoded to 0, this can lead to
some problem when setting a very small rate with CLK_MUX_ROUND_NEAREST.

For example, if the lowest possible rate privided by the mux is 1000Hz,
setting a rate below 500Hz will fail, because no clock can provide a
better rate than the non-existant 0. But it should succeed with 1000Hz
being set.

Setting the initial best parent to current parent could solve this bug
very well.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
This is actually a v2 of [1], but seems too simple to have a unittest.
It's tested in a mmc host driver.

[1]: https://lore.kernel.org/linux-clk/20230421-clk-v3-1-9ff79e7e7fed@outlook.com/
---
 drivers/clk/clk.c | 4 ++++
 1 file changed, 4 insertions(+)


---
base-commit: 8d3dea210042f54b952b481838c1e7dfc4ec751d
change-id: 20240215-mux-6db8b3714590

Best regards,

Comments

Stephen Boyd Feb. 29, 2024, 1:58 a.m. UTC | #1 
Quoting Yang Xiwen via B4 Relay (2024-02-23 09:18:52)
> From: Yang Xiwen <forbidden405@outlook.com>
> 
> Originally, the initial clock rate is hardcoded to 0, this can lead to
> some problem when setting a very small rate with CLK_MUX_ROUND_NEAREST.

Did you mean CLK_MUX_ROUND_CLOSEST?

> 
> For example, if the lowest possible rate privided by the mux is 1000Hz,

s/privided/provided/

> setting a rate below 500Hz will fail, because no clock can provide a
> better rate than the non-existant 0. But it should succeed with 1000Hz
> being set.
> 
> Setting the initial best parent to current parent could solve this bug
> very well.
> 
> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
> ---
> This is actually a v2 of [1], but seems too simple to have a unittest.
> It's tested in a mmc host driver.

It's not too simple for a unittest.

> 
> [1]: https://lore.kernel.org/linux-clk/20230421-clk-v3-1-9ff79e7e7fed@outlook.com/

In that thread I asked you to please Cc Maxime. Please do that.

> ---
>  drivers/clk/clk.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 2253c154a824..d98cebd7ff03 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -649,6 +649,10 @@ int clk_mux_determine_rate_flags(struct clk_hw *hw,
>  
>         /* find the parent that can provide the fastest rate <= rate */
>         num_parents = core->num_parents;
> +       if (core->parent) {
> +               best_parent = core->parent;
> +               best = clk_core_get_rate_nolock(best_parent);
> +       }

Is the problem that we're not using abs_diff()?

----8<----
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index a3bc7fb90d0f..91023345595f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -542,7 +542,7 @@ static bool mux_is_better_rate(unsigned long rate, unsigned long now,
 			   unsigned long best, unsigned long flags)
 {
 	if (flags & CLK_MUX_ROUND_CLOSEST)
-		return abs(now - rate) < abs(best - rate);
+		return abs_diff(now, rate) < abs_diff(best, rate);
 
 	return now <= rate && now > best;
 }
Yang Xiwen Feb. 29, 2024, 2:13 a.m. UTC | #2 
On 2/29/2024 9:58 AM, Stephen Boyd wrote:
> Quoting Yang Xiwen via B4 Relay (2024-02-23 09:18:52)
>> From: Yang Xiwen <forbidden405@outlook.com>
>>
>> Originally, the initial clock rate is hardcoded to 0, this can lead to
>> some problem when setting a very small rate with CLK_MUX_ROUND_NEAREST.
> 
> Did you mean CLK_MUX_ROUND_CLOSEST?

You are right :).

> 
>>
>> For example, if the lowest possible rate privided by the mux is 1000Hz,
> 
> s/privided/provided/
> 
>> setting a rate below 500Hz will fail, because no clock can provide a
>> better rate than the non-existant 0. But it should succeed with 1000Hz
>> being set.
>>
>> Setting the initial best parent to current parent could solve this bug
>> very well.
>>
>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
>> ---
>> This is actually a v2 of [1], but seems too simple to have a unittest.
>> It's tested in a mmc host driver.
> 
> It's not too simple for a unittest.
> 
>>
>> [1]: https://lore.kernel.org/linux-clk/20230421-clk-v3-1-9ff79e7e7fed@outlook.com/
> 
> In that thread I asked you to please Cc Maxime. Please do that.
> 
>> ---
>>  drivers/clk/clk.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 2253c154a824..d98cebd7ff03 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -649,6 +649,10 @@ int clk_mux_determine_rate_flags(struct clk_hw *hw,
>>  
>>         /* find the parent that can provide the fastest rate <= rate */
>>         num_parents = core->num_parents;
>> +       if (core->parent) {
>> +               best_parent = core->parent;
>> +               best = clk_core_get_rate_nolock(best_parent);
>> +       }
> 
> Is the problem that we're not using abs_diff()?


No, i think. It has nothing to do with the code here. It's because of
the initial best_parent/best_parent_rate.

> 
> ----8<----
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index a3bc7fb90d0f..91023345595f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -542,7 +542,7 @@ static bool mux_is_better_rate(unsigned long rate, unsigned long now,
>  			   unsigned long best, unsigned long flags)
>  {
>  	if (flags & CLK_MUX_ROUND_CLOSEST)
> -		return abs(now - rate) < abs(best - rate);
> +		return abs_diff(now, rate) < abs_diff(best, rate);

Without this patch, the initial `best` rate would be always 0. This is
wrong for most cases, 0Hz might (usually) be unavailable. We should use
a valid rate(i.e. current rate) initially.

>
>  	return now <= rate && now > best;
>  }
Stephen Boyd Feb. 29, 2024, 2:25 a.m. UTC | #3 
Quoting Yang Xiwen (2024-02-28 18:13:04)
> On 2/29/2024 9:58 AM, Stephen Boyd wrote:
> > Quoting Yang Xiwen via B4 Relay (2024-02-23 09:18:52)
> >> From: Yang Xiwen <forbidden405@outlook.com>
> >>
> >> Originally, the initial clock rate is hardcoded to 0, this can lead to
> >> some problem when setting a very small rate with CLK_MUX_ROUND_NEAREST.
> > 
> > Did you mean CLK_MUX_ROUND_CLOSEST?
> 
> You are right :).
> 
> > 
> >>
> >> For example, if the lowest possible rate privided by the mux is 1000Hz,
> > 
> > s/privided/provided/
> > 
> >> setting a rate below 500Hz will fail, because no clock can provide a
> >> better rate than the non-existant 0. But it should succeed with 1000Hz
> >> being set.
> >>
> >> Setting the initial best parent to current parent could solve this bug
> >> very well.
> >>
> >> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
> >> ---
> >> This is actually a v2 of [1], but seems too simple to have a unittest.
> >> It's tested in a mmc host driver.
> > 
> > It's not too simple for a unittest.
> > 
> >>
> >> [1]: https://lore.kernel.org/linux-clk/20230421-clk-v3-1-9ff79e7e7fed@outlook.com/
> > 
> > In that thread I asked you to please Cc Maxime. Please do that.
> > 
> >> ---
> >>  drivers/clk/clk.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> >> index 2253c154a824..d98cebd7ff03 100644
> >> --- a/drivers/clk/clk.c
> >> +++ b/drivers/clk/clk.c
> >> @@ -649,6 +649,10 @@ int clk_mux_determine_rate_flags(struct clk_hw *hw,
> >>  
> >>         /* find the parent that can provide the fastest rate <= rate */
> >>         num_parents = core->num_parents;
> >> +       if (core->parent) {
> >> +               best_parent = core->parent;
> >> +               best = clk_core_get_rate_nolock(best_parent);
> >> +       }
> > 
> > Is the problem that we're not using abs_diff()?
> 
> 
> No, i think. It has nothing to do with the code here. It's because of
> the initial best_parent/best_parent_rate.

Alright.

> 
> > 
> > ----8<----
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index a3bc7fb90d0f..91023345595f 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -542,7 +542,7 @@ static bool mux_is_better_rate(unsigned long rate, unsigned long now,
> >                          unsigned long best, unsigned long flags)
> >  {
> >       if (flags & CLK_MUX_ROUND_CLOSEST)
> > -             return abs(now - rate) < abs(best - rate);
> > +             return abs_diff(now, rate) < abs_diff(best, rate);
> 
> Without this patch, the initial `best` rate would be always 0. This is
> wrong for most cases, 0Hz might (usually) be unavailable. We should use
> a valid rate(i.e. current rate) initially.

Ok. But you set best to the parent rate. So why not use 'core->rate'
directly as 'best'?
Yang Xiwen Feb. 29, 2024, 2:33 a.m. UTC | #4 
On 2/29/2024 10:25 AM, Stephen Boyd wrote:
> Quoting Yang Xiwen (2024-02-28 18:13:04)
>> On 2/29/2024 9:58 AM, Stephen Boyd wrote:
>>> Quoting Yang Xiwen via B4 Relay (2024-02-23 09:18:52)
>>>> From: Yang Xiwen <forbidden405@outlook.com>
>>>>
>>>> Originally, the initial clock rate is hardcoded to 0, this can lead to
>>>> some problem when setting a very small rate with CLK_MUX_ROUND_NEAREST.
>>>
>>> Did you mean CLK_MUX_ROUND_CLOSEST?
>>
>> You are right :).
>>
>>>
>>>>
>>>> For example, if the lowest possible rate privided by the mux is 1000Hz,
>>>
>>> s/privided/provided/
>>>
>>>> setting a rate below 500Hz will fail, because no clock can provide a
>>>> better rate than the non-existant 0. But it should succeed with 1000Hz
>>>> being set.
>>>>
>>>> Setting the initial best parent to current parent could solve this bug
>>>> very well.
>>>>
>>>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
>>>> ---
>>>> This is actually a v2 of [1], but seems too simple to have a unittest.
>>>> It's tested in a mmc host driver.
>>>
>>> It's not too simple for a unittest.
>>>
>>>>
>>>> [1]: https://lore.kernel.org/linux-clk/20230421-clk-v3-1-9ff79e7e7fed@outlook.com/
>>>
>>> In that thread I asked you to please Cc Maxime. Please do that.
>>>
>>>> ---
>>>>  drivers/clk/clk.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>>>> index 2253c154a824..d98cebd7ff03 100644
>>>> --- a/drivers/clk/clk.c
>>>> +++ b/drivers/clk/clk.c
>>>> @@ -649,6 +649,10 @@ int clk_mux_determine_rate_flags(struct clk_hw *hw,
>>>>  
>>>>         /* find the parent that can provide the fastest rate <= rate */
>>>>         num_parents = core->num_parents;
>>>> +       if (core->parent) {
>>>> +               best_parent = core->parent;
>>>> +               best = clk_core_get_rate_nolock(best_parent);
>>>> +       }
>>>
>>> Is the problem that we're not using abs_diff()?
>>
>>
>> No, i think. It has nothing to do with the code here. It's because of
>> the initial best_parent/best_parent_rate.
> 
> Alright.
> 
>>
>>>
>>> ----8<----
>>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>>> index a3bc7fb90d0f..91023345595f 100644
>>> --- a/drivers/clk/clk.c
>>> +++ b/drivers/clk/clk.c
>>> @@ -542,7 +542,7 @@ static bool mux_is_better_rate(unsigned long rate, unsigned long now,
>>>                          unsigned long best, unsigned long flags)
>>>  {
>>>       if (flags & CLK_MUX_ROUND_CLOSEST)
>>> -             return abs(now - rate) < abs(best - rate);
>>> +             return abs_diff(now, rate) < abs_diff(best, rate);
>>
>> Without this patch, the initial `best` rate would be always 0. This is
>> wrong for most cases, 0Hz might (usually) be unavailable. We should use
>> a valid rate(i.e. current rate) initially.
> 
> Ok. But you set best to the parent rate. So why not use 'core->rate'
> directly as 'best'?


I can't remember exactly. I just add this piece of code and found it's
working. Is this field already filled prior to setting rate? Anyway,
your suggestion is very reasonable. Maybe dear clk maintainers can fix
it as i'm not familiar with clk core code.
Stephen Boyd March 1, 2024, 1:42 a.m. UTC | #5 
Quoting Yang Xiwen (2024-02-28 18:33:11)
> On 2/29/2024 10:25 AM, Stephen Boyd wrote:
> >>>
> >>> Is the problem that we're not using abs_diff()?
> >>
> >>
> >> No, i think. It has nothing to do with the code here. It's because of
> >> the initial best_parent/best_parent_rate.
> > 
> > Alright.

I will have to fix this as well in a different patch.

> > 
> >>
> >>>
> >>> ----8<----
> >>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> >>> index a3bc7fb90d0f..91023345595f 100644
> >>> --- a/drivers/clk/clk.c
> >>> +++ b/drivers/clk/clk.c
> >>> @@ -542,7 +542,7 @@ static bool mux_is_better_rate(unsigned long rate, unsigned long now,
> >>>                          unsigned long best, unsigned long flags)
> >>>  {
> >>>       if (flags & CLK_MUX_ROUND_CLOSEST)
> >>> -             return abs(now - rate) < abs(best - rate);
> >>> +             return abs_diff(now, rate) < abs_diff(best, rate);
> >>
> >> Without this patch, the initial `best` rate would be always 0. This is
> >> wrong for most cases, 0Hz might (usually) be unavailable. We should use
> >> a valid rate(i.e. current rate) initially.
> > 
> > Ok. But you set best to the parent rate. So why not use 'core->rate'
> > directly as 'best'?
> 
> 
> I can't remember exactly. I just add this piece of code and found it's
> working. Is this field already filled prior to setting rate? Anyway,
> your suggestion is very reasonable. Maybe dear clk maintainers can fix
> it as i'm not familiar with clk core code.

Yes the 'struct clk_rate_request' is pre-filled with many details,
including the rate of the clk and the current parent rate and parent hw
pointer. I'm pretty sure you're trying to fix this fixme from clk_test.c

static const struct clk_ops clk_dummy_single_parent_ops = {
	/*
	 * FIXME: Even though we should probably be able to use
	 * __clk_mux_determine_rate() here, if we use it and call
	 * clk_round_rate() or clk_set_rate() with a rate lower than
	 * what all the parents can provide, it will return -EINVAL.
	 *
	 * This is due to the fact that it has the undocumented
	 * behaviour to always pick up the closest rate higher than the
	 * requested rate. If we get something lower, it thus considers
	 * that it's not acceptable and will return an error.
	 *
	 * It's somewhat inconsistent and creates a weird threshold
	 * between rates above the parent rate which would be rounded to
	 * what the parent can provide, but rates below will simply
	 * return an error.
	 */
Yang Xiwen March 1, 2024, 1:58 a.m. UTC | #6 
On 3/1/2024 9:42 AM, Stephen Boyd wrote:
> Quoting Yang Xiwen (2024-02-28 18:33:11)
>> On 2/29/2024 10:25 AM, Stephen Boyd wrote:
>>>>>
>>>>> Is the problem that we're not using abs_diff()?
>>>>
>>>>
>>>> No, i think. It has nothing to do with the code here. It's because of
>>>> the initial best_parent/best_parent_rate.
>>>
>>> Alright.
> 
> I will have to fix this as well in a different patch.
> 
>>>
>>>>
>>>>>
>>>>> ----8<----
>>>>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>>>>> index a3bc7fb90d0f..91023345595f 100644
>>>>> --- a/drivers/clk/clk.c
>>>>> +++ b/drivers/clk/clk.c
>>>>> @@ -542,7 +542,7 @@ static bool mux_is_better_rate(unsigned long rate, unsigned long now,
>>>>>                          unsigned long best, unsigned long flags)
>>>>>  {
>>>>>       if (flags & CLK_MUX_ROUND_CLOSEST)
>>>>> -             return abs(now - rate) < abs(best - rate);
>>>>> +             return abs_diff(now, rate) < abs_diff(best, rate);
>>>>
>>>> Without this patch, the initial `best` rate would be always 0. This is
>>>> wrong for most cases, 0Hz might (usually) be unavailable. We should use
>>>> a valid rate(i.e. current rate) initially.
>>>
>>> Ok. But you set best to the parent rate. So why not use 'core->rate'
>>> directly as 'best'?
>>
>>
>> I can't remember exactly. I just add this piece of code and found it's
>> working. Is this field already filled prior to setting rate? Anyway,
>> your suggestion is very reasonable. Maybe dear clk maintainers can fix
>> it as i'm not familiar with clk core code.
> 
> Yes the 'struct clk_rate_request' is pre-filled with many details,
> including the rate of the clk and the current parent rate and parent hw
> pointer. I'm pretty sure you're trying to fix this fixme from clk_test.c
> 
> static const struct clk_ops clk_dummy_single_parent_ops = {
> 	/*
> 	 * FIXME: Even though we should probably be able to use
> 	 * __clk_mux_determine_rate() here, if we use it and call
> 	 * clk_round_rate() or clk_set_rate() with a rate lower than
> 	 * what all the parents can provide, it will return -EINVAL.
> 	 *
> 	 * This is due to the fact that it has the undocumented
> 	 * behaviour to always pick up the closest rate higher than the
> 	 * requested rate. If we get something lower, it thus considers
> 	 * that it's not acceptable and will return an error.
> 	 *
> 	 * It's somewhat inconsistent and creates a weird threshold
> 	 * between rates above the parent rate which would be rounded to
> 	 * what the parent can provide, but rates below will simply
> 	 * return an error.
> 	 */

If CLK_MUX_ROUND_CLOSEST is not specified, I think both setting lowest
possible rate and returning -EINVAL are okay, just as documented(It will
ONLY return a rate lower or equal to the rate requested). But if
CLK_MUX_ROUND_CLOSEST is specified, the behavior would be wrong in no doubt.

I don't know which behavior consumers would expect. Maybe some consumer
code has already been relying on this (undocumented) behavior.

This patch indeed also has an influence on clocks without
CLK_MUX_ROUND_CLOSEST. So you are right, I'll have to fix doc for
clk_mux_determine_rate too.
diff mbox series

Patch

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 2253c154a824..d98cebd7ff03 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -649,6 +649,10 @@  int clk_mux_determine_rate_flags(struct clk_hw *hw,
 
 	/* find the parent that can provide the fastest rate <= rate */
 	num_parents = core->num_parents;
+	if (core->parent) {
+		best_parent = core->parent;
+		best = clk_core_get_rate_nolock(best_parent);
+	}
 	for (i = 0; i < num_parents; i++) {
 		unsigned long parent_rate;