[PATCH] power management: no valid states w/o pm_ops + docs

[PATCH] power management: no valid states w/o pm_ops + docs

[Johannes Berg]

This patch changes /sys/power/state to not advertise any valid states
(except for disk if SOFTWARE_SUSPEND is enabled) when no pm_ops have been
set so userspace can easily discover what states should be available.

Also, because the pm ops in powermac are obviously not using them as
intended, I added documentation for it in kernel-doc format.

Reordering the fields in struct pm_ops not only makes the output of
kernel-doc make more sense but also removes a hole from the structure on
64-bit platforms.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Acked-by: Pavel Macheck <pavel@ucw.cz>
Cc: Rafael J. Wysocki <rjw@sisk.pl>
Cc: linux-pm@lists.osdl.org

---
 include/linux/pm.h  |   36 ++++++++++++++++++++++++++++++++++--
 kernel/power/main.c |    5 ++++-
 2 files changed, 38 insertions(+), 3 deletions(-)

--- linux-2.6-git.orig/kernel/power/main.c	2007-02-08 19:23:44.179643144 +0100
+++ linux-2.6-git/kernel/power/main.c	2007-02-08 19:23:54.201643144 +0100
@@ -161,7 +161,10 @@ static inline int valid_state(suspend_st
 	if (state == PM_SUSPEND_DISK)
 		return 1;
 
-	if (pm_ops && pm_ops->valid && !pm_ops->valid(state))
+	/* all other states need lowlevel support and need to be
+	 * valid to the lowlevel implementation, no valid callback
+	 * implies that all are valid. */
+	if (!pm_ops || (pm_ops->valid && !pm_ops->valid(state)))
 		return 0;
 	return 1;
 }
--- linux-2.6-git.orig/include/linux/pm.h	2007-02-08 19:23:44.219643144 +0100
+++ linux-2.6-git/include/linux/pm.h	2007-02-08 19:23:54.204643144 +0100
@@ -120,15 +120,47 @@ typedef int __bitwise suspend_disk_metho
 #define	PM_DISK_TESTPROC	((__force suspend_disk_method_t) 6)
 #define	PM_DISK_MAX		((__force suspend_disk_method_t) 7)
 
+/**
+ * struct pm_ops
+ *
+ * Callbacks for managing platform dependent suspend states.
+ *
+ * @valid: Callback to determine whether the given state can be entered.
+ *         If %CONFIG_SOFTWARE_SUSPEND is set then %PM_SUSPEND_DISK is
+ *         always valid and never passed to this call.
+ *         If not assigned, all suspend states are advertised as valid
+ *         in /sys/power/state (but can still be rejected by prepare or enter.)
+ * @prepare: Prepare the platform for the given suspend state. Can return a
+ *           negative error code if necessary.
+ * @enter: Enter the given suspend state, must be assigned. Can return a
+ *         negative error code if necessary.
+ * @finish: Called when the system has left the given state and all devices
+ *          are resumed. The return value is ignored.
+ * @pm_disk_mode: Set to the disk method that the user should be able to
+ *                configure for suspend-to-disk. Since %PM_DISK_SHUTDOWN,
+ *                %PM_DISK_REBOOT, %PM_DISK_TEST and %PM_DISK_TESTPROC
+ *                are always allowed, currently only %PM_DISK_PLATFORM
+ *                makes sense. If the user then choses %PM_DISK_PLATFORM,
+ *                the @prepare call will be called before suspending to disk
+ *                (if present), the @enter call should be present and will
+ *                be called after all state has been saved and the machine
+ *                is ready to be shut down/suspended/..., and the @finish
+ *                callback is called after state has been restored. All
+ *                these calls are called with %PM_SUSPEND_DISK as the state.
+ */
 struct pm_ops {
-	suspend_disk_method_t pm_disk_mode;
 	int (*valid)(suspend_state_t state);
 	int (*prepare)(suspend_state_t state);
 	int (*enter)(suspend_state_t state);
 	int (*finish)(suspend_state_t state);
+	suspend_disk_method_t pm_disk_mode;
 };
 
-extern void pm_set_ops(struct pm_ops *);
+/**
+ * pm_set_ops - set platform dependent power management ops
+ * @pm_ops: The new power management operations to set.
+ */
+extern void pm_set_ops(struct pm_ops *pm_ops);
 extern struct pm_ops *pm_ops;
 extern int pm_suspend(suspend_state_t state);
 

Re: [PATCH] power management: no valid states w/o pm_ops + docs

[Randy Dunlap]

On Tue, 13 Feb 2007 17:26:20 +0100 Johannes Berg wrote:

> This patch changes /sys/power/state to not advertise any valid states
> (except for disk if SOFTWARE_SUSPEND is enabled) when no pm_ops have been
> set so userspace can easily discover what states should be available.
> 
> Also, because the pm ops in powermac are obviously not using them as
> intended, I added documentation for it in kernel-doc format.

Thanks... but it's not quite in kernel-doc format.
Did you test it?
See Documentation/kernel-doc-nano-HOWTO.txt for details.

> Reordering the fields in struct pm_ops not only makes the output of
> kernel-doc make more sense but also removes a hole from the structure on
> 64-bit platforms.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> Acked-by: Pavel Macheck <pavel@ucw.cz>
> Cc: Rafael J. Wysocki <rjw@sisk.pl>
> Cc: linux-pm@lists.osdl.org
> 
> ---
>  include/linux/pm.h  |   36 ++++++++++++++++++++++++++++++++++--
>  kernel/power/main.c |    5 ++++-
>  2 files changed, 38 insertions(+), 3 deletions(-)
> 
> --- linux-2.6-git.orig/include/linux/pm.h	2007-02-08 19:23:44.219643144 +0100
> +++ linux-2.6-git/include/linux/pm.h	2007-02-08 19:23:54.204643144 +0100
> @@ -120,15 +120,47 @@ typedef int __bitwise suspend_disk_metho
>  #define	PM_DISK_TESTPROC	((__force suspend_disk_method_t) 6)
>  #define	PM_DISK_MAX		((__force suspend_disk_method_t) 7)
>  
> +/**
> + * struct pm_ops
> + *
> + * Callbacks for managing platform dependent suspend states.
> + *

 * struct pm_ops - callbacks for managing platform-dependent suspend states

> + * @valid: Callback to determine whether the given state can be entered.
> + *         If %CONFIG_SOFTWARE_SUSPEND is set then %PM_SUSPEND_DISK is
> + *         always valid and never passed to this call.
> + *         If not assigned, all suspend states are advertised as valid
> + *         in /sys/power/state (but can still be rejected by prepare or enter.)
> + * @prepare: Prepare the platform for the given suspend state. Can return a
> + *           negative error code if necessary.
> + * @enter: Enter the given suspend state, must be assigned. Can return a
> + *         negative error code if necessary.
> + * @finish: Called when the system has left the given state and all devices
> + *          are resumed. The return value is ignored.
> + * @pm_disk_mode: Set to the disk method that the user should be able to
> + *                configure for suspend-to-disk. Since %PM_DISK_SHUTDOWN,
> + *                %PM_DISK_REBOOT, %PM_DISK_TEST and %PM_DISK_TESTPROC
> + *                are always allowed, currently only %PM_DISK_PLATFORM
> + *                makes sense. If the user then choses %PM_DISK_PLATFORM,
> + *                the @prepare call will be called before suspending to disk
> + *                (if present), the @enter call should be present and will
> + *                be called after all state has been saved and the machine
> + *                is ready to be shut down/suspended/..., and the @finish
> + *                callback is called after state has been restored. All
> + *                these calls are called with %PM_SUSPEND_DISK as the state.

We usually just indent the following lines the same for all function
parameters or struct members.

> + */
>  struct pm_ops {
> -	suspend_disk_method_t pm_disk_mode;
>  	int (*valid)(suspend_state_t state);
>  	int (*prepare)(suspend_state_t state);
>  	int (*enter)(suspend_state_t state);
>  	int (*finish)(suspend_state_t state);
> +	suspend_disk_method_t pm_disk_mode;
>  };


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] power management: no valid states w/o pm_ops + docs

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 2957 bytes --]

On Tue, 2007-02-13 at 09:23 -0800, Randy Dunlap wrote:
> On Tue, 13 Feb 2007 17:26:20 +0100 Johannes Berg wrote:
> 
> > This patch changes /sys/power/state to not advertise any valid states
> > (except for disk if SOFTWARE_SUSPEND is enabled) when no pm_ops have been
> > set so userspace can easily discover what states should be available.
> > 
> > Also, because the pm ops in powermac are obviously not using them as
> > intended, I added documentation for it in kernel-doc format.
> 
> Thanks... but it's not quite in kernel-doc format.

Hmm ok :)

> Did you test it?

Yes! The output (in html) looked fine to me.

> > +/**
> > + * struct pm_ops
> > + *
> > + * Callbacks for managing platform dependent suspend states.
> > + *
> 
>  * struct pm_ops - callbacks for managing platform-dependent suspend states

But then the description "callbacks for ..." disappears completely in
the HTML output! Should I duplicate it or something?

> > + * @valid: Callback to determine whether the given state can be entered.
> > + *         If %CONFIG_SOFTWARE_SUSPEND is set then %PM_SUSPEND_DISK is
> > + *         always valid and never passed to this call.
> > + *         If not assigned, all suspend states are advertised as valid
> > + *         in /sys/power/state (but can still be rejected by prepare or enter.)
> > + * @prepare: Prepare the platform for the given suspend state. Can return a
> > + *           negative error code if necessary.
> > + * @enter: Enter the given suspend state, must be assigned. Can return a
> > + *         negative error code if necessary.
> > + * @finish: Called when the system has left the given state and all devices
> > + *          are resumed. The return value is ignored.
> > + * @pm_disk_mode: Set to the disk method that the user should be able to
> > + *                configure for suspend-to-disk. Since %PM_DISK_SHUTDOWN,
> > + *                %PM_DISK_REBOOT, %PM_DISK_TEST and %PM_DISK_TESTPROC
> > + *                are always allowed, currently only %PM_DISK_PLATFORM
> > + *                makes sense. If the user then choses %PM_DISK_PLATFORM,
> > + *                the @prepare call will be called before suspending to disk
> > + *                (if present), the @enter call should be present and will
> > + *                be called after all state has been saved and the machine
> > + *                is ready to be shut down/suspended/..., and the @finish
> > + *                callback is called after state has been restored. All
> > + *                these calls are called with %PM_SUSPEND_DISK as the state.
> 
> We usually just indent the following lines the same for all function
> parameters or struct members.

So I should indent them to the deepest indent I have like this?

 @x:         description for x
             in two lines
 @verylong:  description for verylong
             in two lines

Thanks,
Johannes

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] power management: no valid states w/o pm_ops + docs

[Randy Dunlap]

Johannes Berg wrote:
> On Tue, 2007-02-13 at 09:23 -0800, Randy Dunlap wrote:
>> On Tue, 13 Feb 2007 17:26:20 +0100 Johannes Berg wrote:
>>
>>> This patch changes /sys/power/state to not advertise any valid states
>>> (except for disk if SOFTWARE_SUSPEND is enabled) when no pm_ops have been
>>> set so userspace can easily discover what states should be available.
>>>
>>> Also, because the pm ops in powermac are obviously not using them as
>>> intended, I added documentation for it in kernel-doc format.
>> Thanks... but it's not quite in kernel-doc format.
> 
> Hmm ok :)
> 
>> Did you test it?
> 
> Yes! The output (in html) looked fine to me.

Great.  Some people don't even test.  :(

>>> +/**
>>> + * struct pm_ops
>>> + *
>>> + * Callbacks for managing platform dependent suspend states.
>>> + *
>>  * struct pm_ops - callbacks for managing platform-dependent suspend states
> 
> But then the description "callbacks for ..." disappears completely in
> the HTML output! Should I duplicate it or something?

No, it's right there by the struct name (top line).
As you have it, it shows up at the bottom, after the struct members,
doesn't it (i.e., out of order)?

>>> + * @valid: Callback to determine whether the given state can be entered.
>>> + *         If %CONFIG_SOFTWARE_SUSPEND is set then %PM_SUSPEND_DISK is
>>> + *         always valid and never passed to this call.
>>> + *         If not assigned, all suspend states are advertised as valid
>>> + *         in /sys/power/state (but can still be rejected by prepare or enter.)
>>> + * @prepare: Prepare the platform for the given suspend state. Can return a
>>> + *           negative error code if necessary.
>>> + * @enter: Enter the given suspend state, must be assigned. Can return a
>>> + *         negative error code if necessary.
>>> + * @finish: Called when the system has left the given state and all devices
>>> + *          are resumed. The return value is ignored.
>>> + * @pm_disk_mode: Set to the disk method that the user should be able to
>>> + *                configure for suspend-to-disk. Since %PM_DISK_SHUTDOWN,
>>> + *                %PM_DISK_REBOOT, %PM_DISK_TEST and %PM_DISK_TESTPROC
>>> + *                are always allowed, currently only %PM_DISK_PLATFORM
>>> + *                makes sense. If the user then choses %PM_DISK_PLATFORM,
>>> + *                the @prepare call will be called before suspending to disk
>>> + *                (if present), the @enter call should be present and will
>>> + *                be called after all state has been saved and the machine
>>> + *                is ready to be shut down/suspended/..., and the @finish
>>> + *                callback is called after state has been restored. All
>>> + *                these calls are called with %PM_SUSPEND_DISK as the state.
>> We usually just indent the following lines the same for all function
>> parameters or struct members.
> 
> So I should indent them to the deepest indent I have like this?
> 
>  @x:         description for x
>              in two lines
>  @verylong:  description for verylong
>              in two lines

I would probably just indent all of them by one tab.  Don't worry about
the @verylong one.

-- 
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] power management: no valid states w/o pm_ops + docs

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 1137 bytes --]

On Tue, 2007-02-13 at 09:41 -0800, Randy Dunlap wrote:

> >>  * struct pm_ops - callbacks for managing platform-dependent suspend states
> > 
> > But then the description "callbacks for ..." disappears completely in
> > the HTML output! Should I duplicate it or something?
> 
> No, it's right there by the struct name (top line).
> As you have it, it shows up at the bottom, after the struct members,
> doesn't it (i.e., out of order)?

I now have:

/**
 * struct pm_ops - Callbacks for managing platform dependent suspend states.
 *
 *... (rest kept identical)
 */

and it's not there for me (output of
  ./scripts/kernel-doc -html include/linux/pm.h
attached). I also get an empty "Description" section.

But yes, if I put it into the third line then it shows up in the
"Description" section which is all the way down.

I think I'll put this down to a bug in the html output (that maybe you
get to fix? :) ), I just tested text and man output now and they both
look fine as you describe.

> I would probably just indent all of them by one tab.  Don't worry about
> the @verylong one.

ok.

johannes

[-- Attachment #1.2: pm.html --]
[-- Type: text/html, Size: 2115 bytes --]

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] power management: no valid states w/o pm_ops + docs

[Randy Dunlap]

Johannes Berg wrote:
> On Tue, 2007-02-13 at 09:41 -0800, Randy Dunlap wrote:
> 
>>>>  * struct pm_ops - callbacks for managing platform-dependent suspend states
>>> But then the description "callbacks for ..." disappears completely in
>>> the HTML output! Should I duplicate it or something?
>> No, it's right there by the struct name (top line).
>> As you have it, it shows up at the bottom, after the struct members,
>> doesn't it (i.e., out of order)?
> 
> I now have:
> 
> /**
>  * struct pm_ops - Callbacks for managing platform dependent suspend states.
>  *
>  *... (rest kept identical)
>  */
> 
> and it's not there for me (output of
>   ./scripts/kernel-doc -html include/linux/pm.h
> attached). I also get an empty "Description" section.
> 
> But yes, if I put it into the third line then it shows up in the
> "Description" section which is all the way down.
> 
> I think I'll put this down to a bug in the html output (that maybe you
> get to fix? :) ), I just tested text and man output now and they both
> look fine as you describe.

Uh, please try one more thing.  Delete the '*' line between
 * struct pm_ops - Callbacks ...

and the struct members list and retest that.

-- 
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] power management: no valid states w/o pm_ops + docs

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 412 bytes --]

On Tue, 2007-02-13 at 09:57 -0800, Randy Dunlap wrote:

> Uh, please try one more thing.  Delete the '*' line between
>  * struct pm_ops - Callbacks ...
> 
> and the struct members list and retest that.

Yeah, that causes the empty description to disappear (which actually is
also present in -text output)...

but doesn't cause the "Callbacks ..." text to appear anywhere in html
output.

johannes

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] power management: no valid states w/o pm_ops + docs

[Randy Dunlap]

Johannes Berg wrote:
> On Tue, 2007-02-13 at 09:57 -0800, Randy Dunlap wrote:
> 
>> Uh, please try one more thing.  Delete the '*' line between
>>  * struct pm_ops - Callbacks ...
>>
>> and the struct members list and retest that.
> 
> Yeah, that causes the empty description to disappear (which actually is
> also present in -text output)...
> 
> but doesn't cause the "Callbacks ..." text to appear anywhere in html
> output.

OK, I'll work on it.  Thanks.

-- 
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] power management: no valid states w/o pm_ops + docs

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 657 bytes --]

On Tue, 2007-02-13 at 10:03 -0800, Randy Dunlap wrote:
> Johannes Berg wrote:
> > On Tue, 2007-02-13 at 09:57 -0800, Randy Dunlap wrote:
> > 
> >> Uh, please try one more thing.  Delete the '*' line between
> >>  * struct pm_ops - Callbacks ...
> >>
> >> and the struct members list and retest that.
> > 
> > Yeah, that causes the empty description to disappear (which actually is
> > also present in -text output)...
> > 
> > but doesn't cause the "Callbacks ..." text to appear anywhere in html
> > output.
> 
> OK, I'll work on it.  Thanks.

Ok. I'll send the patch modified according to your comments in a second.
Thanks.

johannes

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH] power management: no valid states w/o pm_ops + docs

[Johannes Berg]

This patch changes /sys/power/state to not advertise any valid states
(except for disk if SOFTWARE_SUSPEND is enabled) when no pm_ops have been
set so userspace can easily discover what states should be available.

Also, because the pm ops in powermac are obviously not using them as
intended, I added documentation for it in kernel-doc format.

Reordering the fields in struct pm_ops not only makes the output of
kernel-doc make more sense but also removes a hole from the structure on
64-bit platforms.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Cc: linux-pm@lists.osdl.org
Cc: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Pavel Macheck <pavel@ucw.cz>

---
 include/linux/pm.h  |   37 +++++++++++++++++++++++++++++++++++--
 kernel/power/main.c |    5 ++++-
 2 files changed, 39 insertions(+), 3 deletions(-)

--- linux-2.6.orig/kernel/power/main.c	2007-02-09 15:23:30.413842184 +0100
+++ linux-2.6/kernel/power/main.c	2007-02-13 18:32:12.876706009 +0100
@@ -161,7 +161,10 @@ static inline int valid_state(suspend_st
 	if (state == PM_SUSPEND_DISK)
 		return 1;
 
-	if (pm_ops && pm_ops->valid && !pm_ops->valid(state))
+	/* all other states need lowlevel support and need to be
+	 * valid to the lowlevel implementation, no valid callback
+	 * implies that all are valid. */
+	if (!pm_ops || (pm_ops->valid && !pm_ops->valid(state)))
 		return 0;
 	return 1;
 }
--- linux-2.6.orig/include/linux/pm.h	2007-02-09 15:23:30.503842184 +0100
+++ linux-2.6/include/linux/pm.h	2007-02-13 19:02:46.157177125 +0100
@@ -120,15 +120,48 @@ typedef int __bitwise suspend_disk_metho
 #define	PM_DISK_TESTPROC	((__force suspend_disk_method_t) 6)
 #define	PM_DISK_MAX		((__force suspend_disk_method_t) 7)
 
+/**
+ * struct pm_ops - Callbacks for managing platform dependent suspend states.
+ * @valid: Callback to determine whether the given state can be entered.
+ * 	If %CONFIG_SOFTWARE_SUSPEND is set then %PM_SUSPEND_DISK is
+ *	always valid and never passed to this call.
+ *	If not assigned, all suspend states are advertised as valid
+ *	in /sys/power/state (but can still be rejected by prepare or enter.)
+ *
+ * @prepare: Prepare the platform for the given suspend state. Can return a
+ *	negative error code if necessary.
+ *
+ * @enter: Enter the given suspend state, must be assigned. Can return a
+ *	negative error code if necessary.
+ *
+ * @finish: Called when the system has left the given state and all devices
+ *	are resumed. The return value is ignored.
+ *
+ * @pm_disk_mode: Set to the disk method that the user should be able to
+ *	configure for suspend-to-disk. Since %PM_DISK_SHUTDOWN,
+ *	%PM_DISK_REBOOT, %PM_DISK_TEST and %PM_DISK_TESTPROC
+ *	are always allowed, currently only %PM_DISK_PLATFORM
+ *	makes sense. If the user then choses %PM_DISK_PLATFORM,
+ *	the @prepare call will be called before suspending to disk
+ *	(if present), the @enter call should be present and will
+ *	be called after all state has been saved and the machine
+ *	is ready to be shut down/suspended/..., and the @finish
+ *	callback is called after state has been restored. All
+ *	these calls are called with %PM_SUSPEND_DISK as the state.
+ */
 struct pm_ops {
-	suspend_disk_method_t pm_disk_mode;
 	int (*valid)(suspend_state_t state);
 	int (*prepare)(suspend_state_t state);
 	int (*enter)(suspend_state_t state);
 	int (*finish)(suspend_state_t state);
+	suspend_disk_method_t pm_disk_mode;
 };
 
-extern void pm_set_ops(struct pm_ops *);
+/**
+ * pm_set_ops - set platform dependent power management ops
+ * @pm_ops: The new power management operations to set.
+ */
+extern void pm_set_ops(struct pm_ops *pm_ops);
 extern struct pm_ops *pm_ops;
 extern int pm_suspend(suspend_state_t state);