[stable -3.14] PM backports for pm_test / CONFIG_PM_DEBUG

[stable -3.14] PM backports for pm_test / CONFIG_PM_DEBUG

[Brian Norris]

Hi,

If I enable CONFIG_PM_DEBUG on a 3.14.y kernel, I can see the following
results:

    # cat /sys/power/state
    freeze standby mem
    # cat /sys/power/pm_test
    [none] core processors platform devices freezer
    # echo core > /sys/power/pm_test
    # cat /sys/power/state
    [   22.581289] Unsupported pm_test mode for freeze state, please choose none/freezer/devices/platform.
    standby mem

Note how 'freeze' is dropped from the supported states, and I get an
extra printk message when I'm just checking if the state is valid.

It looks like if I backport a few fixes, I get more sane behavior, where
the warnings (and -EAGAIN) appear only when I actually try to enter an
unsupported test mode:

    # echo core > /sys/power/pm_test
    # cat /sys/power/state
    freeze standby mem
    # echo freeze > /sys/power/state
    [   27.833141] PM: Unsupported test mode for freeze state,please choose none/freezer/devices/platform.
    sh: echo: write error: Resource temporarily unavailable

I think I've narrowed it down to this commit that should be backported
to -stable (for 3.9+?):

    43e8317b0bba PM / sleep: Use valid_state() for platform-dependent sleep states only

But it needs to drag along this dependency too:

    27ddcc6596e5 PM / sleep: Add state field to pm_states[] entries

While at this, it looks like this commit might be deserving of -stable
(3.9+). I haven't tested this one, as I don't have a good freeze+cpuidle
setup; it just shows up in the commit log with an interesting
description and a 'Fixes' tag:

    f3f125324fc1 PM / suspend: Make cpuidle work in the "freeze" state

Regards,
Brian

Re: [stable -3.14] PM backports for pm_test / CONFIG_PM_DEBUG

[Rafael J. Wysocki]

On Wednesday, September 03, 2014 06:02:19 PM Brian Norris wrote:
> Hi,
> 
> If I enable CONFIG_PM_DEBUG on a 3.14.y kernel, I can see the following
> results:
> 
>     # cat /sys/power/state
>     freeze standby mem
>     # cat /sys/power/pm_test
>     [none] core processors platform devices freezer
>     # echo core > /sys/power/pm_test
>     # cat /sys/power/state
>     [   22.581289] Unsupported pm_test mode for freeze state, please choose none/freezer/devices/platform.
>     standby mem
> 
> Note how 'freeze' is dropped from the supported states, and I get an
> extra printk message when I'm just checking if the state is valid.
> 
> It looks like if I backport a few fixes, I get more sane behavior, where
> the warnings (and -EAGAIN) appear only when I actually try to enter an
> unsupported test mode:
> 
>     # echo core > /sys/power/pm_test
>     # cat /sys/power/state
>     freeze standby mem
>     # echo freeze > /sys/power/state
>     [   27.833141] PM: Unsupported test mode for freeze state,please choose none/freezer/devices/platform.
>     sh: echo: write error: Resource temporarily unavailable
> 
> I think I've narrowed it down to this commit that should be backported
> to -stable (for 3.9+?):
> 
>     43e8317b0bba PM / sleep: Use valid_state() for platform-dependent sleep states only
> 
> But it needs to drag along this dependency too:
> 
>     27ddcc6596e5 PM / sleep: Add state field to pm_states[] entries

That sounds OK.

Can you please send a formal request to include the two commits above to Greg/stable?

> While at this, it looks like this commit might be deserving of -stable
> (3.9+). I haven't tested this one, as I don't have a good freeze+cpuidle
> setup; it just shows up in the commit log with an interesting
> description and a 'Fixes' tag:
> 
>     f3f125324fc1 PM / suspend: Make cpuidle work in the "freeze" state

Well, it basically makes the freeze state save energy, but it's not essential.

I'm not seeing the "Fixes:" tag on it, though.

Rafael

Re: [stable -3.14] PM backports for pm_test / CONFIG_PM_DEBUG

[Brian Norris]

On Thu, Sep 04, 2014 at 09:40:41PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, September 03, 2014 06:02:19 PM Brian Norris wrote:
> > I think I've narrowed it down to this commit that should be backported
> > to -stable (for 3.9+?):
> > 
> >     43e8317b0bba PM / sleep: Use valid_state() for platform-dependent sleep states only
> > 
> > But it needs to drag along this dependency too:
> > 
> >     27ddcc6596e5 PM / sleep: Add state field to pm_states[] entries
> 
> That sounds OK.
> 
> Can you please send a formal request to include the two commits above to Greg/stable?

Sure. I've never done a post-merge -stable request; the format isn't
100% clear, but I'll give it a go.

> > While at this, it looks like this commit might be deserving of -stable
> > (3.9+). I haven't tested this one, as I don't have a good freeze+cpuidle
> > setup; it just shows up in the commit log with an interesting
> > description and a 'Fixes' tag:
> > 
> >     f3f125324fc1 PM / suspend: Make cpuidle work in the "freeze" state
> 
> Well, it basically makes the freeze state save energy, but it's not essential.
> 
> I'm not seeing the "Fixes:" tag on it, though.

Oops, I must have been looking at a different commit. Sorry!

Brian

[PATCH 3.10.y+] PM / sleep: Use valid_state() for platform-dependent sleep states only

[Brian Norris]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

[Upstream commit 43e8317b0bba1d6eb85f38a4a233d82d7c20d732]

Use the observation that, for platform-dependent sleep states
(PM_SUSPEND_STANDBY, PM_SUSPEND_MEM), a given state is either
always supported or always unsupported and store that information
in pm_states[] instead of calling valid_state() every time we
need to check it.

Also do not use valid_state() for PM_SUSPEND_FREEZE, which is always
valid, and move the pm_test_level validity check for PM_SUSPEND_FREEZE
directly into enter_state().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: <stable@vger.kernel.org> # 3.10+: 27ddcc6596e5: PM / sleep: Add state field to pm_states[] entries
Cc: <stable@vger.kernel.org> # 3.10+
---
This is a backport request for these two commits upstream:

    27ddcc6596e5 PM / sleep: Add state field to pm_states[] entries
    43e8317b0bba PM / sleep: Use valid_state() for platform-dependent sleep states only

Rafael ack'd this:

  https://lkml.org/lkml/2014/9/4/543

I've tested these on 3.14, and it looks like they could go back as far as 3.10.

 kernel/power/main.c         |  9 ++++---
 kernel/power/power.h        |  2 --
 kernel/power/suspend.c      | 60 ++++++++++++++++++++++-----------------------
 kernel/power/suspend_test.c |  2 +-
 4 files changed, 36 insertions(+), 37 deletions(-)

diff --git a/kernel/power/main.c b/kernel/power/main.c
index 8e818432253c..9f51f0ab3d86 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -296,7 +296,7 @@ static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
 	suspend_state_t i;
 
 	for (i = PM_SUSPEND_MIN; i < PM_SUSPEND_MAX; i++)
-		if (valid_state(i))
+		if (pm_states[i].state)
 			s += sprintf(s,"%s ", pm_states[i].label);
 
 #endif
@@ -328,8 +328,9 @@ static suspend_state_t decode_state(const char *buf, size_t n)
 
 #ifdef CONFIG_SUSPEND
 	for (s = &pm_states[state]; state < PM_SUSPEND_MAX; s++, state++)
-		if (len == strlen(s->label) && !strncmp(buf, s->label, len))
-			return state;
+		if (s->state && len == strlen(s->label)
+		    && !strncmp(buf, s->label, len))
+			return s->state;
 #endif
 
 	return PM_SUSPEND_ON;
@@ -447,7 +448,7 @@ static ssize_t autosleep_show(struct kobject *kobj,
 
 #ifdef CONFIG_SUSPEND
 	if (state < PM_SUSPEND_MAX)
-		return sprintf(buf, "%s\n", valid_state(state) ?
+		return sprintf(buf, "%s\n", pm_states[state].state ?
 					pm_states[state].label : "error");
 #endif
 #ifdef CONFIG_HIBERNATION
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 99539c5da844..c60f13b5270a 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -186,14 +186,12 @@ struct pm_sleep_state {
 /* kernel/power/suspend.c */
 extern struct pm_sleep_state pm_states[];
 
-extern bool valid_state(suspend_state_t state);
 extern int suspend_devices_and_enter(suspend_state_t state);
 #else /* !CONFIG_SUSPEND */
 static inline int suspend_devices_and_enter(suspend_state_t state)
 {
 	return -ENOSYS;
 }
-static inline bool valid_state(suspend_state_t state) { return false; }
 #endif /* !CONFIG_SUSPEND */
 
 #ifdef CONFIG_PM_TEST_SUSPEND
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 5d93b138a2d4..00aca60904b0 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -32,9 +32,9 @@
 #include "power.h"
 
 struct pm_sleep_state pm_states[PM_SUSPEND_MAX] = {
-	[PM_SUSPEND_FREEZE] = { "freeze", PM_SUSPEND_FREEZE },
-	[PM_SUSPEND_STANDBY] = { "standby", PM_SUSPEND_STANDBY },
-	[PM_SUSPEND_MEM] = { "mem", PM_SUSPEND_MEM },
+	[PM_SUSPEND_FREEZE] = { .label = "freeze", .state = PM_SUSPEND_FREEZE },
+	[PM_SUSPEND_STANDBY] = { .label = "standby", },
+	[PM_SUSPEND_MEM] = { .label = "mem", },
 };
 
 static const struct platform_suspend_ops *suspend_ops;
@@ -68,42 +68,34 @@ void freeze_wake(void)
 }
 EXPORT_SYMBOL_GPL(freeze_wake);
 
+static bool valid_state(suspend_state_t state)
+{
+	/*
+	 * PM_SUSPEND_STANDBY and PM_SUSPEND_MEM states need low level
+	 * support and need to be valid to the low level
+	 * implementation, no valid callback implies that none are valid.
+	 */
+	return suspend_ops && suspend_ops->valid && suspend_ops->valid(state);
+}
+
 /**
  * suspend_set_ops - Set the global suspend method table.
  * @ops: Suspend operations to use.
  */
 void suspend_set_ops(const struct platform_suspend_ops *ops)
 {
+	suspend_state_t i;
+
 	lock_system_sleep();
+
 	suspend_ops = ops;
+	for (i = PM_SUSPEND_STANDBY; i <= PM_SUSPEND_MEM; i++)
+		pm_states[i].state = valid_state(i) ? i : 0;
+
 	unlock_system_sleep();
 }
 EXPORT_SYMBOL_GPL(suspend_set_ops);
 
-bool valid_state(suspend_state_t state)
-{
-	if (state == PM_SUSPEND_FREEZE) {
-#ifdef CONFIG_PM_DEBUG
-		if (pm_test_level != TEST_NONE &&
-		    pm_test_level != TEST_FREEZER &&
-		    pm_test_level != TEST_DEVICES &&
-		    pm_test_level != TEST_PLATFORM) {
-			printk(KERN_WARNING "Unsupported pm_test mode for "
-					"freeze state, please choose "
-					"none/freezer/devices/platform.\n");
-			return false;
-		}
-#endif
-			return true;
-	}
-	/*
-	 * PM_SUSPEND_STANDBY and PM_SUSPEND_MEMORY states need lowlevel
-	 * support and need to be valid to the lowlevel
-	 * implementation, no valid callback implies that none are valid.
-	 */
-	return suspend_ops && suspend_ops->valid && suspend_ops->valid(state);
-}
-
 /**
  * suspend_valid_only_mem - Generic memory-only valid callback.
  *
@@ -330,9 +322,17 @@ static int enter_state(suspend_state_t state)
 {
 	int error;
 
-	if (!valid_state(state))
-		return -ENODEV;
-
+	if (state == PM_SUSPEND_FREEZE) {
+#ifdef CONFIG_PM_DEBUG
+		if (pm_test_level != TEST_NONE && pm_test_level <= TEST_CPUS) {
+			pr_warning("PM: Unsupported test mode for freeze state,"
+				   "please choose none/freezer/devices/platform.\n");
+			return -EAGAIN;
+		}
+#endif
+	} else if (!valid_state(state)) {
+		return -EINVAL;
+	}
 	if (!mutex_trylock(&pm_mutex))
 		return -EBUSY;
 
diff --git a/kernel/power/suspend_test.c b/kernel/power/suspend_test.c
index d4e3ab167a73..269b097e78ea 100644
--- a/kernel/power/suspend_test.c
+++ b/kernel/power/suspend_test.c
@@ -162,7 +162,7 @@ static int __init test_suspend(void)
 	/* PM is initialized by now; is that state testable? */
 	if (test_state == PM_SUSPEND_ON)
 		goto done;
-	if (!valid_state(test_state)) {
+	if (!pm_states[test_state].state) {
 		printk(warn_bad_state, pm_states[test_state].label);
 		goto done;
 	}
-- 
1.9.1

[PATCH 3.10.y+] PM / sleep: Use valid_state() for platform-dependent sleep states only

[Brian Norris]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

[Upstream commit 43e8317b0bba1d6eb85f38a4a233d82d7c20d732]

Use the observation that, for platform-dependent sleep states
(PM_SUSPEND_STANDBY, PM_SUSPEND_MEM), a given state is either
always supported or always unsupported and store that information
in pm_states[] instead of calling valid_state() every time we
need to check it.

Also do not use valid_state() for PM_SUSPEND_FREEZE, which is always
valid, and move the pm_test_level validity check for PM_SUSPEND_FREEZE
directly into enter_state().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: <stable@vger.kernel.org> # 3.10+: 27ddcc6596e5: PM / sleep: Add state field to pm_states[] entries
Cc: <stable@vger.kernel.org> # 3.10+
---
This is a backport request for these two commits upstream:

    27ddcc6596e5 PM / sleep: Add state field to pm_states[] entries
    43e8317b0bba PM / sleep: Use valid_state() for platform-dependent sleep states only

Rafael ack'd this:

  https://lkml.org/lkml/2014/9/4/543

I've tested these on 3.14, and it looks like they could go back as far as 3.10.

 kernel/power/main.c         |  9 ++++---
 kernel/power/power.h        |  2 --
 kernel/power/suspend.c      | 60 ++++++++++++++++++++++-----------------------
 kernel/power/suspend_test.c |  2 +-
 4 files changed, 36 insertions(+), 37 deletions(-)

diff --git a/kernel/power/main.c b/kernel/power/main.c
index 8e818432253c..9f51f0ab3d86 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -296,7 +296,7 @@ static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
 	suspend_state_t i;
 
 	for (i = PM_SUSPEND_MIN; i < PM_SUSPEND_MAX; i++)
-		if (valid_state(i))
+		if (pm_states[i].state)
 			s += sprintf(s,"%s ", pm_states[i].label);
 
 #endif
@@ -328,8 +328,9 @@ static suspend_state_t decode_state(const char *buf, size_t n)
 
 #ifdef CONFIG_SUSPEND
 	for (s = &pm_states[state]; state < PM_SUSPEND_MAX; s++, state++)
-		if (len == strlen(s->label) && !strncmp(buf, s->label, len))
-			return state;
+		if (s->state && len == strlen(s->label)
+		    && !strncmp(buf, s->label, len))
+			return s->state;
 #endif
 
 	return PM_SUSPEND_ON;
@@ -447,7 +448,7 @@ static ssize_t autosleep_show(struct kobject *kobj,
 
 #ifdef CONFIG_SUSPEND
 	if (state < PM_SUSPEND_MAX)
-		return sprintf(buf, "%s\n", valid_state(state) ?
+		return sprintf(buf, "%s\n", pm_states[state].state ?
 					pm_states[state].label : "error");
 #endif
 #ifdef CONFIG_HIBERNATION
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 99539c5da844..c60f13b5270a 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -186,14 +186,12 @@ struct pm_sleep_state {
 /* kernel/power/suspend.c */
 extern struct pm_sleep_state pm_states[];
 
-extern bool valid_state(suspend_state_t state);
 extern int suspend_devices_and_enter(suspend_state_t state);
 #else /* !CONFIG_SUSPEND */
 static inline int suspend_devices_and_enter(suspend_state_t state)
 {
 	return -ENOSYS;
 }
-static inline bool valid_state(suspend_state_t state) { return false; }
 #endif /* !CONFIG_SUSPEND */
 
 #ifdef CONFIG_PM_TEST_SUSPEND
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 5d93b138a2d4..00aca60904b0 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -32,9 +32,9 @@
 #include "power.h"
 
 struct pm_sleep_state pm_states[PM_SUSPEND_MAX] = {
-	[PM_SUSPEND_FREEZE] = { "freeze", PM_SUSPEND_FREEZE },
-	[PM_SUSPEND_STANDBY] = { "standby", PM_SUSPEND_STANDBY },
-	[PM_SUSPEND_MEM] = { "mem", PM_SUSPEND_MEM },
+	[PM_SUSPEND_FREEZE] = { .label = "freeze", .state = PM_SUSPEND_FREEZE },
+	[PM_SUSPEND_STANDBY] = { .label = "standby", },
+	[PM_SUSPEND_MEM] = { .label = "mem", },
 };
 
 static const struct platform_suspend_ops *suspend_ops;
@@ -68,42 +68,34 @@ void freeze_wake(void)
 }
 EXPORT_SYMBOL_GPL(freeze_wake);
 
+static bool valid_state(suspend_state_t state)
+{
+	/*
+	 * PM_SUSPEND_STANDBY and PM_SUSPEND_MEM states need low level
+	 * support and need to be valid to the low level
+	 * implementation, no valid callback implies that none are valid.
+	 */
+	return suspend_ops && suspend_ops->valid && suspend_ops->valid(state);
+}
+
 /**
  * suspend_set_ops - Set the global suspend method table.
  * @ops: Suspend operations to use.
  */
 void suspend_set_ops(const struct platform_suspend_ops *ops)
 {
+	suspend_state_t i;
+
 	lock_system_sleep();
+
 	suspend_ops = ops;
+	for (i = PM_SUSPEND_STANDBY; i <= PM_SUSPEND_MEM; i++)
+		pm_states[i].state = valid_state(i) ? i : 0;
+
 	unlock_system_sleep();
 }
 EXPORT_SYMBOL_GPL(suspend_set_ops);
 
-bool valid_state(suspend_state_t state)
-{
-	if (state == PM_SUSPEND_FREEZE) {
-#ifdef CONFIG_PM_DEBUG
-		if (pm_test_level != TEST_NONE &&
-		    pm_test_level != TEST_FREEZER &&
-		    pm_test_level != TEST_DEVICES &&
-		    pm_test_level != TEST_PLATFORM) {
-			printk(KERN_WARNING "Unsupported pm_test mode for "
-					"freeze state, please choose "
-					"none/freezer/devices/platform.\n");
-			return false;
-		}
-#endif
-			return true;
-	}
-	/*
-	 * PM_SUSPEND_STANDBY and PM_SUSPEND_MEMORY states need lowlevel
-	 * support and need to be valid to the lowlevel
-	 * implementation, no valid callback implies that none are valid.
-	 */
-	return suspend_ops && suspend_ops->valid && suspend_ops->valid(state);
-}
-
 /**
  * suspend_valid_only_mem - Generic memory-only valid callback.
  *
@@ -330,9 +322,17 @@ static int enter_state(suspend_state_t state)
 {
 	int error;
 
-	if (!valid_state(state))
-		return -ENODEV;
-
+	if (state == PM_SUSPEND_FREEZE) {
+#ifdef CONFIG_PM_DEBUG
+		if (pm_test_level != TEST_NONE && pm_test_level <= TEST_CPUS) {
+			pr_warning("PM: Unsupported test mode for freeze state,"
+				   "please choose none/freezer/devices/platform.\n");
+			return -EAGAIN;
+		}
+#endif
+	} else if (!valid_state(state)) {
+		return -EINVAL;
+	}
 	if (!mutex_trylock(&pm_mutex))
 		return -EBUSY;
 
diff --git a/kernel/power/suspend_test.c b/kernel/power/suspend_test.c
index d4e3ab167a73..269b097e78ea 100644
--- a/kernel/power/suspend_test.c
+++ b/kernel/power/suspend_test.c
@@ -162,7 +162,7 @@ static int __init test_suspend(void)
 	/* PM is initialized by now; is that state testable? */
 	if (test_state == PM_SUSPEND_ON)
 		goto done;
-	if (!valid_state(test_state)) {
+	if (!pm_states[test_state].state) {
 		printk(warn_bad_state, pm_states[test_state].label);
 		goto done;
 	}
-- 
1.9.1

Re: [PATCH 3.10.y+] PM / sleep: Use valid_state() for platform-dependent sleep states only

[Francis Moreau]

Hello,

On 09/04/2014 11:21 PM, Brian Norris wrote:
> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> 
> [Upstream commit 43e8317b0bba1d6eb85f38a4a233d82d7c20d732]
> 
> Use the observation that, for platform-dependent sleep states
> (PM_SUSPEND_STANDBY, PM_SUSPEND_MEM), a given state is either
> always supported or always unsupported and store that information
> in pm_states[] instead of calling valid_state() every time we
> need to check it.
> 
> Also do not use valid_state() for PM_SUSPEND_FREEZE, which is always
> valid, and move the pm_test_level validity check for PM_SUSPEND_FREEZE
> directly into enter_state().
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: <stable@vger.kernel.org> # 3.10+: 27ddcc6596e5: PM / sleep: Add state field to pm_states[] entries
> Cc: <stable@vger.kernel.org> # 3.10+
> ---
> This is a backport request for these two commits upstream:
> 
>     27ddcc6596e5 PM / sleep: Add state field to pm_states[] entries
>     43e8317b0bba PM / sleep: Use valid_state() for platform-dependent sleep states only
> 

Wouldn't it be cleaner to have 2 separate backports then ?

Thanks for the backport anyways.

Re: [PATCH 3.10.y+] PM / sleep: Use valid_state() for platform-dependent sleep states only

[Brian Norris]

On Fri, Sep 05, 2014 at 08:29:09AM +0200, Francis Moreau wrote:
> On 09/04/2014 11:21 PM, Brian Norris wrote:
[...]
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Cc: <stable@vger.kernel.org> # 3.10+: 27ddcc6596e5: PM / sleep: Add state field to pm_states[] entries
> > Cc: <stable@vger.kernel.org> # 3.10+
> > ---
> > This is a backport request for these two commits upstream:
> > 
> >     27ddcc6596e5 PM / sleep: Add state field to pm_states[] entries
> >     43e8317b0bba PM / sleep: Use valid_state() for platform-dependent sleep states only
> > 
> 
> Wouldn't it be cleaner to have 2 separate backports then ?

The first is purely a dependency for the second. It has no value on its
own. So I thought the above form made sense and followed the process
mentioned in Documentation/stable_kernel_rules.txt.

Admittedly, it's a little asymmetric. But I really don't know what the
"best" option is, since I'd prefer not having to send around any patch
text at all, unless the backport is not trivial (these apply cleanly).

Related: I don't feel like Documentation/stable_kernel_rules.txt is very
clear under the "Procedure" section. It lists a series of
non-sequential steps, some of which are mutually exclusive.

Any tips on making my post-merge -stable submissions better are
appreciated. And recommendations on improving the text (or just my
interpretation) of Documentation/stable_kernel_rules.txt are welcome
too.

Brian

Re: [PATCH 3.10.y+] PM / sleep: Use valid_state() for platform-dependent sleep states only

[Francis Moreau]

On 09/05/2014 09:45 AM, Brian Norris wrote:
> On Fri, Sep 05, 2014 at 08:29:09AM +0200, Francis Moreau wrote:
>> On 09/04/2014 11:21 PM, Brian Norris wrote:
> [...]
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> Cc: <stable@vger.kernel.org> # 3.10+: 27ddcc6596e5: PM / sleep: Add state field to pm_states[] entries
>>> Cc: <stable@vger.kernel.org> # 3.10+
>>> ---
>>> This is a backport request for these two commits upstream:
>>>
>>>     27ddcc6596e5 PM / sleep: Add state field to pm_states[] entries
>>>     43e8317b0bba PM / sleep: Use valid_state() for platform-dependent sleep states only
>>>
>>
>> Wouldn't it be cleaner to have 2 separate backports then ?
> 
> The first is purely a dependency for the second. It has no value on its
> own. So I thought the above form made sense and followed the process
> mentioned in Documentation/stable_kernel_rules.txt.
> 
> Admittedly, it's a little asymmetric. But I really don't know what the
> "best" option is, since I'd prefer not having to send around any patch
> text at all, unless the backport is not trivial (these apply cleanly).

I don't know, I just find cleaner to cherry-pick upstream commits when
possible so I can retrieve them easily later when inspecting a stable
kernel.

My 2 cents.

Re: [PATCH 3.10.y+] PM / sleep: Use valid_state() for platform-dependent sleep states only

[Greg Kroah-Hartman]

On Fri, Sep 05, 2014 at 12:45:12AM -0700, Brian Norris wrote:
> On Fri, Sep 05, 2014 at 08:29:09AM +0200, Francis Moreau wrote:
> > On 09/04/2014 11:21 PM, Brian Norris wrote:
> [...]
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > Cc: <stable@vger.kernel.org> # 3.10+: 27ddcc6596e5: PM / sleep: Add state field to pm_states[] entries
> > > Cc: <stable@vger.kernel.org> # 3.10+
> > > ---
> > > This is a backport request for these two commits upstream:
> > > 
> > >     27ddcc6596e5 PM / sleep: Add state field to pm_states[] entries
> > >     43e8317b0bba PM / sleep: Use valid_state() for platform-dependent sleep states only
> > > 
> > 
> > Wouldn't it be cleaner to have 2 separate backports then ?
> 
> The first is purely a dependency for the second. It has no value on its
> own. So I thought the above form made sense and followed the process
> mentioned in Documentation/stable_kernel_rules.txt.
> 
> Admittedly, it's a little asymmetric. But I really don't know what the
> "best" option is, since I'd prefer not having to send around any patch
> text at all, unless the backport is not trivial (these apply cleanly).

If they apply cleanly, then just list the git commit ids, and I can take
care of the rest.

Don't merge patches together, it just causes problems and makes it
harder to track what is going on.

thanks,

greg k-h

Re: [PATCH 3.10.y+] PM / sleep: Use valid_state() for platform-dependent sleep states only

[Brian Norris]

On Fri, Sep 05, 2014 at 02:47:58PM -0700, Greg Kroah-Hartman wrote:
> On Fri, Sep 05, 2014 at 12:45:12AM -0700, Brian Norris wrote:
> > On Fri, Sep 05, 2014 at 08:29:09AM +0200, Francis Moreau wrote:
> > > On 09/04/2014 11:21 PM, Brian Norris wrote:
> > [...]
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > Cc: <stable@vger.kernel.org> # 3.10+: 27ddcc6596e5: PM / sleep: Add state field to pm_states[] entries
> > > > Cc: <stable@vger.kernel.org> # 3.10+
> > > > ---
> > > > This is a backport request for these two commits upstream:
> > > > 
> > > >     27ddcc6596e5 PM / sleep: Add state field to pm_states[] entries
> > > >     43e8317b0bba PM / sleep: Use valid_state() for platform-dependent sleep states only
> > > > 
> > > 
> > > Wouldn't it be cleaner to have 2 separate backports then ?
> > 
> > The first is purely a dependency for the second. It has no value on its
> > own. So I thought the above form made sense and followed the process
> > mentioned in Documentation/stable_kernel_rules.txt.
> > 
> > Admittedly, it's a little asymmetric. But I really don't know what the
> > "best" option is, since I'd prefer not having to send around any patch
> > text at all, unless the backport is not trivial (these apply cleanly).
> 
> If they apply cleanly, then just list the git commit ids, and I can take
> care of the rest.

OK. Is this a policy that should be documented? AIUI, we have a few
options:

  1. Include 'Cc: stable@vger.kernel.org' in the original commit that
  gets to Linus

  2. Send an email to stable@vger.kernel.org that just contains the
  commit IDs, after they've made it to Linus

  3. Send patches to stable@vger.kernel.org, if backporting is not
  trivial

#1 is most common, and #2 seems like it would handle most of what misses
#1. #3 seems inferior, whenever #2 would suffice. But #2 is not in
stable_kernel_rules.txt.

> Don't merge patches together, it just causes problems and makes it
> harder to track what is going on.

To be clear, the diff I sent is actually just a single patch (the fix);
it is not a squashed version of both. I realize now that this was
probably unclear.

Brian

Re: [PATCH 3.10.y+] PM / sleep: Use valid_state() for platform-dependent sleep states only

[Greg Kroah-Hartman]

On Fri, Sep 05, 2014 at 03:08:28PM -0700, Brian Norris wrote:
> On Fri, Sep 05, 2014 at 02:47:58PM -0700, Greg Kroah-Hartman wrote:
> > On Fri, Sep 05, 2014 at 12:45:12AM -0700, Brian Norris wrote:
> > > On Fri, Sep 05, 2014 at 08:29:09AM +0200, Francis Moreau wrote:
> > > > On 09/04/2014 11:21 PM, Brian Norris wrote:
> > > [...]
> > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > Cc: <stable@vger.kernel.org> # 3.10+: 27ddcc6596e5: PM / sleep: Add state field to pm_states[] entries
> > > > > Cc: <stable@vger.kernel.org> # 3.10+
> > > > > ---
> > > > > This is a backport request for these two commits upstream:
> > > > > 
> > > > >     27ddcc6596e5 PM / sleep: Add state field to pm_states[] entries
> > > > >     43e8317b0bba PM / sleep: Use valid_state() for platform-dependent sleep states only
> > > > > 
> > > > 
> > > > Wouldn't it be cleaner to have 2 separate backports then ?
> > > 
> > > The first is purely a dependency for the second. It has no value on its
> > > own. So I thought the above form made sense and followed the process
> > > mentioned in Documentation/stable_kernel_rules.txt.
> > > 
> > > Admittedly, it's a little asymmetric. But I really don't know what the
> > > "best" option is, since I'd prefer not having to send around any patch
> > > text at all, unless the backport is not trivial (these apply cleanly).
> > 
> > If they apply cleanly, then just list the git commit ids, and I can take
> > care of the rest.
> 
> OK. Is this a policy that should be documented? AIUI, we have a few
> options:
> 
>   1. Include 'Cc: stable@vger.kernel.org' in the original commit that
>   gets to Linus
> 
>   2. Send an email to stable@vger.kernel.org that just contains the
>   commit IDs, after they've made it to Linus
> 
>   3. Send patches to stable@vger.kernel.org, if backporting is not
>   trivial
> 
> #1 is most common, and #2 seems like it would handle most of what misses
> #1. #3 seems inferior, whenever #2 would suffice. But #2 is not in
> stable_kernel_rules.txt.

I always gladly take patches to that .txt file if you wish to make it
clearer.

thanks,

greg k-h

Re: [PATCH 3.10.y+] PM / sleep: Use valid_state() for platform-dependent sleep states only

[Brian Norris]

On Fri, Sep 05, 2014 at 03:36:26PM -0700, Greg Kroah-Hartman wrote:
> > > If they apply cleanly, then just list the git commit ids, and I can take
> > > care of the rest.
> > 
> > OK. Is this a policy that should be documented? AIUI, we have a few
> > options:
> > 
> >   1. Include 'Cc: stable@vger.kernel.org' in the original commit that
> >   gets to Linus
> > 
> >   2. Send an email to stable@vger.kernel.org that just contains the
> >   commit IDs, after they've made it to Linus
> > 
> >   3. Send patches to stable@vger.kernel.org, if backporting is not
> >   trivial
> > 
> > #1 is most common, and #2 seems like it would handle most of what misses
> > #1. #3 seems inferior, whenever #2 would suffice. But #2 is not in
> > stable_kernel_rules.txt.
> 
> I always gladly take patches to that .txt file if you wish to make it
> clearer.

I was interested in whether I had my facts straight before trying to
document them. I'll take your response to mean I'm not far off base, so
I'll take a stab at patching the file.

Thanks,
Brian