Towards eliminating the freezer

Towards eliminating the freezer

[Alan Stern]

[Note changed $SUBJECT]

On Mon, 23 Jul 2007, Oliver Neukum wrote:

> > > > You are correct about the need to delay/stop device addition.  I don't
> > > > know how this can be done in general; each code path calling
> > > > device_add() may have to be treated individually.
> > > 
> > > What about the old API?
> > 
> > What old API do you mean?
> 
> The find_device() stuff.

You mean like bus_find_device() or driver_find_device()?  I don't see 
any problem with them.  They aren't involved in device registration or 
locking.

> > >  Do we have to block module loading?
> > 
> > No.  Registering new drivers is okay, registering new devices is bad.
> 
> What if it is a driver for virtual devices that don't need probe()
> for actual hardware?

Like I said, registering the new driver is okay.  Registering the 
virtual devices could cause a problem.

> > Of course, some modules do want to register a new device in their init 
> > method.  I don't know what we should do about them.  Force the 
> > registration to fail, I suppose.  How often will people suspend while a 
> > module is loading?
> > 
> > > What happens if a scsi error handler is woken? If it cannot be woken,
> > > how are errors handled?
> > 
> > Why should the error handler wake up?  There isn't supposed to be any 
> > I/O going on, hence no errors to handle.
> 
> What about shared busses? Firewire, FibreChannel? They can get external
> resets, etc ...

The same reasoning applies: If no I/O is going on, why should there be
a reset?  If a reset or any other event is generated externally then it
is handled in the kernel by some device driver for the bus, which
should be smart enough not to register new devices or start up an error
handler until I/O is once again permitted.


=============================


Now here's an idea which might work.  Can we require every caller of
device_add() to hold some existing device's semaphore?  Normally it
would be the semaphore of the new device's parent, but it could be a
higher ancestor.  There even could be a single "root" semaphore for
drivers registering a top-level device with no parent.

(Some testing shows that during startup things like ACPI and IDE don't 
fulfill this requirement, so maybe we should require it only after 
userspace has begun running.  After all, the system can't suspend 
until then.)

It seems like a reasonable sort of thing to do.  Hotplugged devices
tend to be registered as they are discovered by their parent's driver,
so it shouldn't be too much to ask that the parent's semaphore be held
when the new device is registered.  Static devices generally aren't
quite so nice; the serial and floppy drivers in particular would need a
little work (and probably some other drivers too).

If we do this, then once the PM core has acquired the semaphore for 
every device it will be guaranteed that no new devices can be added.  
It would be a simple solution to a rather nasty problem.

Alan Stern

Re: Towards eliminating the freezer

[Oliver Neukum]

Am Montag 23 Juli 2007 schrieb Alan Stern:
> Now here's an idea which might work.  Can we require every caller of
> device_add() to hold some existing device's semaphore?  Normally it
> would be the semaphore of the new device's parent, but it could be a
> higher ancestor.  There even could be a single "root" semaphore for
> drivers registering a top-level device with no parent.

What prevents us from having a device addition semaphore?
Adding device is not critical to performance, is it?

	Regards
		Oliver

Re: Towards eliminating the freezer

[Rafael J. Wysocki]

On Monday, 23 July 2007 22:05, Alan Stern wrote:
> [Note changed $SUBJECT]
[--snip--]
> =============================
> 
> 
> Now here's an idea which might work.  Can we require every caller of
> device_add() to hold some existing device's semaphore?  Normally it
> would be the semaphore of the new device's parent, but it could be a
> higher ancestor.  There even could be a single "root" semaphore for
> drivers registering a top-level device with no parent.
> 
> (Some testing shows that during startup things like ACPI and IDE don't 
> fulfill this requirement, so maybe we should require it only after 
> userspace has begun running.  After all, the system can't suspend 
> until then.)
> 
> It seems like a reasonable sort of thing to do.  Hotplugged devices
> tend to be registered as they are discovered by their parent's driver,
> so it shouldn't be too much to ask that the parent's semaphore be held
> when the new device is registered.  Static devices generally aren't
> quite so nice; the serial and floppy drivers in particular would need a
> little work (and probably some other drivers too).
> 
> If we do this, then once the PM core has acquired the semaphore for 
> every device it will be guaranteed that no new devices can be added.  
> It would be a simple solution to a rather nasty problem.

Hmm, in device_pm_add() and device_pm_remove() we acquire dpm_list_mtx which
also is acquired by device_suspend() and device_resume().  Thus, every attempt
to register a new device or unregister an existing one will be blocked while
either device_suspend() or device_resume() is running.

If we arrange things so that dpm_list_mtx is acquired, but not released, by
device_suspend() and released, but not acquired, by device_resume(), then it
won't be possible to register/unregister a device during a suspend-resume
cycle.

Greetings,
Rafael


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

Re: Towards eliminating the freezer

[Alan Stern]

On Tue, 24 Jul 2007, Oliver Neukum wrote:

> Am Montag 23 Juli 2007 schrieb Alan Stern:
> > Now here's an idea which might work.  Can we require every caller of
> > device_add() to hold some existing device's semaphore?  Normally it
> > would be the semaphore of the new device's parent, but it could be a
> > higher ancestor.  There even could be a single "root" semaphore for
> > drivers registering a top-level device with no parent.
> 
> What prevents us from having a device addition semaphore?
> Adding device is not critical to performance, is it?

It would create a locking order violation.  Many drivers hold a device
semaphore while registering a child device, so they would acquire your
new semaphore while holding a device sem.  But the PM core needs to
prevent registration while calling suspend() methods, so it would need
to acquire the device sems while holding your new semaphore.

Alan Stern

Re: Towards eliminating the freezer

[Alan Stern]

On Tue, 24 Jul 2007, Rafael J. Wysocki wrote:

> > Now here's an idea which might work.  Can we require every caller of
> > device_add() to hold some existing device's semaphore?  Normally it
> > would be the semaphore of the new device's parent, but it could be a
> > higher ancestor.  There even could be a single "root" semaphore for
> > drivers registering a top-level device with no parent.
> > 
> > (Some testing shows that during startup things like ACPI and IDE don't 
> > fulfill this requirement, so maybe we should require it only after 
> > userspace has begun running.  After all, the system can't suspend 
> > until then.)
> > 
> > It seems like a reasonable sort of thing to do.  Hotplugged devices
> > tend to be registered as they are discovered by their parent's driver,
> > so it shouldn't be too much to ask that the parent's semaphore be held
> > when the new device is registered.  Static devices generally aren't
> > quite so nice; the serial and floppy drivers in particular would need a
> > little work (and probably some other drivers too).
> > 
> > If we do this, then once the PM core has acquired the semaphore for 
> > every device it will be guaranteed that no new devices can be added.  
> > It would be a simple solution to a rather nasty problem.
> 
> Hmm, in device_pm_add() and device_pm_remove() we acquire dpm_list_mtx which
> also is acquired by device_suspend() and device_resume().  Thus, every attempt
> to register a new device or unregister an existing one will be blocked while
> either device_suspend() or device_resume() is running.
> 
> If we arrange things so that dpm_list_mtx is acquired, but not released, by
> device_suspend() and released, but not acquired, by device_resume(), then it
> won't be possible to register/unregister a device during a suspend-resume
> cycle.

As with Oliver's suggestion, this would create a locking order 
violation.  Drivers registering children (and thus acquiring 
dpm_list_mtx) will often already hold the parent's sem.  But 
device_suspend() needs to acquire device sems while holding 
dpm_list_mtx.

Alan Stern

Re: Towards eliminating the freezer

[Rafael J. Wysocki]

On Tuesday, 24 July 2007 16:29, Alan Stern wrote:
> On Tue, 24 Jul 2007, Rafael J. Wysocki wrote:
> 
> > > Now here's an idea which might work.  Can we require every caller of
> > > device_add() to hold some existing device's semaphore?  Normally it
> > > would be the semaphore of the new device's parent, but it could be a
> > > higher ancestor.  There even could be a single "root" semaphore for
> > > drivers registering a top-level device with no parent.
> > > 
> > > (Some testing shows that during startup things like ACPI and IDE don't 
> > > fulfill this requirement, so maybe we should require it only after 
> > > userspace has begun running.  After all, the system can't suspend 
> > > until then.)
> > > 
> > > It seems like a reasonable sort of thing to do.  Hotplugged devices
> > > tend to be registered as they are discovered by their parent's driver,
> > > so it shouldn't be too much to ask that the parent's semaphore be held
> > > when the new device is registered.  Static devices generally aren't
> > > quite so nice; the serial and floppy drivers in particular would need a
> > > little work (and probably some other drivers too).
> > > 
> > > If we do this, then once the PM core has acquired the semaphore for 
> > > every device it will be guaranteed that no new devices can be added.  
> > > It would be a simple solution to a rather nasty problem.
> > 
> > Hmm, in device_pm_add() and device_pm_remove() we acquire dpm_list_mtx which
> > also is acquired by device_suspend() and device_resume().  Thus, every attempt
> > to register a new device or unregister an existing one will be blocked while
> > either device_suspend() or device_resume() is running.
> > 
> > If we arrange things so that dpm_list_mtx is acquired, but not released, by
> > device_suspend() and released, but not acquired, by device_resume(), then it
> > won't be possible to register/unregister a device during a suspend-resume
> > cycle.
> 
> As with Oliver's suggestion, this would create a locking order 
> violation.  Drivers registering children (and thus acquiring 
> dpm_list_mtx) will often already hold the parent's sem.  But 
> device_suspend() needs to acquire device sems while holding 
> dpm_list_mtx.

Hmm, but this is done already (ie. device_suspend() acquires device sems
while holding dpm_list_mtx in the current code).

What I'm suggesting is not to let device_suspend() release dpm_list_mtx
when it's finished.  The appended patch illustrates that I mean.

Greetings,
Rafael


---
dpm_list_mtx is used by the PM core to prevent device objects from being
added/removed while device_suspend() and device_resume() are running.  However,
it should also be impossible to add a device after device_suspend() has
finished, because at that time the dpm_active list is empty and adding new
devices to it would break the ordering of devices during the next suspend.
Thus, it seems reasonable to leave device_suspend() with dpm_list_mtx held
and release at the end of device_resume().

In that case device_suspend() and device_resume() cannot be run concurrently
and dpm_mtx is no longer needed.  Also, it's a bug to run device_resume() after
a failing device_suspend(), so the APM code needs to be updated.

---
 arch/i386/kernel/apm.c       |    5 ++++-
 drivers/base/power/main.c    |    1 -
 drivers/base/power/power.h   |    5 -----
 drivers/base/power/resume.c  |    3 ---
 drivers/base/power/suspend.c |    3 ---
 5 files changed, 4 insertions(+), 13 deletions(-)

Index: linux-2.6.23-rc1/arch/i386/kernel/apm.c
===================================================================
--- linux-2.6.23-rc1.orig/arch/i386/kernel/apm.c	2007-07-23 22:28:35.000000000 +0200
+++ linux-2.6.23-rc1/arch/i386/kernel/apm.c	2007-07-24 11:04:45.000000000 +0200
@@ -1202,7 +1202,9 @@ static int suspend(int vetoable)
 		printk(KERN_CRIT "apm: suspend was vetoed, but suspending anyway.\n");
 	}
 
-	device_suspend(PMSG_SUSPEND);
+	err = device_suspend(PMSG_SUSPEND);
+	if (err)
+		goto send_resume;
 	local_irq_disable();
 	device_power_down(PMSG_SUSPEND);
 
@@ -1224,6 +1226,7 @@ static int suspend(int vetoable)
 	device_power_up();
 	local_irq_enable();
 	device_resume();
+ send_resume:
 	pm_send_all(PM_RESUME, (void *)0);
 	queue_event(APM_NORMAL_RESUME, NULL);
  out:
Index: linux-2.6.23-rc1/drivers/base/power/resume.c
===================================================================
--- linux-2.6.23-rc1.orig/drivers/base/power/resume.c	2007-07-23 22:06:42.000000000 +0200
+++ linux-2.6.23-rc1/drivers/base/power/resume.c	2007-07-24 11:18:04.000000000 +0200
@@ -72,7 +72,6 @@ static int resume_device_early(struct de
  */
 void dpm_resume(void)
 {
-	mutex_lock(&dpm_list_mtx);
 	while(!list_empty(&dpm_off)) {
 		struct list_head * entry = dpm_off.next;
 		struct device * dev = to_device(entry);
@@ -99,9 +98,7 @@ void dpm_resume(void)
 void device_resume(void)
 {
 	might_sleep();
-	mutex_lock(&dpm_mtx);
 	dpm_resume();
-	mutex_unlock(&dpm_mtx);
 }
 
 EXPORT_SYMBOL_GPL(device_resume);
Index: linux-2.6.23-rc1/drivers/base/power/suspend.c
===================================================================
--- linux-2.6.23-rc1.orig/drivers/base/power/suspend.c	2007-07-23 22:06:42.000000000 +0200
+++ linux-2.6.23-rc1/drivers/base/power/suspend.c	2007-07-24 11:17:52.000000000 +0200
@@ -127,7 +127,6 @@ int device_suspend(pm_message_t state)
 	int error = 0;
 
 	might_sleep();
-	mutex_lock(&dpm_mtx);
 	mutex_lock(&dpm_list_mtx);
 	while (!list_empty(&dpm_active) && error == 0) {
 		struct list_head * entry = dpm_active.prev;
@@ -153,11 +152,9 @@ int device_suspend(pm_message_t state)
 				error == -EAGAIN ? " (please convert to suspend_late)" : "");
 		put_device(dev);
 	}
-	mutex_unlock(&dpm_list_mtx);
 	if (error)
 		dpm_resume();
 
-	mutex_unlock(&dpm_mtx);
 	return error;
 }
 
Index: linux-2.6.23-rc1/drivers/base/power/main.c
===================================================================
--- linux-2.6.23-rc1.orig/drivers/base/power/main.c	2007-07-23 22:06:42.000000000 +0200
+++ linux-2.6.23-rc1/drivers/base/power/main.c	2007-07-24 11:17:16.000000000 +0200
@@ -28,7 +28,6 @@ LIST_HEAD(dpm_active);
 LIST_HEAD(dpm_off);
 LIST_HEAD(dpm_off_irq);
 
-DEFINE_MUTEX(dpm_mtx);
 DEFINE_MUTEX(dpm_list_mtx);
 
 int (*platform_enable_wakeup)(struct device *dev, int is_on);
Index: linux-2.6.23-rc1/drivers/base/power/power.h
===================================================================
--- linux-2.6.23-rc1.orig/drivers/base/power/power.h	2007-07-23 22:06:42.000000000 +0200
+++ linux-2.6.23-rc1/drivers/base/power/power.h	2007-07-24 11:17:33.000000000 +0200
@@ -12,11 +12,6 @@ extern void device_shutdown(void);
  */
 
 /*
- * Used to synchronize global power management operations.
- */
-extern struct mutex dpm_mtx;
-
-/*
  * Used to serialize changes to the dpm_* lists.
  */
 extern struct mutex dpm_list_mtx;

Re: Towards eliminating the freezer

[Alan Stern]

On Tue, 24 Jul 2007, Rafael J. Wysocki wrote:

> > As with Oliver's suggestion, this would create a locking order 
> > violation.  Drivers registering children (and thus acquiring 
> > dpm_list_mtx) will often already hold the parent's sem.  But 
> > device_suspend() needs to acquire device sems while holding 
> > dpm_list_mtx.
> 
> Hmm, but this is done already (ie. device_suspend() acquires device sems
> while holding dpm_list_mtx in the current code).
> 
> What I'm suggesting is not to let device_suspend() release dpm_list_mtx
> when it's finished.  The appended patch illustrates that I mean.

Oh, okay, I see what you mean.

I should have explained earlier that my proposal was meant to be in the 
context of a previous discussion, where I suggested that 
device_suspend() should go through a preliminary step of acquiring all 
the device semaphores.  This would have the beneficial effect of 
blocking all attempts at driver binding or unbinding while a suspend is 
underway.

Still, this isn't a bad approach.  Maybe the following algorithm could 
be used:

 get_more:
	For each device on dpm_list
		Acquire dev->sem
		Move dev from dpm_list to a temporary list
	Lock dpm_list_mutex
	If (!list_empty(dpm_list)) {
		Unlock dpm_list_mutex
		Goto get_more
	}

(The "For each" loop would have to be written carefully to allow for 
device removal.)

The total number of iterations should never be large.  At the end the 
PM core would own all the device semaphores and no more devices could 
be added.  Then it would be safe to call each device's suspend() 
method.

This will remove one of the barriers to eliminating the freezer.

Alan Stern

Re: Towards eliminating the freezer

[Rafael J. Wysocki]

On Tuesday, 24 July 2007 18:06, Alan Stern wrote:
> On Tue, 24 Jul 2007, Rafael J. Wysocki wrote:
> 
> > > As with Oliver's suggestion, this would create a locking order 
> > > violation.  Drivers registering children (and thus acquiring 
> > > dpm_list_mtx) will often already hold the parent's sem.  But 
> > > device_suspend() needs to acquire device sems while holding 
> > > dpm_list_mtx.
> > 
> > Hmm, but this is done already (ie. device_suspend() acquires device sems
> > while holding dpm_list_mtx in the current code).
> > 
> > What I'm suggesting is not to let device_suspend() release dpm_list_mtx
> > when it's finished.  The appended patch illustrates that I mean.
> 
> Oh, okay, I see what you mean.
> 
> I should have explained earlier that my proposal was meant to be in the 
> context of a previous discussion, where I suggested that 
> device_suspend() should go through a preliminary step of acquiring all 
> the device semaphores.  This would have the beneficial effect of 
> blocking all attempts at driver binding or unbinding while a suspend is 
> underway.
> 
> Still, this isn't a bad approach.  Maybe the following algorithm could 
> be used:
> 
>  get_more:
> 	For each device on dpm_list
> 		Acquire dev->sem
> 		Move dev from dpm_list to a temporary list
> 	Lock dpm_list_mutex
> 	If (!list_empty(dpm_list)) {
> 		Unlock dpm_list_mutex
> 		Goto get_more
> 	}
> 
> (The "For each" loop would have to be written carefully to allow for 
> device removal.)

Hmm, I still don't understand why we can't lock dpm_list_mutex before the
"For each" loop (we already do something like this in device_suspend() and
device_resume()) and that would simplify things.

It seems that we can do something like this:

device_suspend:
	Lock dpm_list_mutex (from now on, new devices cannot be added)
	For each device on dpm_active, reverse
		acquire dev->sem (from now on, no new drivers can bind to dev)
		suspend(dev)
		move dev to dpm_off

device_resume:
	For each device on dpm_off
		move dev to dpm_active
		resume(dev) (this cannot fail)
		release dev->sem (allow new drivers to bind to dev)
	Unlock dpm_list_mutex (allow new devices to be added)

Greetings,
Rafael


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

Re: Towards eliminating the freezer

[Alan Stern]

On Tue, 24 Jul 2007, Rafael J. Wysocki wrote:

> Hmm, I still don't understand why we can't lock dpm_list_mutex before the
> "For each" loop (we already do something like this in device_suspend() and
> device_resume()) and that would simplify things.
> 
> It seems that we can do something like this:
> 
> device_suspend:
> 	Lock dpm_list_mutex (from now on, new devices cannot be added)
> 	For each device on dpm_active, reverse
> 		acquire dev->sem (from now on, no new drivers can bind to dev)
> 		suspend(dev)
> 		move dev to dpm_off

You have a minor error there; it's necessary to unlock dpm_list_mutex 
while acquiring dev-sem and then lock it again.  But more importantly, 
this code acquires the device semaphores in the wrong order.  They have 
to be acquired going forward (from the top of the device tree down), 
not backward.


Here's my proposal in a more explicit form.  Before doing
device_suspend() we call lock_all_devices():

struct list_head dpm_locked;

static void lock_all_devices()
{
	mutex_lock(&dpm_list_mtx);
	while (!list_empty(&dpm_active)) {
		struct list_head *entry = dpm_active.next;
		struct device *dev = to_device(entry);

		get_device(dev);
		mutex_unlock(&dpm_list_mtx);
		down(&dev->sem);
		mutex_lock(&dpm_list_mtx);

		if (list_empty(entry))		/* Device was removed */
			up(&dev->sem);
		else			/* Move it to the dpm_locked list */
			list_move_tail(entry, &dpm_locked);
		put_device(dev);
	}
}

Then device_suspend() can be simplified:

int device_suspend(pm_message_t state)
{
	int error = 0;

	might_sleep();
	list_for_each_entry_reverse(dev, &dpm_locked, power.entry) {
		error = suspend_device(dev, state);

		if (error) {
			printk(KERN_ERR "Could not suspend device %s: "
				"error %d%s\n",
				kobject_name(&dev->kobj), error,
				error == -EAGAIN ? " (please convert to suspend_late)" : "");
			break;
		}
		list_move(&dev->power.entry, &dpm_off);
	}
	if (error)
		dpm_resume();
	return error;
}

Appropriate changes are needed in the resume pathway as well, together 
with an unlock_all_devices() routine:

static void unlock_all_devices(void)
{
	while (!list_empty(&dpm_locked)) {
		struct list_head *entry = dpm_locked.prev;
		struct device *dev = to_device(entry);

		list_move(entry, &dpm_active);
		up(&dev->sem);
	}
	mutex_unlock(&dpm_list_mtx);
}


Incidentally, what is dpm_mtx for?  It doesn't seem to do anything 
useful.  Is it a relic of the former runtime PM support?

Alan Stern

Re: Towards eliminating the freezer

[Rafael J. Wysocki]

On Tuesday, 24 July 2007 22:24, Alan Stern wrote:
> On Tue, 24 Jul 2007, Rafael J. Wysocki wrote:
> 
> > Hmm, I still don't understand why we can't lock dpm_list_mutex before the
> > "For each" loop (we already do something like this in device_suspend() and
> > device_resume()) and that would simplify things.
> > 
> > It seems that we can do something like this:
> > 
> > device_suspend:
> > 	Lock dpm_list_mutex (from now on, new devices cannot be added)
> > 	For each device on dpm_active, reverse
> > 		acquire dev->sem (from now on, no new drivers can bind to dev)
> > 		suspend(dev)
> > 		move dev to dpm_off
> 
> You have a minor error there; it's necessary to unlock dpm_list_mutex 
> while acquiring dev-sem and then lock it again.

Ah, right, now I see that.

> But more importantly, this code acquires the device semaphores in the wrong
> order.  They have to be acquired going forward (from the top of the device
> tree down), not backward.

Yes, I've overlooked that too.

> Here's my proposal in a more explicit form.  Before doing
> device_suspend() we call lock_all_devices():
> 
> struct list_head dpm_locked;
> 
> static void lock_all_devices()
> {
> 	mutex_lock(&dpm_list_mtx);
> 	while (!list_empty(&dpm_active)) {
> 		struct list_head *entry = dpm_active.next;
> 		struct device *dev = to_device(entry);
> 
> 		get_device(dev);
> 		mutex_unlock(&dpm_list_mtx);
> 		down(&dev->sem);
> 		mutex_lock(&dpm_list_mtx);
> 
> 		if (list_empty(entry))		/* Device was removed */
> 			up(&dev->sem);
> 		else			/* Move it to the dpm_locked list */
> 			list_move_tail(entry, &dpm_locked);
> 		put_device(dev);
> 	}
> }
> 
> Then device_suspend() can be simplified:
> 
> int device_suspend(pm_message_t state)
> {
> 	int error = 0;
> 
> 	might_sleep();
> 	list_for_each_entry_reverse(dev, &dpm_locked, power.entry) {
> 		error = suspend_device(dev, state);
> 
> 		if (error) {
> 			printk(KERN_ERR "Could not suspend device %s: "
> 				"error %d%s\n",
> 				kobject_name(&dev->kobj), error,
> 				error == -EAGAIN ? " (please convert to suspend_late)" : "");
> 			break;
> 		}
> 		list_move(&dev->power.entry, &dpm_off);

Is that safe with list_for_each_entry_reverse?

> 	}
> 	if (error)
> 		dpm_resume();
> 	return error;
> }
> 
> Appropriate changes are needed in the resume pathway as well, together 
> with an unlock_all_devices() routine:

Sure.

> static void unlock_all_devices(void)
> {
> 	while (!list_empty(&dpm_locked)) {
> 		struct list_head *entry = dpm_locked.prev;
> 		struct device *dev = to_device(entry);
> 
> 		list_move(entry, &dpm_active);
> 		up(&dev->sem);
> 	}
> 	mutex_unlock(&dpm_list_mtx);
> }

Yes, that looks fine. 

So, who's writing the patch? ;-)

> Incidentally, what is dpm_mtx for?  It doesn't seem to do anything 
> useful.  Is it a relic of the former runtime PM support?

I think so.  IMO it can be removed.

I also think it would be nicer to have all of the functions in
drivers/base/power/{main|suspend|resume}.c moved to one file.

Greetings,
Rafael


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

Re: Towards eliminating the freezer

[Alan Stern]

On Tue, 24 Jul 2007, Rafael J. Wysocki wrote:

> > Then device_suspend() can be simplified:
> > 
> > int device_suspend(pm_message_t state)
> > {
> > 	int error = 0;
> > 
> > 	might_sleep();
> > 	list_for_each_entry_reverse(dev, &dpm_locked, power.entry) {
> > 		error = suspend_device(dev, state);
> > 
> > 		if (error) {
> > 			printk(KERN_ERR "Could not suspend device %s: "
> > 				"error %d%s\n",
> > 				kobject_name(&dev->kobj), error,
> > 				error == -EAGAIN ? " (please convert to suspend_late)" : "");
> > 			break;
> > 		}
> > 		list_move(&dev->power.entry, &dpm_off);
> 
> Is that safe with list_for_each_entry_reverse?

No.  I guess it'll have to resemble the other code.

> Yes, that looks fine. 
> 
> So, who's writing the patch? ;-)

I can do it.  You haven't made any changes to this part of the code, 
have you?  My work tends to be based on Linus's tree, not -mm.

Something to watch out for: With all the extra locking, we run the risk
of blocking the keventd workqueue.  This may or may not matter, but to
be safe perhaps there should be a new general-purpose workqueue which
_expects_ to block (or freeze) during suspends.  Any work routine that 
involves adding or removing a device should go on the new workqueue.

> > Incidentally, what is dpm_mtx for?  It doesn't seem to do anything 
> > useful.  Is it a relic of the former runtime PM support?
> 
> I think so.  IMO it can be removed.
> 
> I also think it would be nicer to have all of the functions in
> drivers/base/power/{main|suspend|resume}.c moved to one file.

Yes, they are all similar enough that there isn't much point keeping 
them separate.

Alan Stern

Re: Towards eliminating the freezer

[Rafael J. Wysocki]

On Wednesday, 25 July 2007 00:14, Alan Stern wrote:
> On Tue, 24 Jul 2007, Rafael J. Wysocki wrote:
> 
> > > Then device_suspend() can be simplified:
> > > 
> > > int device_suspend(pm_message_t state)
> > > {
> > > 	int error = 0;
> > > 
> > > 	might_sleep();
> > > 	list_for_each_entry_reverse(dev, &dpm_locked, power.entry) {
> > > 		error = suspend_device(dev, state);
> > > 
> > > 		if (error) {
> > > 			printk(KERN_ERR "Could not suspend device %s: "
> > > 				"error %d%s\n",
> > > 				kobject_name(&dev->kobj), error,
> > > 				error == -EAGAIN ? " (please convert to suspend_late)" : "");
> > > 			break;
> > > 		}
> > > 		list_move(&dev->power.entry, &dpm_off);
> > 
> > Is that safe with list_for_each_entry_reverse?
> 
> No.  I guess it'll have to resemble the other code.
> 
> > Yes, that looks fine. 
> > 
> > So, who's writing the patch? ;-)
> 
> I can do it.  You haven't made any changes to this part of the code, 
> have you?

Yes, I have, quite recently. :-)

> My work tends to be based on Linus's tree, not -mm. 

At the moment they are pretty much in line, at least as far as this code is
concerned.  Anyway, I'm trying to keep track of PM-related patches,
at http://www.sisk.pl/kernel/hibernation_and_suspend/2.6.23-rc1/

> Something to watch out for: With all the extra locking, we run the risk
> of blocking the keventd workqueue.  This may or may not matter, but to
> be safe perhaps there should be a new general-purpose workqueue which
> _expects_ to block (or freeze) during suspends.  Any work routine that 
> involves adding or removing a device should go on the new workqueue.

Yes, this sounds like a good idea.  Still, I think we can check if there are
problems with the keventd workqueue alone, first.

> > > Incidentally, what is dpm_mtx for?  It doesn't seem to do anything 
> > > useful.  Is it a relic of the former runtime PM support?
> > 
> > I think so.  IMO it can be removed.
> > 
> > I also think it would be nicer to have all of the functions in
> > drivers/base/power/{main|suspend|resume}.c moved to one file.
> 
> Yes, they are all similar enough that there isn't much point keeping 
> them separate.

Plus some variables might be made static, like dpm_off or even dpm_list_mtx.

Greetings,
Rafael


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

Re: Towards eliminating the freezer

[Alan Stern]

On Wed, 25 Jul 2007, Rafael J. Wysocki wrote:

> > > So, who's writing the patch? ;-)
> > 
> > I can do it.  You haven't made any changes to this part of the code, 
> > have you?
> 
> Yes, I have, quite recently. :-)
> 
> > My work tends to be based on Linus's tree, not -mm. 
> 
> At the moment they are pretty much in line, at least as far as this code is
> concerned.  Anyway, I'm trying to keep track of PM-related patches,
> at http://www.sisk.pl/kernel/hibernation_and_suspend/2.6.23-rc1/

I got your snapshot file.  None of the patches in it modify anything
in drivers/base/power/*, so there shouldn't be any interference.

By the way, just checking: Apparently when device_power_down() in 
suspend.c calls sysdev_suspend(), if there's an error it doesn't then 
call dpm_power_up().  Is it correct to assume this is a bug?

Alan Stern

Re: Towards eliminating the freezer

[Rafael J. Wysocki]

On Thursday, 26 July 2007 21:32, Alan Stern wrote:
> On Wed, 25 Jul 2007, Rafael J. Wysocki wrote:
> 
> > > > So, who's writing the patch? ;-)
> > > 
> > > I can do it.  You haven't made any changes to this part of the code, 
> > > have you?
> > 
> > Yes, I have, quite recently. :-)
> > 
> > > My work tends to be based on Linus's tree, not -mm. 
> > 
> > At the moment they are pretty much in line, at least as far as this code is
> > concerned.  Anyway, I'm trying to keep track of PM-related patches,
> > at http://www.sisk.pl/kernel/hibernation_and_suspend/2.6.23-rc1/
> 
> I got your snapshot file.  None of the patches in it modify anything
> in drivers/base/power/*, so there shouldn't be any interference.

I think that's correct.

> By the way, just checking: Apparently when device_power_down() in 
> suspend.c calls sysdev_suspend(), if there's an error it doesn't then 
> call dpm_power_up().  Is it correct to assume this is a bug?

Yes.

Greetings,
Rafael


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