On Sat, Jun 16, 2012 at 03:23:31PM +0200, Bjørn Mork wrote:
Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> writes:
On Fri, Jun 15, 2012 at 11:02:55PM +0200, Geert Uytterhoeven wrote:
As "kernel_ulong_t driver_info" is no longer naturally aligned, the
compiler will
add implicit padding. But the padding depends on the architecture.
Ah, so we were "lucky" before, nice.
I don't really believe in luck :-) I think someone has been really
smart here. Maybe too smart...
No, I think the previous structure was just "lucky" in that it just
happened to be the right alignment. I say this as I think I was the one
who created that structure years ago. Or maybe not, this was back in
the 2.3 kernel days, I can't remember what patches I wrote last week...
It can be fixed by adding explicit padding. Probably it should be padded by
7 bytes (not 3), as kernel_ulong_t may require 8-byte alignment on some 64-bit
platforms. Or by an explicit alignment attribute.
See also
* commit 8175fe2dda1c93a9c596921c8ed4a0b4baccdefe ("HID: fix
hid_device_id for cross compiling")
* commit 7492d4a416d68ab4bd254b36ffcc4e0138daa8ff ("sdio: fix module
device table definition for m68k")
* commit 9e2d3cd34a159948dc753a14573e16bffc04dba8 ("[PATCH]
mod_devicetable.h fixes")
So would the patch below fix this? It should force alignment of the
driver_data field, which is all you want here, right?
Still, there's a bug in file2alias (which is compiled by the host
compiler), in that
it may use different padding than the target platform when cross-compiling.
That's not good, but outside of this specific issue, right? Have we
just been fortunate it hasn't really hit us yet?
thanks,
greg k-h
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 7771d45..6955045 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -122,7 +122,8 @@ struct usb_device_id {
__u8 bInterfaceNumber;
/* not matched against */
- kernel_ulong_t driver_info;
+ kernel_ulong_t driver_info
+ __attribute__((aligned(sizeof(kernel_ulong_t))));
};
This feels a lot like papering over the real problem. It will solve
this instance, but the list of such previous "paper work" that Geert
provided should be enough evidence that this will happen again the next
time someone modifies a device id struct for some subsystem.
Hopefully not, if you add another field here, the alignment force will
keep things lined up properly, from what I can tell. Is that not true?
And adding forced aligment here feels wrong since there is no good
reason why the (target) compiler shouldn't know the proper alignment for
this structure, is there? OK, "feels wrong" is not a good argument. But
it would be better to solve this problem once and for all.
C doesn't require the structure to be aligned. Actually the spec says
it doesn't guarantee anything about this, we just "know" that gcc is
going to be semi-sane and try to do the best it can. Hopefully clang is
also semi-sane as well.
So because of that, we have to give it some guidance, hence the patch.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
