linux-kernel - Re: [RFC,PATCH 1/3] Add a common struct clk

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]

Date:	Tue, 15 Feb 2011 00:33:25 -0800 (PST)
From:	"Saravana Kannan" <skannan@...eaurora.org>
To:	"Jeremy Kerr" <jeremy.kerr@...onical.com>
Cc:	"Saravana Kannan" <skannan@...eaurora.org>,
	"Nicolas Pitre" <nicolas.pitre@...aro.org>,
	"Dima Zavin" <dmitriyz@...gle.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@....com>,
	"Russell King" <linux@....linux.org.uk>, linux-sh@...r.kernel.org,
	"Ben Herrenschmidt" <benh@...nel.crashing.org>,
	"Sascha Hauer" <s.hauer@...gutronix.de>,
	linux-kernel@...r.kernel.org, "Paul Mundt" <lethal@...ux-sh.org>,
	"Ben Dooks" <ben-linux@...ff.org>,
	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>,
	"Vincent Guittot" <vincent.guittot@...aro.org>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC,PATCH 1/3] Add a common struct clk

Hi Jeremy,

Sorry, if the formatting is weird. Using webmail client

On Mon, February 14, 2011 11:26 pm, Jeremy Kerr wrote:
>> > We
>> > may even want to disallow set_rate (and set_parent) when prepare_count
>> is
>> > non- zero.
>>
>> This is definitely not right.
>
> Why is that? Consider two devices using one clock; one does some
> initialisation based on the return value of clk_get_rate(), the other
> calls
> clk_set_rate() some time later. Now the first device is incorrectly
> initialised.

The case you describe is certainly something I consider as incorrect and
agree with you in that we should try to prevent it. But

(prepare_count != 0) != (two devices using one clock).

For one, prepare_count == 1 would be a normal case when a clock is enabled
and the MSM drivers certainly want to be able to set the rate when the
clock is enabled.

But (prepare_count > 1 || enable_count > 1) doesn't mean more than one
device/device driver using the clock either. A simple example would be a
driver wrapping all it's register accesses with a clk_enable/clk_disable
and not having to worry about if a clock is enabled during register access
when it has multiple execution paths (threads, interrupt handler, timers,
etc) that access registers. The driver would just do the enable/disable
around register accesses and let the clock code's ref counting dealing
with making sure a clock is never turned off when it's needed. In these
case both the prepare_count (less likely, but likely) and enable_count can
greater than 1.

Long story short, I support your desire to prevent one driver from
changing the rate from underneath another driver, but the condition you
chose to figure that out is not accurate.

> Regardless, this is definitely something to flag for a later discussion.
> I'm
> happy to return to that, but we should focus on one issue at a time here.

Sure, this discussion of set rate with count is non-zero can be reserved
for later. But I think the discussion of grabbing the lock during
set_parent should be discussed in the context of this patch.

Waiting to see how others feel about this.

Thanks,
Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

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