Re: [GIT PATCH] ACPI patches for 2.6.23-rc1

Re: [GIT PATCH] ACPI patches for 2.6.23-rc1

[Adrian Bunk]

On Fri, Jul 27, 2007 at 06:25:12PM +0200, Thomas Renninger wrote:
> On Fri, 2007-07-27 at 08:26 +0200, Jan Dittmer wrote:
> > Len Brown wrote:
> > > Hi Linus,
> > > 
> > > please pull from: 
> > > 
> > > git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux-acpi-2.6.git release
> > 
> > This seems to break ia64 defconfig:
> > 
> >    Building modules, stage 2.
> >    MODPOST 157 modules
> > FATAL: drivers/acpi/button: sizeof(struct acpi_device_id)  is not a modulo of the size of section __mod_acpi_device_table\x144.
> > Fix definition of struct acpi_device_id in mod_devicetable.h
> > make[2]: *** [__modpost] Error 1
> > make[1]: *** [modules] Error 2
> > make: *** [_all] Error 2
> > 
> > gcc 3.3.6, binutils 2.15.94
> > 
> > http://l4x.org/k/?d2569
> 
> This is strange, I just compiled on a IA64 with button as module
> (defconfig), but with gcc version 4.1.2, all is fine.
> Anyone an idea how to run into that?

I don't have an idea how this happens, but it sounds like an alignment 
issue:

sizeof(struct acpi_device_id) 
"struct acpi_device_id button_device_ids[]" contains 6 elements
144 = 6 * 24 = 6 * (3 * 8)

So it seems on ia64 with gcc 3.3.6 there's some 8 byte alignment of the 
array members?

Sam and the ia64 maintainers Cc'ed - they might know better what's going 
on here.

> I won't be able to detailed debug this before Monday, but be sure I will
> do so then.
> 
> Thanks,
> 
>     Thomas

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: [GIT PATCH] ACPI patches for 2.6.23-rc1

[Andreas Schwab]

Adrian Bunk <bunk@stusta.de> writes:

> I don't have an idea how this happens, but it sounds like an alignment 
> issue:
>
> sizeof(struct acpi_device_id) 

This is wrong.  The structure has 24 bytes (including 7(!) bytes of
padding).

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

scripts/mod/file2alias.c cross compile problem

[Adrian Bunk]

On Fri, Jul 27, 2007 at 04:21:47PM -0700, Luck, Tony wrote:
> > So it seems on ia64 with gcc 3.3.6 there's some 8 byte alignment of the 
> > array members?
> >
> > Sam and the ia64 maintainers Cc'ed - they might know better what's going 
> > on here.
> 
> This ia64 maintainer is baffled ... but I don't see the problem here (perhaps
> because my build machine has gcc 3.4.6).


I found what causes this problem, and it only occurs during cross 
compilation.


The struct is:

#define ACPI_ID_LEN     9

struct acpi_device_id {
	 __u8 id[ACPI_ID_LEN];
	kernel_ulong_t driver_data;
};


When compiling for ia64, this results in:

struct acpi_device_id {
	__u8 id[9];
	uint64_t driver_data;
};


sizeof(struct acpi_device_id) for ia64 is due to different padding
after id[] 20 bytes on i386 but 24 bytes on ia64.

scripts/mod/file2alias.c is compiled with HOSTCC and ensures that 
kernel_ulong_t is correct (in this case uint64_t for ia64), but it can't 
cope with different padding on different architectures.


> -Tony

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: scripts/mod/file2alias.c cross compile problem

[Thomas Renninger]

On Sat, 2007-07-28 at 03:39 +0200, Adrian Bunk wrote:
> On Fri, Jul 27, 2007 at 04:21:47PM -0700, Luck, Tony wrote:
> > > So it seems on ia64 with gcc 3.3.6 there's some 8 byte alignment of the 
> > > array members?
> > >
> > > Sam and the ia64 maintainers Cc'ed - they might know better what's going 
> > > on here.
> > 
> > This ia64 maintainer is baffled ... but I don't see the problem here (perhaps
> > because my build machine has gcc 3.4.6).
> 
> 
> I found what causes this problem, and it only occurs during cross 
> compilation.
> 
> 
> The struct is:
> 
> #define ACPI_ID_LEN     9
> 
> struct acpi_device_id {
> 	 __u8 id[ACPI_ID_LEN];
> 	kernel_ulong_t driver_data;
> };
> 
> 
> When compiling for ia64, this results in:
> 
> struct acpi_device_id {
> 	__u8 id[9];
> 	uint64_t driver_data;
> };
> 
> 
> sizeof(struct acpi_device_id) for ia64 is due to different padding
> after id[] 20 bytes on i386 but 24 bytes on ia64.
> 
> scripts/mod/file2alias.c is compiled with HOSTCC and ensures that 
> kernel_ulong_t is correct (in this case uint64_t for ia64), but it can't 
> cope with different padding on different architectures.

This one should workaround it. Is this acceptable for now?
Don't know how to fix this in a cleaner way (but I am sure it's
possible... ). Maybe an IA64 guy who is more used to such things has a
better idea and could have a look at this if there is time...

   Thomas

-------------

Cross-compilation between e.g. i386 -> 64bit could break -> work around it

Adrian Bunk: scripts/mod/file2alias.c is compiled with HOSTCC and ensures that
kernel_ulong_t is correct, but it can't cope with different padding on
different architectures.

---
 include/linux/mod_devicetable.h |    2 ++
 1 file changed, 2 insertions(+)

Index: torvalds/include/linux/mod_devicetable.h
=================================--- torvalds.orig/include/linux/mod_devicetable.h
+++ torvalds/include/linux/mod_devicetable.h
@@ -160,9 +160,11 @@ struct ap_device_id {
 #define AP_DEVICE_ID_MATCH_DEVICE_TYPE		0x01
 
 #define ACPI_ID_LEN	9
+#define FILLUP_LEN	7 /* dirty fix for i386 -> 64bit cross-compilation */
 
 struct acpi_device_id {
 	__u8 id[ACPI_ID_LEN];
+	__u8 dummy[FILLUP_LEN];
 	kernel_ulong_t driver_data;
 };
 

RE: scripts/mod/file2alias.c cross compile problem

[Luck, Tony]

> Adrian Bunk: scripts/mod/file2alias.c is compiled with HOSTCC and ensures that
> kernel_ulong_t is correct, but it can't cope with different padding on
> different architectures.

Surely this is the root cause ... you can't expect that the alignment
rules of HOSTCC to make any sense for an arbitraty target.

> +#define FILLUP_LEN	7 /* dirty fix for i386 -> 64bit cross-compilation */
>  
>  struct acpi_device_id {
>  	__u8 id[ACPI_ID_LEN];
> +	__u8 dummy[FILLUP_LEN];
>  	kernel_ulong_t driver_data;
>  };

What's so special about this structure that we get an error?  Surely
there are many kernel structures with different alignment/padding
when built with i386 complier compared with ia64 compiler.  We can't
go around manually padding them all (it wastes time, and also wastes
memory on the 32-bit systems that don't need this padding).

-Tony
 

Re: scripts/mod/file2alias.c cross compile problem

[Andreas Schwab]

"Luck, Tony" <tony.luck@intel.com> writes:

>> +#define FILLUP_LEN	7 /* dirty fix for i386 -> 64bit cross-compilation */
>>  
>>  struct acpi_device_id {
>>  	__u8 id[ACPI_ID_LEN];
>> +	__u8 dummy[FILLUP_LEN];
>>  	kernel_ulong_t driver_data;
>>  };
>
> What's so special about this structure that we get an error?

It's special because it's a device_id structure, and those structures
must come out identical using either the host or the target compiler.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

RE: scripts/mod/file2alias.c cross compile problem

[Luck, Tony]

>>>  struct acpi_device_id {
>>>  	__u8 id[ACPI_ID_LEN];
>>> +	__u8 dummy[FILLUP_LEN];
>>>  	kernel_ulong_t driver_data;
>>>  };
>>
>> What's so special about this structure that we get an error?
>
> It's special because it's a device_id structure, and those structures
> must come out identical using either the host or the target compiler.

That didn't help me understand.  Are device_id structures visible
in some user-level API?  If so, then I can see why they'd need to
be the same (but then I'd be confused why this structure uses a
"kernel_ulong_t" type).

-Tony

Re: scripts/mod/file2alias.c cross compile problem

[Sam Ravnborg]

On Thu, Aug 02, 2007 at 10:40:14AM -0700, Luck, Tony wrote:
> >>>  struct acpi_device_id {
> >>>  	__u8 id[ACPI_ID_LEN];
> >>> +	__u8 dummy[FILLUP_LEN];
> >>>  	kernel_ulong_t driver_data;
> >>>  };
> >>
> >> What's so special about this structure that we get an error?
> >
> > It's special because it's a device_id structure, and those structures
> > must come out identical using either the host or the target compiler.
> 
> That didn't help me understand.  Are device_id structures visible
> in some user-level API?  If so, then I can see why they'd need to
> be the same (but then I'd be confused why this structure uses a
> "kernel_ulong_t" type).

I second this. For anything visible in userspace from
include/* we require usage of the kernel specific
__u8, __u16, __u32, __u64 typedefs but for device_id we accept
kernel_ulong_t which result in the following crap in
file2alias.c:

/* We use the ELF typedefs for kernel_ulong_t but bite the bullet and
 * use either stdint.h or inttypes.h for the rest. */
#if KERNEL_ELFCLASS = ELFCLASS32
typedef Elf32_Addr      kernel_ulong_t;
#define BITS_PER_LONG 32
#else
typedef Elf64_Addr      kernel_ulong_t;
#define BITS_PER_LONG 64
#endif

And we ought to have __u64 available.
See for example types.h from asm-i386:
#if defined(__GNUC__) && !defined(__STRICT_ANSI__)
typedef __signed__ long long __s64;
typedef unsigned long long __u64;
#endif


	Sam

Re: scripts/mod/file2alias.c cross compile problem

[Adrian Bunk]

On Thu, Aug 02, 2007 at 08:09:03PM +0200, Sam Ravnborg wrote:
> 
> I second this. For anything visible in userspace from
> include/* we require usage of the kernel specific
> __u8, __u16, __u32, __u64 typedefs but for device_id we accept
> kernel_ulong_t which result in the following crap in
> file2alias.c:
> 
> /* We use the ELF typedefs for kernel_ulong_t but bite the bullet and
>  * use either stdint.h or inttypes.h for the rest. */
> #if KERNEL_ELFCLASS = ELFCLASS32
> typedef Elf32_Addr      kernel_ulong_t;
> #define BITS_PER_LONG 32
> #else
> typedef Elf64_Addr      kernel_ulong_t;
> #define BITS_PER_LONG 64
> #endif
> 
> And we ought to have __u64 available.
> See for example types.h from asm-i386:
> #if defined(__GNUC__) && !defined(__STRICT_ANSI__)
> typedef __signed__ long long __s64;
> typedef unsigned long long __u64;
> #endif


You are talking about something different than the current problem.

The current problem is that when crosscompiling we get a different 
_padding_ due to file2alias.c being compiled with HOSTCC.


What you are talking about is a different issue, and I can't talk about 
it since I don't understand these interactions between modpost and depmod.


> 	Sam

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: scripts/mod/file2alias.c cross compile problem

[Sam Ravnborg]

On Thu, Aug 02, 2007 at 09:15:09PM +0200, Adrian Bunk wrote:
> On Thu, Aug 02, 2007 at 08:09:03PM +0200, Sam Ravnborg wrote:
> > 
> > I second this. For anything visible in userspace from
> > include/* we require usage of the kernel specific
> > __u8, __u16, __u32, __u64 typedefs but for device_id we accept
> > kernel_ulong_t which result in the following crap in
> > file2alias.c:
> > 
> > /* We use the ELF typedefs for kernel_ulong_t but bite the bullet and
> >  * use either stdint.h or inttypes.h for the rest. */
> > #if KERNEL_ELFCLASS = ELFCLASS32
> > typedef Elf32_Addr      kernel_ulong_t;
> > #define BITS_PER_LONG 32
> > #else
> > typedef Elf64_Addr      kernel_ulong_t;
> > #define BITS_PER_LONG 64
> > #endif
> > 
> > And we ought to have __u64 available.
> > See for example types.h from asm-i386:
> > #if defined(__GNUC__) && !defined(__STRICT_ANSI__)
> > typedef __signed__ long long __s64;
> > typedef unsigned long long __u64;
> > #endif
> 
> 
> You are talking about something different than the current problem.
> 
> The current problem is that when crosscompiling we get a different 
> _padding_ due to file2alias.c being compiled with HOSTCC.

I am well aware of that.
My point was that we are dealing with userspace - in this case depmod.
So I wondered why we had special (as in less strict) rules here.

	Sam

Re: scripts/mod/file2alias.c cross compile problem

[Al Viro]

On Thu, Aug 02, 2007 at 09:24:56PM +0200, Sam Ravnborg wrote:
> I am well aware of that.
> My point was that we are dealing with userspace - in this case depmod.
> So I wondered why we had special (as in less strict) rules here.

In reality it's a pointer...

RE: scripts/mod/file2alias.c cross compile problem

[Rusty Russell]

On Thu, 2007-08-02 at 09:25 -0700, Luck, Tony wrote:
> > Adrian Bunk: scripts/mod/file2alias.c is compiled with HOSTCC and ensures that
> > kernel_ulong_t is correct, but it can't cope with different padding on
> > different architectures.
> 
> Surely this is the root cause ... you can't expect that the alignment
> rules of HOSTCC to make any sense for an arbitraty target.
> 
> > +#define FILLUP_LEN	7 /* dirty fix for i386 -> 64bit cross-compilation */
> >  
> >  struct acpi_device_id {
> >  	__u8 id[ACPI_ID_LEN];
> > +	__u8 dummy[FILLUP_LEN];
> >  	kernel_ulong_t driver_data;
> >  };
> 
> What's so special about this structure that we get an error?

It's in mod_devicetable.h: see comment at top of that file.  These
structures serve dual purpose: to describe the capabilities of the
driver to the kernel probing functions, *and* to export them to
userspace tables.  The former purpose is why there's a data pointer in
there.

scripts/mod/file2alias is the program that reads this: although it can
be altered to parse 32-vs-64, Adrian's fix is the simplest.

Hope that clarifies,
Rusty.

Re: scripts/mod/file2alias.c cross compile problem

[Adrian Bunk]

On Fri, Aug 03, 2007 at 08:08:20AM +1000, Rusty Russell wrote:
>...
> scripts/mod/file2alias is the program that reads this: although it can
> be altered to parse 32-vs-64, Adrian's fix is the simplest.

s/Adrian/Thomas/

> Hope that clarifies,
> 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: scripts/mod/file2alias.c cross compile problem

[Thomas Renninger]

On Fri, 2007-08-03 at 08:08 +1000, Rusty Russell wrote:
> On Thu, 2007-08-02 at 09:25 -0700, Luck, Tony wrote:
> > > Adrian Bunk: scripts/mod/file2alias.c is compiled with HOSTCC and ensures that
> > > kernel_ulong_t is correct, but it can't cope with different padding on
> > > different architectures.
> > 
> > Surely this is the root cause ... you can't expect that the alignment
> > rules of HOSTCC to make any sense for an arbitraty target.
> > 
> > > +#define FILLUP_LEN	7 /* dirty fix for i386 -> 64bit cross-compilation */
> > >  
> > >  struct acpi_device_id {
> > >  	__u8 id[ACPI_ID_LEN];
> > > +	__u8 dummy[FILLUP_LEN];
> > >  	kernel_ulong_t driver_data;
> > >  };
> > 
> > What's so special about this structure that we get an error?
> 
> It's in mod_devicetable.h: see comment at top of that file.  These
> structures serve dual purpose: to describe the capabilities of the
> driver to the kernel probing functions, *and* to export them to
> userspace tables.  The former purpose is why there's a data pointer in
> there.
> 
> scripts/mod/file2alias is the program that reads this: although it can
> be altered to parse 32-vs-64, Adrian's fix is the simplest.

Oops, this will cause a lot build warnings, as this struct gets
initialized like that:
        ..
        {"PNP0C0A", 0},
        ..
at a lot places. It would be better to bump up the id itself like the
attached patch does...

Thanks,

    Thomas

----------------------

Cross-compilation between e.g. i386 -> 64bit could break -> work around it

Adrian Bunk: scripts/mod/file2alias.c is compiled with HOSTCC and ensures that
kernel_ulong_t is correct, but it can't cope with different padding on
different architectures.

Signed-off-by: Thomas Renninger <trenn@suse.de>

---
 include/linux/mod_devicetable.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux-2.6.23-rc3/include/linux/mod_devicetable.h
=================================--- linux-2.6.23-rc3.orig/include/linux/mod_devicetable.h
+++ linux-2.6.23-rc3/include/linux/mod_devicetable.h
@@ -159,7 +159,8 @@ struct ap_device_id {
 
 #define AP_DEVICE_ID_MATCH_DEVICE_TYPE		0x01
 
-#define ACPI_ID_LEN	9
+#define ACPI_ID_LEN	16 /* only 9 bytes needed here, 16 bytes are used */
+			   /* to workaround crosscompile issues */
 
 struct acpi_device_id {
 	__u8 id[ACPI_ID_LEN];

RE: scripts/mod/file2alias.c cross compile problem

[Luck, Tony]

> -#define ACPI_ID_LEN	9
> +#define ACPI_ID_LEN	16 /* only 9 bytes needed here, 16 bytes are used */

What will happen if someone uses more than 9 bytes?  With the old
limit there would be a compile time error it someone initialized
with:

	{"PNP0C0ABCDEFGH", 0},

But if we change ACPI_ID_LEN the error will move to run-time.

-Tony

RE: scripts/mod/file2alias.c cross compile problem

[Thomas Renninger]

On Thu, 2007-08-16 at 09:26 -0700, Luck, Tony wrote:
> > -#define ACPI_ID_LEN	9
> > +#define ACPI_ID_LEN	16 /* only 9 bytes needed here, 16 bytes are used */
> 
> What will happen if someone uses more than 9 bytes?  With the old
> limit there would be a compile time error it someone initialized
> with:
> 
> 	{"PNP0C0ABCDEFGH", 0},
> 
> But if we change ACPI_ID_LEN the error will move to run-time.

That should not harm.
From spec point of view only 8 bytes are needed. Here 9 bytes are used
as then string functions can be used. The whole rest of the kernel does
not use ACPI_ID_LEN.
If someone defines such an id, his driver won't ever get loaded and he
gets bug reports very soon anyway.

Hmm, I wonder whether this even may come in handy later:
Some BIOSes have some spec violating strange hids like:
"*PNP0C0A"
or
"_PNP0C0A"
While I thought the "_" or "*" should be cut away by ACPI parser, it
might be a hint to the OS to do something special. AFAIK only very
specific devices (WMI and some graphics?) have such strange hids
defined, those drivers could explicitly match for the spec violating
string then.

   Thomas