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.