PM: Asynchronous suspend and resume

The patch below introduces a mechanism allowing some devices to be resumed asynchronously, using completions with the following rules:
(1) There is a completion, dev->power.comp, for each device object.
(2) All of these completions are reset before suspend as well as each resume stage (dpm_resume_noirq(), dpm_resume()).
(3) If dev->power.async_suspend is set for dev or for its parent, the PM core waits for the parent's completion before attempting to run the resume callbacks, appropriate for this particular stage of resume, for dev.

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

 #ifdef CONFIG_PM_SLEEP
 	struct list_head	entry;
+	struct completion	comp;
+	pm_message_t		async_state;
 #endif
 }
 
+static inline void device_enable_async_suspend(struct device *dev, bool enable)
+{
+	if (dev->power.status == DPM_ON)
+		dev->power.async_suspend = enable;
+}
+

@@ -163,6 +166,34 @@ void device_pm_move_last(struct device *
 	list_move_tail(&dev->power.entry, &dpm_list);
 }
 
+static void dpm_synchronize_noirq(void)
+{
+	struct device *dev;
+
+	async_synchronize_full();
+
+	list_for_each_entry(dev, &dpm_list, power.entry)
+		INIT_COMPLETION(dev->power.comp);
+}
+
+static void dpm_synchronize(void)

The patch below introduces a mechanism allowing some devices to be resumed asynchronously, using completions with the following rules:
(1) There is a completion, dev->power.comp, for each device object.
(2) All of these completions are reset before suspend as well as each resume stage (dpm_resume_noirq(), dpm_resume()).
(3) If dev->power.async_suspend is set for dev or for its parent, the PM core waits for the parent's completion before attempting to run the resume callbacks, appropriate for this particular stage of resume, for dev.

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

 #ifdef CONFIG_PM_SLEEP
 	struct list_head	entry;
+	struct completion	comp;
+	pm_message_t		async_state;
 #endif
 }
 
+static inline void device_enable_async_suspend(struct device *dev, bool enable)
+{
+	if (dev->power.status == DPM_ON)
+		dev->power.async_suspend = enable;
+}
+

@@ -163,6 +166,34 @@ void device_pm_move_last(struct device *
 	list_move_tail(&dev->power.entry, &dpm_list);
 }
 
+static void dpm_synchronize_noirq(void)
+{
+	struct device *dev;
+
+	async_synchronize_full();
+
+	list_for_each_entry(dev, &dpm_list, power.entry)
+		INIT_COMPLETION(dev->power.comp);
+}
+
+static void dpm_synchronize(void)

Quoted 9 lines

The patch below introduces a mechanism allowing some devices to be resumed asynchronously, using completions with the following rules:
(1) There is a completion, dev->power.comp, for each device object.
(2) All of these completions are reset before suspend as well as each resume stage (dpm_resume_noirq(), dpm_resume()).
(3) If dev->power.async_suspend is set for dev or for its parent, the PM core waits for the parent's completion before attempting to run the resume callbacks, appropriate for this particular stage of resume, for dev.

at least this needs to go in as a comment.

include/linux/pm.h struct dev_pm_info { · line 416

Quoted 7 lines

(4) dev->power.comp is completed for each device after running its

include/linux/pm.h +1 -0

@@ -411,9 +412,12 @@ struct dev_pm_info {
 	pm_message_t		power_state;
 	unsigned int		can_wakeup:1;
 	unsigned int		should_wakeup:1;
+	unsigned		async_suspend:1;
 	enum dpm_state		status;		/* Owned by the PM core */

unsigned int? Or bool?

Should it go under config_pm_sleep?

Quoted 32 lines

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

 #ifdef CONFIG_PM_SLEEP
 	struct list_head	entry;
+	struct completion	comp;
+	pm_message_t		async_state;
 #endif
 }
 
+static inline void device_enable_async_suspend(struct device *dev, bool enable)
+{
+	if (dev->power.status == DPM_ON)
+		dev->power.async_suspend = enable;
+}
+

@@ -163,6 +166,34 @@ void device_pm_move_last(struct device *
 	list_move_tail(&dev->power.entry, &dpm_list);
 }
 
+static void dpm_synchronize_noirq(void)
+{
+	struct device *dev;
+
+	async_synchronize_full();
+
+	list_for_each_entry(dev, &dpm_list, power.entry)
+		INIT_COMPLETION(dev->power.comp);
+}
+
+static void dpm_synchronize(void)
+{
+	struct device *dev;
+
+	async_synchronize_full();

drivers/base/power/main.c void device_pm_move_last(struct device * · line 189

Quoted 6 lines

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

+
+	mutex_lock(&dpm_list_mtx);
+	list_for_each_entry(dev, &dpm_list, power.entry)
+		INIT_COMPLETION(dev->power.comp);
+	mutex_unlock(&dpm_list_mtx);
+}

Why is it ok to avoid locking in noirq case?

Do we really need async for noirq handlers?

drivers/base/power/main.c static void pm_dev_err(struct device *de · line 369

Quoted 12 lines

drivers/base/power/main.c +5 -4

 /**
- *	device_resume_noirq - Power on one device (early resume).
- *	@dev:	Device.
- *	@state: PM transition of the system being carried out.
+ * __device_resume_noirq - Execute an "early resume" callback for given device.
+ * @dev: Device to resume.
+ * @state: PM transition of the system being carried out.
  *
- *	Must be called with interrupts disabled.
+ * The driver of the device won't receive interrupts while this function is
+ * being executed.
  */

You still want it called with interrupts disabled, right?

Quoted 1 line

Do we really need async for noirq handlers?

Yes, we do. Specifically, for PCI.

Hi!

Quoted 3 lines

Quoted 1 line

Do we really need async for noirq handlers?

Yes, we do. Specifically, for PCI.

Do noirq parts of PCI handling really so long that we need to make them async?

Async in normal paths looks pretty much okay to me, but in noirq parts, locking is tricky, etc...

Hi!

Quoted 3 lines

Quoted 1 line

Do we really need async for noirq handlers?

Yes, we do. Specifically, for PCI.

Do noirq parts of PCI handling really so long that we need to make them async?

Async in normal paths looks pretty much okay to me, but in noirq parts, locking is tricky, etc...

Quoted 1 line

Do we really need async for noirq handlers?

Yes, we do. Specifically, for PCI.

Quoted 9 lines

The patch below introduces a mechanism allowing some devices to be resumed asynchronously, using completions with the following rules:
(1) There is a completion, dev->power.comp, for each device object.
(2) All of these completions are reset before suspend as well as each resume stage (dpm_resume_noirq(), dpm_resume()).
(3) If dev->power.async_suspend is set for dev or for its parent, the PM core waits for the parent's completion before attempting to run the resume callbacks, appropriate for this particular stage of resume, for dev.

at least this needs to go in as a comment.

include/linux/pm.h struct dev_pm_info { · line 416

Quoted 7 lines

(4) dev->power.comp is completed for each device after running its

include/linux/pm.h +1 -0

@@ -411,9 +412,12 @@ struct dev_pm_info {
 	pm_message_t		power_state;
 	unsigned int		can_wakeup:1;
 	unsigned int		should_wakeup:1;
+	unsigned		async_suspend:1;
 	enum dpm_state		status;		/* Owned by the PM core */

unsigned int? Or bool?

Should it go under config_pm_sleep?

Quoted 32 lines

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

 #ifdef CONFIG_PM_SLEEP
 	struct list_head	entry;
+	struct completion	comp;
+	pm_message_t		async_state;
 #endif
 }
 
+static inline void device_enable_async_suspend(struct device *dev, bool enable)
+{
+	if (dev->power.status == DPM_ON)
+		dev->power.async_suspend = enable;
+}
+

@@ -163,6 +166,34 @@ void device_pm_move_last(struct device *
 	list_move_tail(&dev->power.entry, &dpm_list);
 }
 
+static void dpm_synchronize_noirq(void)
+{
+	struct device *dev;
+
+	async_synchronize_full();
+
+	list_for_each_entry(dev, &dpm_list, power.entry)
+		INIT_COMPLETION(dev->power.comp);
+}
+
+static void dpm_synchronize(void)
+{
+	struct device *dev;
+
+	async_synchronize_full();

drivers/base/power/main.c void device_pm_move_last(struct device * · line 189

Quoted 6 lines

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

+
+	mutex_lock(&dpm_list_mtx);
+	list_for_each_entry(dev, &dpm_list, power.entry)
+		INIT_COMPLETION(dev->power.comp);
+	mutex_unlock(&dpm_list_mtx);
+}

Why is it ok to avoid locking in noirq case?

Do we really need async for noirq handlers?

drivers/base/power/main.c static void pm_dev_err(struct device *de · line 369

Quoted 12 lines

drivers/base/power/main.c +5 -4

 /**
- *	device_resume_noirq - Power on one device (early resume).
- *	@dev:	Device.
- *	@state: PM transition of the system being carried out.
+ * __device_resume_noirq - Execute an "early resume" callback for given device.
+ * @dev: Device to resume.
+ * @state: PM transition of the system being carried out.
  *
- *	Must be called with interrupts disabled.
+ * The driver of the device won't receive interrupts while this function is
+ * being executed.
  */

You still want it called with interrupts disabled, right?

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

+ * The driver of the device won't receive interrupts while this function is
+ * being executed.
  */

@@ -696,13 +746,19 @@ int dpm_suspend_noirq(pm_message_t state
 	suspend_device_irqs();
 	mutex_lock(&dpm_list_mtx);
 	list_for_each_entry_reverse(dev, &dpm_list, power.entry) {

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

+ * The driver of the device won't receive interrupts while this function is
+ * being executed.
  */

@@ -696,13 +746,19 @@ int dpm_suspend_noirq(pm_message_t state
 	suspend_device_irqs();
 	mutex_lock(&dpm_list_mtx);
 	list_for_each_entry_reverse(dev, &dpm_list, power.entry) {

Quoted 10 lines

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

@@ -659,26 +674,61 @@ static pm_message_t resume_event(pm_mess
 }
 
 /**
- *	device_suspend_noirq - Shut down one device (late suspend).
- *	@dev:	Device.
- *	@state: PM transition of the system being carried out.
+ * __device_suspend_noirq - Execute a "late" suspend callback for given device.
+ * @dev: Device to suspend.
+ * @state: PM transition of the system being carried out.

drivers/base/power/main.c static pm_message_t resume_event(pm_mess · line 666

Quoted 2 lines

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

  *
- *	This is called with interrupts off and only a single CPU running.

that still looks like useful comment... why delete it?

Quoted 11 lines

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

+ * The driver of the device won't receive interrupts while this function is
+ * being executed.
  */

@@ -696,13 +746,19 @@ int dpm_suspend_noirq(pm_message_t state
 	suspend_device_irqs();
 	mutex_lock(&dpm_list_mtx);
 	list_for_each_entry_reverse(dev, &dpm_list, power.entry) {
+		dev->power.status = DPM_OFF_IRQ;
 		error = device_suspend_noirq(dev, state);
 		if (error) {
 			pm_dev_err(dev, state, " late", error);

drivers/base/power/main.c int dpm_suspend_noirq(pm_message_t state · line 758

Quoted 6 lines

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

+			dev->power.status = DPM_OFF;
+			break;
+		}
+		if (async_error) {
+			error = async_error;
 			break;

async_error is 'interesting'. How does locking work in noirq case?

Quoted 11 lines

Quoted 11 lines

+ * The driver of the device won't receive interrupts while this function is
+ * being executed.
*/

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

@@ -696,13 +746,19 @@ int dpm_suspend_noirq(pm_message_t state
 	suspend_device_irqs();
 	mutex_lock(&dpm_list_mtx);
 	list_for_each_entry_reverse(dev, &dpm_list, power.entry) {
+		dev->power.status = DPM_OFF_IRQ;
 		error = device_suspend_noirq(dev, state);
 		if (error) {
 			pm_dev_err(dev, state, " late", error);

Quoted 8 lines

drivers/base/power/main.c int dpm_suspend_noirq(pm_message_t state · line 758

Quoted 6 lines

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

+			dev->power.status = DPM_OFF;
+			break;
+		}
+		if (async_error) {
+			error = async_error;
 			break;

async_error is 'interesting'. How does locking work in noirq case?

It's racy, a little bit. :-)

If two async drivers return errors exactly at the same time, one of them will win the race, but it doesn't really matter which one wins as long as async_error is different from zero as a result. And it will be, since it's an 'int' and the integrity of these is guaranteed.

Quoted 11 lines

Quoted 11 lines

+ * The driver of the device won't receive interrupts while this function is
+ * being executed.
*/

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

@@ -696,13 +746,19 @@ int dpm_suspend_noirq(pm_message_t state
 	suspend_device_irqs();
 	mutex_lock(&dpm_list_mtx);
 	list_for_each_entry_reverse(dev, &dpm_list, power.entry) {
+		dev->power.status = DPM_OFF_IRQ;
 		error = device_suspend_noirq(dev, state);
 		if (error) {
 			pm_dev_err(dev, state, " late", error);

Quoted 8 lines

drivers/base/power/main.c int dpm_suspend_noirq(pm_message_t state · line 758

Quoted 6 lines

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

+			dev->power.status = DPM_OFF;
+			break;
+		}
+		if (async_error) {
+			error = async_error;
 			break;

async_error is 'interesting'. How does locking work in noirq case?

It's racy, a little bit. :-)

If two async drivers return errors exactly at the same time, one of them will win the race, but it doesn't really matter which one wins as long as async_error is different from zero as a result. And it will be, since it's an 'int' and the integrity of these is guaranteed.

Quoted 20 lines

Quoted 11 lines

Quoted 11 lines

+ * The driver of the device won't receive interrupts while this function is
+ * being executed.
*/

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

@@ -696,13 +746,19 @@ int dpm_suspend_noirq(pm_message_t state
 	suspend_device_irqs();
 	mutex_lock(&dpm_list_mtx);
 	list_for_each_entry_reverse(dev, &dpm_list, power.entry) {
+		dev->power.status = DPM_OFF_IRQ;
 		error = device_suspend_noirq(dev, state);
 		if (error) {
 			pm_dev_err(dev, state, " late", error);

Quoted 8 lines

drivers/base/power/main.c int dpm_suspend_noirq(pm_message_t state · line 758

Quoted 6 lines

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

+			dev->power.status = DPM_OFF;
+			break;
+		}
+		if (async_error) {
+			error = async_error;
 			break;

async_error is 'interesting'. How does locking work in noirq case?

Quoted 6 lines

It's racy, a little bit. :-)

If two async drivers return errors exactly at the same time, one of them will win the race, but it doesn't really matter which one wins as long as async_error is different from zero as a result. And it will be, since it's an 'int' and the integrity of these is guaranteed.

Rather than relying on atomicity of 'int' (where half of kernel hackers says it is and second half says it is not), can we just use atomic_t? It compiles to same code on sane architectures, and serves as documentation/warning...

Quoted 20 lines

Quoted 11 lines

Quoted 11 lines

+ * The driver of the device won't receive interrupts while this function is
+ * being executed.
*/

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

@@ -696,13 +746,19 @@ int dpm_suspend_noirq(pm_message_t state
 	suspend_device_irqs();
 	mutex_lock(&dpm_list_mtx);
 	list_for_each_entry_reverse(dev, &dpm_list, power.entry) {
+		dev->power.status = DPM_OFF_IRQ;
 		error = device_suspend_noirq(dev, state);
 		if (error) {
 			pm_dev_err(dev, state, " late", error);

Quoted 8 lines

drivers/base/power/main.c int dpm_suspend_noirq(pm_message_t state · line 758

Quoted 6 lines

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

+			dev->power.status = DPM_OFF;
+			break;
+		}
+		if (async_error) {
+			error = async_error;
 			break;

async_error is 'interesting'. How does locking work in noirq case?

Quoted 6 lines

It's racy, a little bit. :-)

If two async drivers return errors exactly at the same time, one of them will win the race, but it doesn't really matter which one wins as long as async_error is different from zero as a result. And it will be, since it's an 'int' and the integrity of these is guaranteed.

Rather than relying on atomicity of 'int' (where half of kernel hackers says it is and second half says it is not), can we just use atomic_t? It compiles to same code on sane architectures, and serves as documentation/warning...

Quoted 10 lines

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

@@ -659,26 +674,61 @@ static pm_message_t resume_event(pm_mess
 }
 
 /**
- *	device_suspend_noirq - Shut down one device (late suspend).
- *	@dev:	Device.
- *	@state: PM transition of the system being carried out.
+ * __device_suspend_noirq - Execute a "late" suspend callback for given device.
+ * @dev: Device to suspend.
+ * @state: PM transition of the system being carried out.

drivers/base/power/main.c static pm_message_t resume_event(pm_mess · line 666

Quoted 2 lines

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

  *
- *	This is called with interrupts off and only a single CPU running.

that still looks like useful comment... why delete it?

Quoted 11 lines

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

+ * The driver of the device won't receive interrupts while this function is
+ * being executed.
  */

@@ -696,13 +746,19 @@ int dpm_suspend_noirq(pm_message_t state
 	suspend_device_irqs();
 	mutex_lock(&dpm_list_mtx);
 	list_for_each_entry_reverse(dev, &dpm_list, power.entry) {
+		dev->power.status = DPM_OFF_IRQ;
 		error = device_suspend_noirq(dev, state);
 		if (error) {
 			pm_dev_err(dev, state, " late", error);

drivers/base/power/main.c int dpm_suspend_noirq(pm_message_t state · line 758

Quoted 6 lines

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

+			dev->power.status = DPM_OFF;
+			break;
+		}
+		if (async_error) {
+			error = async_error;
 			break;

async_error is 'interesting'. How does locking work in noirq case?

Hi,

The following patches introduce a mechanism allowing us to execute device drivers' suspend and resume callbacks asynchronously during system sleep transitions, such as suspend to RAM. The idea is explained in the [1/1] patch message.

Comments welcome.

Quoted 2 lines · Rafael J. Wysocki

Hi,

Quoted 6 lines

The following patches introduce a mechanism allowing us to execute device drivers' suspend and resume callbacks asynchronously during system sleep transitions, such as suspend to RAM. The idea is explained in the [1/1] patch message.

Comments welcome.

I get the idea. Not bad.

Have you tried it in a serious way? For example, turning on the async_suspend flag for every device?

In one way it isn't as efficient as it could be. You fire off a bunch of async threads and then make many of them wait for parent or child devices. They could be doing useful work instead.

It would be interesting to invent a way of representing explicitly the non-tree dependencies -- assuming there aren't too many of them! (I can just hear the TI guys hollering about power and timer domains...)

Quoted 4 lines · Alan Stern

Quoted 2 lines · Rafael J. Wysocki

Hi,

Quoted 8 lines

Quoted 6 lines

The following patches introduce a mechanism allowing us to execute device drivers' suspend and resume callbacks asynchronously during system sleep transitions, such as suspend to RAM. The idea is explained in the [1/1] patch message.

Comments welcome.

I get the idea. Not bad.

Thanks!

Quoted 2 lines

Have you tried it in a serious way? For example, turning on the async_suspend flag for every device?

No, I've only tested it with a few selected drivers. I'm going to try the "async everyone" scenario, though.

Quoted 3 lines

In one way it isn't as efficient as it could be. You fire off a bunch of async threads and then make many of them wait for parent or child devices. They could be doing useful work instead.

I kind of agree, but then the patches would be more complicated.

Quoted 3 lines

It would be interesting to invent a way of representing explicitly the non-tree dependencies -- assuming there aren't too many of them! (I can just hear the TI guys hollering about power and timer domains...)

I have an idea.

Every such dependency involves two devices, one of which is a "master" and the second of which is a "slave", meaning that the "slave" have to be suspended before the "master" and cannot be resumed before it. In principle we could give each device two lists of "dependency objects", one containing "dependency objects" where the device is the "master" and the other containing "dependency objects" where the device is the "slave". Then, each "dependency object" could be represented as

struct pm_connection {
    struct device *master;
    struct list_head master_hook;
    struct device *slave;
    struct list_head slave_hook;
};

Add some locking, helpers for adding / removing "dependency objects" etc. and it should work. Instead of checking the parent, walk the list of "masters", instead of walking the list of children, walk the list of "slaves".

The core could create those objects for parent-child relationships automatically, the other ones would have to be added by platforms / bus types / drivers etc.

This approach has a problem that it's prone to adding circular dependencies by mistake, but then I think it would apply to any other approach just as well.

Best,
Rafael

Quoted 4 lines · Alan Stern

Quoted 2 lines · Rafael J. Wysocki

Hi,

Quoted 8 lines

Quoted 6 lines

The following patches introduce a mechanism allowing us to execute device drivers' suspend and resume callbacks asynchronously during system sleep transitions, such as suspend to RAM. The idea is explained in the [1/1] patch message.

Comments welcome.

I get the idea. Not bad.

Thanks!

Quoted 2 lines

Have you tried it in a serious way? For example, turning on the async_suspend flag for every device?

No, I've only tested it with a few selected drivers. I'm going to try the "async everyone" scenario, though.

Quoted 3 lines

In one way it isn't as efficient as it could be. You fire off a bunch of async threads and then make many of them wait for parent or child devices. They could be doing useful work instead.

I kind of agree, but then the patches would be more complicated.

Quoted 3 lines

It would be interesting to invent a way of representing explicitly the non-tree dependencies -- assuming there aren't too many of them! (I can just hear the TI guys hollering about power and timer domains...)

I have an idea.

Every such dependency involves two devices, one of which is a "master" and the second of which is a "slave", meaning that the "slave" have to be suspended before the "master" and cannot be resumed before it. In principle we could give each device two lists of "dependency objects", one containing "dependency objects" where the device is the "master" and the other containing "dependency objects" where the device is the "slave". Then, each "dependency object" could be represented as

struct pm_connection {
    struct device *master;
    struct list_head master_hook;
    struct device *slave;
    struct list_head slave_hook;
};

Add some locking, helpers for adding / removing "dependency objects" etc. and it should work. Instead of checking the parent, walk the list of "masters", instead of walking the list of children, walk the list of "slaves".

The core could create those objects for parent-child relationships automatically, the other ones would have to be added by platforms / bus types / drivers etc.

This approach has a problem that it's prone to adding circular dependencies by mistake, but then I think it would apply to any other approach just as well.

Best,
Rafael

Quoted 14 lines · Rafael J. Wysocki

Quoted 4 lines · Alan Stern

Quoted 2 lines · Rafael J. Wysocki

Hi,

Quoted 8 lines

Quoted 6 lines

The following patches introduce a mechanism allowing us to execute device drivers' suspend and resume callbacks asynchronously during system sleep transitions, such as suspend to RAM. The idea is explained in the [1/1] patch message.

Comments welcome.

I get the idea. Not bad.

Quoted 12 lines

Thanks!

Quoted 2 lines

Have you tried it in a serious way? For example, turning on the async_suspend flag for every device?

No, I've only tested it with a few selected drivers. I'm going to try the "async everyone" scenario, though.

Quoted 3 lines

In one way it isn't as efficient as it could be. You fire off a bunch of async threads and then make many of them wait for parent or child devices. They could be doing useful work instead.

are you talking about this scenario, or I find another problem of this approach:
there is a part of dpm_list, dev1->dev_aaa->...->dev_bbb->dev2

dev2 is dev1's first child.
dev1 resume takes 1s
dev_aaa~dev_bbb resume takes 0.1s.

if we call device_enable_async_suspend(dev1, true) in order to resume device1 asynchronously, the real asynchronous resume only happens between dev1 and dev_aaa to dev_bbb because dev2 needs to wait until dev1 resume finished.

so kernel schedules dev1 resume in an async thread first, and then takes 0.1s to finish the dev_aaa to dev_bbb resume, and then sleep 0.9s

Quoted 2 lines

I kind of agree, but then the patches would be more complicated.

The problem is that we need to invoke device_resume for every device synchronously.

I wonder if we can make the child devices inherit the parent's dev->power.async_suspend flag, so that devices that need to wait are resumed asynchronously, i.e. we never wait/sleep when parsing the dpm_list.

this doesn't bring too much benefit in suspend case but it can speed up the resume process a lot.

Of cause, this is not a problem if we turn on the async_suspend flag for every device.

Quoted 25 lines

Quoted 3 lines

It would be interesting to invent a way of representing explicitly the non-tree dependencies -- assuming there aren't too many of them! (I can just hear the TI guys hollering about power and timer domains...)

I have an idea.

Every such dependency involves two devices, one of which is a "master" and the second of which is a "slave", meaning that the "slave" have to be suspended before the "master" and cannot be resumed before it. In principle we could give each device two lists of "dependency objects", one containing "dependency objects" where the device is the "master" and the other containing "dependency objects" where the device is the "slave". Then, each "dependency object" could be represented as

struct pm_connection {
    struct device *master;
    struct list_head master_hook;
    struct device *slave;
    struct list_head slave_hook;
};

Add some locking, helpers for adding / removing "dependency objects" etc. and it should work. Instead of checking the parent, walk the list of "masters", instead of walking the list of children, walk the list of "slaves".

Quoted 4 lines

The core could create those objects for parent-child relationships automatically, the other ones would have to be added by platforms / bus types / drivers etc.

this sounds great. :)

Quoted 31 lines · Zhang Rui

Quoted 14 lines · Rafael J. Wysocki

Quoted 4 lines · Alan Stern

Quoted 2 lines · Rafael J. Wysocki

Hi,

Quoted 8 lines

Quoted 6 lines

The following patches introduce a mechanism allowing us to execute device drivers' suspend and resume callbacks asynchronously during system sleep transitions, such as suspend to RAM. The idea is explained in the [1/1] patch message.

Comments welcome.

I get the idea. Not bad.

Quoted 12 lines

Thanks!

Quoted 2 lines

Have you tried it in a serious way? For example, turning on the async_suspend flag for every device?

No, I've only tested it with a few selected drivers. I'm going to try the "async everyone" scenario, though.

Quoted 3 lines

In one way it isn't as efficient as it could be. You fire off a bunch of async threads and then make many of them wait for parent or child devices. They could be doing useful work instead.

are you talking about this scenario, or I find another problem of this approach:
there is a part of dpm_list, dev1->dev_aaa->...->dev_bbb->dev2

Quoted 8 lines

dev2 is dev1's first child.
dev1 resume takes 1s
dev_aaa~dev_bbb resume takes 0.1s.

if we call device_enable_async_suspend(dev1, true) in order to resume device1 asynchronously, the real asynchronous resume only happens between dev1 and dev_aaa to dev_bbb because dev2 needs to wait until dev1 resume finished.

Yes, that's how it works, but I would call it a limitation rather than a problem. It partially stems from the fact that __async_schedule() executes ptr() synchronously in some circumstances (e.g. async_enabled unset), so the suspend and resume callbacks have to be scheduled in the same order, in which they would have been called synchronously.

Quoted 7 lines

so kernel schedules dev1 resume in an async thread first, and then takes 0.1s to finish the dev_aaa to dev_bbb resume, and then sleep 0.9s

Quoted 2 lines

I kind of agree, but then the patches would be more complicated.

The problem is that we need to invoke device_resume for every device synchronously.

Yes, we do.

Quoted 5 lines

I wonder if we can make the child devices inherit the parent's dev->power.async_suspend flag, so that devices that need to wait are resumed asynchronously, i.e. we never wait/sleep when parsing the dpm_list.

Quoted 2 lines

this doesn't bring too much benefit in suspend case but it can speed up the resume process a lot.

We can do that at the core level,

because there may be dependencies between
the children the core doesn't know about. Subsystems are free to set async_suspend for the entire branches of device hierarchy if they are known not to contain any off-tree dependencies, but the core has no information about that.

Quoted 2 lines

Of cause, this is not a problem if we turn on the async_suspend flag for every device.

Yes, but we cannot do that at this point.

Quoted 25 lines

Quoted 25 lines

Quoted 3 lines

It would be interesting to invent a way of representing explicitly the non-tree dependencies -- assuming there aren't too many of them! (I can just hear the TI guys hollering about power and timer domains...)

I have an idea.

Every such dependency involves two devices, one of which is a "master" and the second of which is a "slave", meaning that the "slave" have to be suspended before the "master" and cannot be resumed before it. In principle we could give each device two lists of "dependency objects", one containing "dependency objects" where the device is the "master" and the other containing "dependency objects" where the device is the "slave". Then, each "dependency object" could be represented as

struct pm_connection {
    struct device *master;
    struct list_head master_hook;
    struct device *slave;
    struct list_head slave_hook;
};

Add some locking, helpers for adding / removing "dependency objects" etc. and it should work. Instead of checking the parent, walk the list of "masters", instead of walking the list of children, walk the list of "slaves".

Quoted 5 lines

Quoted 4 lines

The core could create those objects for parent-child relationships automatically, the other ones would have to be added by platforms / bus types / drivers etc.

this sounds great. :)

It can only be the next step, though, because it will affect the runtime PM as well, among other things.

Thanks,
Rafael

Quoted 62 lines · Rafael J. Wysocki

Quoted 31 lines · Zhang Rui

Quoted 14 lines · Rafael J. Wysocki

Quoted 4 lines · Alan Stern

Quoted 2 lines · Rafael J. Wysocki

Hi,

Quoted 8 lines

Quoted 6 lines

The following patches introduce a mechanism allowing us to execute device drivers' suspend and resume callbacks asynchronously during system sleep transitions, such as suspend to RAM. The idea is explained in the [1/1] patch message.

Comments welcome.

I get the idea. Not bad.

Quoted 12 lines

Thanks!

Quoted 2 lines

Have you tried it in a serious way? For example, turning on the async_suspend flag for every device?

No, I've only tested it with a few selected drivers. I'm going to try the "async everyone" scenario, though.

Quoted 3 lines

In one way it isn't as efficient as it could be. You fire off a bunch of async threads and then make many of them wait for parent or child devices. They could be doing useful work instead.

are you talking about this scenario, or I find another problem of this approach:
there is a part of dpm_list, dev1->dev_aaa->...->dev_bbb->dev2

Quoted 8 lines

dev2 is dev1's first child.
dev1 resume takes 1s
dev_aaa~dev_bbb resume takes 0.1s.

if we call device_enable_async_suspend(dev1, true) in order to resume device1 asynchronously, the real asynchronous resume only happens between dev1 and dev_aaa to dev_bbb because dev2 needs to wait until dev1 resume finished.

Yes, that's how it works, but I would call it a limitation rather than a problem. It partially stems from the fact that __async_schedule() executes ptr() synchronously in some circumstances (e.g. async_enabled unset), so the suspend and resume callbacks have to be scheduled in the same order, in which they would have been called synchronously.

Quoted 7 lines

so kernel schedules dev1 resume in an async thread first, and then takes 0.1s to finish the dev_aaa to dev_bbb resume, and then sleep 0.9s

Quoted 2 lines

I kind of agree, but then the patches would be more complicated.

The problem is that we need to invoke device_resume for every device synchronously.

Yes, we do.

Quoted 5 lines

I wonder if we can make the child devices inherit the parent's dev->power.async_suspend flag, so that devices that need to wait are resumed asynchronously, i.e. we never wait/sleep when parsing the dpm_list.

Quoted 4 lines

Quoted 2 lines

this doesn't bring too much benefit in suspend case but it can speed up the resume process a lot.

We can do that at the core level,

I think you mean we can't do that at the core level.

Quoted 6 lines

because there may be dependencies between
the children the core doesn't know about. Subsystems are free to set async_suspend for the entire branches of device hierarchy if they are known not to contain any off-tree dependencies, but the core has no information about that.

hmm, but the current patch doesn't handle the off-tree dependencies neither.
e.g.
dev1, dev2, dev3
dev2 depends on dev1, dev3 is dev1's first child,
we only promise that dev1 has been resumed before resuming dev3 in the current proposal.

anyway, this is not a problem after the pm_connection stuff is implemented. :)

Quoted 62 lines · Rafael J. Wysocki

Quoted 31 lines · Zhang Rui

Quoted 14 lines · Rafael J. Wysocki

Quoted 4 lines · Alan Stern

Quoted 2 lines · Rafael J. Wysocki

Hi,

Quoted 8 lines

Quoted 6 lines

The following patches introduce a mechanism allowing us to execute device drivers' suspend and resume callbacks asynchronously during system sleep transitions, such as suspend to RAM. The idea is explained in the [1/1] patch message.

Comments welcome.

I get the idea. Not bad.

Quoted 12 lines

Thanks!

Quoted 2 lines

Have you tried it in a serious way? For example, turning on the async_suspend flag for every device?

No, I've only tested it with a few selected drivers. I'm going to try the "async everyone" scenario, though.

Quoted 3 lines

In one way it isn't as efficient as it could be. You fire off a bunch of async threads and then make many of them wait for parent or child devices. They could be doing useful work instead.

are you talking about this scenario, or I find another problem of this approach:
there is a part of dpm_list, dev1->dev_aaa->...->dev_bbb->dev2

Quoted 8 lines

dev2 is dev1's first child.
dev1 resume takes 1s
dev_aaa~dev_bbb resume takes 0.1s.

if we call device_enable_async_suspend(dev1, true) in order to resume device1 asynchronously, the real asynchronous resume only happens between dev1 and dev_aaa to dev_bbb because dev2 needs to wait until dev1 resume finished.

Yes, that's how it works, but I would call it a limitation rather than a problem. It partially stems from the fact that __async_schedule() executes ptr() synchronously in some circumstances (e.g. async_enabled unset), so the suspend and resume callbacks have to be scheduled in the same order, in which they would have been called synchronously.

Quoted 7 lines

so kernel schedules dev1 resume in an async thread first, and then takes 0.1s to finish the dev_aaa to dev_bbb resume, and then sleep 0.9s

Quoted 2 lines

I kind of agree, but then the patches would be more complicated.

The problem is that we need to invoke device_resume for every device synchronously.

Yes, we do.

Quoted 5 lines

I wonder if we can make the child devices inherit the parent's dev->power.async_suspend flag, so that devices that need to wait are resumed asynchronously, i.e. we never wait/sleep when parsing the dpm_list.

Quoted 4 lines

Quoted 2 lines

this doesn't bring too much benefit in suspend case but it can speed up the resume process a lot.

We can do that at the core level,

I think you mean we can't do that at the core level.

Quoted 6 lines

because there may be dependencies between
the children the core doesn't know about. Subsystems are free to set async_suspend for the entire branches of device hierarchy if they are known not to contain any off-tree dependencies, but the core has no information about that.

hmm, but the current patch doesn't handle the off-tree dependencies neither.
e.g.
dev1, dev2, dev3
dev2 depends on dev1, dev3 is dev1's first child,
we only promise that dev1 has been resumed before resuming dev3 in the current proposal.

anyway, this is not a problem after the pm_connection stuff is implemented. :)

Quoted 31 lines · Zhang Rui

Quoted 14 lines · Rafael J. Wysocki

Quoted 4 lines · Alan Stern

Quoted 2 lines · Rafael J. Wysocki

Hi,

Quoted 8 lines

Quoted 6 lines

The following patches introduce a mechanism allowing us to execute device drivers' suspend and resume callbacks asynchronously during system sleep transitions, such as suspend to RAM. The idea is explained in the [1/1] patch message.

Comments welcome.

I get the idea. Not bad.

Quoted 12 lines

Thanks!

Quoted 2 lines

Have you tried it in a serious way? For example, turning on the async_suspend flag for every device?

No, I've only tested it with a few selected drivers. I'm going to try the "async everyone" scenario, though.

Quoted 3 lines

In one way it isn't as efficient as it could be. You fire off a bunch of async threads and then make many of them wait for parent or child devices. They could be doing useful work instead.

are you talking about this scenario, or I find another problem of this approach:
there is a part of dpm_list, dev1->dev_aaa->...->dev_bbb->dev2

Quoted 8 lines

dev2 is dev1's first child.
dev1 resume takes 1s
dev_aaa~dev_bbb resume takes 0.1s.

if we call device_enable_async_suspend(dev1, true) in order to resume device1 asynchronously, the real asynchronous resume only happens between dev1 and dev_aaa to dev_bbb because dev2 needs to wait until dev1 resume finished.

Yes, that's how it works, but I would call it a limitation rather than a problem. It partially stems from the fact that __async_schedule() executes ptr() synchronously in some circumstances (e.g. async_enabled unset), so the suspend and resume callbacks have to be scheduled in the same order, in which they would have been called synchronously.

Quoted 7 lines

so kernel schedules dev1 resume in an async thread first, and then takes 0.1s to finish the dev_aaa to dev_bbb resume, and then sleep 0.9s

Quoted 2 lines

I kind of agree, but then the patches would be more complicated.

The problem is that we need to invoke device_resume for every device synchronously.

Yes, we do.

Quoted 5 lines

I wonder if we can make the child devices inherit the parent's dev->power.async_suspend flag, so that devices that need to wait are resumed asynchronously, i.e. we never wait/sleep when parsing the dpm_list.

Quoted 2 lines

this doesn't bring too much benefit in suspend case but it can speed up the resume process a lot.

We can do that at the core level,

because there may be dependencies between
the children the core doesn't know about. Subsystems are free to set async_suspend for the entire branches of device hierarchy if they are known not to contain any off-tree dependencies, but the core has no information about that.

Quoted 2 lines

Of cause, this is not a problem if we turn on the async_suspend flag for every device.

Yes, but we cannot do that at this point.

Quoted 25 lines

Quoted 25 lines

Quoted 3 lines

It would be interesting to invent a way of representing explicitly the non-tree dependencies -- assuming there aren't too many of them! (I can just hear the TI guys hollering about power and timer domains...)

I have an idea.

Every such dependency involves two devices, one of which is a "master" and the second of which is a "slave", meaning that the "slave" have to be suspended before the "master" and cannot be resumed before it. In principle we could give each device two lists of "dependency objects", one containing "dependency objects" where the device is the "master" and the other containing "dependency objects" where the device is the "slave". Then, each "dependency object" could be represented as

struct pm_connection {
    struct device *master;
    struct list_head master_hook;
    struct device *slave;
    struct list_head slave_hook;
};

Add some locking, helpers for adding / removing "dependency objects" etc. and it should work. Instead of checking the parent, walk the list of "masters", instead of walking the list of children, walk the list of "slaves".

Quoted 5 lines

Quoted 4 lines

The core could create those objects for parent-child relationships automatically, the other ones would have to be added by platforms / bus types / drivers etc.

this sounds great. :)

It can only be the next step, though, because it will affect the runtime PM as well, among other things.

Thanks,
Rafael

Quoted 14 lines · Rafael J. Wysocki

Quoted 4 lines · Alan Stern

Quoted 2 lines · Rafael J. Wysocki

Hi,

Quoted 8 lines

Quoted 6 lines

The following patches introduce a mechanism allowing us to execute device drivers' suspend and resume callbacks asynchronously during system sleep transitions, such as suspend to RAM. The idea is explained in the [1/1] patch message.

Comments welcome.

I get the idea. Not bad.

Quoted 12 lines

Thanks!

Quoted 2 lines

Have you tried it in a serious way? For example, turning on the async_suspend flag for every device?

No, I've only tested it with a few selected drivers. I'm going to try the "async everyone" scenario, though.

Quoted 3 lines

In one way it isn't as efficient as it could be. You fire off a bunch of async threads and then make many of them wait for parent or child devices. They could be doing useful work instead.

are you talking about this scenario, or I find another problem of this approach:
there is a part of dpm_list, dev1->dev_aaa->...->dev_bbb->dev2

dev2 is dev1's first child.
dev1 resume takes 1s
dev_aaa~dev_bbb resume takes 0.1s.

if we call device_enable_async_suspend(dev1, true) in order to resume device1 asynchronously, the real asynchronous resume only happens between dev1 and dev_aaa to dev_bbb because dev2 needs to wait until dev1 resume finished.

so kernel schedules dev1 resume in an async thread first, and then takes 0.1s to finish the dev_aaa to dev_bbb resume, and then sleep 0.9s

Quoted 2 lines

I kind of agree, but then the patches would be more complicated.

The problem is that we need to invoke device_resume for every device synchronously.

I wonder if we can make the child devices inherit the parent's dev->power.async_suspend flag, so that devices that need to wait are resumed asynchronously, i.e. we never wait/sleep when parsing the dpm_list.

this doesn't bring too much benefit in suspend case but it can speed up the resume process a lot.

Of cause, this is not a problem if we turn on the async_suspend flag for every device.

Quoted 25 lines

Quoted 3 lines

It would be interesting to invent a way of representing explicitly the non-tree dependencies -- assuming there aren't too many of them! (I can just hear the TI guys hollering about power and timer domains...)

I have an idea.

Every such dependency involves two devices, one of which is a "master" and the second of which is a "slave", meaning that the "slave" have to be suspended before the "master" and cannot be resumed before it. In principle we could give each device two lists of "dependency objects", one containing "dependency objects" where the device is the "master" and the other containing "dependency objects" where the device is the "slave". Then, each "dependency object" could be represented as

struct pm_connection {
    struct device *master;
    struct list_head master_hook;
    struct device *slave;
    struct list_head slave_hook;
};

Add some locking, helpers for adding / removing "dependency objects" etc. and it should work. Instead of checking the parent, walk the list of "masters", instead of walking the list of children, walk the list of "slaves".

Quoted 4 lines

The core could create those objects for parent-child relationships automatically, the other ones would have to be added by platforms / bus types / drivers etc.

this sounds great. :)

I have an idea.

Every such dependency involves two devices, one of which is a "master" and the second of which is a "slave", meaning that the "slave" have to be suspended before the "master" and cannot be resumed before it. In principle we could give each device two lists of "dependency objects", one containing "dependency objects" where the device is the "master" and the other containing "dependency objects" where the device is the "slave". Then, each "dependency object" could be represented as

struct pm_connection {
    struct device *master;
    struct list_head master_hook;
    struct device *slave;
    struct list_head slave_hook;
};

Add some locking, helpers for adding / removing "dependency objects" etc. and it should work. Instead of checking the parent, walk the list of "masters", instead of walking the list of children, walk the list of "slaves".

The core could create those objects for parent-child relationships automatically, the other ones would have to be added by platforms / bus types / drivers etc.

This approach has a problem that it's prone to adding circular dependencies by mistake, but then I think it would apply to any other approach just as well.

I have an idea.

Every such dependency involves two devices, one of which is a "master" and the second of which is a "slave", meaning that the "slave" have to be suspended before the "master" and cannot be resumed before it. In principle we could give each device two lists of "dependency objects", one containing "dependency objects" where the device is the "master" and the other containing "dependency objects" where the device is the "slave". Then, each "dependency object" could be represented as

struct pm_connection {
    struct device *master;
    struct list_head master_hook;
    struct device *slave;
    struct list_head slave_hook;
};

Add some locking, helpers for adding / removing "dependency objects" etc. and it should work. Instead of checking the parent, walk the list of "masters", instead of walking the list of children, walk the list of "slaves".

The core could create those objects for parent-child relationships automatically, the other ones would have to be added by platforms / bus types / drivers etc.

This approach has a problem that it's prone to adding circular dependencies by mistake, but then I think it would apply to any other approach just as well.

Quoted 17 lines · Rafael J. Wysocki

I have an idea.

Every such dependency involves two devices, one of which is a "master" and the second of which is a "slave", meaning that the "slave" have to be suspended before the "master" and cannot be resumed before it. In principle we could give each device two lists of "dependency objects", one containing "dependency objects" where the device is the "master" and the other containing "dependency objects" where the device is the "slave". Then, each "dependency object" could be represented as

struct pm_connection {
    struct device *master;
    struct list_head master_hook;
    struct device *slave;
    struct list_head slave_hook;
};

Quoted 3 lines

Add some locking, helpers for adding / removing "dependency objects" etc. and it should work. Instead of checking the parent, walk the list of "masters", instead of walking the list of children, walk the list of "slaves".

If the set of pm_connection objects is reasonably small then the master_hook and slave_hook aren't needed; you can just read through the entire list. Leaving out parent-child connections, which we already know, would help shrink the set.

The layout of the pm_connection objects could then be improved slightly. Each object could contain a variable-sized array of device pointers together with two integers, M and S. The first M pointers would be masters and the remaining S pointers would be slaves. This could be useful when, for example, a large number of devices all depend on one particular power device.

Quoted 4 lines

If the set of pm_connection objects is reasonably small then the master_hook and slave_hook aren't needed; you can just read through the entire list. Leaving out parent-child connections, which we already know, would help shrink the set.

Do you mean a global list for the entire system? I'm not sure if that wouldn't hurt performance (please note that it also affects runtime PM).

We could use counters of "suspended slaves" and "active masters" to optimize things, but still updating these counters could take time (one has to find all devices dependent on given one and update a counter for each of them).

Quoted 5 lines · Rafael J. Wysocki

Quoted 4 lines

If the set of pm_connection objects is reasonably small then the master_hook and slave_hook aren't needed; you can just read through the entire list. Leaving out parent-child connections, which we already know, would help shrink the set.

Quoted 2 lines

Do you mean a global list for the entire system? I'm not sure if that wouldn't hurt performance (please note that it also affects runtime PM).

It doesn't have to affect runtime PM. That is, for runtime PM we can make the bus subsystems responsible for enforcing the dependencies.

Quoted 5 lines · Rafael J. Wysocki

Quoted 4 lines

If the set of pm_connection objects is reasonably small then the master_hook and slave_hook aren't needed; you can just read through the entire list. Leaving out parent-child connections, which we already know, would help shrink the set.

Quoted 2 lines

Do you mean a global list for the entire system? I'm not sure if that wouldn't hurt performance (please note that it also affects runtime PM).

It doesn't have to affect runtime PM. That is, for runtime PM we can make the bus subsystems responsible for enforcing the dependencies.

Quoted 4 lines

If the set of pm_connection objects is reasonably small then the master_hook and slave_hook aren't needed; you can just read through the entire list. Leaving out parent-child connections, which we already know, would help shrink the set.

Do you mean a global list for the entire system? I'm not sure if that wouldn't hurt performance (please note that it also affects runtime PM).

We could use counters of "suspended slaves" and "active masters" to optimize things, but still updating these counters could take time (one has to find all devices dependent on given one and update a counter for each of them).

Quoted 17 lines · Rafael J. Wysocki

I have an idea.

Every such dependency involves two devices, one of which is a "master" and the second of which is a "slave", meaning that the "slave" have to be suspended before the "master" and cannot be resumed before it. In principle we could give each device two lists of "dependency objects", one containing "dependency objects" where the device is the "master" and the other containing "dependency objects" where the device is the "slave". Then, each "dependency object" could be represented as

struct pm_connection {
    struct device *master;
    struct list_head master_hook;
    struct device *slave;
    struct list_head slave_hook;
};

Quoted 3 lines

Add some locking, helpers for adding / removing "dependency objects" etc. and it should work. Instead of checking the parent, walk the list of "masters", instead of walking the list of children, walk the list of "slaves".

If the set of pm_connection objects is reasonably small then the master_hook and slave_hook aren't needed; you can just read through the entire list. Leaving out parent-child connections, which we already know, would help shrink the set.

The layout of the pm_connection objects could then be improved slightly. Each object could contain a variable-sized array of device pointers together with two integers, M and S. The first M pointers would be masters and the remaining S pointers would be slaves. This could be useful when, for example, a large number of devices all depend on one particular power device.

Quoted 4 lines · Alan Stern

Quoted 2 lines · Rafael J. Wysocki

Hi,

Quoted 8 lines

Quoted 6 lines

The following patches introduce a mechanism allowing us to execute device drivers' suspend and resume callbacks asynchronously during system sleep transitions, such as suspend to RAM. The idea is explained in the [1/1] patch message.

Comments welcome.

I get the idea. Not bad.

Thanks!

Quoted 2 lines

Have you tried it in a serious way? For example, turning on the async_suspend flag for every device?

No, I've only tested it with a few selected drivers. I'm going to try the "async everyone" scenario, though.

Quoted 2 lines

No, I've only tested it with a few selected drivers. I'm going to try the "async everyone" scenario, though.

On HP nx6325 booted with init=/bin/bash it doesn't pass the 'echo devices > /sys/power/pm_test && echo mem > /sys/power/state' test.

The suspend part actually seems to work, but the resume part crashes miserably.

Quoted 3 lines · Rafael J. Wysocki

Quoted 2 lines

No, I've only tested it with a few selected drivers. I'm going to try the "async everyone" scenario, though.

Quoted 5 lines

On HP nx6325 booted with init=/bin/bash it doesn't pass the 'echo devices > /sys/power/pm_test && echo mem > /sys/power/state' test.

The suspend part actually seems to work, but the resume part crashes miserably.

Any details? Can you tell where it crashes?

Quoted 8 lines · Alan Stern

Quoted 6 lines · Rafael J. Wysocki

Quoted 2 lines

No, I've only tested it with a few selected drivers. I'm going to try the "async everyone" scenario, though.

On HP nx6325 booted with init=/bin/bash it doesn't pass the 'echo devices > /sys/power/pm_test && echo mem > /sys/power/state' test.

Quoted 4 lines

Quoted 2 lines

The suspend part actually seems to work, but the resume part crashes miserably.

Any details? Can you tell where it crashes?

Unfortunately it ends up in a continuous flood of backtraces, so I can't say much.

However, if I set async_suspend for all PCI devices, as well as for ACPI battery and i8042, everything apparently works, even with real suspend-resume.

Quoted 9 lines · Rafael J. Wysocki

Quoted 8 lines · Alan Stern

Quoted 6 lines · Rafael J. Wysocki

Quoted 2 lines

No, I've only tested it with a few selected drivers. I'm going to try the "async everyone" scenario, though.

On HP nx6325 booted with init=/bin/bash it doesn't pass the 'echo devices > /sys/power/pm_test && echo mem > /sys/power/state' test.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

The suspend part actually seems to work, but the resume part crashes miserably.

Any details? Can you tell where it crashes?

Unfortunately it ends up in a continuous flood of backtraces, so I can't say much.

Same sort of thing happened with me, but it appeared to happen during suspend. (I say "appeared" because the system froze for a while at the start of the suspend until I hit a key. This may be related to all the recent churn in the TTY layer, combined with the fact that I was using a serial console. It didn't help -- none of the backtraces got sent out the serial port.)

Quoted 2 lines

However, if I set async_suspend for all PCI devices, as well as for ACPI battery and i8042, everything apparently works, even with real suspend-resume.

I think it would be worthwhile to track down these unknown problems and dependencies. It won't be easy, though...

Quoted 2 lines

I think it would be worthwhile to track down these unknown problems and dependencies. It won't be easy, though...

Agreed.

Hm. Perhaps it's a good idea to get this code in, so that people can play with it. The more people will look into this, the greater the chance to identify problematic drivers.

Quoted 2 lines

I think it would be worthwhile to track down these unknown problems and dependencies. It won't be easy, though...

Agreed.

Hm. Perhaps it's a good idea to get this code in, so that people can play with it. The more people will look into this, the greater the chance to identify problematic drivers.

Quoted 26 lines · Alan Stern

Quoted 9 lines · Rafael J. Wysocki

Quoted 8 lines · Alan Stern

Quoted 6 lines · Rafael J. Wysocki

Quoted 2 lines

No, I've only tested it with a few selected drivers. I'm going to try the "async everyone" scenario, though.

On HP nx6325 booted with init=/bin/bash it doesn't pass the 'echo devices > /sys/power/pm_test && echo mem > /sys/power/state' test.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

The suspend part actually seems to work, but the resume part crashes miserably.

Any details? Can you tell where it crashes?

Unfortunately it ends up in a continuous flood of backtraces, so I can't say much.

Same sort of thing happened with me, but it appeared to happen during suspend. (I say "appeared" because the system froze for a while at the start of the suspend until I hit a key. This may be related to all the recent churn in the TTY layer, combined with the fact that I was using a serial console. It didn't help -- none of the backtraces got sent out the serial port.)

Quoted 5 lines

Quoted 2 lines

However, if I set async_suspend for all PCI devices, as well as for ACPI battery and i8042, everything apparently works, even with real suspend-resume.

I think it would be worthwhile to track down these unknown problems and dependencies. It won't be easy, though...

Agreed.

Quoted 26 lines · Alan Stern

Quoted 9 lines · Rafael J. Wysocki

Quoted 8 lines · Alan Stern

Quoted 6 lines · Rafael J. Wysocki

Quoted 2 lines

No, I've only tested it with a few selected drivers. I'm going to try the "async everyone" scenario, though.

On HP nx6325 booted with init=/bin/bash it doesn't pass the 'echo devices > /sys/power/pm_test && echo mem > /sys/power/state' test.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

The suspend part actually seems to work, but the resume part crashes miserably.

Any details? Can you tell where it crashes?

Unfortunately it ends up in a continuous flood of backtraces, so I can't say much.

Same sort of thing happened with me, but it appeared to happen during suspend. (I say "appeared" because the system froze for a while at the start of the suspend until I hit a key. This may be related to all the recent churn in the TTY layer, combined with the fact that I was using a serial console. It didn't help -- none of the backtraces got sent out the serial port.)

Quoted 5 lines

Quoted 2 lines

However, if I set async_suspend for all PCI devices, as well as for ACPI battery and i8042, everything apparently works, even with real suspend-resume.

I think it would be worthwhile to track down these unknown problems and dependencies. It won't be easy, though...

Agreed.

Quoted 9 lines · Rafael J. Wysocki

Quoted 8 lines · Alan Stern

Quoted 6 lines · Rafael J. Wysocki

Quoted 2 lines

No, I've only tested it with a few selected drivers. I'm going to try the "async everyone" scenario, though.

On HP nx6325 booted with init=/bin/bash it doesn't pass the 'echo devices > /sys/power/pm_test && echo mem > /sys/power/state' test.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

The suspend part actually seems to work, but the resume part crashes miserably.

Any details? Can you tell where it crashes?

Unfortunately it ends up in a continuous flood of backtraces, so I can't say much.

Same sort of thing happened with me, but it appeared to happen during suspend. (I say "appeared" because the system froze for a while at the start of the suspend until I hit a key. This may be related to all the recent churn in the TTY layer, combined with the fact that I was using a serial console. It didn't help -- none of the backtraces got sent out the serial port.)

Quoted 2 lines

However, if I set async_suspend for all PCI devices, as well as for ACPI battery and i8042, everything apparently works, even with real suspend-resume.

I think it would be worthwhile to track down these unknown problems and dependencies. It won't be easy, though...

Quoted 8 lines · Alan Stern

Quoted 6 lines · Rafael J. Wysocki

Quoted 2 lines

No, I've only tested it with a few selected drivers. I'm going to try the "async everyone" scenario, though.

On HP nx6325 booted with init=/bin/bash it doesn't pass the 'echo devices > /sys/power/pm_test && echo mem > /sys/power/state' test.

Quoted 4 lines

Quoted 2 lines

The suspend part actually seems to work, but the resume part crashes miserably.

Any details? Can you tell where it crashes?

Unfortunately it ends up in a continuous flood of backtraces, so I can't say much.

However, if I set async_suspend for all PCI devices, as well as for ACPI battery and i8042, everything apparently works, even with real suspend-resume.

Quoted 3 lines · Rafael J. Wysocki

Quoted 2 lines

No, I've only tested it with a few selected drivers. I'm going to try the "async everyone" scenario, though.

Quoted 5 lines

On HP nx6325 booted with init=/bin/bash it doesn't pass the 'echo devices > /sys/power/pm_test && echo mem > /sys/power/state' test.

The suspend part actually seems to work, but the resume part crashes miserably.

Any details? Can you tell where it crashes?

Quoted 2 lines

No, I've only tested it with a few selected drivers. I'm going to try the "async everyone" scenario, though.

On HP nx6325 booted with init=/bin/bash it doesn't pass the 'echo devices > /sys/power/pm_test && echo mem > /sys/power/state' test.

The suspend part actually seems to work, but the resume part crashes miserably.

Quoted 4 lines · Alan Stern

Quoted 2 lines · Rafael J. Wysocki

Hi,

Quoted 8 lines

Quoted 6 lines

The following patches introduce a mechanism allowing us to execute device drivers' suspend and resume callbacks asynchronously during system sleep transitions, such as suspend to RAM. The idea is explained in the [1/1] patch message.

Comments welcome.

I get the idea. Not bad.

Thanks!

Quoted 2 lines

Have you tried it in a serious way? For example, turning on the async_suspend flag for every device?

No, I've only tested it with a few selected drivers. I'm going to try the "async everyone" scenario, though.

Quoted 2 lines · Rafael J. Wysocki

Hi,

Quoted 6 lines

The following patches introduce a mechanism allowing us to execute device drivers' suspend and resume callbacks asynchronously during system sleep transitions, such as suspend to RAM. The idea is explained in the [1/1] patch message.

Comments welcome.

I get the idea. Not bad.

Have you tried it in a serious way? For example, turning on the async_suspend flag for every device?

In one way it isn't as efficient as it could be. You fire off a bunch of async threads and then make many of them wait for parent or child devices. They could be doing useful work instead.

It would be interesting to invent a way of representing explicitly the non-tree dependencies -- assuming there aren't too many of them! (I can just hear the TI guys hollering about power and timer domains...)

Hi,

The following patches introduce a mechanism allowing us to execute device drivers' suspend and resume callbacks asynchronously during system sleep transitions, such as suspend to RAM. The idea is explained in the [1/1] patch message.

Comments welcome.

The following patches introduce a mechanism allowing us to execute device drivers' suspend and resume callbacks asynchronously during system sleep transitions, such as suspend to RAM. The idea is explained in the [1/1] patch message.

The following patches introduce a mechanism allowing us to execute device drivers' suspend and resume callbacks asynchronously during system sleep transitions, such as suspend to RAM. The idea is explained in the [1/1] patch message.

Quoted 4 lines

The following patches introduce a mechanism allowing us to execute device drivers' suspend and resume callbacks asynchronously during system sleep transitions, such as suspend to RAM. The idea is explained in the [1/1] patch message.

it looks sane to me... Not too long and nice code :-)

Quoted 4 lines

The following patches introduce a mechanism allowing us to execute device drivers' suspend and resume callbacks asynchronously during system sleep transitions, such as suspend to RAM. The idea is explained in the [1/1] patch message.

it looks sane to me... Not too long and nice code :-)

Hi,

The following patches introduce a mechanism allowing us to execute device drivers' suspend and resume callbacks asynchronously during system sleep transitions, such as suspend to RAM. The idea is explained in the [1/1] patch message.

From: Rafael J. Wysocki <[email protected]>
Subject: PM: Update kerneldoc comments in drivers/base/power/main.c

The kerneldoc comments in drivers/base/power/main.c are generally outdated and some of them don't describe the functions very accurately. Update them and standardize the format to use spaces instead of tabs.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/power/main.c |  169 ++++++++++++++++++++++------------------------
 1 file changed, 84 insertions(+), 85 deletions(-)

Index: linux-2.6/drivers/base/power/main.c
===================================================================

drivers/base/power/main.c +84 -86

@@ -50,7 +50,7 @@ static DEFINE_MUTEX(dpm_list_mtx);
 static bool transition_started;
 
 /**
- * device_pm_init - Initialize the PM-related part of a device object
+ * device_pm_init - Initialize the PM-related part of a device object.
  * @dev: Device object being initialized.
  */
 void device_pm_init(struct device *dev)

@@ -61,7 +61,7 @@ void device_pm_init(struct device *dev)
 }
 
 /**
- *	device_pm_lock - lock the list of active devices used by the PM core
+ * device_pm_lock - Lock the list of active devices used by the PM core.
  */
 void device_pm_lock(void)
 {

@@ -69,7 +69,7 @@ void device_pm_lock(void)
 }
 
 /**
- *	device_pm_unlock - unlock the list of active devices used by the PM core
+ * device_pm_unlock - Unlock the list of active devices used by the PM core.
  */
 void device_pm_unlock(void)
 {

@@ -77,8 +77,8 @@ void device_pm_unlock(void)
 }
 
 /**
- *	device_pm_add - add a device to the list of active devices
- *	@dev:	Device to be added to the list
+ * device_pm_add - Add a device to the PM core's list of active devices.
+ * @dev: Device to add to the list.
  */
 void device_pm_add(struct device *dev)
 {

@@ -104,10 +104,8 @@ void device_pm_add(struct device *dev)
 }
 
 /**
- *	device_pm_remove - remove a device from the list of active devices
- *	@dev:	Device to be removed from the list
- *
- *	This function also removes the device's PM-related sysfs attributes.
+ * device_pm_remove - Remove a device from the PM core's list of active devices.
+ * @dev: Device to be removed from the list.
  */
 void device_pm_remove(struct device *dev)
 {

@@ -121,9 +119,9 @@ void device_pm_remove(struct device *dev
 }
 
 /**
- *	device_pm_move_before - move device in dpm_list
- *	@deva:  Device to move in dpm_list
- *	@devb:  Device @deva should come before
+ * device_pm_move_before - Move device in the PM core's list of active devices.
+ * @deva: Device to move in dpm_list.
+ * @devb: Device @deva should come before.
  */
 void device_pm_move_before(struct device *deva, struct device *devb)
 {

@@ -137,9 +135,9 @@ void device_pm_move_before(struct device
 }
 
 /**
- *	device_pm_move_after - move device in dpm_list
- *	@deva:  Device to move in dpm_list
- *	@devb:  Device @deva should come after
+ * device_pm_move_after - Move device in the PM core's list of active devices.
+ * @deva: Device to move in dpm_list.
+ * @devb: Device @deva should come after.
  */
 void device_pm_move_after(struct device *deva, struct device *devb)
 {

@@ -153,8 +151,8 @@ void device_pm_move_after(struct device 
 }
 
 /**
- * 	device_pm_move_last - move device to end of dpm_list
- * 	@dev:   Device to move in dpm_list
+ * device_pm_move_last - Move device to end of the PM core's list of devices.
+ * @dev: Device to move in dpm_list.
  */
 void device_pm_move_last(struct device *dev)
 {

@@ -165,10 +163,10 @@ void device_pm_move_last(struct device *
 }
 
 /**
- *	pm_op - execute the PM operation appropiate for given PM event
- *	@dev:	Device.
- *	@ops:	PM operations to choose from.
- *	@state:	PM transition of the system being carried out.
+ * pm_op - Execute the PM operation appropiate for given PM event.
+ * @dev: Device to handle.
+ * @ops: PM operations to choose from.
+ * @state: PM transition of the system being carried out.
  */
 static int pm_op(struct device *dev,
 		 const struct dev_pm_ops *ops,

@@ -226,13 +224,13 @@ static int pm_op(struct device *dev,
 }
 
 /**
- *	pm_noirq_op - execute the PM operation appropiate for given PM event
- *	@dev:	Device.
- *	@ops:	PM operations to choose from.
- *	@state: PM transition of the system being carried out.
+ * pm_noirq_op - Execute the PM operation appropiate for given PM event.
+ * @dev: Device to handle.
+ * @ops: PM operations to choose from.
+ * @state: PM transition of the system being carried out.
  *
- *	The operation is executed with interrupts disabled by the only remaining
- *	functional CPU in the system.
+ * The driver of @dev will not receive interrupts while this fuction is being
+ * executed.
  */
 static int pm_noirq_op(struct device *dev,
 			const struct dev_pm_ops *ops,

@@ -330,11 +328,12 @@ static void pm_dev_err(struct device *de
 /*------------------------- Resume routines -------------------------*/
 
 /**
- *	device_resume_noirq - Power on one device (early resume).
- *	@dev:	Device.
- *	@state: PM transition of the system being carried out.
+ * device_resume_noirq - Execute an "early resume" callback for given device.
+ * @dev: Device to handle.
+ * @state: PM transition of the system being carried out.
  *
- *	Must be called with interrupts disabled.
+ * The driver of @dev will not receive interrupts while this fuction is being
+ * executed.
  */
 static int device_resume_noirq(struct device *dev, pm_message_t state)
 {

@@ -356,14 +355,11 @@ static int device_resume_noirq(struct de
 }
 
 /**
- *	dpm_resume_noirq - Power on all regular (non-sysdev) devices.
- *	@state: PM transition of the system being carried out.
- *
- *	Call the "noirq" resume handlers for all devices marked as
- *	DPM_OFF_IRQ and enable device drivers to receive interrupts.
+ * dpm_resume_noirq - Execute "early resume" callbacks for non-sysdev devices.
+ * @state: PM transition of the system being carried out.
  *
- *	Must be called under dpm_list_mtx.  Device drivers should not receive
- *	interrupts while it's being executed.
+ * Call the "noirq" resume handlers for all devices marked as DPM_OFF_IRQ and
+ * enable device drivers to receive interrupts.
  */
 void dpm_resume_noirq(pm_message_t state)
 {

@@ -385,9 +381,9 @@ void dpm_resume_noirq(pm_message_t state
 EXPORT_SYMBOL_GPL(dpm_resume_noirq);
 
 /**
- *	device_resume - Restore state for one device.
- *	@dev:	Device.
- *	@state: PM transition of the system being carried out.
+ * device_resume - Execute "resume" callbacks for given device.
+ * @dev: Device to handle.
+ * @state: PM transition of the system being carried out.
  */
 static int device_resume(struct device *dev, pm_message_t state)
 {

@@ -436,11 +432,11 @@ static int device_resume(struct device *
 }
 
 /**
- *	dpm_resume - Resume every device.
- *	@state: PM transition of the system being carried out.
+ * dpm_resume - Execute "resume" callbacks for non-sysdev devices.
+ * @state: PM transition of the system being carried out.
  *
- *	Execute the appropriate "resume" callback for all devices the status of
- *	which indicates that they are inactive.
+ * Execute the appropriate "resume" callback for all devices the status of which
+ * indicates that they are suspended.
  */
 static void dpm_resume(pm_message_t state)
 {

@@ -477,9 +473,9 @@ static void dpm_resume(pm_message_t stat
 }
 
 /**
- *	device_complete - Complete a PM transition for given device
- *	@dev:	Device.
- *	@state: PM transition of the system being carried out.
+ * device_complete - Complete a PM transition for given device.
+ * @dev: Device to handle.
+ * @state: PM transition of the system being carried out.
  */
 static void device_complete(struct device *dev, pm_message_t state)
 {

@@ -504,11 +500,11 @@ static void device_complete(struct devic
 }
 
 /**
- *	dpm_complete - Complete a PM transition for all devices.
- *	@state: PM transition of the system being carried out.
+ * dpm_complete - Complete a PM transition for all non-sysdev devices.
+ * @state: PM transition of the system being carried out.
  *
- *	Execute the ->complete() callbacks for all devices that are not marked
- *	as DPM_ON.
+ * Execute the ->complete() callbacks for all devices the PM status of which is
+ * not DPM_ON (this allows new devices to be registered).
  */
 static void dpm_complete(pm_message_t state)
 {

@@ -538,11 +534,11 @@ static void dpm_complete(pm_message_t st
 }
 
 /**
- *	dpm_resume_end - Restore state of each device in system.
- *	@state: PM transition of the system being carried out.
+ * dpm_resume_end - Execute "resume" callbacks and complete system transition.
+ * @state: PM transition of the system being carried out.
  *
- *	Resume all the devices, unlock them all, and allow new
- *	devices to be registered once again.
+ * Resume all the devices, unlock them all, and complete the PM transition of
+ * the system.
  */
 void dpm_resume_end(pm_message_t state)
 {

@@ -556,9 +552,11 @@ EXPORT_SYMBOL_GPL(dpm_resume_end);
 /*------------------------- Suspend routines -------------------------*/
 
 /**
- *	resume_event - return a PM message representing the resume event
- *	               corresponding to given sleep state.
- *	@sleep_state: PM message representing a sleep state.
+ * resume_event - Return a "resume" message for given "suspend" sleep state.
+ * @sleep_state: PM message representing a sleep state.
+ *
+ * Return a PM message representing the resume event corresponding to given
+ * sleep state.
  */
 static pm_message_t resume_event(pm_message_t sleep_state)
 {

@@ -575,11 +573,12 @@ static pm_message_t resume_event(pm_mess
 }
 
 /**
- *	device_suspend_noirq - Shut down one device (late suspend).
- *	@dev:	Device.
- *	@state: PM transition of the system being carried out.
+ * device_suspend_noirq - Execute a "late suspend" callback for given device.
+ * @dev: Device to handle.
+ * @state: PM transition of the system being carried out.
  *
- *	This is called with interrupts off and only a single CPU running.
+ * The driver of @dev will not receive interrupts while this fuction is being
+ * executed.
  */
 static int device_suspend_noirq(struct device *dev, pm_message_t state)
 {

@@ -596,13 +595,11 @@ static int device_suspend_noirq(struct d
 }
 
 /**
- *	dpm_suspend_noirq - Power down all regular (non-sysdev) devices.
- *	@state: PM transition of the system being carried out.
+ * dpm_suspend_noirq - Execute "late suspend" callbacks for non-sysdev devices.
+ * @state: PM transition of the system being carried out.
  *
- *	Prevent device drivers from receiving interrupts and call the "noirq"
- *	suspend handlers.
- *
- *	Must be called under dpm_list_mtx.
+ * Prevent device drivers from receiving interrupts and call the "noirq" suspend
+ * handlers for all non-sysdev devices.
  */
 int dpm_suspend_noirq(pm_message_t state)
 {

@@ -627,9 +624,9 @@ int dpm_suspend_noirq(pm_message_t state
 EXPORT_SYMBOL_GPL(dpm_suspend_noirq);
 
 /**
- *	device_suspend - Save state of one device.
- *	@dev:	Device.
- *	@state: PM transition of the system being carried out.
+ * device_suspend - Execute "suspend" callbacks for given device.
+ * @dev: Device to handle.
+ * @state: PM transition of the system being carried out.
  */
 static int device_suspend(struct device *dev, pm_message_t state)
 {

@@ -676,10 +673,8 @@ static int device_suspend(struct device 
 }
 
 /**
- *	dpm_suspend - Suspend every device.
- *	@state: PM transition of the system being carried out.
- *
- *	Execute the appropriate "suspend" callbacks for all devices.
+ * dpm_suspend - Execute "suspend" callbacks for all non-sysdev devices.
+ * @state: PM transition of the system being carried out.
  */
 static int dpm_suspend(pm_message_t state)
 {

@@ -713,9 +708,12 @@ static int dpm_suspend(pm_message_t stat
 }
 
 /**
- *	device_prepare - Execute the ->prepare() callback(s) for given device.
- *	@dev:	Device.
- *	@state: PM transition of the system being carried out.
+ * device_prepare - Prepare a device for system power transition.
+ * @dev: Device to handle.
+ * @state: PM transition of the system being carried out.
+ *
+ * Execute the ->prepare() callback(s) for given device.  No new children of the
+ * device may be registered after this function has returned.
  */
 static int device_prepare(struct device *dev, pm_message_t state)
 {

@@ -751,10 +749,10 @@ static int device_prepare(struct device 
 }
 
 /**
- *	dpm_prepare - Prepare all devices for a PM transition.
- *	@state: PM transition of the system being carried out.
+ * dpm_prepare - Prepare all non-sysdev devices for a system PM transition.
+ * @state: PM transition of the system being carried out.
  *
- *	Execute the ->prepare() callback for all devices.
+ * Execute the ->prepare() callback(s) for all devices.
  */
 static int dpm_prepare(pm_message_t state)
 {

@@ -805,10 +803,11 @@ static int dpm_prepare(pm_message_t stat
 }
 
 /**
- *	dpm_suspend_start - Save state and stop all devices in system.
- *	@state: PM transition of the system being carried out.
+ * dpm_suspend_start - Prepare devices for PM transition and suspend them.
+ * @state: PM transition of the system being carried out.
  *
- *	Prepare and suspend all devices.
+ * Prepare all non-sysdev devices for system PM transition and execute "suspend"
+ * callbacks for them.
  */
 int dpm_suspend_start(pm_message_t state)
 {
--

Quoted 3 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
Subject: PM: Update kerneldoc comments in drivers/base/power/main.c

Quoted 6 lines

The kerneldoc comments in drivers/base/power/main.c are generally outdated and some of them don't describe the functions very accurately. Update them and standardize the format to use spaces instead of tabs.

Signed-off-by Rafael J. Wysocki <[email protected]>

Any objections to this patch from anyone? Greg, Alan, Randy?

Best,
Rafael

Quoted 3 lines

---
 drivers/base/power/main.c |  169 ++++++++++++++++++++++------------------------
 1 file changed, 84 insertions(+), 85 deletions(-)

Quoted 3 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
Subject: PM: Update kerneldoc comments in drivers/base/power/main.c

Quoted 6 lines

The kerneldoc comments in drivers/base/power/main.c are generally outdated and some of them don't describe the functions very accurately. Update them and standardize the format to use spaces instead of tabs.

Signed-off-by Rafael J. Wysocki <[email protected]>

Any objections to this patch from anyone? Greg, Alan, Randy?

Best,
Rafael

Quoted 3 lines

---
 drivers/base/power/main.c |  169 ++++++++++++++++++++++------------------------
 1 file changed, 84 insertions(+), 85 deletions(-)

Quoted 3 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
Subject: PM: Update kerneldoc comments in drivers/base/power/main.c

Quoted 6 lines

The kerneldoc comments in drivers/base/power/main.c are generally outdated and some of them don't describe the functions very accurately. Update them and standardize the format to use spaces instead of tabs.

Signed-off-by Rafael J. Wysocki <[email protected]>

Any objections to this patch from anyone? Greg, Alan, Randy?

Quoted 3 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
Subject: PM: Update kerneldoc comments in drivers/base/power/main.c

Quoted 6 lines

The kerneldoc comments in drivers/base/power/main.c are generally outdated and some of them don't describe the functions very accurately. Update them and standardize the format to use spaces instead of tabs.

Signed-off-by Rafael J. Wysocki <[email protected]>

Any objections to this patch from anyone? Greg, Alan, Randy?

Quoted 3 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
Subject: PM: Update kerneldoc comments in drivers/base/power/main.c

Quoted 6 lines

The kerneldoc comments in drivers/base/power/main.c are generally outdated and some of them don't describe the functions very accurately. Update them and standardize the format to use spaces instead of tabs.

Signed-off-by Rafael J. Wysocki <[email protected]>

Any objections to this patch from anyone? Greg, Alan, Randy?

Quoted 10 lines

/**
- * pm_op - execute the PM operation appropiate for given PM event
- * @dev: Device.
- * @ops: PM operations to choose from.
- * @state: PM transition of the system being carried out.
+ * pm_op - Execute the PM operation appropiate for given PM event.
+ * @dev: Device to handle.
+ * @ops: PM operations to choose from.
+ * @state: PM transition of the system being carried out. */

Quoted 7 lines

/**
- * pm_noirq_op - execute the PM operation appropiate for given PM event
- * @dev: Device.
- * @ops: PM operations to choose from.
- * @state: PM transition of the system being carried out.
+ * pm_noirq_op - Execute the PM operation appropiate for given PM event.
+ * @dev: Device to handle.

Quoted 8 lines

+ * @ops: PM operations to choose from.
+ * @state: PM transition of the system being carried out.
*
- * The operation is executed with interrupts disabled by the only remaining
- * functional CPU in the system.
+ * The driver of @dev will not receive interrupts while this fuction is being
+ * executed.
*/

Quoted 11 lines

/**
- * dpm_resume - Resume every device.
- * @state: PM transition of the system being carried out.
+ * dpm_resume - Execute "resume" callbacks for non-sysdev devices.
+ * @state: PM transition of the system being carried out.
*
- * Execute the appropriate "resume" callback for all devices the status of
- * which indicates that they are inactive.
+ * Execute the appropriate "resume" callback for all devices the status of which
+ * indicates that they are suspended.
*/

Quoted 11 lines

/**
- * dpm_resume_end - Restore state of each device in system.
- * @state: PM transition of the system being carried out.
+ * dpm_resume_end - Execute "resume" callbacks and complete system transition.
+ * @state: PM transition of the system being carried out.
*
- * Resume all the devices, unlock them all, and allow new
- * devices to be registered once again.
+ * Resume all the devices, unlock them all, and complete the PM transition of
+ * the system.
*/

Quoted 9 lines · Rafael J. Wysocki

Quoted 8 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
Subject: PM: Update kerneldoc comments in drivers/base/power/main.c

The kerneldoc comments in drivers/base/power/main.c are generally outdated and some of them don't describe the functions very accurately. Update them and standardize the format to use spaces instead of tabs.

Quoted 3 lines

Quoted 1 line

Signed-off-by Rafael J. Wysocki <[email protected]>

Any objections to this patch from anyone? Greg, Alan, Randy?

Just some very minor changes.

Quoted 10 lines

Quoted 10 lines

/**
- * pm_op - execute the PM operation appropiate for given PM event
- * @dev: Device.
- * @ops: PM operations to choose from.
- * @state: PM transition of the system being carried out.
+ * pm_op - Execute the PM operation appropiate for given PM event.
+ * @dev: Device to handle.
+ * @ops: PM operations to choose from.
+ * @state: PM transition of the system being carried out. */

Here and in several places below, "appropriate" is missing an "r".

Quoted 7 lines

Quoted 7 lines

/**
- * pm_noirq_op - execute the PM operation appropiate for given PM event
- * @dev: Device.
- * @ops: PM operations to choose from.
- * @state: PM transition of the system being carried out.
+ * pm_noirq_op - Execute the PM operation appropiate for given PM event.
+ * @dev: Device to handle.

Quoted 8 lines

Quoted 8 lines

+ * @ops: PM operations to choose from.
+ * @state: PM transition of the system being carried out.
*
- * The operation is executed with interrupts disabled by the only remaining
- * functional CPU in the system.
+ * The driver of @dev will not receive interrupts while this fuction is being
+ * executed.
*/

Here and in several places below, "function" is missing an "n".

Quoted 11 lines

Quoted 11 lines

/**
- * dpm_resume - Resume every device.
- * @state: PM transition of the system being carried out.
+ * dpm_resume - Execute "resume" callbacks for non-sysdev devices.
+ * @state: PM transition of the system being carried out.
*
- * Execute the appropriate "resume" callback for all devices the status of
- * which indicates that they are inactive.
+ * Execute the appropriate "resume" callback for all devices the status of which
+ * indicates that they are suspended.
*/

The phrasing is slightly awkward. "... all devices whose status indicates..." would be better. Below as well.

Quoted 11 lines

Quoted 11 lines

/**
- * dpm_resume_end - Restore state of each device in system.
- * @state: PM transition of the system being carried out.
+ * dpm_resume_end - Execute "resume" callbacks and complete system transition.
+ * @state: PM transition of the system being carried out.
*
- * Resume all the devices, unlock them all, and allow new
- * devices to be registered once again.
+ * Resume all the devices, unlock them all, and complete the PM transition of
+ * the system.
*/

The "unlock them all" part is a fossil. It should be removed.