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;
};
> 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
"Luck, Tony" <[email protected]> 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, [email protected]
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."
>>> 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
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
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
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
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...
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.
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
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 <[email protected]>
---
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];
> -#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
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