linux-kernel - Re: [RFC,PATCH 1/2] 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]

Message-ID: <20100604000605.GF4720@trinity.fluff.org>
Date:	Fri, 4 Jun 2010 01:06:05 +0100
From:	Ben Dooks <ben-linux@...ff.org>
To:	Russell King - ARM Linux <linux@....linux.org.uk>
Cc:	Jeremy Kerr <jeremy.kerr@...onical.com>,
	Ben Dooks <ben-linux@...ff.org>,
	Ben Herrenchmidt <benh@...nel.crashing.org>,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC,PATCH 1/2] Add a common struct clk

On Thu, Jun 03, 2010 at 12:05:33PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jun 03, 2010 at 06:24:50PM +0800, Jeremy Kerr wrote:
> > OK, this would mean adding parent to struct clk:
> > 
> > struct clk {
> > 	struct clk_operations	*ops;
> > 	atomic_t				enable_count;
> 
> I don't think it makes sense for this to be an atomic type.
> 
> > static inline int clk_enable(struct clk *clk)
> > {
> > 	int ret;
> > 
> > 	if (atomic_inc(clk->enable_count) != 1))
> > 		return 0;
> 
> Okay, so what if we sleep at this point, and someone else comes along
> and calls clk_enable(), which'll merely increment the count and return.
> 
> Unfortunately, the clock is still disabled at that point - which is a
> violation of what's supposed to happen.
> 
> Or to put it another way, the above method results in clk_enable()
> sometimes returning with the clock enabled and sometimes with the
> clock still disabled.
> 
> That's not nice behaviour for drivers which may need the clock enabled
> to read/write the device registers.

You're right, especially if the clock takes time to enable.

Either we need some form of locking to deal with this. An overall lock
will cause problems with respect to re-using clk_enable() internally
and per-clock locks are probably a bad idea with respect of trying to
lock the parents (deadlock) depending on the implementation.

I'm not sure if something like a wake queue is required, where any caller
who is calling when the clock is disabled gets placed on the queue
awaiting the original enabler finishing the enable operation.

	 if (atomic_increment(&clk->usage_count) == 1) {
	    /* start enable */
	    atomic_set(&clk->is_enabled, 1);
	    wake_up(&clk->wait_for_enable);   
	 } else if (atomic_read(&clk->is_enabled) == 0) {
	   wait_event(&clk->wait_for_enable);
	 }

Also, what about the behaviour across >1 CPU? There's a pile of
up-comming SoCs with SMP ARM cores. If CPU1 enables the clock, and CPU2
comes along and tries to do that also, we have the same race condition
again.

As an addendum to Russell's comments, we need to specify the behaviour
with resepect to what happens about clock stabilisation as well, if the
clock we enabled takes time to stabilise, then should we wait? This will
be even worse when we realise some of these architectures have clocks
which are source from PLLs that can be turned on/off, and these PLLs
have settling time in 100s of usecs.

Even if the clock is enabled, it requires time to stabilise to avoid
problems when the driver then goes to try and use it.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

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