2008-01-17 06:25:46

by Andrew Morton

[permalink] [raw]
Subject: echo mem > /sys/power/state


So I take everyone's latest and greatest product and injudiciously type the
above command. The result five minutes later is at
http://userweb.kernel.org/~akpm/borkage.jpg. See if you can count all the bugs.

Sorry, but I've had it with this stuff and I'm tired of fixing everyone else's
stuff. I'm just going to ship it. Good luck.


2008-01-17 17:37:33

by Zan Lynx

[permalink] [raw]
Subject: Re: echo mem > /sys/power/state


On Wed, 2008-01-16 at 22:24 -0800, Andrew Morton wrote:
> So I take everyone's latest and greatest product and injudiciously type the
> above command. The result five minutes later is at
> http://userweb.kernel.org/~akpm/borkage.jpg. See if you can count all the bugs.
>
> Sorry, but I've had it with this stuff and I'm tired of fixing everyone else's
> stuff. I'm just going to ship it. Good luck.

Heh. Laptop suspend to anything has been so broken for so long in the
-mm series on my Compaq R3000 that I didn't even know it was ever
supposed to work.
--
Zan Lynx <[email protected]>


Attachments:
signature.asc (189.00 B)
This is a digitally signed message part

2008-01-17 18:19:32

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: echo mem > /sys/power/state

On Thursday, 17 of January 2008, Andrew Morton wrote:
>
> So I take everyone's latest and greatest product and injudiciously type the
> above command. The result five minutes later is at
> http://userweb.kernel.org/~akpm/borkage.jpg. See if you can count all the bugs.
>
> Sorry, but I've had it with this stuff and I'm tired of fixing everyone else's
> stuff. I'm just going to ship it. Good luck.

Like I said in another thread, please try without
pm-acquire-device-locks-on-suspend-rev-3.patch and without git-acpi.patch.
If that still doesn't work, it's not my fault. ;-)

2008-01-17 19:14:30

by Andrew Morton

[permalink] [raw]
Subject: Re: echo mem > /sys/power/state

On Thu, 17 Jan 2008 10:36:51 -0700 Zan Lynx <[email protected]> wrote:

>
> On Wed, 2008-01-16 at 22:24 -0800, Andrew Morton wrote:
> > So I take everyone's latest and greatest product and injudiciously type the
> > above command. The result five minutes later is at
> > http://userweb.kernel.org/~akpm/borkage.jpg. See if you can count all the bugs.
> >
> > Sorry, but I've had it with this stuff and I'm tired of fixing everyone else's
> > stuff. I'm just going to ship it. Good luck.
>
> Heh. Laptop suspend to anything has been so broken for so long in the
> -mm series on my Compaq R3000 that I didn't even know it was ever
> supposed to work.

It gets broken more often than anything else. I do test each release on
two laptops and I get to do a lot of bisection searching and
grumpygramming as a result.

Probably it would be more efficient to have the people who wrote the code
also test it.

2008-01-17 21:35:41

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: echo mem > /sys/power/state

On Thursday, 17 of January 2008, Andrew Morton wrote:
> On Thu, 17 Jan 2008 10:36:51 -0700 Zan Lynx <[email protected]> wrote:
>
> >
> > On Wed, 2008-01-16 at 22:24 -0800, Andrew Morton wrote:
> > > So I take everyone's latest and greatest product and injudiciously type the
> > > above command. The result five minutes later is at
> > > http://userweb.kernel.org/~akpm/borkage.jpg. See if you can count all the bugs.
> > >
> > > Sorry, but I've had it with this stuff and I'm tired of fixing everyone else's
> > > stuff. I'm just going to ship it. Good luck.
> >
> > Heh. Laptop suspend to anything has been so broken for so long in the
> > -mm series on my Compaq R3000 that I didn't even know it was ever
> > supposed to work.
>
> It gets broken more often than anything else. I do test each release on
> two laptops and I get to do a lot of bisection searching and
> grumpygramming as a result.
>
> Probably it would be more efficient to have the people who wrote the code
> also test it.

Well, that would certainly help.

I do test all of my patches and generally all of the patches I sign off, but
surely that's not enough.

2008-01-17 21:49:52

by Nigel Cunningham

[permalink] [raw]
Subject: Re: echo mem > /sys/power/state

Hi.

Rafael J. Wysocki wrote:
> On Thursday, 17 of January 2008, Andrew Morton wrote:
>> On Thu, 17 Jan 2008 10:36:51 -0700 Zan Lynx <[email protected]> wrote:
>>
>>> On Wed, 2008-01-16 at 22:24 -0800, Andrew Morton wrote:
>>>> So I take everyone's latest and greatest product and injudiciously type the
>>>> above command. The result five minutes later is at
>>>> http://userweb.kernel.org/~akpm/borkage.jpg. See if you can count all the bugs.
>>>>
>>>> Sorry, but I've had it with this stuff and I'm tired of fixing everyone else's
>>>> stuff. I'm just going to ship it. Good luck.
>>> Heh. Laptop suspend to anything has been so broken for so long in the
>>> -mm series on my Compaq R3000 that I didn't even know it was ever
>>> supposed to work.
>> It gets broken more often than anything else. I do test each release on
>> two laptops and I get to do a lot of bisection searching and
>> grumpygramming as a result.
>>
>> Probably it would be more efficient to have the people who wrote the code
>> also test it.
>
> Well, that would certainly help.
>
> I do test all of my patches and generally all of the patches I sign off, but
> surely that's not enough.

It is far too easy to take a cursory glance, say 'That looks okay' and
move on to the next thing, isn't it? I was horrified when I saw the list
of acks etc (including me) on the commit with the helper_unlock issue we
just fixed. It's truly scary to think that none of us looked closely
enough to pick that up at the time.

Nigel

2008-01-17 21:50:21

by Jiri Slaby

[permalink] [raw]
Subject: Re: echo mem > /sys/power/state

On 01/17/2008 08:13 PM, Andrew Morton wrote:
> On Thu, 17 Jan 2008 10:36:51 -0700 Zan Lynx <[email protected]> wrote:
>> Heh. Laptop suspend to anything has been so broken for so long in the
>> -mm series on my Compaq R3000 that I didn't even know it was ever
>> supposed to work.
>
> It gets broken more often than anything else. I do test each release on
> two laptops and I get to do a lot of bisection searching and
> grumpygramming as a result.
>
> Probably it would be more efficient to have the people who wrote the code
> also test it.

Big fat ACK from here. Suspend issues in past few -mms were *very* hard (and
time consuming) to track down.

Looking forward to test suspend of -rc8-mm1 :),
--js

2008-01-17 22:24:58

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: echo mem > /sys/power/state

On Thursday, 17 of January 2008, Nigel Cunningham wrote:
> Hi.
>
> Rafael J. Wysocki wrote:
> > On Thursday, 17 of January 2008, Andrew Morton wrote:
> >> On Thu, 17 Jan 2008 10:36:51 -0700 Zan Lynx <[email protected]> wrote:
> >>
> >>> On Wed, 2008-01-16 at 22:24 -0800, Andrew Morton wrote:
> >>>> So I take everyone's latest and greatest product and injudiciously type the
> >>>> above command. The result five minutes later is at
> >>>> http://userweb.kernel.org/~akpm/borkage.jpg. See if you can count all the bugs.
> >>>>
> >>>> Sorry, but I've had it with this stuff and I'm tired of fixing everyone else's
> >>>> stuff. I'm just going to ship it. Good luck.
> >>> Heh. Laptop suspend to anything has been so broken for so long in the
> >>> -mm series on my Compaq R3000 that I didn't even know it was ever
> >>> supposed to work.
> >> It gets broken more often than anything else. I do test each release on
> >> two laptops and I get to do a lot of bisection searching and
> >> grumpygramming as a result.
> >>
> >> Probably it would be more efficient to have the people who wrote the code
> >> also test it.
> >
> > Well, that would certainly help.
> >
> > I do test all of my patches and generally all of the patches I sign off, but
> > surely that's not enough.
>
> It is far too easy to take a cursory glance, say 'That looks okay' and
> move on to the next thing, isn't it? I was horrified when I saw the list
> of acks etc (including me) on the commit with the helper_unlock issue we
> just fixed. It's truly scary to think that none of us looked closely
> enough to pick that up at the time.

Well, the code in question was not in the patch, so if you didn't actually look
at the _patched_ kmod.c, you would have a little chance to spot it. I
overlooked it, even though I did look at the patched file ...

Still, this particular problem hasn't been triggering it testing for quite some
time.

Greetings,
Rafael

2008-01-18 08:37:14

by Ingo Molnar

[permalink] [raw]
Subject: Re: echo mem > /sys/power/state


* Rafael J. Wysocki <[email protected]> wrote:

> > Probably it would be more efficient to have the people who wrote the
> > code also test it.
>
> Well, that would certainly help.
>
> I do test all of my patches and generally all of the patches I sign
> off, but surely that's not enough.

please add a .config option dependent on CONFIG_DEBUG_KERNEL=y [and
default-disabled] that auto-tests suspend/resume functionality 60
seconds after hitting user-space (the suspend/resume cycle kept small
via a small RTC timeout) and s2ram correctness will be tested _a lot_
more.

(it doesnt matter if graphics does not resume fine - at least for my
tests)

kprobes had similar problems and it now has a few simple smoke-tests -
which i just saw trigger on a patch that i did not notice would break
kprobes. I think this should be done for all functionality that is not
regularly triggered by a normal distro bootup (and which is easy to
overlook in testing).

Ingo

2008-01-18 08:48:18

by Andrew Morton

[permalink] [raw]
Subject: Re: echo mem > /sys/power/state

On Fri, 18 Jan 2008 09:36:10 +0100 Ingo Molnar <[email protected]> wrote:

>
> * Rafael J. Wysocki <[email protected]> wrote:
>
> > > Probably it would be more efficient to have the people who wrote the
> > > code also test it.
> >
> > Well, that would certainly help.
> >
> > I do test all of my patches and generally all of the patches I sign
> > off, but surely that's not enough.
>
> please add a .config option dependent on CONFIG_DEBUG_KERNEL=y [and
> default-disabled] that auto-tests suspend/resume functionality 60
> seconds after hitting user-space (the suspend/resume cycle kept small
> via a small RTC timeout) and s2ram correctness will be tested _a lot_
> more.
>
> (it doesnt matter if graphics does not resume fine - at least for my
> tests)
>
> kprobes had similar problems and it now has a few simple smoke-tests -
> which i just saw trigger on a patch that i did not notice would break
> kprobes. I think this should be done for all functionality that is not
> regularly triggered by a normal distro bootup (and which is easy to
> overlook in testing).
>

Seeing as we're so lame about being able to distribute userspace stuff:
create a shell script in /proc/rc.kernel and start teaching initscripts to
run it. Then we can modify it at will.

I hate me.

2008-01-18 08:52:17

by Ingo Molnar

[permalink] [raw]
Subject: Re: echo mem > /sys/power/state


* Jiri Slaby <[email protected]> wrote:

> On 01/17/2008 08:13 PM, Andrew Morton wrote:
>> On Thu, 17 Jan 2008 10:36:51 -0700 Zan Lynx <[email protected]> wrote:
>>> Heh. Laptop suspend to anything has been so broken for so long in the
>>> -mm series on my Compaq R3000 that I didn't even know it was ever
>>> supposed to work.
>>
>> It gets broken more often than anything else. I do test each release on
>> two laptops and I get to do a lot of bisection searching and
>> grumpygramming as a result.
>>
>> Probably it would be more efficient to have the people who wrote the code
>> also test it.
>
> Big fat ACK from here. Suspend issues in past few -mms were *very*
> hard (and time consuming) to track down.

it's really all a matter of reducing latencies. Testing suspend/resume
is manual work currently (it either needs me hit a key on the laptop or
necessiates the use of a test-script that might or might not work
depending on whether the new /dev/rtc driver is enabled). So few people
besides those that rely on it will do it. The more a patch that breaks
suspend is out in the open unidentified, the more damage it does: it
gets into more trees, gets harder to bisect, etc.

So please give us overworked maintainers an easy to use .config option
dependent on CONFIG_DEBUG_KERNEL=y that automatically triggers a simple
suspend+resume sequence 60 seconds after bootup. It would be godsent.
(dont worry about proper gx resume) I compile and boot up every patch i
add to x86.git, so this would catch crap the moment we add it to the
tree.

The other, more long-term trick is to make rarely used functionality
more widely used. Consolidate code. Try to merge as much of
suspend/resume with bootup/module-insert/shutdown sequences as possible.
Suspend unused devices more agressively - such as non-mounted block
devices or downed networking ports. Try create more network effects with
other functionality, suspend and resume is not just about suspending
laptops, it can/could be used for so much more stuff. Try to get Pavel's
"Sleepy Linux" concept to work reasonably well - so that more people
(including developers) would use it in a daily basis.

Test coverage of a given piece of code is a direct function of its
utility and of its ease of testing. Decreeing "this is important" really
wont get more testing done. What you should realize i think is that this
is not a social/mindset problem (so no need to get frustrated about it),
this is a mostly technology problem: you can gradually _code_ your way
into people's test efforts.

Ingo

2008-01-18 09:04:30

by Harvey Harrison

[permalink] [raw]
Subject: Re: echo mem > /sys/power/state

On Fri, 2008-01-18 at 00:47 -0800, Andrew Morton wrote:
> On Fri, 18 Jan 2008 09:36:10 +0100 Ingo Molnar <[email protected]> wrote:
>
> >
> > * Rafael J. Wysocki <[email protected]> wrote:
> >
> > > > Probably it would be more efficient to have the people who wrote the
> > > > code also test it.
> > >
> > > Well, that would certainly help.
> > >
> > > I do test all of my patches and generally all of the patches I sign
> > > off, but surely that's not enough.
> >
> > please add a .config option dependent on CONFIG_DEBUG_KERNEL=y [and
> > default-disabled] that auto-tests suspend/resume functionality 60
> > seconds after hitting user-space (the suspend/resume cycle kept small
> > via a small RTC timeout) and s2ram correctness will be tested _a lot_
> > more.
> >
> > (it doesnt matter if graphics does not resume fine - at least for my
> > tests)
> >
> > kprobes had similar problems and it now has a few simple smoke-tests -
> > which i just saw trigger on a patch that i did not notice would break
> > kprobes. I think this should be done for all functionality that is not
> > regularly triggered by a normal distro bootup (and which is easy to
> > overlook in testing).
> >
>
> Seeing as we're so lame about being able to distribute userspace stuff:
> create a shell script in /proc/rc.kernel and start teaching initscripts to
> run it. Then we can modify it at will.
>
> I hate me.

With all the discussion lately about boot-time smoketests and self-tests
maybe this kind of stuff would be a good first candidate for useful
new early-userspace functionality. Then the kernel build could be
taught about building an initramfs that runs a bunch of tests
and leaves the user in a shell letting them know if it passed or not.

This would be a great way for increasing the number of testers, just
ask them to build with that option and a _known_ testsuite could
be reported as working or not.

Or maybe I'm out to lunch....

Harvey

2008-01-18 12:18:33

by Ingo Molnar

[permalink] [raw]
Subject: Re: echo mem > /sys/power/state


* Andrew Morton <[email protected]> wrote:

> > (it doesnt matter if graphics does not resume fine - at least for my
> > tests)
> >
> > kprobes had similar problems and it now has a few simple smoke-tests
> > - which i just saw trigger on a patch that i did not notice would
> > break kprobes. I think this should be done for all functionality
> > that is not regularly triggered by a normal distro bootup (and which
> > is easy to overlook in testing).
>
> Seeing as we're so lame about being able to distribute userspace
> stuff: create a shell script in /proc/rc.kernel and start teaching
> initscripts to run it. Then we can modify it at will.

would be fine to me. The problem isnt just distribution and the tests
getting out of sync with the kernel (and its capabilities enabled in the
.config, etc.), the other problem is unintrusive testing: i for example
use unmodified images of various distributions, with only a new bzImage
plopped in. That is an intentionally minimal impact test vector, any
userspace side changes are discouraged. And that's how many people test
new kernels, they just plop it in. The moment we require any userlevel
changes, the testing barrier increases significantly.

[ and a Friday rant: that's why the objection against my
make-relatime-actually-useful-to-people patch is so stupid,
shortsighted, self-defeating and misplaced. The "Dont put policy into
the kernel, use a new mount option and update your /etc/fstab"
stupidity that people have been repeating for 10 years without
thinking about it is a move that scares away 90% of our testers and
99% of the distros. See:

http://people.redhat.com/mingo/relatime-patches/improve-relatime.patch

Trivial improvement, it's 6 months and still not upstream. We are just
so good at shooting in our feet by creating artificial barriers and
making it as hard as possible for people to actually use our kernel
features ;-) ]

Ingo

2008-01-18 14:23:54

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: echo mem > /sys/power/state

On Friday, 18 of January 2008, Ingo Molnar wrote:
>
> * Andrew Morton <[email protected]> wrote:
>
> > > (it doesnt matter if graphics does not resume fine - at least for my
> > > tests)
> > >
> > > kprobes had similar problems and it now has a few simple smoke-tests
> > > - which i just saw trigger on a patch that i did not notice would
> > > break kprobes. I think this should be done for all functionality
> > > that is not regularly triggered by a normal distro bootup (and which
> > > is easy to overlook in testing).
> >
> > Seeing as we're so lame about being able to distribute userspace
> > stuff: create a shell script in /proc/rc.kernel and start teaching
> > initscripts to run it. Then we can modify it at will.
>
> would be fine to me.

Yes, that might work.

> The problem isnt just distribution and the tests
> getting out of sync with the kernel (and its capabilities enabled in the
> .config, etc.), the other problem is unintrusive testing: i for example
> use unmodified images of various distributions, with only a new bzImage
> plopped in. That is an intentionally minimal impact test vector, any
> userspace side changes are discouraged. And that's how many people test
> new kernels, they just plop it in. The moment we require any userlevel
> changes, the testing barrier increases significantly.

Very true.

Greetings,
Rafael

2008-01-20 23:39:16

by Pavel Machek

[permalink] [raw]
Subject: Re: echo mem > /sys/power/state

Hi!

Ingo wanted simple sleep self-test... Here's something.

set_alarm needs to move into rtc-cmos, and I guess I should boottest
it...

Pavel ~be careful what you wish for~ Machek

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 29cf145..d830ed2 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -78,7 +78,7 @@ static inline int is_intr(u8 rtc_intr)

/*----------------------------------------------------------------*/

-static int cmos_read_time(struct device *dev, struct rtc_time *t)
+int cmos_read_time(struct device *dev, struct rtc_time *t)
{
/* REVISIT: if the clock has a "century" register, use
* that instead of the heuristic in get_rtc_time().
@@ -170,7 +170,7 @@ static int cmos_read_alarm(struct device
return 0;
}

-static int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t)
+int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t)
{
struct cmos_rtc *cmos = dev_get_drvdata(dev);
unsigned char mon, mday, hrs, min, sec;
@@ -394,6 +394,7 @@ static const struct rtc_class_ops cmos_r
/*----------------------------------------------------------------*/

static struct cmos_rtc cmos_rtc;
+struct device *pc_rtc_device;

static irqreturn_t cmos_interrupt(int irq, void *p)
{
@@ -431,6 +432,8 @@ cmos_do_probe(struct device *dev, struct
if (cmos_rtc.dev)
return -EBUSY;

+ pc_rtc_device = dev;
+
if (!ports)
return -ENODEV;

@@ -546,7 +549,7 @@ cleanup0:

static void cmos_do_shutdown(void)
{
- unsigned char rtc_control;
+ unsigned char rtc_control;

spin_lock_irq(&rtc_lock);
rtc_control = CMOS_READ(RTC_CONTROL);

diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index 8e186c6..2886056 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -53,14 +53,20 @@ config PM_TRACE
RTC across reboots, so that you can debug a machine that just hangs
during suspend (or more commonly, during resume).

- To use this debugging feature you should attempt to suspend the machine,
- then reboot it, then run
+ To use this debugging feature you should attempt to suspend the
+ machine, then reboot it, then run

dmesg -s 1000000 | grep 'hash matches'

CAUTION: this option will cause your machine's real-time clock to be
set to an invalid time after a resume.

+config PM_SLEEPY_TEST
+ bool "Test suspend/resume during bootup"
+ depends on PM_DEBUG && PM_SLEEP
+ ---help---
+ This option will suspend/resume your machine during bootup.
+
config PM_SLEEP_SMP
bool
depends on SUSPEND_SMP_POSSIBLE || HIBERNATION_SMP_POSSIBLE
diff --git a/kernel/power/Makefile b/kernel/power/Makefile
index f7dfff2..e5693d6 100644
--- a/kernel/power/Makefile
+++ b/kernel/power/Makefile
@@ -5,7 +5,7 @@ endif

obj-y := main.o
obj-$(CONFIG_PM_LEGACY) += pm.o
-obj-$(CONFIG_PM_SLEEP) += process.o console.o
+obj-$(CONFIG_PM_SLEEP) += process.o console.o sleepy.o
obj-$(CONFIG_HIBERNATION) += swsusp.o disk.o snapshot.o swap.o user.o

obj-$(CONFIG_MAGIC_SYSRQ) += poweroff.o
diff --git a/kernel/power/sleepy.c b/kernel/power/sleepy.c
new file mode 100644
index 0000000..bb9bbaa
--- /dev/null
+++ b/kernel/power/sleepy.c
@@ -0,0 +1,68 @@
+/*
+ * Copyright (c) 2007 Pavel Machek <[email protected]>
+ *
+ * This file is released under the GPLv2
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/suspend.h>
+#include <linux/kobject.h>
+#include <linux/string.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/console.h>
+#include <linux/cpu.h>
+#include <linux/resume-trace.h>
+#include <linux/freezer.h>
+#include <linux/vmstat.h>
+#include <linux/syscalls.h>
+#include <linux/rtc.h>
+
+#include <asm/percpu.h>
+
+#include "power.h"
+
+
+extern struct device *pc_rtc_device;
+extern int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t);
+extern int cmos_read_time(struct device *dev, struct rtc_time *t);
+
+int set_alarm(int length)
+{
+ ssize_t retval;
+ unsigned long now, alarm;
+ struct rtc_wkalrm alm;
+ struct rtc_device *rtc = to_rtc_device(pc_rtc_device);
+
+ retval = cmos_read_time(pc_rtc_device, &alm.time);
+ if (retval < 0) {
+ printk("Auto sleep: can't get time?\n");
+ return retval;
+ }
+ rtc_tm_to_time(&alm.time, &now);
+ printk("Auto sleep: Now %ld\n", now);
+
+ alarm = now+length;
+ rtc_time_to_tm(alarm, &alm.time);
+
+ retval = cmos_set_alarm(rtc, &alm);
+ if (retval < 0) {
+ printk("Auto sleep: can't set alarm.\n");
+ return retval;
+ }
+ printk("Auto sleep: Alarm set\n");
+}
+
+#ifdef CONFIG_SLEEPY_TEST
+static int
+test_sleep(void)
+{
+ set_alarm(1);
+ pm_suspend(PM_SUSPEND_MEM);
+ return 0;
+}
+
+late_initcall(test_sleep);
+#endif

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

2008-01-20 23:42:54

by Ingo Molnar

[permalink] [raw]
Subject: Re: echo mem > /sys/power/state


* Pavel Machek <[email protected]> wrote:

> Hi!
>
> Ingo wanted simple sleep self-test... Here's something.

thanks :)

threw it into auto-qa, and it got thrown back with:

dione:~/linux/linux> ./err
kernel/power/sleepy.c: In function 'set_alarm':
kernel/power/sleepy.c:50: warning: passing argument 1 of 'cmos_set_alarm' from incompatible pointer type
kernel/power/sleepy.c:56: warning: control reaches end of non-void function
kernel/built-in.o: In function `set_alarm':
: undefined reference to `pc_rtc_device'
kernel/built-in.o: In function `set_alarm':
: undefined reference to `cmos_read_time'
kernel/built-in.o: In function `set_alarm':

config attached.

Ingo


Attachments:
(No filename) (667.00 B)
config (36.46 kB)
Download all attachments

2008-01-21 00:31:16

by Pavel Machek

[permalink] [raw]
Subject: Re: echo mem > /sys/power/state

On Mon 2008-01-21 00:42:08, Ingo Molnar wrote:
>
> * Pavel Machek <[email protected]> wrote:
>
> > Hi!
> >
> > Ingo wanted simple sleep self-test... Here's something.
>
> thanks :)
>
> threw it into auto-qa, and it got thrown back with:

Ok... it needs to depend on cmos_rtc... Plus timeout needs to be
longer, because if it fails to suspend within timeout, it will hang.

I actually tested this version... and it has some problems with
nohz. I have to press shift (probably any key will do) to make it
suspend :-(.

How big is your autotest farm, btw?
Pavel


diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 29cf145..d830ed2 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -78,7 +78,7 @@ static inline int is_intr(u8 rtc_intr)

/*----------------------------------------------------------------*/

-static int cmos_read_time(struct device *dev, struct rtc_time *t)
+int cmos_read_time(struct device *dev, struct rtc_time *t)
{
/* REVISIT: if the clock has a "century" register, use
* that instead of the heuristic in get_rtc_time().
@@ -170,7 +170,7 @@ static int cmos_read_alarm(struct device
return 0;
}

-static int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t)
+int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t)
{
struct cmos_rtc *cmos = dev_get_drvdata(dev);
unsigned char mon, mday, hrs, min, sec;
@@ -394,6 +394,7 @@ static const struct rtc_class_ops cmos_r
/*----------------------------------------------------------------*/

static struct cmos_rtc cmos_rtc;
+struct device *pc_rtc_device;

static irqreturn_t cmos_interrupt(int irq, void *p)
{
@@ -431,6 +432,8 @@ cmos_do_probe(struct device *dev, struct
if (cmos_rtc.dev)
return -EBUSY;

+ pc_rtc_device = dev;
+
if (!ports)
return -ENODEV;

@@ -546,7 +549,7 @@ cleanup0:

static void cmos_do_shutdown(void)
{
- unsigned char rtc_control;
+ unsigned char rtc_control;

spin_lock_irq(&rtc_lock);
rtc_control = CMOS_READ(RTC_CONTROL);
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index 8e186c6..b719fd3 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -53,14 +53,20 @@ config PM_TRACE
RTC across reboots, so that you can debug a machine that just hangs
during suspend (or more commonly, during resume).

- To use this debugging feature you should attempt to suspend the machine,
- then reboot it, then run
+ To use this debugging feature you should attempt to suspend the
+ machine, then reboot it, then run

dmesg -s 1000000 | grep 'hash matches'

CAUTION: this option will cause your machine's real-time clock to be
set to an invalid time after a resume.

+config PM_SLEEPY_TEST
+ bool "Test suspend/resume during bootup"
+ depends on PM_DEBUG && PM_SLEEP && RTC_DRV_CMOS
+ ---help---
+ This option will suspend/resume your machine during bootup.
+
config PM_SLEEP_SMP
bool
depends on SUSPEND_SMP_POSSIBLE || HIBERNATION_SMP_POSSIBLE
diff --git a/kernel/power/Makefile b/kernel/power/Makefile
index f7dfff2..e5693d6 100644
--- a/kernel/power/Makefile
+++ b/kernel/power/Makefile
@@ -5,7 +5,7 @@ endif

obj-y := main.o
obj-$(CONFIG_PM_LEGACY) += pm.o
-obj-$(CONFIG_PM_SLEEP) += process.o console.o
+obj-$(CONFIG_PM_SLEEP) += process.o console.o sleepy.o
obj-$(CONFIG_HIBERNATION) += swsusp.o disk.o snapshot.o swap.o user.o

obj-$(CONFIG_MAGIC_SYSRQ) += poweroff.o
diff --git a/kernel/power/sleepy.c b/kernel/power/sleepy.c
new file mode 100644
index 0000000..a437054
--- /dev/null
+++ b/kernel/power/sleepy.c
@@ -0,0 +1,70 @@
+/*
+ * Copyright (c) 2007 Pavel Machek <[email protected]>
+ *
+ * This file is released under the GPLv2
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/suspend.h>
+#include <linux/kobject.h>
+#include <linux/string.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/console.h>
+#include <linux/cpu.h>
+#include <linux/resume-trace.h>
+#include <linux/freezer.h>
+#include <linux/vmstat.h>
+#include <linux/syscalls.h>
+#include <linux/rtc.h>
+
+#include <asm/percpu.h>
+
+#include "power.h"
+
+extern int set_alarm(int length);
+
+extern struct device *pc_rtc_device;
+extern int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t);
+extern int cmos_read_time(struct device *dev, struct rtc_time *t);
+
+int set_alarm(int length)
+{
+ ssize_t retval;
+ unsigned long now, alarm;
+ struct rtc_wkalrm alm;
+ struct rtc_device *rtc = to_rtc_device(pc_rtc_device);
+
+ retval = cmos_read_time(pc_rtc_device, &alm.time);
+ if (retval < 0) {
+ printk("Auto sleep: can't get time?\n");
+ return retval;
+ }
+ rtc_tm_to_time(&alm.time, &now);
+ printk("Auto sleep: Now %ld\n", now);
+
+ alarm = now+length;
+ rtc_time_to_tm(alarm, &alm.time);
+
+ retval = cmos_set_alarm(rtc, &alm);
+ if (retval < 0) {
+ printk("Auto sleep: can't set alarm.\n");
+ return retval;
+ }
+ printk("Auto sleep: Alarm set\n");
+ return 0;
+}
+
+#ifdef CONFIG_PM_SLEEPY_TEST
+static int
+test_sleep(void)
+{
+ set_alarm(5);
+ pm_suspend(PM_SUSPEND_MEM);
+ return 0;
+}
+
+late_initcall(test_sleep);
+#endif

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

2008-01-21 02:13:33

by Johannes Weiner

[permalink] [raw]
Subject: Re: echo mem > /sys/power/state

Hi,

Pavel Machek <[email protected]> writes:

> diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
> index 29cf145..d830ed2 100644
> --- a/drivers/rtc/rtc-cmos.c
> +++ b/drivers/rtc/rtc-cmos.c
> @@ -78,7 +78,7 @@ static inline int is_intr(u8 rtc_intr)
>
> /*----------------------------------------------------------------*/
>
> -static int cmos_read_time(struct device *dev, struct rtc_time *t)
> +int cmos_read_time(struct device *dev, struct rtc_time *t)
> {
> /* REVISIT: if the clock has a "century" register, use
> * that instead of the heuristic in get_rtc_time().
> @@ -170,7 +170,7 @@ static int cmos_read_alarm(struct device
> return 0;
> }
>
> -static int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t)
> +int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t)
> {
> struct cmos_rtc *cmos = dev_get_drvdata(dev);
> unsigned char mon, mday, hrs, min, sec;
> @@ -394,6 +394,7 @@ static const struct rtc_class_ops cmos_r
> /*----------------------------------------------------------------*/
>
> static struct cmos_rtc cmos_rtc;
> +struct device *pc_rtc_device;
>
> static irqreturn_t cmos_interrupt(int irq, void *p)
> {
> @@ -431,6 +432,8 @@ cmos_do_probe(struct device *dev, struct
> if (cmos_rtc.dev)
> return -EBUSY;
>
> + pc_rtc_device = dev;
> +
> if (!ports)
> return -ENODEV;
>
> @@ -546,7 +549,7 @@ cleanup0:
>
> static void cmos_do_shutdown(void)
> {
> - unsigned char rtc_control;
> + unsigned char rtc_control;
>
> spin_lock_irq(&rtc_lock);
> rtc_control = CMOS_READ(RTC_CONTROL);
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index 8e186c6..b719fd3 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -53,14 +53,20 @@ config PM_TRACE
> RTC across reboots, so that you can debug a machine that just hangs
> during suspend (or more commonly, during resume).
>
> - To use this debugging feature you should attempt to suspend the machine,
> - then reboot it, then run
> + To use this debugging feature you should attempt to suspend the
> + machine, then reboot it, then run
>
> dmesg -s 1000000 | grep 'hash matches'
>
> CAUTION: this option will cause your machine's real-time clock to be
> set to an invalid time after a resume.
>
> +config PM_SLEEPY_TEST
> + bool "Test suspend/resume during bootup"
> + depends on PM_DEBUG && PM_SLEEP && RTC_DRV_CMOS
> + ---help---
> + This option will suspend/resume your machine during bootup.
> +
> config PM_SLEEP_SMP
> bool
> depends on SUSPEND_SMP_POSSIBLE || HIBERNATION_SMP_POSSIBLE
> diff --git a/kernel/power/Makefile b/kernel/power/Makefile
> index f7dfff2..e5693d6 100644
> --- a/kernel/power/Makefile
> +++ b/kernel/power/Makefile
> @@ -5,7 +5,7 @@ endif
>
> obj-y := main.o
> obj-$(CONFIG_PM_LEGACY) += pm.o
> -obj-$(CONFIG_PM_SLEEP) += process.o console.o
> +obj-$(CONFIG_PM_SLEEP) += process.o console.o sleepy.o

Why compiling it uncoditionally? What would be wrong with

obj-$(CONFIG_PM_SLEEPY_TEST += sleepy.o

(besides the missing parenthesis, of course)?

> obj-$(CONFIG_HIBERNATION) += swsusp.o disk.o snapshot.o swap.o user.o
>
> obj-$(CONFIG_MAGIC_SYSRQ) += poweroff.o
> diff --git a/kernel/power/sleepy.c b/kernel/power/sleepy.c
> new file mode 100644
> index 0000000..a437054
> --- /dev/null
> +++ b/kernel/power/sleepy.c
> @@ -0,0 +1,70 @@
> +/*
> + * Copyright (c) 2007 Pavel Machek <[email protected]>
> + *
> + * This file is released under the GPLv2
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/suspend.h>
> +#include <linux/kobject.h>
> +#include <linux/string.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/console.h>
> +#include <linux/cpu.h>
> +#include <linux/resume-trace.h>
> +#include <linux/freezer.h>
> +#include <linux/vmstat.h>
> +#include <linux/syscalls.h>
> +#include <linux/rtc.h>
> +
> +#include <asm/percpu.h>
> +
> +#include "power.h"
> +
> +extern int set_alarm(int length);

Huh?

> +
> +extern struct device *pc_rtc_device;
> +extern int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t);
> +extern int cmos_read_time(struct device *dev, struct rtc_time *t);
> +
> +int set_alarm(int length)
> +{
> + ssize_t retval;
> + unsigned long now, alarm;
> + struct rtc_wkalrm alm;
> + struct rtc_device *rtc = to_rtc_device(pc_rtc_device);
> +
> + retval = cmos_read_time(pc_rtc_device, &alm.time);
> + if (retval < 0) {
> + printk("Auto sleep: can't get time?\n");
> + return retval;
> + }
> + rtc_tm_to_time(&alm.time, &now);
> + printk("Auto sleep: Now %ld\n", now);
> +
> + alarm = now+length;
> + rtc_time_to_tm(alarm, &alm.time);
> +
> + retval = cmos_set_alarm(rtc, &alm);

cmos_set_alarm() takes a struct device * too.

> + if (retval < 0) {
> + printk("Auto sleep: can't set alarm.\n");
> + return retval;
> + }
> + printk("Auto sleep: Alarm set\n");
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEPY_TEST
> +static int
> +test_sleep(void)
> +{
> + set_alarm(5);
> + pm_suspend(PM_SUSPEND_MEM);
> + return 0;
> +}
> +
> +late_initcall(test_sleep);
> +#endif

Hannes

2008-01-21 22:37:54

by Pavel Machek

[permalink] [raw]
Subject: Re: echo mem > /sys/power/state

Hi!

> > index f7dfff2..e5693d6 100644
> > --- a/kernel/power/Makefile
> > +++ b/kernel/power/Makefile
> > @@ -5,7 +5,7 @@ endif
> >
> > obj-y := main.o
> > obj-$(CONFIG_PM_LEGACY) += pm.o
> > -obj-$(CONFIG_PM_SLEEP) += process.o console.o
> > +obj-$(CONFIG_PM_SLEEP) += process.o console.o sleepy.o
>
> Why compiling it uncoditionally? What would be wrong with

Its part of my world domination plan ;-).

> > +extern int set_alarm(int length);
>
> Huh?

Set alarm belongs in drivers/rtc, moved now.

> > + alarm = now+length;
> > + rtc_time_to_tm(alarm, &alm.time);
> > +
> > + retval = cmos_set_alarm(rtc, &alm);
>
> cmos_set_alarm() takes a struct device * too.

Fixed.

Thanks,
Pavel

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

2008-01-22 14:43:22

by Ingo Molnar

[permalink] [raw]
Subject: Re: echo mem > /sys/power/state


* Pavel Machek <[email protected]> wrote:

> Set alarm belongs in drivers/rtc, moved now.
>
> > > + alarm = now+length;
> > > + rtc_time_to_tm(alarm, &alm.time);
> > > +
> > > + retval = cmos_set_alarm(rtc, &alm);
> >
> > cmos_set_alarm() takes a struct device * too.
>
> Fixed.

any updated patch i could try? Thanks,

Ingo

2008-01-23 21:27:58

by Pavel Machek

[permalink] [raw]
Subject: Re: echo mem > /sys/power/state

Hi!

> > Set alarm belongs in drivers/rtc, moved now.
> >
> > > > + alarm = now+length;
> > > > + rtc_time_to_tm(alarm, &alm.time);
> > > > +
> > > > + retval = cmos_set_alarm(rtc, &alm);
> > >
> > > cmos_set_alarm() takes a struct device * too.
> >
> > Fixed.
>
> any updated patch i could try? Thanks,

Here you go, but notice that it needs nohz=off here. (Or press shift
few times, that makes it proceed). idle=poll also works around that
specific problem, but provokes deadlock in sysfs instead :-(.

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 29cf145..5bbdb70 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -78,7 +78,7 @@ static inline int is_intr(u8 rtc_intr)

/*----------------------------------------------------------------*/

-static int cmos_read_time(struct device *dev, struct rtc_time *t)
+int cmos_read_time(struct device *dev, struct rtc_time *t)
{
/* REVISIT: if the clock has a "century" register, use
* that instead of the heuristic in get_rtc_time().
@@ -170,7 +170,7 @@ static int cmos_read_alarm(struct device
return 0;
}

-static int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t)
+int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t)
{
struct cmos_rtc *cmos = dev_get_drvdata(dev);
unsigned char mon, mday, hrs, min, sec;
@@ -394,6 +394,33 @@ static const struct rtc_class_ops cmos_r
/*----------------------------------------------------------------*/

static struct cmos_rtc cmos_rtc;
+static struct device *pc_rtc_device;
+
+int set_alarm(int length)
+{
+ ssize_t retval;
+ unsigned long now, alarm;
+ struct rtc_wkalrm alm;
+
+ retval = cmos_read_time(pc_rtc_device, &alm.time);
+ if (retval < 0) {
+ printk("Auto sleep: can't get time?\n");
+ return retval;
+ }
+ rtc_tm_to_time(&alm.time, &now);
+ printk("Auto sleep: Now %ld\n", now);
+
+ alarm = now+length;
+ rtc_time_to_tm(alarm, &alm.time);
+
+ retval = cmos_set_alarm(pc_rtc_device, &alm);
+ if (retval < 0) {
+ printk("Auto sleep: can't set alarm.\n");
+ return retval;
+ }
+ printk("Auto sleep: Alarm set\n");
+ return 0;
+}

static irqreturn_t cmos_interrupt(int irq, void *p)
{
@@ -431,6 +458,8 @@ cmos_do_probe(struct device *dev, struct
if (cmos_rtc.dev)
return -EBUSY;

+ pc_rtc_device = dev;
+
if (!ports)
return -ENODEV;

@@ -546,7 +575,7 @@ cleanup0:

static void cmos_do_shutdown(void)
{
- unsigned char rtc_control;
+ unsigned char rtc_control;

spin_lock_irq(&rtc_lock);
rtc_control = CMOS_READ(RTC_CONTROL);
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index 8e186c6..b719fd3 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -53,14 +53,20 @@ config PM_TRACE
RTC across reboots, so that you can debug a machine that just hangs
during suspend (or more commonly, during resume).

- To use this debugging feature you should attempt to suspend the machine,
- then reboot it, then run
+ To use this debugging feature you should attempt to suspend the
+ machine, then reboot it, then run

dmesg -s 1000000 | grep 'hash matches'

CAUTION: this option will cause your machine's real-time clock to be
set to an invalid time after a resume.

+config PM_SLEEPY_TEST
+ bool "Test suspend/resume during bootup"
+ depends on PM_DEBUG && PM_SLEEP && RTC_DRV_CMOS
+ ---help---
+ This option will suspend/resume your machine during bootup.
+
config PM_SLEEP_SMP
bool
depends on SUSPEND_SMP_POSSIBLE || HIBERNATION_SMP_POSSIBLE
diff --git a/kernel/power/Makefile b/kernel/power/Makefile
index f7dfff2..e5693d6 100644
--- a/kernel/power/Makefile
+++ b/kernel/power/Makefile
@@ -5,7 +5,7 @@ endif

obj-y := main.o
obj-$(CONFIG_PM_LEGACY) += pm.o
-obj-$(CONFIG_PM_SLEEP) += process.o console.o
+obj-$(CONFIG_PM_SLEEP) += process.o console.o sleepy.o
obj-$(CONFIG_HIBERNATION) += swsusp.o disk.o snapshot.o swap.o user.o

obj-$(CONFIG_MAGIC_SYSRQ) += poweroff.o
diff --git a/kernel/power/sleepy.c b/kernel/power/sleepy.c
new file mode 100644
index 0000000..d363260
--- /dev/null
+++ b/kernel/power/sleepy.c
@@ -0,0 +1,39 @@
+/*
+ * Copyright (c) 2007 Pavel Machek <[email protected]>
+ *
+ * This file is released under the GPLv2
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/suspend.h>
+#include <linux/kobject.h>
+#include <linux/string.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/console.h>
+#include <linux/cpu.h>
+#include <linux/resume-trace.h>
+#include <linux/freezer.h>
+#include <linux/vmstat.h>
+#include <linux/syscalls.h>
+#include <linux/rtc.h>
+
+#include <asm/percpu.h>
+
+#include "power.h"
+
+extern int set_alarm(int length);
+
+#ifdef CONFIG_PM_SLEEPY_TEST
+static int
+test_sleep(void)
+{
+ set_alarm(5);
+ pm_suspend(PM_SUSPEND_MEM);
+ return 0;
+}
+
+late_initcall(test_sleep);
+#endif





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