device_pm_add (was: Re: 2.6.25-git2: BUG: unable to handle kernel paging request at ffffffffffffffff)

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Tuesday, 22 of April 2008, Linus Torvalds wrote:
> 
> On Tue, 22 Apr 2008, Zdenek Kabelac wrote:
> > 
> > Unsure how it is related to my orginal Oops post - but now when I've
> > debug pagealloc enabled this appeared in my log after resume - should
> > I open new bug for this - or could this be part of the problem I've
> > experienced later?
> > 
> > (Note - now I'm running commit: 8a81f2738f10ca817c975cec893aa58497e873b2
> > 
> >  sd 0:0:0:0: [sda] Starting disk
> >  mmc0: new SD card at address 5a61
> >  mmc mmc0:5a61: parent mmc0 is sleeping, will not add
> >  ------------[ cut here ]------------
> >  WARNING: at drivers/base/power/main.c:78 device_pm_add+0x6c/0xf0()
> 
> This is unrelated to the other issue, I think.
> 
> Your warning comes from commit 58aca23226a19983571bd3b65167521fc64f5869, 
> which admittedly looks like total crap.

Well, I'm sorry that you think so.

> Rafael, what's the point of that commit?

More or less as stated in the changelog.  If we register a child of a sleeping
device, the child ends up on dpm_active before the parent, so the ordering will
be wrong during the next suspend.

That was discussed on linux-pm, mainly with Alan Stern.

> I read the commit message, but I can't make myself agree with the commit 
> code itself. If it's a "checking that the order is correct" thing, it 
> should be a warning, but not change the actual _action_ of the code.

That is easy to change.  Please find appended a patch for that.

> Because the commit refused to add the device, it is also then the direct 
> reason for the oops you get later, as far as I can tell:
> 
> >  BUG: unable to handle kernel NULL pointer dereference at 0000000000000050
> >  IP: [klist_del+29/128] klist_del+0x1d/0x80
> >  PGD 0
> >  Oops: 0000 [1] PREEMPT SMP DEBUG_PAGEALLOC
> >  CPU 0
> >  Call Trace:
> >   [bus_remove_device+158/208] bus_remove_device+0x9e/0xd0
> >   [device_add+1358/1376] device_add+0x54e/0x560

There is a bug in device_add() that IMO can be fixed this way:

Index: linux-2.6/drivers/base/core.c
===================================================================
--- linux-2.6.orig/drivers/base/core.c
+++ linux-2.6/drivers/base/core.c
@@ -820,11 +820,11 @@ int device_add(struct device *dev)
 	error = bus_add_device(dev);
 	if (error)
 		goto BusError;
+	bus_attach_device(dev);
 	error = device_pm_add(dev);
 	if (error)
 		goto PMError;
 	kobject_uevent(&dev->kobj, KOBJ_ADD);
-	bus_attach_device(dev);
 	if (parent)
 		klist_add_tail(&dev->knode_parent, &parent->klist_children);
 
The problem is that bus_remove_device() assumes bus_attach_device() to have
run, AFAICS.

> So I would suggest reverting that commit, or at least just making it a 
> warning (while still registering the device).

Are drivers supposed to register children of suspended devices?  That doesn't
make much sense IMO ...

Thanks,
Rafael


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/main.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -76,12 +76,10 @@ int device_pm_add(struct device *dev)
 		else
 			dev_warn(dev, "devices are sleeping, will not add\n");
 		WARN_ON(true);
-		error = -EBUSY;
-	} else {
-		error = dpm_sysfs_add(dev);
-		if (!error)
-			list_add_tail(&dev->power.entry, &dpm_active);
 	}
+	error = dpm_sysfs_add(dev);
+	if (!error)
+		list_add_tail(&dev->power.entry, &dpm_active);
 	mutex_unlock(&dpm_list_mtx);
 	return error;
 }