Re: [RFC/PATCH 2/2] driver core: power management debugging

[Greg KH <greg@kroah.com>]

On Sat, Apr 28, 2007 at 07:38:17AM +1000, Nigel Cunningham wrote:
> Hi.
> 
> On Fri, 2007-04-27 at 08:40 -0700, Greg KH wrote:
> > On Fri, Apr 27, 2007 at 03:25:46PM +0300, Pekka J Enberg wrote:
> > > From: Nigel Cunningham <nigel@nigel.suspend2.net>
> > > 
> > > Add power management related debugging into driver core. Make the
> > > kernel complain if a device driver lacks bus and class support for
> > > resume or if a PCI or USB driver does not have a driver specific
> > > resume function.
> > > 
> > > Cc: Pavel Machek <pavel@ucw.cz>
> > > Cc: Rafael J. Wysocki <rjw@sisk.pl>
> > > Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
> > > ---
> > >  drivers/base/core.c       |   13 +++++++++++++
> > >  drivers/pci/pci-driver.c  |    6 ++++++
> > >  drivers/usb/core/driver.c |    6 ++++++
> > >  include/linux/device.h    |    1 +
> > >  4 files changed, 26 insertions(+)
> > > 
> > > Index: 2.6/drivers/base/core.c
> > > ===================================================================
> > > --- 2.6.orig/drivers/base/core.c	2007-04-27 14:42:13.000000000 +0300
> > > +++ 2.6/drivers/base/core.c	2007-04-27 14:43:14.000000000 +0300
> > > @@ -652,6 +652,18 @@ int device_add(struct device *dev)
> > >  				class_intf->add_dev(dev, class_intf);
> > >  		up(&dev->class->sem);
> > >  	}
> > > +
> > > +#ifdef CONFIG_PM
> > > +	if (!((dev->class && dev->class->resume) ||
> > > +	      (dev->bus && (dev->bus->resume || dev->bus->resume_early))) &&
> > > +	    !dev->pm_safe) {
> > > +		printk(KERN_WARNING "Device driver %s lacks bus and class "
> > > +				    "support for being resumed.\n",
> > > +				    kobject_name(&dev->kobj));
> > > +		dump_stack();
> > > +	}
> > > +#endif
> > 
> > 
> > I think you are reporting the wrong thing here, we want to know about
> > the busses and classes without suspend support, not the individual
> > devices, right?
> 
> Yeah, I suppose you're right. I started off seeking to make a message
> for each device lacking anything that would save its state. Guess I got
> muddled somewhere :)
> 
> > And dumping stuff like this to the log for every bus/class isn't the
> > nicest :(
> > 
> > 
> > > +
> > >   Done:
> > >   	kfree(class_name);
> > >  	put_device(dev);
> > > @@ -989,6 +1001,7 @@ struct device *device_create(struct clas
> > >  	dev->class = class;
> > >  	dev->parent = parent;
> > >  	dev->release = device_create_release;
> > > +	dev->pm_safe = 1;
> > 
> > I don't understand the use of this flag, it looks like it is only being
> > set, which doesn't really make it very useful.
> 
> The use was above (!dev->pm_safe).

Yes, but nothing was ever turnning that flag off, so how could that
check ever fail?

Oh wait, it's only set in device_create()...

Ugh, device_create is _not_ the proper place for that, lots of kernel
code adds struct device to the tree with a class assigned to it without
using device_create(), that's just there to be a helper for people who
don't need the additional flexibility.

So that code isn't even doing what you think it is doing :)

This patch is pretty much completely wrong :(

thanks,

greg k-h