PM: Acquire device locks on suspend

Greg, Andrew,

The appended patch is a replacement for gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch that deadlocked suspend and hibernation on some systems.

Please consider for applying.

Greg, Andrew,

The appended patch is a replacement for gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch that deadlocked suspend and hibernation on some systems.

Please consider for applying.

Quoted 7 lines · Rafael J. Wysocki

Greg, Andrew,

The appended patch is a replacement for gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch that deadlocked suspend and hibernation on some systems.

Please consider for applying.

This warning message:

Quoted 4 lines

drivers/base/core.c +0 -0

@@ -905,6 +915,13 @@ void device_del(struct device * dev)
 	struct device * parent = dev->parent;
 	struct class_interface *class_intf;
 

drivers/base/core.c void device_del(struct device * dev) · line 921

Quoted 4 lines

drivers/base/core.c +4 -0

+	if (pm_sleep_lock()) {
+		dev_warn(dev, "Illegal %s during suspend\n", __FUNCTION__);
+		dump_stack();
+	} else {

will unavoidably be triggered by this code:

Quoted 8 lines

drivers/base/power/main.c +8 -0

+void device_pm_destroy_suspended(struct device *dev)
+{
+	pr_debug("PM: Removing suspended device %s:%s\n",
+		 dev->bus ? dev->bus->name : "No Bus",
+		 kobject_name(&dev->kobj));
+	mutex_lock(&dpm_list_mtx);
+	list_del_init(&dev->power.entry);
+	mutex_unlock(&dpm_list_mtx);

drivers/base/power/main.c void device_pm_remove(struct device *dev · line 97

Quoted 3 lines

drivers/base/power/main.c +3 -0

+	up(&dev->sem);
+	device_unregister(dev);
+}

since the call to device_del() will occur while the pm_sleep_rwsem is still locked for writing. That's why I suggested not unregistering these devices until after everything else has been resumed and the rwsem has been dropped.

Another thing to watch out for: Just in case somebody ends up calling destroy_suspended_device(dev) from within dev's own resume method, you should interchange the resume_device() and the list_move_tail() calls in dpm_resume().

Quoted 4 lines

Another thing to watch out for: Just in case somebody ends up calling destroy_suspended_device(dev) from within dev's own resume method, you should interchange the resume_device() and the list_move_tail() calls in dpm_resume().

However, if we unregister them all at once after releasing pm_sleep_rwsem, that shouldn't be necessary, right?

Quoted 5 lines · Rafael J. Wysocki

Quoted 4 lines

Another thing to watch out for: Just in case somebody ends up calling destroy_suspended_device(dev) from within dev's own resume method, you should interchange the resume_device() and the list_move_tail() calls in dpm_resume().

Quoted 2 lines

However, if we unregister them all at once after releasing pm_sleep_rwsem, that shouldn't be necessary, right?

It's still necessary, because destroy_suspended_device() still has to move the device from one list to another. You don't want it to end up on the dpm_locked list.

Quoted 10 lines · Alan Stern

Quoted 7 lines · Rafael J. Wysocki

Quoted 4 lines

Another thing to watch out for: Just in case somebody ends up calling destroy_suspended_device(dev) from within dev's own resume method, you should interchange the resume_device() and the list_move_tail() calls in dpm_resume().

However, if we unregister them all at once after releasing pm_sleep_rwsem, that shouldn't be necessary, right?

Quoted 3 lines

It's still necessary, because destroy_suspended_device() still has to move the device from one list to another. You don't want it to end up on the dpm_locked list.

Hmm. That means we'd have to do the same thing in dpm_power_up() in case someone calls destroy_suspended_device() from resume_device_early(dev).

Still, even doing that is not enough, since someone can call destroy_suspended_device() from a .suspend() routine and then the device will end up on a wrong list just as well.

Quoted 10 lines · Alan Stern

Quoted 7 lines · Rafael J. Wysocki

Quoted 4 lines

Another thing to watch out for: Just in case somebody ends up calling destroy_suspended_device(dev) from within dev's own resume method, you should interchange the resume_device() and the list_move_tail() calls in dpm_resume().

However, if we unregister them all at once after releasing pm_sleep_rwsem, that shouldn't be necessary, right?

Quoted 3 lines

It's still necessary, because destroy_suspended_device() still has to move the device from one list to another. You don't want it to end up on the dpm_locked list.

Hmm. That means we'd have to do the same thing in dpm_power_up() in case someone calls destroy_suspended_device() from resume_device_early(dev).

Still, even doing that is not enough, since someone can call destroy_suspended_device() from a .suspend() routine and then the device will end up on a wrong list just as well.

Quoted 11 lines · Rafael J. Wysocki

Quoted 10 lines · Alan Stern

Quoted 7 lines · Rafael J. Wysocki

Quoted 4 lines

Another thing to watch out for: Just in case somebody ends up calling destroy_suspended_device(dev) from within dev's own resume method, you should interchange the resume_device() and the list_move_tail() calls in dpm_resume().

However, if we unregister them all at once after releasing pm_sleep_rwsem, that shouldn't be necessary, right?

Quoted 6 lines

Quoted 3 lines

It's still necessary, because destroy_suspended_device() still has to move the device from one list to another. You don't want it to end up on the dpm_locked list.

Hmm. That means we'd have to do the same thing in dpm_power_up() in case someone calls destroy_suspended_device() from resume_device_early(dev).

Yes.

Quoted 3 lines

Still, even doing that is not enough, since someone can call destroy_suspended_device() from a .suspend() routine and then the device will end up on a wrong list just as well.

That should never happen. The whole idea of destroy_suspended_device() is that the device couldn't be resumed and in fact should be unregistered because it is no longer working or no longer present. A suspend routine won't detect this sort of thing since it doesn't try to resume the device.

But it wouldn't hurt to mention in the kerneldoc that destroy_suspended_device() is meant to be called only during a system resume.

Quoted 12 lines · Alan Stern

Quoted 3 lines · Rafael J. Wysocki

Still, even doing that is not enough, since someone can call destroy_suspended_device() from a .suspend() routine and then the device will end up on a wrong list just as well.

That should never happen. The whole idea of destroy_suspended_device() is that the device couldn't be resumed and in fact should be unregistered because it is no longer working or no longer present. A suspend routine won't detect this sort of thing since it doesn't try to resume the device.

Quoted 3 lines

But it wouldn't hurt to mention in the kerneldoc that destroy_suspended_device() is meant to be called only during a system resume.

Hmm. Please have a look at the appended patch.

I have removed the warning from device_del() and used list_empty() to detect removed devices in the .suspend() routines. Is that viable?

Quoted 12 lines · Alan Stern

Quoted 3 lines · Rafael J. Wysocki

Still, even doing that is not enough, since someone can call destroy_suspended_device() from a .suspend() routine and then the device will end up on a wrong list just as well.

That should never happen. The whole idea of destroy_suspended_device() is that the device couldn't be resumed and in fact should be unregistered because it is no longer working or no longer present. A suspend routine won't detect this sort of thing since it doesn't try to resume the device.

Quoted 3 lines

But it wouldn't hurt to mention in the kerneldoc that destroy_suspended_device() is meant to be called only during a system resume.

Hmm. Please have a look at the appended patch.

I have removed the warning from device_del() and used list_empty() to detect removed devices in the .suspend() routines. Is that viable?

Quoted 13 lines · Rafael J. Wysocki

Quoted 12 lines · Alan Stern

Quoted 3 lines · Rafael J. Wysocki

Still, even doing that is not enough, since someone can call destroy_suspended_device() from a .suspend() routine and then the device will end up on a wrong list just as well.

That should never happen. The whole idea of destroy_suspended_device() is that the device couldn't be resumed and in fact should be unregistered because it is no longer working or no longer present. A suspend routine won't detect this sort of thing since it doesn't try to resume the device.

Quoted 8 lines

Quoted 3 lines

But it wouldn't hurt to mention in the kerneldoc that destroy_suspended_device() is meant to be called only during a system resume.

Hmm. Please have a look at the appended patch.

I have removed the warning from device_del() and used list_empty() to detect removed devices in the .suspend() routines. Is that viable?

It's not good.

The warning in device_del() is vital. It's what will tell people where the problem is when a deadlock occurs during system resume because some driver has mistakenly tried to unregister a device at the wrong time. It would have pointed immediately to the msr driver in the case of the bug Andrew found, for instance.

If you can figure out a way to disable the warning in device_del() for just the one device being unregistered by device_pm_destroy_suspended(),

I suppose that would be okay.

Quoted 3 lines

If you can figure out a way to disable the warning in device_del() for just the one device being unregistered by device_pm_destroy_suspended(),

Something like this, perhaps:

(quoted diff) +5 -0

@@ -905,6 +915,18 @@ void device_del(struct device * dev)
 	struct device * parent = dev->parent;
 	struct class_interface *class_intf;
 
+	if (down_trylock(&dev->sem)) {
+		if (pm_sleep_lock()) {
+			dev_warn(dev, "Illegal %s during suspend\n",
+				__FUNCTION__);
+			dump_stack();

Quoted 3 lines

If you can figure out a way to disable the warning in device_del() for just the one device being unregistered by device_pm_destroy_suspended(),

Something like this, perhaps:

(quoted diff) +5 -0

@@ -905,6 +915,18 @@ void device_del(struct device * dev)
 	struct device * parent = dev->parent;
 	struct class_interface *class_intf;
 
+	if (down_trylock(&dev->sem)) {
+		if (pm_sleep_lock()) {
+			dev_warn(dev, "Illegal %s during suspend\n",
+				__FUNCTION__);
+			dump_stack();

Quoted 19 lines · Rafael J. Wysocki

Quoted 3 lines

If you can figure out a way to disable the warning in device_del() for just the one device being unregistered by device_pm_destroy_suspended(),

Something like this, perhaps:

(quoted diff) +9 -0

@@ -905,6 +915,18 @@ void device_del(struct device * dev)
 	struct device * parent = dev->parent;
 	struct class_interface *class_intf;
 
+	if (down_trylock(&dev->sem)) {
+		if (pm_sleep_lock()) {
+			dev_warn(dev, "Illegal %s during suspend\n",
+				__FUNCTION__);
+			dump_stack();
+		} else {
+			pm_sleep_unlock();
+		}
+	} else {

drivers/base/power/main.c void device_pm_remove(struct device *dev

Quoted 6 lines

drivers/base/power/main.c +3 -0

+		up(&dev->sem);
+	}
+
 	if (parent)
 		klist_del(&dev->knode_parent);
 	if (MAJOR(dev->devt))

Bizarre, but it should work.

Be sure to include plenty of explanatory comments

-- otherwise nobody will be able to figure it out! :-)

Quoted 21 lines · Alan Stern

Quoted 19 lines · Rafael J. Wysocki

Quoted 3 lines

If you can figure out a way to disable the warning in device_del() for just the one device being unregistered by device_pm_destroy_suspended(),

Something like this, perhaps:

(quoted diff) +9 -0

@@ -905,6 +915,18 @@ void device_del(struct device * dev)
 	struct device * parent = dev->parent;
 	struct class_interface *class_intf;
 
+	if (down_trylock(&dev->sem)) {
+		if (pm_sleep_lock()) {
+			dev_warn(dev, "Illegal %s during suspend\n",
+				__FUNCTION__);
+			dump_stack();
+		} else {
+			pm_sleep_unlock();
+		}
+	} else {

Quoted 8 lines

drivers/base/power/main.c void device_pm_remove(struct device *dev

Quoted 6 lines

drivers/base/power/main.c +3 -0

+		up(&dev->sem);
+	}
+
 	if (parent)
 		klist_del(&dev->knode_parent);
 	if (MAJOR(dev->devt))

Bizarre, but it should work.

OK

Still, shouldn't we fail the removal of the device apart from giving the warning?

Quoted 31 lines · Rafael J. Wysocki

Quoted 21 lines · Alan Stern

Quoted 19 lines · Rafael J. Wysocki

Quoted 3 lines

If you can figure out a way to disable the warning in device_del() for just the one device being unregistered by device_pm_destroy_suspended(),

Something like this, perhaps:

(quoted diff) +9 -0

@@ -905,6 +915,18 @@ void device_del(struct device * dev)
 	struct device * parent = dev->parent;
 	struct class_interface *class_intf;
 
+	if (down_trylock(&dev->sem)) {
+		if (pm_sleep_lock()) {
+			dev_warn(dev, "Illegal %s during suspend\n",
+				__FUNCTION__);
+			dump_stack();
+		} else {
+			pm_sleep_unlock();
+		}
+	} else {

Quoted 8 lines

drivers/base/power/main.c void device_pm_remove(struct device *dev

Quoted 6 lines

drivers/base/power/main.c +3 -0

+		up(&dev->sem);
+	}
+
 	if (parent)
 		klist_del(&dev->knode_parent);
 	if (MAJOR(dev->devt))

Bizarre, but it should work.

Quoted 4 lines

OK

Still, shouldn't we fail the removal of the device apart from giving the warning?

Actually, having thought about it a bit more, I don't see the point in preventing the removal of the device from the list in device_pm_remove() if we allow all of the operations in device_del() preceding it to be performed.

Shouldn't we just take pm_sleep_rwsem in device_del() upfront and block on that if locked?

Quoted 31 lines · Rafael J. Wysocki

Quoted 21 lines · Alan Stern

Quoted 19 lines · Rafael J. Wysocki

Quoted 3 lines

If you can figure out a way to disable the warning in device_del() for just the one device being unregistered by device_pm_destroy_suspended(),

Something like this, perhaps:

(quoted diff) +9 -0

@@ -905,6 +915,18 @@ void device_del(struct device * dev)
 	struct device * parent = dev->parent;
 	struct class_interface *class_intf;
 
+	if (down_trylock(&dev->sem)) {
+		if (pm_sleep_lock()) {
+			dev_warn(dev, "Illegal %s during suspend\n",
+				__FUNCTION__);
+			dump_stack();
+		} else {
+			pm_sleep_unlock();
+		}
+	} else {

Quoted 8 lines

drivers/base/power/main.c void device_pm_remove(struct device *dev

Quoted 6 lines

drivers/base/power/main.c +3 -0

+		up(&dev->sem);
+	}
+
 	if (parent)
 		klist_del(&dev->knode_parent);
 	if (MAJOR(dev->devt))

Bizarre, but it should work.

Quoted 4 lines

OK

Still, shouldn't we fail the removal of the device apart from giving the warning?

Actually, having thought about it a bit more, I don't see the point in preventing the removal of the device from the list in device_pm_remove() if we allow all of the operations in device_del() preceding it to be performed.

Shouldn't we just take pm_sleep_rwsem in device_del() upfront and block on that if locked?

Quoted 2 lines

Still, shouldn't we fail the removal of the device apart from giving the warning?

Actually, having thought about it a bit more, I don't see the point in preventing the removal of the device from the list in device_pm_remove() if we allow all of the operations in device_del() preceding it to be performed.

Shouldn't we just take pm_sleep_rwsem in device_del() upfront and block on that if locked?

Quoted 2 lines

Still, shouldn't we fail the removal of the device apart from giving the warning?

Actually, having thought about it a bit more, I don't see the point in preventing the removal of the device from the list in device_pm_remove() if we allow all of the operations in device_del() preceding it to be performed.

Shouldn't we just take pm_sleep_rwsem in device_del() upfront and block on that if locked?

Quoted 6 lines · Rafael J. Wysocki

Quoted 2 lines

Still, shouldn't we fail the removal of the device apart from giving the warning?

Actually, having thought about it a bit more, I don't see the point in preventing the removal of the device from the list in device_pm_remove() if we allow all of the operations in device_del() preceding it to be performed.

That's not the issue. We _don't_ allow all of the operations in device_del() preceding the call to device_pm_remove(). In particular, the call to the device's driver's remove method will deadlock because device_release_driver() always has to acquire dev->sem.

Quoted 2 lines

Shouldn't we just take pm_sleep_rwsem in device_del() upfront and block on that if locked?

No -- the whole idea here is to print an error message in the system log if a driver's resume method tries to call device_del(). Deadlock is unavoidable in this case, but at least we'll know which driver is guilty.

Quoted 4 lines

No -- the whole idea here is to print an error message in the system log if a driver's resume method tries to call device_del(). Deadlock is unavoidable in this case, but at least we'll know which driver is guilty.

Still, if we do that, we won't need to acquire dev->sem in device_pm_remove() any more.

Apart from this, by acqiring pm_sleep_rwsem for reading in device_del() we can prevent a suspend from starting while the device is being removed.

Consider, for example, the scenario possible with the $subject patch:
- device_del() starts and notices pm_sleep_rwsem unlocked, so the warning is not printed
- it proceeds and everything before device_pm_remove() succeeds
- now, device_suspend() is called and locks dev->sem
- device_del() calls device_pm_remove() and blocks on that with the device partialy removed
I think we should prevent this from happening.

Quoted 5 lines · Rafael J. Wysocki

Quoted 4 lines

No -- the whole idea here is to print an error message in the system log if a driver's resume method tries to call device_del(). Deadlock is unavoidable in this case, but at least we'll know which driver is guilty.

Quoted 2 lines

Still, if we do that, we won't need to acquire dev->sem in device_pm_remove() any more.

There's a window in lock_all_devices() when dpm_list_mtx isn't held. We don't want device_pm_remove() taking an already-locked device off the dpm_locked list at that time. So we do need to acquire dev->sem in device_pm_remove().

Quoted 4 lines

Apart from this, by acqiring pm_sleep_rwsem for reading in device_del() we can prevent a suspend from starting while the device is being removed.

Quoted 8 lines

Consider, for example, the scenario possible with the $subject patch:
- device_del() starts and notices pm_sleep_rwsem unlocked, so the warning is not printed
- it proceeds and everything before device_pm_remove() succeeds
- now, device_suspend() is called and locks dev->sem
- device_del() calls device_pm_remove() and blocks on that with the device partialy removed
I think we should prevent this from happening.

I don't see anything wrong with it. All that will happen is that the removal will start before the suspend and finish after the resume.

Quoted 2 lines

I don't see anything wrong with it. All that will happen is that the removal will start before the suspend and finish after the resume.

In that case, we'll attempt to call the device's .suspend() and .resume() routines, but we shouldn't do that, IMHO.

Quoted 3 lines · Rafael J. Wysocki

Quoted 2 lines

I don't see anything wrong with it. All that will happen is that the removal will start before the suspend and finish after the resume.

Quoted 2 lines

In that case, we'll attempt to call the device's .suspend() and .resume() routines, but we shouldn't do that, IMHO.

I don't see anything wrong with that since the driver must be prepared to handle that even in the regular case, it's the only thing you can
guarantee: no more method calls after removal finishes. Am I totally misunderstanding things?

Quoted 5 lines · Johannes Berg

Quoted 3 lines · Rafael J. Wysocki

Quoted 2 lines

I don't see anything wrong with it. All that will happen is that the removal will start before the suspend and finish after the resume.

Quoted 7 lines

Quoted 2 lines

In that case, we'll attempt to call the device's .suspend() and .resume() routines, but we shouldn't do that, IMHO.

I don't see anything wrong with that since the driver must be prepared to handle that even in the regular case, it's the only thing you can
guarantee: no more method calls after removal finishes. Am I totally misunderstanding things?

Well, we are towards the end of device removal at this point, having called bus_remove_device(dev) for example, but still we've got it on dpm_active ...

This may not be technically wrong (ie. we should be able to recover from that), but it seems conceptually wrong and with pm_sleep_rwsem in place it can be avoided.

Quoted 5 lines · Johannes Berg

Quoted 3 lines · Rafael J. Wysocki

Quoted 2 lines

I don't see anything wrong with it. All that will happen is that the removal will start before the suspend and finish after the resume.

Quoted 7 lines

Quoted 2 lines

In that case, we'll attempt to call the device's .suspend() and .resume() routines, but we shouldn't do that, IMHO.

I don't see anything wrong with that since the driver must be prepared to handle that even in the regular case, it's the only thing you can
guarantee: no more method calls after removal finishes. Am I totally misunderstanding things?

Well, we are towards the end of device removal at this point, having called bus_remove_device(dev) for example, but still we've got it on dpm_active ...

This may not be technically wrong (ie. we should be able to recover from that), but it seems conceptually wrong and with pm_sleep_rwsem in place it can be avoided.

Quoted 3 lines · Rafael J. Wysocki

Quoted 2 lines

I don't see anything wrong with it. All that will happen is that the removal will start before the suspend and finish after the resume.

Quoted 2 lines

In that case, we'll attempt to call the device's .suspend() and .resume() routines, but we shouldn't do that, IMHO.

I don't see anything wrong with that since the driver must be prepared to handle that even in the regular case, it's the only thing you can
guarantee: no more method calls after removal finishes. Am I totally misunderstanding things?

Let's try to summarize the main issues here:

1. We want the PM core to lock all devices during suspend and hibernation. This implies that registration and unregistration at such times can't work, because they need to lock the device sem in order to make probe and remove method calls.

2. Registration calls can be failed, with an error message in the system log. However unregistration calls cannot fail. They _can_ block until the system resumes, but if the unregistration call was made from within a suspend or resume method it will deadlock. This seems inescapable, but at least we should print an error in the log so the offending driver can be identified.

3. In response to 2, the PM core should have a special routine for unregistering devices while a suspend is in progress. Rafael proposed that the core should unlock the device to permit the call to go through. This seems dangerous to me; I would prefer to leave the locks in place and defer the unregistration until after the system is back up and the locks have all been dropped. This would avoid all sorts of locking, deadlock, and mutual exclusion problems.

(As a side note: destroy_suspended_device() has a rather limited interface anyway, since it can handle only devices that were created by create_device().)

4. Rafael pointed out that unregistration can occur concurrently with system suspend. When this happens we can end up trying to suspend a device which has already been through bus_remove_device(), because it hasn't yet been removed from the dpm_active list. He proposes we make unregistration block system suspend, just as registration does.

I don't see 4 as a real problem. Starting an unregistration before the suspend and finishing it afterward should be okay. Once a device has gone through bus_remove_device() it hasn't got a suspend method any more, so trying to suspend it won't do anything at all -- the tests in suspend_device() will all fail. Conversely, if bus_remove_device() hasn't run yet then we would end up calling the driver's suspend method before the device_del() call returns. As Johannes pointed out, this is a normal race that would exist anyway.

On the other hand, having unregistration block system suspend wouldn't actually be wrong. I simply don't think it is necessary. But note that making the two mutually exclusive would complicate Rafael's synchronous approach for destroy_suspended_device().

5. All the discussion about pm_sleep_rwsem and so on is implementation details. Once we have settled on the correct approach for 1-4, the implementation should be relatively easy.

Please see the patch at: http://lkml.org/lkml/2008/1/6/298 . It represents my current idea about how to do that.

Please see the patch at: http://lkml.org/lkml/2008/1/6/298 . It represents my current idea about how to do that.

Quoted 2 lines · Rafael J. Wysocki

Please see the patch at: http://lkml.org/lkml/2008/1/6/298 . It represents my current idea about how to do that.

It has some problems.

First, note that the list manipulations in dpm_suspend(), device_power_down(), and so on aren't protected by dpm_list_mtx. So your patch could corrupt the list pointers.

Are you assuming that no other threads can be running at this time?

Note also that device_pm_destroy_suspended() does up(&dev->sem), but it doesn't know whether or not dev->sem was locked to begin with.

Do you want to rule out the possibility of a driver's suspend or remove methods calling destroy_suspended_device() on its own device? With your synchronous approach, this would mean that the suspend/resume method would indirectly end up calling the remove method. This is dangerous at best; with USB it would be a lockdep violation. With an asynchronous approach, on the other hand, this wouldn't be a problem.

Quoted 7 lines · Alan Stern

Quoted 2 lines · Rafael J. Wysocki

Please see the patch at: http://lkml.org/lkml/2008/1/6/298 . It represents my current idea about how to do that.

It has some problems.

Quoted 3 lines

First, note that the list manipulations in dpm_suspend(), device_power_down(), and so on aren't protected by dpm_list_mtx. So your patch could corrupt the list pointers.

Yes, they need the locking. I have overlooked that, mostly because the locking was removed by gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch too (because you assumed there woundn't be any need to remove a device during a suspend, right?).

Quoted 1 line

Are you assuming that no other threads can be running at this time?

No, I'm not.

Quoted 2 lines

Note also that device_pm_destroy_suspended() does up(&dev->sem), but it doesn't know whether or not dev->sem was locked to begin with.

Do you mean it might have been released already by another thread calling device_pm_destroy_suspended() on the same device?

Quoted 6 lines

Do you want to rule out the possibility of a driver's suspend or remove methods calling destroy_suspended_device() on its own device? With your synchronous approach, this would mean that the suspend/resume method would indirectly end up calling the remove method. This is dangerous at best; with USB it would be a lockdep violation. With an asynchronous approach, on the other hand, this wouldn't be a problem.

Well, the asynchronous apprach has the problem that the device may end up on a wrong list when removed by one of the .suspend() callbacks (and I don't see how to avoid that without extra complexity). Perhaps that's something we can live with, though.

One more question: is there any particular reason not to call device_pm_remove() at the beginning of device_del()?

Quoted 7 lines · Alan Stern

Quoted 2 lines · Rafael J. Wysocki

Please see the patch at: http://lkml.org/lkml/2008/1/6/298 . It represents my current idea about how to do that.

It has some problems.

Quoted 3 lines

First, note that the list manipulations in dpm_suspend(), device_power_down(), and so on aren't protected by dpm_list_mtx. So your patch could corrupt the list pointers.

Yes, they need the locking. I have overlooked that, mostly because the locking was removed by gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch too (because you assumed there woundn't be any need to remove a device during a suspend, right?).

Quoted 1 line

Are you assuming that no other threads can be running at this time?

No, I'm not.

Quoted 2 lines

Note also that device_pm_destroy_suspended() does up(&dev->sem), but it doesn't know whether or not dev->sem was locked to begin with.

Do you mean it might have been released already by another thread calling device_pm_destroy_suspended() on the same device?

Quoted 6 lines

Do you want to rule out the possibility of a driver's suspend or remove methods calling destroy_suspended_device() on its own device? With your synchronous approach, this would mean that the suspend/resume method would indirectly end up calling the remove method. This is dangerous at best; with USB it would be a lockdep violation. With an asynchronous approach, on the other hand, this wouldn't be a problem.

Well, the asynchronous apprach has the problem that the device may end up on a wrong list when removed by one of the .suspend() callbacks (and I don't see how to avoid that without extra complexity). Perhaps that's something we can live with, though.

One more question: is there any particular reason not to call device_pm_remove() at the beginning of device_del()?

Quoted 8 lines · Rafael J. Wysocki

Quoted 7 lines · Alan Stern

Quoted 2 lines · Rafael J. Wysocki

Please see the patch at: http://lkml.org/lkml/2008/1/6/298 . It represents my current idea about how to do that.

It has some problems.

Quoted 8 lines

Quoted 3 lines

First, note that the list manipulations in dpm_suspend(), device_power_down(), and so on aren't protected by dpm_list_mtx. So your patch could corrupt the list pointers.

Yes, they need the locking. I have overlooked that, mostly because the locking was removed by gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch too (because you assumed there woundn't be any need to remove a device during a suspend, right?).

Right.

Quoted 4 lines

Quoted 1 line

Are you assuming that no other threads can be running at this time?

No, I'm not.

Quoted 5 lines

Quoted 2 lines

Note also that device_pm_destroy_suspended() does up(&dev->sem), but it doesn't know whether or not dev->sem was locked to begin with.

Do you mean it might have been released already by another thread calling device_pm_destroy_suspended() on the same device?

I was thinking that it might be called before lock_all_devices().

However let's ignore that possibility and simplify the discussion by assuming that destroy_suspended_device() is never called except by a suspend or resume method for that device or one of its ancestors.

(This still leaves the possibility that it might get called by mistake during a runtime suspend or resume...)

Quoted 7 lines

Quoted 6 lines

Do you want to rule out the possibility of a driver's suspend or remove methods calling destroy_suspended_device() on its own device? With your synchronous approach, this would mean that the suspend/resume method would indirectly end up calling the remove method. This is dangerous at best; with USB it would be a lockdep violation. With an asynchronous approach, on the other hand, this wouldn't be a problem.

Quoted 4 lines

Well, the asynchronous apprach has the problem that the device may end up on a wrong list when removed by one of the .suspend() callbacks (and I don't see how to avoid that without extra complexity). Perhaps that's something we can live with, though.

The same problem affects the synchronous approach.

We can fix it by having dpm_suspend() do the list_move() before calling suspend_device(). Then if the suspend fails move the device back.

Quoted 2 lines

One more question: is there any particular reason not to call device_pm_remove() at the beginning of device_del()?

I think it's done this way to avoid having a window where the device isn't on a PM list and is still owned by the bus and the driver. But if a suspend occurs during that window, it shouldn't matter that the device will be left unsuspended. After all, the same thing would have happened if the suspend occurred after bus_remove_device().

So no, there shouldn't be a problem with moving the call.

Quoted 4 lines

Quoted 2 lines

Do you mean it might have been released already by another thread calling device_pm_destroy_suspended() on the same device?

I was thinking that it might be called before lock_all_devices().

I've added pm_sleep_start_end_mtx and the locking dance in device_pm_destroy_suspended() specifically to prevent this from happening.

Quoted 3 lines

However let's ignore that possibility and simplify the discussion by assuming that destroy_suspended_device() is never called except by a suspend or resume method for that device or one of its ancestors.

It may also be called by one of the CPU hotplug notifiers.

Okay, well, now I'm leaning towards the asynchronous approach.

I'll prepare a new patch and send it later today.

Quoted 4 lines

Quoted 2 lines

Do you mean it might have been released already by another thread calling device_pm_destroy_suspended() on the same device?

I was thinking that it might be called before lock_all_devices().

I've added pm_sleep_start_end_mtx and the locking dance in device_pm_destroy_suspended() specifically to prevent this from happening.

Quoted 3 lines

However let's ignore that possibility and simplify the discussion by assuming that destroy_suspended_device() is never called except by a suspend or resume method for that device or one of its ancestors.

It may also be called by one of the CPU hotplug notifiers.

Okay, well, now I'm leaning towards the asynchronous approach.

I'll prepare a new patch and send it later today.

On Mon, 7 Jan 2008, Rafael J. Wysocki wrote:

Quoted 4 lines

Okay, well, now I'm leaning towards the asynchronous approach.

I'll prepare a new patch and send it later today.

Okay.

Appended is what I managed to put together today.

It probably still has some problems, but I'm not seeing them right now (too tired). At least, it doesn't break my system. ;-)

Please review.

Appended is what I managed to put together today.

It probably still has some problems, but I'm not seeing them right now (too tired). At least, it doesn't break my system. ;-)

Please review.

Quoted 5 lines · Rafael J. Wysocki

Appended is what I managed to put together today.

It probably still has some problems, but I'm not seeing them right now (too tired). At least, it doesn't break my system. ;-)

Quoted 1 line

Please review.

Okay, this seems to be better. I like the way the complicated tests are all localized in power/main.c.

In dpm_resume() you shouldn't need to use dpm_list_mtx at all, because the list_move_tail() comes before the resume_device(). It's the same as in dpm_power_up().

The same is true for dpm_suspend(). Once all the device have been locked, there shouldn't be any other tasks accessing the dpm lists. Hence there should be no need to protect the list.

Which reminds me, the kerneldoc for device_pm_schedule_removal() is inaccurate. The routine always just moves the device to dpm_destroy list for later processing.

Also, the kerneldoc for destroy_suspended_device() should contain an extra paragraph warning that the routine should never be called except within the scope of a system sleep transition. In practice this means it has to be directly or indirectly invoked by a suspend or resume method.

It looks good.

Quoted 3 lines

In dpm_resume() you shouldn't need to use dpm_list_mtx at all, because the list_move_tail() comes before the resume_device(). It's the same as in dpm_power_up().

Still, device_pm_schedule_removal() can (in theory) be called concurrently with dpm_resume() by another thread and this might corrupt the list without the locking.

Quoted 5 lines

Also, the kerneldoc for destroy_suspended_device() should contain an extra paragraph warning that the routine should never be called except within the scope of a system sleep transition. In practice this means it has to be directly or indirectly invoked by a suspend or resume method.

Or by a CPU hotplug notifier (that will be the majority of cases, IMO).

Quoted 3 lines

In dpm_resume() you shouldn't need to use dpm_list_mtx at all, because the list_move_tail() comes before the resume_device(). It's the same as in dpm_power_up().

Still, device_pm_schedule_removal() can (in theory) be called concurrently with dpm_resume() by another thread and this might corrupt the list without the locking.

Quoted 5 lines

Also, the kerneldoc for destroy_suspended_device() should contain an extra paragraph warning that the routine should never be called except within the scope of a system sleep transition. In practice this means it has to be directly or indirectly invoked by a suspend or resume method.

Or by a CPU hotplug notifier (that will be the majority of cases, IMO).

Quoted 7 lines · Rafael J. Wysocki

Quoted 3 lines

In dpm_resume() you shouldn't need to use dpm_list_mtx at all, because the list_move_tail() comes before the resume_device(). It's the same as in dpm_power_up().

Still, device_pm_schedule_removal() can (in theory) be called concurrently with dpm_resume() by another thread and this might corrupt the list without the locking.

Any thread doing that would be in violation of the restrictions you're going to add to the kerneldoc for destroy_suspended_device().

However the overhead for the locking isn't critical. There won't be any contention (if everything is working right) and it isn't a hot path anyway. So you can leave the extra locking in if you want. But then you should put it in all the routines where the lists get manipulated, not just some of them. That is: device_power_down(), dpm_power_up(), and even unregister_dropped_devices().

Quoted 7 lines

Quoted 5 lines

Also, the kerneldoc for destroy_suspended_device() should contain an extra paragraph warning that the routine should never be called except within the scope of a system sleep transition. In practice this means it has to be directly or indirectly invoked by a suspend or resume method.

Or by a CPU hotplug notifier (that will be the majority of cases, IMO).

In your patch the call is made in response to a CPU_UP_CANCELED_FROZEN notification. Isn't it true that this notification is issued only as part of a system sleep transition?

We wouldn't want to allow destroy_suspended_device() to be called when an arbitrary CPU hotplug notification occurs.

Quoted 13 lines · Alan Stern

Quoted 7 lines · Rafael J. Wysocki

Quoted 3 lines

In dpm_resume() you shouldn't need to use dpm_list_mtx at all, because the list_move_tail() comes before the resume_device(). It's the same as in dpm_power_up().

Still, device_pm_schedule_removal() can (in theory) be called concurrently with dpm_resume() by another thread and this might corrupt the list without the locking.

Any thread doing that would be in violation of the restrictions you're going to add to the kerneldoc for destroy_suspended_device().

Quoted 6 lines

However the overhead for the locking isn't critical. There won't be any contention (if everything is working right) and it isn't a hot path anyway. So you can leave the extra locking in if you want. But then you should put it in all the routines where the lists get manipulated, not just some of them. That is: device_power_down(), dpm_power_up(), and even unregister_dropped_devices().

Except for those run on one CPU with interrupts disabled, I think.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Also, the kerneldoc for destroy_suspended_device() should contain an extra paragraph warning that the routine should never be called except within the scope of a system sleep transition. In practice this means it has to be directly or indirectly invoked by a suspend or resume method.

Quoted 5 lines

Quoted 1 line

Or by a CPU hotplug notifier (that will be the majority of cases, IMO).

In your patch the call is made in response to a CPU_UP_CANCELED_FROZEN notification. Isn't it true that this notification is issued only as part of a system sleep transition?

Yes, it is.

Quoted 13 lines · Alan Stern

Quoted 7 lines · Rafael J. Wysocki

Quoted 3 lines

In dpm_resume() you shouldn't need to use dpm_list_mtx at all, because the list_move_tail() comes before the resume_device(). It's the same as in dpm_power_up().

Still, device_pm_schedule_removal() can (in theory) be called concurrently with dpm_resume() by another thread and this might corrupt the list without the locking.

Any thread doing that would be in violation of the restrictions you're going to add to the kerneldoc for destroy_suspended_device().

Quoted 6 lines

However the overhead for the locking isn't critical. There won't be any contention (if everything is working right) and it isn't a hot path anyway. So you can leave the extra locking in if you want. But then you should put it in all the routines where the lists get manipulated, not just some of them. That is: device_power_down(), dpm_power_up(), and even unregister_dropped_devices().

Except for those run on one CPU with interrupts disabled, I think.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Also, the kerneldoc for destroy_suspended_device() should contain an extra paragraph warning that the routine should never be called except within the scope of a system sleep transition. In practice this means it has to be directly or indirectly invoked by a suspend or resume method.

Quoted 5 lines

Quoted 1 line

Or by a CPU hotplug notifier (that will be the majority of cases, IMO).

In your patch the call is made in response to a CPU_UP_CANCELED_FROZEN notification. Isn't it true that this notification is issued only as part of a system sleep transition?

Yes, it is.

Quoted 21 lines · Rafael J. Wysocki

Quoted 19 lines · Alan Stern

Quoted 7 lines · Rafael J. Wysocki

Quoted 3 lines

In dpm_resume() you shouldn't need to use dpm_list_mtx at all, because the list_move_tail() comes before the resume_device(). It's the same as in dpm_power_up().

Still, device_pm_schedule_removal() can (in theory) be called concurrently with dpm_resume() by another thread and this might corrupt the list without the locking.

Any thread doing that would be in violation of the restrictions you're going to add to the kerneldoc for destroy_suspended_device().

However the overhead for the locking isn't critical. There won't be any contention (if everything is working right) and it isn't a hot path anyway. So you can leave the extra locking in if you want. But then you should put it in all the routines where the lists get manipulated, not just some of them. That is: device_power_down(), dpm_power_up(), and even unregister_dropped_devices().

Quoted 1 line

Except for those run on one CPU with interrupts disabled, I think.

Not unregister_dropped_devices()!

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

Also, the kerneldoc for destroy_suspended_device() should contain an extra paragraph warning that the routine should never be called except within the scope of a system sleep transition. In practice this means it has to be directly or indirectly invoked by a suspend or resume method.

Or by a CPU hotplug notifier (that will be the majority of cases, IMO).

Quoted 5 lines

Quoted 3 lines

In your patch the call is made in response to a CPU_UP_CANCELED_FROZEN notification. Isn't it true that this notification is issued only as part of a system sleep transition?

Yes, it is.

So it counts as being indirectly invoked by a resume method.

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Quoted 5 lines

Also, the kerneldoc for destroy_suspended_device() should contain an extra paragraph warning that the routine should never be called except within the scope of a system sleep transition. In practice this means it has to be directly or indirectly invoked by a suspend or resume method.

Or by a CPU hotplug notifier (that will be the majority of cases, IMO).

In your patch the call is made in response to a CPU_UP_CANCELED_FROZEN notification. Isn't it true that this notification is issued only as part of a system sleep transition?

Quoted 3 lines

Quoted 1 line

Yes, it is.

So it counts as being indirectly invoked by a resume method.

Rather, by the resume core. Technically, it's invoked by enable_nonboot_cpus(), which is not a resume method literally.

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Quoted 5 lines

Also, the kerneldoc for destroy_suspended_device() should contain an extra paragraph warning that the routine should never be called except within the scope of a system sleep transition. In practice this means it has to be directly or indirectly invoked by a suspend or resume method.

Or by a CPU hotplug notifier (that will be the majority of cases, IMO).

In your patch the call is made in response to a CPU_UP_CANCELED_FROZEN notification. Isn't it true that this notification is issued only as part of a system sleep transition?

Quoted 3 lines

Quoted 1 line

Yes, it is.

So it counts as being indirectly invoked by a resume method.

Rather, by the resume core. Technically, it's invoked by enable_nonboot_cpus(), which is not a resume method literally.

Quoted 21 lines · Rafael J. Wysocki

Quoted 19 lines · Alan Stern

Quoted 7 lines · Rafael J. Wysocki

Quoted 3 lines

In dpm_resume() you shouldn't need to use dpm_list_mtx at all, because the list_move_tail() comes before the resume_device(). It's the same as in dpm_power_up().

Still, device_pm_schedule_removal() can (in theory) be called concurrently with dpm_resume() by another thread and this might corrupt the list without the locking.

Any thread doing that would be in violation of the restrictions you're going to add to the kerneldoc for destroy_suspended_device().

However the overhead for the locking isn't critical. There won't be any contention (if everything is working right) and it isn't a hot path anyway. So you can leave the extra locking in if you want. But then you should put it in all the routines where the lists get manipulated, not just some of them. That is: device_power_down(), dpm_power_up(), and even unregister_dropped_devices().

Quoted 1 line

Except for those run on one CPU with interrupts disabled, I think.

Not unregister_dropped_devices()!

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

Also, the kerneldoc for destroy_suspended_device() should contain an extra paragraph warning that the routine should never be called except within the scope of a system sleep transition. In practice this means it has to be directly or indirectly invoked by a suspend or resume method.

Or by a CPU hotplug notifier (that will be the majority of cases, IMO).

Quoted 5 lines

Quoted 3 lines

In your patch the call is made in response to a CPU_UP_CANCELED_FROZEN notification. Isn't it true that this notification is issued only as part of a system sleep transition?

Yes, it is.

So it counts as being indirectly invoked by a resume method.

Quoted 7 lines · Rafael J. Wysocki

Quoted 3 lines

In dpm_resume() you shouldn't need to use dpm_list_mtx at all, because the list_move_tail() comes before the resume_device(). It's the same as in dpm_power_up().

Still, device_pm_schedule_removal() can (in theory) be called concurrently with dpm_resume() by another thread and this might corrupt the list without the locking.

Any thread doing that would be in violation of the restrictions you're going to add to the kerneldoc for destroy_suspended_device().

However the overhead for the locking isn't critical. There won't be any contention (if everything is working right) and it isn't a hot path anyway. So you can leave the extra locking in if you want. But then you should put it in all the routines where the lists get manipulated, not just some of them. That is: device_power_down(), dpm_power_up(), and even unregister_dropped_devices().

Quoted 7 lines

Quoted 5 lines

Also, the kerneldoc for destroy_suspended_device() should contain an extra paragraph warning that the routine should never be called except within the scope of a system sleep transition. In practice this means it has to be directly or indirectly invoked by a suspend or resume method.

Or by a CPU hotplug notifier (that will be the majority of cases, IMO).

In your patch the call is made in response to a CPU_UP_CANCELED_FROZEN notification. Isn't it true that this notification is issued only as part of a system sleep transition?

We wouldn't want to allow destroy_suspended_device() to be called when an arbitrary CPU hotplug notification occurs.

Quoted 5 lines · Rafael J. Wysocki

Appended is what I managed to put together today.

It probably still has some problems, but I'm not seeing them right now (too tired). At least, it doesn't break my system. ;-)

Quoted 1 line

Please review.

Okay, this seems to be better. I like the way the complicated tests are all localized in power/main.c.

In dpm_resume() you shouldn't need to use dpm_list_mtx at all, because the list_move_tail() comes before the resume_device(). It's the same as in dpm_power_up().

The same is true for dpm_suspend(). Once all the device have been locked, there shouldn't be any other tasks accessing the dpm lists. Hence there should be no need to protect the list.

Which reminds me, the kerneldoc for device_pm_schedule_removal() is inaccurate. The routine always just moves the device to dpm_destroy list for later processing.

Also, the kerneldoc for destroy_suspended_device() should contain an extra paragraph warning that the routine should never be called except within the scope of a system sleep transition. In practice this means it has to be directly or indirectly invoked by a suspend or resume method.

It looks good.

On Mon, 7 Jan 2008, Rafael J. Wysocki wrote:

Quoted 4 lines

Okay, well, now I'm leaning towards the asynchronous approach.

I'll prepare a new patch and send it later today.

Okay.

Quoted 8 lines · Rafael J. Wysocki

Quoted 7 lines · Alan Stern

Quoted 2 lines · Rafael J. Wysocki

Please see the patch at: http://lkml.org/lkml/2008/1/6/298 . It represents my current idea about how to do that.

It has some problems.

Quoted 8 lines

Quoted 3 lines

First, note that the list manipulations in dpm_suspend(), device_power_down(), and so on aren't protected by dpm_list_mtx. So your patch could corrupt the list pointers.

Yes, they need the locking. I have overlooked that, mostly because the locking was removed by gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch too (because you assumed there woundn't be any need to remove a device during a suspend, right?).

Right.

Quoted 4 lines

Quoted 1 line

Are you assuming that no other threads can be running at this time?

No, I'm not.

Quoted 5 lines

Quoted 2 lines

Note also that device_pm_destroy_suspended() does up(&dev->sem), but it doesn't know whether or not dev->sem was locked to begin with.

Do you mean it might have been released already by another thread calling device_pm_destroy_suspended() on the same device?

I was thinking that it might be called before lock_all_devices().

However let's ignore that possibility and simplify the discussion by assuming that destroy_suspended_device() is never called except by a suspend or resume method for that device or one of its ancestors.

(This still leaves the possibility that it might get called by mistake during a runtime suspend or resume...)

Quoted 7 lines

Quoted 6 lines

Do you want to rule out the possibility of a driver's suspend or remove methods calling destroy_suspended_device() on its own device? With your synchronous approach, this would mean that the suspend/resume method would indirectly end up calling the remove method. This is dangerous at best; with USB it would be a lockdep violation. With an asynchronous approach, on the other hand, this wouldn't be a problem.

Quoted 4 lines

Well, the asynchronous apprach has the problem that the device may end up on a wrong list when removed by one of the .suspend() callbacks (and I don't see how to avoid that without extra complexity). Perhaps that's something we can live with, though.

The same problem affects the synchronous approach.

We can fix it by having dpm_suspend() do the list_move() before calling suspend_device(). Then if the suspend fails move the device back.

Quoted 2 lines

One more question: is there any particular reason not to call device_pm_remove() at the beginning of device_del()?

I think it's done this way to avoid having a window where the device isn't on a PM list and is still owned by the bus and the driver. But if a suspend occurs during that window, it shouldn't matter that the device will be left unsuspended. After all, the same thing would have happened if the suspend occurred after bus_remove_device().

So no, there shouldn't be a problem with moving the call.

Quoted 2 lines · Rafael J. Wysocki

Please see the patch at: http://lkml.org/lkml/2008/1/6/298 . It represents my current idea about how to do that.

It has some problems.

First, note that the list manipulations in dpm_suspend(), device_power_down(), and so on aren't protected by dpm_list_mtx. So your patch could corrupt the list pointers.

Are you assuming that no other threads can be running at this time?

Note also that device_pm_destroy_suspended() does up(&dev->sem), but it doesn't know whether or not dev->sem was locked to begin with.

Do you want to rule out the possibility of a driver's suspend or remove methods calling destroy_suspended_device() on its own device? With your synchronous approach, this would mean that the suspend/resume method would indirectly end up calling the remove method. This is dangerous at best; with USB it would be a lockdep violation. With an asynchronous approach, on the other hand, this wouldn't be a problem.

Let's try to summarize the main issues here:

1. We want the PM core to lock all devices during suspend and hibernation. This implies that registration and unregistration at such times can't work, because they need to lock the device sem in order to make probe and remove method calls.

2. Registration calls can be failed, with an error message in the system log. However unregistration calls cannot fail. They _can_ block until the system resumes, but if the unregistration call was made from within a suspend or resume method it will deadlock. This seems inescapable, but at least we should print an error in the log so the offending driver can be identified.

3. In response to 2, the PM core should have a special routine for unregistering devices while a suspend is in progress. Rafael proposed that the core should unlock the device to permit the call to go through. This seems dangerous to me; I would prefer to leave the locks in place and defer the unregistration until after the system is back up and the locks have all been dropped. This would avoid all sorts of locking, deadlock, and mutual exclusion problems.

(As a side note: destroy_suspended_device() has a rather limited interface anyway, since it can handle only devices that were created by create_device().)

4. Rafael pointed out that unregistration can occur concurrently with system suspend. When this happens we can end up trying to suspend a device which has already been through bus_remove_device(), because it hasn't yet been removed from the dpm_active list. He proposes we make unregistration block system suspend, just as registration does.

I don't see 4 as a real problem. Starting an unregistration before the suspend and finishing it afterward should be okay. Once a device has gone through bus_remove_device() it hasn't got a suspend method any more, so trying to suspend it won't do anything at all -- the tests in suspend_device() will all fail. Conversely, if bus_remove_device() hasn't run yet then we would end up calling the driver's suspend method before the device_del() call returns. As Johannes pointed out, this is a normal race that would exist anyway.

On the other hand, having unregistration block system suspend wouldn't actually be wrong. I simply don't think it is necessary. But note that making the two mutually exclusive would complicate Rafael's synchronous approach for destroy_suspended_device().

5. All the discussion about pm_sleep_rwsem and so on is implementation details. Once we have settled on the correct approach for 1-4, the implementation should be relatively easy.

Quoted 5 lines · Rafael J. Wysocki

Quoted 4 lines

No -- the whole idea here is to print an error message in the system log if a driver's resume method tries to call device_del(). Deadlock is unavoidable in this case, but at least we'll know which driver is guilty.

Quoted 2 lines

Still, if we do that, we won't need to acquire dev->sem in device_pm_remove() any more.

There's a window in lock_all_devices() when dpm_list_mtx isn't held. We don't want device_pm_remove() taking an already-locked device off the dpm_locked list at that time. So we do need to acquire dev->sem in device_pm_remove().

Quoted 4 lines

Apart from this, by acqiring pm_sleep_rwsem for reading in device_del() we can prevent a suspend from starting while the device is being removed.

Quoted 8 lines

Consider, for example, the scenario possible with the $subject patch:
- device_del() starts and notices pm_sleep_rwsem unlocked, so the warning is not printed
- it proceeds and everything before device_pm_remove() succeeds
- now, device_suspend() is called and locks dev->sem
- device_del() calls device_pm_remove() and blocks on that with the device partialy removed
I think we should prevent this from happening.

I don't see anything wrong with it. All that will happen is that the removal will start before the suspend and finish after the resume.

Quoted 4 lines

No -- the whole idea here is to print an error message in the system log if a driver's resume method tries to call device_del(). Deadlock is unavoidable in this case, but at least we'll know which driver is guilty.

Still, if we do that, we won't need to acquire dev->sem in device_pm_remove() any more.

Apart from this, by acqiring pm_sleep_rwsem for reading in device_del() we can prevent a suspend from starting while the device is being removed.

Consider, for example, the scenario possible with the $subject patch:
- device_del() starts and notices pm_sleep_rwsem unlocked, so the warning is not printed
- it proceeds and everything before device_pm_remove() succeeds
- now, device_suspend() is called and locks dev->sem
- device_del() calls device_pm_remove() and blocks on that with the device partialy removed
I think we should prevent this from happening.

Quoted 6 lines · Rafael J. Wysocki

Quoted 2 lines

Still, shouldn't we fail the removal of the device apart from giving the warning?

Actually, having thought about it a bit more, I don't see the point in preventing the removal of the device from the list in device_pm_remove() if we allow all of the operations in device_del() preceding it to be performed.

That's not the issue. We _don't_ allow all of the operations in device_del() preceding the call to device_pm_remove(). In particular, the call to the device's driver's remove method will deadlock because device_release_driver() always has to acquire dev->sem.

Quoted 2 lines

Shouldn't we just take pm_sleep_rwsem in device_del() upfront and block on that if locked?

No -- the whole idea here is to print an error message in the system log if a driver's resume method tries to call device_del(). Deadlock is unavoidable in this case, but at least we'll know which driver is guilty.

Quoted 21 lines · Alan Stern

Quoted 19 lines · Rafael J. Wysocki

Quoted 3 lines

If you can figure out a way to disable the warning in device_del() for just the one device being unregistered by device_pm_destroy_suspended(),

Something like this, perhaps:

(quoted diff) +9 -0

@@ -905,6 +915,18 @@ void device_del(struct device * dev)
 	struct device * parent = dev->parent;
 	struct class_interface *class_intf;
 
+	if (down_trylock(&dev->sem)) {
+		if (pm_sleep_lock()) {
+			dev_warn(dev, "Illegal %s during suspend\n",
+				__FUNCTION__);
+			dump_stack();
+		} else {
+			pm_sleep_unlock();
+		}
+	} else {

Quoted 8 lines

drivers/base/power/main.c void device_pm_remove(struct device *dev

Quoted 6 lines

drivers/base/power/main.c +3 -0

+		up(&dev->sem);
+	}
+
 	if (parent)
 		klist_del(&dev->knode_parent);
 	if (MAJOR(dev->devt))

Bizarre, but it should work.

OK

Still, shouldn't we fail the removal of the device apart from giving the warning?

On Sunday, 6 of January 2008, Alan Stern wrote:

Still, shouldn't we fail the removal of the device apart from giving the warning?

On Sunday, 6 of January 2008, Alan Stern wrote:

Still, shouldn't we fail the removal of the device apart from giving the warning?

Quoted 4 lines · Rafael J. Wysocki

On Sunday, 6 of January 2008, Alan Stern wrote:

Still, shouldn't we fail the removal of the device apart from giving the warning?

We can't. device_del() can't fail -- it returns void. Besides, how can a driver hope to deal with an unregistration failure?

Still, our present approach doesn't seem to be correct overall. For example, I think we should prevent a suspend from happening while a device is being removed.

Still, our present approach doesn't seem to be correct overall. For example, I think we should prevent a suspend from happening while a device is being removed.

Quoted 4 lines · Rafael J. Wysocki

On Sunday, 6 of January 2008, Alan Stern wrote:

Still, shouldn't we fail the removal of the device apart from giving the warning?

We can't. device_del() can't fail -- it returns void. Besides, how can a driver hope to deal with an unregistration failure?

Quoted 19 lines · Rafael J. Wysocki

Quoted 3 lines

If you can figure out a way to disable the warning in device_del() for just the one device being unregistered by device_pm_destroy_suspended(),

Something like this, perhaps:

(quoted diff) +9 -0

@@ -905,6 +915,18 @@ void device_del(struct device * dev)
 	struct device * parent = dev->parent;
 	struct class_interface *class_intf;
 
+	if (down_trylock(&dev->sem)) {
+		if (pm_sleep_lock()) {
+			dev_warn(dev, "Illegal %s during suspend\n",
+				__FUNCTION__);
+			dump_stack();
+		} else {
+			pm_sleep_unlock();
+		}
+	} else {

drivers/base/power/main.c void device_pm_remove(struct device *dev

Quoted 6 lines

drivers/base/power/main.c +3 -0

+		up(&dev->sem);
+	}
+
 	if (parent)
 		klist_del(&dev->knode_parent);
 	if (MAJOR(dev->devt))

Bizarre, but it should work.

Be sure to include plenty of explanatory comments

-- otherwise nobody will be able to figure it out! :-)

Quoted 13 lines · Rafael J. Wysocki

Quoted 12 lines · Alan Stern

Quoted 3 lines · Rafael J. Wysocki

Still, even doing that is not enough, since someone can call destroy_suspended_device() from a .suspend() routine and then the device will end up on a wrong list just as well.

That should never happen. The whole idea of destroy_suspended_device() is that the device couldn't be resumed and in fact should be unregistered because it is no longer working or no longer present. A suspend routine won't detect this sort of thing since it doesn't try to resume the device.

Quoted 8 lines

Quoted 3 lines

But it wouldn't hurt to mention in the kerneldoc that destroy_suspended_device() is meant to be called only during a system resume.

Hmm. Please have a look at the appended patch.

I have removed the warning from device_del() and used list_empty() to detect removed devices in the .suspend() routines. Is that viable?

It's not good.

The warning in device_del() is vital. It's what will tell people where the problem is when a deadlock occurs during system resume because some driver has mistakenly tried to unregister a device at the wrong time. It would have pointed immediately to the msr driver in the case of the bug Andrew found, for instance.

If you can figure out a way to disable the warning in device_del() for just the one device being unregistered by device_pm_destroy_suspended(),

I suppose that would be okay.

Quoted 11 lines · Rafael J. Wysocki

Quoted 10 lines · Alan Stern

Quoted 7 lines · Rafael J. Wysocki

Quoted 4 lines

Another thing to watch out for: Just in case somebody ends up calling destroy_suspended_device(dev) from within dev's own resume method, you should interchange the resume_device() and the list_move_tail() calls in dpm_resume().

However, if we unregister them all at once after releasing pm_sleep_rwsem, that shouldn't be necessary, right?

Quoted 6 lines

Quoted 3 lines

It's still necessary, because destroy_suspended_device() still has to move the device from one list to another. You don't want it to end up on the dpm_locked list.

Hmm. That means we'd have to do the same thing in dpm_power_up() in case someone calls destroy_suspended_device() from resume_device_early(dev).

Yes.

Quoted 3 lines

Still, even doing that is not enough, since someone can call destroy_suspended_device() from a .suspend() routine and then the device will end up on a wrong list just as well.

That should never happen. The whole idea of destroy_suspended_device() is that the device couldn't be resumed and in fact should be unregistered because it is no longer working or no longer present. A suspend routine won't detect this sort of thing since it doesn't try to resume the device.

But it wouldn't hurt to mention in the kerneldoc that destroy_suspended_device() is meant to be called only during a system resume.

Quoted 5 lines · Rafael J. Wysocki

Quoted 4 lines

Another thing to watch out for: Just in case somebody ends up calling destroy_suspended_device(dev) from within dev's own resume method, you should interchange the resume_device() and the list_move_tail() calls in dpm_resume().

Quoted 2 lines

However, if we unregister them all at once after releasing pm_sleep_rwsem, that shouldn't be necessary, right?

It's still necessary, because destroy_suspended_device() still has to move the device from one list to another. You don't want it to end up on the dpm_locked list.

Quoted 4 lines

Another thing to watch out for: Just in case somebody ends up calling destroy_suspended_device(dev) from within dev's own resume method, you should interchange the resume_device() and the list_move_tail() calls in dpm_resume().

However, if we unregister them all at once after releasing pm_sleep_rwsem, that shouldn't be necessary, right?

Quoted 7 lines · Rafael J. Wysocki

Greg, Andrew,

The appended patch is a replacement for gregkh-driver-pm-acquire-device-locks-prior-to-suspending.patch that deadlocked suspend and hibernation on some systems.

Please consider for applying.

This warning message:

Quoted 4 lines

drivers/base/core.c +0 -0

@@ -905,6 +915,13 @@ void device_del(struct device * dev)
 	struct device * parent = dev->parent;
 	struct class_interface *class_intf;
 

drivers/base/core.c void device_del(struct device * dev) · line 921

Quoted 4 lines

drivers/base/core.c +4 -0

+	if (pm_sleep_lock()) {
+		dev_warn(dev, "Illegal %s during suspend\n", __FUNCTION__);
+		dump_stack();
+	} else {

will unavoidably be triggered by this code:

Quoted 8 lines

drivers/base/power/main.c +8 -0

+void device_pm_destroy_suspended(struct device *dev)
+{
+	pr_debug("PM: Removing suspended device %s:%s\n",
+		 dev->bus ? dev->bus->name : "No Bus",
+		 kobject_name(&dev->kobj));
+	mutex_lock(&dpm_list_mtx);
+	list_del_init(&dev->power.entry);
+	mutex_unlock(&dpm_list_mtx);

drivers/base/power/main.c void device_pm_remove(struct device *dev · line 97

Quoted 3 lines

drivers/base/power/main.c +3 -0

+	up(&dev->sem);
+	device_unregister(dev);
+}

since the call to device_del() will occur while the pm_sleep_rwsem is still locked for writing. That's why I suggested not unregistering these devices until after everything else has been resumed and the rwsem has been dropped.

Another thing to watch out for: Just in case somebody ends up calling destroy_suspended_device(dev) from within dev's own resume method, you should interchange the resume_device() and the list_move_tail() calls in dpm_resume().