Re: [LinuxBIOS] r418 - in LinuxBIOSv3: arch/x86 device include include/arch/x86 include/arch/x86/arch include/device lib util/dtc

[Uwe Hermann]

On Fri, Jun 29, 2007 at 06:57:23PM +0200, [e-mail] wrote:
> start using arch/foo.h again instead of archfoo.h (trivial)
> make constructor an initializer.

I don't understand. What's an initializer in this context?
> if(c && c->ops && c->ops->constructor)
> c->ops->constructor(dev, c);
> else
> printk(BIOS_INFO, "No constructor called for %s.\n", 
> 	dev_id_string(&c->id));

BIOS_DEBUG?
> Copied: LinuxBIOSv3/include/arch/x86/arch/elf.h (from rev 417, LinuxBIOSv3/include/arch/x86/archelf.h)
>
> --- LinuxBIOSv3/include/arch/x86/arch/elf.h	                        (rev 0)
> +++ LinuxBIOSv3/include/arch/x86/arch/elf.h	2007-06-29 16:57:23 UTC (rev 418)

Missing license header. Trivial file, so feel free to add yourself as
copyright owner.
> @@ -0,0 +1,8 @@
> +#ifndef ARCH_ELF_H
> +#define ARCH_ELF_H

Following our current guidelines this should be ARCH_X86_ARCH_ELF_H,
which looks a bit stupid, but I think it's necessary.

ARCH_ELF_H doesn't work as we'd have another ARCH_ELF_H if we add
another architecture...
> +#define ELF_CLASS	ELFCLASS32
> +#define ELF_DATA	ELFDATA2LSB
> +#define ELF_ARCH	EM_386
>
> +#endif /* ARCH_ELF_H */
> Added: LinuxBIOSv3/include/arch/x86/arch/spinlock.h
>
> --- LinuxBIOSv3/include/arch/x86/arch/spinlock.h	                        (rev 0)
> +++ LinuxBIOSv3/include/arch/x86/arch/spinlock.h	2007-06-29 16:57:23 UTC (rev 418)
> @@ -0,0 +1,83 @@
> +/*
> This file is part of the LinuxBIOS project.
> Copyright (C) 2001 Linux Networx
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 2 of the License, or
> (at your option) any later version.

This should be GPLv2, I think. We know from email conversation with
Eric Biederman / Linux Networx that their code is GPLv2 only.

But this file is based on Linux code anyway, thus definately GPLv2,
I'll update the header and HACKING accordingly (later).
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> */
>
> +#ifndef ARCH_SPINLOCK_H
> +#define ARCH_SPINLOCK_H

See above.
> +/*
> Your basic SMP spinlocks, allowing only a single CPU anywhere
> */
>
> +typedef struct {
> volatile unsigned int lock;

Why volatile? I think the Linux file doesn't have that.
> +} spinlock_t;

Drop spinlock_t and use 'struct spinlock' as we do for other structs?
> +#define SPIN_LOCK_UNLOCKED (spinlock_t) { 1 }
>
> +/*
> Simple spin lock operations.  There are two variants, one clears IRQ's
> on the local processor, one does not.
> We make no fairness assumptions. They have a cost.
> */
>
> +#define barrier() __asm__ __volatile__("": : :"memory")

#define barrier() (__asm__ __volatile__("": : :"memory"))

(better safe than sorry)
> +#define spin_is_locked(x)	(*(volatile char *)(&(x)->lock) <= 0)
> +#define spin_unlock_wait(x)	do { barrier(); } while(spin_is_locked(x))

#define spin_unlock_wait(x) (do { barrier(); } while(spin_is_locked(x)))
> +#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"
>
> +/*
> This works. Despite all the confusion.
> */
>
> +#define spin_unlock_string \
> "movb $1,%0"

Shall we drop these #defines? They're only used in one place and a small
comment which says what the asm does should be enough.
> +static inline __attribute__((always_inline)) void spin_lock(spinlock_t *lock)
> +{
> __asm__ __volatile__(
> spin_lock_string
> :"=m" (lock->lock) : : "memory");
>
> +}
>
> +static inline __attribute__((always_inline)) void spin_unlock(spinlock_t *lock)
> +{
> __asm__ __volatile__(
> spin_unlock_string
> :"=m" (lock->lock) : : "memory");
>
> +}
> Modified: LinuxBIOSv3/util/dtc/dtc-parser.y
>
> --- LinuxBIOSv3/util/dtc/dtc-parser.y	2007-06-29 15:32:19 UTC (rev 417)
> +++ LinuxBIOSv3/util/dtc/dtc-parser.y	2007-06-29 16:57:23 UTC (rev 418)
> @@ -142,7 +142,7 @@
>    	}
>    	')' ';' { 
>    			/* convention: first property is labeled with path */
> 		$6->label = strdup($3.val);
> 		$6->label = strdup((char *)$3.val);

Is this the correct fix for the warning? I didn't notice this warning
before so it must have been introduced by some recent change?

Uwe.

[LinuxBIOS] r418 - in LinuxBIOSv3: arch/x86 device include include/arch/x86 include/arch/x86/arch include/device lib util/dtc

[ron minnich]

On 6/30/07, Uwe Hermann  wrote:
> >           ')' ';' {
> >                           /* convention: first property is labeled with path */
> >                         $6->label = strdup($3.val);
> >                         $6->label = strdup((char *)$3.val);
>
> Is this the correct fix for the warning? I didn't notice this warning
> before so it must have been introduced by some recent change?

I don't know how that crept in. it was while I was trying to figure things out.

It's a mistake, I will submit a patch to yank it.

ron

[LinuxBIOS] r418 - in LinuxBIOSv3: arch/x86 device include include/arch/x86 include/arch/x86/arch include/device lib util/dtc

[Stefan Reinauer]

* Uwe Hermann  [070630 21:18]:
> On Fri, Jun 29, 2007 at 06:57:23PM +0200, [e-mail] wrote:
> > start using arch/foo.h again instead of archfoo.h (trivial)
> > make constructor an initializer.
>
> I don't understand. What's an initializer in this context?

It does not allocate the data structure anymore since this has been done
already. Before any device with a constructor would actually create two
struct device  entries
> > printk(BIOS_INFO, "No constructor called for %s.\n", 
> > 	dev_id_string(&c->id));
>
> BIOS_DEBUG?

Yeah. there are some other too verbose printks in the device code.
> > +#ifndef ARCH_ELF_H
> > +#define ARCH_ELF_H
>
> Following our current guidelines this should be ARCH_X86_ARCH_ELF_H,
> which looks a bit stupid, but I think it's necessary.
>
> ARCH_ELF_H doesn't work as we'd have another ARCH_ELF_H if we add
> another architecture...

I think this is dangerous, because you would accidently be able to
include both the ppc and the x86 version of the file, which you dont
want.
> > Copyright (C) 2001 Linux Networx
> > This program is free software; you can redistribute it and/or modify
> > it under the terms of the GNU General Public License as published by
> > the Free Software Foundation; either version 2 of the License, or
> > (at your option) any later version.
>
> This should be GPLv2, I think. We know from email conversation with
> Eric Biederman / Linux Networx that their code is GPLv2 only.

Ok. I took the file from v2. Should we change it? Can you make a patch
> But this file is based on Linux code anyway, thus definately GPLv2,
> I'll update the header and HACKING accordingly (later).

great!!
> > +typedef struct {
> > volatile unsigned int lock;
> Why volatile? I think the Linux file doesn't have that.

Then Linux is broken. The CPU is not supposed to cache this value under
any circumstances, hence volatile. If CPU1 writes it, CPU0 must not
assume the last value it wrote to it.
> > +} spinlock_t;
>
> Drop spinlock_t and use 'struct spinlock' as we do for other structs?

I really hate this dropping of the _t types, but since we kind of agreed
on that... I guess you are right.
> > +#define barrier() __asm__ __volatile__("": : :"memory")
>
> #define barrier() (__asm__ __volatile__("": : :"memory"))
>
> (better safe than sorry)

this will break code using it. try barrier();
> > +#define spin_is_locked(x)	(*(volatile char *)(&(x)->lock) <= 0)
> > +#define spin_unlock_wait(x)	do { barrier(); } while(spin_is_locked(x))
>
> #define spin_unlock_wait(x) (do { barrier(); } while(spin_is_locked(x)))

same thing. these are functions, not expressions.
> > +#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"
> >
> > +/*
> > This works. Despite all the confusion.
> > */
> >
> > +#define spin_unlock_string \
> > "movb $1,%0"
>
> Shall we drop these #defines? They're only used in one place and a small
> comment which says what the asm does should be enough.

Hm. possibly. Anyone has a comment?
> > 		/* convention: first property is labeled with path */
> > 		$6->label = strdup($3.val);
> > 		$6->label = strdup((char *)$3.val);
>
> Is this the correct fix for the warning? I didn't notice this warning
> before so it must have been introduced by some recent change?

Maybe it is due to the compiler I am using..?

It is the correct fix though, strdup expects a signed char.

[LinuxBIOS] r418 - in LinuxBIOSv3: arch/x86 device include include/arch/x86 include/arch/x86/arch include/device lib util/dtc

[ron minnich]

On 6/30/07, Stefan Reinauer  wrote:
> > > +typedef struct {
> > > volatile unsigned int lock;
> > Why volatile? I think the Linux file doesn't have that.

Quoth Linus:
"we should _never_ make anything volatile. There just isn't any reason
to. The compiler will never do any "better" with a volatile, it will
only ever do worse.

If there are memory ordering constraints etc, the compiler won't be
able to handle them anyway, and volatile will be a no-op. That's why
we have "barrier()" and "mb()" and friends.

The _only_ acceptable use of "volatile" is basically:

*

in _code_ (not data structures), where we might mark a place as
making a special type of access. For example, in the PCI MMIO read
functions, rather than use inline assembly to force the particular
access (which would work equally well), we can force the pointer to a
volatile type.

Similarly, we force this for "test_bit()" macros etc, because
they are documented to work on SMP-safe. But it's the _code_ that
works that way, not the data structures.

And this is an important distinctions: there are specific pieces
of _code_ that may be SMP-safe (for whatever reasons the writer
thinks). Data structures themselves are never SMP safe.

Ergo: never mark data structures "volatile". It's a sure sign of
a bug if the thing isn't a memory-mapped IO register (and even then
it's likely a bug, since you really should be using the proper
functions).

(Some driver writers use "volatile" for mailboxes that are
updated by DMA from the hardware. It _can_ be correct there, but the
fact is, you might as well put the "volatile" in the code just out of
principle).

That said, the "sure sign of a bug" case has one specific sub-case:

* to paste over bugs that you really don't tink are worth fixing
any other way. This is why "jiffies" itself is declared volatile: just
to let people write code that does "while (time_before(xxx,
jiffies))".

But the "jiffies" case is safe only _exactly_ because it's an atomic
read. You always get a valid value - so it's actually "safe" to mark
jiffies as baing volatile. It allows people to be sloppy (bad), but at
least it allows people to be sloppy in a safe way.

In contrast, "jiffies_64" is _not_ an area where you can safely let
the compiler read a unstable value, so "volatile" is fundamentally
wrong for it. You need to have some locking, or to explicitly say "we
don't care in this case", and in both cases it would be wrong to call
the thing "volatile". With locking, it _isn't_ volatile, and with "we
don't care", it had better not make any difference. In either case the
"volatile" is wrong.

We had absolutely _tons_ of bugs in the original networking code,
where clueless people thougth that "volatile" somehow means that you
don't need locking. EVERY SINGLE CASE, and I mean EVERY ONE, was a
bug.

There are some other cases where the "volatile" keyword is fine,
notably inline asm has a specific meaning that is pretty well-defined
and very useful. But in all other cases I'd suggest having the
volatile be part of the code, possibly with an explanation _why_ it is
ok to use volatile there unless it is obvious.
"

It's an interesting viewpoint.

But it does argue that we should not use volatile. And it also argues
we need to learn how to use stuff like mb(). Oh joy :-)

ron

[LinuxBIOS] r418 - in LinuxBIOSv3: arch/x86 device include include/arch/x86 include/arch/x86/arch include/device lib util/dtc

[Peter Stuge]

On Sat, Jun 30, 2007 at 09:38:32PM +0200, Stefan Reinauer wrote:
> > > +#define barrier() __asm__ __volatile__("": : :"memory")
> >
> > #define barrier() (__asm__ __volatile__("": : :"memory"))
>
> this will break code using it. try barrier();

#define barier() do { __asm__ .. } while(0)

is common, does it work here?

//Peter

[LinuxBIOS] r418 - in LinuxBIOSv3: arch/x86 device include include/arch/x86 include/arch/x86/arch include/device lib util/dtc

[Peter Stuge]

On Sat, Jun 30, 2007 at 02:56:22PM -0700, ron minnich wrote:
> we need to learn how to use stuff like mb(). Oh joy :-)

Isn't all that bad..

Call wmb() to "commit" writes. Call rmb() to "commit" reads. Call
mb() to "commit" both.

Good thing they have short names.

//Peter