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
| ||
|
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/
Powered by blists - more mailing lists