* Re: acpi_cpu_freq_init warning...
2008-05-06 6:29 ` OGAWA Hirofumi
@ 2008-05-06 6:47 ` Greg KH
2008-05-06 7:13 ` OGAWA Hirofumi
2008-05-06 14:47 ` Linus Torvalds
2008-05-06 15:44 ` Thomas Renninger
2 siblings, 1 reply; 10+ messages in thread
From: Greg KH @ 2008-05-06 6:47 UTC (permalink / raw)
To: OGAWA Hirofumi
Cc: Yinghai Lu, Linus Torvalds, Ingo Molnar, Len Brown,
linux-kernel@vger.kernel.org, linux-acpi
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: acpi_cpu_freq_init warning...
2008-05-06 6:47 ` Greg KH
@ 2008-05-06 7:13 ` OGAWA Hirofumi
0 siblings, 0 replies; 10+ messages in thread
From: OGAWA Hirofumi @ 2008-05-06 7:13 UTC (permalink / raw)
To: Greg KH
Cc: Yinghai Lu, Linus Torvalds, Ingo Molnar, Len Brown,
linux-kernel@vger.kernel.org, linux-acpi
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>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: acpi_cpu_freq_init warning...
2008-05-06 6:29 ` OGAWA Hirofumi
2008-05-06 6:47 ` Greg KH
@ 2008-05-06 14:47 ` Linus Torvalds
2008-05-06 19:02 ` OGAWA Hirofumi
2008-05-06 15:44 ` Thomas Renninger
2 siblings, 1 reply; 10+ messages in thread
From: Linus Torvalds @ 2008-05-06 14:47 UTC (permalink / raw)
To: OGAWA Hirofumi
Cc: Yinghai Lu, Greg KH, Ingo Molnar, Len Brown,
linux-kernel@vger.kernel.org, linux-acpi
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: acpi_cpu_freq_init warning...
2008-05-06 14:47 ` Linus Torvalds
@ 2008-05-06 19:02 ` OGAWA Hirofumi
2008-05-06 20:20 ` Greg KH
0 siblings, 1 reply; 10+ messages in thread
From: OGAWA Hirofumi @ 2008-05-06 19:02 UTC (permalink / raw)
To: Linus Torvalds
Cc: Yinghai Lu, Greg KH, Ingo Molnar, Len Brown,
linux-kernel@vger.kernel.org, linux-acpi
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>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: acpi_cpu_freq_init warning...
2008-05-06 19:02 ` OGAWA Hirofumi
@ 2008-05-06 20:20 ` Greg KH
2008-05-06 20:27 ` Linus Torvalds
0 siblings, 1 reply; 10+ messages in thread
From: Greg KH @ 2008-05-06 20:20 UTC (permalink / raw)
To: OGAWA Hirofumi
Cc: Linus Torvalds, Yinghai Lu, Ingo Molnar, Len Brown,
linux-kernel@vger.kernel.org, linux-acpi
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: acpi_cpu_freq_init warning...
2008-05-06 20:20 ` Greg KH
@ 2008-05-06 20:27 ` Linus Torvalds
2008-05-06 21:38 ` Greg KH
0 siblings, 1 reply; 10+ messages in thread
From: Linus Torvalds @ 2008-05-06 20:27 UTC (permalink / raw)
To: Greg KH
Cc: OGAWA Hirofumi, Yinghai Lu, Ingo Molnar, Len Brown,
linux-kernel@vger.kernel.org, linux-acpi
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: acpi_cpu_freq_init warning...
2008-05-06 20:27 ` Linus Torvalds
@ 2008-05-06 21:38 ` Greg KH
0 siblings, 0 replies; 10+ messages in thread
From: Greg KH @ 2008-05-06 21:38 UTC (permalink / raw)
To: Linus Torvalds
Cc: OGAWA Hirofumi, Yinghai Lu, Ingo Molnar, Len Brown,
linux-kernel@vger.kernel.org, linux-acpi
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: acpi_cpu_freq_init warning...
2008-05-06 6:29 ` OGAWA Hirofumi
2008-05-06 6:47 ` Greg KH
2008-05-06 14:47 ` Linus Torvalds
@ 2008-05-06 15:44 ` Thomas Renninger
2 siblings, 0 replies; 10+ messages in thread
From: Thomas Renninger @ 2008-05-06 15:44 UTC (permalink / raw)
To: OGAWA Hirofumi
Cc: Yinghai Lu, Greg KH, Linus Torvalds, Ingo Molnar, Len Brown,
linux-kernel@vger.kernel.org, linux-acpi, Jiri Kosina,
Pallipadi, Venkatesh
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 "
_
^ permalink raw reply [flat|nested] 10+ messages in thread