[0/25] Merge pmdisk and swsusp

[0/25] Merge pmdisk and swsusp

[Patrick Mochel]

Hi there.

About a year ago, I became frustrated with the process of trying to merge
a bunch of cleanups that I had done to the swsusp (suspend-to-disk) code.
The reasons for this were numerous, but largely irrelevant at this point.
In an attempt to accelerate things, I forked the code, called it pmdisk,
and merged the cleanups. I had intended to merge the two, but circumstance
took another turn for the worst, and I was left with absolutely no time to
tend to it, leaving the net effect a major detriment to the overall
effort.

Forking the code was the wrong thing to do. I apologize to Pavel for
slighting him, and the users that are still left with a suspend-to-disk
implementation in limbo.

I've managed to shave off a bit of time, and have cut a set of patches
that merge the two, applicable against Linus's latest BK tree. No
functionality has been lost, and the cumulative benefit should be better
than the previous two efforts. The short summary of the patches follow
this email. The patches themselves are in seperate emails. I do not have a
publically accessible BK tree, but I can work on that if anyone desires
it.

In the end, these patches remove pmdisk from the kernel and clean up the
swsusp code base. The result is a single code base with greatly improved
code, that will hopefully help others underestand it better.

The swsusp code has also been integrated with the rest of the, albeit
small, Power Managment core. This removes a bit of code duplication, and
simplifies the main entry points a bit. The major benefit of this is that
swsusp does not depend on /proc/acpi/sleep or a modified sys_reboot()
system call to be present. It can be used by writing to /sys/power/state.
The other major plus is that it can leverage the real low-power states of
the platform (e.g. the ACPI S4 state), rather than always shutting the
machine down.

I've done a minimal amount of testing, as I am literally on my way out the
door Ottawa, but I have verified that it works on at least 1 Pentium-M
based laptop (a Compaq Evo N620c). I have not had a chance to port the
low-level changes to the x86-64 architecture. It's on the remaining TODO
list, along with writing a more formal explanation of the technical
changes for Documentation/

I'm interested to hear what people have to say about the patches and
encourage everyone to give them a try. [Though, considering many people
will be in Ottawa over the next week, I expect most feedback to come from
there.. ]

Thanks,


	Pat



Please pull from

	bk://kernel.bkbits.net:/home/mochel/linux-2.6-power

This will update the following files:

 arch/i386/power/pmdisk.S |   56 -
 kernel/power/pmdisk.c    |   35
 arch/i386/power/Makefile |    1
 arch/i386/power/pmdisk.S |    4
 arch/i386/power/swsusp.S |   78 --
 include/linux/suspend.h  |   20
 kernel/power/Kconfig     |   49 -
 kernel/power/Makefile    |    3
 kernel/power/disk.c      |   73 +
 kernel/power/main.c      |   15
 kernel/power/pmdisk.c    | 1321 ++---------------------------------
 kernel/power/power.h     |   23
 kernel/power/swsusp.c    | 1730 +++++++++++++++++++++++------------------------
 13 files changed, 1082 insertions(+), 2326 deletions(-)

through these ChangeSets:

<mochel@digitalimplant.org> (04/07/17 1.1867)
   [swsusp] Fix nasty typo.

<mochel@digitalimplant.org> (04/07/17 1.1866)
   [Power Mgmt] Merge swsusp entry points with the PM core.

   - Add {enable,disable}_nonboot_cpus() to prepare() and finish() in
     kernel/power/disk.c
   - Move swsusp __setup options there. Remove resume_status variable in favor
     of simpler 'noresume' variable, and check it in pm_resume().
   - Remove software_resume() from swsusp; rename pm_resume() to software_resume().
     The latter is considerably cleaner, and leverages the core code.
   - Move software_suspend() to kernel/power/main.c and shrink it down a
     wrapper that  takes pm_sem and calls pm_suspend_disk(), which does the
     same as the old software_suspend() did.
     This keeps the same entry points (via ACPI sleep and the reboot() syscall),
     but actually allows those to use the low-level power states of the system
     rather than always shutting off the system.
   - Remove now-unused functions from swsusp.

<mochel@digitalimplant.org> (04/07/17 1.1865)
   [swsusp] Remove unneeded suspend_pagedir_lock.

<mochel@digitalimplant.org> (04/07/17 1.1864)
   [Power Mgmt] Remove pmdisk.

   - Remove kernel/power/pmdisk.c.
   - Remove CONFIG_PM_STD config option.
   - Fix up Makefile.

<mochel@digitalimplant.org> (04/07/17 1.1863)
   [Power Mgmt] Make default partition config option part of swsusp.

   - Remove from pmdisk.
   - Remove pmdisk= command line option.

<mochel@digitalimplant.org> (04/07/17 1.1862)
   [Power Mgmt] Remove pmdisk_free()

   - Change name of free_suspend_pagedir() to swsusp_free().
   - Call from kernel/power/disk.c

<mochel@digitalimplant.org> (04/07/17 1.1861)
   [Power Mgmt] Merge pmdisk and swsusp write wrappers.

   - Merge suspend_save_image() from both into one.
   - Rename to swsusp_write().
   - Remove pmdisk_write().
   - Fixup call in kernel/power/disk.c and software_suspend().
   - Mark lock_swapdevices() static again.

<mochel@digitalimplant.org> (04/07/17 1.1860)
   [Power Mgmt] Merge pmdisk and swsusp read wrappers.

   - Merge pmdisk_read() and __read_suspend_image() and rename to swsusp_read()
   - Fix up call in kernel/power/disk.c to call new name.
   - Remove extra error checking from software_resume().

<mochel@digitalimplant.org> (04/07/17 1.1859)
   [Power Mgmt] Merge pmdisk and swsusp pagedir handling.

   This embodies the core of the swsusp->pmdisk cleanups. Instead of using the
   ->dummy variable at the end of each pagedir for a linked list of the page
   dirs, this uses a static array, which is kept in the empty space of the
   swsusp header.

   There are 768 entries, and could be scaled up based on the size of the page
   and the amount of room remaining. 768 should be enough anyway, since each
   entry is a swp_entry_t to a page-length array of pages. With larger systems
   and more memory come larger pages, so each page-sized array will
   automatically scale up.

   This replaces the read_suspend_image() and write_suspend_image() in swsusp
   with the much more concise pmdisk versions (not that big of change at this
   point) and fixes up the callers so software_resume() gets it right.

   Also, mark the helpers only used in swsusp as static again.

<mochel@digitalimplant.org> (04/07/17 1.1858)
   [Power Mgmt] Merge pmdisk and swsusp signature handling.

   - Move struct pmdisk_header definition to swsusp and change name to struct
     swsusp_header.
   - Statically allocate one (swsusp_header), and use it during mark_swapfiles()
     and when checking sig on resume.
   - Move check_sig() from pmdisk to swsusp.
   - Wrap with swsusp_verify(), and move check_header() there.
   - Fix up calls in pmdisk and swsusp.
   - Make new wrapper swsusp_close_swap() and call from write_suspend_image().
   - Look for swsusp_info info in swsusp_header.swsusp_info, instead of magic
     location at end of struct.

<mochel@digitalimplant.org> (04/07/17 1.1857)
   [swsusp] Fix nasty bug in calculating next address to read from.

   - The bio code already does this for us..

<mochel@digitalimplant.org> (04/07/17 1.1856)
   [Power Mgmt] Merge swsusp and pmdisk info headers.

   - Move definition of struct pmdsik_info to power.h and rename to struct
     swsusp_info.
   - Kill struct suspend_header.
   - Move helpers from pmdisk into swsusp: init_header(), dump_info(),
     write_header(), sanity_check(), check_header().
   - Fix up calls in pmdisk to call the right ones.
   - Clean up swsusp code to use helpers; delete duplicates.

<mochel@digitalimplant.org> (04/07/17 1.1855)
   [Power Mgmt] Merge pmdisk/swsusp image reading code.

   - Create swsusp_data_read() and call from read_suspend_image() in both
     swsusp and pmdisk.
   - Mark swsusp_pagedir_relocate() as static again.

<mochel@digitalimplant.org> (04/07/17 1.1854)
   [Power Mgmt] Consolidate pmdisk and swsusp early swap access.

   - Move bio helpers to swsusp.
   - Convert swsusp to use them, rathen buffer_heads.
   - Expose and fix up calls in pmdisk.
   - Clean up swsusp::read_suspend_image() a bit.

<mochel@digitalimplant.org> (04/07/17 1.1853)
   [Power Mgmt] Fix up call in kernel/power/disk.c to swsusp_suspend().

<mochel@digitalimplant.org> (04/07/17 1.1852)
   [Power Mgmt] Remove arch/i386/power/pmdisk.S

<mochel@digitalimplant.org> (04/07/17 1.1851)
   [Power Mgmt] Consolidate pmdisk and swsusp low-level handling.

   - Split do_magic into swsusp_arch_suspend() and swsusp_arch_resume().
   - Clean up based on pmdisk implementation
     - Only save registers we need to.
     - Use rep;movsl for copying, rather than doing each byte.
   - Create swsusp_suspend and swsusp_resume wrappers for calling the assmebly
     routines that:
     - Call {save,restore}_processor_state() in each.
     - Disable/enable interrupts in each.
   - Call swsusp_{suspend,restore} in software_{suspend,resume}
   - Kill all the do_magic_* functions.
   - Remove prototypes from linux/suspend.h
   - Remove similar pmdisk functions.
   - Update calls in kernel/power/disk.c to use swsusp versions.

<mochel@digitalimplant.org> (04/07/17 1.1850)
   [swsusp] Add helper suspend_finish, move common code there.

   - Move call out of assembly-callbacks and into software_suspend() after
     do_magic() returns.

<mochel@digitalimplant.org> (04/07/17 1.1849)
   [Power Mgmt] Consolidate page count/copy code of pmdisk and swsusp.

   - Split count_and_copy_data_pages() into count_data_pages() and
     copy_data_pages().
   - Move helper saveable() from pmdisk to swsusp, and update to work with
     page zones.
   - Get rid of uneeded defines in pmdisk.

<mochel@digitalimplant.org> (04/07/17 1.1848)
   [Power Mgmt] Consolidate code for allocating image pages in pmdisk and swsusp

   - Move helpers calc_order(), alloc_pagedir(), alloc_image_pages(),
     enough_free_mem(), and enough_swap() into swsusp.
   - Wrap them all with a new function - swsusp_alloc().
   - Fix up pmdisk to just call that.
   - Fix up suspend_prepare_image() to call that, instead of doing it inline.

<mochel@digitalimplant.org> (04/07/17 1.1847)
   [Power Mgmt] Merge first part of image writing code.

   - Introduce helpers to swsusp - swsusp_write_page(), swsusp_data_write() and
     swsusp_data_free().
   - Delete duplicate copies from pmdisk and fixup names in calls.
   - Clean up write_suspend_image() in swsusp and use the helpers.

<mochel@digitalimplant.org> (04/07/17 1.1846)
   [Power Mgmt] Share variables between pmdisk and swsusp.

   - In pmdisk, change pm_pagedir_nosave back to pagedir_nosave, and
     pmdisk_pages back to nr_copy_pages.
   - Mark them, and other page count/pagedir variables extern.
   - Make sure they're not static in swsusp.
   - Remove mention from include/linux/suspend.h, since no one else needs them.

<mochel@digitalimplant.org> (04/07/17 1.1845)
   [Power Mgmt] Remove more duplicate code from pmdisk.

   - Use read_swapfiles() in swsusp and rename to swsusp_swap_check().
   - Use lock_swapdevices() in swsusp and rename to swsusp_swap_lock().

<mochel@digitalimplant.org> (04/07/17 1.1844)
   [Power Mgmt] Remove duplicate relocate_pagedir() from pmdisk.

   - Expose and use version in swsusp.

<mochel@digitalimplant.org> (04/07/17 1.1843)
   [Power Mgmt] Make pmdisk dependent on swsusp.

Re: [0/25] Merge pmdisk and swsusp

[Pavel Machek]

Hi!

> In the end, these patches remove pmdisk from the kernel and clean up the
> swsusp code base. The result is a single code base with greatly improved
> code, that will hopefully help others underestand it better.

Thanks a lot for the patches.

> Please pull from
> 
> 	bk://kernel.bkbits.net:/home/mochel/linux-2.6-power

Unfortuanetly I can't just pull (I'm not allowed to use bitkeeper). I
could roll them into one big patch and then push them to akpm on my
own, but that would loose history :-(.

Patches #1 .. #4 are trivial enough to go in as soon as you want. I'd
prefer the rest of the patches to be tested in -mmX kernels a bit (for
a testing and so that I can do x86-64 support).. Comments to specific
patches follow.

								Pavel

> <mochel@digitalimplant.org> (04/07/17 1.1846)
>    [Power Mgmt] Share variables between pmdisk and swsusp.
> 
>    - In pmdisk, change pm_pagedir_nosave back to pagedir_nosave, and
>      pmdisk_pages back to nr_copy_pages.
>    - Mark them, and other page count/pagedir variables extern.
>    - Make sure they're not static in swsusp.
>    - Remove mention from include/linux/suspend.h, since no one else needs them.
> 
> <mochel@digitalimplant.org> (04/07/17 1.1845)
>    [Power Mgmt] Remove more duplicate code from pmdisk.
> 
>    - Use read_swapfiles() in swsusp and rename to swsusp_swap_check().
>    - Use lock_swapdevices() in swsusp and rename to swsusp_swap_lock().
> 
> <mochel@digitalimplant.org> (04/07/17 1.1844)
>    [Power Mgmt] Remove duplicate relocate_pagedir() from pmdisk.
> 
>    - Expose and use version in swsusp.
> 
> <mochel@digitalimplant.org> (04/07/17 1.1843)
>    [Power Mgmt] Make pmdisk dependent on swsusp.


-- 
Horseback riding is like software...
...vgf orggre jura vgf serr.

Re: [0/25] Merge pmdisk and swsusp

[Nigel Cunningham]

Hi.

I'll wait for you guys to complete your merge before I submit mine :>
(I'll probably need to rework parts for compatibility anyway).

Regards,

Nigel

Re: [0/25] Merge pmdisk and swsusp

[Andrew Morton]

Pavel Machek <pavel@ucw.cz> wrote:
>
> Unfortuanetly I can't just pull (I'm not allowed to use bitkeeper). I
> could roll them into one big patch and then push them to akpm on my
> own, but that would loose history :-(.

I'll just add Pat's BK URL to my list-of-bk-trees-to-add-to-mm-kernels. 
That brings it up to 25 external trees, believe it or not.

Pat, that means that anything which you commit gets autosucked into -mm, so
if there is a different URL which I should be using, please let me know.

Re: [0/25] Merge pmdisk and swsusp

[Pavel Machek]

Hi!

> Please pull from
> 
> 	bk://kernel.bkbits.net:/home/mochel/linux-2.6-power

I noticed that it now fixes swap signatures early, good.

> <mochel@digitalimplant.org> (04/07/17 1.1865)
>    [swsusp] Remove unneeded suspend_pagedir_lock.

How do you guarantee that while copying pages back and forth,
interrupts are disabled? They have to be, because memory snapshots are
no longer atomic.

In one patch, there's "Invalid partition" message when there's
incorrect signature may be confusing.

Otherwise patches are good. Perhaps it is easier to merge them with
Andrew now and fix remaining few issues with follow up patches?
								Pavel
-- 
Horseback riding is like software...
...vgf orggre jura vgf serr.

Re: [0/25] Merge pmdisk and swsusp

[Patrick Mochel]

On Mon, 19 Jul 2004, Pavel Machek wrote:

> > <mochel@digitalimplant.org> (04/07/17 1.1865)
> >    [swsusp] Remove unneeded suspend_pagedir_lock.
>
> How do you guarantee that while copying pages back and forth,
> interrupts are disabled? They have to be, because memory snapshots are
> no longer atomic.

(This was cleared up in person in Ottawa, but just for the record: )

Interrupts are disabled in swsusp_suspend() and swsusp_resume(), using
local_irq_disable(), instead of doing spin_lock_irq().


	Pat

Re: [0/25] Merge pmdisk and swsusp

[Pavel Machek]

Hi!

> In the end, these patches remove pmdisk from the kernel and clean up the
> swsusp code base. The result is a single code base with greatly improved
> code, that will hopefully help others underestand it better.

Followup patch:

* if machine halt fails, it is very dangerous to continue.

diff -ur linux.middle/kernel/power/disk.c linux/kernel/power/disk.c
--- linux.middle/kernel/power/disk.c	2004-07-19 08:58:08.000000000 -0700
+++ linux/kernel/power/disk.c	2004-07-19 15:00:16.000000000 -0700
@@ -63,6 +63,9 @@
 		break;
 	}
 	machine_halt();
+	/* Valid image is on the disk, if we continue we risk serious data corruption
+	   after resume. */
+	while(1);
 	device_power_up();
 	local_irq_restore(flags);
 	return 0;

* software_suspend() did not check for smp, this fixes it.

diff -ur linux.middle/kernel/power/main.c linux/kernel/power/main.c
--- linux.middle/kernel/power/main.c	2004-07-19 08:58:08.000000000 -0700
+++ linux/kernel/power/main.c	2004-07-20 08:32:43.000000000 -0700
@@ -175,13 +175,7 @@
  */
 int software_suspend(void)
 {
-	int error;
-
-	if (down_trylock(&pm_sem))
-		return -EBUSY;
-	error = pm_suspend_disk();
-	up(&pm_sem);
-	return error;
+	return enter_state(PM_SUSPEND_DISK);
 }
 
 

* copy_page() is dangerous. This is actually my fault.

diff -ur linux.middle/kernel/power/swsusp.c linux/kernel/power/swsusp.c
--- linux.middle/kernel/power/swsusp.c	2004-07-19 09:07:09.000000000 -0700
+++ linux/kernel/power/swsusp.c	2004-07-19 14:30:07.000000000 -0700
@@ -614,12 +614,8 @@
 					struct page * page;
 					page = pfn_to_page(zone_pfn + zone->zone_start_pfn);
 					pbe->orig_address = (long) page_address(page);
-					/* Copy page is dangerous: it likes to mess with
-					   preempt count on specific cpus. Wrong preempt
-					   count is then copied, oops.
-					*/
-					copy_page((void *)pbe->address,
-						  (void *)pbe->orig_address);
+					/* copy_page is no usable for copying task structs. */
+					memcpy((void *)pbe->address, (void *)pbe->orig_address, PAGE_SIZE);
 					pbe++;
 				}
 			}


-- 
Horseback riding is like software...
...vgf orggre jura vgf serr.

Re: [0/25] Merge pmdisk and swsusp

[sam]

On Tue, Jul 20, 2004 at 06:46:40PM +0200, Pavel Machek wrote:
> Hi!
> 
> > In the end, these patches remove pmdisk from the kernel and clean up the
> > swsusp code base. The result is a single code base with greatly improved
> > code, that will hopefully help others underestand it better.
> 
> Followup patch:
> 
> * if machine halt fails, it is very dangerous to continue.
> 
> diff -ur linux.middle/kernel/power/disk.c linux/kernel/power/disk.c
> --- linux.middle/kernel/power/disk.c	2004-07-19 08:58:08.000000000 -0700
> +++ linux/kernel/power/disk.c	2004-07-19 15:00:16.000000000 -0700
> @@ -63,6 +63,9 @@
>  		break;
>  	}
>  	machine_halt();
> +	/* Valid image is on the disk, if we continue we risk serious data corruption
> +	   after resume. */
> +	while(1);

Would be nicer to use:

	while(1)
		/* Loop forever */;

	Sam

Re: [0/25] Merge pmdisk and swsusp

[Dmitry Torokhov]

On Tuesday 20 July 2004 02:28 pm, sam@ravnborg.org wrote:
> On Tue, Jul 20, 2004 at 06:46:40PM +0200, Pavel Machek wrote:
> > Hi!
> > 
> > > In the end, these patches remove pmdisk from the kernel and clean up the
> > > swsusp code base. The result is a single code base with greatly improved
> > > code, that will hopefully help others underestand it better.
> > 
> > Followup patch:
> > 
> > * if machine halt fails, it is very dangerous to continue.
> > 
> > diff -ur linux.middle/kernel/power/disk.c linux/kernel/power/disk.c
> > --- linux.middle/kernel/power/disk.c	2004-07-19 08:58:08.000000000 -0700
> > +++ linux/kernel/power/disk.c	2004-07-19 15:00:16.000000000 -0700
> > @@ -63,6 +63,9 @@
> >  		break;
> >  	}
> >  	machine_halt();
> > +	/* Valid image is on the disk, if we continue we risk serious data corruption
> > +	   after resume. */
> > +	while(1);
> 
> Would be nicer to use:
> 
> 	while(1)
> 		/* Loop forever */;
> 
> 	Sam

And even nicer would be remove swsusp signature from swap and restore state as
original version did so user could do clean shutdown...

-- 
Dmitry

Re: [0/25] Merge pmdisk and swsusp

[Pavel Machek]

Hi!

> > > diff -ur linux.middle/kernel/power/disk.c linux/kernel/power/disk.c
> > > --- linux.middle/kernel/power/disk.c	2004-07-19 08:58:08.000000000 -0700
> > > +++ linux/kernel/power/disk.c	2004-07-19 15:00:16.000000000 -0700
> > > @@ -63,6 +63,9 @@
> > >  		break;
> > >  	}
> > >  	machine_halt();
> > > +	/* Valid image is on the disk, if we continue we risk serious data corruption
> > > +	   after resume. */
> > > +	while(1);
> > 
> > Would be nicer to use:
> > 
> > 	while(1)
> > 		/* Loop forever */;
> > 
> > 	Sam
> 
> And even nicer would be remove swsusp signature from swap and restore state as
> original version did so user could do clean shutdown...

Actually, at this point you are expected to just power down, and what
you wanted is done: machine is suspended to disk.

No need to kill signature and make swsusp unusable when powerdown does
not work.
								Pavel
-- 
Horseback riding is like software...
...vgf orggre jura vgf serr.

Re: [0/25] Merge pmdisk and swsusp

[Patrick Mochel]

On Tue, 20 Jul 2004, Pavel Machek wrote:

> Followup patch:
>
> * if machine halt fails, it is very dangerous to continue.
>
> diff -ur linux.middle/kernel/power/disk.c linux/kernel/power/disk.c
> --- linux.middle/kernel/power/disk.c	2004-07-19 08:58:08.000000000 -0700
> +++ linux/kernel/power/disk.c	2004-07-19 15:00:16.000000000 -0700
> @@ -63,6 +63,9 @@
>  		break;
>  	}
>  	machine_halt();
> +	/* Valid image is on the disk, if we continue we risk serious data corruption
> +	   after resume. */
> +	while(1);
>  	device_power_up();
>  	local_irq_restore(flags);
>  	return 0;

This is nasty. We have to fail gracefully, ideally without expecting user
input.

Adding 'while(1)' will cause the CPU to enter a busy loop, artificially
increasing the power consumption of the system, which would be counter-
productive in a system that was configured to suspend when the battery was
low.

We need to at least print a message specifying what happened and
instructing them to reboot. It's dorky, but over time, all every system
should eventually be fixed to either enter a low-power mode or shut down
properly.

Perhaps we could also fill in machine_halt(), which the patch below also
does.

> * software_suspend() did not check for smp, this fixes it.

Applied, thanks.

> * copy_page() is dangerous. This is actually my fault.

Why is copy_page() dangerous? Shouldn't it be fixed if that is the case?

Thanks,


	Pat
===== arch/i386/kernel/reboot.c 1.16 vs edited =====
--- 1.16/arch/i386/kernel/reboot.c	2004-07-05 03:28:50 -07:00
+++ edited/arch/i386/kernel/reboot.c	2004-08-01 22:40:58 -07:00
@@ -367,6 +367,8 @@

 void machine_halt(void)
 {
+	while (1)
+		asm volatile ("hlt":::"memory");
 }

 EXPORT_SYMBOL(machine_halt);
===== kernel/power/disk.c 1.16 vs edited =====
--- 1.16/kernel/power/disk.c	2004-08-01 20:36:39 -07:00
+++ edited/kernel/power/disk.c	2004-08-01 22:38:19 -07:00
@@ -59,6 +59,7 @@
 		machine_restart(NULL);
 		break;
 	}
+	printk(KERN_EMERG "Suspend-to-disk succeeded, but power-down failed. Please reboot.\n");
 	machine_halt();
 	device_power_up();
 	local_irq_restore(flags);

Re: [0/25] Merge pmdisk and swsusp

[Pavel Machek]

Hi!

> > * if machine halt fails, it is very dangerous to continue.
> >
> > diff -ur linux.middle/kernel/power/disk.c linux/kernel/power/disk.c
> > --- linux.middle/kernel/power/disk.c	2004-07-19 08:58:08.000000000 -0700
> > +++ linux/kernel/power/disk.c	2004-07-19 15:00:16.000000000 -0700
> > @@ -63,6 +63,9 @@
> >  		break;
> >  	}
> >  	machine_halt();
> > +	/* Valid image is on the disk, if we continue we risk serious data corruption
> > +	   after resume. */
> > +	while(1);
> >  	device_power_up();
> >  	local_irq_restore(flags);
> >  	return 0;
> 
> This is nasty. We have to fail gracefully, ideally without expecting user
> input.
> 
> Adding 'while(1)' will cause the CPU to enter a busy loop, artificially
> increasing the power consumption of the system, which would be counter-
> productive in a system that was configured to suspend when the battery was
> low.

> We need to at least print a message specifying what happened and
> instructing them to reboot. It's dorky, but over time, all every system
> should eventually be fixed to either enter a low-power mode or shut down
> properly.

Ok, it was a "too hot hotfix". Your solution is better (but see below).

> Perhaps we could also fill in machine_halt(), which the patch below also
> does.

Good.

> > * copy_page() is dangerous. This is actually my fault.
> 
> Why is copy_page() dangerous? Shouldn't it be fixed if that is the
> case?

copy_page sometimes changes struct task_struct, does copy, changes it
back. That makes it bad choice for copying task_structs,
unfortunately. Do you want me to retransmit the patch?

> ===== kernel/power/disk.c 1.16 vs edited =====
> --- 1.16/kernel/power/disk.c	2004-08-01 20:36:39 -07:00
> +++ edited/kernel/power/disk.c	2004-08-01 22:38:19 -07:00
> @@ -59,6 +59,7 @@
>  		machine_restart(NULL);
>  		break;
>  	}
> +	printk(KERN_EMERG "Suspend-to-disk succeeded, but power-down failed. Please reboot.\n");
>  	machine_halt();
>  	device_power_up();
>  	local_irq_restore(flags);

If i386 got it wrong, it is possible that other architectures get it
wrong, too. Fixing i386 is good, but we should not risk continuing
machine with valid image on disk.

I guess "device_power_up / local_irq_restore" should be replaced with
BUG() or while(1)?

								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: [0/25] Merge pmdisk and swsusp

[Felipe Alfaro Solana]

On Sat, 2004-07-17 at 15:34 -0700, Patrick Mochel wrote:

> I'm interested to hear what people have to say about the patches and
> encourage everyone to give them a try. [Though, considering many people
> will be in Ottawa over the next week, I expect most feedback to come from
> there.. ]

I have been using this pathset since 2.6.8-rc1 and PM features like
Suspend (S3) and Hibernate (S4) keep working fine on my laptop. Thanks.