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

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

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Commit 725c7b570fda (ata: libahci_platform: move port_map parameters
into the AHCI structure) moves flags into the struct ahci_host_priv's
.flags field, which causes compiler warnings on 64-bit builds when that
value is cast to a void * pointer. Rather than adding additional casting
to silence the warning, turn the flags field into a unsigned long.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/ata/ahci.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 59ae0ee00149..e68532ca3826 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -317,7 +317,7 @@ struct ahci_port_priv {
 
 struct ahci_host_priv {
 	/* Input fields */
-	unsigned int		flags;		/* AHCI_HFLAG_* */
+	unsigned long		flags;		/* AHCI_HFLAG_* */
 	u32			force_port_map;	/* force port map */
 	u32			mask_port_map;	/* mask out particular bits */
 
-- 
2.0.3

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

[Tejun Heo]

On Fri, Aug 01, 2014 at 01:39:41PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Commit 725c7b570fda (ata: libahci_platform: move port_map parameters
> into the AHCI structure) moves flags into the struct ahci_host_priv's
> .flags field, which causes compiler warnings on 64-bit builds when that
> value is cast to a void * pointer. Rather than adding additional casting
> to silence the warning, turn the flags field into a unsigned long.

Unless we're talking about a lot of places where such casting is
necessary, I'd actually prefer to keep the flags uint and use explicit
casting.  How many are we talking about?

Thanks.

-- 
tejun

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1439 bytes --]

On Fri, Aug 01, 2014 at 08:52:06AM -0400, Tejun Heo wrote:
> On Fri, Aug 01, 2014 at 01:39:41PM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Commit 725c7b570fda (ata: libahci_platform: move port_map parameters
> > into the AHCI structure) moves flags into the struct ahci_host_priv's
> > .flags field, which causes compiler warnings on 64-bit builds when that
> > value is cast to a void * pointer. Rather than adding additional casting
> > to silence the warning, turn the flags field into a unsigned long.
> 
> Unless we're talking about a lot of places where such casting is
> necessary, I'd actually prefer to keep the flags uint and use explicit
> casting.  How many are we talking about?

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.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[Tejun Heo]

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

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1923 bytes --]

On Fri, Aug 01, 2014 at 10:09:28AM -0400, Tejun Heo wrote:
> 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).

Oh well, as you wish, then.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]