linux-kernel - Re: Locking in the clk API, part 2: clk_prepare/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]

Message-ID: <20110204105710.GF14627@n2100.arm.linux.org.uk>
Date:	Fri, 4 Feb 2011 10:57:10 +0000
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
Cc:	Richard Zhao <linuxzsc@...il.com>,
	Nicolas Pitre <nicolas.pitre@...aro.org>,
	Lorenzo Pieralisi <Lorenzo.Pieralisi@....com>,
	Vincent Guittot <vincent.guittot@...aro.org>,
	linux-sh@...r.kernel.org,
	Ben Herrenschmidt <benh@...nel.crashing.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Paul Mundt <lethal@...ux-sh.org>,
	Stephen Boyd <sboyd@...eaurora.org>,
	linux-kernel@...r.kernel.org, Dima Zavin <dmitriyz@...gle.com>,
	Saravana Kannan <skannan@...eaurora.org>,
	Ben Dooks <ben-linux@...ff.org>,
	Jeremy Kerr <jeremy.kerr@...onical.com>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: Locking in the clk API, part 2: clk_prepare/clk_unprepare

On Fri, Feb 04, 2011 at 11:21:20AM +0100, Uwe Kleine-König wrote:
> I happily point out that the prepare_count needs to be protected by a
> spinlock and you need a flag that signals a prepare or unprepare is
> currently running.

It's really simple.  You don't use a struct clk pointer in any way until
you've called a clk_get() to get a pointer.  So what's the problem with
ensuring that you do clk_prepare() on it before you register whatever
services may end up calling clk_enable().

That is good practice.  It's precisely the same practice which says that
you shall not register device drivers with subsystems, thereby making
them visible, until you're absolutely ready in the driver to start taking
requests to use your driver.  Precisely the same thing applies here.

In other words, to go back to the UART console driver case, in the
UART console setup function, you do this:

	clk = clk_get(...);
	if (IS_ERR(clk))
		return PTR_ERR(clk);

	err = clk_prepare(clk);
	if (err) {
		clk_put(clk);
		return err;
	}

	rate = clk_get_rate(clk);
	... setup UART, setup baud rate according to rate ...

	return 0;

So, this means that clk_enable() in the console write function will not
be called until after the initialization function has finished - by which
time clk_prepare() will have completed.

There is no need for any kind of spinlocking, atomic types or other such
crap for the prepare count.  We do not care about concurrent clk_enables().

The only time you'd need such games as you're suggesting is if you're still
promoting your idea about calling clk_prepare() from clk_enable() "in case
driver writers forget it", which is soo broken it's untrue.
--
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/