m68k build failure

m68k build failure

[Andrew Morton]

Current Linus tree give me this, with m68k allmodconfig:

FATAL: drivers/bluetooth/btsdio: sizeof(struct sdio_device_id)=12 is not a modulo of the size of section __mod_sdio_device_table=30.
Fix definition of struct sdio_device_id in mod_devicetable.h

which I haven't seen before.  Any ideas?

Thanks.

Re: m68k build failure

[Pierre Ossman]

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

On Tue, 27 Nov 2007 22:07:23 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> 
> Current Linus tree give me this, with m68k allmodconfig:
> 
> FATAL: drivers/bluetooth/btsdio: sizeof(struct sdio_device_id)=12 is not a modulo of the size of section __mod_sdio_device_table=30.
> Fix definition of struct sdio_device_id in mod_devicetable.h
> 
> which I haven't seen before.  Any ideas?
> 

No the slightest. 12 seems like the correct, padded size. A size of 10 is just weird as the unpadded size is 9 bytes. Could you dump the __mod_sdio_device_table section so we can determine if it is cropped or just oddly padded.

Rgds
Pierre

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

Re: m68k build failure

[Andrew Morton]

On Wed, 28 Nov 2007 09:48:56 +0100 Pierre Ossman <drzeus@drzeus.cx> wrote:

> On Tue, 27 Nov 2007 22:07:23 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > 
> > Current Linus tree give me this, with m68k allmodconfig:
> > 
> > FATAL: drivers/bluetooth/btsdio: sizeof(struct sdio_device_id)=12 is not a modulo of the size of section __mod_sdio_device_table=30.
> > Fix definition of struct sdio_device_id in mod_devicetable.h
> > 
> > which I haven't seen before.  Any ideas?
> > 
> 
> No the slightest. 12 seems like the correct, padded size. A size of 10 is just weird as the unpadded size is 9 bytes. Could you dump the __mod_sdio_device_table section so we can determine if it is cropped or just oddly padded.
> 

err, I'd rather not.  I have no shortage of bugs to be going on with here.

http://userweb.kernel.org/~akpm/cross-compilers/ has the i386->m68k
cross-compiler which I use.

Re: m68k build failure

[Al Viro]

On Wed, Nov 28, 2007 at 01:00:56AM -0800, Andrew Morton wrote:
> > No the slightest. 12 seems like the correct, padded size. A size of 10 is just weird as the unpadded size is 9 bytes. Could you dump the __mod_sdio_device_table section so we can determine if it is cropped or just oddly padded.
> > 
> 
> err, I'd rather not.  I have no shortage of bugs to be going on with here.
> 
> http://userweb.kernel.org/~akpm/cross-compilers/ has the i386->m68k
> cross-compiler which I use.

Eh...  m68k has 16bit alignment for unsigned long.  

diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -343,7 +343,8 @@ struct sdio_device_id {
 	__u8	class;			/* Standard interface or SDIO_ANY_ID */
 	__u16	vendor;			/* Vendor or SDIO_ANY_ID */
 	__u16	device;			/* Device ID or SDIO_ANY_ID */
-	kernel_ulong_t driver_data;	/* Data private to the driver */
+	kernel_ulong_t driver_data	/* Data private to the driver */
+		__attribute__((aligned(sizeof(kernel_ulong_t))));
 };
 
 /* SSB core, see drivers/ssb/ */

Re: m68k build failure

[Pierre Ossman]

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

On Wed, 28 Nov 2007 09:28:56 +0000
Al Viro <viro@ftp.linux.org.uk> wrote:

> 
> Eh...  m68k has 16bit alignment for unsigned long.  
> 
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -343,7 +343,8 @@ struct sdio_device_id {
>  	__u8	class;			/* Standard interface or SDIO_ANY_ID */
>  	__u16	vendor;			/* Vendor or SDIO_ANY_ID */
>  	__u16	device;			/* Device ID or SDIO_ANY_ID */
> -	kernel_ulong_t driver_data;	/* Data private to the driver */
> +	kernel_ulong_t driver_data	/* Data private to the driver */
> +		__attribute__((aligned(sizeof(kernel_ulong_t))));
>  };
>  
>  /* SSB core, see drivers/ssb/ */

Unfortunately, that just papers over the symptom and doesn't solve the underlying issue. If you cross-compile on/for an arch with byte alignment, then the issue is back. Or one that uses 4-byte alignment even for u16.

Is there no directive we can stick in there that forces a reasonable alignment (e.g. alignment == sizeof(type)) independently of arch?

Rgds
Pierre

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

Re: m68k build failure

[Geert Uytterhoeven]

On Wed, 28 Nov 2007, Pierre Ossman wrote:
> On Wed, 28 Nov 2007 09:28:56 +0000
> Al Viro <viro@ftp.linux.org.uk> wrote:
> > 
> > Eh...  m68k has 16bit alignment for unsigned long.  
> > 
> > diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> > --- a/include/linux/mod_devicetable.h
> > +++ b/include/linux/mod_devicetable.h
> > @@ -343,7 +343,8 @@ struct sdio_device_id {
> >  	__u8	class;			/* Standard interface or SDIO_ANY_ID */
> >  	__u16	vendor;			/* Vendor or SDIO_ANY_ID */
> >  	__u16	device;			/* Device ID or SDIO_ANY_ID */
> > -	kernel_ulong_t driver_data;	/* Data private to the driver */
> > +	kernel_ulong_t driver_data	/* Data private to the driver */
> > +		__attribute__((aligned(sizeof(kernel_ulong_t))));
> >  };
> >  
> >  /* SSB core, see drivers/ssb/ */
> 
> Unfortunately, that just papers over the symptom and doesn't solve the underlying issue. If you cross-compile on/for an arch with byte alignment, then the issue is back. Or one that uses 4-byte alignment even for u16.
> 
> Is there no directive we can stick in there that forces a reasonable alignment (e.g. alignment == sizeof(type)) independently of arch?

We could use something like is used for compat_*.
E.g. compare compat_s64 in <asm/compat.h> for x86 and powerpc.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: m68k build failure

[Pierre Ossman]

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

On Wed, 28 Nov 2007 13:34:02 +0100 (CET)
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> On Wed, 28 Nov 2007, Pierre Ossman wrote:
> > 
> > Is there no directive we can stick in there that forces a reasonable alignment (e.g. alignment == sizeof(type)) independently of arch?
> 
> We could use something like is used for compat_*.
> E.g. compare compat_s64 in <asm/compat.h> for x86 and powerpc.
> 

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;

and let all structures in mod_devicetable.h use those types.

Now does anyone have the time to code and test this?

Rgds
Pierre

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

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

[Geert Uytterhoeven]

On Sat, 1 Dec 2007, Pierre Ossman wrote:
> On Wed, 28 Nov 2007 13:34:02 +0100 (CET)
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
> > On Wed, 28 Nov 2007, Pierre Ossman wrote:
> > > 
> > > Is there no directive we can stick in there that forces a reasonable alignment (e.g. alignment == sizeof(type)) independently of arch?
> > 
> > We could use something like is used for compat_*.
> > E.g. compare compat_s64 in <asm/compat.h> for x86 and powerpc.
> > 
> 
> 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;
> 
> and let all structures in mod_devicetable.h use those types.
> 
> Now does anyone have the time to code and test this?

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.

diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index e9fddb4..e1f98b1 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -10,15 +10,22 @@
 #ifdef __KERNEL__
 #include <linux/types.h>
 typedef unsigned long kernel_ulong_t;
+typedef u8 kernel_u8;
+typedef u16 kernel_u16;
+typedef u32 kernel_u32;
+#define BITS_PER_KERNEL_LONG	BITS_PER_LONG
 #endif
 
 #define PCI_ANY_ID (~0)
 
 struct pci_device_id {
-	__u32 vendor, device;		/* Vendor and device ID or PCI_ANY_ID*/
-	__u32 subvendor, subdevice;	/* Subsystem ID's or PCI_ANY_ID */
-	__u32 class, class_mask;	/* (class,subclass,prog-if) triplet */
-	kernel_ulong_t driver_data;	/* Data private to the driver */
+	kernel_u32	vendor;		/* Vendor ID or PCI_ANY_ID*/
+	kernel_u32	device;		/* Device ID or PCI_ANY_ID*/
+	kernel_u32	subvendor;	/* Subsystem ID or PCI_ANY_ID */
+	kernel_u32	subdevice;	/* Subsystem ID or PCI_ANY_ID */
+	kernel_u32	class;		/* (class,subclass,prog-if) triplet */
+	kernel_u32	class_mask;	/* (class,subclass,prog-if) triplet */
+	kernel_ulong_t	driver_data;	/* Data private to the driver */
 };
 
 
@@ -28,13 +35,12 @@ struct pci_device_id {
 #define IEEE1394_MATCH_VERSION		0x0008
 
 struct ieee1394_device_id {
-	__u32 match_flags;
-	__u32 vendor_id;
-	__u32 model_id;
-	__u32 specifier_id;
-	__u32 version;
-	kernel_ulong_t driver_data
-		__attribute__((aligned(sizeof(kernel_ulong_t))));
+	kernel_u32	match_flags;
+	kernel_u32	vendor_id;
+	kernel_u32	model_id;
+	kernel_u32	specifier_id;
+	kernel_u32	version;
+	kernel_ulong_t	driver_data;
 };
 
 
@@ -97,23 +103,23 @@ struct ieee1394_device_id {
  */
 struct usb_device_id {
 	/* which fields to match against? */
-	__u16		match_flags;
+	kernel_u16	match_flags;
 
 	/* Used for product specific matches; range is inclusive */
-	__u16		idVendor;
-	__u16		idProduct;
-	__u16		bcdDevice_lo;
-	__u16		bcdDevice_hi;
+	kernel_u16	idVendor;
+	kernel_u16	idProduct;
+	kernel_u16	bcdDevice_lo;
+	kernel_u16	bcdDevice_hi;
 
 	/* Used for device class matches */
-	__u8		bDeviceClass;
-	__u8		bDeviceSubClass;
-	__u8		bDeviceProtocol;
+	kernel_u8	bDeviceClass;
+	kernel_u8	bDeviceSubClass;
+	kernel_u8	bDeviceProtocol;
 
 	/* Used for interface class matches */
-	__u8		bInterfaceClass;
-	__u8		bInterfaceSubClass;
-	__u8		bInterfaceProtocol;
+	kernel_u8	bInterfaceClass;
+	kernel_u8	bInterfaceSubClass;
+	kernel_u8	bInterfaceProtocol;
 
 	/* not matched against */
 	kernel_ulong_t	driver_info;
@@ -133,12 +139,12 @@ struct usb_device_id {
 
 /* s390 CCW devices */
 struct ccw_device_id {
-	__u16	match_flags;	/* which fields to match against */
+	kernel_u16	match_flags;	/* which fields to match against */
 
-	__u16	cu_type;	/* control unit type     */
-	__u16	dev_type;	/* device type           */
-	__u8	cu_model;	/* control unit model    */
-	__u8	dev_model;	/* device model          */
+	kernel_u16	cu_type;	/* control unit type     */
+	kernel_u16	dev_type;	/* device type           */
+	kernel_u8	cu_model;	/* control unit model    */
+	kernel_u8	dev_model;	/* device model          */
 
 	kernel_ulong_t driver_info;
 };
@@ -150,11 +156,11 @@ struct ccw_device_id {
 
 /* s390 AP bus devices */
 struct ap_device_id {
-	__u16 match_flags;	/* which fields to match against */
-	__u8 dev_type;		/* device type */
-	__u8 pad1;
-	__u32 pad2;
-	kernel_ulong_t driver_info;
+	kernel_u16	match_flags;	/* which fields to match against */
+	kernel_u8	dev_type;	/* device type */
+	kernel_u8	pad1;
+	kernel_u32	pad2;
+	kernel_ulong_t	driver_info;
 };
 
 #define AP_DEVICE_ID_MATCH_DEVICE_TYPE		0x01
@@ -163,23 +169,23 @@ struct ap_device_id {
 			   /* to workaround crosscompile issues */
 
 struct acpi_device_id {
-	__u8 id[ACPI_ID_LEN];
-	kernel_ulong_t driver_data;
+	kernel_u8	id[ACPI_ID_LEN];
+	kernel_ulong_t	driver_data;
 };
 
 #define PNP_ID_LEN	8
 #define PNP_MAX_DEVICES	8
 
 struct pnp_device_id {
-	__u8 id[PNP_ID_LEN];
-	kernel_ulong_t driver_data;
+	kernel_u8	id[PNP_ID_LEN];
+	kernel_ulong_t	driver_data;
 };
 
 struct pnp_card_device_id {
-	__u8 id[PNP_ID_LEN];
-	kernel_ulong_t driver_data;
+	kernel_u8	id[PNP_ID_LEN];
+	kernel_ulong_t	driver_data;
 	struct {
-		__u8 id[PNP_ID_LEN];
+		kernel_u8	id[PNP_ID_LEN];
 	} devs[PNP_MAX_DEVICES];
 };
 
@@ -187,10 +193,10 @@ struct pnp_card_device_id {
 #define SERIO_ANY	0xff
 
 struct serio_device_id {
-	__u8 type;
-	__u8 extra;
-	__u8 id;
-	__u8 proto;
+	kernel_u8	type;
+	kernel_u8	extra;
+	kernel_u8	id;
+	kernel_u8	proto;
 };
 
 /*
@@ -198,47 +204,45 @@ struct serio_device_id {
  */
 struct of_device_id
 {
-	char	name[32];
-	char	type[32];
-	char	compatible[128];
+	char		name[32];
+	char		type[32];
+	char		compatible[128];
 #ifdef __KERNEL__
-	void	*data;
+	void *		data;
 #else
-	kernel_ulong_t data;
+	kernel_ulong_t	data;
 #endif
 };
 
 /* VIO */
 struct vio_device_id {
-	char type[32];
-	char compat[32];
+	char		type[32];
+	char		compat[32];
 };
 
 /* PCMCIA */
 
 struct pcmcia_device_id {
-	__u16		match_flags;
+	kernel_u16	match_flags;
 
-	__u16		manf_id;
-	__u16 		card_id;
+	kernel_u16	manf_id;
+	kernel_u16	card_id;
 
-	__u8  		func_id;
+	kernel_u8	func_id;
 
 	/* for real multi-function devices */
-	__u8  		function;
+	kernel_u8	function;
 
 	/* for pseudo multi-function devices */
-	__u8  		device_no;
+	kernel_u8	device_no;
 
-	__u32 		prod_id_hash[4]
-		__attribute__((aligned(sizeof(__u32))));
+	kernel_u32	prod_id_hash[4];
 
 	/* not matched against in kernelspace*/
 #ifdef __KERNEL__
 	const char *	prod_id[4];
 #else
-	kernel_ulong_t	prod_id[4]
-		__attribute__((aligned(sizeof(kernel_ulong_t))));
+	kernel_ulong_t	prod_id[4];
 #endif
 
 	/* not matched against */
@@ -291,24 +295,24 @@ struct pcmcia_device_id {
 
 struct input_device_id {
 
-	kernel_ulong_t flags;
+	kernel_ulong_t	flags;
 
-	__u16 bustype;
-	__u16 vendor;
-	__u16 product;
-	__u16 version;
+	kernel_u16	bustype;
+	kernel_u16	vendor;
+	kernel_u16	product;
+	kernel_u16	version;
 
-	kernel_ulong_t evbit[INPUT_DEVICE_ID_EV_MAX / BITS_PER_LONG + 1];
-	kernel_ulong_t keybit[INPUT_DEVICE_ID_KEY_MAX / BITS_PER_LONG + 1];
-	kernel_ulong_t relbit[INPUT_DEVICE_ID_REL_MAX / BITS_PER_LONG + 1];
-	kernel_ulong_t absbit[INPUT_DEVICE_ID_ABS_MAX / BITS_PER_LONG + 1];
-	kernel_ulong_t mscbit[INPUT_DEVICE_ID_MSC_MAX / BITS_PER_LONG + 1];
-	kernel_ulong_t ledbit[INPUT_DEVICE_ID_LED_MAX / BITS_PER_LONG + 1];
-	kernel_ulong_t sndbit[INPUT_DEVICE_ID_SND_MAX / BITS_PER_LONG + 1];
-	kernel_ulong_t ffbit[INPUT_DEVICE_ID_FF_MAX / BITS_PER_LONG + 1];
-	kernel_ulong_t swbit[INPUT_DEVICE_ID_SW_MAX / BITS_PER_LONG + 1];
+	kernel_ulong_t	evbit[INPUT_DEVICE_ID_EV_MAX / BITS_PER_KERNEL_LONG + 1];
+	kernel_ulong_t	keybit[INPUT_DEVICE_ID_KEY_MAX / BITS_PER_KERNEL_LONG + 1];
+	kernel_ulong_t	relbit[INPUT_DEVICE_ID_REL_MAX / BITS_PER_KERNEL_LONG + 1];
+	kernel_ulong_t	absbit[INPUT_DEVICE_ID_ABS_MAX / BITS_PER_KERNEL_LONG + 1];
+	kernel_ulong_t	mscbit[INPUT_DEVICE_ID_MSC_MAX / BITS_PER_KERNEL_LONG + 1];
+	kernel_ulong_t	ledbit[INPUT_DEVICE_ID_LED_MAX / BITS_PER_KERNEL_LONG + 1];
+	kernel_ulong_t	sndbit[INPUT_DEVICE_ID_SND_MAX / BITS_PER_KERNEL_LONG + 1];
+	kernel_ulong_t	ffbit[INPUT_DEVICE_ID_FF_MAX / BITS_PER_KERNEL_LONG + 1];
+	kernel_ulong_t	swbit[INPUT_DEVICE_ID_SW_MAX / BITS_PER_KERNEL_LONG + 1];
 
-	kernel_ulong_t driver_info;
+	kernel_ulong_t	driver_info;
 };
 
 /* EISA */
@@ -317,17 +321,17 @@ struct input_device_id {
 
 /* The EISA signature, in ASCII form, null terminated */
 struct eisa_device_id {
-	char          sig[EISA_SIG_LEN];
-	kernel_ulong_t driver_data;
+	char		sig[EISA_SIG_LEN];
+	kernel_ulong_t	driver_data;
 };
 
 #define EISA_DEVICE_MODALIAS_FMT "eisa:s%s"
 
 struct parisc_device_id {
-	__u8	hw_type;	/* 5 bits used */
-	__u8	hversion_rev;	/* 4 bits */
-	__u16	hversion;	/* 12 bits */
-	__u32	sversion;	/* 20 bits */
+	kernel_u8	hw_type;	/* 5 bits used */
+	kernel_u8	hversion_rev;	/* 4 bits */
+	kernel_u16	hversion;	/* 12 bits */
+	kernel_u32	sversion;	/* 20 bits */
 };
 
 #define PA_HWTYPE_ANY_ID	0xff
@@ -340,17 +344,17 @@ struct parisc_device_id {
 #define SDIO_ANY_ID (~0)
 
 struct sdio_device_id {
-	__u8	class;			/* Standard interface or SDIO_ANY_ID */
-	__u16	vendor;			/* Vendor or SDIO_ANY_ID */
-	__u16	device;			/* Device ID or SDIO_ANY_ID */
-	kernel_ulong_t driver_data;	/* Data private to the driver */
+	kernel_u8	class;		/* Standard interface or SDIO_ANY_ID */
+	kernel_u16	vendor;		/* Vendor or SDIO_ANY_ID */
+	kernel_u16	device;		/* Device ID or SDIO_ANY_ID */
+	kernel_ulong_t	driver_data;	/* Data private to the driver */
 };
 
 /* SSB core, see drivers/ssb/ */
 struct ssb_device_id {
-	__u16	vendor;
-	__u16	coreid;
-	__u8	revision;
+	kernel_u16	vendor;
+	kernel_u16	coreid;
+	kernel_u8	revision;
 };
 #define SSB_DEVICE(_vendor, _coreid, _revision)  \
 	{ .vendor = _vendor, .coreid = _coreid, .revision = _revision, }
@@ -362,8 +366,8 @@ struct ssb_device_id {
 #define SSB_ANY_REV		0xFF
 
 struct virtio_device_id {
-	__u32 device;
-	__u32 vendor;
+	kernel_u32	device;
+	kernel_u32	vendor;
 };
 #define VIRTIO_DEV_ANY_ID	0xffffffff
 
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index d802b5a..4c95e53 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -12,15 +12,6 @@
 
 #include "modpost.h"
 
-/* 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
 #ifdef __sun__
 #include <inttypes.h>
 #else
@@ -29,13 +20,10 @@ typedef Elf64_Addr	kernel_ulong_t;
 
 #include <ctype.h>
 
-typedef uint32_t	__u32;
-typedef uint16_t	__u16;
-typedef unsigned char	__u8;
-
 /* Big exception to the "don't include kernel headers into userspace, which
  * even potentially has different endianness and word sizes, since
  * we handle those differences explicitly below */
+#include "../../include/asm/kerneltypes.h"
 #include "../../include/linux/mod_devicetable.h"
 
 #define ADD(str, sep, cond, field)                              \
@@ -422,7 +410,8 @@ static void do_input(char *alias,
 	unsigned int i;
 
 	for (i = min; i < max; i++)
-		if (arr[i / BITS_PER_LONG] & (1L << (i%BITS_PER_LONG)))
+		if (arr[i / BITS_PER_KERNEL_LONG] &
+		    (1L << (i%BITS_PER_KERNEL_LONG)))
 			sprintf(alias + strlen(alias), "%X,*", i);
 }
 
@@ -504,9 +493,9 @@ static int do_sdio_entry(const char *filename,
 	id->device = TO_NATIVE(id->device);
 
 	strcpy(alias, "sdio:");
-	ADD(alias, "c", id->class != (__u8)SDIO_ANY_ID, id->class);
-	ADD(alias, "v", id->vendor != (__u16)SDIO_ANY_ID, id->vendor);
-	ADD(alias, "d", id->device != (__u16)SDIO_ANY_ID, id->device);
+	ADD(alias, "c", id->class != (kernel_u8)SDIO_ANY_ID, id->class);
+	ADD(alias, "v", id->vendor != (kernel_u16)SDIO_ANY_ID, id->vendor);
+	ADD(alias, "d", id->device != (kernel_u16)SDIO_ANY_ID, id->device);
 	return 1;
 }
 
--- /dev/null	2007-11-25 11:21:46.692011602 +0100
+++ b/include/asm-m68k/kerneltypes.h	2007-12-02 12:03:25.000000000 +0100
@@ -0,0 +1,17 @@
+#ifndef _ASM_M68K_KERNELTYPES_H
+#define _ASM_M68K_KERNELTYPES_H
+
+/*
+ * Architecture specific types and alignment, suitable for cross-compiling
+ */
+
+#include <stdint.h>
+
+typedef uint32_t __attribute__((aligned(2))) kernel_ulong_t;
+typedef uint8_t __attribute__((aligned(1))) kernel_u8;
+typedef uint16_t __attribute__((aligned(2))) kernel_u16;
+typedef uint32_t __attribute__((aligned(2))) kernel_u32;
+
+#define BITS_PER_KERNEL_LONG	32
+
+#endif /* _ASM_M68K_KERNELTYPES_H */

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

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

[Rusty Russell]

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).

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 */".

Thanks,
Rusty.

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: 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)

[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.

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)

[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: m68k build failure

[Andrew Morton]

On Wed, 28 Nov 2007 09:28:56 +0000
Al Viro <viro@ftp.linux.org.uk> wrote:

> On Wed, Nov 28, 2007 at 01:00:56AM -0800, Andrew Morton wrote:
> > > No the slightest. 12 seems like the correct, padded size. A size of 10 is just weird as the unpadded size is 9 bytes. Could you dump the __mod_sdio_device_table section so we can determine if it is cropped or just oddly padded.
> > > 
> > 
> > err, I'd rather not.  I have no shortage of bugs to be going on with here.
> > 
> > http://userweb.kernel.org/~akpm/cross-compilers/ has the i386->m68k
> > cross-compiler which I use.
> 
> Eh...  m68k has 16bit alignment for unsigned long.  
> 
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -343,7 +343,8 @@ struct sdio_device_id {
>  	__u8	class;			/* Standard interface or SDIO_ANY_ID */
>  	__u16	vendor;			/* Vendor or SDIO_ANY_ID */
>  	__u16	device;			/* Device ID or SDIO_ANY_ID */
> -	kernel_ulong_t driver_data;	/* Data private to the driver */
> +	kernel_ulong_t driver_data	/* Data private to the driver */
> +		__attribute__((aligned(sizeof(kernel_ulong_t))));
>  };
>  
>  /* SSB core, see drivers/ssb/ */

That works, but it's a bit obscure.

sdio_device_id didn't exist in 2.6.23 so we still have time to turn it into
some saner layout.

Who owns this code, anwyay?

Re: m68k build failure

[Adrian Bunk]

On Thu, Nov 29, 2007 at 01:19:42PM -0800, Andrew Morton wrote:
> On Wed, 28 Nov 2007 09:28:56 +0000
> Al Viro <viro@ftp.linux.org.uk> wrote:
> 
> > On Wed, Nov 28, 2007 at 01:00:56AM -0800, Andrew Morton wrote:
> > > > No the slightest. 12 seems like the correct, padded size. A size of 10 is just weird as the unpadded size is 9 bytes. Could you dump the __mod_sdio_device_table section so we can determine if it is cropped or just oddly padded.
> > > > 
> > > 
> > > err, I'd rather not.  I have no shortage of bugs to be going on with here.
> > > 
> > > http://userweb.kernel.org/~akpm/cross-compilers/ has the i386->m68k
> > > cross-compiler which I use.
> > 
> > Eh...  m68k has 16bit alignment for unsigned long.  
> > 
> > diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> > --- a/include/linux/mod_devicetable.h
> > +++ b/include/linux/mod_devicetable.h
> > @@ -343,7 +343,8 @@ struct sdio_device_id {
> >  	__u8	class;			/* Standard interface or SDIO_ANY_ID */
> >  	__u16	vendor;			/* Vendor or SDIO_ANY_ID */
> >  	__u16	device;			/* Device ID or SDIO_ANY_ID */
> > -	kernel_ulong_t driver_data;	/* Data private to the driver */
> > +	kernel_ulong_t driver_data	/* Data private to the driver */
> > +		__attribute__((aligned(sizeof(kernel_ulong_t))));
> >  };
> >  
> >  /* SSB core, see drivers/ssb/ */
> 
> That works, but it's a bit obscure.
> 
> sdio_device_id didn't exist in 2.6.23 so we still have time to turn it into
> some saner layout.
> 
> Who owns this code, anwyay?

Rusty Russell (Cc'ed)


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: m68k build failure

[Geert Uytterhoeven]

On Wed, 28 Nov 2007, Pierre Ossman wrote:
> On Tue, 27 Nov 2007 22:07:23 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
> > Current Linus tree give me this, with m68k allmodconfig:
> > 
> > FATAL: drivers/bluetooth/btsdio: sizeof(struct sdio_device_id)=12 is not a modulo of the size of section __mod_sdio_device_table=30.
> > Fix definition of struct sdio_device_id in mod_devicetable.h
> > 
> > which I haven't seen before.  Any ideas?
> 
> No the slightest. 12 seems like the correct, padded size. A size of 10 is just weird as the unpadded size is 9 bytes. Could you dump the __mod_sdio_device_table section so we can determine if it is cropped or just oddly padded.

10 is correct. On m68k, the natural alignment of quantities larger than
one byte is 2 bytes, not 4 bytes.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: m68k build failure

[Geert Uytterhoeven]

On Wed, 28 Nov 2007, Geert Uytterhoeven wrote:
> On Wed, 28 Nov 2007, Pierre Ossman wrote:
> > On Tue, 27 Nov 2007 22:07:23 -0800
> > Andrew Morton <akpm@linux-foundation.org> wrote:
> > > Current Linus tree give me this, with m68k allmodconfig:
> > > 
> > > FATAL: drivers/bluetooth/btsdio: sizeof(struct sdio_device_id)=12 is not a modulo of the size of section __mod_sdio_device_table=30.
> > > Fix definition of struct sdio_device_id in mod_devicetable.h
> > > 
> > > which I haven't seen before.  Any ideas?
> > 
> > No the slightest. 12 seems like the correct, padded size. A size of 10 is just weird as the unpadded size is 9 bytes. Could you dump the __mod_sdio_device_table section so we can determine if it is cropped or just oddly padded.
> 
> 10 is correct. On m68k, the natural alignment of quantities larger than
> one byte is 2 bytes, not 4 bytes.

So the problem is in scripts/mod/file2alias.c, which gives a different
sizeof(struct sdio_device_id) on the cross-compile host:
  - sizeof(struct sdio_device_id) = 12 on ia32
  - sizeof(struct sdio_device_id) = 10 on m68k

While file2alias.c has code to handle 32 vs. 64 bit correctly when
cross-compiling, it doesn't handle alignment differences between host
and target.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: m68k build failure

[Pierre Ossman]

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

On Wed, 28 Nov 2007 10:27:18 +0100 (CET)
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> 
> So the problem is in scripts/mod/file2alias.c, which gives a different
> sizeof(struct sdio_device_id) on the cross-compile host:
>   - sizeof(struct sdio_device_id) = 12 on ia32
>   - sizeof(struct sdio_device_id) = 10 on m68k
> 
> While file2alias.c has code to handle 32 vs. 64 bit correctly when
> cross-compiling, it doesn't handle alignment differences between host
> and target.
> 

Delightful. So what are the options here? Start packing the device table structs is the obvious quick fix. Declaring cross-compilation unsupported isn't really viable, and I guess determining padding differences is far from easy.

Rgds
Pierre

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

Re: m68k build failure

[Alan Cox]

> Delightful. So what are the options here? Start packing the device table structs is the obvious quick fix. Declaring cross-compilation unsupported isn't really viable, and I guess determining padding differences is far from easy.

There are some ugly options:

Cross compile a test object containing nothing but

	struct whatever fred;

then dump it with the relevant cross nm

Re: m68k build failure

[Andreas Schwab]

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

>> Delightful. So what are the options here? Start packing the device table structs is the obvious quick fix. Declaring cross-compilation unsupported isn't really viable, and I guess determining padding differences is far from easy.
>
> There are some ugly options:
>
> Cross compile a test object containing nothing but
>
> 	struct whatever fred;
>
> then dump it with the relevant cross nm

Everything would be much easier if all those driver_data members of the
device table structs would not be there.

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: m68k build failure

[Andreas Schwab]

Pierre Ossman <drzeus@drzeus.cx> writes:

> On Tue, 27 Nov 2007 22:07:23 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
>
>> 
>> Current Linus tree give me this, with m68k allmodconfig:
>> 
>> FATAL: drivers/bluetooth/btsdio: sizeof(struct sdio_device_id)=12 is not a modulo of the size of section __mod_sdio_device_table=30.
>> Fix definition of struct sdio_device_id in mod_devicetable.h
>> 
>> which I haven't seen before.  Any ideas?
>> 
>
> No the slightest. 12 seems like the correct, padded size. A size of 10 is just weird as the unpadded size is 9 bytes. Could you dump the __mod_sdio_device_table section so we can determine if it is cropped or just oddly padded.

A size of 10 is correct.  On m68k no type is aligned to more than 2
bytes.

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."