Re: [-next] FATAL: drivers/gpu/drm/udl/udl: sizeof(struct usb_device_id)=24 is not a modulo of the size of section __mod_usb_device_table=44.

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Sat, Jun 16, 2012 at 03:23:31PM +0200, Bjørn Mork wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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