* [PATCH] PM: Handle unregistering devices during suspend/hibernation
@ 2008-02-22 23:53 Rafael J. Wysocki
2008-02-23 0:02 ` Greg KH
0 siblings, 1 reply; 5+ messages in thread
From: Rafael J. Wysocki @ 2008-02-22 23:53 UTC (permalink / raw)
To: Greg KH; +Cc: ACPI Devel Maling List, Alan Stern, Pavel Machek, pm list
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.
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;
+ }
device_pm_remove(dev);
if (parent)
klist_del(&dev->knode_parent);
Index: linux-2.6/drivers/base/power/power.h
===================================================================
--- linux-2.6.orig/drivers/base/power/power.h
+++ linux-2.6/drivers/base/power/power.h
@@ -11,6 +11,7 @@ static inline struct device *to_device(s
return container_of(entry, struct device, power.entry);
}
+extern bool in_suspend_context(void);
extern void device_pm_add(struct device *);
extern void device_pm_remove(struct device *);
extern int pm_sleep_lock(void);
@@ -18,6 +19,10 @@ extern void pm_sleep_unlock(void);
#else /* CONFIG_PM_SLEEP */
+static inline bool in_suspend_context(void)
+{
+ return false;
+}
static inline void device_pm_add(struct device *dev)
{
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] PM: Handle unregistering devices during suspend/hibernation
2008-02-22 23:53 [PATCH] PM: Handle unregistering devices during suspend/hibernation Rafael J. Wysocki
@ 2008-02-23 0:02 ` Greg KH
2008-02-23 0:21 ` Rafael J. Wysocki
0 siblings, 1 reply; 5+ messages in thread
From: Greg KH @ 2008-02-23 0:02 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: ACPI Devel Maling List, Alan Stern, Pavel Machek, pm list
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.
>
> 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?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] PM: Handle unregistering devices during suspend/hibernation
2008-02-23 0:02 ` Greg KH
@ 2008-02-23 0:21 ` Rafael J. Wysocki
2008-02-23 1:19 ` Rafael J. Wysocki
0 siblings, 1 reply; 5+ messages in thread
From: Rafael J. Wysocki @ 2008-02-23 0:21 UTC (permalink / raw)
To: Greg KH; +Cc: ACPI Devel Maling List, Alan Stern, Pavel Machek, pm list
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.
> >
> > 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] PM: Handle unregistering devices during suspend/hibernation
2008-02-23 0:21 ` Rafael J. Wysocki
@ 2008-02-23 1:19 ` Rafael J. Wysocki
2008-02-23 3:47 ` Greg KH
0 siblings, 1 reply; 5+ messages in thread
From: Rafael J. Wysocki @ 2008-02-23 1:19 UTC (permalink / raw)
To: Greg KH; +Cc: ACPI Devel Maling List, Alan Stern, Pavel Machek, pm list
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] PM: Handle unregistering devices during suspend/hibernation
2008-02-23 1:19 ` Rafael J. Wysocki
@ 2008-02-23 3:47 ` Greg KH
0 siblings, 0 replies; 5+ messages in thread
From: Greg KH @ 2008-02-23 3:47 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: ACPI Devel Maling List, Alan Stern, Pavel Machek, pm list
On Sat, Feb 23, 2008 at 02:19:27AM +0100, Rafael J. Wysocki wrote:
> 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.
No problem at all.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2008-02-23 3:45 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-02-22 23:53 [PATCH] PM: Handle unregistering devices during suspend/hibernation Rafael J. Wysocki
2008-02-23 0:02 ` Greg KH
2008-02-23 0:21 ` Rafael J. Wysocki
2008-02-23 1:19 ` Rafael J. Wysocki
2008-02-23 3:47 ` Greg KH
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox