[PATCHv5 02/10] ARM: OMAP3+: voltage: add support for voltagedomain usecounts

[Tero Kristo]

On Tue, 2012-09-25 at 10:41 +0100, Russell King - ARM Linux wrote:
> On Tue, Sep 25, 2012 at 12:32:37PM +0300, Tero Kristo wrote:
> > diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
> > index ba49029..ca54aec 100644
> > --- a/arch/arm/mach-omap2/powerdomain.c
> > +++ b/arch/arm/mach-omap2/powerdomain.c
> > @@ -1475,10 +1477,16 @@ int pwrdm_state_switch(struct powerdomain *pwrdm)
> >   */
> >  void pwrdm_clkdm_enable(struct powerdomain *pwrdm)
> >  {
> > +	unsigned long flags;
> > +
> >  	if (!pwrdm)
> >  		return;
> >  
> > -	atomic_inc(&pwrdm->usecount);
> > +	if (atomic_inc_return(&pwrdm->usecount) == 1) {
> > +		spin_lock_irqsave(&pwrdm->lock, flags);
> > +		voltdm_pwrdm_enable(pwrdm->voltdm.ptr);
> > +		spin_unlock_irqrestore(&pwrdm->lock, flags);
> > +	}
> 
> This looks like the classic "I like atomic types because they have magic
> properties" brain-deadness.

Hi Russell,

Thats a good catch, I was actually thinking about this sequence myself
also, but decided to leave it as is here due to similarity with the
existing code in mach-omap2/clockdomain.c, see e.g.
_clkdm_clk_hwmod_enable. Maybe those parts should be fixed also...?

> 
> What would happen to users of this if you had this sequence:
> 
> pwrdm->usecount starts off as 1.
> 
> Thread0				Thread1
> atomic_inc_return() (returns 1)
> 				atomic_inc_return() (returns 2)
> 				starts using stuff in power domain
> spin_lock_irqsave()
> voltdm_pwrdm_enable()
> spin_unlock_irqrestore()
> 
> ?

That as such wouldn't break anything, as the callback isn't doing
anything too critical, but yes, for the sequencing of events it is bad.
The alternate implementation I was thinking was to drop the atomic_t and
just use an int for the usecount, and protect the usecount also with the
spinlock. However, there might be some performance issues if this is
done (but I think it is actually better than having some rather
mysterious bugs instead.)

-Tero