| Message ID | 1406893181-24764-1-git-send-email-thierry.reding@gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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.
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
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.
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
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 */