Re: Correct types for mod_devicetable.h (was: Re: m68k build failure)

Re: Correct types for mod_devicetable.h (was: Re: m68k build failure)

[Adrian Bunk]

On Mon, Dec 03, 2007 at 09:02:19PM +1100, Rusty Russell wrote:
> On Sunday 02 December 2007 22:22:31 Geert Uytterhoeven wrote:
> > On Sat, 1 Dec 2007, Pierre Ossman wrote:
> > > Yeah, that could work. Have a header with stuff like this:
> > >
> > > typedef u16 __attribute__((aligned(2))) aligned_u16;
> > > typedef u32 __attribute__((aligned(4))) aligned_u32;
> >
> > I gave it a try:
> 
> This seems to turn a molehill into a mountain.
> 
> We can change that mod_devicetable.h at any time; it's not supposed to be a 
> userspace API (the kernel build system doesn't count).

module-init-tools is userspace and not shipped as part of the kernel 
build system...

> So, just insert two bits of padding in sdio_device_id and insert a comment 
> saying "/* Explicit padding: works even if we're cross-compiling */".

We had one such problem in 2.6.23 and now we had a similar one in 2.6.24.

Getting the alignment issues automatically right would really be an 
improvement...

> Thanks,
> Rusty.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: Correct types for mod_devicetable.h (was: Re: m68k build failure)

[Jon Masters]

On Sat, 2007-12-08 at 22:58 +0100, Adrian Bunk wrote:
> On Mon, Dec 03, 2007 at 09:02:19PM +1100, Rusty Russell wrote:
> > On Sunday 02 December 2007 22:22:31 Geert Uytterhoeven wrote:
> > > On Sat, 1 Dec 2007, Pierre Ossman wrote:
> > > > Yeah, that could work. Have a header with stuff like this:
> > > >
> > > > typedef u16 __attribute__((aligned(2))) aligned_u16;
> > > > typedef u32 __attribute__((aligned(4))) aligned_u32;
> > >
> > > I gave it a try:
> > 
> > This seems to turn a molehill into a mountain.
> > 
> > We can change that mod_devicetable.h at any time; it's not supposed to be a 
> > userspace API (the kernel build system doesn't count).
> 
> module-init-tools is userspace and not shipped as part of the kernel 
> build system...

Not really an issue, so long as I get a head's up, but we'll force users
to upgrade so let's be sure we want to do this/everyone's happy.

Jon.

Re: Correct types for mod_devicetable.h (was: Re: m68k build failure)

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 694 bytes --]

On Sun, 2 Dec 2007 12:22:31 +0100 (CET)
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> 
> I gave it a try:
>   - Remove existing alignment attributes from some device_id types
>   - Introduce kernel_* types with proper size and alignment for
>     cross-compilation (sample <asm/kerneltypes.h> for m68k included)
>   - Introduce BITS_PER_KERNEL_LONG, to make it clearer it applies to the target
> 
> Apart from a cross-compile session for m68k, it's untested.
> 

This still requires a bit of maintenance to set up a kerneltypes.h for every arch. It also means we have to be very careful that gcc's internal alignment settings matched the ones in our header.

Rgds
Pierre

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Correct types for mod_devicetable.h (was: Re: m68k build failure)

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 592 bytes --]

On Sat, 8 Dec 2007 22:58:11 +0100
Adrian Bunk <bunk@kernel.org> wrote:

> On Mon, Dec 03, 2007 at 09:02:19PM +1100, Rusty Russell wrote:
> > So, just insert two bits of padding in sdio_device_id and insert a comment 
> > saying "/* Explicit padding: works even if we're cross-compiling */".
> 
> We had one such problem in 2.6.23 and now we had a similar one in 2.6.24.
> 
> Getting the alignment issues automatically right would really be an 
> improvement...
> 

Agreed. I won't veto a quick and dirty fix, but I would prefer if this could be solved properly.

Rgds
Pierre

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Correct types for mod_devicetable.h (was: Re: m68k build failure)

[Adrian Bunk]

On Sun, Dec 09, 2007 at 06:08:18PM +0100, Pierre Ossman wrote:
> On Sun, 2 Dec 2007 12:22:31 +0100 (CET)
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
> > 
> > I gave it a try:
> >   - Remove existing alignment attributes from some device_id types
> >   - Introduce kernel_* types with proper size and alignment for
> >     cross-compilation (sample <asm/kerneltypes.h> for m68k included)
> >   - Introduce BITS_PER_KERNEL_LONG, to make it clearer it applies to the target
> > 
> > Apart from a cross-compile session for m68k, it's untested.
> > 
> 
> This still requires a bit of maintenance to set up a kerneltypes.h for every arch.

Better doing this work once than fixing similar issues again and again.

> It also means we have to be very careful that gcc's internal alignment settings matched the ones in our header.

There's nothing "gcc internal" about struct alignment - remember that 
any change in struct alignment would change our userspace ABI.

> Rgds
> Pierre

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: Correct types for mod_devicetable.h (was: Re: m68k build failure)

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 852 bytes --]

On Mon, 10 Dec 2007 19:11:12 +0100
Adrian Bunk <bunk@kernel.org> wrote:

> On Sun, Dec 09, 2007 at 06:08:18PM +0100, Pierre Ossman wrote:
> > 
> > This still requires a bit of maintenance to set up a kerneltypes.h for every arch.
> 
> Better doing this work once than fixing similar issues again and again.
> 

Agreed. But it raises the bar quite a bit to get this ready for upstream.

> > It also means we have to be very careful that gcc's internal alignment settings matched the ones in our header.
> 
> There's nothing "gcc internal" about struct alignment - remember that 
> any change in struct alignment would change our userspace ABI.
> 

Oh? I would have guessed that everything going to and from userspace would be packed. Should probably consider myself lucky I have do deal with few such interactions. :)

Rgds
Pierre

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Correct types for mod_devicetable.h (was: Re: m68k build failure)

[Rusty Russell]

On Sunday 09 December 2007 08:58:11 Adrian Bunk wrote:
> On Mon, Dec 03, 2007 at 09:02:19PM +1100, Rusty Russell wrote:
> > On Sunday 02 December 2007 22:22:31 Geert Uytterhoeven wrote:
> > > On Sat, 1 Dec 2007, Pierre Ossman wrote:
> > > > Yeah, that could work. Have a header with stuff like this:
> > > >
> > > > typedef u16 __attribute__((aligned(2))) aligned_u16;
> > > > typedef u32 __attribute__((aligned(4))) aligned_u32;
> > >
> > > I gave it a try:
> >
> > This seems to turn a molehill into a mountain.
> >
> > We can change that mod_devicetable.h at any time; it's not supposed to be
> > a userspace API (the kernel build system doesn't count).
>
> module-init-tools is userspace and not shipped as part of the kernel
> build system...

But module-init-tools is *not* supposed to read this; that code is a 2.4 
backwards compatibility layer which should be removed (and certainly not 
enhanced to cover new types in mod_devicetable.h!).

I repeat: this is *only* for the internal modpost code.  

> > So, just insert two bits of padding in sdio_device_id and insert a
> > comment saying "/* Explicit padding: works even if we're cross-compiling
> > */".
>
> We had one such problem in 2.6.23 and now we had a similar one in 2.6.24.
>
> Getting the alignment issues automatically right would really be an
> improvement...

Not if it makes the code (even) more obscure.  Put an attribute((packed)) on 
every structure and add an explanation at the top of the file if it makes you 
feel safer (new additions to the file are likely to copy existing ones), and 
that's sufficient.

Hope that clarifies,
Rusty.