[patch 1/7] typhoon section fix

[patch 1/7] typhoon section fix

[akpm]

From: Andrew Morton <akpm@linux-foundation.org>

gcc-3.4.4 on powerpc:

drivers/net/typhoon.c:137: error: version causes a section type conflict

Cc: Jeff Garzik <jeff@garzik.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/net/typhoon.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff -puN drivers/net/typhoon.c~typhoon-section-fix drivers/net/typhoon.c
--- a/drivers/net/typhoon.c~typhoon-section-fix
+++ a/drivers/net/typhoon.c
@@ -134,7 +134,7 @@ static const int multicast_filter_limit 
 #include "typhoon.h"
 #include "typhoon-firmware.h"
 
-static const char version[] __devinitdata =
+static char version[] __devinitdata =
     "typhoon.c: version " DRV_MODULE_VERSION " (" DRV_MODULE_RELDATE ")\n";
 
 MODULE_AUTHOR("David Dillow <dave@thedillows.org>");
@@ -178,7 +178,7 @@ enum typhoon_cards {
 };
 
 /* directly indexed by enum typhoon_cards, above */
-static const struct typhoon_card_info typhoon_card_info[] __devinitdata = {
+static struct typhoon_card_info typhoon_card_info[] __devinitdata = {
 	{ "3Com Typhoon (3C990-TX)",
 		TYPHOON_CRYPTO_NONE},
 	{ "3Com Typhoon (3CR990-TX-95)",
_

Re: [patch 1/7] typhoon section fix

[David Dillow]

On Fri, 2008-02-08 at 03:11 -0800, akpm@linux-foundation.org wrote:
> From: Andrew Morton <akpm@linux-foundation.org>
> 
> gcc-3.4.4 on powerpc:
> 
> drivers/net/typhoon.c:137: error: version causes a section type conflict
> 
> Cc: Jeff Garzik <jeff@garzik.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

> -static const char version[] __devinitdata =
> +static char version[] __devinitdata =
>      "typhoon.c: version " DRV_MODULE_VERSION " (" DRV_MODULE_RELDATE ")\n";

Wouldn't going to __devinitconst be better? I'll try to whip up a patch
and test-compile it.

Re: [patch 1/7] typhoon section fix

[Andrew Morton]

On Fri, 08 Feb 2008 08:59:10 -0500 David Dillow <dave@thedillows.org> wrote:

> 
> On Fri, 2008-02-08 at 03:11 -0800, akpm@linux-foundation.org wrote:
> > From: Andrew Morton <akpm@linux-foundation.org>
> > 
> > gcc-3.4.4 on powerpc:
> > 
> > drivers/net/typhoon.c:137: error: version causes a section type conflict
> > 
> > Cc: Jeff Garzik <jeff@garzik.org>
> > Cc: Sam Ravnborg <sam@ravnborg.org>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> 
> > -static const char version[] __devinitdata =
> > +static char version[] __devinitdata =
> >      "typhoon.c: version " DRV_MODULE_VERSION " (" DRV_MODULE_RELDATE ")\n";
> 
> Wouldn't going to __devinitconst be better? I'll try to whip up a patch
> and test-compile it.

Sam told me that doesn't work right, that this approach is the one to use
and, iirc, that __devinitcont and friends will be removed.

I'm not sure why, actually.  I think I missed that dicussion.

Re: [patch 1/7] typhoon section fix

[David Dillow]

On Fri, 2008-02-08 at 09:52 -0800, Andrew Morton wrote:
> On Fri, 08 Feb 2008 08:59:10 -0500 David Dillow <dave@thedillows.org> wrote:
> > On Fri, 2008-02-08 at 03:11 -0800, akpm@linux-foundation.org wrote:
> > > From: Andrew Morton <akpm@linux-foundation.org>
> > > 
> > > gcc-3.4.4 on powerpc:
> > > 
> > > drivers/net/typhoon.c:137: error: version causes a section type conflict
> > > 
> > > Cc: Jeff Garzik <jeff@garzik.org>
> > > Cc: Sam Ravnborg <sam@ravnborg.org>
> > > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > 
> > > -static const char version[] __devinitdata =
> > > +static char version[] __devinitdata =
> > >      "typhoon.c: version " DRV_MODULE_VERSION " (" DRV_MODULE_RELDATE ")\n";
> > 
> > Wouldn't going to __devinitconst be better? I'll try to whip up a patch
> > and test-compile it.
> 
> Sam told me that doesn't work right, that this approach is the one to use
> and, iirc, that __devinitcont and friends will be removed.
> 
> I'm not sure why, actually.  I think I missed that dicussion.

Thanks for the searchable terms -- this is the thread, I think:
http://lkml.org/lkml/2008/2/3/99

It looks like Jan had an idea to fold the const into the __devinitconst
marker, and if that seems to be viable, then I'd prefer that route to
keep the const'ness where it is possible.

Otherwise, your patch is fine as-is.

Acked-by: David Dillow <dave@thedillows.org>

Re: [patch 1/7] typhoon section fix

[Sam Ravnborg]

On Fri, Feb 08, 2008 at 01:21:57PM -0500, David Dillow wrote:
> 
> On Fri, 2008-02-08 at 09:52 -0800, Andrew Morton wrote:
> > On Fri, 08 Feb 2008 08:59:10 -0500 David Dillow <dave@thedillows.org> wrote:
> > > On Fri, 2008-02-08 at 03:11 -0800, akpm@linux-foundation.org wrote:
> > > > From: Andrew Morton <akpm@linux-foundation.org>
> > > > 
> > > > gcc-3.4.4 on powerpc:
> > > > 
> > > > drivers/net/typhoon.c:137: error: version causes a section type conflict
> > > > 
> > > > Cc: Jeff Garzik <jeff@garzik.org>
> > > > Cc: Sam Ravnborg <sam@ravnborg.org>
> > > > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > > 
> > > > -static const char version[] __devinitdata =
> > > > +static char version[] __devinitdata =
> > > >      "typhoon.c: version " DRV_MODULE_VERSION " (" DRV_MODULE_RELDATE ")\n";
> > > 
> > > Wouldn't going to __devinitconst be better? I'll try to whip up a patch
> > > and test-compile it.
> > 
> > Sam told me that doesn't work right, that this approach is the one to use
> > and, iirc, that __devinitcont and friends will be removed.
> > 
> > I'm not sure why, actually.  I think I missed that dicussion.

After introducing dedicated sections for __devinit, __devinitdata & friends
and introducing __devinitconst for const data we started to see
section type conflict _errors_ emitted from gcc for certain architectures.
64bit powerpc and ia64 being two of them.

That was rooted down by Al to the following:
===================================================================
; cat >a.c <<'EOF'
const char foo[] __attribute__ ((__section__(".blah"))) = "";
const char * const bar __attribute__((__section__(".blah"))) = "";
EOF
; gcc -m32 -S a.c
; gcc -m64 -S a.c
a.c:2: error: bar causes a section type conflict
;
===================================================================

Which tells us that the same data in some cases are put in a readonly
section and in other cases not.
And when we force data for tow different variables where gcc thinks one is
const and the other is not const to the same section gcc errros
out with the "section type conflict" error.

And this is becoming increasingly visible when people start to constify
the data all around and do test build on x86 64bit that does not exhibit
this behaviour.

The rationale behind the increased constification of data is to get
improved use of the DEBUG_RODATA thing.
Another good reason s that this is a good way to let gcc catch accidental
writes to data that otherwise should have been read-only.

But if we advocate for constification of data than we should have
a reliable way to detect the section type conflict errors on x86
(at least 64 bit) which we do not have. Missing that will casue us to see
an increasing flood of error reports from our power pc and ia64 builders.

The rationale behind __devinitconst was to create a dedicated section
for data marked read-only by gcc and thus avoiding the
section type conflict.

But as shown by the above code snippet this is not easy.
For x86 (32 + 64bit) both variables end up in READ-ONLY section.
For Powerpc 64 bit the variable bar end up in a read-write section.
And I see no way to check this for a x86 build so we warn early and
with the most widely used tool-chain.

So do we have other options that to drop the constification and thus
dropping the __devinitconst and the other __*const annotations - I think not :-(


	Sam

Re: [patch 1/7] typhoon section fix

[Jan Beulich]

>So do we have other options that to drop the constification and thus
>dropping the __devinitconst and the other __*const annotations - I think not :-(

As I had said already in another reply - I think the most reasonable thing
to do is to fold section name attribute *and* const into __*initconst
(while defining __*initconst to __*initdata on those targets not allowing
relocations in read-only sections, which should be done by a promptless
config option to avoid enumerating all the targets each time such a
conditional is needed somewhere). While that doesn't allow the 'const'
compile time checking on those problem targets, it is consistent for all
targets and provides the intended DEBUG_RODATA benefit where
possible. Additionally, the const compile time checking will then be more
tight on the presumable more frequently tested x86 targets, so the
chances of bad constructs slipping in should be reduced.

Jan

Re: [patch 1/7] typhoon section fix

[David Miller]

From: akpm@linux-foundation.org
Date: Fri, 08 Feb 2008 03:11:13 -0800

> From: Andrew Morton <akpm@linux-foundation.org>
> 
> gcc-3.4.4 on powerpc:
> 
> drivers/net/typhoon.c:137: error: version causes a section type conflict
> 
> Cc: Jeff Garzik <jeff@garzik.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

Applied.