Re: [PATCH] ata: libahci: Make host flags unsigned long

[Tejun Heo <tj@kernel.org>]

Hello,

On Fri, Aug 01, 2014 at 04:01:23PM +0200, Thierry Reding wrote:
> I think there's just one occurrence. Turning the flags into an unsigned
> long seems like a much more natural thing to do, though Besides it being
> what many other parts of the kernel use for flags it gives us natural
> alignment within struct ahci_host_priv. The structure currently looks
> like this:
> 
> 	struct ahci_host_priv {
> 		unsigned int flags;
> 		u32 force_port_map;
> 		u32 mask_port_map;
> 
> 		void __iomem *mmio;
> 		...
> 	};
> 
> On 64-bit that unsigned int will be 32-bit and cause additional padding
> to be inserted between mask_port_map and mmio to align the 64-bit
> pointer.
> 
> It's not like the alignment is that *hugely* important, but it's still,
> you know, pretty.

I don't get how that's pretty.  Sure, that space is consumed by
something on 64bit archs but the extra space consumed can't be
utilized unless we're gonna change how we use flags depending on the
bitness of the architecture.  You're saying that rather than leaving
unused space as unused it's better to commit that space to a variable
which can't make use of that extra space anyway.  What if we later
wanna add another int there?  Do we make that int a long too or modify
the complete unrelated ->flags back to int?

Prettiness is a good thing but code fundamentally should match what
the underlying requirement dictates it to.  We sure have to trade that
off too at times when the benefit gained is worthwhile, but I
completely fail to see how this feel-good packed prettiness is
anything worthwhile.

In general, please don't do things like this in the kernel.  Use ulong
for flags iff it's necessary (atomic bitops).

Thanks.

-- 
tejun