On Mon, Mar 19, 2012 at 4:28 AM, Sascha Hauer [off-list ref] wrote:

On Mon, Mar 19, 2012 at 04:52:05PM +0530, Rajendra Nayak wrote:

quoted

Hi Mike,

quoted

+/*
+ * calculate the new rates returning the topmost clock that has to be
+ * changed.
+ */
+static struct clk *clk_calc_new_rates(struct clk *clk, unsigned long rate)
+{
+ ? ?struct clk *top = clk;
+ ? ?unsigned long best_parent_rate = clk->parent->rate;

Shouldn't you check for a valid parent before dereferencing it? A
clk_set_rate() on a root clock might throw up some issues otherwise.

Yes, should be checked.

The clk_calc_new_rates code assumes a valid parent pointer in several
locations.  Thanks for the catch Rajendra.  Will roll into my fixes
series.

quoted

quoted

+ ? ?unsigned long new_rate;
+
+ ? ?if (!clk->ops->round_rate&& ?!(clk->flags& ?CLK_SET_RATE_PARENT)) {
+ ? ? ? ? ? ?clk->new_rate = clk->rate;
+ ? ? ? ? ? ?return NULL;

So does this mean a clk_set_rate() fails for a clk which does not have
a valid .round_rate and does not have a CLK_SET_RATE_PARENT flag set?
I was thinking this could do a..
? ? ? ? ? ? ? clk->new_rate = rate;
? ? ? ? ? ? ? top = clk;
? ? ? ? ? ? ? goto out;
..instead.

The core should make sure that either both set_rate and round_rate are
present or none of them.

Agreed.  The documentation covers which clk_ops are hard dependencies
(based on supported operations), but the code doesn't strictly check
this.  I'll add a small state machine to __clk_init which validates
that .round_rate, .recalc_rate and .set_rate are *all* present if any
one of them are present, and present a WARN if otherwise.

Thanks,
Mike