[LinuxBIOS] [PATCH][v3] Spinlock fixes
Uwe Hermann
uwe at hermann-uwe.de
Fri Jul 6 17:07:54 CEST 2007
On Fri, Jul 06, 2007 at 08:22:59AM +0200, Stefan Reinauer wrote:
> * Uwe Hermann <uwe at hermann-uwe.de> [070706 02:05]:
> > Various spinlock-related cleanups:
> >
> > - Revert back to spinlock_t (instead of struct spinlock). This is fine
> > in this special case, as the contents of spinlock_t are not meant to
> > ever be accessed directly (only by "accessor" functions).
>
> NACK.
>
> Why would that be required? The code in svn needs no fixing. Let's not
> invent typedefs without any need for them
It's not strictly required, but as the original code had a spinlock_t
(which we removed because I made some stupid comments before actually
thinking), I thought we could add it back.
It makes sense in this case, as we'll never access anything in the
struct directly. spinlock_t could probably be implemented differently,
and that's the whole point of the typedef in this case -- we don't care
how it's implemented, we only ever access the variables via
spin_lock(foo) and spin_unlock(foo), never via foo->lock directly.
> > - Drop the spin_lock_string and spin_unlock_string macros, they're pretty
> > useless as they're only used in a single place and a macro doesn't make
> > this code any more readable, IMO.
>
> NACK.
>
> They are not useless at all as they are _the_ (one and only) spinlock
> implementation on x86. The "only single place" is the declaration of the
> x86 specific spin_lock and spin_unlock functions.
Huh? I don't understand.
My remark was meant to say the following: version a) is not more
readable than b), so let's use b).
a)
#define spin_lock_string \
"\n1:\t" \
"lock ; decb %0\n\t" \
"js 2f\n" \
".section .text.lock,\"ax\"\n" \
"2:\t" \
"cmpb $0,%0\n\t" \
"rep;nop\n\t" \
"jle 2b\n\t" \
"jmp 1b\n" \
".previous"
static inline __attribute__((always_inline)) void spin_lock(spinlock_t *lock)
{
__asm__ __volatile__(
spin_lock_string
:"=m" (lock->lock) : : "memory");
}
b)
static inline __attribute__((always_inline)) void spin_lock(spinlock_t *lock)
{
__asm__ __volatile__("\n1:\t"
"lock ; decb %0\n\t"
"js 2f\n"
".section .text.lock,\"ax\"\n"
"2:\t"
"cmpb $0,%0\n\t"
"rep;nop\n\t"
"jle 2b\n\t"
"jmp 1b\n"
".previous"
:"=m" (lock->lock) : : "memory");
}
Are there any drawbacks if we use version b)? I think not. This is
indeed x86 specific, a spinlock implementation for PowerPC for example
would be in a different file (arch/powerpc/arch/spinlock.h) and would
implement spin_lock() from scratch anyway.
It's not like we define another spin_lock_string for PowerPC and use it
in a generic spin_lock() function. If we do that, then the spin_lock()
should not be in include/arch/x86/arch/spinlock.h (x86-specific), but rather
in include/spinlock.h (generic, arch-independent), correct?
Uwe.
--
http://www.hermann-uwe.de | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20070706/7e3d47de/attachment.sig>
More information about the coreboot
mailing list