Re: [PATCH v2] drivers/misc: Add ASpeed LPC control driver

[Greg KH <gregkh@linuxfoundation.org>]

On Sun, Jan 15, 2017 at 10:45:15PM -0600, Benjamin Herrenschmidt wrote:
> On Mon, 2017-01-16 at 09:43 +1100, Cyril Bur wrote:
> > > > +struct aspeed_lpc_ctrl_mapping {
> > > > +   __u8    window_type;
> > > > +   __u8    window_id;
> > > > +   __u32   addr;
> > > > +   __u32   offset;
> > > > +   __u32   size;
> > > 
> > > That's some crazy alignment, do you really mean to put a 32bit value
> > > aligned like that?  Will it work properly on your systems?
> > > 
> > 
> > Well, in my probably unrealistic and rushed testing it did work - but
> > then this was all with one compiler on one machine so I'm not
> > surprised. I'll put the u8s at the end.
> 
> This structure isn't packed, so the alignment is fine, the compiler
> will align "addr" to a 32-bit boundary. What might have been better
> would have be to be explicit about it by adding two u8 of padding
> (or a u16) but the above works. Moving things around just makes the
> structure more weird, the window and id make more sense being at the
> beginning.

For an ioctl structure like this, can you guarantee that the "padding"
will always be the same?  Last time I looked, the C standard didn't say
anything about that, so I would strongly recommend making it so that no
padding is needed at all...

thanks,

greg k-h