[RFC] [PATCH] Fix b43 driver build for arm

[RFC] [PATCH] Fix b43 driver build for arm

[Gordon Farquharson]

The b43 driver in 2.6.25-rc[12] fails to build for arm on an x86_64
box using a cross-compiler:

FATAL: drivers/net/wireless/b43/b43: sizeof(struct ssb_device_id)=6 is
not a modulo of the size of section __mod_ssb_device_table=64.
Fix definition of struct ssb_device_id in mod_devicetable.h

The following patch fixes the build, but given the discussion in
regarding the fix for the module device table definition for m68k [1],
I'm not sure that this patch is the right thing to do. However, the
fix for m68k was implemented in 2.6.25 [2].

diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 139d49d..0471294 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -351,7 +351,8 @@ struct sdio_device_id {
 struct ssb_device_id {
 	__u16	vendor;
 	__u16	coreid;
-	__u8	revision;
+	__u8	revision
+		__attribute__((aligned(sizeof(__u32))));
 };
 #define SSB_DEVICE(_vendor, _coreid, _revision)  \
 	{ .vendor = _vendor, .coreid = _coreid, .revision = _revision, }

Please CC me on replies as I'm not subscribed to the list.

Gordon

[1] http://lkml.org/lkml/2007/11/28/12
[2] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7492d4a416d68ab4bd254b36ffcc4e0138daa8ff

-- 
Gordon Farquharson
GnuPG Key ID: 32D6D676

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Michael Buesch]

On Monday 18 February 2008 23:03:10 Gordon Farquharson wrote:
> The b43 driver in 2.6.25-rc[12] fails to build for arm on an x86_64
> box using a cross-compiler:
> 
> FATAL: drivers/net/wireless/b43/b43: sizeof(struct ssb_device_id)=6 is
> not a modulo of the size of section __mod_ssb_device_table=64.
> Fix definition of struct ssb_device_id in mod_devicetable.h

Why does ARM have this special requirement and what is it about?

-- 
Greetings Michael.

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Russell King]

On Mon, Feb 18, 2008 at 11:08:56PM +0100, Michael Buesch wrote:
> On Monday 18 February 2008 23:03:10 Gordon Farquharson wrote:
> > The b43 driver in 2.6.25-rc[12] fails to build for arm on an x86_64
> > box using a cross-compiler:
> > 
> > FATAL: drivers/net/wireless/b43/b43: sizeof(struct ssb_device_id)=6 is
> > not a modulo of the size of section __mod_ssb_device_table=64.
> > Fix definition of struct ssb_device_id in mod_devicetable.h
> 
> Why does ARM have this special requirement and what is it about?

Because ARM is a 32 bit architecture.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Michael Buesch]

On Monday 18 February 2008 23:13:24 Russell King wrote:
> On Mon, Feb 18, 2008 at 11:08:56PM +0100, Michael Buesch wrote:
> > On Monday 18 February 2008 23:03:10 Gordon Farquharson wrote:
> > > The b43 driver in 2.6.25-rc[12] fails to build for arm on an x86_64
> > > box using a cross-compiler:
> > > 
> > > FATAL: drivers/net/wireless/b43/b43: sizeof(struct ssb_device_id)=6 is
> > > not a modulo of the size of section __mod_ssb_device_table=64.
> > > Fix definition of struct ssb_device_id in mod_devicetable.h
> > 
> > Why does ARM have this special requirement and what is it about?
> 
> Because ARM is a 32 bit architecture.

Ehm, I didn't see this warning on any other architecture nor did we
have _any_ problem with it on any other architecture.
This code runs fine on x86_32, x86_64, powerpc and mips, at least.

-- 
Greetings Michael.

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Russell King]

On Mon, Feb 18, 2008 at 11:24:44PM +0100, Michael Buesch wrote:
> On Monday 18 February 2008 23:13:24 Russell King wrote:
> > On Mon, Feb 18, 2008 at 11:08:56PM +0100, Michael Buesch wrote:
> > > On Monday 18 February 2008 23:03:10 Gordon Farquharson wrote:
> > > > The b43 driver in 2.6.25-rc[12] fails to build for arm on an x86_64
> > > > box using a cross-compiler:
> > > > 
> > > > FATAL: drivers/net/wireless/b43/b43: sizeof(struct ssb_device_id)=6 is
> > > > not a modulo of the size of section __mod_ssb_device_table=64.
> > > > Fix definition of struct ssb_device_id in mod_devicetable.h
> > > 
> > > Why does ARM have this special requirement and what is it about?
> > 
> > Because ARM is a 32 bit architecture.
> 
> Ehm, I didn't see this warning on any other architecture nor did we
> have _any_ problem with it on any other architecture.
> This code runs fine on x86_32, x86_64, powerpc and mips, at least.

Well, don't expect this driver to work until you fix your broken
assumptions about alignment requirements.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Michael Buesch]

On Monday 18 February 2008 23:34:10 Russell King wrote:
> On Mon, Feb 18, 2008 at 11:24:44PM +0100, Michael Buesch wrote:
> > On Monday 18 February 2008 23:13:24 Russell King wrote:
> > > On Mon, Feb 18, 2008 at 11:08:56PM +0100, Michael Buesch wrote:
> > > > On Monday 18 February 2008 23:03:10 Gordon Farquharson wrote:
> > > > > The b43 driver in 2.6.25-rc[12] fails to build for arm on an x86_64
> > > > > box using a cross-compiler:
> > > > > 
> > > > > FATAL: drivers/net/wireless/b43/b43: sizeof(struct ssb_device_id)=6 is
> > > > > not a modulo of the size of section __mod_ssb_device_table=64.
> > > > > Fix definition of struct ssb_device_id in mod_devicetable.h
> > > > 
> > > > Why does ARM have this special requirement and what is it about?
> > > 
> > > Because ARM is a 32 bit architecture.
> > 
> > Ehm, I didn't see this warning on any other architecture nor did we
> > have _any_ problem with it on any other architecture.
> > This code runs fine on x86_32, x86_64, powerpc and mips, at least.
> 
> Well, don't expect this driver to work until you fix your broken
> assumptions about alignment requirements.

Mr King, I'm not an idiot!

Can you _please_ explain what makes ARM so special here?
Why can't we have an array of this structure on ARM?

struct ssb_device_id {
       __u16   vendor;
       __u16   coreid;
       __u8    revision;
};

I will not apply any patches that I don't understand.
Why doesn't the compiler handle this? What's special? Can you please explain?

-- 
Greetings Michael.

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Harvey Harrison]

On Mon, 2008-02-18 at 23:43 +0100, Michael Buesch wrote:
> On Monday 18 February 2008 23:34:10 Russell King wrote:
> > 
> > Well, don't expect this driver to work until you fix your broken
> > assumptions about alignment requirements.
> 
> Mr King, I'm not an idiot!
> 
> Can you _please_ explain what makes ARM so special here?
> Why can't we have an array of this structure on ARM?
> 
> struct ssb_device_id {
>        __u16   vendor;
>        __u16   coreid;
>        __u8    revision;
> };
> 
> I will not apply any patches that I don't understand.
> Why doesn't the compiler handle this? What's special? Can you please explain?
> 

I believe this is a good place to start (although I could be totally
off-base)

http://lkml.org/lkml/2007/11/22/120

Harvey

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Michael Buesch]

On Monday 18 February 2008 23:50:30 Harvey Harrison wrote:
> On Mon, 2008-02-18 at 23:43 +0100, Michael Buesch wrote:
> > On Monday 18 February 2008 23:34:10 Russell King wrote:
> > > 
> > > Well, don't expect this driver to work until you fix your broken
> > > assumptions about alignment requirements.
> > 
> > Mr King, I'm not an idiot!
> > 
> > Can you _please_ explain what makes ARM so special here?
> > Why can't we have an array of this structure on ARM?
> > 
> > struct ssb_device_id {
> >        __u16   vendor;
> >        __u16   coreid;
> >        __u8    revision;
> > };
> > 
> > I will not apply any patches that I don't understand.
> > Why doesn't the compiler handle this? What's special? Can you please explain?
> > 
> 
> I believe this is a good place to start (although I could be totally
> off-base)
> 
> http://lkml.org/lkml/2007/11/22/120

I know very well what unaligned access is. As I said the code
works on MIPS, which can't do unaligned accesses.

The _real_ question is, why doesn't align the compiler the stuff properly
on ARM? It does the right thing on x86_32/64, powerpc and MIPS. Why doesn't
it do the right thing on ARM and we have to manually align stuff?

See section "Code that doesn't cause unaligned access"

-- 
Greetings Michael.

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Russell King]

On Mon, Feb 18, 2008 at 11:43:12PM +0100, Michael Buesch wrote:
> On Monday 18 February 2008 23:34:10 Russell King wrote:
> > On Mon, Feb 18, 2008 at 11:24:44PM +0100, Michael Buesch wrote:
> > > On Monday 18 February 2008 23:13:24 Russell King wrote:
> > > > On Mon, Feb 18, 2008 at 11:08:56PM +0100, Michael Buesch wrote:
> > > > > On Monday 18 February 2008 23:03:10 Gordon Farquharson wrote:
> > > > > > The b43 driver in 2.6.25-rc[12] fails to build for arm on an x86_64
> > > > > > box using a cross-compiler:
> > > > > > 
> > > > > > FATAL: drivers/net/wireless/b43/b43: sizeof(struct ssb_device_id)=6 is
> > > > > > not a modulo of the size of section __mod_ssb_device_table=64.
> > > > > > Fix definition of struct ssb_device_id in mod_devicetable.h
> > > > > 
> > > > > Why does ARM have this special requirement and what is it about?
> > > > 
> > > > Because ARM is a 32 bit architecture.
> > > 
> > > Ehm, I didn't see this warning on any other architecture nor did we
> > > have _any_ problem with it on any other architecture.
> > > This code runs fine on x86_32, x86_64, powerpc and mips, at least.
> > 
> > Well, don't expect this driver to work until you fix your broken
> > assumptions about alignment requirements.
> 
> Mr King, I'm not an idiot!

I get extremely pissed off everytime I have to try to explain random
alignment issues to people.  "It doesn't work like i386 so it must be
broken" is a rediculous position to take.

> Can you _please_ explain what makes ARM so special here?

No because I don't really know.

> Why can't we have an array of this structure on ARM?
> 
> struct ssb_device_id {
>        __u16   vendor;

2 bytes

>        __u16   coreid;

2 bytes

>        __u8    revision;

1 byte

> };

and therefore sizeof this structure will be 5 bytes, but because of the
ABI rules (which are _explicitly_ allowed by the C standard), it'll
become 8 bytes due to padding afterwards.

At a _guess_ and its only a guess, the linker will enforce this rule
between compilation units, otherwise the implications are disgusting
(would probably result in all loads having to be individual byte loads
and instructions to combine the result - since ARM has strict alignment
requirements.)

What I can say is that the ABI will not be changed because someone in the
kernel decides they don't like it.  So the options are: either fix it so
it works, or accept that the code is broken and will never work on ARM.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Russell King]

On Mon, Feb 18, 2008 at 10:53:54PM +0000, Russell King wrote:
> On Mon, Feb 18, 2008 at 11:43:12PM +0100, Michael Buesch wrote:
> > On Monday 18 February 2008 23:34:10 Russell King wrote:
> > > On Mon, Feb 18, 2008 at 11:24:44PM +0100, Michael Buesch wrote:
> > > > On Monday 18 February 2008 23:13:24 Russell King wrote:
> > > > > On Mon, Feb 18, 2008 at 11:08:56PM +0100, Michael Buesch wrote:
> > > > > > On Monday 18 February 2008 23:03:10 Gordon Farquharson wrote:
> > > > > > > The b43 driver in 2.6.25-rc[12] fails to build for arm on an x86_64
> > > > > > > box using a cross-compiler:
> > > > > > > 
> > > > > > > FATAL: drivers/net/wireless/b43/b43: sizeof(struct ssb_device_id)=6 is
> > > > > > > not a modulo of the size of section __mod_ssb_device_table=64.
> > > > > > > Fix definition of struct ssb_device_id in mod_devicetable.h
> > > > > > 
> > > > > > Why does ARM have this special requirement and what is it about?
> > > > > 
> > > > > Because ARM is a 32 bit architecture.
> > > > 
> > > > Ehm, I didn't see this warning on any other architecture nor did we
> > > > have _any_ problem with it on any other architecture.
> > > > This code runs fine on x86_32, x86_64, powerpc and mips, at least.
> > > 
> > > Well, don't expect this driver to work until you fix your broken
> > > assumptions about alignment requirements.
> > 
> > Mr King, I'm not an idiot!
> 
> I get extremely pissed off everytime I have to try to explain random
> alignment issues to people.  "It doesn't work like i386 so it must be
> broken" is a rediculous position to take.
> 
> > Can you _please_ explain what makes ARM so special here?
> 
> No because I don't really know.
> 
> > Why can't we have an array of this structure on ARM?
> > 
> > struct ssb_device_id {
> >        __u16   vendor;
> 
> 2 bytes
> 
> >        __u16   coreid;
> 
> 2 bytes
> 
> >        __u8    revision;
> 
> 1 byte
> 
> > };
> 
> and therefore sizeof this structure will be 5 bytes, but because of the
> ABI rules (which are _explicitly_ allowed by the C standard), it'll
> become 8 bytes due to padding afterwards.

Another guess might be that, if using AEABI, this structure might
be 6 bytes in size, but the linker will align structures to 4 bytes.

As I say, this is all guess work.  I don't know for sure.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Michael Buesch]

On Tuesday 19 February 2008 00:00:58 Russell King wrote:
> > > Why can't we have an array of this structure on ARM?
> > > 
> > > struct ssb_device_id {
> > >        __u16   vendor;
> > 
> > 2 bytes
> > 
> > >        __u16   coreid;
> > 
> > 2 bytes
> > 
> > >        __u8    revision;
> > 
> > 1 byte
> > 
> > > };
> > 
> > and therefore sizeof this structure will be 5 bytes, but because of the
> > ABI rules (which are _explicitly_ allowed by the C standard), it'll
> > become 8 bytes due to padding afterwards.
> 
> Another guess might be that, if using AEABI, this structure might
> be 6 bytes in size, but the linker will align structures to 4 bytes.

If the struct is padded to 6 bytes and the linker aligns it to 4 byte
everything will be naturally aligned, as far as I can see.

> FATAL: drivers/net/wireless/b43/b43: sizeof(struct ssb_device_id)=6 is
> not a modulo of the size of section __mod_ssb_device_table=64.
> Fix definition of struct ssb_device_id in mod_devicetable.h

So this message tells me the table size is 64 bytes. There are 8 entries,
so it seems the structure is padded to 8 bytes.
But above that it says that sizeof(struct ssb_device_id)=6

IMO this sanity check is broken and not the code.

Where does this sanity check message come from? The linker?

-- 
Greetings Michael.

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Sam Ravnborg]

On Tue, Feb 19, 2008 at 12:17:04AM +0100, Michael Buesch wrote:
> On Tuesday 19 February 2008 00:00:58 Russell King wrote:
> > > > Why can't we have an array of this structure on ARM?
> > > > 
> > > > struct ssb_device_id {
> > > >        __u16   vendor;
> > > 
> > > 2 bytes
> > > 
> > > >        __u16   coreid;
> > > 
> > > 2 bytes
> > > 
> > > >        __u8    revision;
> > > 
> > > 1 byte
> > > 
> > > > };
> > > 
> > > and therefore sizeof this structure will be 5 bytes, but because of the
> > > ABI rules (which are _explicitly_ allowed by the C standard), it'll
> > > become 8 bytes due to padding afterwards.
> > 
> > Another guess might be that, if using AEABI, this structure might
> > be 6 bytes in size, but the linker will align structures to 4 bytes.
> 
> If the struct is padded to 6 bytes and the linker aligns it to 4 byte
> everything will be naturally aligned, as far as I can see.
> 
> > FATAL: drivers/net/wireless/b43/b43: sizeof(struct ssb_device_id)=6 is
> > not a modulo of the size of section __mod_ssb_device_table=64.
> > Fix definition of struct ssb_device_id in mod_devicetable.h
> 
> So this message tells me the table size is 64 bytes. There are 8 entries,
> so it seems the structure is padded to 8 bytes.
> But above that it says that sizeof(struct ssb_device_id)=6
> 
> IMO this sanity check is broken and not the code.
> 
> Where does this sanity check message come from? The linker?
$ git grep 'not a modulo'
scripts/mod/file2alias.c:               fatal("%s: sizeof(struct %s_device_id)=%lu is not a modulo "

It is a consistencycheck between host and target
layout of data.
You need to pad the structure so it becomes 8 byte in size.

	Sam

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Michael Buesch]

On Tuesday 19 February 2008 00:42:12 Sam Ravnborg wrote:
> On Tue, Feb 19, 2008 at 12:17:04AM +0100, Michael Buesch wrote:
> > On Tuesday 19 February 2008 00:00:58 Russell King wrote:
> > > > > Why can't we have an array of this structure on ARM?
> > > > > 
> > > > > struct ssb_device_id {
> > > > >        __u16   vendor;
> > > > 
> > > > 2 bytes
> > > > 
> > > > >        __u16   coreid;
> > > > 
> > > > 2 bytes
> > > > 
> > > > >        __u8    revision;
> > > > 
> > > > 1 byte
> > > > 
> > > > > };
> > > > 
> > > > and therefore sizeof this structure will be 5 bytes, but because of the
> > > > ABI rules (which are _explicitly_ allowed by the C standard), it'll
> > > > become 8 bytes due to padding afterwards.
> > > 
> > > Another guess might be that, if using AEABI, this structure might
> > > be 6 bytes in size, but the linker will align structures to 4 bytes.
> > 
> > If the struct is padded to 6 bytes and the linker aligns it to 4 byte
> > everything will be naturally aligned, as far as I can see.
> > 
> > > FATAL: drivers/net/wireless/b43/b43: sizeof(struct ssb_device_id)=6 is
> > > not a modulo of the size of section __mod_ssb_device_table=64.
> > > Fix definition of struct ssb_device_id in mod_devicetable.h
> > 
> > So this message tells me the table size is 64 bytes. There are 8 entries,
> > so it seems the structure is padded to 8 bytes.
> > But above that it says that sizeof(struct ssb_device_id)=6
> > 
> > IMO this sanity check is broken and not the code.
> > 
> > Where does this sanity check message come from? The linker?
> $ git grep 'not a modulo'
> scripts/mod/file2alias.c:               fatal("%s: sizeof(struct %s_device_id)=%lu is not a modulo "
> 
> It is a consistencycheck between host and target
> layout of data.
> You need to pad the structure so it becomes 8 byte in size.

Ok, I looked at the code and it is hightly questionable to me that this
check does work in a crosscompile environment (which the ARM build
most likely is).

It seems to check the size of the structure in the .o file against
the size of the structure on the _host_ where it is compiled.
I can't see why we would want to check _anything_ of the target stuff
to the host this stuff is compiled on.
I can compile an ARM kernel on any machine I want.

There actually is a comment:
 * Check that sizeof(device_id type) are consistent with size of section
 * in .o file. If in-consistent then userspace and kernel does not agree
 * on actual size which is a bug.

So it seems what this check _wants_ to compare the sizeof the structure
in the kernel to the size of the stucture in the userland of the target system.
But it does _not_ do that.
It does compare the size of the structure in the kernel against the size of
the stucture in userland on the machine it is _compiled_ on.
That is wrong.

-- 
Greetings Michael.

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Gordon Farquharson]

On Feb 18, 2008 5:01 PM, Michael Buesch <mb@bu3sch.de> wrote:
>
> On Tuesday 19 February 2008 00:42:12 Sam Ravnborg wrote:
> > On Tue, Feb 19, 2008 at 12:17:04AM +0100, Michael Buesch wrote:
> > > On Tuesday 19 February 2008 00:00:58 Russell King wrote:
> > > > > > Why can't we have an array of this structure on ARM?
> > > > > >
> > > > > > struct ssb_device_id {
> > > > > >        __u16   vendor;
> > > > >
> > > > > 2 bytes
> > > > >
> > > > > >        __u16   coreid;
> > > > >
> > > > > 2 bytes
> > > > >
> > > > > >        __u8    revision;
> > > > >
> > > > > 1 byte
> > > > >
> > > > > > };
> > > > >
> > > > > and therefore sizeof this structure will be 5 bytes, but because of the
> > > > > ABI rules (which are _explicitly_ allowed by the C standard), it'll
> > > > > become 8 bytes due to padding afterwards.
> > > >
> > > > Another guess might be that, if using AEABI, this structure might
> > > > be 6 bytes in size, but the linker will align structures to 4 bytes.
> > >
> > > If the struct is padded to 6 bytes and the linker aligns it to 4 byte
> > > everything will be naturally aligned, as far as I can see.
> > >
> > > > FATAL: drivers/net/wireless/b43/b43: sizeof(struct ssb_device_id)=6 is
> > > > not a modulo of the size of section __mod_ssb_device_table=64.
> > > > Fix definition of struct ssb_device_id in mod_devicetable.h
> > >
> > > So this message tells me the table size is 64 bytes. There are 8 entries,
> > > so it seems the structure is padded to 8 bytes.
> > > But above that it says that sizeof(struct ssb_device_id)=6
> > >
> > > IMO this sanity check is broken and not the code.
> > >
> > > Where does this sanity check message come from? The linker?
> > $ git grep 'not a modulo'
> > scripts/mod/file2alias.c:               fatal("%s: sizeof(struct %s_device_id)=%lu is not a modulo "
> >
> > It is a consistencycheck between host and target
> > layout of data.
> > You need to pad the structure so it becomes 8 byte in size.
>
> Ok, I looked at the code and it is hightly questionable to me that this
> check does work in a crosscompile environment (which the ARM build
> most likely is).
>
> It seems to check the size of the structure in the .o file against
> the size of the structure on the _host_ where it is compiled.
> I can't see why we would want to check _anything_ of the target stuff
> to the host this stuff is compiled on.
> I can compile an ARM kernel on any machine I want.
>
> There actually is a comment:
>  * Check that sizeof(device_id type) are consistent with size of section
>  * in .o file. If in-consistent then userspace and kernel does not agree
>  * on actual size which is a bug.
>
> So it seems what this check _wants_ to compare the sizeof the structure
> in the kernel to the size of the stucture in the userland of the target system.
> But it does _not_ do that.
> It does compare the size of the structure in the kernel against the size of
> the stucture in userland on the machine it is _compiled_ on.
> That is wrong.

Does this thread [1] provide any clues as to the Right Thing (TM) to do?

It should be noted that Linus and Andrew signed off on the m68k fix
[2]. I'm CC'ing them and Al Viro on this email to solicit their input.

Gordon

[1] http://www.gossamer-threads.com/lists/linux/kernel/801528
[2] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7492d4a416d68ab4bd254b36ffcc4e0138daa8ff

-- 
Gordon Farquharson
GnuPG Key ID: 32D6D676

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Michael Buesch]

On Tuesday 19 February 2008 05:59:21 Gordon Farquharson wrote:
> Does this thread [1] provide any clues as to the Right Thing (TM) to do?
> 
> It should be noted that Linus and Andrew signed off on the m68k fix
> [2]. I'm CC'ing them and Al Viro on this email to solicit their input.
> 
> Gordon
> 
> [1] http://www.gossamer-threads.com/lists/linux/kernel/801528
> [2] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7492d4a416d68ab4bd254b36ffcc4e0138daa8ff
> 

That doesn't cause me to magically sign off this sort of patches, too.
The sanity check is clearly broken in file2alias.c, as it checks something
from the target kernel against the host environment it is compiled on.
That doesn't make any sense at all.

I also don't see why we want to compare the size of the struct in kernel
and userland. It is not used in userland.

So NACK for this SSB patch.

-- 
Greetings Michael.

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Gordon Farquharson]

Hi Michael

On Feb 19, 2008 3:41 AM, Michael Buesch <mb@bu3sch.de> wrote:

> > [2] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7492d4a416d68ab4bd254b36ffcc4e0138daa8ff
> >
>
> That doesn't cause me to magically sign off this sort of patches, too.
> The sanity check is clearly broken in file2alias.c, as it checks something
> from the target kernel against the host environment it is compiled on.
> That doesn't make any sense at all.

I think that you make some good points, but I'm at a loss as to how to
fix the problem. Do you have any suggestions? Could we temporarily
apply the patch, so that people can build a kernel with the b43 driver
with a cross compiler, until a more permanent solution is found?

Gordon

-- 
Gordon Farquharson
GnuPG Key ID: 32D6D676

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Michael Buesch]

On Wednesday 20 February 2008 01:44:38 Gordon Farquharson wrote:
> Hi Michael
> 
> On Feb 19, 2008 3:41 AM, Michael Buesch <mb@bu3sch.de> wrote:
> 
> > > [2] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7492d4a416d68ab4bd254b36ffcc4e0138daa8ff
> > >
> >
> > That doesn't cause me to magically sign off this sort of patches, too.
> > The sanity check is clearly broken in file2alias.c, as it checks something
> > from the target kernel against the host environment it is compiled on.
> > That doesn't make any sense at all.
> 
> I think that you make some good points, but I'm at a loss as to how to
> fix the problem. Do you have any suggestions?

Remove the broken sanity check, if it's not possible the check there.

-- 
Greetings Michael.

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Sam Ravnborg]

On Wed, Feb 20, 2008 at 03:44:04PM +0100, Michael Buesch wrote:
> On Wednesday 20 February 2008 01:44:38 Gordon Farquharson wrote:
> > Hi Michael
> > 
> > On Feb 19, 2008 3:41 AM, Michael Buesch <mb@bu3sch.de> wrote:
> > 
> > > > [2] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7492d4a416d68ab4bd254b36ffcc4e0138daa8ff
> > > >
> > >
> > > That doesn't cause me to magically sign off this sort of patches, too.
> > > The sanity check is clearly broken in file2alias.c, as it checks something
> > > from the target kernel against the host environment it is compiled on.
> > > That doesn't make any sense at all.
> > 
> > I think that you make some good points, but I'm at a loss as to how to
> > fix the problem. Do you have any suggestions?
> 
> Remove the broken sanity check, if it's not possible the check there.
The check is valid for > 99% of the kernel builds as
cross compile builds are not that typical.
And the check is there for the sake of modutils.

The details I do not remember.
So we have a few possiblities:

1) Remove the consistency check and try to deal with the
rare cases where it fails and spend many hours investigating
before we realise it is difference in layout of data.

2) Pad a few structures with a few bytes so this consitency
check works even in cross build environments.

3) Detect that we are doing cross builds and skip the check
in this case.

Option 1) is the worst of the three as that can cost
of many hours bug-hunting.
Option 3) may seem optimal but I do not like to add more
complexity to this part of the build. And really I do not
know a reliable way to detech when we do cross builds anyway.

Leaving us with option 2) that is simple, strighforward and harmless.

	Sam

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Gordon Farquharson]

Hi Sam

On Wed, Feb 20, 2008 at 12:37 PM, Sam Ravnborg <sam@ravnborg.org> wrote:

>  Option 1) is the worst of the three as that can cost
>  of many hours bug-hunting.
>  Option 3) may seem optimal but I do not like to add more
>  complexity to this part of the build. And really I do not
>  know a reliable way to detech when we do cross builds anyway.
>
>  Leaving us with option 2) that is simple, strighforward and harmless.

Are you willing to sign off on and commit the patch?

Gordon

-- 
Gordon Farquharson
GnuPG Key ID: 32D6D676

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Gordon Farquharson]

On Thu, Feb 21, 2008 at 9:24 PM, Gordon Farquharson
<gordonfarquharson@gmail.com> wrote:
> Hi Sam
>
>
>  On Wed, Feb 20, 2008 at 12:37 PM, Sam Ravnborg <sam@ravnborg.org> wrote:
>
>  >  Option 1) is the worst of the three as that can cost
>  >  of many hours bug-hunting.
>  >  Option 3) may seem optimal but I do not like to add more
>  >  complexity to this part of the build. And really I do not
>  >  know a reliable way to detech when we do cross builds anyway.
>  >
>  >  Leaving us with option 2) that is simple, strighforward and harmless.
>
>  Are you willing to sign off on and commit the patch?

I realize that I forgot to put

Signed-off-by: Gordon Farquharson <gordonfarquharson@gmail.com>

in my original email.

Gordon

-- 
Gordon Farquharson
GnuPG Key ID: 32D6D676

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Michael Buesch]

On Friday 22 February 2008 05:24:32 Gordon Farquharson wrote:
> Hi Sam
> 
> On Wed, Feb 20, 2008 at 12:37 PM, Sam Ravnborg <sam@ravnborg.org> wrote:
> 
> >  Option 1) is the worst of the three as that can cost
> >  of many hours bug-hunting.
> >  Option 3) may seem optimal but I do not like to add more
> >  complexity to this part of the build. And really I do not
> >  know a reliable way to detech when we do cross builds anyway.
> >
> >  Leaving us with option 2) that is simple, strighforward and harmless.
> 
> Are you willing to sign off on and commit the patch?

Only with a big fat comment added that the alignment is only needed
because of a broken sanity check in file2alias.c.

-- 
Greetings Michael.

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Gordon Farquharson]

On Fri, Feb 22, 2008 at 7:07 AM, Michael Buesch <mb@bu3sch.de> wrote:
> On Friday 22 February 2008 05:24:32 Gordon Farquharson wrote:
>  > On Wed, Feb 20, 2008 at 12:37 PM, Sam Ravnborg <sam@ravnborg.org> wrote:
>  >
>  > >  Option 1) is the worst of the three as that can cost
>  > >  of many hours bug-hunting.
>  > >  Option 3) may seem optimal but I do not like to add more
>  > >  complexity to this part of the build. And really I do not
>  > >  know a reliable way to detech when we do cross builds anyway.
>  > >
>  > >  Leaving us with option 2) that is simple, strighforward and harmless.
>  >
>  > Are you willing to sign off on and commit the patch?
>
>  Only with a big fat comment added that the alignment is only needed
>  because of a broken sanity check in file2alias.c.

How about this?

---

Align the members of the SSB device structure to a 32 bit boundary so
that the b43 driver can be built for arm using a cross compiler. This
change is required so that the test in scripts/mod/file2alias.c that
checks that the size of the device ID type against the size of the
section in the object file succeeds (see
http://lkml.org/lkml/2008/2/18/481 for discussion).

Signed-off-by: Gordon Farquharson <gordonfarquharson@gmail.com>

---

diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 139d49d..93083ad 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -351,7 +351,9 @@ struct sdio_device_id {
 struct ssb_device_id {
        __u16   vendor;
        __u16   coreid;
-       __u8    revision;
+       /* Explicit padding to support cross-compilation. */
+       __u8    revision
+               __attribute__((aligned(sizeof(__u32))));
 };
 #define SSB_DEVICE(_vendor, _coreid, _revision)  \
        { .vendor = _vendor, .coreid = _coreid, .revision = _revision, }

-- 
Gordon Farquharson
GnuPG Key ID: 32D6D676

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Michael Buesch]

On Saturday 23 February 2008, Gordon Farquharson wrote:
> On Fri, Feb 22, 2008 at 7:07 AM, Michael Buesch <mb@bu3sch.de> wrote:
> > On Friday 22 February 2008 05:24:32 Gordon Farquharson wrote:
> >  > On Wed, Feb 20, 2008 at 12:37 PM, Sam Ravnborg <sam@ravnborg.org> wrote:
> >  >
> >  > >  Option 1) is the worst of the three as that can cost
> >  > >  of many hours bug-hunting.
> >  > >  Option 3) may seem optimal but I do not like to add more
> >  > >  complexity to this part of the build. And really I do not
> >  > >  know a reliable way to detech when we do cross builds anyway.
> >  > >
> >  > >  Leaving us with option 2) that is simple, strighforward and harmless.
> >  >
> >  > Are you willing to sign off on and commit the patch?
> >
> >  Only with a big fat comment added that the alignment is only needed
> >  because of a broken sanity check in file2alias.c.
> 
> How about this?
> 
> ---
> 
> Align the members of the SSB device structure to a 32 bit boundary so
> that the b43 driver can be built for arm using a cross compiler. This
> change is required so that the test in scripts/mod/file2alias.c that
> checks that the size of the device ID type against the size of the
> section in the object file succeeds (see
> http://lkml.org/lkml/2008/2/18/481 for discussion).
> 
> Signed-off-by: Gordon Farquharson <gordonfarquharson@gmail.com>
> 
> ---
> 
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 139d49d..93083ad 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -351,7 +351,9 @@ struct sdio_device_id {
>  struct ssb_device_id {
>         __u16   vendor;
>         __u16   coreid;
> -       __u8    revision;
> +       /* Explicit padding to support cross-compilation. */

A big fat comment is something like that:

/* Explicit padding to support a broken sanity check in file2alias.c.
 * The check will compare the size of the structure in the kernel
 * object file to the userspace the kernel is compiled on.
 * This breaks on cross-compilation. This padding is a workaround
 * for this. */

> +       __u8    revision
> +               __attribute__((aligned(sizeof(__u32))));
>  };
>  #define SSB_DEVICE(_vendor, _coreid, _revision)  \
>         { .vendor = _vendor, .coreid = _coreid, .revision = _revision, }
> 

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Gordon Farquharson]

On Fri, Feb 22, 2008 at 10:51 PM, Michael Buesch <mb@bu3sch.de> wrote:
>  A big fat comment is something like that:
>
>  /* Explicit padding to support a broken sanity check in file2alias.c.
>   * The check will compare the size of the structure in the kernel
>   * object file to the userspace the kernel is compiled on.
>   * This breaks on cross-compilation. This padding is a workaround
>   * for this. */

---

Align the members of the SSB device structure to a 32 bit boundary so
that the b43 driver can be built for arm using a cross compiler. This
alignment is required so that the test in scripts/mod/file2alias.c
that checks that the size of the device ID type against the size of
the section in the object file succeeds (see comment and
http://lkml.org/lkml/2008/2/18/481 for explanation).

Signed-off-by: Gordon Farquharson <gordonfarquharson@gmail.com>

---

diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 139d49d..208d49a 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -351,7 +351,13 @@ struct sdio_device_id {
 struct ssb_device_id {
 	__u16	vendor;
 	__u16	coreid;
-	__u8	revision;
+	/* Explicit padding to support a broken sanity check in file2alias.c.
+	 * The check compares the size of the structure in the kernel
+	 * object file to the size of the structure reported in userspace for
+	 * the system on which the kernel is compiled. The check breaks on
+	 * cross-compilation, and the padding is a workaround for this. */
+	__u8	revision
+		__attribute__((aligned(sizeof(__u32))));
 };
 #define SSB_DEVICE(_vendor, _coreid, _revision)  \
 	{ .vendor = _vendor, .coreid = _coreid, .revision = _revision, }

-- 
Gordon Farquharson
GnuPG Key ID: 32D6D676

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Michael Buesch]

On Saturday 23 February 2008 11:14:23 Gordon Farquharson wrote:
> On Fri, Feb 22, 2008 at 10:51 PM, Michael Buesch <mb@bu3sch.de> wrote:
> >  A big fat comment is something like that:
> >
> >  /* Explicit padding to support a broken sanity check in file2alias.c.
> >   * The check will compare the size of the structure in the kernel
> >   * object file to the userspace the kernel is compiled on.
> >   * This breaks on cross-compilation. This padding is a workaround
> >   * for this. */
> 
> ---
> 
> Align the members of the SSB device structure to a 32 bit boundary so
> that the b43 driver can be built for arm using a cross compiler. This
> alignment is required so that the test in scripts/mod/file2alias.c
> that checks that the size of the device ID type against the size of
> the section in the object file succeeds (see comment and
> http://lkml.org/lkml/2008/2/18/481 for explanation).
> 
> Signed-off-by: Gordon Farquharson <gordonfarquharson@gmail.com>

Acked-by: Michael Buesch <mb@bu3sch.de>

> ---
> 
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 139d49d..208d49a 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -351,7 +351,13 @@ struct sdio_device_id {
>  struct ssb_device_id {
>  	__u16	vendor;
>  	__u16	coreid;
> -	__u8	revision;
> +	/* Explicit padding to support a broken sanity check in file2alias.c.
> +	 * The check compares the size of the structure in the kernel
> +	 * object file to the size of the structure reported in userspace for
> +	 * the system on which the kernel is compiled. The check breaks on
> +	 * cross-compilation, and the padding is a workaround for this. */
> +	__u8	revision
> +		__attribute__((aligned(sizeof(__u32))));
>  };
>  #define SSB_DEVICE(_vendor, _coreid, _revision)  \
>  	{ .vendor = _vendor, .coreid = _coreid, .revision = _revision, }
> 



-- 
Greetings Michael.

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Ben Dooks]

On Wed, Feb 20, 2008 at 08:37:09PM +0100, Sam Ravnborg wrote:
> On Wed, Feb 20, 2008 at 03:44:04PM +0100, Michael Buesch wrote:
> > On Wednesday 20 February 2008 01:44:38 Gordon Farquharson wrote:
> > > Hi Michael
> > > 
> > > On Feb 19, 2008 3:41 AM, Michael Buesch <mb@bu3sch.de> wrote:
> > > 
> > > > > [2] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7492d4a416d68ab4bd254b36ffcc4e0138daa8ff
> > > > >
> > > >
> > > > That doesn't cause me to magically sign off this sort of patches, too.
> > > > The sanity check is clearly broken in file2alias.c, as it checks something
> > > > from the target kernel against the host environment it is compiled on.
> > > > That doesn't make any sense at all.
> > > 
> > > I think that you make some good points, but I'm at a loss as to how to
> > > fix the problem. Do you have any suggestions?
> > 
> > Remove the broken sanity check, if it's not possible the check there.
> The check is valid for > 99% of the kernel builds as
> cross compile builds are not that typical.
> And the check is there for the sake of modutils.

I build all of my ARM kernels on an x86 box, it is much faster
and I don't have to ensure I have a read/write capable filesystem
for any of my ARM boards.

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Gordon Farquharson]

Hi Ben

On Tue, Feb 26, 2008 at 7:37 AM, Ben Dooks <ben-linux@fluff.org> wrote:

>  I build all of my ARM kernels on an x86 box, it is much faster
>  and I don't have to ensure I have a read/write capable filesystem
>  for any of my ARM boards.

The patch has been merged into Andrew's -mm tree.

http://www.mail-archive.com/mm-commits@vger.kernel.org/msg35079.html

Gordon

-- 
Gordon Farquharson
GnuPG Key ID: 32D6D676

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Sam Ravnborg]

> > 
> > It is a consistencycheck between host and target
> > layout of data.
> > You need to pad the structure so it becomes 8 byte in size.
> 
> Ok, I looked at the code and it is hightly questionable to me that this
> check does work in a crosscompile environment (which the ARM build
> most likely is).
> 
> It seems to check the size of the structure in the .o file against
> the size of the structure on the _host_ where it is compiled.
> I can't see why we would want to check _anything_ of the target stuff
> to the host this stuff is compiled on.
> I can compile an ARM kernel on any machine I want.
> 
> There actually is a comment:
>  * Check that sizeof(device_id type) are consistent with size of section
>  * in .o file. If in-consistent then userspace and kernel does not agree
>  * on actual size which is a bug.
> 
> So it seems what this check _wants_ to compare the sizeof the structure
> in the kernel to the size of the stucture in the userland of the target system.
> But it does _not_ do that.
> It does compare the size of the structure in the kernel against the size of
> the stucture in userland on the machine it is _compiled_ on.
> That is wrong.
In at least 99% of the cases this is OK and the check has found
several bugs where things would not have worked due to different
alignmnet between kernel and userland. Just think about the
issues in a mixed 32/64 bit world.

In this particular case you _could_ be right that it would work
on ARM userland. But the only way to be sure is to pad the
structure so it become 8 byte in size.
Or as was recently done in the m68k case to tell the
compiler to specifically align it to 8 byte boundary.

	Sam

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Matthieu CASTET]

Sam Ravnborg <sam <at> ravnborg.org> writes:

> 
> In at least 99% of the cases this is OK and the check has found
> several bugs where things would not have worked due to different
> alignmnet between kernel and userland. Just think about the
> issues in a mixed 32/64 bit world.
> 
I don't see how this check is supposed to work.
For some arch userspace can have different alignement depending on compilation
option :
- x86_64 : 32 or 64 bits mode
- arm : 32 bits or thumb mode
- mips : 32 bits or mips16 mode
[...]

The check is done only for one case ?


> In this particular case you _could_ be right that it would work
> on ARM userland. But the only way to be sure is to pad the
> structure so it become 8 byte in size.
> Or as was recently done in the m68k case to tell the
> compiler to specifically align it to 8 byte boundary.
An easy way to fix this is just to issue a warning but not an error. So it could
ignored on cross compilation case.

Matthieu

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Michael Buesch]

On Monday 18 February 2008 23:53:54 Russell King wrote:
> I get extremely pissed off everytime I have to try to explain random
> alignment issues to people.  "It doesn't work like i386 so it must be
> broken" is a rediculous position to take.

I did _not_ ask for a general description of alignment. I did
_just_ ask why ARM is special and why the compiler didn't handle
it there. I do develop code for MIPS, so I do know what alignment is about.

> > Can you _please_ explain what makes ARM so special here?
> 
> No because I don't really know.
> 
> > Why can't we have an array of this structure on ARM?
> > 
> > struct ssb_device_id {
> >        __u16   vendor;
> 
> 2 bytes
> 
> >        __u16   coreid;
> 
> 2 bytes
> 
> >        __u8    revision;
> 
> 1 byte
> 
> > };
> 
> and therefore sizeof this structure will be 5 bytes, but because of the
> ABI rules (which are _explicitly_ allowed by the C standard), it'll
> become 8 bytes due to padding afterwards.

So that would be fine. I don't see what would be unaligned then.
Even if it would pad only one byte after the "revision" it would all
be aligned in a natural way.

> At a _guess_ and its only a guess, the linker will enforce this rule
> between compilation units, otherwise the implications are disgusting
> (would probably result in all loads having to be individual byte loads
> and instructions to combine the result - since ARM has strict alignment
> requirements.)

Yeah. As I said. The code does work on MIPS, which has strict alignment
requirements as well.

> What I can say is that the ABI will not be changed because someone in the
> kernel decides they don't like it.  So the options are: either fix it so
> it works, or accept that the code is broken and will never work on ARM.

I did never ask to change some ABI, sorry.
Still I can't see why this structure will cause alignment issues, as the
compiler will pad it up to the right boundary automagically, as you said
above. Why doesn't the ARM compiler do this?

-- 
Greetings Michael.

Re: [RFC] [PATCH] Fix b43 driver build for arm

[Geert Uytterhoeven]

On Tue, 19 Feb 2008, Michael Buesch wrote:
> Still I can't see why this structure will cause alignment issues, as the
> compiler will pad it up to the right boundary automagically, as you said
> above. Why doesn't the ARM compiler do this?

The ARM compiler handles it correctly.

But the ugly hacks to get useful information about the module device
table using the _host_ compiler fail.

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: [RFC] [PATCH] Fix b43 driver build for arm

[Michael Buesch]

On Tuesday 19 February 2008 09:37:05 Geert Uytterhoeven wrote:
> On Tue, 19 Feb 2008, Michael Buesch wrote:
> > Still I can't see why this structure will cause alignment issues, as the
> > compiler will pad it up to the right boundary automagically, as you said
> > above. Why doesn't the ARM compiler do this?
> 
> The ARM compiler handles it correctly.
> 
> But the ugly hacks to get useful information about the module device
> table using the _host_ compiler fail.

Ok, fine. So I'm not going to apply this patch. We need
to fix the sanity check instead.

-- 
Greetings Michael.