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

<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 <redacted>
---
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/ |