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@xxxxxxx>
> > >
> > > 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@xxxxxxx>
> > > Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
--
"Premature optimization is the root of all evil." - Donald Knuth
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
