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

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Mon, 2017-01-16 at 11:45 +0100, Greg KH wrote:
> 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...

The implicit padding is a matter of ABI more than C standard, we
already rely on this in a bazillion of places but it *has* bitten us in
a few corner cases (mostly when u64 is involved due to ABI differences
between 32-bit and 64-bit), so explicit padding is indeed preferred.

Cyril can you respin with:

	struct aspeed_lpc_ctrl_mapping {
		__u8    window_type;
		__u8    window_id;
		__u16   pad;
		__u32   addr;
		__u32   offset;
		__u32   size;

I prefer that, it makes more sense to keep the window type/id at the
top. Alternatively call "pad", "flags", and describe that clients must
zero it, that way we can use it for future extensions of we ever have
to.

Also for mmap, you have:

       /* AHB accesses are not cache coherent */
       if (file->f_flags & O_DSYNC)
               prot = pgprot_noncached(prot);

Why the test for O_DSYNC ? I'd rather not make this a userspace
responsibility to get right... I'd have made it unconditional.

I notice ARM has pgprot_dmacoherent() that might just do what we want
here, ie, non-cachable if needed. Otherwise, I'd just unconditionally
set it to noncached, we can revisit that if Aspeed ever comes up with a
coherent SoC which I very much doubt but ...

Cheers,
Ben.