[IDE] Fix build bug

[IDE] Fix build bug

[Ralf Baechle]

  CC      drivers/ide/pci/generic.o
drivers/ide/pci/generic.c:52: error: __setup_str_ide_generic_all_on causes a
+section type conflict

This sort of build error is becoming a regular issue.  Either all or non
of the elements that go into a particular section of a compilation unit
need to be const.  Or an error may result such as in this case if
CONFIG_HOTPLUG is unset.

Maybe worth a check in checkpatch.pl - but certainly gcc's interolerance
is also being less than helpful here.

---
 drivers/ide/pci/generic.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ide/pci/generic.c b/drivers/ide/pci/generic.c
index f44d708..0047684 100644
--- a/drivers/ide/pci/generic.c
+++ b/drivers/ide/pci/generic.c
@@ -67,7 +67,7 @@ MODULE_PARM_DESC(all_generic_ide, "IDE generic will claim all unknown PCI IDE st
 		.udma_mask	= ATA_UDMA6, \
 	}
 
-static const struct ide_port_info generic_chipsets[] __devinitdata = {
+static struct ide_port_info generic_chipsets[] __devinitdata = {
 	/*  0 */ DECLARE_GENERIC_PCI_DEV("Unknown",	0),
 
 	{	/* 1 */

Re: [IDE] Fix build bug

[Sam Ravnborg]

On Thu, Oct 25, 2007 at 02:53:34PM +0100, Ralf Baechle wrote:
>   CC      drivers/ide/pci/generic.o
> drivers/ide/pci/generic.c:52: error: __setup_str_ide_generic_all_on causes a
> +section type conflict
> 
> This sort of build error is becoming a regular issue.  Either all or non
> of the elements that go into a particular section of a compilation unit
> need to be const.  Or an error may result such as in this case if
> CONFIG_HOTPLUG is unset.
So we can avoid this if we invent a __constinitdata tag that uses
a new section?
I ask mainly to understand this error - not that I am that found
of the idea.

	Sam

Re: [IDE] Fix build bug

[Ralf Baechle]

On Thu, Oct 25, 2007 at 04:13:05PM +0200, Sam Ravnborg wrote:

> On Thu, Oct 25, 2007 at 02:53:34PM +0100, Ralf Baechle wrote:
> >   CC      drivers/ide/pci/generic.o
> > drivers/ide/pci/generic.c:52: error: __setup_str_ide_generic_all_on causes a
> > +section type conflict
> > 
> > This sort of build error is becoming a regular issue.  Either all or non
> > of the elements that go into a particular section of a compilation unit
> > need to be const.  Or an error may result such as in this case if
> > CONFIG_HOTPLUG is unset.
> So we can avoid this if we invent a __constinitdata tag that uses
> a new section?
> I ask mainly to understand this error - not that I am that found
> of the idea.

Yes.

  Ralf

Re: [IDE] Fix build bug

[Maciej W. Rozycki]

On Thu, 25 Oct 2007, Sam Ravnborg wrote:

> So we can avoid this if we invent a __constinitdata tag that uses
> a new section?

 That would do.

> I ask mainly to understand this error - not that I am that found
> of the idea.

 Somebody wants to mix up read-only and read/write data in the same 
section and GCC quite legitimately complains about it.  You cannot have 
both at a time.

  Maciej

Re: [IDE] Fix build bug

[Ralf Baechle]

On Thu, Oct 25, 2007 at 03:47:16PM +0100, Maciej W. Rozycki wrote:

> > So we can avoid this if we invent a __constinitdata tag that uses
> > a new section?
> 
>  That would do.
> 
> > I ask mainly to understand this error - not that I am that found
> > of the idea.
> 
>  Somebody wants to mix up read-only and read/write data in the same 
> section and GCC quite legitimately complains about it.  You cannot have 
> both at a time.

My interpretation is that it would be perfectly ok for a C compiler to
do minimal handling of const by only throwing errors for attempted
assignments to const objects but otherwise treating them as if they
were non-const, that is for example putting them into an r/w section.

  Ralf

Re: [IDE] Fix build bug

[Maciej W. Rozycki]

On Thu, 25 Oct 2007, Ralf Baechle wrote:

> >  Somebody wants to mix up read-only and read/write data in the same 
> > section and GCC quite legitimately complains about it.  You cannot have 
> > both at a time.
> 
> My interpretation is that it would be perfectly ok for a C compiler to
> do minimal handling of const by only throwing errors for attempted
> assignments to const objects but otherwise treating them as if they
> were non-const, that is for example putting them into an r/w section.

 That would probably be valid (any C standard expert please correct me if 
I am wrong), but the approach looks like: since we have the capability in 
the hardware and the OS, then why not actually enforce the rule at the run 
time as well?

  Maciej

Re: [IDE] Fix build bug

[Bartlomiej Zolnierkiewicz]

On Thursday 25 October 2007, Sam Ravnborg wrote:
> On Thu, Oct 25, 2007 at 02:53:34PM +0100, Ralf Baechle wrote:
> >   CC      drivers/ide/pci/generic.o
> > drivers/ide/pci/generic.c:52: error: __setup_str_ide_generic_all_on causes a
> > +section type conflict
> > 
> > This sort of build error is becoming a regular issue.  Either all or non
> > of the elements that go into a particular section of a compilation unit
> > need to be const.  Or an error may result such as in this case if
> > CONFIG_HOTPLUG is unset.
> So we can avoid this if we invent a __constinitdata tag that uses
> a new section?

I asked similar question on LKML few days ago together with the list of
potentially problematic places:

	http://article.gmane.org/gmane.linux.kernel/594427

Now I see that the list is only partially complete since __initdata and co.
may be also hidden in things like __setup() etc.

Thanks,
Bart

Re: [IDE] Fix build bug

[Bartlomiej Zolnierkiewicz]

Hi,

On Thursday 25 October 2007, Ralf Baechle wrote:
>   CC      drivers/ide/pci/generic.o
> drivers/ide/pci/generic.c:52: error: __setup_str_ide_generic_all_on causes a
> +section type conflict
> 
> This sort of build error is becoming a regular issue.  Either all or non
> of the elements that go into a particular section of a compilation unit
> need to be const.  Or an error may result such as in this case if
> CONFIG_HOTPLUG is unset.
> 
> Maybe worth a check in checkpatch.pl - but certainly gcc's interolerance
> is also being less than helpful here.
> 
> ---
>  drivers/ide/pci/generic.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ide/pci/generic.c b/drivers/ide/pci/generic.c
> index f44d708..0047684 100644
> --- a/drivers/ide/pci/generic.c
> +++ b/drivers/ide/pci/generic.c
> @@ -67,7 +67,7 @@ MODULE_PARM_DESC(all_generic_ide, "IDE generic will claim all unknown PCI IDE st
>  		.udma_mask	= ATA_UDMA6, \
>  	}
>  
> -static const struct ide_port_info generic_chipsets[] __devinitdata = {
> +static struct ide_port_info generic_chipsets[] __devinitdata = {
>  	/*  0 */ DECLARE_GENERIC_PCI_DEV("Unknown",	0),
>  
>  	{	/* 1 */

I would prefer to not remove const from generic_chipsets[] so:

[PATCH] drivers/ide/pci/generic: fix build for CONFIG_HOTPLUG=n

It turns out that const and __{dev}initdata cannot be mixed currently
and that generic IDE PCI host driver is also affected by the same issue:

On Thursday 25 October 2007, Ralf Baechle wrote:
>   CC      drivers/ide/pci/generic.o
> drivers/ide/pci/generic.c:52: error: __setup_str_ide_generic_all_on causes a
> +section type conflict

[ Also reported by Martijn Uffing <mp3project@sarijopen.student.utwente.nl>. ]

This patch workarounds the problem in a bit hackish way but without
removing const from generic_chipsets[] (it adds const to __setup() so
__setup_str_ide_generic_all becomes const).

Now all __{dev}initdata data in generic IDE PCI host driver are read-only
so it builds again (driver's .init.data section gets marked as READONLY).

Cc: Martijn Uffing <mp3project@sarijopen.student.utwente.nl>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/pci/generic.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: b/drivers/ide/pci/generic.c
===================================================================
--- a/drivers/ide/pci/generic.c
+++ b/drivers/ide/pci/generic.c
@@ -49,7 +49,7 @@ static int __init ide_generic_all_on(cha
 	printk(KERN_INFO "IDE generic will claim all unknown PCI IDE storage controllers.\n");
 	return 1;
 }
-__setup("all-generic-ide", ide_generic_all_on);
+const __setup("all-generic-ide", ide_generic_all_on);
 #endif
 module_param_named(all_generic_ide, ide_generic_all, bool, 0444);
 MODULE_PARM_DESC(all_generic_ide, "IDE generic will claim all unknown PCI IDE storage controllers.");

Re: [IDE] Fix build bug

[Denys Vlasenko]

On Thursday 25 October 2007 22:41, Bartlomiej Zolnierkiewicz wrote:
> > -static const struct ide_port_info generic_chipsets[] __devinitdata = {
> > +static struct ide_port_info generic_chipsets[] __devinitdata = {
> >  	/*  0 */ DECLARE_GENERIC_PCI_DEV("Unknown",	0),
> >  
> >  	{	/* 1 */
> 
> I would prefer to not remove const from generic_chipsets[] so:
> 
> [PATCH] drivers/ide/pci/generic: fix build for CONFIG_HOTPLUG=n
> 
> It turns out that const and __{dev}initdata cannot be mixed currently
> and that generic IDE PCI host driver is also affected by the same issue:
> 
> On Thursday 25 October 2007, Ralf Baechle wrote:
> >   CC      drivers/ide/pci/generic.o
> > drivers/ide/pci/generic.c:52: error: __setup_str_ide_generic_all_on causes a
> > +section type conflict
> 
> [ Also reported by Martijn Uffing <mp3project@sarijopen.student.utwente.nl>. ]
> 
> This patch workarounds the problem in a bit hackish way but without
> removing const from generic_chipsets[] (it adds const to __setup() so
> __setup_str_ide_generic_all becomes const).

You wouldn't believe how much const data is not marked as const because
we don't have __constinitdata etc. Literally megabytes.
--
vda

Re: [IDE] Fix build bug

[Ralf Baechle]

On Tue, Oct 30, 2007 at 11:34:29AM +0000, Denys Vlasenko wrote:

> On Thursday 25 October 2007 22:41, Bartlomiej Zolnierkiewicz wrote:
> > > -static const struct ide_port_info generic_chipsets[] __devinitdata = {
> > > +static struct ide_port_info generic_chipsets[] __devinitdata = {
> > >  	/*  0 */ DECLARE_GENERIC_PCI_DEV("Unknown",	0),
> > >  
> > >  	{	/* 1 */
> > 
> > I would prefer to not remove const from generic_chipsets[] so:
> > 
> > [PATCH] drivers/ide/pci/generic: fix build for CONFIG_HOTPLUG=n
> > 
> > It turns out that const and __{dev}initdata cannot be mixed currently
> > and that generic IDE PCI host driver is also affected by the same issue:
> > 
> > On Thursday 25 October 2007, Ralf Baechle wrote:
> > >   CC      drivers/ide/pci/generic.o
> > > drivers/ide/pci/generic.c:52: error: __setup_str_ide_generic_all_on causes a
> > > +section type conflict
> > 
> > [ Also reported by Martijn Uffing <mp3project@sarijopen.student.utwente.nl>. ]
> > 
> > This patch workarounds the problem in a bit hackish way but without
> > removing const from generic_chipsets[] (it adds const to __setup() so
> > __setup_str_ide_generic_all becomes const).
> 
> You wouldn't believe how much const data is not marked as const because
> we don't have __constinitdata etc. Literally megabytes.

The gain from marking it const is very little and once any non-const
__initdata object is added to a compilation unit all other const declarations
will have to be removed.  Bad tradeoff.

  Ralf

Re: [IDE] Fix build bug

[Bartlomiej Zolnierkiewicz]

On Tuesday 30 October 2007, Ralf Baechle wrote:
> On Tue, Oct 30, 2007 at 11:34:29AM +0000, Denys Vlasenko wrote:
> 
> > On Thursday 25 October 2007 22:41, Bartlomiej Zolnierkiewicz wrote:
> > > > -static const struct ide_port_info generic_chipsets[] __devinitdata = {
> > > > +static struct ide_port_info generic_chipsets[] __devinitdata = {
> > > >  	/*  0 */ DECLARE_GENERIC_PCI_DEV("Unknown",	0),
> > > >  
> > > >  	{	/* 1 */
> > > 
> > > I would prefer to not remove const from generic_chipsets[] so:
> > > 
> > > [PATCH] drivers/ide/pci/generic: fix build for CONFIG_HOTPLUG=n
> > > 
> > > It turns out that const and __{dev}initdata cannot be mixed currently
> > > and that generic IDE PCI host driver is also affected by the same issue:
> > > 
> > > On Thursday 25 October 2007, Ralf Baechle wrote:
> > > >   CC      drivers/ide/pci/generic.o
> > > > drivers/ide/pci/generic.c:52: error: __setup_str_ide_generic_all_on causes a
> > > > +section type conflict
> > > 
> > > [ Also reported by Martijn Uffing <mp3project@sarijopen.student.utwente.nl>. ]
> > > 
> > > This patch workarounds the problem in a bit hackish way but without
> > > removing const from generic_chipsets[] (it adds const to __setup() so
> > > __setup_str_ide_generic_all becomes const).
> > 
> > You wouldn't believe how much const data is not marked as const because
> > we don't have __constinitdata etc. Literally megabytes.
> 
> The gain from marking it const is very little and once any non-const
> __initdata object is added to a compilation unit all other const declarations
> will have to be removed.  Bad tradeoff.

In this case (struct ide_port_info) and probably few others having const
is important (maybe even more important than having __{dev}initdata since
majority of people use CONFIG_HOTPLUG=y) because it allows developers to
catch subtle yet hard to find bugs very early in the development process.

We had a few such cases in IDE - struct ide_port_info _template_ was being
modified because some quirk was needed for one version of the hardware which
was of course incorrect if another version of the hardware was also present
in the system.

Some other potential gains of using const like the better optimized code
or the protection of read-only kernel data are only an extra bonuses. :)

I agree that we need __const{dev}initdata but until then the workaround
that all __{dev}initdata must be const is an acceptable temporary solution
for IDE host drivers.

Thanks,
Bart

Re: [IDE] Fix build bug

[Denys Vlasenko]

On Tuesday 30 October 2007 12:41, Ralf Baechle wrote:
> On Tue, Oct 30, 2007 at 11:34:29AM +0000, Denys Vlasenko wrote:
> 
> > On Thursday 25 October 2007 22:41, Bartlomiej Zolnierkiewicz wrote:
> > > > -static const struct ide_port_info generic_chipsets[] __devinitdata = {
> > > > +static struct ide_port_info generic_chipsets[] __devinitdata = {
> > > >  	/*  0 */ DECLARE_GENERIC_PCI_DEV("Unknown",	0),
> > > >  
> > > >  	{	/* 1 */
> > > 
> > > I would prefer to not remove const from generic_chipsets[] so:
> > > 
> > > [PATCH] drivers/ide/pci/generic: fix build for CONFIG_HOTPLUG=n
> > > 
> > > It turns out that const and __{dev}initdata cannot be mixed currently
> > > and that generic IDE PCI host driver is also affected by the same issue:
> > > 
> > > On Thursday 25 October 2007, Ralf Baechle wrote:
> > > >   CC      drivers/ide/pci/generic.o
> > > > drivers/ide/pci/generic.c:52: error: __setup_str_ide_generic_all_on causes a
> > > > +section type conflict
> > > 
> > > [ Also reported by Martijn Uffing <mp3project@sarijopen.student.utwente.nl>. ]
> > > 
> > > This patch workarounds the problem in a bit hackish way but without
> > > removing const from generic_chipsets[] (it adds const to __setup() so
> > > __setup_str_ide_generic_all becomes const).
> > 
> > You wouldn't believe how much const data is not marked as const because
> > we don't have __constinitdata etc. Literally megabytes.
> 
> The gain from marking it const is very little and once any non-const
> __initdata object is added to a compilation unit all other const declarations
> will have to be removed.  Bad tradeoff.

We can intrduce new, ro sections or teach gcc that combining const objects into
non-ro sections is not a crime. I wonder why it currently disallows that.
(And it does it only _somethimes_, const pointers happily go into rw sections!)
--
vda

Re: [IDE] Fix build bug

[Ralf Baechle]

On Thu, Nov 01, 2007 at 06:43:16PM +0000, Denys Vlasenko wrote:

> We can intrduce new, ro sections or teach gcc that combining const objects into
> non-ro sections is not a crime. I wonder why it currently disallows that.
> (And it does it only _somethimes_, const pointers happily go into rw sections!)

The pattern seems to be that const-ness of the first object placed into
a particular section determines the writability of that section.  If that
conflicts with the requirements for a later object such as a non-const
object into a section r/o gcc doesn't consider making the section r/w
but throws an error instead.

  Ralf