[PATCH] PM / sleep: add configurable delay for pm_test

[PATCH] PM / sleep: add configurable delay for pm_test

[Brian Norris]

When CONFIG_PM_DEBUG=y, we provide a sysfs file (/sys/power/pm_test) for
selecting one of a few suspend test modes, where rather than entering a
full suspend state, the kernel will perform some subset of suspend
steps, wait 5 seconds, and then resume back to normal operation.

This mode is useful for (among other things) observing the state of the
system just before entering a sleep mode, for debugging or analysis
purposes. However, a constant 5 second wait is not sufficient for some
sorts of analysis; for example, on an SoC, one might want to use
external tools to probe the power states of various on-chip controllers
or clocks.

This patch adds a companion sysfs file (/sys/power/pm_test_delay) that
allows user-space to configure how long the system waits in this test
state before resuming. It also updates the PM debugging documentation to
mention the new file.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 Documentation/power/basic-pm-debugging.txt | 14 ++++++++------
 kernel/power/main.c                        | 27 +++++++++++++++++++++++++++
 kernel/power/power.h                       |  5 +++++
 kernel/power/suspend.c                     |  8 ++++++--
 4 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/Documentation/power/basic-pm-debugging.txt b/Documentation/power/basic-pm-debugging.txt
index edeecd447d23..bd9f27ae99fe 100644
--- a/Documentation/power/basic-pm-debugging.txt
+++ b/Documentation/power/basic-pm-debugging.txt
@@ -75,12 +75,14 @@ you should do the following:
 # echo platform > /sys/power/disk
 # echo disk > /sys/power/state
 
-Then, the kernel will try to freeze processes, suspend devices, wait 5 seconds,
-resume devices and thaw processes.  If "platform" is written to
-/sys/power/pm_test , then after suspending devices the kernel will additionally
-invoke the global control methods (eg. ACPI global control methods) used to
-prepare the platform firmware for hibernation.  Next, it will wait 5 seconds and
-invoke the platform (eg. ACPI) global methods used to cancel hibernation etc.
+Then, the kernel will try to freeze processes, suspend devices, wait a few
+seconds (5 by default, but configurable via /sys/power/pm_test_delay), resume
+devices and thaw processes.  If "platform" is written to /sys/power/pm_test,
+then after suspending devices the kernel will additionally invoke the global
+control methods (eg. ACPI global control methods) used to prepare the platform
+firmware for hibernation.  Next, it will wait a configurable number of seconds
+and invoke the platform (eg. ACPI) global methods used to cancel hibernation
+etc.
 
 Writing "none" to /sys/power/pm_test causes the kernel to switch to the normal
 hibernation/suspend operations.  Also, when open for reading, /sys/power/pm_test
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 9a59d042ea84..4d242c8b43a0 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -73,6 +73,7 @@ power_attr(pm_async);
 
 #ifdef CONFIG_PM_DEBUG
 int pm_test_level = TEST_NONE;
+int pm_test_seconds = PM_TEST_DELAY_DEFAULT;
 
 static const char * const pm_tests[__TEST_AFTER_LAST] = {
 	[TEST_NONE] = "none",
@@ -132,6 +133,31 @@ static ssize_t pm_test_store(struct kobject *kobj, struct kobj_attribute *attr,
 }
 
 power_attr(pm_test);
+
+static ssize_t pm_test_delay_show(struct kobject *kobj,
+				  struct kobj_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", pm_test_seconds);
+}
+
+static ssize_t pm_test_delay_store(struct kobject *kobj,
+				   struct kobj_attribute *attr,
+				   const char *buf, size_t n)
+{
+	int val;
+
+	if (kstrtoint(buf, 10, &val))
+		return -EINVAL;
+
+	if (val < 0)
+		return -EINVAL;
+
+	pm_test_seconds = val;
+
+	return n;
+}
+
+power_attr(pm_test_delay);
 #endif /* CONFIG_PM_DEBUG */
 
 #ifdef CONFIG_DEBUG_FS
@@ -601,6 +627,7 @@ static struct attribute * g[] = {
 #endif
 #ifdef CONFIG_PM_DEBUG
 	&pm_test_attr.attr,
+	&pm_test_delay_attr.attr,
 #endif
 #ifdef CONFIG_PM_SLEEP_DEBUG
 	&pm_print_times_attr.attr,
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 5d49dcac2537..28111795da71 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -230,6 +230,11 @@ enum {
 
 extern int pm_test_level;
 
+/* Default to 5 second delay */
+#define PM_TEST_DELAY_DEFAULT		5
+
+extern int pm_test_seconds;
+
 #ifdef CONFIG_SUSPEND_FREEZER
 static inline int suspend_freeze_processes(void)
 {
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 6dadb25cb0d8..2372a99d4356 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -196,8 +196,12 @@ static int suspend_test(int level)
 {
 #ifdef CONFIG_PM_DEBUG
 	if (pm_test_level == level) {
-		printk(KERN_INFO "suspend debug: Waiting for 5 seconds.\n");
-		mdelay(5000);
+		int i;
+
+		pr_info("suspend debug: waiting for %d second(s)\n",
+				pm_test_seconds);
+		for (i = 0; i < pm_test_seconds; i++)
+			mdelay(1000);
 		return 1;
 	}
 #endif /* !CONFIG_PM_DEBUG */
-- 
1.9.1

Re: [PATCH] PM / sleep: add configurable delay for pm_test

[Pavel Machek]

Hi!

> When CONFIG_PM_DEBUG=y, we provide a sysfs file (/sys/power/pm_test) for
> selecting one of a few suspend test modes, where rather than entering a
> full suspend state, the kernel will perform some subset of suspend
> steps, wait 5 seconds, and then resume back to normal operation.
> 
> This mode is useful for (among other things) observing the state of the
> system just before entering a sleep mode, for debugging or analysis
> purposes. However, a constant 5 second wait is not sufficient for some
> sorts of analysis; for example, on an SoC, one might want to use
> external tools to probe the power states of various on-chip controllers
> or clocks.

When you are doing this kind of analysis, perhaps directly modifying
kernel source is the way to go ...?

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

Re: [PATCH] PM / sleep: add configurable delay for pm_test

[Brian Norris]

On Thu, Sep 04, 2014 at 09:14:12AM +0200, Pavel Machek wrote:
> > When CONFIG_PM_DEBUG=y, we provide a sysfs file (/sys/power/pm_test) for
> > selecting one of a few suspend test modes, where rather than entering a
> > full suspend state, the kernel will perform some subset of suspend
> > steps, wait 5 seconds, and then resume back to normal operation.
> > 
> > This mode is useful for (among other things) observing the state of the
> > system just before entering a sleep mode, for debugging or analysis
> > purposes. However, a constant 5 second wait is not sufficient for some
> > sorts of analysis; for example, on an SoC, one might want to use
> > external tools to probe the power states of various on-chip controllers
> > or clocks.
> 
> When you are doing this kind of analysis, perhaps directly modifying
> kernel source is the way to go ...?

That's what I've been doing for now, but I have a few engineers who need
to do this sort of testing and aren't kernel developers. I could
continue to maintain my own patch for this, but I just thought I'd see
what others thought.

Is there a good reason this can't be in mainline? These features are
hidden behind a Kconfig symbol called PM_DEBUG anyway, and I think this
classifies as a pretty simple extension to the limited existing PM
debugging options.

Regards,
Brian

Re: [PATCH] PM / sleep: add configurable delay for pm_test

[Chirantan Ekbote]

On Thu, Sep 4, 2014 at 10:54 AM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Thu, Sep 04, 2014 at 09:14:12AM +0200, Pavel Machek wrote:
>> > When CONFIG_PM_DEBUG=y, we provide a sysfs file (/sys/power/pm_test) for
>> > selecting one of a few suspend test modes, where rather than entering a
>> > full suspend state, the kernel will perform some subset of suspend
>> > steps, wait 5 seconds, and then resume back to normal operation.
>> >
>> > This mode is useful for (among other things) observing the state of the
>> > system just before entering a sleep mode, for debugging or analysis
>> > purposes. However, a constant 5 second wait is not sufficient for some
>> > sorts of analysis; for example, on an SoC, one might want to use
>> > external tools to probe the power states of various on-chip controllers
>> > or clocks.
>>
>> When you are doing this kind of analysis, perhaps directly modifying
>> kernel source is the way to go ...?
>
> That's what I've been doing for now, but I have a few engineers who need
> to do this sort of testing and aren't kernel developers. I could
> continue to maintain my own patch for this, but I just thought I'd see
> what others thought.
>
> Is there a good reason this can't be in mainline? These features are
> hidden behind a Kconfig symbol called PM_DEBUG anyway, and I think this
> classifies as a pretty simple extension to the limited existing PM
> debugging options.
>

We're currently carrying a similar patch in the chrome os kernel tree.
I would also like to see this go into mainline.

-Chirantan

> Regards,
> 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

Re: [PATCH] PM / sleep: add configurable delay for pm_test

[Brian Norris]

Hi Rafael,

On Wed, Sep 03, 2014 at 04:55:35PM -0700, Brian Norris wrote:
> When CONFIG_PM_DEBUG=y, we provide a sysfs file (/sys/power/pm_test) for
> selecting one of a few suspend test modes, where rather than entering a
> full suspend state, the kernel will perform some subset of suspend
> steps, wait 5 seconds, and then resume back to normal operation.
> 
> This mode is useful for (among other things) observing the state of the
> system just before entering a sleep mode, for debugging or analysis
> purposes. However, a constant 5 second wait is not sufficient for some
> sorts of analysis; for example, on an SoC, one might want to use
> external tools to probe the power states of various on-chip controllers
> or clocks.
> 
> This patch adds a companion sysfs file (/sys/power/pm_test_delay) that
> allows user-space to configure how long the system waits in this test
> state before resuming. It also updates the PM debugging documentation to
> mention the new file.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

What do you think about this patch? It seems there is at least one other
developer who is independently interested in this.

Thanks,
Brian

> ---
>  Documentation/power/basic-pm-debugging.txt | 14 ++++++++------
>  kernel/power/main.c                        | 27 +++++++++++++++++++++++++++
>  kernel/power/power.h                       |  5 +++++
>  kernel/power/suspend.c                     |  8 ++++++--
>  4 files changed, 46 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/power/basic-pm-debugging.txt b/Documentation/power/basic-pm-debugging.txt
> index edeecd447d23..bd9f27ae99fe 100644
> --- a/Documentation/power/basic-pm-debugging.txt
> +++ b/Documentation/power/basic-pm-debugging.txt
> @@ -75,12 +75,14 @@ you should do the following:
>  # echo platform > /sys/power/disk
>  # echo disk > /sys/power/state
>  
> -Then, the kernel will try to freeze processes, suspend devices, wait 5 seconds,
> -resume devices and thaw processes.  If "platform" is written to
> -/sys/power/pm_test , then after suspending devices the kernel will additionally
> -invoke the global control methods (eg. ACPI global control methods) used to
> -prepare the platform firmware for hibernation.  Next, it will wait 5 seconds and
> -invoke the platform (eg. ACPI) global methods used to cancel hibernation etc.
> +Then, the kernel will try to freeze processes, suspend devices, wait a few
> +seconds (5 by default, but configurable via /sys/power/pm_test_delay), resume
> +devices and thaw processes.  If "platform" is written to /sys/power/pm_test,
> +then after suspending devices the kernel will additionally invoke the global
> +control methods (eg. ACPI global control methods) used to prepare the platform
> +firmware for hibernation.  Next, it will wait a configurable number of seconds
> +and invoke the platform (eg. ACPI) global methods used to cancel hibernation
> +etc.
>  
>  Writing "none" to /sys/power/pm_test causes the kernel to switch to the normal
>  hibernation/suspend operations.  Also, when open for reading, /sys/power/pm_test
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 9a59d042ea84..4d242c8b43a0 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -73,6 +73,7 @@ power_attr(pm_async);
>  
>  #ifdef CONFIG_PM_DEBUG
>  int pm_test_level = TEST_NONE;
> +int pm_test_seconds = PM_TEST_DELAY_DEFAULT;
>  
>  static const char * const pm_tests[__TEST_AFTER_LAST] = {
>  	[TEST_NONE] = "none",
> @@ -132,6 +133,31 @@ static ssize_t pm_test_store(struct kobject *kobj, struct kobj_attribute *attr,
>  }
>  
>  power_attr(pm_test);
> +
> +static ssize_t pm_test_delay_show(struct kobject *kobj,
> +				  struct kobj_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%d\n", pm_test_seconds);
> +}
> +
> +static ssize_t pm_test_delay_store(struct kobject *kobj,
> +				   struct kobj_attribute *attr,
> +				   const char *buf, size_t n)
> +{
> +	int val;
> +
> +	if (kstrtoint(buf, 10, &val))
> +		return -EINVAL;
> +
> +	if (val < 0)
> +		return -EINVAL;
> +
> +	pm_test_seconds = val;
> +
> +	return n;
> +}
> +
> +power_attr(pm_test_delay);
>  #endif /* CONFIG_PM_DEBUG */
>  
>  #ifdef CONFIG_DEBUG_FS
> @@ -601,6 +627,7 @@ static struct attribute * g[] = {
>  #endif
>  #ifdef CONFIG_PM_DEBUG
>  	&pm_test_attr.attr,
> +	&pm_test_delay_attr.attr,
>  #endif
>  #ifdef CONFIG_PM_SLEEP_DEBUG
>  	&pm_print_times_attr.attr,
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index 5d49dcac2537..28111795da71 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -230,6 +230,11 @@ enum {
>  
>  extern int pm_test_level;
>  
> +/* Default to 5 second delay */
> +#define PM_TEST_DELAY_DEFAULT		5
> +
> +extern int pm_test_seconds;
> +
>  #ifdef CONFIG_SUSPEND_FREEZER
>  static inline int suspend_freeze_processes(void)
>  {
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 6dadb25cb0d8..2372a99d4356 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -196,8 +196,12 @@ static int suspend_test(int level)
>  {
>  #ifdef CONFIG_PM_DEBUG
>  	if (pm_test_level == level) {
> -		printk(KERN_INFO "suspend debug: Waiting for 5 seconds.\n");
> -		mdelay(5000);
> +		int i;
> +
> +		pr_info("suspend debug: waiting for %d second(s)\n",
> +				pm_test_seconds);
> +		for (i = 0; i < pm_test_seconds; i++)
> +			mdelay(1000);
>  		return 1;
>  	}
>  #endif /* !CONFIG_PM_DEBUG */
> -- 
> 1.9.1
> 

Re: [PATCH] PM / sleep: add configurable delay for pm_test

[Pavel Machek]

On Fri 2014-12-12 18:55:30, Brian Norris wrote:
> Hi Rafael,
> 
> On Wed, Sep 03, 2014 at 04:55:35PM -0700, Brian Norris wrote:
> > When CONFIG_PM_DEBUG=y, we provide a sysfs file (/sys/power/pm_test) for
> > selecting one of a few suspend test modes, where rather than entering a
> > full suspend state, the kernel will perform some subset of suspend
> > steps, wait 5 seconds, and then resume back to normal operation.
> > 
> > This mode is useful for (among other things) observing the state of the
> > system just before entering a sleep mode, for debugging or analysis
> > purposes. However, a constant 5 second wait is not sufficient for some
> > sorts of analysis; for example, on an SoC, one might want to use
> > external tools to probe the power states of various on-chip controllers
> > or clocks.
> > 
> > This patch adds a companion sysfs file (/sys/power/pm_test_delay) that
> > allows user-space to configure how long the system waits in this test
> > state before resuming. It also updates the PM debugging documentation to
> > mention the new file.
> > 
> > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> 
> What do you think about this patch? It seems there is at least one other
> developer who is independently interested in this.

40 lines of code, and new sysfs interface for use by someone who puts
the probes on board, anyway... (so should be able to add the single
mdelay himself).

Does not struck me as a good balance.
								Pavel
								
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] PM / sleep: add configurable delay for pm_test

[Brian Norris]

On Sat, Dec 13, 2014 at 09:31:23AM +0100, Pavel Machek wrote:
> On Fri 2014-12-12 18:55:30, Brian Norris wrote:
> > On Wed, Sep 03, 2014 at 04:55:35PM -0700, Brian Norris wrote:
> > > When CONFIG_PM_DEBUG=y, we provide a sysfs file (/sys/power/pm_test) for
> > > selecting one of a few suspend test modes, where rather than entering a
> > > full suspend state, the kernel will perform some subset of suspend
> > > steps, wait 5 seconds, and then resume back to normal operation.
> > > 
> > > This mode is useful for (among other things) observing the state of the
> > > system just before entering a sleep mode, for debugging or analysis
> > > purposes. However, a constant 5 second wait is not sufficient for some
> > > sorts of analysis; for example, on an SoC, one might want to use
> > > external tools to probe the power states of various on-chip controllers
> > > or clocks.
> > > 
> > > This patch adds a companion sysfs file (/sys/power/pm_test_delay) that
> > > allows user-space to configure how long the system waits in this test
> > > state before resuming. It also updates the PM debugging documentation to
> > > mention the new file.
> > > 
> > > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > 
> > What do you think about this patch? It seems there is at least one other
> > developer who is independently interested in this.
> 
> 40 lines of code, and new sysfs interface for use by someone who puts
> the probes on board, anyway... (so should be able to add the single
> mdelay himself).

I heard your complaint the first time:

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

And I responded to it already:

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

You did not respond, but Chirantan spoke up saying he wanted such a
patch too. He came up with a very similar solution independently:

  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/15bccc2c63c3475ef61d3187c73ccf1d80b18c7e

But since you've decided to make the same comment again, I will detail
more of the reasons why I think your suggestion ("go add the mdelay
yourself") is off-base.

 1. This is behind a debug config option (CONFIG_PM_DEBUG). So what's
 the problem with improving its usefulness? Non-debug users can easily
 compile it out if they're worried about 40 lines.

 2. The current debug code encodes a particular policy (which kernels
 generally should not). Is it better if I submit a patch that changes
 the current magic delay to 60000 milliseconds? What about 1334
 milliseconds?

 3. To continue your argument: why would I ever try to patch the
 upstream kernel, if I'm perfectly capable of doing this myself?

 4. How does putting probes on a board suddenly qualify someone for
 patching and rebuilding their kernel? I noted that I have *users* who
 want to do this. Hence, I'm patching a *user* interface.

Brian

Re: [PATCH] PM / sleep: add configurable delay for pm_test

[Pavel Machek]

> > 40 lines of code, and new sysfs interface for use by someone who puts
> > the probes on board, anyway... (so should be able to add the single
> > mdelay himself).
> 
> I heard your complaint the first time:
> 
>   https://lkml.org/lkml/2014/9/4/63
> 
> And I responded to it already:
> 
>   https://lkml.org/lkml/2014/9/4/494
> 
> You did not respond, but Chirantan spoke up saying he wanted such a
> patch too. He came up with a very similar solution independently:

I know. I believe your patch is so crazy that no more discussion was neccessary.

>   https://chromium.googlesource.com/chromiumos/third_party/kernel/+/15bccc2c63c3475ef61d3187c73ccf1d80b18c7e
> 
> But since you've decided to make the same comment again, I will detail
> more of the reasons why I think your suggestion ("go add the mdelay
> yourself") is off-base.
> 
>  1. This is behind a debug config option (CONFIG_PM_DEBUG). So what's
>  the problem with improving its usefulness? Non-debug users can easily
>  compile it out if they're worried about 40 lines.

We are talking 40 unneccessary lines of source. Imagine everyone who
needed to change one value added 40 lines of code. Terabyte
kernel. Overengineered. Crazy. Something we'll have to maintain
forever.

Make it module parameter so that the patch is two lines of code. If
that does not work for you, think of something that does.

>  2. The current debug code encodes a particular policy (which kernels
>  generally should not). Is it better if I submit a patch that changes
>  the current magic delay to 60000 milliseconds? What about 1334
>  milliseconds?
> 
>  3. To continue your argument: why would I ever try to patch the
>  upstream kernel, if I'm perfectly capable of doing this myself?

Good questions. Showing that perhaps you should not patch the
upstream.

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

Re: [PATCH] PM / sleep: add configurable delay for pm_test

[Brian Norris]

On Wed, Dec 17, 2014 at 09:09:47AM +0100, Pavel Machek wrote:
> 
> > > 40 lines of code, and new sysfs interface for use by someone who puts
> > > the probes on board, anyway... (so should be able to add the single
> > > mdelay himself).
> > 
> > I heard your complaint the first time:
> > 
> >   https://lkml.org/lkml/2014/9/4/63
> > 
> > And I responded to it already:
> > 
> >   https://lkml.org/lkml/2014/9/4/494
> > 
> > You did not respond, but Chirantan spoke up saying he wanted such a
> > patch too. He came up with a very similar solution independently:
> 
> I know. I believe your patch is so crazy that no more discussion was neccessary.

That's awfully disrespectful. You never responded to my follow up
question, and now you ridicule me for asking again.

> >   https://chromium.googlesource.com/chromiumos/third_party/kernel/+/15bccc2c63c3475ef61d3187c73ccf1d80b18c7e
> > 
> > But since you've decided to make the same comment again, I will detail
> > more of the reasons why I think your suggestion ("go add the mdelay
> > yourself") is off-base.
> > 
> >  1. This is behind a debug config option (CONFIG_PM_DEBUG). So what's
> >  the problem with improving its usefulness? Non-debug users can easily
> >  compile it out if they're worried about 40 lines.
> 
> We are talking 40 unneccessary lines of source.

"Unnecessary" is highly subjective. Just because you don't need it
doesn't mean no one else does.

There are at least two completely different projects that have mentioned
the need for it. Thus (as I explained in my "ping") I don't think that
qualifies as unnecessary.

> Imagine everyone who
> needed to change one value added 40 lines of code. Terabyte
> kernel.

That's an exaggeration.

> Overengineered. Crazy. Something we'll have to maintain
> forever.

It follows the same form and style of the other kernel/power/ debug
sysfs code. Its interface is well documented and straightforward. It's
really quite obvious. So how is this a maintenance problem?

Additionally, because it's explicitly a debug feature (CONFIG_PM_DEBUG),
I'm not sure it would really qualify as an unchangeable ABI that can
never be touched, if it really became a maintenance problem somehow.

> Make it module parameter so that the patch is two lines of code. If
> that does not work for you, think of something that does.

OK, so that's actually constructive. If lines of code is really the most
important factor here, then I suppose I can do that. I'd argue that a
module parameter is a much less sensible interface here, though, given
that it is coupled with the existing /sys/power/pm_test interface.

> >  2. The current debug code encodes a particular policy (which kernels
> >  generally should not). Is it better if I submit a patch that changes
> >  the current magic delay to 60000 milliseconds? What about 1334
> >  milliseconds?
> > 
> >  3. To continue your argument: why would I ever try to patch the
> >  upstream kernel, if I'm perfectly capable of doing this myself?
> 
> Good questions. Showing that perhaps you should not patch the
> upstream.

You seem to not understand the point behind my rhetorical questions. And
I don't feel like you've honestly addressed any of the 4 points I made.
That's disappointing.

I'd appreciate some feedback from Rafael. Based on his feedback (or lack
thereof) my options seem to be:

1. Sit and wait on this patch for another few months
2. Resend this patch as a module parameter instead
3. Forget this entirely

Thanks,
Brian

Re: [PATCH] PM / sleep: add configurable delay for pm_test

[Pavel Machek]

> > Make it module parameter so that the patch is two lines of code. If
> > that does not work for you, think of something that does.
> 
> OK, so that's actually constructive. If lines of code is really the most
> important factor here, then I suppose I can do that. I'd argue that a
> module parameter is a much less sensible interface here, though, given
> that it is coupled with the existing /sys/power/pm_test interface.

If module parameter works for you, we have a winner, that should be
two lines of code and two lines of documentation.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] PM / sleep: add configurable delay for pm_test

[Florian Fainelli]

2014-12-17 1:22 GMT-08:00 Pavel Machek <pavel@ucw.cz>:
>
>> > Make it module parameter so that the patch is two lines of code. If
>> > that does not work for you, think of something that does.
>>
>> OK, so that's actually constructive. If lines of code is really the most
>> important factor here, then I suppose I can do that. I'd argue that a
>> module parameter is a much less sensible interface here, though, given
>> that it is coupled with the existing /sys/power/pm_test interface.
>
> If module parameter works for you, we have a winner, that should be
> two lines of code and two lines of documentation.

I do not think that a module parameter is good, some of the uses cases
I can think about (from real people using that facility) involve
setting up a Linux system in a lab with multiple measurement
equipments, having to reboot Linux to change this delay is going to be
a no-go for them because that will break the
uptime/endurance/stability/automated testing they might be doing.
Having them be able to change the PM delay at runtime is completely
satisfactory though.

Both module parameters and sysfs entries need to remain stable, and
potentially there forever, once introduced, yet the sysfs entries are
a lot more flexible.

Considering that Brian's change are enclosed within a CONFIG_PM_DEBUG
ifdef, can we really use the code bloat as a technical argument here?

Thanks!
-- 
Florian

Re: [PATCH] PM / sleep: add configurable delay for pm_test

[Pavel Machek]

> Considering that Brian's change are enclosed within a CONFIG_PM_DEBUG
> ifdef, can we really use the code bloat as a technical argument here?

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

Re: [PATCH] PM / sleep: add configurable delay for pm_test

[Florian Fainelli]

2015-02-21 12:32 GMT-08:00 Pavel Machek <pavel@ucw.cz>:
>
>
>
>> Considering that Brian's change are enclosed within a CONFIG_PM_DEBUG
>> ifdef, can we really use the code bloat as a technical argument here?
>
> Yes.

Help me understand a few things here:

- this particular change is enclosed within a debug option, so only
people interested in enabling PM_DEBUG will get access to it

- if we need to turn on PM_DEBUG all the time, including mainline
distributions, does that mean that:
           - portions of code existing only in PM_DEBUG should be
moved out of it because it is actually useful outside of debug option?
           - CONFIG_PM itself is not self sufficient and there are
still problems that require PM_DEBUG to be turned on?
           - should there be a second level debug option (e.g:
CONFIG_PM_DEBUG_ADV) which gates specific control knobs like PM delay?

The current 5 seconds delay is completely arbitrary and goes against
the principle of not enforcing a policy, having this configurable
brings this back in the mechanism principle.
-- 
Florian

Re: [PATCH] PM / sleep: add configurable delay for pm_test

[Pavel Machek]

On Sat 2015-02-21 14:56:01, Florian Fainelli wrote:
> 2015-02-21 12:32 GMT-08:00 Pavel Machek <pavel@ucw.cz>:
> >
> >
> >
> >> Considering that Brian's change are enclosed within a CONFIG_PM_DEBUG
> >> ifdef, can we really use the code bloat as a technical argument here?
> >
> > Yes.
> 
> Help me understand a few things here:

What you are missing is that we try to keep _sources_ small and
readable, too.

> - if we need to turn on PM_DEBUG all the time, including mainline
> distributions, does that mean that:
>            - portions of code existing only in PM_DEBUG should be
> moved out of it because it is actually useful outside of debug option?
>            - CONFIG_PM itself is not self sufficient and there are
> still problems that require PM_DEBUG to be turned on?
>            - should there be a second level debug option (e.g:
> CONFIG_PM_DEBUG_ADV) which gates specific control knobs like PM delay?
> 
> The current 5 seconds delay is completely arbitrary and goes against
> the principle of not enforcing a policy, having this configurable
> brings this back in the mechanism principle.

5 second delay is arbitrary, and slightly ugly debugging hack, that is
acceptable because it is useful, small and unobtrusive. The second you
add 40 lines of sysfs glue.. it is no longer small and unobtrusive,
and no longer acceptable.

And yes, distros probably ship with PM_DEBUG on, and no, adding
another config option would not make the big & ugly hack more
acceptable.

OTOH if you added a hook to kprobes that alowed easy binary patching
of the value "5" while not adding 40 lines of overhead, that might be
neccessary.

Actually, you can probably pull the address from System.map and then
just write to it using /dev/mem, no? Just arrange for "5" to be in
variable that is in System.map.

And you also want to check the kernel parameters, they are modifiable
in runtime in at least same cases.

But we are not adding 40 lines of uglyness. Clear?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] PM / sleep: add configurable delay for pm_test

[Brian Norris]

On Sun, Feb 22, 2015 at 12:24:29AM +0100, Pavel Machek wrote:
> And you also want to check the kernel parameters, they are modifiable
> in runtime in at least same cases.

I forgot about this fact. In this case, I think this satisfies all that
we need. So we can do something like this at runtime:

   # echo 30 > /sys/module/suspend/parameters/pm_test_delay

I'll send a patch shortly.

Brian

[PATCH v2] PM / sleep: add configurable delay for pm_test

[Brian Norris]

When CONFIG_PM_DEBUG=y, we provide a sysfs file (/sys/power/pm_test) for
selecting one of a few suspend test modes, where rather than entering a
full suspend state, the kernel will perform some subset of suspend
steps, wait 5 seconds, and then resume back to normal operation.

This mode is useful for (among other things) observing the state of the
system just before entering a sleep mode, for debugging or analysis
purposes. However, a constant 5 second wait is not sufficient for some
sorts of analysis; for example, on an SoC, one might want to use
external tools to probe the power states of various on-chip controllers
or clocks.

This patch turns this 5 second delay into a configurable module
parameter, so users can determine how long to wait in this
pseudo-suspend state before resuming the system.

Example (wait 30 seconds);

  # echo 30 > /sys/module/suspend/parameters/pm_test_delay
  # echo core > /sys/power/pm_test
  # time echo mem  > /sys/power/state
  ...
  [   17.583625] suspend debug: Waiting for 30 second(s).
  ...
  real	0m30.381s
  user	0m0.017s
  sys	0m0.080s

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
v2: - make this a module param instead of an explicit sysfs file
    - drop the for loop; mdelay() does the same loop internally
    - decrease +36 lines of code and +2 lines of doc, to +6 lines of code and
      +2 lines of doc

 kernel/power/suspend.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index c347e3ce3a55..aee23dab0a55 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -28,6 +28,7 @@
 #include <linux/ftrace.h>
 #include <trace/events/power.h>
 #include <linux/compiler.h>
+#include <linux/moduleparam.h>
 
 #include "power.h"
 
@@ -204,12 +205,20 @@ static bool platform_suspend_again(suspend_state_t state)
 		suspend_ops->suspend_again() : false;
 }
 
+#ifdef CONFIG_PM_DEBUG
+static unsigned int pm_test_delay = 5;
+module_param(pm_test_delay, uint, 0644);
+MODULE_PARM_DESC(pm_test_delay,
+		 "Number of seconds to wait before resuming from suspend test");
+#endif
+
 static int suspend_test(int level)
 {
 #ifdef CONFIG_PM_DEBUG
 	if (pm_test_level == level) {
-		printk(KERN_INFO "suspend debug: Waiting for 5 seconds.\n");
-		mdelay(5000);
+		printk(KERN_INFO "suspend debug: Waiting for %d second(s).\n",
+				pm_test_delay);
+		mdelay(pm_test_delay * 1000);
 		return 1;
 	}
 #endif /* !CONFIG_PM_DEBUG */
-- 
2.3.0

Re: [PATCH v2] PM / sleep: add configurable delay for pm_test

[Pavel Machek]

On Sun 2015-02-22 00:26:54, Brian Norris wrote:
> When CONFIG_PM_DEBUG=y, we provide a sysfs file (/sys/power/pm_test) for
> selecting one of a few suspend test modes, where rather than entering a
> full suspend state, the kernel will perform some subset of suspend
> steps, wait 5 seconds, and then resume back to normal operation.
> 
> This mode is useful for (among other things) observing the state of the
> system just before entering a sleep mode, for debugging or analysis
> purposes. However, a constant 5 second wait is not sufficient for some
> sorts of analysis; for example, on an SoC, one might want to use
> external tools to probe the power states of various on-chip controllers
> or clocks.
> 
> This patch turns this 5 second delay into a configurable module
> parameter, so users can determine how long to wait in this
> pseudo-suspend state before resuming the system.
> 
> Example (wait 30 seconds);
> 
>   # echo 30 > /sys/module/suspend/parameters/pm_test_delay
>   # echo core > /sys/power/pm_test
>   # time echo mem  > /sys/power/state
>   ...
>   [   17.583625] suspend debug: Waiting for 30 second(s).
>   ...
>   real	0m30.381s
>   user	0m0.017s
>   sys	0m0.080s
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

(Does this need documentation in kernel-parameters.txt or in place
where pm_test is documented?)

Thanks,
								Pavel
								

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

Re: [PATCH v2] PM / sleep: add configurable delay for pm_test

[Kevin Cernekee]

On Sun, Feb 22, 2015 at 4:27 AM, Pavel Machek <pavel@ucw.cz> wrote:
> On Sun 2015-02-22 00:26:54, Brian Norris wrote:
>> When CONFIG_PM_DEBUG=y, we provide a sysfs file (/sys/power/pm_test) for
>> selecting one of a few suspend test modes, where rather than entering a
>> full suspend state, the kernel will perform some subset of suspend
>> steps, wait 5 seconds, and then resume back to normal operation.
>>
>> This mode is useful for (among other things) observing the state of the
>> system just before entering a sleep mode, for debugging or analysis
>> purposes. However, a constant 5 second wait is not sufficient for some
>> sorts of analysis; for example, on an SoC, one might want to use
>> external tools to probe the power states of various on-chip controllers
>> or clocks.
>>
>> This patch turns this 5 second delay into a configurable module
>> parameter, so users can determine how long to wait in this
>> pseudo-suspend state before resuming the system.
>>
>> Example (wait 30 seconds);
>>
>>   # echo 30 > /sys/module/suspend/parameters/pm_test_delay
>>   # echo core > /sys/power/pm_test
>>   # time echo mem  > /sys/power/state
>>   ...
>>   [   17.583625] suspend debug: Waiting for 30 second(s).
>>   ...
>>   real        0m30.381s
>>   user        0m0.017s
>>   sys 0m0.080s
>>
>> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
>
> Acked-by: Pavel Machek <pavel@ucw.cz>
>
> (Does this need documentation in kernel-parameters.txt or in place
> where pm_test is documented?)

Perhaps both places?  Documentation/power/basic-pm-debugging.txt still
has the "wait 5 seconds" verbiage.

Beyond that, it looks good to me:

Reviewed-by: Kevin Cernekee <cernekee@chromium.org>

Re: [PATCH v2] PM / sleep: add configurable delay for pm_test

[Florian Fainelli]

Le 22/02/2015 00:26, Brian Norris a écrit :
> When CONFIG_PM_DEBUG=y, we provide a sysfs file (/sys/power/pm_test) for
> selecting one of a few suspend test modes, where rather than entering a
> full suspend state, the kernel will perform some subset of suspend
> steps, wait 5 seconds, and then resume back to normal operation.
> 
> This mode is useful for (among other things) observing the state of the
> system just before entering a sleep mode, for debugging or analysis
> purposes. However, a constant 5 second wait is not sufficient for some
> sorts of analysis; for example, on an SoC, one might want to use
> external tools to probe the power states of various on-chip controllers
> or clocks.
> 
> This patch turns this 5 second delay into a configurable module
> parameter, so users can determine how long to wait in this
> pseudo-suspend state before resuming the system.
> 
> Example (wait 30 seconds);
> 
>   # echo 30 > /sys/module/suspend/parameters/pm_test_delay
>   # echo core > /sys/power/pm_test
>   # time echo mem  > /sys/power/state
>   ...
>   [   17.583625] suspend debug: Waiting for 30 second(s).
>   ...
>   real	0m30.381s
>   user	0m0.017s
>   sys	0m0.080s
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>

> ---
> v2: - make this a module param instead of an explicit sysfs file
>     - drop the for loop; mdelay() does the same loop internally
>     - decrease +36 lines of code and +2 lines of doc, to +6 lines of code and
>       +2 lines of doc
> 
>  kernel/power/suspend.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index c347e3ce3a55..aee23dab0a55 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -28,6 +28,7 @@
>  #include <linux/ftrace.h>
>  #include <trace/events/power.h>
>  #include <linux/compiler.h>
> +#include <linux/moduleparam.h>
>  
>  #include "power.h"
>  
> @@ -204,12 +205,20 @@ static bool platform_suspend_again(suspend_state_t state)
>  		suspend_ops->suspend_again() : false;
>  }
>  
> +#ifdef CONFIG_PM_DEBUG
> +static unsigned int pm_test_delay = 5;
> +module_param(pm_test_delay, uint, 0644);
> +MODULE_PARM_DESC(pm_test_delay,
> +		 "Number of seconds to wait before resuming from suspend test");
> +#endif
> +
>  static int suspend_test(int level)
>  {
>  #ifdef CONFIG_PM_DEBUG
>  	if (pm_test_level == level) {
> -		printk(KERN_INFO "suspend debug: Waiting for 5 seconds.\n");
> -		mdelay(5000);
> +		printk(KERN_INFO "suspend debug: Waiting for %d second(s).\n",
> +				pm_test_delay);
> +		mdelay(pm_test_delay * 1000);
>  		return 1;
>  	}
>  #endif /* !CONFIG_PM_DEBUG */
> 

[PATCH v3] PM / sleep: add configurable delay for pm_test

[Brian Norris]

When CONFIG_PM_DEBUG=y, we provide a sysfs file (/sys/power/pm_test) for
selecting one of a few suspend test modes, where rather than entering a
full suspend state, the kernel will perform some subset of suspend
steps, wait 5 seconds, and then resume back to normal operation.

This mode is useful for (among other things) observing the state of the
system just before entering a sleep mode, for debugging or analysis
purposes. However, a constant 5 second wait is not sufficient for some
sorts of analysis; for example, on an SoC, one might want to use
external tools to probe the power states of various on-chip controllers
or clocks.

This patch turns this 5 second delay into a configurable module
parameter, so users can determine how long to wait in this
pseudo-suspend state before resuming the system.

Example (wait 30 seconds);

  # echo 30 > /sys/module/suspend/parameters/pm_test_delay
  # echo core > /sys/power/pm_test
  # time echo mem  > /sys/power/state
  ...
  [   17.583625] suspend debug: Waiting for 30 second(s).
  ...
  real	0m30.381s
  user	0m0.017s
  sys	0m0.080s

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
Reviewed-by: Kevin Cernekee <cernekee@chromium.org>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
---
v3: - document in a few more places

v2: - make this a module param instead of an explicit sysfs file
    - drop the for loop; mdelay() does the same loop internally
    - decrease +36 lines of code and +2 lines of doc, to +6 lines of code and
      +2 lines of doc

 Documentation/kernel-parameters.txt        |  7 +++++++
 Documentation/power/basic-pm-debugging.txt | 10 ++++++----
 kernel/power/suspend.c                     | 13 +++++++++++--
 3 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 176d4fe4f076..0b8a1fd0d08d 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3433,6 +3433,13 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			improve throughput, but will also increase the
 			amount of memory reserved for use by the client.
 
+	suspend.pm_test_delay=
+			[SUSPEND]
+			Sets the number of seconds to remain in a suspend test
+			mode before resuming the system (see
+			/sys/power/pm_test). Only available when CONFIG_PM_DEBUG
+			is set. Default value is 5.
+
 	swapaccount=[0|1]
 			[KNL] Enable accounting of swap in memory resource
 			controller if no parameter or 1 is given or disable
diff --git a/Documentation/power/basic-pm-debugging.txt b/Documentation/power/basic-pm-debugging.txt
index edeecd447d23..b96098ccfe69 100644
--- a/Documentation/power/basic-pm-debugging.txt
+++ b/Documentation/power/basic-pm-debugging.txt
@@ -75,12 +75,14 @@ you should do the following:
 # echo platform > /sys/power/disk
 # echo disk > /sys/power/state
 
-Then, the kernel will try to freeze processes, suspend devices, wait 5 seconds,
-resume devices and thaw processes.  If "platform" is written to
+Then, the kernel will try to freeze processes, suspend devices, wait a few
+seconds (5 by default, but configurable by the suspend.pm_test_delay module
+parameter), resume devices and thaw processes.  If "platform" is written to
 /sys/power/pm_test , then after suspending devices the kernel will additionally
 invoke the global control methods (eg. ACPI global control methods) used to
-prepare the platform firmware for hibernation.  Next, it will wait 5 seconds and
-invoke the platform (eg. ACPI) global methods used to cancel hibernation etc.
+prepare the platform firmware for hibernation.  Next, it will wait a
+configurable number of seconds and invoke the platform (eg. ACPI) global
+methods used to cancel hibernation etc.
 
 Writing "none" to /sys/power/pm_test causes the kernel to switch to the normal
 hibernation/suspend operations.  Also, when open for reading, /sys/power/pm_test
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index c347e3ce3a55..aee23dab0a55 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -28,6 +28,7 @@
 #include <linux/ftrace.h>
 #include <trace/events/power.h>
 #include <linux/compiler.h>
+#include <linux/moduleparam.h>
 
 #include "power.h"
 
@@ -204,12 +205,20 @@ static bool platform_suspend_again(suspend_state_t state)
 		suspend_ops->suspend_again() : false;
 }
 
+#ifdef CONFIG_PM_DEBUG
+static unsigned int pm_test_delay = 5;
+module_param(pm_test_delay, uint, 0644);
+MODULE_PARM_DESC(pm_test_delay,
+		 "Number of seconds to wait before resuming from suspend test");
+#endif
+
 static int suspend_test(int level)
 {
 #ifdef CONFIG_PM_DEBUG
 	if (pm_test_level == level) {
-		printk(KERN_INFO "suspend debug: Waiting for 5 seconds.\n");
-		mdelay(5000);
+		printk(KERN_INFO "suspend debug: Waiting for %d second(s).\n",
+				pm_test_delay);
+		mdelay(pm_test_delay * 1000);
 		return 1;
 	}
 #endif /* !CONFIG_PM_DEBUG */
-- 
2.3.0