From mboxrd@z Thu Jan 1 00:00:00 1970 From: Greg KH Subject: Re: acpi_cpu_freq_init warning... Date: Tue, 6 May 2008 13:20:33 -0700 Message-ID: <20080506202033.GA3114@suse.de> References: <86802c440805051951u153ebc21p15f0d73904496217@mail.gmail.com> <87k5i8hrmk.fsf@duaron.myhome.or.jp> <87skwvcl2a.fsf@duaron.myhome.or.jp> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <87skwvcl2a.fsf@duaron.myhome.or.jp> Sender: linux-kernel-owner@vger.kernel.org To: OGAWA Hirofumi Cc: Linus Torvalds , Yinghai Lu , Ingo Molnar , Len Brown , "linux-kernel@vger.kernel.org" , linux-acpi List-Id: linux-acpi@vger.kernel.org On Wed, May 07, 2008 at 04:02:53AM +0900, OGAWA Hirofumi wrote: > Linus Torvalds 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