[RFC][PATCH] PM: Make PM core handle device registrations concurrent with suspend/hibernation

[RFC][PATCH] PM: Make PM core handle device registrations concurrent with suspend/hibernation

[Rafael J. Wysocki]

Hi,

The appended patch is intended to fix the issue with the PM core that it allows
device registrations to complete successfully even if they run concurrently
with the suspending of their parents, which may lead to a wrong ordering of
devices on the dpm_active list and, as a result, to failures during suspend and
hibernation transitions.

Comments welcome.

Thanks,
Rafael


---
From: Rafael J. Wysocki <rjw@sisk.pl>

Modify the PM core to protect its data structures, specifically the
dpm_active list, from being corrupted if a child of the currently
suspending device is registered concurrently with its ->suspend()
callback.  In that case, since the new device (the child) is added
to dpm_active after its parent, the PM core will attempt to
suspend it after the parent, which is wrong.

Introduce a new member of struct dev_pm_info, called 'sleeping',
and use it to check if the parent of the device being added to
dpm_active has been suspended, in which case the device registration
fails.  Also, use 'sleeping' for checking if the ordering of devices
on dpm_active is correct.

Remove pm_sleep_rwsem which is not necessary any more.

Special thanks to Alan Stern for discussions and suggestions that
lead to the creation of this patch.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 Documentation/power/devices.txt |    4 +++
 drivers/base/core.c             |    6 ++++-
 drivers/base/power/main.c       |   47 ++++++++++++----------------------------
 drivers/base/power/power.h      |   23 ++-----------------
 include/linux/pm.h              |    1 
 5 files changed, 28 insertions(+), 53 deletions(-)

Index: linux-2.6/include/linux/pm.h
===================================================================
--- linux-2.6.orig/include/linux/pm.h
+++ linux-2.6/include/linux/pm.h
@@ -186,6 +186,7 @@ struct dev_pm_info {
 #ifdef	CONFIG_PM_SLEEP
 	unsigned		should_wakeup:1;
 	struct list_head	entry;
+	bool			sleeping;	/* Owned by the PM core */
 #endif
 };
 
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
@@ -54,22 +54,26 @@ static LIST_HEAD(dpm_destroy);
 
 static DEFINE_MUTEX(dpm_list_mtx);
 
-static DECLARE_RWSEM(pm_sleep_rwsem);
-
 int (*platform_enable_wakeup)(struct device *dev, int is_on);
 
 /**
  *	device_pm_add - add a device to the list of active devices
  *	@dev:	Device to be added to the list
  */
-void device_pm_add(struct device *dev)
+int device_pm_add(struct device *dev)
 {
+	int error = 0;
+
 	pr_debug("PM: Adding info for %s:%s\n",
 		 dev->bus ? dev->bus->name : "No Bus",
 		 kobject_name(&dev->kobj));
 	mutex_lock(&dpm_list_mtx);
-	list_add_tail(&dev->power.entry, &dpm_active);
+	if (dev->parent && dev->parent->power.sleeping)
+		error = -EBUSY;
+	else
+		list_add_tail(&dev->power.entry, &dpm_active);
 	mutex_unlock(&dpm_list_mtx);
+	return error;
 }
 
 /**
@@ -107,32 +111,6 @@ void device_pm_schedule_removal(struct d
 }
 EXPORT_SYMBOL_GPL(device_pm_schedule_removal);
 
-/**
- *	pm_sleep_lock - mutual exclusion for registration and suspend
- *
- *	Returns 0 if no suspend is underway and device registration
- *	may proceed, otherwise -EBUSY.
- */
-int pm_sleep_lock(void)
-{
-	if (down_read_trylock(&pm_sleep_rwsem))
-		return 0;
-
-	return -EBUSY;
-}
-
-/**
- *	pm_sleep_unlock - mutual exclusion for registration and suspend
- *
- *	This routine undoes the effect of device_pm_add_lock
- *	when a device's registration is complete.
- */
-void pm_sleep_unlock(void)
-{
-	up_read(&pm_sleep_rwsem);
-}
-
-
 /*------------------------- Resume routines -------------------------*/
 
 /**
@@ -247,6 +225,7 @@ static void dpm_resume(void)
 		struct device *dev = to_device(entry);
 
 		list_move_tail(entry, &dpm_active);
+		dev->power.sleeping = false;
 		mutex_unlock(&dpm_list_mtx);
 		resume_device(dev);
 		mutex_lock(&dpm_list_mtx);
@@ -285,7 +264,6 @@ void device_resume(void)
 	might_sleep();
 	dpm_resume();
 	unregister_dropped_devices();
-	up_write(&pm_sleep_rwsem);
 }
 EXPORT_SYMBOL_GPL(device_resume);
 
@@ -426,6 +404,11 @@ static int dpm_suspend(pm_message_t stat
 		struct list_head *entry = dpm_active.prev;
 		struct device *dev = to_device(entry);
 
+		if (dev->parent && dev->parent->power.sleeping) {
+			error = -EAGAIN;
+			break;
+		}
+		dev->power.sleeping = true;;
 		mutex_unlock(&dpm_list_mtx);
 		error = suspend_device(dev, state);
 		mutex_lock(&dpm_list_mtx);
@@ -437,6 +420,7 @@ static int dpm_suspend(pm_message_t stat
 					(error == -EAGAIN ?
 					" (please convert to suspend_late)" :
 					""));
+			dev->power.sleeping = false;
 			break;
 		}
 		if (!list_empty(&dev->power.entry))
@@ -459,7 +443,6 @@ int device_suspend(pm_message_t state)
 	int error;
 
 	might_sleep();
-	down_write(&pm_sleep_rwsem);
 	error = dpm_suspend(state);
 	if (error)
 		device_resume();
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,30 +11,13 @@ static inline struct device *to_device(s
 	return container_of(entry, struct device, power.entry);
 }
 
-extern void device_pm_add(struct device *);
+extern int device_pm_add(struct device *);
 extern void device_pm_remove(struct device *);
-extern int pm_sleep_lock(void);
-extern void pm_sleep_unlock(void);
 
 #else /* CONFIG_PM_SLEEP */
 
-
-static inline void device_pm_add(struct device *dev)
-{
-}
-
-static inline void device_pm_remove(struct device *dev)
-{
-}
-
-static inline int pm_sleep_lock(void)
-{
-	return 0;
-}
-
-static inline void pm_sleep_unlock(void)
-{
-}
+static inline int device_pm_add(struct device *dev) { return 0; }
+static inline void device_pm_remove(struct device *dev) {}
 
 #endif
 
Index: linux-2.6/drivers/base/core.c
===================================================================
--- linux-2.6.orig/drivers/base/core.c
+++ linux-2.6/drivers/base/core.c
@@ -814,7 +814,11 @@ int device_add(struct device *dev)
 	error = dpm_sysfs_add(dev);
 	if (error)
 		goto PMError;
-	device_pm_add(dev);
+	error = device_pm_add(dev);
+	if (error) {
+		dpm_sysfs_remove(dev);
+		goto PMError;
+	}
 	error = bus_add_device(dev);
 	if (error)
 		goto BusError;
Index: linux-2.6/Documentation/power/devices.txt
===================================================================
--- linux-2.6.orig/Documentation/power/devices.txt
+++ linux-2.6/Documentation/power/devices.txt
@@ -196,6 +196,10 @@ its parent; and can't be removed or susp
 
 The policy is that the device tree should match hardware bus topology.
 (Or at least the control bus, for devices which use multiple busses.)
+In particular, this means that a device registration may fail if the parent of
+the device is suspending (ie. has just been chosen by the PM core as the next
+device to suspend) or has already suspended.  Device drivers must be prepared
+to cope with such situations.
 
 
 Suspending Devices

Re: [RFC][PATCH] PM: Make PM core handle device registrations concurrent with suspend/hibernation

[Alan Stern]

On Tue, 4 Mar 2008, Rafael J. Wysocki wrote:

> Hi,
> 
> The appended patch is intended to fix the issue with the PM core that it allows
> device registrations to complete successfully even if they run concurrently
> with the suspending of their parents, which may lead to a wrong ordering of
> devices on the dpm_active list and, as a result, to failures during suspend and
> hibernation transitions.
> 
> Comments welcome.

> Index: linux-2.6/include/linux/pm.h
> ===================================================================
> --- linux-2.6.orig/include/linux/pm.h
> +++ linux-2.6/include/linux/pm.h
> @@ -186,6 +186,7 @@ struct dev_pm_info {
>  #ifdef	CONFIG_PM_SLEEP
>  	unsigned		should_wakeup:1;
>  	struct list_head	entry;
> +	bool			sleeping;	/* Owned by the PM core */
>  #endif
>  };

Drivers might want to use this field without having to add protective 
"#ifdef CONFIG_PM_SLEEP" lines.  You can change it to a single-bit 
bitfield and place it adjacent to can_wakeup.

> -void device_pm_add(struct device *dev)
> +int device_pm_add(struct device *dev)
>  {
> +	int error = 0;
> +
>  	pr_debug("PM: Adding info for %s:%s\n",
>  		 dev->bus ? dev->bus->name : "No Bus",
>  		 kobject_name(&dev->kobj));
>  	mutex_lock(&dpm_list_mtx);
> -	list_add_tail(&dev->power.entry, &dpm_active);
> +	if (dev->parent && dev->parent->power.sleeping)
> +		error = -EBUSY;

Add a stack dump?  When this isn't a race, it's the kind of bug we want
to fix.

> +	else
> +		list_add_tail(&dev->power.entry, &dpm_active);
>  	mutex_unlock(&dpm_list_mtx);
> +	return error;
>  }

> @@ -426,6 +404,11 @@ static int dpm_suspend(pm_message_t stat
>  		struct list_head *entry = dpm_active.prev;
>  		struct device *dev = to_device(entry);
>  
> +		if (dev->parent && dev->parent->power.sleeping) {
> +			error = -EAGAIN;
> +			break;
> +		}

It's not clear that we want to have this check.  It would cause
problems if the device ordering got mixed up by device_move(), which is
pretty much the only way it could be triggered.

If you do want to leave it in, add a stack dump (and perhaps make it 
not return an error).  This would help force people to figure out safe 
ways to use device_move().

> +		dev->power.sleeping = true;;

Extra ';'.

Alan Stern

Re: [RFC][PATCH] PM: Make PM core handle device registrations concurrent with suspend/hibernation

[Rafael J. Wysocki]

On Tuesday, 4 of March 2008, Alan Stern wrote:
> On Tue, 4 Mar 2008, Rafael J. Wysocki wrote:
> 
> > Hi,
> > 
> > The appended patch is intended to fix the issue with the PM core that it allows
> > device registrations to complete successfully even if they run concurrently
> > with the suspending of their parents, which may lead to a wrong ordering of
> > devices on the dpm_active list and, as a result, to failures during suspend and
> > hibernation transitions.
> > 
> > Comments welcome.
> 
> > Index: linux-2.6/include/linux/pm.h
> > ===================================================================
> > --- linux-2.6.orig/include/linux/pm.h
> > +++ linux-2.6/include/linux/pm.h
> > @@ -186,6 +186,7 @@ struct dev_pm_info {
> >  #ifdef	CONFIG_PM_SLEEP
> >  	unsigned		should_wakeup:1;
> >  	struct list_head	entry;
> > +	bool			sleeping;	/* Owned by the PM core */
> >  #endif
> >  };
> 
> Drivers might want to use this field without having to add protective 
> "#ifdef CONFIG_PM_SLEEP" lines.

I guess you mean that driver writers may not want to make each piece of code
referring to this flag depend on CONFIG_PM_SLEEP.  Any driver that actually
writes to it will be buggy ...

> You can change it to a single-bit bitfield and place it adjacent to
> can_wakeup. 

Yeah, good catch.  I will.

> > -void device_pm_add(struct device *dev)
> > +int device_pm_add(struct device *dev)
> >  {
> > +	int error = 0;
> > +
> >  	pr_debug("PM: Adding info for %s:%s\n",
> >  		 dev->bus ? dev->bus->name : "No Bus",
> >  		 kobject_name(&dev->kobj));
> >  	mutex_lock(&dpm_list_mtx);
> > -	list_add_tail(&dev->power.entry, &dpm_active);
> > +	if (dev->parent && dev->parent->power.sleeping)
> > +		error = -EBUSY;
> 
> Add a stack dump?  When this isn't a race, it's the kind of bug we want
> to fix.

Yes, I'll do that.

> > +	else
> > +		list_add_tail(&dev->power.entry, &dpm_active);
> >  	mutex_unlock(&dpm_list_mtx);
> > +	return error;
> >  }
> 
> > @@ -426,6 +404,11 @@ static int dpm_suspend(pm_message_t stat
> >  		struct list_head *entry = dpm_active.prev;
> >  		struct device *dev = to_device(entry);
> >  
> > +		if (dev->parent && dev->parent->power.sleeping) {
> > +			error = -EAGAIN;
> > +			break;
> > +		}
> 
> It's not clear that we want to have this check.  It would cause
> problems if the device ordering got mixed up by device_move(), which is
> pretty much the only way it could be triggered.

Well, isn't it actually dangerous to suspend parents before their children?
What happens, for example, if putting the parent into D3 causes power to be
cut from the children and then we attempt to suspend them?

> If you do want to leave it in, add a stack dump (and perhaps make it 
> not return an error).  This would help force people to figure out safe 
> ways to use device_move().

Yes, I'll add a stack dump here.

In fact, I'm going to add WARN_ON(true) in both places.

> > +		dev->power.sleeping = true;;
> 
> Extra ';'.

Hah, thanks, will remove.

Rafael

Re: [RFC][PATCH] PM: Make PM core handle device registrations concurrent with suspend/hibernation

[Rafael J. Wysocki]

On Tuesday, 4 of March 2008, Alan Stern wrote:
> On Tue, 4 Mar 2008, Rafael J. Wysocki wrote:
> 
> > Hi,
> > 
> > The appended patch is intended to fix the issue with the PM core that it allows
> > device registrations to complete successfully even if they run concurrently
> > with the suspending of their parents, which may lead to a wrong ordering of
> > devices on the dpm_active list and, as a result, to failures during suspend and
> > hibernation transitions.
> > 
> > Comments welcome.
> 
> > Index: linux-2.6/include/linux/pm.h
> > ===================================================================
> > --- linux-2.6.orig/include/linux/pm.h
> > +++ linux-2.6/include/linux/pm.h
> > @@ -186,6 +186,7 @@ struct dev_pm_info {
> >  #ifdef	CONFIG_PM_SLEEP
> >  	unsigned		should_wakeup:1;
> >  	struct list_head	entry;
> > +	bool			sleeping;	/* Owned by the PM core */
> >  #endif
> >  };
> 
> Drivers might want to use this field without having to add protective 
> "#ifdef CONFIG_PM_SLEEP" lines.  You can change it to a single-bit 
> bitfield and place it adjacent to can_wakeup.

Below is a refined version that should address all of your comments.  Please
have a look.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>

Modify the PM core to protect its data structures, specifically the
dpm_active list, from being corrupted if a child of the currently
suspending device is registered concurrently with its ->suspend()
callback.  In that case, since the new device (the child) is added
to dpm_active after its parent, the PM core will attempt to
suspend it after the parent, which is wrong.

Introduce a new member of struct dev_pm_info, called 'sleeping',
and use it to check if the parent of the device being added to
dpm_active has been suspended, in which case the device registration
fails.  Also, use 'sleeping' for checking if the ordering of devices
on dpm_active is correct.

Remove pm_sleep_rwsem which is not necessary any more.

Special thanks to Alan Stern for discussions and suggestions that
lead to the creation of this patch.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 Documentation/power/devices.txt |    4 +++
 drivers/base/core.c             |    6 ++++
 drivers/base/power/main.c       |   50 ++++++++++++++--------------------------
 drivers/base/power/power.h      |   23 ++----------------
 include/linux/pm.h              |    1 
 5 files changed, 31 insertions(+), 53 deletions(-)

Index: linux-2.6/include/linux/pm.h
===================================================================
--- linux-2.6.orig/include/linux/pm.h
+++ linux-2.6/include/linux/pm.h
@@ -183,6 +183,7 @@ typedef struct pm_message {
 struct dev_pm_info {
 	pm_message_t		power_state;
 	unsigned		can_wakeup:1;
+	bool			sleeping:1;	/* Owned by the PM core */
 #ifdef	CONFIG_PM_SLEEP
 	unsigned		should_wakeup:1;
 	struct list_head	entry;
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
@@ -54,22 +54,28 @@ static LIST_HEAD(dpm_destroy);
 
 static DEFINE_MUTEX(dpm_list_mtx);
 
-static DECLARE_RWSEM(pm_sleep_rwsem);
-
 int (*platform_enable_wakeup)(struct device *dev, int is_on);
 
 /**
  *	device_pm_add - add a device to the list of active devices
  *	@dev:	Device to be added to the list
  */
-void device_pm_add(struct device *dev)
+int device_pm_add(struct device *dev)
 {
+	int error = 0;
+
 	pr_debug("PM: Adding info for %s:%s\n",
 		 dev->bus ? dev->bus->name : "No Bus",
 		 kobject_name(&dev->kobj));
 	mutex_lock(&dpm_list_mtx);
-	list_add_tail(&dev->power.entry, &dpm_active);
+	if (dev->parent && dev->parent->power.sleeping) {
+		WARN_ON(true);
+		error = -EBUSY;
+	} else {
+		list_add_tail(&dev->power.entry, &dpm_active);
+	}
 	mutex_unlock(&dpm_list_mtx);
+	return error;
 }
 
 /**
@@ -107,32 +113,6 @@ void device_pm_schedule_removal(struct d
 }
 EXPORT_SYMBOL_GPL(device_pm_schedule_removal);
 
-/**
- *	pm_sleep_lock - mutual exclusion for registration and suspend
- *
- *	Returns 0 if no suspend is underway and device registration
- *	may proceed, otherwise -EBUSY.
- */
-int pm_sleep_lock(void)
-{
-	if (down_read_trylock(&pm_sleep_rwsem))
-		return 0;
-
-	return -EBUSY;
-}
-
-/**
- *	pm_sleep_unlock - mutual exclusion for registration and suspend
- *
- *	This routine undoes the effect of device_pm_add_lock
- *	when a device's registration is complete.
- */
-void pm_sleep_unlock(void)
-{
-	up_read(&pm_sleep_rwsem);
-}
-
-
 /*------------------------- Resume routines -------------------------*/
 
 /**
@@ -247,6 +227,7 @@ static void dpm_resume(void)
 		struct device *dev = to_device(entry);
 
 		list_move_tail(entry, &dpm_active);
+		dev->power.sleeping = false;
 		mutex_unlock(&dpm_list_mtx);
 		resume_device(dev);
 		mutex_lock(&dpm_list_mtx);
@@ -285,7 +266,6 @@ void device_resume(void)
 	might_sleep();
 	dpm_resume();
 	unregister_dropped_devices();
-	up_write(&pm_sleep_rwsem);
 }
 EXPORT_SYMBOL_GPL(device_resume);
 
@@ -426,6 +406,12 @@ static int dpm_suspend(pm_message_t stat
 		struct list_head *entry = dpm_active.prev;
 		struct device *dev = to_device(entry);
 
+		if (dev->parent && dev->parent->power.sleeping) {
+			WARN_ON(true);
+			error = -EAGAIN;
+			break;
+		}
+		dev->power.sleeping = true;
 		mutex_unlock(&dpm_list_mtx);
 		error = suspend_device(dev, state);
 		mutex_lock(&dpm_list_mtx);
@@ -437,6 +423,7 @@ static int dpm_suspend(pm_message_t stat
 					(error == -EAGAIN ?
 					" (please convert to suspend_late)" :
 					""));
+			dev->power.sleeping = false;
 			break;
 		}
 		if (!list_empty(&dev->power.entry))
@@ -459,7 +446,6 @@ int device_suspend(pm_message_t state)
 	int error;
 
 	might_sleep();
-	down_write(&pm_sleep_rwsem);
 	error = dpm_suspend(state);
 	if (error)
 		device_resume();
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,30 +11,13 @@ static inline struct device *to_device(s
 	return container_of(entry, struct device, power.entry);
 }
 
-extern void device_pm_add(struct device *);
+extern int device_pm_add(struct device *);
 extern void device_pm_remove(struct device *);
-extern int pm_sleep_lock(void);
-extern void pm_sleep_unlock(void);
 
 #else /* CONFIG_PM_SLEEP */
 
-
-static inline void device_pm_add(struct device *dev)
-{
-}
-
-static inline void device_pm_remove(struct device *dev)
-{
-}
-
-static inline int pm_sleep_lock(void)
-{
-	return 0;
-}
-
-static inline void pm_sleep_unlock(void)
-{
-}
+static inline int device_pm_add(struct device *dev) { return 0; }
+static inline void device_pm_remove(struct device *dev) {}
 
 #endif
 
Index: linux-2.6/drivers/base/core.c
===================================================================
--- linux-2.6.orig/drivers/base/core.c
+++ linux-2.6/drivers/base/core.c
@@ -814,7 +814,11 @@ int device_add(struct device *dev)
 	error = dpm_sysfs_add(dev);
 	if (error)
 		goto PMError;
-	device_pm_add(dev);
+	error = device_pm_add(dev);
+	if (error) {
+		dpm_sysfs_remove(dev);
+		goto PMError;
+	}
 	error = bus_add_device(dev);
 	if (error)
 		goto BusError;
Index: linux-2.6/Documentation/power/devices.txt
===================================================================
--- linux-2.6.orig/Documentation/power/devices.txt
+++ linux-2.6/Documentation/power/devices.txt
@@ -196,6 +196,10 @@ its parent; and can't be removed or susp
 
 The policy is that the device tree should match hardware bus topology.
 (Or at least the control bus, for devices which use multiple busses.)
+In particular, this means that a device registration may fail if the parent of
+the device is suspending (ie. has been chosen by the PM core as the next
+device to suspend) or has already suspended.  Device drivers must be prepared
+to cope with such situations.
 
 
 Suspending Devices

Re: [RFC][PATCH] PM: Make PM core handle device registrations concurrent with suspend/hibernation

[Alan Stern]

On Tue, 4 Mar 2008, Rafael J. Wysocki wrote:

> > It's not clear that we want to have this check.  It would cause
> > problems if the device ordering got mixed up by device_move(), which is
> > pretty much the only way it could be triggered.
> 
> Well, isn't it actually dangerous to suspend parents before their children?
> What happens, for example, if putting the parent into D3 causes power to be
> cut from the children and then we attempt to suspend them?

It depends on the devices involved.  For PCI devices, obviously there 
will be problems of the sort you described.  But for other devices 
there might not be.

For example, the USB Bluetooth driver will sometimes create a new TTY
device and then move the existing Bluetooth device underneath it (this
description is oversimplified and probably wrong in some important
respects, but you get the idea).  Suspending the Bluetooth device
before suspending the TTY won't cause any power-related problems,
because a TTY is a purely logical device with no power implications at
all.

There might still be logical problems, of course...  We need to add a
mechanism for reordering the dpm_active list when device_move() is
called.  But first we need to get in touch with the people using
device_move() -- basically just Marcel and Cornelia -- and see what
sort of mechanism they will need.

Alan Stern

Re: [RFC][PATCH] PM: Make PM core handle device registrations concurrent with suspend/hibernation

[Cornelia Huck]

On Wed, 5 Mar 2008 11:03:52 -0500 (EST),
Alan Stern <stern@rowland.harvard.edu> wrote:

> There might still be logical problems, of course...  We need to add a
> mechanism for reordering the dpm_active list when device_move() is
> called.  But first we need to get in touch with the people using
> device_move() -- basically just Marcel and Cornelia -- and see what
> sort of mechanism they will need.

On s390 we currently don't do anything suspend related at all - I'd
need to think about that. (I use device_move() to reflect topology
changes on the (virtual) hardware, so I'd guess the "don't suspend
parent before child rule" should still apply afterwards. But of course,
that also depends on what we actually need to do on the parent's bus and
what on the child's bus.)

Re: [RFC][PATCH] PM: Make PM core handle device registrations concurrent with suspend/hibernation

[Alan Stern]

On Wed, 5 Mar 2008, Rafael J. Wysocki wrote:

> -void device_pm_add(struct device *dev)
> +int device_pm_add(struct device *dev)
>  {
> +	int error = 0;
> +
>  	pr_debug("PM: Adding info for %s:%s\n",
>  		 dev->bus ? dev->bus->name : "No Bus",
>  		 kobject_name(&dev->kobj));
>  	mutex_lock(&dpm_list_mtx);
> -	list_add_tail(&dev->power.entry, &dpm_active);
> +	if (dev->parent && dev->parent->power.sleeping) {
> +		WARN_ON(true);

I would prefer to put a dev_warn() line here, so that people reading
the kernel log can easily tell which device caused the problem and what
sort of problem it is.  Something like this:

		dev_warn(dev, "device added while parent %s is asleep\n",
				dev->parent->bus_id);
		WARN_ON(true);

> @@ -426,6 +406,12 @@ static int dpm_suspend(pm_message_t stat
>  		struct list_head *entry = dpm_active.prev;
>  		struct device *dev = to_device(entry);
>  
> +		if (dev->parent && dev->parent->power.sleeping) {
> +			WARN_ON(true);
> +			error = -EAGAIN;
> +			break;

Again, a dev_warn() would be appropriate.

And you might consider taking out the "error = -EAGAIN" and the
"break".  When this occurs it doesn't mean that devices were suspended
in the wrong order; it means that the ordering of the parent pointers
fails to match the ordering of dpm_active.  The only way for this to
happen is if the parent pointers are messed up by device_move() --
dpm_active will still be correct.

The rest of the patch looks fine.

Alan Stern

Re: [RFC][PATCH] PM: Make PM core handle device registrations concurrent with suspend/hibernation

[Rafael J. Wysocki]

On Wednesday, 5 of March 2008, Alan Stern wrote:
> On Wed, 5 Mar 2008, Rafael J. Wysocki wrote:
> 
> > -void device_pm_add(struct device *dev)
> > +int device_pm_add(struct device *dev)
> >  {
> > +	int error = 0;
> > +
> >  	pr_debug("PM: Adding info for %s:%s\n",
> >  		 dev->bus ? dev->bus->name : "No Bus",
> >  		 kobject_name(&dev->kobj));
> >  	mutex_lock(&dpm_list_mtx);
> > -	list_add_tail(&dev->power.entry, &dpm_active);
> > +	if (dev->parent && dev->parent->power.sleeping) {
> > +		WARN_ON(true);
> 
> I would prefer to put a dev_warn() line here, so that people reading
> the kernel log can easily tell which device caused the problem and what
> sort of problem it is.  Something like this:
> 
> 		dev_warn(dev, "device added while parent %s is asleep\n",
> 				dev->parent->bus_id);
> 		WARN_ON(true);

Added.

> > @@ -426,6 +406,12 @@ static int dpm_suspend(pm_message_t stat
> >  		struct list_head *entry = dpm_active.prev;
> >  		struct device *dev = to_device(entry);
> >  
> > +		if (dev->parent && dev->parent->power.sleeping) {
> > +			WARN_ON(true);
> > +			error = -EAGAIN;
> > +			break;
> 
> Again, a dev_warn() would be appropriate.

I chose just "WARN_ON(dev->parent && dev->parent->power.sleeping)",
which is shorter. :-)

I don't really expect it to appear and if it's reported, it'll be easy to
figure out everything from the stack trace.

Revised patch below.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>

Modify the PM core to protect its data structures, specifically the
dpm_active list, from being corrupted if a child of the currently
suspending device is registered concurrently with its ->suspend()
callback.  In that case, since the new device (the child) is added
to dpm_active after its parent, the PM core will attempt to
suspend it after the parent, which is wrong.

Introduce a new member of struct dev_pm_info, called 'sleeping',
and use it to check if the parent of the device being added to
dpm_active has been suspended, in which case the device registration
fails.  Also, use 'sleeping' for checking if the ordering of devices
on dpm_active is correct.

Remove pm_sleep_rwsem which is not necessary any more.

Special thanks to Alan Stern for discussions and suggestions that
lead to the creation of this patch.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 Documentation/power/devices.txt |    4 +++
 drivers/base/core.c             |    6 ++++
 drivers/base/power/main.c       |   50 ++++++++++++++--------------------------
 drivers/base/power/power.h      |   23 ++----------------
 include/linux/pm.h              |    1 
 5 files changed, 31 insertions(+), 53 deletions(-)

Index: linux-2.6/include/linux/pm.h
===================================================================
--- linux-2.6.orig/include/linux/pm.h
+++ linux-2.6/include/linux/pm.h
@@ -183,6 +183,7 @@ typedef struct pm_message {
 struct dev_pm_info {
 	pm_message_t		power_state;
 	unsigned		can_wakeup:1;
+	bool			sleeping:1;	/* Owned by the PM core */
 #ifdef	CONFIG_PM_SLEEP
 	unsigned		should_wakeup:1;
 	struct list_head	entry;
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
@@ -54,22 +54,31 @@ static LIST_HEAD(dpm_destroy);
 
 static DEFINE_MUTEX(dpm_list_mtx);
 
-static DECLARE_RWSEM(pm_sleep_rwsem);
-
 int (*platform_enable_wakeup)(struct device *dev, int is_on);
 
 /**
  *	device_pm_add - add a device to the list of active devices
  *	@dev:	Device to be added to the list
  */
-void device_pm_add(struct device *dev)
+int device_pm_add(struct device *dev)
 {
+	int error = 0;
+
 	pr_debug("PM: Adding info for %s:%s\n",
 		 dev->bus ? dev->bus->name : "No Bus",
 		 kobject_name(&dev->kobj));
 	mutex_lock(&dpm_list_mtx);
-	list_add_tail(&dev->power.entry, &dpm_active);
+	if (dev->parent && dev->parent->power.sleeping) {
+		dev_warn(dev,
+			"refusing to add device while parent %s is asleep\n",
+			dev->parent->bus_id);
+		WARN_ON(true);
+		error = -EBUSY;
+	} else {
+		list_add_tail(&dev->power.entry, &dpm_active);
+	}
 	mutex_unlock(&dpm_list_mtx);
+	return error;
 }
 
 /**
@@ -107,32 +116,6 @@ void device_pm_schedule_removal(struct d
 }
 EXPORT_SYMBOL_GPL(device_pm_schedule_removal);
 
-/**
- *	pm_sleep_lock - mutual exclusion for registration and suspend
- *
- *	Returns 0 if no suspend is underway and device registration
- *	may proceed, otherwise -EBUSY.
- */
-int pm_sleep_lock(void)
-{
-	if (down_read_trylock(&pm_sleep_rwsem))
-		return 0;
-
-	return -EBUSY;
-}
-
-/**
- *	pm_sleep_unlock - mutual exclusion for registration and suspend
- *
- *	This routine undoes the effect of device_pm_add_lock
- *	when a device's registration is complete.
- */
-void pm_sleep_unlock(void)
-{
-	up_read(&pm_sleep_rwsem);
-}
-
-
 /*------------------------- Resume routines -------------------------*/
 
 /**
@@ -247,6 +230,7 @@ static void dpm_resume(void)
 		struct device *dev = to_device(entry);
 
 		list_move_tail(entry, &dpm_active);
+		dev->power.sleeping = false;
 		mutex_unlock(&dpm_list_mtx);
 		resume_device(dev);
 		mutex_lock(&dpm_list_mtx);
@@ -285,7 +269,6 @@ void device_resume(void)
 	might_sleep();
 	dpm_resume();
 	unregister_dropped_devices();
-	up_write(&pm_sleep_rwsem);
 }
 EXPORT_SYMBOL_GPL(device_resume);
 
@@ -426,6 +409,9 @@ static int dpm_suspend(pm_message_t stat
 		struct list_head *entry = dpm_active.prev;
 		struct device *dev = to_device(entry);
 
+		WARN_ON(dev->parent && dev->parent->power.sleeping);
+
+		dev->power.sleeping = true;
 		mutex_unlock(&dpm_list_mtx);
 		error = suspend_device(dev, state);
 		mutex_lock(&dpm_list_mtx);
@@ -437,6 +423,7 @@ static int dpm_suspend(pm_message_t stat
 					(error == -EAGAIN ?
 					" (please convert to suspend_late)" :
 					""));
+			dev->power.sleeping = false;
 			break;
 		}
 		if (!list_empty(&dev->power.entry))
@@ -459,7 +446,6 @@ int device_suspend(pm_message_t state)
 	int error;
 
 	might_sleep();
-	down_write(&pm_sleep_rwsem);
 	error = dpm_suspend(state);
 	if (error)
 		device_resume();
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,30 +11,13 @@ static inline struct device *to_device(s
 	return container_of(entry, struct device, power.entry);
 }
 
-extern void device_pm_add(struct device *);
+extern int device_pm_add(struct device *);
 extern void device_pm_remove(struct device *);
-extern int pm_sleep_lock(void);
-extern void pm_sleep_unlock(void);
 
 #else /* CONFIG_PM_SLEEP */
 
-
-static inline void device_pm_add(struct device *dev)
-{
-}
-
-static inline void device_pm_remove(struct device *dev)
-{
-}
-
-static inline int pm_sleep_lock(void)
-{
-	return 0;
-}
-
-static inline void pm_sleep_unlock(void)
-{
-}
+static inline int device_pm_add(struct device *dev) { return 0; }
+static inline void device_pm_remove(struct device *dev) {}
 
 #endif
 
Index: linux-2.6/drivers/base/core.c
===================================================================
--- linux-2.6.orig/drivers/base/core.c
+++ linux-2.6/drivers/base/core.c
@@ -815,7 +815,11 @@ int device_add(struct device *dev)
 	error = dpm_sysfs_add(dev);
 	if (error)
 		goto PMError;
-	device_pm_add(dev);
+	error = device_pm_add(dev);
+	if (error) {
+		dpm_sysfs_remove(dev);
+		goto PMError;
+	}
 	error = bus_add_device(dev);
 	if (error)
 		goto BusError;
Index: linux-2.6/Documentation/power/devices.txt
===================================================================
--- linux-2.6.orig/Documentation/power/devices.txt
+++ linux-2.6/Documentation/power/devices.txt
@@ -196,6 +196,10 @@ its parent; and can't be removed or susp
 
 The policy is that the device tree should match hardware bus topology.
 (Or at least the control bus, for devices which use multiple busses.)
+In particular, this means that a device registration may fail if the parent of
+the device is suspending (ie. has been chosen by the PM core as the next
+device to suspend) or has already suspended.  Device drivers must be prepared
+to cope with such situations.
 
 
 Suspending Devices

Re: [RFC][PATCH] PM: Make PM core handle device registrations concurrent with suspend/hibernation

[Alan Stern]

On Wed, 5 Mar 2008, Rafael J. Wysocki wrote:

> On Wednesday, 5 of March 2008, Alan Stern wrote:
> > On Wed, 5 Mar 2008, Rafael J. Wysocki wrote:
> > 
> > > -void device_pm_add(struct device *dev)
> > > +int device_pm_add(struct device *dev)
> > >  {
> > > +	int error = 0;
> > > +
> > >  	pr_debug("PM: Adding info for %s:%s\n",
> > >  		 dev->bus ? dev->bus->name : "No Bus",
> > >  		 kobject_name(&dev->kobj));
> > >  	mutex_lock(&dpm_list_mtx);
> > > -	list_add_tail(&dev->power.entry, &dpm_active);
> > > +	if (dev->parent && dev->parent->power.sleeping) {
> > > +		WARN_ON(true);
> > 
> > I would prefer to put a dev_warn() line here, so that people reading
> > the kernel log can easily tell which device caused the problem and what
> > sort of problem it is.  Something like this:
> > 
> > 		dev_warn(dev, "device added while parent %s is asleep\n",
> > 				dev->parent->bus_id);
> > 		WARN_ON(true);
> 
> Added.
> 
> > > @@ -426,6 +406,12 @@ static int dpm_suspend(pm_message_t stat
> > >  		struct list_head *entry = dpm_active.prev;
> > >  		struct device *dev = to_device(entry);
> > >  
> > > +		if (dev->parent && dev->parent->power.sleeping) {
> > > +			WARN_ON(true);
> > > +			error = -EAGAIN;
> > > +			break;
> > 
> > Again, a dev_warn() would be appropriate.
> 
> I chose just "WARN_ON(dev->parent && dev->parent->power.sleeping)",
> which is shorter. :-)
> 
> I don't really expect it to appear and if it's reported, it'll be easy to
> figure out everything from the stack trace.

Okay.

> Revised patch below.

It looks good.  Let's hope it doesn't mess up ACPI too badly...  :-)

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern

Re: [RFC][PATCH] PM: Make PM core handle device registrations concurrent with suspend/hibernation

[Rafael J. Wysocki]

On Wednesday, 5 of March 2008, Alan Stern wrote:
> On Wed, 5 Mar 2008, Rafael J. Wysocki wrote:
> 
> > On Wednesday, 5 of March 2008, Alan Stern wrote:
> > > On Wed, 5 Mar 2008, Rafael J. Wysocki wrote:
> > > 
> > > > -void device_pm_add(struct device *dev)
> > > > +int device_pm_add(struct device *dev)
> > > >  {
> > > > +	int error = 0;
> > > > +
> > > >  	pr_debug("PM: Adding info for %s:%s\n",
> > > >  		 dev->bus ? dev->bus->name : "No Bus",
> > > >  		 kobject_name(&dev->kobj));
> > > >  	mutex_lock(&dpm_list_mtx);
> > > > -	list_add_tail(&dev->power.entry, &dpm_active);
> > > > +	if (dev->parent && dev->parent->power.sleeping) {
> > > > +		WARN_ON(true);
> > > 
> > > I would prefer to put a dev_warn() line here, so that people reading
> > > the kernel log can easily tell which device caused the problem and what
> > > sort of problem it is.  Something like this:
> > > 
> > > 		dev_warn(dev, "device added while parent %s is asleep\n",
> > > 				dev->parent->bus_id);
> > > 		WARN_ON(true);
> > 
> > Added.
> > 
> > > > @@ -426,6 +406,12 @@ static int dpm_suspend(pm_message_t stat
> > > >  		struct list_head *entry = dpm_active.prev;
> > > >  		struct device *dev = to_device(entry);
> > > >  
> > > > +		if (dev->parent && dev->parent->power.sleeping) {
> > > > +			WARN_ON(true);
> > > > +			error = -EAGAIN;
> > > > +			break;
> > > 
> > > Again, a dev_warn() would be appropriate.
> > 
> > I chose just "WARN_ON(dev->parent && dev->parent->power.sleeping)",
> > which is shorter. :-)
> > 
> > I don't really expect it to appear and if it's reported, it'll be easy to
> > figure out everything from the stack trace.
> 
> Okay.
> 
> > Revised patch below.
> 
> It looks good.  Let's hope it doesn't mess up ACPI too badly...  :-)
> 
> Acked-by: Alan Stern <stern@rowland.harvard.edu>

Thanks!

Okay, I also have the appended patch on top of this one. :-)

Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>

Include dpm_sysfs_add() into device_pm_add(), in analogy with
device_pm_remove(), and modify device_add() to call the latter after
bus_add_device(), to avoid situations in which the PM core may
attempt to suspend a device the registration of which has not been
successful.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/core.c       |   15 +++++----------
 drivers/base/power/main.c |    4 +++-
 2 files changed, 8 insertions(+), 11 deletions(-)

Index: linux-2.6/drivers/base/core.c
===================================================================
--- linux-2.6.orig/drivers/base/core.c
+++ linux-2.6/drivers/base/core.c
@@ -812,17 +812,12 @@ int device_add(struct device *dev)
 	error = device_add_attrs(dev);
 	if (error)
 		goto AttrsError;
-	error = dpm_sysfs_add(dev);
-	if (error)
-		goto PMError;
-	error = device_pm_add(dev);
-	if (error) {
-		dpm_sysfs_remove(dev);
-		goto PMError;
-	}
 	error = bus_add_device(dev);
 	if (error)
 		goto BusError;
+	error = device_pm_add(dev);
+	if (error)
+		goto PMError;
 	kobject_uevent(&dev->kobj, KOBJ_ADD);
 	bus_attach_device(dev);
 	if (parent)
@@ -842,9 +837,9 @@ int device_add(struct device *dev)
  Done:
 	put_device(dev);
 	return error;
- BusError:
-	device_pm_remove(dev);
  PMError:
+	bus_remove_device(dev);
+ BusError:
 	if (dev->bus)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 					     BUS_NOTIFY_DEL_DEVICE, dev);
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
@@ -75,7 +75,9 @@ int device_pm_add(struct device *dev)
 		WARN_ON(true);
 		error = -EBUSY;
 	} else {
-		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;

Re: [RFC][PATCH] PM: Make PM core handle device registrations concurrent with suspend/hibernation

[Alan Stern]

On Thu, 6 Mar 2008, Rafael J. Wysocki wrote:

> > > Revised patch below.
> > 
> > It looks good.  Let's hope it doesn't mess up ACPI too badly...  :-)
> > 
> > Acked-by: Alan Stern <stern@rowland.harvard.edu>
> 
> Thanks!

I thought of one more thing you might want to add: device_pm_add() 
doesn't handle the case where dev->parent is NULL.  You could put in 
that static "all_devices_sleeping" flag, which gets set at the end of 
dpm_suspend() and cleared at the start of dpm_resume().

> Okay, I also have the appended patch on top of this one. :-)
> 
> Rafael
> 
> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Include dpm_sysfs_add() into device_pm_add(), in analogy with
> device_pm_remove(), and modify device_add() to call the latter after
> bus_add_device(), to avoid situations in which the PM core may
> attempt to suspend a device the registration of which has not been
> successful.

Clean and simple.  Ack.

Alan Stern

Re: [RFC][PATCH] PM: Make PM core handle device registrations concurrent with suspend/hibernation

[Rafael J. Wysocki]

On Thursday, 6 of March 2008, Alan Stern wrote:
> On Thu, 6 Mar 2008, Rafael J. Wysocki wrote:
> 
> > > > Revised patch below.
> > > 
> > > It looks good.  Let's hope it doesn't mess up ACPI too badly...  :-)
> > > 
> > > Acked-by: Alan Stern <stern@rowland.harvard.edu>
> > 
> > Thanks!
> 
> I thought of one more thing you might want to add: device_pm_add() 
> doesn't handle the case where dev->parent is NULL.

I'm not sure what you mean.

If dev->parent is NULL, we get into the "successful" branch where the device is
added to dpm_active.  Do you think we should add any extra handling of this
case?

> You could put in that static "all_devices_sleeping" flag, which gets set at
> the end of dpm_suspend() and cleared at the start of dpm_resume().

Well, I don't think it's necessary.  dpm_active is empty in that case and
we can use the list_empty(&dpm_active) check instead.

Thanks,
Rafael

Re: [RFC][PATCH] PM: Make PM core handle device registrations concurrent with suspend/hibernation

[Alan Stern]

On Thu, 6 Mar 2008, Rafael J. Wysocki wrote:

> > I thought of one more thing you might want to add: device_pm_add() 
> > doesn't handle the case where dev->parent is NULL.
> 
> I'm not sure what you mean.
> 
> If dev->parent is NULL, we get into the "successful" branch where the device is
> added to dpm_active.  Do you think we should add any extra handling of this
> case?

If a device is registered after dpm_suspend() has returned, the
device won't be suspended properly before the system goes to sleep.  

If the device has a parent then you're okay, because the parent must
already be suspended and so device_pm_add() will fail.  But if the
device doesn't have a parent then device_pm_add() will succeed, which
you don't want.

> > You could put in that static "all_devices_sleeping" flag, which gets set at
> > the end of dpm_suspend() and cleared at the start of dpm_resume().
> 
> Well, I don't think it's necessary.  dpm_active is empty in that case and
> we can use the list_empty(&dpm_active) check instead.

What would happen the very first time the system registers a device 
during startup?

Alan Stern

Re: [RFC][PATCH] PM: Make PM core handle device registrations concurrent with suspend/hibernation

[Rafael J. Wysocki]

On Thursday, 6 of March 2008, Alan Stern wrote:
> On Thu, 6 Mar 2008, Rafael J. Wysocki wrote:
> 
> > > I thought of one more thing you might want to add: device_pm_add() 
> > > doesn't handle the case where dev->parent is NULL.
> > 
> > I'm not sure what you mean.
> > 
> > If dev->parent is NULL, we get into the "successful" branch where the device is
> > added to dpm_active.  Do you think we should add any extra handling of this
> > case?
> 
> If a device is registered after dpm_suspend() has returned, the
> device won't be suspended properly before the system goes to sleep.  
> 
> If the device has a parent then you're okay, because the parent must
> already be suspended and so device_pm_add() will fail.  But if the
> device doesn't have a parent then device_pm_add() will succeed, which
> you don't want.

Well, can it happen in practice?  If it can, then what way can it happen?

> > > You could put in that static "all_devices_sleeping" flag, which gets set at
> > > the end of dpm_suspend() and cleared at the start of dpm_resume().
> > 
> > Well, I don't think it's necessary.  dpm_active is empty in that case and
> > we can use the list_empty(&dpm_active) check instead.
> 
> What would happen the very first time the system registers a device 
> during startup?

For any other uses than in device_pm_add(), the list_empty(&dpm_active) test
should be sufficient, IMHO.

It seems to me that we're discussing purely academic stuff here.  If it turns
out to be of any practical relevance, it will be trivial to add the handling of
it in the future.

Thanks,
Rafael

Re: [RFC][PATCH] PM: Make PM core handle device registrations concurrent with suspend/hibernation

[Alan Stern]

On Thu, 6 Mar 2008, Rafael J. Wysocki wrote:

> On Thursday, 6 of March 2008, Alan Stern wrote:
> > On Thu, 6 Mar 2008, Rafael J. Wysocki wrote:
> > 
> > > > I thought of one more thing you might want to add: device_pm_add() 
> > > > doesn't handle the case where dev->parent is NULL.
> > > 
> > > I'm not sure what you mean.
> > > 
> > > If dev->parent is NULL, we get into the "successful" branch where the device is
> > > added to dpm_active.  Do you think we should add any extra handling of this
> > > case?
> > 
> > If a device is registered after dpm_suspend() has returned, the
> > device won't be suspended properly before the system goes to sleep.  
> > 
> > If the device has a parent then you're okay, because the parent must
> > already be suspended and so device_pm_add() will fail.  But if the
> > device doesn't have a parent then device_pm_add() will succeed, which
> > you don't want.
> 
> Well, can it happen in practice?  If it can, then what way can it happen?

Yes, it can happen in practice when a new module is loaded.  In some 
ways, modules' init routines are like probe methods.

Maybe it can happen some other ways too, but I don't know of any.

> It seems to me that we're discussing purely academic stuff here.  If it turns
> out to be of any practical relevance, it will be trivial to add the handling of
> it in the future.

To be safe, I think we should make system sleep mutually exclusive with 
module loading.

Alan Stern

Re: [RFC][PATCH] PM: Make PM core handle device registrations concurrent with suspend/hibernation

[Rafael J. Wysocki]

On Thursday, 6 of March 2008, Alan Stern wrote:
> On Thu, 6 Mar 2008, Rafael J. Wysocki wrote:
> 
> > On Thursday, 6 of March 2008, Alan Stern wrote:
> > > On Thu, 6 Mar 2008, Rafael J. Wysocki wrote:
> > > 
> > > > > I thought of one more thing you might want to add: device_pm_add() 
> > > > > doesn't handle the case where dev->parent is NULL.
> > > > 
> > > > I'm not sure what you mean.
> > > > 
> > > > If dev->parent is NULL, we get into the "successful" branch where the device is
> > > > added to dpm_active.  Do you think we should add any extra handling of this
> > > > case?
> > > 
> > > If a device is registered after dpm_suspend() has returned, the
> > > device won't be suspended properly before the system goes to sleep.  
> > > 
> > > If the device has a parent then you're okay, because the parent must
> > > already be suspended and so device_pm_add() will fail.  But if the
> > > device doesn't have a parent then device_pm_add() will succeed, which
> > > you don't want.
> > 
> > Well, can it happen in practice?  If it can, then what way can it happen?
> 
> Yes, it can happen in practice when a new module is loaded.  In some 
> ways, modules' init routines are like probe methods.

Hmm, I'm not sure if it really is possible to load a module when all devices
have been suspended.  Never mind, though.

> Maybe it can happen some other ways too, but I don't know of any.
> 
> > It seems to me that we're discussing purely academic stuff here.  If it turns
> > out to be of any practical relevance, it will be trivial to add the handling of
> > it in the future.
> 
> To be safe, I think we should make system sleep mutually exclusive with 
> module loading.

Okay, is the (yet another) version of the patch below fine by you?

Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>

Modify the PM core to protect its data structures, specifically the
dpm_active list, from being corrupted if a child of the currently
suspending device is registered concurrently with its ->suspend()
callback.  In that case, since the new device (the child) is added
to dpm_active after its parent, the PM core will attempt to
suspend it after the parent, which is wrong.

Introduce a new member of struct dev_pm_info, called 'sleeping',
and use it to check if the parent of the device being added to
dpm_active has been suspended, in which case the device registration
fails.  Also, use 'sleeping' for checking if the ordering of devices
on dpm_active is correct.

Remove pm_sleep_rwsem which is not necessary any more.

Special thanks to Alan Stern for discussions and suggestions that
lead to the creation of this patch.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 Documentation/power/devices.txt |    5 +++
 drivers/base/core.c             |    6 +++-
 drivers/base/power/main.c       |   57 ++++++++++++++++++----------------------
 drivers/base/power/power.h      |   23 ++--------------
 include/linux/pm.h              |    1 
 5 files changed, 40 insertions(+), 52 deletions(-)

Index: linux-2.6/include/linux/pm.h
===================================================================
--- linux-2.6.orig/include/linux/pm.h
+++ linux-2.6/include/linux/pm.h
@@ -183,6 +183,7 @@ typedef struct pm_message {
 struct dev_pm_info {
 	pm_message_t		power_state;
 	unsigned		can_wakeup:1;
+	bool			sleeping:1;	/* Owned by the PM core */
 #ifdef	CONFIG_PM_SLEEP
 	unsigned		should_wakeup:1;
 	struct list_head	entry;
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
@@ -54,7 +54,8 @@ static LIST_HEAD(dpm_destroy);
 
 static DEFINE_MUTEX(dpm_list_mtx);
 
-static DECLARE_RWSEM(pm_sleep_rwsem);
+/* 'true' if all devices have been suspended, protected by dpm_list_mtx */
+static bool all_sleeping;
 
 int (*platform_enable_wakeup)(struct device *dev, int is_on);
 
@@ -62,14 +63,28 @@ int (*platform_enable_wakeup)(struct dev
  *	device_pm_add - add a device to the list of active devices
  *	@dev:	Device to be added to the list
  */
-void device_pm_add(struct device *dev)
+int device_pm_add(struct device *dev)
 {
+	int error = 0;
+
 	pr_debug("PM: Adding info for %s:%s\n",
 		 dev->bus ? dev->bus->name : "No Bus",
 		 kobject_name(&dev->kobj));
 	mutex_lock(&dpm_list_mtx);
-	list_add_tail(&dev->power.entry, &dpm_active);
+	if ((dev->parent && dev->parent->power.sleeping) || all_sleeping) {
+		if (dev->parent->power.sleeping)
+			dev_warn(dev,
+				"parent %s is sleeping, will not add\n",
+				dev->parent->bus_id);
+		else
+			dev_warn(dev, "devices are sleeping, will not add\n");
+		WARN_ON(true);
+		error = -EBUSY;
+	} else {
+		list_add_tail(&dev->power.entry, &dpm_active);
+	}
 	mutex_unlock(&dpm_list_mtx);
+	return error;
 }
 
 /**
@@ -107,32 +122,6 @@ void device_pm_schedule_removal(struct d
 }
 EXPORT_SYMBOL_GPL(device_pm_schedule_removal);
 
-/**
- *	pm_sleep_lock - mutual exclusion for registration and suspend
- *
- *	Returns 0 if no suspend is underway and device registration
- *	may proceed, otherwise -EBUSY.
- */
-int pm_sleep_lock(void)
-{
-	if (down_read_trylock(&pm_sleep_rwsem))
-		return 0;
-
-	return -EBUSY;
-}
-
-/**
- *	pm_sleep_unlock - mutual exclusion for registration and suspend
- *
- *	This routine undoes the effect of device_pm_add_lock
- *	when a device's registration is complete.
- */
-void pm_sleep_unlock(void)
-{
-	up_read(&pm_sleep_rwsem);
-}
-
-
 /*------------------------- Resume routines -------------------------*/
 
 /**
@@ -242,11 +231,13 @@ static int resume_device(struct device *
 static void dpm_resume(void)
 {
 	mutex_lock(&dpm_list_mtx);
+	all_sleeping = false;
 	while(!list_empty(&dpm_off)) {
 		struct list_head *entry = dpm_off.next;
 		struct device *dev = to_device(entry);
 
 		list_move_tail(entry, &dpm_active);
+		dev->power.sleeping = false;
 		mutex_unlock(&dpm_list_mtx);
 		resume_device(dev);
 		mutex_lock(&dpm_list_mtx);
@@ -285,7 +276,6 @@ void device_resume(void)
 	might_sleep();
 	dpm_resume();
 	unregister_dropped_devices();
-	up_write(&pm_sleep_rwsem);
 }
 EXPORT_SYMBOL_GPL(device_resume);
 
@@ -426,6 +416,9 @@ static int dpm_suspend(pm_message_t stat
 		struct list_head *entry = dpm_active.prev;
 		struct device *dev = to_device(entry);
 
+		WARN_ON(dev->parent && dev->parent->power.sleeping);
+
+		dev->power.sleeping = true;
 		mutex_unlock(&dpm_list_mtx);
 		error = suspend_device(dev, state);
 		mutex_lock(&dpm_list_mtx);
@@ -437,11 +430,14 @@ static int dpm_suspend(pm_message_t stat
 					(error == -EAGAIN ?
 					" (please convert to suspend_late)" :
 					""));
+			dev->power.sleeping = false;
 			break;
 		}
 		if (!list_empty(&dev->power.entry))
 			list_move(&dev->power.entry, &dpm_off);
 	}
+	if (!error)
+		all_sleeping = true;
 	mutex_unlock(&dpm_list_mtx);
 
 	return error;
@@ -459,7 +455,6 @@ int device_suspend(pm_message_t state)
 	int error;
 
 	might_sleep();
-	down_write(&pm_sleep_rwsem);
 	error = dpm_suspend(state);
 	if (error)
 		device_resume();
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,30 +11,13 @@ static inline struct device *to_device(s
 	return container_of(entry, struct device, power.entry);
 }
 
-extern void device_pm_add(struct device *);
+extern int device_pm_add(struct device *);
 extern void device_pm_remove(struct device *);
-extern int pm_sleep_lock(void);
-extern void pm_sleep_unlock(void);
 
 #else /* CONFIG_PM_SLEEP */
 
-
-static inline void device_pm_add(struct device *dev)
-{
-}
-
-static inline void device_pm_remove(struct device *dev)
-{
-}
-
-static inline int pm_sleep_lock(void)
-{
-	return 0;
-}
-
-static inline void pm_sleep_unlock(void)
-{
-}
+static inline int device_pm_add(struct device *dev) { return 0; }
+static inline void device_pm_remove(struct device *dev) {}
 
 #endif
 
Index: linux-2.6/drivers/base/core.c
===================================================================
--- linux-2.6.orig/drivers/base/core.c
+++ linux-2.6/drivers/base/core.c
@@ -815,7 +815,11 @@ int device_add(struct device *dev)
 	error = dpm_sysfs_add(dev);
 	if (error)
 		goto PMError;
-	device_pm_add(dev);
+	error = device_pm_add(dev);
+	if (error) {
+		dpm_sysfs_remove(dev);
+		goto PMError;
+	}
 	error = bus_add_device(dev);
 	if (error)
 		goto BusError;
Index: linux-2.6/Documentation/power/devices.txt
===================================================================
--- linux-2.6.orig/Documentation/power/devices.txt
+++ linux-2.6/Documentation/power/devices.txt
@@ -196,6 +196,11 @@ its parent; and can't be removed or susp
 
 The policy is that the device tree should match hardware bus topology.
 (Or at least the control bus, for devices which use multiple busses.)
+In particular, this means that a device registration may fail if the parent of
+the device is suspending (ie. has been chosen by the PM core as the next
+device to suspend) or has already suspended, as well as after all of the other
+devices have been suspended.  Device drivers must be prepared to cope with such
+situations.
 
 
 Suspending Devices

Re: [RFC][PATCH] PM: Make PM core handle device registrations concurrent with suspend/hibernation

[Alan Stern]

On Thu, 6 Mar 2008, Rafael J. Wysocki wrote:

> > > Well, can it happen in practice?  If it can, then what way can it happen?
> > 
> > Yes, it can happen in practice when a new module is loaded.  In some 
> > ways, modules' init routines are like probe methods.
> 
> Hmm, I'm not sure if it really is possible to load a module when all devices
> have been suspended.  Never mind, though.

You can load the module before devices are suspended, and then its init 
routine can run while the suspend is starting.

> > To be safe, I think we should make system sleep mutually exclusive with 
> > module loading.
> 
> Okay, is the (yet another) version of the patch below fine by you?

Yes, it's fine.  Mutual exclusion with module loading can be added 
later.  (Ironically, it may require putting pm_sleep_rwsem back!)

Alan Stern

Re: [RFC][PATCH] PM: Make PM core handle device registrations concurrent with suspend/hibernation

[Rafael J. Wysocki]

On Thursday, 6 of March 2008, Alan Stern wrote:
> On Thu, 6 Mar 2008, Rafael J. Wysocki wrote:
> 
> > > > Well, can it happen in practice?  If it can, then what way can it happen?
> > > 
> > > Yes, it can happen in practice when a new module is loaded.  In some 
> > > ways, modules' init routines are like probe methods.
> > 
> > Hmm, I'm not sure if it really is possible to load a module when all devices
> > have been suspended.  Never mind, though.
> 
> You can load the module before devices are suspended, and then its init 
> routine can run while the suspend is starting.

Yeah.

> > > To be safe, I think we should make system sleep mutually exclusive with 
> > > module loading.
> > 
> > Okay, is the (yet another) version of the patch below fine by you?
> 
> Yes, it's fine.  Mutual exclusion with module loading can be added 
> later.  (Ironically, it may require putting pm_sleep_rwsem back!)

I'm going to send this patch and the "include dpm_sysfs_add() into
device_pm_add()" patch for -mm/linux-next testing, if you don't mind.

I'm working on a new version of the "PM: Separate suspend and hibernation
callbacks" patch, on top of the two.

Thanks,
Rafael