[PATCH] fix setting of sn_hub_info->shub_1_1_found

[PATCH] fix setting of sn_hub_info->shub_1_1_found

[Dean Nelson]

Fix a bug in which shub_1_1_found is not being properly initialized or set,
resulting in the improper setting of sn_hub_info->shub_1_1_found.

Signed-off-by: Dean Nelson <dcn@sgi.com>


Index: test-2.6.git/arch/ia64/sn/kernel/setup.c
=================================--- test-2.6.git.orig/arch/ia64/sn/kernel/setup.c	2005-06-03 06:46:36.157095014 -0500
+++ test-2.6.git/arch/ia64/sn/kernel/setup.c	2005-06-03 06:50:07.272080003 -0500
@@ -216,7 +216,7 @@
 
 extern int platform_intr_list[];
 extern nasid_t master_nasid;
-static int shub_1_1_found __initdata;
+static int __initdata shub_1_1_found = 0;
 
 /*
  * sn_check_for_wars
@@ -245,7 +245,7 @@
 	} else {
 		for_each_online_node(cnode) {
 			if (is_shub_1_1(cnodeid_to_nasid(cnode)))
-				sn_hub_info->shub_1_1_found = 1;
+				shub_1_1_found = 1;
 		}
 	}
 }

Re: [PATCH] fix setting of sn_hub_info->shub_1_1_found

[Robin Holt]

On Fri, Jun 03, 2005 at 07:25:28AM -0500, Dean Nelson wrote:
>  		for_each_online_node(cnode) {
>  			if (is_shub_1_1(cnodeid_to_nasid(cnode)))
> -				sn_hub_info->shub_1_1_found = 1;
> +				shub_1_1_found = 1;

This explains why XPC has been so troublesome to load on older systems.
Thanks Dean!

Tony, can this still get into 2.6.12?

Thanks,
Robin

RE: [PATCH] fix setting of sn_hub_info->shub_1_1_found

[Luck, Tony]

>This explains why XPC has been so troublesome to load on older systems.
>Thanks Dean!
>
>Tony, can this still get into 2.6.12?

Probably ... but the first hunk of the patch looks like a no-op,
and contravenes some style guidlines about initializing global
variables to 0.

-static int shub_1_1_found __initdata;
+static int __initdata shub_1_1_found = 0;

Presumably you'd still like the second hunk?

-Tony

Re: [PATCH] fix setting of sn_hub_info->shub_1_1_found

[Dean Nelson]

On Fri, Jun 03, 2005 at 11:50:55AM -0700, Luck, Tony wrote:
> >This explains why XPC has been so troublesome to load on older systems.
> >Thanks Dean!
> >
> >Tony, can this still get into 2.6.12?
> 
> Probably ... but the first hunk of the patch looks like a no-op,
> and contravenes some style guidlines about initializing global
> variables to 0.
> 
> -static int shub_1_1_found __initdata;
> +static int __initdata shub_1_1_found = 0;

It was my understanding from linux/Documentation/DocBook/kernel-hacking.tmpl
that '__initdata' meant that the variable shub_1_1_found was not initialized
to anything (unlike ordinary static data).

Perhaps that is no longer (or never was) true? If so, then indeed ignore
the first hunk.

> Presumably you'd still like the second hunk?

But definitely apply the second hunk.

Thanks,
Dean

RE: [PATCH] fix setting of sn_hub_info->shub_1_1_found

[Luck, Tony]

>It was my understanding from linux/Documentation/DocBook/kernel-hacking.tmpl
>that '__initdata' meant that the variable shub_1_1_found was not initialized
>to anything (unlike ordinary static data).

I'd never noticed that ... but it does make sense.  I wonder whether there
are any other __initdata declarations, written by people as clueless as me,
that are wrong?

-Tony

Re: [PATCH] fix setting of sn_hub_info->shub_1_1_found

[Dean Nelson]

On Fri, Jun 03, 2005 at 12:36:07PM -0700, Luck, Tony wrote:
> >It was my understanding from linux/Documentation/DocBook/kernel-hacking.tmpl
> >that '__initdata' meant that the variable shub_1_1_found was not initialized
> >to anything (unlike ordinary static data).
> 
> I'd never noticed that ... but it does make sense.  I wonder whether there
> are any other __initdata declarations, written by people as clueless as me,
> that are wrong?

I'd assume there are other uninitialized uses of __initdata declarations.
This one had been.

I'm assuming that you will be keeping my first hunk.

RE: [PATCH] fix setting of sn_hub_info->shub_1_1_found

[Luck, Tony]

>I'm assuming that you will be keeping my first hunk.

Yes.  I applied both hunks.  I should show up in my GIT tree when
kernel.org does the magic mirror thing from the back-end machines
to the web-server (a process that has been having problems recently).

-Tony

Re: [PATCH] fix setting of sn_hub_info->shub_1_1_found

[Russ Anderson]

Tony Luck wrote:
> 
> >It was my understanding from linux/Documentation/DocBook/kernel-hacking.tmpl
> >that '__initdata' meant that the variable shub_1_1_found was not initialized
> >to anything (unlike ordinary static data).
> 
> I'd never noticed that ... but it does make sense.  I wonder whether there
> are any other __initdata declarations, written by people as clueless as me,
> that are wrong?

Looks like a few in arch/ia64/kernel/acpi.c

Signed-off-by: Russ Anderson (rja@sgi.com)

---------------------------------------------------------
Index: linux-2.6.git/arch/ia64/kernel/acpi.c
=================================--- linux-2.6.git.orig/arch/ia64/kernel/acpi.c	2005-05-16 15:09:02.442863872 -0500
+++ linux-2.6.git/arch/ia64/kernel/acpi.c	2005-06-03 15:57:47.581162358 -0500
@@ -163,8 +163,8 @@
                             Boot-time Table Parsing
    -------------------------------------------------------------------------- */
 
-static int			total_cpus __initdata;
-static int			available_cpus __initdata;
+static int __initdata		total_cpus = 0;
+static int __initdata		available_cpus = 0;
 struct acpi_table_madt *	acpi_madt __initdata;
 static u8			has_8259;
 
@@ -356,7 +356,7 @@
 
 #define PXM_FLAG_LEN ((MAX_PXM_DOMAINS + 1)/32)
 
-static int __initdata srat_num_cpus;			/* number of cpus */
+static int __initdata srat_num_cpus = 0;		/* number of cpus */
 static u32 __devinitdata pxm_flag[PXM_FLAG_LEN];
 #define pxm_bit_set(bit)	(set_bit(bit,(void *)pxm_flag))
 #define pxm_bit_test(bit)	(test_bit(bit,(void *)pxm_flag))

Re: [PATCH] fix setting of sn_hub_info->shub_1_1_found

[David Mosberger]

>>>>> On Fri, 3 Jun 2005 16:09:25 -0500 (CDT), Russ Anderson <rja@sgi.com> said:

  Russ> Tony Luck wrote:

  >>  >It was my understanding from
  >> linux/Documentation/DocBook/kernel-hacking.tmpl >that
  >> '__initdata' meant that the variable shub_1_1_found was not
  >> initialized >to anything (unlike ordinary static data).

  >> I'd never noticed that ... but it does make sense.

Is it really true though?  .init.data is a section that is loaded from
the ELF file and AFAIK the toolchain zeroes data not explicitly
initialized.

	--david

Re: [PATCH] fix setting of sn_hub_info->shub_1_1_found

[Keith Owens]

On Fri, 3 Jun 2005 11:50:55 -0700, 
"Luck, Tony" <tony.luck@intel.com> wrote:
>>This explains why XPC has been so troublesome to load on older systems.
>>Thanks Dean!
>>
>>Tony, can this still get into 2.6.12?
>
>Probably ... but the first hunk of the patch looks like a no-op,
>and contravenes some style guidlines about initializing global
>variables to 0.
>
>-static int shub_1_1_found __initdata;
>+static int __initdata shub_1_1_found = 0;

There is a linker restriction that you must initialise variables that
are to be stored in different sections, which is what __initdata does.
without initialization shub_1_1_found ends up in .bss, with
initialization it ends up in .init.data.

RE: [PATCH] fix setting of sn_hub_info->shub_1_1_found

[Luck, Tony]

>There is a linker restriction that you must initialise variables that
>are to be stored in different sections, which is what __initdata does.
>without initialization shub_1_1_found ends up in .bss, with
>initialization it ends up in .init.data.

Keith,

Thanks for the explanation.

At least things will still work as expected, we just won't free up the
memory for uninitialized __initdata objects, which is better than having
__initdata objects initialized to random values.

But it does mean that the docs are right, and that we should fix up
other places where __initdata objects do not have initializers.

-Tony

Re: [PATCH] fix setting of sn_hub_info->shub_1_1_found

[David Mosberger]

>>>>> On Sat, 04 Jun 2005 14:18:16 +1000, Keith Owens <kaos@sgi.com> said:

  Keith> On Fri, 3 Jun 2005 11:50:55 -0700, 
  Keith> "Luck, Tony" <tony.luck@intel.com> wrote:
  >>> This explains why XPC has been so troublesome to load on older systems.
  >>> Thanks Dean!
  >>> 
  >>> Tony, can this still get into 2.6.12?
  >> 
  >> Probably ... but the first hunk of the patch looks like a no-op,
  >> and contravenes some style guidlines about initializing global
  >> variables to 0.
  >> 
  >> -static int shub_1_1_found __initdata;
  >> +static int __initdata shub_1_1_found = 0;

  Keith> There is a linker restriction that you must initialise variables that
  Keith> are to be stored in different sections, which is what __initdata does.
  Keith> without initialization shub_1_1_found ends up in .bss, with
  Keith> initialization it ends up in .init.data.

Interesting.  Strikes me as a toolchain-bug, though.  Clearly the
__initdata declaration is in conflict with putting the data in .bss,
yet there is no warning or error.

	--david

Re: [PATCH] fix setting of sn_hub_info->shub_1_1_found

[Andreas Schwab]

David Mosberger <davidm@napali.hpl.hp.com> writes:

>>>>>> On Sat, 04 Jun 2005 14:18:16 +1000, Keith Owens <kaos@sgi.com> said:
>
>   Keith> On Fri, 3 Jun 2005 11:50:55 -0700, 
>   Keith> "Luck, Tony" <tony.luck@intel.com> wrote:
>   >>> This explains why XPC has been so troublesome to load on older systems.
>   >>> Thanks Dean!
>   >>> 
>   >>> Tony, can this still get into 2.6.12?
>   >> 
>   >> Probably ... but the first hunk of the patch looks like a no-op,
>   >> and contravenes some style guidlines about initializing global
>   >> variables to 0.
>   >> 
>   >> -static int shub_1_1_found __initdata;
>   >> +static int __initdata shub_1_1_found = 0;
>
>   Keith> There is a linker restriction that you must initialise variables that
>   Keith> are to be stored in different sections, which is what __initdata does.
>   Keith> without initialization shub_1_1_found ends up in .bss, with
>   Keith> initialization it ends up in .init.data.
>
> Interesting.  Strikes me as a toolchain-bug, though.  Clearly the
> __initdata declaration is in conflict with putting the data in .bss,
> yet there is no warning or error.

Can't reproduce that with gcc 3.2, or anything newer.

$ cat section.c
int __attribute__((section(".init.data"))) foo;
$ gcc -S section.c
$ cat section.s
	.file	"section.c"
	.pred.safe_across_calls p1-p5,p16-p63
	.global foo#
	.section	.init.data,"aw",@progbits
	.align 4
	.type	foo#,@object
	.size	foo#,4
foo:
	.skip	4
	.ident	"GCC: (GNU) 3.2.2"

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] fix setting of sn_hub_info->shub_1_1_found

[David Mosberger]

>>>>> On Mon, 06 Jun 2005 19:04:21 +0200, Andreas Schwab <schwab@suse.de> said:

  Andreas> David Mosberger <davidm@napali.hpl.hp.com> writes:
  >>>>>>> On Sat, 04 Jun 2005 14:18:16 +1000, Keith Owens <kaos@sgi.com> said:
  >>
  Keith> On Fri, 3 Jun 2005 11:50:55 -0700,
  Keith> "Luck, Tony" <tony.luck@intel.com> wrote:
  >> >>> This explains why XPC has been so troublesome to load on older systems.
  >> >>> Thanks Dean!
  >> >>>
  >> >>> Tony, can this still get into 2.6.12?
  >> >>
  >> >> Probably ... but the first hunk of the patch looks like a no-op,
  >> >> and contravenes some style guidlines about initializing global
  >> >> variables to 0.
  >> >>
  >> >> -static int shub_1_1_found __initdata;
  >> >> +static int __initdata shub_1_1_found = 0;
  >>
  Keith> There is a linker restriction that you must initialise variables that
  Keith> are to be stored in different sections, which is what __initdata does.
  Keith> without initialization shub_1_1_found ends up in .bss, with
  Keith> initialization it ends up in .init.data.

  >> Interesting.  Strikes me as a toolchain-bug, though.  Clearly the
  >> __initdata declaration is in conflict with putting the data in .bss,
  >> yet there is no warning or error.

  Andreas> Can't reproduce that with gcc 3.2, or anything newer.

  Andreas> $ cat section.c
  Andreas> int __attribute__((section(".init.data"))) foo;
  Andreas> $ gcc -S section.c
  Andreas> $ cat section.s
  Andreas> .file	"section.c"
  Andreas> .pred.safe_across_calls p1-p5,p16-p63
  Andreas> .global foo#
  Andreas> .section	.init.data,"aw",@progbits
  Andreas> .align 4
  Andreas> .type	foo#,@object
  Andreas> .size	foo#,4
  Andreas> foo:
  Andreas> .skip	4
  Andreas> .ident	"GCC: (GNU) 3.2.2"

I remembering GCC 3.x getting more picky about section conflicts.  So
perhaps these changes also fixed this particular issue.  Good.  I
guess it's time to declare the documentation officially out-of-date?

	--david