Hello Stephen,

On Wed, Feb 10, 2021 at 07:14:09PM -0800, Stephen Boyd wrote:

Quoting Uwe (2021-01-13 00:30:42)

quoted

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

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