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 <[email protected]>
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
>
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 <[email protected]>
>
> 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
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 <[email protected]>
> >
> > 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
> > 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
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
> > 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
2014-12-17 1:22 GMT-08:00 Pavel Machek <[email protected]>:
>
>> > 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
> 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
2015-02-21 12:32 GMT-08:00 Pavel Machek <[email protected]>:
>
>
>
>> 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
On Sat 2015-02-21 14:56:01, Florian Fainelli wrote:
> 2015-02-21 12:32 GMT-08:00 Pavel Machek <[email protected]>:
> >
> >
> >
> >> 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
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