[LinuxBIOS] [PATCH] v3 - spinlock cleanup
uwe at hermann-uwe.de
Tue Jul 3 01:39:06 CEST 2007
On Mon, Jul 02, 2007 at 12:50:04AM +0200, Stefan Reinauer wrote:
> * Uwe Hermann <uwe at hermann-uwe.de> [070701 02:14]:
> > OK, I totally misread what the code is supposed to do.
> > I'm not sure the spin_define() is such a good idea then. It has a nice
> > name and all, but it might be obfuscating things more than necessary.
> > Maybe a simple variable declaration (i.e., drop the #define) would be better?
> I don't see why it would be better, there are no obvious advantages, but
> some obvious disadvantages.
> Here our principles start clashing. We can't do this for several
> * The declaration is static, so it has to be surrounded by #if
> defined(CONFIG_SMP) would waste all the places in the code. This is
> controverse to our goal to make changes as local as possible.
> If we don't, we end up with warnings in the code for almost every
> * Since some of us started hating typedefs, we don't have anything to
> declare anymore. You simply can not rely that a spinlock is
> represented by struct spinlock on all possible systems.
> So to get this done right, we would have to go back to spinlock_t,
> in which case I would vote to also go back to device_t and all the
> others, so we stay consistent.
Hm, ok forget what I said.
I think we should go back to using spinlock_t (but not device_t!).
As far as I remember the problem with device_t was that it was not
just a typedef for a struct, but a typedef for a _pointer_to_a_struct_.
That causes many nasty possibilities for bugs to creep in when you
see foo_t and think it's an int or struct where in reality it is
actually a _pointer_ to something.
So I think we should revert back to spinlock_t and use the macro, too.
Sorry for the confusion. 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...
Size: 189 bytes
Desc: Digital signature
More information about the coreboot