clk_get_rate for disabled clks

clk_get_rate for disabled clks

[Uwe Kleine-König]

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

Hello,

the documentation about clk_get_rate in include/linux/clk.h reads:

	[...] obtain the current clock rate (in Hz) for a clock source.
	This is only valid once the clock source has been enabled.

The second part isn't enforced and (I think) there are many consumers
who don't ensure the clock being enabled. (I just stumbled over
rockchip_pwm_get_state().)

I wonder if it would be sensible to add a development check to
clk_get_rate, something like:

	if (WARN(!clk->usecount, "Trying to get rate of a disabled clk"))
		return 0;

(or something less consequent like not returning 0 but the value it also
returns today). Or is the statement in the comment wrong today and it
can be assumed that clk_get_rate() also works for a disabled clock (and
yields the rate it would have were it enabled)?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: clk_get_rate for disabled clks

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1427 bytes --]

On 12/21/20 10:27 AM, Uwe Kleine-König wrote:
> the documentation about clk_get_rate in include/linux/clk.h reads:
> 
> 	[...] obtain the current clock rate (in Hz) for a clock source.
> 	This is only valid once the clock source has been enabled.
> 
> The second part isn't enforced and (I think) there are many consumers
> who don't ensure the clock being enabled. (I just stumbled over
> rockchip_pwm_get_state().)
> 
> I wonder if it would be sensible to add a development check to
> clk_get_rate, something like:
> 
> 	if (WARN(!clk->usecount, "Trying to get rate of a disabled clk"))
> 		return 0;
> 
> (or something less consequent like not returning 0 but the value it also
> returns today). Or is the statement in the comment wrong today and it
> can be assumed that clk_get_rate() also works for a disabled clock (and
> yields the rate it would have were it enabled)?

What about the problem, that if a different clock (which has a common parent
with the clock in question) is enabled and sets the rate, so that our clock's
rate would changes? Does it make a difference if our clock is enabled or not?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH] clk: Warn when clk_get_rate is called for a disabled clk

[Uwe Kleine-König]

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

<linux/clk.h> claims that clk_get_rate() must only be called for enabled
clocks. So emit a warning if a consumer calls this function without
ensuring the clock being on.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

I didn't hear back, so went on to create a proper patch now.

On Mon, Dec 21, 2020 at 10:27:13AM +0100, Uwe Kleine-König wrote:
> the documentation about clk_get_rate in include/linux/clk.h reads:
> 
> 	[...] obtain the current clock rate (in Hz) for a clock source.
> 	This is only valid once the clock source has been enabled.
> 
> The second part isn't enforced and (I think) there are many consumers
> who don't ensure the clock being enabled. (I just stumbled over
> rockchip_pwm_get_state().)
> 
> I wonder if it would be sensible to add a development check to
> clk_get_rate, something like:
> 
> 	if (WARN(!clk->usecount, "Trying to get rate of a disabled clk"))
> 		return 0;
> 
> (or something less consequent like not returning 0 but the value it also
> returns today).

This conservative approach is what I implemented now, and I only emit 1
warning to not overflow systems that trigger that problem several times.

I'm unsure if I really must take the enable_lock, but it is not
completely wrong.

 drivers/clk/clk.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 8c1d04db990d..7558753883dc 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1614,6 +1614,16 @@ static void __clk_recalc_rates(struct clk_core *core, unsigned long msg)
 
 static unsigned long clk_core_get_rate_recalc(struct clk_core *core)
 {
+	unsigned long flags;
+	unsigned int enable_count;
+
+	flags = clk_enable_lock();
+	enable_count = core->enable_count;
+	clk_enable_unlock(flags);
+
+	WARN_ONCE(enable_count == 0,
+		  "A clock must be enabled to determine its rate\n");
+
 	if (core && (core->flags & CLK_GET_RATE_NOCACHE))
 		__clk_recalc_rates(core, 0);
 
-- 
2.29.2

> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |



-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] clk: Warn when clk_get_rate is called for a disabled clk

[Stephen Boyd]

Quoting Uwe (2021-01-13 00:30:42)
> <linux/clk.h> claims that clk_get_rate() must only be called for enabled
> clocks. So emit a warning if a consumer calls this function without
> ensuring the clock being on.
> 
> ---
> Hello,
> 
> I didn't hear back, so went on to create a proper patch now.
> 
> On Mon, Dec 21, 2020 at 10:27:13AM +0100, Uwe wrote:
> > the documentation about clk_get_rate in include/linux/clk.h reads:
> > 
> >       [...] obtain the current clock rate (in Hz) for a clock source.
> >       This is only valid once the clock source has been enabled.
> > 
> > The second part isn't enforced and (I think) there are many consumers
> > who don't ensure the clock being enabled. (I just stumbled over
> > rockchip_pwm_get_state().)
> > 
> > I wonder if it would be sensible to add a development check to
> > clk_get_rate, something like:
> > 
> >       if (WARN(!clk->usecount, "Trying to get rate of a disabled clk"))
> >               return 0;
> > 
> > (or something less consequent like not returning 0 but the value it also
> > returns today).
> 
> This conservative approach is what I implemented now, and I only emit 1
> warning to not overflow systems that trigger that problem several times.
> 
> I'm unsure if I really must take the enable_lock, but it is not
> completely wrong.

I'm not totally opposed to this but I'm curious if you have a plan to
fix various drivers that are violating the documentation? I'm more
inclined to leave the documentation as is, which indicates that it isn't
promised to work but sometimes does work. Given that we've supported it
for quite some time I don't see the downside to keeping supporting it
vs. the many downsides of implementing a check like this and having to
fix various places that now WARN_ON() (and if you have many on some
particular device then you'll have to work through them one by one?)

What problem are you trying to address? Is there some issue you've
encountered in the kernel that would have been fixed by having this
warning? If so, please point to it in the commit text! Then we can all
see the value of this patch. Right now I don't see much value.

> 
>  drivers/clk/clk.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 8c1d04db990d..7558753883dc 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1614,6 +1614,16 @@ static void __clk_recalc_rates(struct clk_core *core, unsigned long msg)
>  
>  static unsigned long clk_core_get_rate_recalc(struct clk_core *core)
>  {
> +       unsigned long flags;
> +       unsigned int enable_count;
> +
> +       flags = clk_enable_lock();
> +       enable_count = core->enable_count;
> +       clk_enable_unlock(flags);
> +
> +       WARN_ONCE(enable_count == 0,
> +                 "A clock must be enabled to determine its rate\n");
> +
>         if (core && (core->flags & CLK_GET_RATE_NOCACHE))
>                 __clk_recalc_rates(core, 0);
>

Re: [PATCH] clk: Warn when clk_get_rate is called for a disabled clk

[Uwe Kleine-König]

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

Hello Stephen,

On Wed, Feb 10, 2021 at 07:14:09PM -0800, Stephen Boyd wrote:
> Quoting Uwe (2021-01-13 00:30:42)
> > <linux/clk.h> claims that clk_get_rate() must only be called for enabled
> > clocks. So emit a warning if a consumer calls this function without
> > ensuring the clock being on.
> > 
> > ---
> > Hello,
> > 
> > I didn't hear back, so went on to create a proper patch now.
> > 
> > On Mon, Dec 21, 2020 at 10:27:13AM +0100, Uwe wrote:
> > > the documentation about clk_get_rate in include/linux/clk.h reads:
> > > 
> > >       [...] obtain the current clock rate (in Hz) for a clock source.
> > >       This is only valid once the clock source has been enabled.
> > > 
> > > The second part isn't enforced and (I think) there are many consumers
> > > who don't ensure the clock being enabled. (I just stumbled over
> > > rockchip_pwm_get_state().)
> > > 
> > > I wonder if it would be sensible to add a development check to
> > > clk_get_rate, something like:
> > > 
> > >       if (WARN(!clk->usecount, "Trying to get rate of a disabled clk"))
> > >               return 0;
> > > 
> > > (or something less consequent like not returning 0 but the value it also
> > > returns today).
> > 
> > This conservative approach is what I implemented now, and I only emit 1
> > warning to not overflow systems that trigger that problem several times.
> > 
> > I'm unsure if I really must take the enable_lock, but it is not
> > completely wrong.
> 
> I'm not totally opposed to this but I'm curious if you have a plan to
> fix various drivers that are violating the documentation? I'm more
> inclined to leave the documentation as is, which indicates that it isn't
> promised to work but sometimes does work. Given that we've supported it
> for quite some time I don't see the downside to keeping supporting it
> vs. the many downsides of implementing a check like this and having to
> fix various places that now WARN_ON() (and if you have many on some
> particular device then you'll have to work through them one by one?)

The WARN_ONCE vs. WARN is a trade off. Picking WARN has the downside to
(maybe) overflow your kernel log hiding the things you currently care
for. If you want to address the rounding "problems" making this a WARN
might be sensible.

> What problem are you trying to address? Is there some issue you've
> encountered in the kernel that would have been fixed by having this
> warning?

The warning obviously doesn't fix anything. My eventual goal is to
answer the question in the initial mail in this thread. The motivating
situation is: Should I continue to tell patch authors who use
clk_get_rate() that they have to ensure that the given clk must be
enabled as the documentation suggests? And if yes: Can we please check
this automatically (e.g. with my patch or by returning 0 for a disabled
clk) and don't rely on human review to adhere to this rule.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] clk: Warn when clk_get_rate is called for a disabled clk

[Stephen Boyd]

Quoting Uwe (2021-02-11 00:05:09)
> Hello Stephen,
> 
> On Wed, Feb 10, 2021 at 07:14:09PM -0800, Stephen Boyd wrote:
> 
> > What problem are you trying to address? Is there some issue you've
> > encountered in the kernel that would have been fixed by having this
> > warning?
> 
> The warning obviously doesn't fix anything. My eventual goal is to
> answer the question in the initial mail in this thread. The motivating
> situation is: Should I continue to tell patch authors who use
> clk_get_rate() that they have to ensure that the given clk must be
> enabled as the documentation suggests? And if yes: Can we please check
> this automatically (e.g. with my patch or by returning 0 for a disabled
> clk) and don't rely on human review to adhere to this rule.
> 

I suggest to stop telling folks that they must enable the clk before
getting the rate. The documentation says

 "This is only valid once the clock source has been enabled"

which is really ambiguous. What is "this?" supposed to be? Is it trying
to say the clk isn't toggling at the frequency until it is enabled, so
the rate isn't valid until it is toggling? Or is it saying that the
return value isn't valid until the clk is enabled?  It's been there for
a long time, in fact since Russell introduced the header file on arm in
2004[1]. I'm inclined to read it as:

 "The clk won't be toggling at the rate returned by this function until
  the clk is enabled by clk_enable()"

Maybe send a documentation update instead?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/include/asm-arm/hardware/clock.h?id=0471b5fb91816b448f10e43f922b8a5e264fe039

Re: [PATCH] clk: Warn when clk_get_rate is called for a disabled clk

[Russell King - ARM Linux admin]

On Fri, Feb 12, 2021 at 05:14:36PM -0800, Stephen Boyd wrote:
> Quoting Uwe (2021-02-11 00:05:09)
> > Hello Stephen,
> > 
> > On Wed, Feb 10, 2021 at 07:14:09PM -0800, Stephen Boyd wrote:
> > 
> > > What problem are you trying to address? Is there some issue you've
> > > encountered in the kernel that would have been fixed by having this
> > > warning?
> > 
> > The warning obviously doesn't fix anything. My eventual goal is to
> > answer the question in the initial mail in this thread. The motivating
> > situation is: Should I continue to tell patch authors who use
> > clk_get_rate() that they have to ensure that the given clk must be
> > enabled as the documentation suggests? And if yes: Can we please check
> > this automatically (e.g. with my patch or by returning 0 for a disabled
> > clk) and don't rely on human review to adhere to this rule.
> > 
> 
> I suggest to stop telling folks that they must enable the clk before
> getting the rate. The documentation says
> 
>  "This is only valid once the clock source has been enabled"
> 
> which is really ambiguous. What is "this?" supposed to be?

Please Cc me on CLK API matters, as per my entry in MAINTAINERS.

The subject is the clk_get_rate() function, and as it is in the
section describing that function, I don't see it is ambiguous.

From what I remember, the restriction came after some discussion,
and the problem that on some platforms, the clock tree would not
be known prior to the clock being enabled (at that time, which
was before the "prepare" stuff got added.) Consequently, because
the child/parent relationships were not known, and PLLs were not
initialised, the rate that a particular clock would be ticking
could be different before and after it being enabled.

For this reason, it was decided that the only sensible approach
was to declare that the return value of clk_get_rate() on a
disabled clock is undefined.

That said, finding the discussion is proving difficult, so I may
be misremembering.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] clk: Warn when clk_get_rate is called for a disabled clk

[Uwe Kleine-König]

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

Hello,

On Sat, Feb 13, 2021 at 04:54:06PM +0000, Russell King - ARM Linux admin wrote:
> On Fri, Feb 12, 2021 at 05:14:36PM -0800, Stephen Boyd wrote:
> > Quoting Uwe (2021-02-11 00:05:09)
> > > On Wed, Feb 10, 2021 at 07:14:09PM -0800, Stephen Boyd wrote:
> > > 
> > > > What problem are you trying to address? Is there some issue you've
> > > > encountered in the kernel that would have been fixed by having this
> > > > warning?
> > > 
> > > The warning obviously doesn't fix anything. My eventual goal is to
> > > answer the question in the initial mail in this thread. The motivating
> > > situation is: Should I continue to tell patch authors who use
> > > clk_get_rate() that they have to ensure that the given clk must be
> > > enabled as the documentation suggests? And if yes: Can we please check
> > > this automatically (e.g. with my patch or by returning 0 for a disabled
> > > clk) and don't rely on human review to adhere to this rule.
> > > 
> > 
> > I suggest to stop telling folks that they must enable the clk before
> > getting the rate. The documentation says
> > 
> >  "This is only valid once the clock source has been enabled"
> > 
> > which is really ambiguous. What is "this?" supposed to be?
> 
> Please Cc me on CLK API matters, as per my entry in MAINTAINERS.
> 
> The subject is the clk_get_rate() function, and as it is in the
> section describing that function, I don't see it is ambiguous.

So you interpret it as: "The value returned by clk_get_rate() for a
disabled clk might differ from the actual rate the clk has once it is
enabled.", right?

I agree with Stephen that the semantic isn't as clear as it might be, so
this should be improved.

> From what I remember, the restriction came after some discussion,
> and the problem that on some platforms, the clock tree would not
> be known prior to the clock being enabled (at that time, which
> was before the "prepare" stuff got added.) Consequently, because
> the child/parent relationships were not known, and PLLs were not
> initialised, the rate that a particular clock would be ticking
> could be different before and after it being enabled.
> 
> For this reason, it was decided that the only sensible approach
> was to declare that the return value of clk_get_rate() on a
> disabled clock is undefined.
> 
> That said, finding the discussion is proving difficult, so I may
> be misremembering.

I wonder what should be the consequence here. Is it still true that
clk_get_rate() should not be called for a disabled clk? Or for an
unprepared clk?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] clk: Warn when clk_get_rate is called for a disabled clk

[Marc Kleine-Budde]

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

On 12.11.2021 09:34:30, Uwe Kleine-König wrote:
> > From what I remember, the restriction came after some discussion,
> > and the problem that on some platforms, the clock tree would not
> > be known prior to the clock being enabled (at that time, which
> > was before the "prepare" stuff got added.) Consequently, because
> > the child/parent relationships were not known, and PLLs were not
> > initialised, the rate that a particular clock would be ticking
> > could be different before and after it being enabled.
> > 
> > For this reason, it was decided that the only sensible approach
> > was to declare that the return value of clk_get_rate() on a
> > disabled clock is undefined.
> > 
> > That said, finding the discussion is proving difficult, so I may
> > be misremembering.
> 
> I wonder what should be the consequence here. Is it still true that
> clk_get_rate() should not be called for a disabled clk? Or for an
> unprepared clk?

Anecdotal evidence:

In the early days of the stm32mp1 in the vendor kernel, there was a
problem with the CAN driver. All CAN drivers call clk_get_rate() during
probe to read the clock rate and later in the network device open
callback they use this rate to calculate the dividers to get the
configured bit rate.

The CAN driver was not working, as the clock tree changed between probe
and open callback. The problem was worked around by using a different
parent PLL that doesn't change during startup.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]