Re: [PATCH 1/2] pci: Block config access during BIST (resend)

[Grant Grundler <grundler@parisc-linux.org>]

On Thu, Sep 08, 2005 at 08:39:15AM +1000, Paul Mackerras wrote:
> Grant Grundler writes:
> 
> > I would argue it more obvious. People looking at the code
> > are immediately going to realize it was a deliberate choice to
> > not use a spinlock.
> 
> It achieves exactly the same effect as spin_lock/spin_unlock, just
> more verbosely. :)

Not exactly identical. spin_try_lock() doesn't attempt to acquire
the lock and thus force exclusive access to the cacheline.
Other than that, I agree with you.

I don't see a problem with being verbose here since putting
a spinlock around something that's already atomic (assignment)
even caught Andrew's attention.


> > relax_cpu() doesn't do that?
> 
> No, how can it, when it doesn't get told which virtual cpu to yield
> to?  spin_lock knows which virtual cpu to yield to because we store
> that in the lock variable.

Ah ok. I was expecting relax_cpu() to just pick a "random" different one.
 :^)

> > it's not wrong - just misleading IMHO. There is no
> > "critical section" in that particular chunk of code.
> 
> Other code is using the lock to ensure the atomicity of a compound
> action, which involves the testing the flag and taking some action
> based on the value of the flag.  We take the lock to preserve that
> atomicity.  Locks are often used to make a set of compound actions
> atomic with respect to each other, which is what we're doing here.

Yes.
We agree this chunk only needs to wait until the lock is released.
Other critical sections need to acquire/release the lock.

> > If relax_cpu doesn't allow time-slice donation, then I guess
> > spinlock/unlock with only a comment inside it explain why
> > would be ok too.
> 
> Preferable, in fact. :)

Ok. I was just suggesting the alternative since
Andrew (correctly) questioned the use of a spinlock
and it didn't look right to me either.

thanks,
grant