Re: [0/25] Merge pmdisk and swsusp

[Pavel Machek <pavel@suse.cz>]

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!