diff mbox

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

Message ID 1409865665-5375-1-git-send-email-computersforpeace@gmail.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Brian Norris Sept. 4, 2014, 9:21 p.m. UTC 
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(-)

Comments

Francis Moreau Sept. 5, 2014, 6:29 a.m. UTC | #1 
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Brian Norris Sept. 5, 2014, 7:45 a.m. UTC | #2 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Francis Moreau Sept. 5, 2014, 2:52 p.m. UTC | #3 
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greg Kroah-Hartman Sept. 5, 2014, 9:47 p.m. UTC | #4 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Brian Norris Sept. 5, 2014, 10:08 p.m. UTC | #5 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greg Kroah-Hartman Sept. 5, 2014, 10:36 p.m. UTC | #6 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Brian Norris Sept. 5, 2014, 10:45 p.m. UTC | #7 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;
 	}