acpi_cpu_freq_init warning...

acpi_cpu_freq_init warning...

[Yinghai Lu]

calling  acpi_cpufreq_init+0x0/0x68()
boot acpi=off got

calling  powernowk8_init+0x0/0xfd()
powernow-k8: Found 2 Quad-Core AMD Opteron(tm) Processor 2356
processors (8 cpu cores) (version 2.20.00)
powernow-k8: MP systems not supported by PSB BIOS structure
powernow-k8: MP systems not supported by PSB BIOS structure
powernow-k8: MP systems not supported by PSB BIOS structure
powernow-k8: MP systems not supported by PSB BIOS structure
powernow-k8: MP systems not supported by PSB BIOS structure
powernow-k8: MP systems not supported by PSB BIOS structure
powernow-k8: MP systems not supported by PSB BIOS structure
powernow-k8: MP systems not supported by PSB BIOS structure
initcall powernowk8_init+0x0/0xfd() returned -19 after 49 msecs

calling  acpi_cpufreq_init+0x0/0x68()
sysdev: class cpu: driver (ffffffff80da0110) has already been
registered to a class, something is wrong, but will forge on!
------------[ cut here ]------------
WARNING: at drivers/base/sys.c:183 sysdev_driver_register+0x82/0x150()
Modules linked in:
Pid: 1, comm: swapper Not tainted
2.6.26-rc1-sched-devel.git-x86-latest.git-00485-g253148c-dirty #333

Call Trace:
 [<ffffffff802611c6>] warn_on_slowpath+0x67/0xa2
 [<ffffffff80262582>] ? printk+0xa0/0xc1
 [<ffffffff80504b31>] ? __next_cpu_nr+0x31/0x51
 [<ffffffff80504bc1>] ? __first_cpu+0x21/0x44
 [<ffffffff80562ac8>] ? acpi_processor_preregister_performance+0x51c/0x544
 [<ffffffff8059d0b8>] sysdev_driver_register+0x82/0x150
 [<ffffffff8082de67>] cpufreq_register_driver+0xa8/0x159
 [<ffffffff80e40b3f>] acpi_cpufreq_init+0x52/0x68
 [<ffffffff80e33c38>] kernel_init+0x1ec/0x32c
 [<ffffffff8022a398>] child_rip+0xa/0x12
 [<ffffffff80a15fd3>] ? _etext+0x0/0x1
 [<ffffffff80e33a4c>] ? kernel_init+0x0/0x32c
 [<ffffffff8022a38e>] ? child_rip+0x0/0x12

---[ end trace 42a7898e9c11b2ed ]---
initcall acpi_cpufreq_init+0x0/0x68() returned -19 after 87 msecs


---[ end trace 42a7898e9c11b2ed ]---

Re: acpi_cpu_freq_init warning...

[OGAWA Hirofumi]

> calling  acpi_cpufreq_init+0x0/0x68()
> sysdev: class cpu: driver (ffffffff80da0110) has already been
> registered to a class, something is wrong, but will forge on!
> ------------[ cut here ]------------
> WARNING: at drivers/base/sys.c:183 sysdev_driver_register+0x82/0x150()
> Modules linked in:
> Pid: 1, comm: swapper Not tainted
> 2.6.26-rc1-sched-devel.git-x86-latest.git-00485-g253148c-dirty #333

I've seen this too. The following patch seems to work for me.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>


[PATCH] Fix bogus warning in sysdev_driver_register()

	if ((drv->entry.next != drv->entry.prev) ||
	    (drv->entry.next != NULL)) {

warns list_empty(&drv->entry).

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 drivers/base/sys.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -puN drivers/base/sys.c~fix-sys-bogus-warning drivers/base/sys.c
--- linux-2.6/drivers/base/sys.c~fix-sys-bogus-warning	2008-05-06 14:30:42.000000000 +0900
+++ linux-2.6-hirofumi/drivers/base/sys.c	2008-05-06 14:30:45.000000000 +0900
@@ -175,7 +175,7 @@ int sysdev_driver_register(struct sysdev
 	}
 
 	/* Check whether this driver has already been added to a class. */
-	if ((drv->entry.next != drv->entry.prev) ||
+	if ((drv->entry.next != drv->entry.prev) &&
 	    (drv->entry.next != NULL)) {
 		printk(KERN_WARNING "sysdev: class %s: driver (%p) has already"
 			" been registered to a class, something is wrong, but "
_

Re: acpi_cpu_freq_init warning...

[Greg KH]

On Tue, May 06, 2008 at 03:29:55PM +0900, OGAWA Hirofumi wrote:
> > calling  acpi_cpufreq_init+0x0/0x68()
> > sysdev: class cpu: driver (ffffffff80da0110) has already been
> > registered to a class, something is wrong, but will forge on!
> > ------------[ cut here ]------------
> > WARNING: at drivers/base/sys.c:183 sysdev_driver_register+0x82/0x150()
> > Modules linked in:
> > Pid: 1, comm: swapper Not tainted
> > 2.6.26-rc1-sched-devel.git-x86-latest.git-00485-g253148c-dirty #333
> 
> I've seen this too. The following patch seems to work for me.
> -- 
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> 
> 
> [PATCH] Fix bogus warning in sysdev_driver_register()
> 
> 	if ((drv->entry.next != drv->entry.prev) ||
> 	    (drv->entry.next != NULL)) {
> 
> warns list_empty(&drv->entry).

should we just call list_empty() here to make sure it is correct?

thanks,

greg k-h

Re: acpi_cpu_freq_init warning...

[OGAWA Hirofumi]

Greg KH <gregkh@suse.de> writes:

> On Tue, May 06, 2008 at 03:29:55PM +0900, OGAWA Hirofumi wrote:
>> > calling  acpi_cpufreq_init+0x0/0x68()
>> > sysdev: class cpu: driver (ffffffff80da0110) has already been
>> > registered to a class, something is wrong, but will forge on!
>> > ------------[ cut here ]------------
>> > WARNING: at drivers/base/sys.c:183 sysdev_driver_register+0x82/0x150()
>> > Modules linked in:
>> > Pid: 1, comm: swapper Not tainted
>> > 2.6.26-rc1-sched-devel.git-x86-latest.git-00485-g253148c-dirty #333
>> 
>> I've seen this too. The following patch seems to work for me.
>> -- 
>> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>> 
>> 
>> [PATCH] Fix bogus warning in sysdev_driver_register()
>> 
>> 	if ((drv->entry.next != drv->entry.prev) ||
>> 	    (drv->entry.next != NULL)) {
>> 
>> warns list_empty(&drv->entry).
>
> should we just call list_empty() here to make sure it is correct?

Unfortunately we can't. drv->entry is initialized by NULL like following.

static struct sysdev_driver cpufreq_sysdev_driver = {
	.add		= cpufreq_add_dev,
	.remove		= cpufreq_remove_dev,
	.suspend	= cpufreq_suspend,
	.resume		= cpufreq_resume,
};

I assumed this (for compatible) is an intent.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: acpi_cpu_freq_init warning...

[Linus Torvalds]

On Tue, 6 May 2008, OGAWA Hirofumi wrote:
> -	if ((drv->entry.next != drv->entry.prev) ||
> +	if ((drv->entry.next != drv->entry.prev) &&
>  	    (drv->entry.next != NULL)) {

Umm. That code still makes no sense.

The "drv->entry.next == drv->entry.prev" condition will trigger under 
*three* different circumstances:

 - next/prev == NULL (uninitialized). Checked for by the explicit check 
   against NULL.

 - list empty (both next/prev point back to itself), which I assume the 
   check was *meant* for.

 - list has only *one* entry, when next/prev both point to the list head.

and I'm pretty damn sure that whoever wrote that code didn't mean that 
last one, but who knows..

The fact is, looking at next/prev this way is a sure way to have bugs.

What is that PoS *trying* to test for? I assume it is meant to test for

	/* Is the list initialized and non-empty? */
	if (drv->entry.next && !list_empty(&drv->entry)) {
		...

and dammit, just doing it that way is shorter and simpler.

				Linus

Re: acpi_cpu_freq_init warning...

[Thomas Renninger]

On Tue, 2008-05-06 at 15:29 +0900, OGAWA Hirofumi wrote:
> > calling  acpi_cpufreq_init+0x0/0x68()
> > sysdev: class cpu: driver (ffffffff80da0110) has already been
> > registered to a class, something is wrong, but will forge on!
> > ------------[ cut here ]------------
> > WARNING: at drivers/base/sys.c:183 sysdev_driver_register+0x82/0x150()
> > Modules linked in:
> > Pid: 1, comm: swapper Not tainted
> > 2.6.26-rc1-sched-devel.git-x86-latest.git-00485-g253148c-dirty #333
> 
> I've seen this too. The following patch seems to work for me.

Venkatesh Greg, should this patch also go into 2.6.25 stable kernel?
Not 100% sure, but IIRC Jiri reported the same problem on a 2.6.25
kernel.

OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>


[PATCH] Fix bogus warning in sysdev_driver_register()

        if ((drv->entry.next != drv->entry.prev) ||
            (drv->entry.next != NULL)) {

warns list_empty(&drv->entry).

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 drivers/base/sys.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -puN drivers/base/sys.c~fix-sys-bogus-warning drivers/base/sys.c
--- linux-2.6/drivers/base/sys.c~fix-sys-bogus-warning  2008-05-06 14:30:42.000000000 +0900
+++ linux-2.6-hirofumi/drivers/base/sys.c       2008-05-06 14:30:45.000000000 +0900
@@ -175,7 +175,7 @@ int sysdev_driver_register(struct sysdev
        }
 
        /* Check whether this driver has already been added to a class. */
-       if ((drv->entry.next != drv->entry.prev) ||
+       if ((drv->entry.next != drv->entry.prev) &&
            (drv->entry.next != NULL)) {
                printk(KERN_WARNING "sysdev: class %s: driver (%p) has already"
                        " been registered to a class, something is wrong, but "
_

Re: acpi_cpu_freq_init warning...

[OGAWA Hirofumi]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Tue, 6 May 2008, OGAWA Hirofumi wrote:
>> -	if ((drv->entry.next != drv->entry.prev) ||
>> +	if ((drv->entry.next != drv->entry.prev) &&
>>  	    (drv->entry.next != NULL)) {
>
> Umm. That code still makes no sense.
>
> The "drv->entry.next == drv->entry.prev" condition will trigger under 
> *three* different circumstances:
>
>  - next/prev == NULL (uninitialized). Checked for by the explicit check 
>    against NULL.
>
>  - list empty (both next/prev point back to itself), which I assume the 
>    check was *meant* for.
>
>  - list has only *one* entry, when next/prev both point to the list head.
>
> and I'm pretty damn sure that whoever wrote that code didn't mean that 
> last one, but who knows..
>
> The fact is, looking at next/prev this way is a sure way to have bugs.
>
> What is that PoS *trying* to test for? I assume it is meant to test for
>
> 	/* Is the list initialized and non-empty? */
> 	if (drv->entry.next && !list_empty(&drv->entry)) {
> 		...
>
> and dammit, just doing it that way is shorter and simpler.

Whoops, sorry. You are right.

diff -puN drivers/base/sys.c~fix-sys-bogus-warning drivers/base/sys.c
--- linux-2.6/drivers/base/sys.c~fix-sys-bogus-warning	2008-05-07 03:51:00.000000000 +0900
+++ linux-2.6-hirofumi/drivers/base/sys.c	2008-05-07 04:01:14.000000000 +0900
@@ -175,8 +175,7 @@ int sysdev_driver_register(struct sysdev
 	}
 
 	/* Check whether this driver has already been added to a class. */
-	if ((drv->entry.next != drv->entry.prev) ||
-	    (drv->entry.next != NULL)) {
+	if (drv->entry.next && !list_empty(&drv->entry)) {
 		printk(KERN_WARNING "sysdev: class %s: driver (%p) has already"
 			" been registered to a class, something is wrong, but "
 			"will forge on!\n", cls->name, drv);
_
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: acpi_cpu_freq_init warning...

[Greg KH]

On Wed, May 07, 2008 at 04:02:53AM +0900, OGAWA Hirofumi wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > On Tue, 6 May 2008, OGAWA Hirofumi wrote:
> >> -	if ((drv->entry.next != drv->entry.prev) ||
> >> +	if ((drv->entry.next != drv->entry.prev) &&
> >>  	    (drv->entry.next != NULL)) {
> >
> > Umm. That code still makes no sense.
> >
> > The "drv->entry.next == drv->entry.prev" condition will trigger under 
> > *three* different circumstances:
> >
> >  - next/prev == NULL (uninitialized). Checked for by the explicit check 
> >    against NULL.
> >
> >  - list empty (both next/prev point back to itself), which I assume the 
> >    check was *meant* for.
> >
> >  - list has only *one* entry, when next/prev both point to the list head.
> >
> > and I'm pretty damn sure that whoever wrote that code didn't mean that 
> > last one, but who knows..
> >
> > The fact is, looking at next/prev this way is a sure way to have bugs.
> >
> > What is that PoS *trying* to test for? I assume it is meant to test for
> >
> > 	/* Is the list initialized and non-empty? */
> > 	if (drv->entry.next && !list_empty(&drv->entry)) {
> > 		...
> >
> > and dammit, just doing it that way is shorter and simpler.

But I don't think that will work as others have pointed out, this
structure's list field isn't initialized yet.

> Whoops, sorry. You are right.
> 
> diff -puN drivers/base/sys.c~fix-sys-bogus-warning drivers/base/sys.c
> --- linux-2.6/drivers/base/sys.c~fix-sys-bogus-warning	2008-05-07 03:51:00.000000000 +0900
> +++ linux-2.6-hirofumi/drivers/base/sys.c	2008-05-07 04:01:14.000000000 +0900
> @@ -175,8 +175,7 @@ int sysdev_driver_register(struct sysdev
>  	}
>  
>  	/* Check whether this driver has already been added to a class. */
> -	if ((drv->entry.next != drv->entry.prev) ||
> -	    (drv->entry.next != NULL)) {
> +	if (drv->entry.next && !list_empty(&drv->entry)) {

Did you try this patch out?

I say rip the whole thing out, it was added to try to make some bugs in
upper layers more obvious, but if it can't be correct, I have no
objection to removing the thing.  Other bad things happen later on if
the developer messes this one up, this is not a 'user can cause this'
type of error at all.

thanks,

greg k-h

Re: acpi_cpu_freq_init warning...

[Linus Torvalds]

On Tue, 6 May 2008, Greg KH wrote:

> On Wed, May 07, 2008 at 04:02:53AM +0900, OGAWA Hirofumi wrote:
> > Linus Torvalds <torvalds@linux-foundation.org> writes:
> > 
> > > On Tue, 6 May 2008, OGAWA Hirofumi wrote:
> > >> -	if ((drv->entry.next != drv->entry.prev) ||
> > >> +	if ((drv->entry.next != drv->entry.prev) &&
> > >>  	    (drv->entry.next != NULL)) {
> > >
> > > Umm. That code still makes no sense.
> > >
> > > The "drv->entry.next == drv->entry.prev" condition will trigger under 
> > > *three* different circumstances:
> > >
> > >  - next/prev == NULL (uninitialized). Checked for by the explicit check 
> > >    against NULL.
> > >
> > >  - list empty (both next/prev point back to itself), which I assume the 
> > >    check was *meant* for.
> > >
> > >  - list has only *one* entry, when next/prev both point to the list head.
> > >
> > > and I'm pretty damn sure that whoever wrote that code didn't mean that 
> > > last one, but who knows..
> > >
> > > The fact is, looking at next/prev this way is a sure way to have bugs.
> > >
> > > What is that PoS *trying* to test for? I assume it is meant to test for
> > >
> > > 	/* Is the list initialized and non-empty? */
> > > 	if (drv->entry.next && !list_empty(&drv->entry)) {
> > > 		...
> > >
> > > and dammit, just doing it that way is shorter and simpler.
> 
> But I don't think that will work as others have pointed out, this
> structure's list field isn't initialized yet.

Umm. And what do you think the test for drv->entry.next is there for?

Ie the assumption is that it's at least zeroed out, if it's not 
initialized. 

Now, admittedly it could be *total* crud, but if so, I'd seriously suggest 
just fixing the callers, rather than passing totally uninitialized 
structures with random crap in it around.

			Linus

Re: acpi_cpu_freq_init warning...

[Greg KH]

On Tue, May 06, 2008 at 01:27:37PM -0700, Linus Torvalds wrote:
> 
> 
> On Tue, 6 May 2008, Greg KH wrote:
> 
> > On Wed, May 07, 2008 at 04:02:53AM +0900, OGAWA Hirofumi wrote:
> > > Linus Torvalds <torvalds@linux-foundation.org> writes:
> > > 
> > > > On Tue, 6 May 2008, OGAWA Hirofumi wrote:
> > > >> -	if ((drv->entry.next != drv->entry.prev) ||
> > > >> +	if ((drv->entry.next != drv->entry.prev) &&
> > > >>  	    (drv->entry.next != NULL)) {
> > > >
> > > > Umm. That code still makes no sense.
> > > >
> > > > The "drv->entry.next == drv->entry.prev" condition will trigger under 
> > > > *three* different circumstances:
> > > >
> > > >  - next/prev == NULL (uninitialized). Checked for by the explicit check 
> > > >    against NULL.
> > > >
> > > >  - list empty (both next/prev point back to itself), which I assume the 
> > > >    check was *meant* for.
> > > >
> > > >  - list has only *one* entry, when next/prev both point to the list head.
> > > >
> > > > and I'm pretty damn sure that whoever wrote that code didn't mean that 
> > > > last one, but who knows..
> > > >
> > > > The fact is, looking at next/prev this way is a sure way to have bugs.
> > > >
> > > > What is that PoS *trying* to test for? I assume it is meant to test for
> > > >
> > > > 	/* Is the list initialized and non-empty? */
> > > > 	if (drv->entry.next && !list_empty(&drv->entry)) {
> > > > 		...
> > > >
> > > > and dammit, just doing it that way is shorter and simpler.
> > 
> > But I don't think that will work as others have pointed out, this
> > structure's list field isn't initialized yet.
> 
> Umm. And what do you think the test for drv->entry.next is there for?

Doh, nevermind, I don't know what I was thinking, this should be fine.

I think I need a nap...

greg k-h