Re: [PATCH] clk: Warn when clk_get_rate is called for a disabled clk
From: Stephen Boyd <sboyd@kernel.org>
Date: 2021-02-11 03:14:53
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:quoted
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.
quoted hunk ↗ jump to hunk
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);