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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 1 Aug 2014 10:09:28 -0400
From:	Tejun Heo <tj@...nel.org>
To:	Thierry Reding <thierry.reding@...il.com>
Cc:	Antoine Ténart 
	<antoine.tenart@...e-electrons.com>, linux-ide@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ata: libahci: Make host flags unsigned long

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/