[PATCH v3 3/5] clk: introduce the common clock framework

[Paul Walmsley]

Hello,

Here are some initial comments on clk_get_rate().

On Mon, 21 Nov 2011, Mike Turquette wrote:

> +/**
> + * clk_get_rate - return the rate of clk
> + * @clk: the clk whose rate is being returned
> + *
> + * Simply returns the cached rate of the clk.  Does not query the hardware.  If
> + * clk is NULL then returns 0.
> + */
> +unsigned long clk_get_rate(struct clk *clk)
> +{
> +	if (!clk)
> +		return 0;
> +
> +	return clk->rate;
> +}
> +EXPORT_SYMBOL_GPL(clk_get_rate);

This implementation of clk_get_rate() is racy, and is, in general, unsafe. 
The problem is that, in many cases, the clock's rate may change between 
the time that clk_get_rate() is called and the time that the returned 
rate is used.  This is the case for many clocks which are part of a 
larger DVFS domain, for example.

Several changes are needed to fix this:

1. When a clock user calls clk_enable() on a clock, the clock framework 
should prevent other users of the clock from changing the clock's rate.  
This should persist until the clock user calls clk_disable() (but see also 
#2 below).  This will ensure that clock users can rely on the rate 
returned by clk_get_rate(), as long as it's called between clk_enable() 
and clk_disable().  And since the clock's rate is guaranteed to remain the 
same during this time, code that cannot tolerate clock rate changes 
without special handling (such as driver code for external I/O devices) 
will work safely without further modification.

2. Since the protocol described in #1 above will prevent DVFS from working 
when the clock is part of a larger DVFS clock group, functions need to be 
added to allow and prevent other clock users from changing the clock's 
rate.  I'll use the function names "clk_allow_rate_changes(struct clk *)" 
and "clk_block_rate_changes(struct clk *)" for this discussion.  These 
functions can be used by clock users to define critical sections during 
which other entities on the system are allowed to change a clock's rate - 
even if the clock is currently enabled.  (Note that when a clock is 
prevented from changing its rate, all of the clocks from it up to the root 
of the tree should also be prevented from changing rates, since parent 
rate changes generally cause disruptions in downstream clocks.)

3. For the above changes to work, the clock framework will need to 
discriminate between different clock users' calls to clock functions like 
clk_{get,set}_rate(), etc.  Luckily this should be possible within the 
current clock interface.  clk_get() can allocate and return a new struct 
clk that clk_put() can later free.  One useful side effect of doing this 
is that the clock framework could catch unbalanced clk_{enable,disable}() 
calls.

4. clk_get_rate() must return an error when it's called in situations 
where the caller hasn't ensured that the clock's rate won't be changed by 
other entities.  For non-fixed rate clocks, these forbidden sections would 
include code between a clk_get() and a clk_enable(), or between a 
clk_disable() and a clk_put() or clk_enable(), or between a 
clk_allow_rate_changes() and clk_block_rate_changes().  The first and 
second of those three cases will require some code auditing of 
clk_get_rate() users to ensure that they are only calling it after they've 
enabled the clock.  And presumably most of them are not checking for 
error.  include/linux/clk.h doesn't define an error return value, so this 
needs to be explicitly defined in clk.h.


- Paul