linux-kernel - Re: [PATCH v7 2/3] clk: introduce the common clock framework

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJOA=zOgHXy1_J+LG1LUWwsESwrtqAvrgHKUR22AJdsbRHa+YA@mail.gmail.com>
Date:	Mon, 19 Mar 2012 12:09:01 -0700
From:	"Turquette, Mike" <mturquette@...com>
To:	Sascha Hauer <s.hauer@...gutronix.de>
Cc:	Rajendra Nayak <rnayak@...com>,
	Russell King <linux@....linux.org.uk>,
	Paul Walmsley <paul@...an.com>,
	Linus Walleij <linus.walleij@...ricsson.com>,
	patches@...aro.org, Stephen Boyd <sboyd@...eaurora.org>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Magnus Damm <magnus.damm@...il.com>,
	linux-kernel@...r.kernel.org,
	Amit Kucheria <amit.kucheria@...aro.org>,
	Richard Zhao <richard.zhao@...aro.org>,
	Grant Likely <grant.likely@...retlab.ca>,
	Deepak Saxena <dsaxena@...aro.org>,
	Saravana Kannan <skannan@...eaurora.org>,
	Shawn Guo <shawn.guo@...escale.com>,
	linaro-dev@...ts.linaro.org,
	Jeremy Kerr <jeremy.kerr@...onical.com>,
	Arnd Bergman <arnd.bergmann@...aro.org>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v7 2/3] clk: introduce the common clock framework

On Mon, Mar 19, 2012 at 4:28 AM, Sascha Hauer <s.hauer@...gutronix.de> wrote:
> On Mon, Mar 19, 2012 at 04:52:05PM +0530, Rajendra Nayak wrote:
>> Hi Mike,
>>
>> >+/*
>> >+ * 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.

>> >+    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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/