Re: [PATCH] PM: Handle unregistering devices during suspend/hibernation

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

On Saturday, 23 of February 2008, Rafael J. Wysocki wrote:
> On Saturday, 23 of February 2008, Greg KH wrote:
> > On Sat, Feb 23, 2008 at 12:53:11AM +0100, Rafael J. Wysocki wrote:
> > > Hi Greg,
> > > 
> > > The appended patch fixes the issue with the new code for suspending/resuming
> > > devices, related to the fact that some device drivers and CPU hotplug notifiers
> > > unregister device objects while suspend is in progress, which leads to
> > > deadlocks.
> > > 
> > > Please consider taking it for 2.6.25.

Ouch, please disregard this patch.

Alan rightfully noticed that it may confuse subsystems assuming that after
device_unregister() has returned, the driver's ->release() method has run.

Unfortunately, he did that in a Bugzilla comment that has never reached
my mailbox (strangely enough, I haven't been receiving any messages from
the Bugzilla for the last two days or so).

Sorry for the trouble.

Thanks,
Rafael


> > > ---
> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > Introduce a mechanism preventing drivers and CPU hotplug notifiers
> > > from deadlocking suspend/hibernation by unregistering device objects
> > > while it is in progress.  Specifically, make device_del() detect if
> > > it has been called by the suspending task and automatically defer the
> > > removal of the device object if that's the case.
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > Acked-by: Alan Stern <stern@rowland.harvard.edu>
> > > ---
> > >  drivers/base/core.c        |    5 +++++
> > >  drivers/base/power/main.c  |    9 +++++++++
> > >  drivers/base/power/power.h |    5 +++++
> > >  3 files changed, 19 insertions(+)
> > > 
> > > 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
> > > @@ -59,6 +59,13 @@ static DECLARE_RWSEM(pm_sleep_rwsem);
> > >  
> > >  int (*platform_enable_wakeup)(struct device *dev, int is_on);
> > >  
> > > +static struct task_struct *suspending_task;
> > > +
> > > +bool in_suspend_context(void)
> > > +{
> > > +	return (suspending_task == current);
> > > +}
> > > +
> > >  /**
> > >   *	device_pm_add - add a device to the list of active devices
> > >   *	@dev:	Device to be added to the list
> > > @@ -272,6 +279,7 @@ static void dpm_resume(void)
> > >  		mutex_lock(&dpm_list_mtx);
> > >  	}
> > >  	mutex_unlock(&dpm_list_mtx);
> > > +	suspending_task = NULL;
> > >  }
> > >  
> > >  /**
> > > @@ -460,6 +468,7 @@ static int dpm_suspend(pm_message_t stat
> > >  {
> > >  	int error = 0;
> > >  
> > > +	suspending_task = current;
> > >  	mutex_lock(&dpm_list_mtx);
> > >  	while (!list_empty(&dpm_locked)) {
> > >  		struct list_head *entry = dpm_locked.prev;
> > > Index: linux-2.6/drivers/base/core.c
> > > ===================================================================
> > > --- linux-2.6.orig/drivers/base/core.c
> > > +++ linux-2.6/drivers/base/core.c
> > > @@ -929,6 +929,11 @@ void device_del(struct device *dev)
> > >  	struct device *parent = dev->parent;
> > >  	struct class_interface *class_intf;
> > >  
> > > +	if (in_suspend_context()) {
> > > +		get_device(dev);
> > > +		device_pm_schedule_removal(dev);
> > > +		return;
> > > +	}
> > 
> > Why are you grabbing an additional reference to the device here?  That
> > would seem to get out of balance when the device is later scheduled for
> > removal, right?
> 
> No, IMO the reference is necessary, because unregister_dropped_devices() uses
> device_unregister() that does the put_device() eventually.
> 
> If we are called by device_unregister(), the get_device() is needed to balance
> the put_device() that will be called by device_unregister() after we return.
> 
> OTOH, if we are called directly, then we need to balance the put_device()
> that will be done by device_unregister() called from
> unregister_dropped_devices().
> 
> Thanks,
> Rafael
> -
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 



-- 
"Premature optimization is the root of all evil." - Donald Knuth