[PATCH 12/35] davinci: clock framework: remove spinlock usage

[linux@arm.linux.org.uk (Russell King - ARM Linux)]

On Wed, Jan 06, 2010 at 10:31:54AM -0800, Kevin Hilman wrote:
> From: Sekhar Nori <nsekhar@ti.com>
> 
> Currently, the spinlock in DaVinci clock framework is being
> used to:
> 
> 1) Protect clock structure variables usecount and rate against
> concurrent modification.
> 
> 2) Protect against simultaneous PSC enables/disables ie.
> serialize davinci_psc_config().
> 
> 3) Serialize clk_set_rate():
> 	i.	Prevent simultaneous setting of clock rates
> 	ii.	Ensure clock list remains sane during rate
> 		propagation (also in clk_set_parent).
> 
> Remove the spinlock usage in clock framework by:
> 
> 1) Making clock rate and usecount as atomic variables.
> 
> 2) Making davinci_psc_config() protect itself instead of
> relying on serialization by caller.
> 
> 3) (i) Allowing the clk->set_rate to serialize itself. There
> should be no need to serialize all clock rate settings.
> Currently the only user of rate setting is cpufreq driver on
> DA850. cpufreq naturally serializes the calls to set CPU rate.
> Also, there appears no need to lock IRQs during CPU rate
> transtitions. If required, IRQs can be locked in the actual
> set_rate function.
> 
> 3) (ii) Use the mutex already in place for serialzing clock list
> manipulation for serializing clock rate propagation as well.
> 
> Apart from the general benefit of reducing locking granurlarity,
> the main motivation behind this change is to enable usage of
> clock framework for off-chip clock synthesizers. One such
> synthesizer, CDCE949, is present on DM6467 EVM. Access to the
> synthesizer happens through I2C bus - accessing which can lead to
> CPU sleep. Having IRQs locked in clk_set_rate prevents the
> clk->set_rate function from sleeping.

This patch is extremely bogus.  Just because something is called 'atomic'
does not make it so.

atomic_set()..atomic_read() are NOT atomic.  They are just plain write
and read of the underlying atomic value - the former is intended for
one-time initialization, and the latter is intended for debugging.

> @@ -41,15 +40,16 @@ static void __clk_enable(struct clk *clk)
>  {
>  	if (clk->parent)
>  		__clk_enable(clk->parent);
> -	if (clk->usecount++ == 0 && (clk->flags & CLK_PSC))
> +	if (atomic_read(&clk->usecount) == 0 && (clk->flags & CLK_PSC))
>  		davinci_psc_config(psc_domain(clk), clk->gpsc, clk->lpsc, 1);
> +	atomic_inc(&clk->usecount);

This is wrong on two levels.  Firstly, consider a simultaneous clk_enable
and clk_disable.

CPU0						CPU1
__clk_disable

atomic_dec_and_test(&clk->usecount) -> true

						__clk_enable
						atomic_read(&clk->usecount) returns zero
						davinci_psc_config(..., 1);
						atomic_inc(&clk->usecount);
davinci_psc_config(..., 0);

The result is that we now have the situation where the usecount is nonzero
but the clock is disabled.

Similar bogosities can arise when an already enabled clock tries to be
simultaneously enabled for a second time and disabled.

Secondly, davinci_psc_config() does a lot of read-modify-writing.  With
the spinlock gone, what's the protection against simultaneous r-m-w on
these registers?

> @@ -88,7 +80,7 @@ unsigned long clk_get_rate(struct clk *clk)
>  	if (clk == NULL || IS_ERR(clk))
>  		return -EINVAL;
>  
> -	return clk->rate;
> +	return atomic_read(&clk->rate);

atomic here doesn't make clk->rate magically right.  This is precisely
equivalent to reading clk->rate directly.

>  int clk_set_rate(struct clk *clk, unsigned long rate)
>  {
> -	unsigned long flags;
>  	int ret = -EINVAL;
>  
>  	if (clk == NULL || IS_ERR(clk))
>  		return ret;
>  
> -	spin_lock_irqsave(&clockfw_lock, flags);
>  	if (clk->set_rate)
>  		ret = clk->set_rate(clk, rate);
>  	if (ret == 0) {
>  		if (clk->recalc)
> -			clk->rate = clk->recalc(clk);
> +			atomic_set(&clk->rate, clk->recalc(clk));

This atomic_set does nothing to protect you from races.  You might as
well be writing directly to clk->rate.