[PATCH] introduce /sys/power/disk_powerdown_mode

[PATCH] introduce /sys/power/disk_powerdown_mode

[Johannes Berg]

This patch introduces /sys/power/disk_powerdown_mode and
deprecates /sys/power/disk. The former has the advantages
 * a much more expressive name
 * contains all valid modes as well as the selected one
   in a fashion known from the LED subsystem.

The disk attribute is scheduled for removal, but since there
are no disadvantages in keeping we give it plenty of time.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

---
 Documentation/feature-removal-schedule.txt |   12 +++
 kernel/power/Kconfig                       |   13 ++++
 kernel/power/disk.c                        |   92 ++++++++++++++++++++++++++++-
 3 files changed, 114 insertions(+), 3 deletions(-)

--- linux-2.6.orig/kernel/power/disk.c	2007-03-21 08:21:18.726639252 +0100
+++ linux-2.6/kernel/power/disk.c	2007-03-21 10:16:21.802243895 +0100
@@ -307,7 +307,7 @@ static const char * const pm_disk_modes[
 };
 
 /**
- *	disk - Control suspend-to-disk mode
+ *	disk/disk_powerdown_mode - Control suspend-to-disk mode
  *
  *	Suspend-to-disk can be handled in several ways. We have a few options
  *	for putting the system to sleep - using the platform driver (e.g. ACPI
@@ -331,6 +331,7 @@ static const char * const pm_disk_modes[
  *	supports it (as determined from pm_ops->pm_disk_mode).
  */
 
+#ifdef CONFIG_PM_DISKATTR
 static ssize_t disk_show(struct subsystem * subsys, char * buf)
 {
 	return sprintf(buf, "%s\n", pm_disk_modes[pm_disk_mode]);
@@ -374,13 +375,95 @@ static ssize_t disk_store(struct subsyst
 		error = -EINVAL;
 	}
 
-	pr_debug("PM: suspend-to-disk mode set to '%s'\n",
-		 pm_disk_modes[mode]);
+	if (!error)
+		pr_debug("PM: suspend-to-disk mode set to '%s'\n",
+			 pm_disk_modes[mode]);
 	mutex_unlock(&pm_mutex);
 	return error ? error : n;
 }
 
 power_attr(disk);
+#endif
+
+static ssize_t disk_powerdown_mode_show(struct subsystem *subsys, char *buf)
+{
+	int i;
+	char *start = buf;
+
+	for (i = PM_DISK_PLATFORM; i < PM_DISK_MAX; i++) {
+		if (!pm_disk_modes[i])
+			continue;
+		switch (i) {
+		case PM_DISK_SHUTDOWN:
+		case PM_DISK_REBOOT:
+		case PM_DISK_TEST:
+		case PM_DISK_TESTPROC:
+			break;
+		default:
+			if (pm_ops && pm_ops->enter &&
+			    (i == pm_ops->pm_disk_mode))
+				break;
+			/* not a valid mode, continue with loop */
+			continue;
+		}
+		if (i == pm_disk_mode)
+			buf += sprintf(buf, "[%s]", pm_disk_modes[i]);
+		else
+			buf += sprintf(buf, "%s", pm_disk_modes[i]);
+		if (i+1 != PM_DISK_MAX)
+			buf += sprintf(buf, " ");
+	}
+	buf += sprintf(buf, "\n");
+	return buf-start;
+}
+
+
+static ssize_t disk_powerdown_mode_store(struct subsystem *s,
+					 const char *buf, size_t n)
+{
+	int error = 0;
+	int i;
+	int len;
+	char *p;
+	suspend_disk_method_t mode = 0;
+
+	p = memchr(buf, '\n', n);
+	len = p ? p - buf : n;
+
+	mutex_lock(&pm_mutex);
+	for (i = PM_DISK_PLATFORM; i < PM_DISK_MAX; i++) {
+		if (!strncmp(buf, pm_disk_modes[i], len)) {
+			mode = i;
+			break;
+		}
+	}
+	if (mode) {
+		switch (mode) {
+		case PM_DISK_SHUTDOWN:
+		case PM_DISK_REBOOT:
+		case PM_DISK_TEST:
+		case PM_DISK_TESTPROC:
+			pm_disk_mode = mode;
+			break;
+		default:
+			if (pm_ops && pm_ops->enter &&
+			    (mode == pm_ops->pm_disk_mode))
+				pm_disk_mode = mode;
+			else
+				error = -EINVAL;
+		}
+	} else {
+		error = -EINVAL;
+	}
+
+	if (!error)
+		pr_debug("PM: suspend-to-disk mode set to '%s'\n",
+			 pm_disk_modes[mode]);
+	mutex_unlock(&pm_mutex);
+	return error ? error : n;
+}
+
+power_attr(disk_powerdown_mode);
 
 static ssize_t resume_show(struct subsystem * subsys, char *buf)
 {
@@ -434,7 +517,10 @@ static ssize_t image_size_store(struct s
 power_attr(image_size);
 
 static struct attribute * g[] = {
+#ifdef CONFIG_PM_DISKATTR
 	&disk_attr.attr,
+#endif
+	&disk_powerdown_mode_attr.attr,
 	&resume_attr.attr,
 	&image_size_attr.attr,
 	NULL,
--- linux-2.6.orig/kernel/power/Kconfig	2007-03-21 08:27:18.496639252 +0100
+++ linux-2.6/kernel/power/Kconfig	2007-03-21 10:17:42.562243895 +0100
@@ -19,6 +19,19 @@ config PM
 	  will issue the hlt instruction if nothing is to be done, thereby
 	  sending the processor to sleep and saving power.
 
+config PM_DISKATTR
+	bool "disk attribute in /sys/power/"
+	depends on PM
+	default y
+	---help---
+	  The "disk" attribute in /sys/power/ is being deprecated in favour
+	  of the "disk_powerdown_mode" attribute because the former does not
+	  let userspace see which modes are possible and the name is not
+	  very expressive.
+
+	  Deselect this option to turn off the deprecated disk attribute.
+	  If unsure, leave this option enabled.
+
 config PM_LEGACY
 	bool "Legacy Power Management API (DEPRECATED)"
 	depends on PM
--- linux-2.6.orig/Documentation/feature-removal-schedule.txt	2007-03-21 10:11:52.452243895 +0100
+++ linux-2.6/Documentation/feature-removal-schedule.txt	2007-03-21 10:18:37.832243895 +0100
@@ -334,3 +334,15 @@ Why:	powermac supports proper generic pm
 Who:	Johannes Berg <johannes@sipsolutions.net>
 
 ---------------------------
+
+What:	/sys/power/disk
+When:	Set PM_DISKATTR default n in 2.6.24, remove in 2.6.26
+Files:	kernel/power/disk.c kernel/power/Kconfig
+Why:	/sys/power/disk is not a very expressive name for what it
+	really is: /sys/power/disk_powerdown_mode. Having introduced
+	the latter which also has the advantage that it contains all
+	valid modes as well as the currently selected mode,
+	/sys/power/disk becomes useless.
+Who:	Johannes Berg <johannes@sipsolutions.net>
+
+---------------------------

Re: [PATCH] introduce /sys/power/disk_powerdown_mode

[Rafael J. Wysocki]

On Wednesday, 21 March 2007 10:23, Johannes Berg wrote:
> This patch introduces /sys/power/disk_powerdown_mode

I don't think this name is the right one too, since the platform mode is not
exactly the power down mode (let alone test and testproc).

> and deprecates /sys/power/disk. The former has the advantages
>  * a much more expressive name

Well, please choose another name (I'd probably use disk_mode or something
like that, but 'disk' is well-documented, so why change it?).

>  * contains all valid modes as well as the selected one
>    in a fashion known from the LED subsystem.

Pleases review the documentation under Documentation/power and
Documentation/ABI and update it in accordance with the changes you're making.

Greetings,
Rafael

Re: [PATCH] introduce /sys/power/disk_powerdown_mode

[Johannes Berg]

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

On Wed, 2007-03-21 at 13:25 +0100, Rafael J. Wysocki wrote:
> On Wednesday, 21 March 2007 10:23, Johannes Berg wrote:
> > This patch introduces /sys/power/disk_powerdown_mode
> 
> I don't think this name is the right one too, since the platform mode is not
> exactly the power down mode (let alone test and testproc).
> 
> > and deprecates /sys/power/disk. The former has the advantages
> >  * a much more expressive name
> 
> Well, please choose another name (I'd probably use disk_mode or something
> like that, but 'disk' is well-documented, so why change it?).

I initially just had 'disk_mode' but then I remembered the confusion
about 'firmware' (which I just removed) and changed it. I can change it
again. Another point for changing it from disk was to let both APIs
coexist.

> >  * contains all valid modes as well as the selected one
> >    in a fashion known from the LED subsystem.
> 
> Pleases review the documentation under Documentation/power and
> Documentation/ABI and update it in accordance with the changes you're making.

Good point.

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] introduce /sys/power/disk_powerdown_mode

[Pavel Machek]

Hi!

> This patch introduces /sys/power/disk_powerdown_mode and
> deprecates /sys/power/disk. The former has the advantages
>  * a much more expressive name
>  * contains all valid modes as well as the selected one
>    in a fashion known from the LED subsystem.
> 
> The disk attribute is scheduled for removal, but since there
> are no disadvantages in keeping we give it plenty of time.

Does it really need to be this complex? Maybe we can disply change
/sys/power/disk format? Is the change required at all?
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] introduce /sys/power/disk_powerdown_mode

[Johannes Berg]

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

On Wed, 2007-03-21 at 13:29 +0100, Pavel Machek wrote:

> Does it really need to be this complex? Maybe we can disply change
> /sys/power/disk format? Is the change required at all?

Dunno. Does anything ever read it? Maybe the whole change isn't
necessary since this thing exists only as a hack for ACPI anyway, but if
anybody starts using it with an actual choice being able to see the
choices would imho be useful.

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] introduce /sys/power/disk_powerdown_mode

[Pavel Machek]

On Wed 2007-03-21 13:32:55, Johannes Berg wrote:
> On Wed, 2007-03-21 at 13:29 +0100, Pavel Machek wrote:
> 
> > Does it really need to be this complex? Maybe we can disply change
> > /sys/power/disk format? Is the change required at all?
> 
> Dunno. Does anything ever read it? Maybe the whole change isn't
> necessary since this thing exists only as a hack for ACPI anyway, but if
> anybody starts using it with an actual choice being able to see the
> choices would imho be useful.

I believe we can just show the list of choices, without changing the
name.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] introduce /sys/power/disk_powerdown_mode

[Johannes Berg]

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

On Wed, 2007-03-21 at 14:11 +0100, Pavel Machek wrote:
> On Wed 2007-03-21 13:32:55, Johannes Berg wrote:
> > On Wed, 2007-03-21 at 13:29 +0100, Pavel Machek wrote:
> > 
> > > Does it really need to be this complex? Maybe we can disply change
> > > /sys/power/disk format? Is the change required at all?
> > 
> > Dunno. Does anything ever read it? Maybe the whole change isn't
> > necessary since this thing exists only as a hack for ACPI anyway, but if
> > anybody starts using it with an actual choice being able to see the
> > choices would imho be useful.
> 
> I believe we can just show the list of choices, without changing the
> name.

Ok, I can simplify the patch down to that, the only thing about that is
that it breaks userspace ABI. Dunno how people react to that with
suspend though :)

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] introduce /sys/power/disk_powerdown_mode

[Pavel Machek]

Hi!

> > > > Does it really need to be this complex? Maybe we can disply change
> > > > /sys/power/disk format? Is the change required at all?
> > > 
> > > Dunno. Does anything ever read it? Maybe the whole change isn't
> > > necessary since this thing exists only as a hack for ACPI anyway, but if
> > > anybody starts using it with an actual choice being able to see the
> > > choices would imho be useful.
> > 
> > I believe we can just show the list of choices, without changing the
> > name.
> 
> Ok, I can simplify the patch down to that, the only thing about that is
> that it breaks userspace ABI. Dunno how people react to that with
> suspend though :)

I do not know... maybe you can just ask akpm first. I believe we can
do that.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] introduce /sys/power/disk_powerdown_mode

[Rafael J. Wysocki]

On Wednesday, 21 March 2007 14:17, Pavel Machek wrote:
> Hi!
> 
> > > > > Does it really need to be this complex? Maybe we can disply change
> > > > > /sys/power/disk format? Is the change required at all?
> > > > 
> > > > Dunno. Does anything ever read it? Maybe the whole change isn't
> > > > necessary since this thing exists only as a hack for ACPI anyway, but if
> > > > anybody starts using it with an actual choice being able to see the
> > > > choices would imho be useful.
> > > 
> > > I believe we can just show the list of choices, without changing the
> > > name.
> > 
> > Ok, I can simplify the patch down to that, the only thing about that is
> > that it breaks userspace ABI. Dunno how people react to that with
> > suspend though :)
> 
> I do not know... maybe you can just ask akpm first. I believe we can
> do that.

I agree, but _all_ of the doocumentation will have to be updated in the same
patch (or patch series), IMHO.

Greetings,
Rafael


-- 
If you don't have the time to read,
you don't have the time or the tools to write.
		- Stephen King

Re: [PATCH] introduce /sys/power/disk_powerdown_mode

[Pavel Machek]

Hi!

> > > > > > Does it really need to be this complex? Maybe we can disply change
> > > > > > /sys/power/disk format? Is the change required at all?
> > > > > 
> > > > > Dunno. Does anything ever read it? Maybe the whole change isn't
> > > > > necessary since this thing exists only as a hack for ACPI anyway, but if
> > > > > anybody starts using it with an actual choice being able to see the
> > > > > choices would imho be useful.
> > > > 
> > > > I believe we can just show the list of choices, without changing the
> > > > name.
> > > 
> > > Ok, I can simplify the patch down to that, the only thing about that is
> > > that it breaks userspace ABI. Dunno how people react to that with
> > > suspend though :)
> > 
> > I do not know... maybe you can just ask akpm first. I believe we can
> > do that.
> 
> I agree, but _all_ of the doocumentation will have to be updated in the same
> patch (or patch series), IMHO.

Yep.

(Stefan, we are talking about changing /sys/power/disk during _read_
so it shows list of available options as well as current one. I do not
expect that to break anything, but... if I'm wrong, just shout).
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-pm] [PATCH] introduce /sys/power/disk_powerdown_mode

[Stefan Seyfried]

On Wed, Mar 21, 2007 at 11:08:58PM +0100, Pavel Machek wrote:
 
> (Stefan, we are talking about changing /sys/power/disk during _read_
> so it shows list of available options as well as current one. I do not
> expect that to break anything, but... if I'm wrong, just shout).

I don't think it will break anything for me, and if it does, it should
be pretty easy to fix. So just go ahead.

But thanks for the warning in advance anyway :-)
-- 
Stefan Seyfried

"Any ideas, John?"
"Well, surrounding them's out." 

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

[PATCH] power management: change /sys/power/disk display

[Johannes Berg]

This patch changes /sys/power/disk to display all valid modes
as well as the currently selected one in a fashion known from
the LED subsystem.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

---
This patch is the simplified version that just changes the attribute
instead of introducing a new on. It also contains the relevant updates
to the documentation.

Good to push for .22?

 Documentation/power/interface.txt |    8 ++++++--
 kernel/power/disk.c               |   34 +++++++++++++++++++++++++++++++---
 2 files changed, 37 insertions(+), 5 deletions(-)

--- linux-2.6.orig/kernel/power/disk.c	2007-03-22 13:56:20.184654656 +0100
+++ linux-2.6/kernel/power/disk.c	2007-03-22 14:00:18.754654656 +0100
@@ -333,7 +333,34 @@ static const char * const pm_disk_modes[
 
 static ssize_t disk_show(struct subsystem * subsys, char * buf)
 {
-	return sprintf(buf, "%s\n", pm_disk_modes[pm_disk_mode]);
+	int i;
+	char *start = buf;
+
+	for (i = PM_DISK_PLATFORM; i < PM_DISK_MAX; i++) {
+		if (!pm_disk_modes[i])
+			continue;
+		switch (i) {
+		case PM_DISK_SHUTDOWN:
+		case PM_DISK_REBOOT:
+		case PM_DISK_TEST:
+		case PM_DISK_TESTPROC:
+			break;
+		default:
+			if (pm_ops && pm_ops->enter &&
+			    (i == pm_ops->pm_disk_mode))
+				break;
+			/* not a valid mode, continue with loop */
+			continue;
+		}
+		if (i == pm_disk_mode)
+			buf += sprintf(buf, "[%s]", pm_disk_modes[i]);
+		else
+			buf += sprintf(buf, "%s", pm_disk_modes[i]);
+		if (i+1 != PM_DISK_MAX)
+			buf += sprintf(buf, " ");
+	}
+	buf += sprintf(buf, "\n");
+	return buf-start;
 }
 
 
@@ -374,8 +401,9 @@ static ssize_t disk_store(struct subsyst
 		error = -EINVAL;
 	}
 
-	pr_debug("PM: suspend-to-disk mode set to '%s'\n",
-		 pm_disk_modes[mode]);
+	if (!error)
+		pr_debug("PM: suspend-to-disk mode set to '%s'\n",
+			 pm_disk_modes[mode]);
 	mutex_unlock(&pm_mutex);
 	return error ? error : n;
 }
--- linux-2.6.orig/Documentation/power/interface.txt	2007-03-22 14:01:09.684654656 +0100
+++ linux-2.6/Documentation/power/interface.txt	2007-03-22 14:02:22.984654656 +0100
@@ -34,8 +34,12 @@ for 5 seconds, resume devices, unfreeze 
 we are able to look in the log messages and work out, for example, which code
 is being slow and which device drivers are misbehaving.
 
-Reading from this file will display what the mode is currently set
-to. Writing to this file will accept one of
+Reading from this file will display all supported modes and the currently
+selected one in brackets, for example
+
+	[shutdown] reboot test testproc
+
+Writing to this file will accept one of
 
        'platform' (only if the platform supports it)
        'shutdown'

Re: [PATCH] power management: change /sys/power/disk display

[Rafael J. Wysocki]

On Thursday, 22 March 2007 15:34, Johannes Berg wrote:
> This patch changes /sys/power/disk to display all valid modes
> as well as the currently selected one in a fashion known from
> the LED subsystem.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

No objections from me.

> ---
> This patch is the simplified version that just changes the attribute
> instead of introducing a new on. It also contains the relevant updates
> to the documentation.
> 
> Good to push for .22?

Well, I think it can go to -mm for now (if Pavel agrees with it).  Later we'll
decide whether it should go into .22 or .23.

Greetings,
Rafael

Re: [PATCH] power management: change /sys/power/disk display

[Pavel Machek]

On Thu 2007-03-22 15:34:11, Johannes Berg wrote:
> This patch changes /sys/power/disk to display all valid modes
> as well as the currently selected one in a fashion known from
> the LED subsystem.

looks ok to me. It changes userland interface, but I believe noone
uses this particular detail.


> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

ack.

> ---
> This patch is the simplified version that just changes the attribute
> instead of introducing a new on. It also contains the relevant updates
> to the documentation.
> 
> Good to push for .22?

Actually, this may even be .23 material.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] power management: change /sys/power/disk display

[Johannes Berg]

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

On Fri, 2007-03-23 at 01:05 +0100, Pavel Machek wrote:

> Actually, this may even be .23 material.

Fine with me, I have no particular need for this. Just don't want to
track the patch forever ;)

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: change /sys/power/disk display

[Pavel Machek]

On Fri 2007-03-23 14:48:47, Johannes Berg wrote:
> On Fri, 2007-03-23 at 01:05 +0100, Pavel Machek wrote:
> 
> > Actually, this may even be .23 material.
> 
> Fine with me, I have no particular need for this. Just don't want to
> track the patch forever ;)

Send it to akpm, he's good at tracking :).
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] power management: change /sys/power/disk display

[Johannes Berg]

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

On Sat, 2007-03-24 at 00:46 +0100, Pavel Machek wrote:

> Send it to akpm, he's good at tracking :).

Heh. But then I'd have to know what kernel we want it in ;)

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: change /sys/power/disk display

[Pavel Machek]

On Sat 2007-03-24 00:53:41, Johannes Berg wrote:
> On Sat, 2007-03-24 at 00:46 +0100, Pavel Machek wrote:
> 
> > Send it to akpm, he's good at tracking :).
> 
> Heh. But then I'd have to know what kernel we want it in ;)

No, just send him patch, and let the decision when to push it on me
and him.
								Pavel



-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] power management: change /sys/power/disk display

[Johannes Berg]

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

On Sat, 2007-03-24 at 01:14 +0100, Pavel Machek wrote:
> On Sat 2007-03-24 00:53:41, Johannes Berg wrote:
> > On Sat, 2007-03-24 at 00:46 +0100, Pavel Machek wrote:
> > 
> > > Send it to akpm, he's good at tracking :).
> > 
> > Heh. But then I'd have to know what kernel we want it in ;)
> 
> No, just send him patch, and let the decision when to push it on me
> and him.

Ok, that works too, will do that later then.

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 --]