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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
