Re: swsusp 'disk' fails in bk-current - intel_agp at fault?

Re: swsusp 'disk' fails in bk-current - intel_agp at fault?

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1771 bytes --]

Hi!

> > > Well, there lies a problem - some devices have to do execve because
> > > they need firmware to operate. Also, again, some buses with
> > > hot-pluggable devices will attempt to clean up unsuccessful resume and
> > > this will cause hotplug events. The point is you either resume system
> > > or you don't. We probably need a separate "unfreeze" callback,
> > > although this is kind of messy.
> > 
> > There's a better solution for firmware: You should load your firmware
> > prior to suspend and store it in RAM. Anything else just plain does
> > not work. (Because your wireless firmware might be on NFS mounted over
> > that wireless card).
> > 
> > Hotplug... I guess udev just needs to hold that callbacks before
> > system is fully up... it has to do something similar on regular boot,
> > no?
> 
> Well, I did not really look into udev but hotplug (which can iteract
> with udev) does not keep anything. If it fails its ok - that's why
> there are coldplug scripts that "recover" lost events. But here we
> block trying to start hotplug - we not getting an error - and this is
> bad. Unfortunately I am not familiar with block devices working to say
> why it hangs.
> 
> Should we pull Jens into the discussion?

I don't really want us to try execve during resume... Could we simply
artifically fail that execve with something if (in_suspend()) return
-EINVAL; [except that in_suspend() just is not there, but there were
some proposals to add it].

Or just avoid calling hotplug at all in resume case? And then do
coldplug-like scan when userspace is ready...

But we perhaps should cc linux-pm list.
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: swsusp 'disk' fails in bk-current - intel_agp at fault?

[Dmitry Torokhov]

On Tue, 29 Mar 2005 22:52:25 +0200, Pavel Machek <pavel-AlSwsSmVLrQ@public.gmane.org> wrote:
> I don't really want us to try execve during resume... Could we simply
> artifically fail that execve with something if (in_suspend()) return
> -EINVAL; [except that in_suspend() just is not there, but there were
> some proposals to add it].
> 
> Or just avoid calling hotplug at all in resume case? And then do
> coldplug-like scan when userspace is ready...
> 

I am leaning towards calling disable_usermodehelper (not writtent yet)
after swsusp completes snapshotting memory. We really don't care about
hotplug events in this case and this will allow keeping "normal"
resume in drivers as is. What do you think?

-- 
Dmitry

Re: swsusp 'disk' fails in bk-current - intel_agp at fault?

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 978 bytes --]

Hi!

> > I don't really want us to try execve during resume... Could we simply
> > artifically fail that execve with something if (in_suspend()) return
> > -EINVAL; [except that in_suspend() just is not there, but there were
> > some proposals to add it].
> > 
> > Or just avoid calling hotplug at all in resume case? And then do
> > coldplug-like scan when userspace is ready...
> > 
> 
> I am leaning towards calling disable_usermodehelper (not writtent yet)
> after swsusp completes snapshotting memory. We really don't care about
> hotplug events in this case and this will allow keeping "normal"
> resume in drivers as is. What do you think?

That would certianly do the trick.

[Or perhaps in_suspend() is slightly nicer solution? People wanted it
for other stuff (sanity checking, like BUG_ON(in_suspend())), too....]

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Re: swsusp 'disk' fails in bk-current - intel_agp at fault?

[Patrick Mochel]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 664 bytes --]


On Tue, 29 Mar 2005, Pavel Machek wrote:

> I don't really want us to try execve during resume... Could we simply
> artifically fail that execve with something if (in_suspend()) return
> -EINVAL; [except that in_suspend() just is not there, but there were
> some proposals to add it].
>
> Or just avoid calling hotplug at all in resume case? And then do
> coldplug-like scan when userspace is ready...

I thought that cold-plugging only worked for devices, not all objects.

Can we just queue up hotplug events? That way we wouldn't lose any across
the transition, and could be used to send resume events to userspace for
various devices that need help..


	Pat


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: swsusp 'disk' fails in bk-current - intel_agp at fault?

[Dmitry Torokhov]

On Tue, 29 Mar 2005 23:12:39 +0200, Pavel Machek <pavel-AlSwsSmVLrQ@public.gmane.org> wrote:
> >
> > I am leaning towards calling disable_usermodehelper (not writtent yet)
> > after swsusp completes snapshotting memory. We really don't care about
> > hotplug events in this case and this will allow keeping "normal"
> > resume in drivers as is. What do you think?
> 
> That would certianly do the trick.
> 
> [Or perhaps in_suspend() is slightly nicer solution? People wanted it
> for other stuff (sanity checking, like BUG_ON(in_suspend())), too....]
> 

We might want having both... Hmm... in_suspend - is it only for swsusp
(in_swsusp) or for suspend-to-ram as well? For suspend to ram we might
need slightly different rules, I don't know. A separate call will
allow more fine-grained control and will explicitely tell reader what
is happening.

I do not have a strong preference though.

-- 
Dmitry

Re: Re: swsusp 'disk' fails in bk-current - intel_agp at fault?

[Dmitry Torokhov]

On Tue, 29 Mar 2005 13:23:35 -0800 (PST), Patrick Mochel
<mochel-og82NyAXoxR0I81aPRRTmR2eb7JE58TQ@public.gmane.org> wrote:
> 
> On Tue, 29 Mar 2005, Pavel Machek wrote:
> 
> > I don't really want us to try execve during resume... Could we simply
> > artifically fail that execve with something if (in_suspend()) return
> > -EINVAL; [except that in_suspend() just is not there, but there were
> > some proposals to add it].
> >
> > Or just avoid calling hotplug at all in resume case? And then do
> > coldplug-like scan when userspace is ready...
> 
> I thought that cold-plugging only worked for devices, not all objects.
> 

It really depens on the script - nothing stops it from traversing
entire /sys tree and if an object it not exported in the tree I'd say
userspace should not care about such object anyway.

> Can we just queue up hotplug events? That way we wouldn't lose any across
> the transition, and could be used to send resume events to userspace for
> various devices that need help..
>

The point is that at this point any changes to the system state will
be discarded - we already did the image and about to write it. When we
resume for real all those events will be regenerated once again.
 
-- 
Dmitry

Re: swsusp 'disk' fails in bk-current - intel_agp at fault?

[Pavel Machek]

On Út 29-03-05 16:33:04, Dmitry Torokhov wrote:
> On Tue, 29 Mar 2005 23:12:39 +0200, Pavel Machek <pavel-AlSwsSmVLrQ@public.gmane.org> wrote:
> > >
> > > I am leaning towards calling disable_usermodehelper (not writtent yet)
> > > after swsusp completes snapshotting memory. We really don't care about
> > > hotplug events in this case and this will allow keeping "normal"
> > > resume in drivers as is. What do you think?
> > 
> > That would certianly do the trick.
> > 
> > [Or perhaps in_suspend() is slightly nicer solution? People wanted it
> > for other stuff (sanity checking, like BUG_ON(in_suspend())), too....]
> > 
> 
> We might want having both... Hmm... in_suspend - is it only for swsusp
> (in_swsusp) or for suspend-to-ram as well? For suspend to ram we might
> need slightly different rules, I don't know. A separate call will
> allow more fine-grained control and will explicitely tell reader what
> is happening.

We currently freeze processes for suspend-to-ram, too. I guess that
disable_usermodehelper is probably better and that in_suspend() should
only be used for sanity checks... go with disable_usermodehelper and
sorry for the noise.
								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: Re: swsusp 'disk' fails in bk-current - intel_agp at fault?

[Nigel Cunningham]

[-- Attachment #1: Type: text/plain, Size: 962 bytes --]

Hi.

On Wed, 2005-03-30 at 07:44, Pavel Machek wrote:
> We currently freeze processes for suspend-to-ram, too. I guess that
> disable_usermodehelper is probably better and that in_suspend() should
> only be used for sanity checks... go with disable_usermodehelper and
> sorry for the noise.

Here's another possibility: Freeze the workqueue that
call_usermodehelper uses (remember that code I didn't push hard enough
to Andrew?), and let invocations of call_usermodehelper block in
TASK_UNINTERRUPTIBLE. In refrigerating processes, don't choke on kernel
processes in that state. Of course if you won't want the freeze
processes for str, but do want to freeze call_usermodehelper, I guess
you'd still need the in_suspend() macro.

Regards,

Nigel
-- 
Nigel Cunningham
Software Engineer, Canberra, Australia
http://www.cyclades.com
Bus: +61 (2) 6291 9554; Hme: +61 (2) 6292 8028;  Mob: +61 (417) 100 574

Maintainer of Suspend2 Kernel Patches http://suspend2.net


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Re: swsusp 'disk' fails in bk-current - intel_agp at fault?

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 860 bytes --]

Hi!

> > We currently freeze processes for suspend-to-ram, too. I guess that
> > disable_usermodehelper is probably better and that in_suspend() should
> > only be used for sanity checks... go with disable_usermodehelper and
> > sorry for the noise.
> 
> Here's another possibility: Freeze the workqueue that
> call_usermodehelper uses (remember that code I didn't push hard enough
> to Andrew?), and let invocations of call_usermodehelper block in
> TASK_UNINTERRUPTIBLE. In refrigerating processes, don't choke on

There may be many devices in the system, and you are going to need
quite a lot of RAM for all that... That's why they do not queue it
during boot, IIRC. Disabling usermode helper seems right.
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: swsusp 'disk' fails in bk-current - intel_agp at fault?

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 1324 bytes --]

Hi,

On Tuesday, 29 of March 2005 23:12, Pavel Machek wrote:
> Hi!
> 
> > > I don't really want us to try execve during resume... Could we simply
> > > artifically fail that execve with something if (in_suspend()) return
> > > -EINVAL; [except that in_suspend() just is not there, but there were
> > > some proposals to add it].
> > > 
> > > Or just avoid calling hotplug at all in resume case? And then do
> > > coldplug-like scan when userspace is ready...
> > > 
> > 
> > I am leaning towards calling disable_usermodehelper (not writtent yet)
> > after swsusp completes snapshotting memory. We really don't care about
> > hotplug events in this case and this will allow keeping "normal"
> > resume in drivers as is. What do you think?
> 
> That would certianly do the trick.
> 
> [Or perhaps in_suspend() is slightly nicer solution? People wanted it
> for other stuff (sanity checking, like BUG_ON(in_suspend())), too....]

IMHO, they are not mutually exclusive.    However, by using
disable_usermodehelper we would get rid of the reason (ie hotplug events)
instead of just curing the symptoms (ie execve() during suspend).

Greets,
Rafael


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Re: swsusp 'disk' fails in bk-current - intel_agp at fault?

[Nigel Cunningham]

[-- Attachment #1: Type: text/plain, Size: 3754 bytes --]

Hi.

On Wed, 2005-03-30 at 08:35, Pavel Machek wrote:
> Hi!
> 
> > > We currently freeze processes for suspend-to-ram, too. I guess that
> > > disable_usermodehelper is probably better and that in_suspend() should
> > > only be used for sanity checks... go with disable_usermodehelper and
> > > sorry for the noise.
> > 
> > Here's another possibility: Freeze the workqueue that
> > call_usermodehelper uses (remember that code I didn't push hard enough
> > to Andrew?), and let invocations of call_usermodehelper block in
> > TASK_UNINTERRUPTIBLE. In refrigerating processes, don't choke on
> 
> There may be many devices in the system, and you are going to need
> quite a lot of RAM for all that... That's why they do not queue it
> during boot, IIRC. Disabling usermode helper seems right.

Many devices is true, but very few of them invoke usermode helpers.

[desktop build-2.6.12-rc1]# find -name *.[ch] | xargs grep usermodehelper
./drivers/s390/crypto/z90main.c:        call_usermodehelper(argv[0], argv, envp, 0);
./drivers/net/hamradio/baycom_epp.c:    return call_usermodehelper(eppconfig_path, argv, envp, 1);
./drivers/acpi/thermal.c:       call_usermodehelper(argv[0], argv, envp, 0);
./drivers/acpi/thermal.mod.c:   { 0x436006da, "call_usermodehelper" },
./drivers/input/input.c:        value = call_usermodehelper(argv [0], argv, envp, 0);
./drivers/pnp/pnpbios/core.c:   value = call_usermodehelper (argv [0], argv, envp, 0);
./drivers/macintosh/therm_pm72.c:       return call_usermodehelper(critical_overtemp_path, argv, envp, 0);
./arch/i386/mach-voyager/voyager_thread.c:      if ((ret = call_usermodehelper(argv[0], argv, envp, 1)) != 0) {
./include/linux/kmod.h:extern int call_usermodehelper(char *path, char *argv[], char *envp[], int wait);
./include/linux/kmod.h:extern void usermodehelper_init(void);
./kernel/power/main.c:  return call_usermodehelper(argv[0], argv, envp, 1);
./kernel/power/suspend_userui.c:        retval = call_usermodehelper(userui_program, argv, envp, 0);
./kernel/kmod.c:        call_usermodehelper wait flag, and remove exec_usermodehelper.
./kernel/kmod.c:        ret = call_usermodehelper(modprobe_path, argv, envp, 1);
./kernel/kmod.c:static int ____call_usermodehelper(void *data)
./kernel/kmod.c:        pid = kernel_thread(____call_usermodehelper, sub_info, SIGCHLD);
./kernel/kmod.c:static void __call_usermodehelper(void *data)
./kernel/kmod.c:                pid = kernel_thread(____call_usermodehelper, sub_info,
./kernel/kmod.c: * call_usermodehelper - start a usermode application
./kernel/kmod.c:int call_usermodehelper(char *path, char **argv, char **envp, int wait)
./kernel/kmod.c:        DECLARE_WORK(work, __call_usermodehelper, &sub_info);
./kernel/kmod.c:EXPORT_SYMBOL(call_usermodehelper);
./kernel/kmod.c:void __init usermodehelper_init(void)
./kernel/cpuset.c: * Note final arg to call_usermodehelper() is 0 - that means
./kernel/cpuset.c:      return call_usermodehelper(argv[0], argv, envp, 0);
./security/keys/request_key.c:  return call_usermodehelper(argv[0], argv, envp, 1);
./lib/kobject_uevent.c: retval = call_usermodehelper (argv[0], argv, envp, 0);
./lib/kobject_uevent.c:         pr_debug ("%s - call_usermodehelper returned %d\n",
./init/main.c:  usermodehelper_init();

Of course there will be indirect invocations (via kobjects, for
example), but I still think the number is not that great. I'm already
using the method I suggested in unreleased Suspend2 code, and the only
invocation I'm catch is at resume time, for the keseriod.

Regards,

Nigel
-- 
Nigel Cunningham
Software Engineer, Canberra, Australia
http://www.cyclades.com
Bus: +61 (2) 6291 9554; Hme: +61 (2) 6292 8028;  Mob: +61 (417) 100 574

Maintainer of Suspend2 Kernel Patches http://suspend2.net


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Re: swsusp 'disk' fails in bk-current - intel_agp at fault?

[Greg KH]

[-- Attachment #1: Type: text/plain, Size: 1114 bytes --]

On Tue, Mar 29, 2005 at 01:23:35PM -0800, Patrick Mochel wrote:
> 
> On Tue, 29 Mar 2005, Pavel Machek wrote:
> 
> > I don't really want us to try execve during resume... Could we simply
> > artifically fail that execve with something if (in_suspend()) return
> > -EINVAL; [except that in_suspend() just is not there, but there were
> > some proposals to add it].
> >
> > Or just avoid calling hotplug at all in resume case? And then do
> > coldplug-like scan when userspace is ready...
> 
> I thought that cold-plugging only worked for devices, not all objects.

We can walk the whole sysfs tree and create "cold" hotplug events.
udevstart does that for devices that udev cares about (as an example.)

> Can we just queue up hotplug events? That way we wouldn't lose any across
> the transition, and could be used to send resume events to userspace for
> various devices that need help..

Ick, I really hate this idea, but there is a patch in the SuSE kernel to
do this at boot time.  Hopefully the author of that patch resubmitts it
again and maybe it will make it eventually into mainline...

thanks,

greg k-h

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Re: swsusp 'disk' fails in bk-current - intel_agp at fault?

[Dmitry Torokhov]

On Tuesday 29 March 2005 17:35, Pavel Machek wrote:
> Hi!
> 
> > > We currently freeze processes for suspend-to-ram, too. I guess that
> > > disable_usermodehelper is probably better and that in_suspend() should
> > > only be used for sanity checks... go with disable_usermodehelper and
> > > sorry for the noise.
> > 
> > Here's another possibility: Freeze the workqueue that
> > call_usermodehelper uses (remember that code I didn't push hard enough
> > to Andrew?), and let invocations of call_usermodehelper block in
> > TASK_UNINTERRUPTIBLE. In refrigerating processes, don't choke on
> 
> There may be many devices in the system, and you are going to need
> quite a lot of RAM for all that... That's why they do not queue it
> during boot, IIRC. Disabling usermode helper seems right.

Ok, what do you think about this one?

===================================================================

swsusp: disable usermodehelper after generating memory snapshot and
        before resuming devices, so when device fails to resume we
        won't try to call hotplug - userspace stopped anyway.

Signed-off-by: Dmitry Torokhov <dtor-JGs/UdohzUI@public.gmane.org>


 include/linux/kmod.h  |    3 +++
 kernel/kmod.c         |   14 +++++++++++++-
 kernel/power/disk.c   |    2 ++
 kernel/power/swsusp.c |    1 -
 4 files changed, 18 insertions(+), 2 deletions(-)

Index: dtor/kernel/power/disk.c
===================================================================
--- dtor.orig/kernel/power/disk.c
+++ dtor/kernel/power/disk.c
@@ -205,6 +205,8 @@ int pm_suspend_disk(void)
 
 	if (in_suspend) {
 		pr_debug("PM: writing image.\n");
+		usermodehelper_disable();
+		device_resume();
 		error = swsusp_write();
 		if (!error)
 			power_down(pm_disk_mode);
Index: dtor/kernel/power/swsusp.c
===================================================================
--- dtor.orig/kernel/power/swsusp.c
+++ dtor/kernel/power/swsusp.c
@@ -853,7 +853,6 @@ static int suspend_prepare_image(void)
 int swsusp_write(void)
 {
 	int error;
-	device_resume();
 	lock_swapdevices();
 	error = write_suspend_image();
 	/* This will unlock ignored swap devices since writing is finished */
Index: dtor/kernel/kmod.c
===================================================================
--- dtor.orig/kernel/kmod.c
+++ dtor/kernel/kmod.c
@@ -124,6 +124,8 @@ struct subprocess_info {
 	int retval;
 };
 
+static int usermodehelper_disabled;
+
 /*
  * This is the task which runs the usermode application
  */
@@ -240,7 +242,7 @@ int call_usermodehelper(char *path, char
 	if (!khelper_wq)
 		return -EBUSY;
 
-	if (path[0] == '\0')
+	if (usermodehelper_disabled || path[0] == '\0')
 		return 0;
 
 	queue_work(khelper_wq, &work);
@@ -249,6 +251,16 @@ int call_usermodehelper(char *path, char
 }
 EXPORT_SYMBOL(call_usermodehelper);
 
+void usermodehelper_enable(void)
+{
+	usermodehelper_disabled = 0;
+}
+
+void usermodehelper_disable(void)
+{
+	usermodehelper_disabled = 1;
+}
+
 void __init usermodehelper_init(void)
 {
 	khelper_wq = create_singlethread_workqueue("khelper");
Index: dtor/include/linux/kmod.h
===================================================================
--- dtor.orig/include/linux/kmod.h
+++ dtor/include/linux/kmod.h
@@ -34,7 +34,10 @@ static inline int request_module(const c
 #endif
 
 #define try_then_request_module(x, mod...) ((x) ?: (request_module(mod), (x)))
+
 extern int call_usermodehelper(char *path, char *argv[], char *envp[], int wait);
 extern void usermodehelper_init(void);
+extern void usermodehelper_enable(void);
+extern void usermodehelper_disable(void);
 
 #endif /* __LINUX_KMOD_H__ */

Re: Re: swsusp 'disk' fails in bk-current - intel_agp at fault?

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2468 bytes --]

Hi!

> > > > We currently freeze processes for suspend-to-ram, too. I guess that
> > > > disable_usermodehelper is probably better and that in_suspend() should
> > > > only be used for sanity checks... go with disable_usermodehelper and
> > > > sorry for the noise.
> > > 
> > > Here's another possibility: Freeze the workqueue that
> > > call_usermodehelper uses (remember that code I didn't push hard enough
> > > to Andrew?), and let invocations of call_usermodehelper block in
> > > TASK_UNINTERRUPTIBLE. In refrigerating processes, don't choke on
> > 
> > There may be many devices in the system, and you are going to need
> > quite a lot of RAM for all that... That's why they do not queue it
> > during boot, IIRC. Disabling usermode helper seems right.
> 
> Ok, what do you think about this one?
> 
> ===================================================================
> 
> swsusp: disable usermodehelper after generating memory snapshot and
>         before resuming devices, so when device fails to resume we
>         won't try to call hotplug - userspace stopped anyway.
> 
> Signed-off-by: Dmitry Torokhov <dtor-JGs/UdohzUI@public.gmane.org>
> 
> 
>  include/linux/kmod.h  |    3 +++
>  kernel/kmod.c         |   14 +++++++++++++-
>  kernel/power/disk.c   |    2 ++
>  kernel/power/swsusp.c |    1 -
>  4 files changed, 18 insertions(+), 2 deletions(-)
> 
> Index: dtor/kernel/power/disk.c
> ===================================================================
> --- dtor.orig/kernel/power/disk.c
> +++ dtor/kernel/power/disk.c
> @@ -205,6 +205,8 @@ int pm_suspend_disk(void)
>  
>  	if (in_suspend) {
>  		pr_debug("PM: writing image.\n");
> +		usermodehelper_disable();
> +		device_resume();
>  		error = swsusp_write();
>  		if (!error)
>  			power_down(pm_disk_mode);
> Index: dtor/kernel/power/swsusp.c
> ===================================================================
> --- dtor.orig/kernel/power/swsusp.c
> +++ dtor/kernel/power/swsusp.c
> @@ -853,7 +853,6 @@ static int suspend_prepare_image(void)
>  int swsusp_write(void)
>  {
>  	int error;
> -	device_resume();
>  	lock_swapdevices();
>  	error = write_suspend_image();
>  	/* This will unlock ignored swap devices since writing is

Looks good, except... why move code around? Could you just call
usermodehelper_disable from swsusp_write?
							Pavel

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Re: swsusp 'disk' fails in bk-current - intel_agp at fault?

[Dmitry Torokhov]

On Thu, 31 Mar 2005 10:39:10 +0200, Pavel Machek <pavel-AlSwsSmVLrQ@public.gmane.org> wrote:
> >  int swsusp_write(void)
> >  {
> >       int error;
> > -     device_resume();
> >       lock_swapdevices();
> >       error = write_suspend_image();
> >       /* This will unlock ignored swap devices since writing is
> 
> Looks good, except... why move code around? Could you just call
> usermodehelper_disable from swsusp_write?

That's because I don't think that swsusp_write is a proper place for
it ;) It looks like a lean and mean function that does just write and
manipulating usermodehelper state _and_ system (device) state is
wrong. Let it do one thing, don't overload with actions that I think
belong to the upper level. Do you agree?

I think I need to stick in usermodehelper_enable call in case
swsusp_write fails though.

-- 
Dmitry

Re: Re: swsusp 'disk' fails in bk-current - intel_agp at fault?

[Patrick Mochel]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 531 bytes --]


On Thu, 31 Mar 2005, Dmitry Torokhov wrote:

> Ok, what do you think about this one?
>
> ===================================================================
>
> swsusp: disable usermodehelper after generating memory snapshot and
>         before resuming devices, so when device fails to resume we
>         won't try to call hotplug - userspace stopped anyway.

Hm, shouldn't we disable it before we start to freeze processes? We don't
want any more processes trying to start up after we've taken care of
them..

Thanks,


	Pat


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Re: swsusp 'disk' fails in bk-current - intel_agp at fault?

[Dmitry Torokhov]

On Thu, 31 Mar 2005 08:02:44 -0800 (PST), Patrick Mochel
<mochel-og82NyAXoxR0I81aPRRTmR2eb7JE58TQ@public.gmane.org> wrote:
> 
> On Thu, 31 Mar 2005, Dmitry Torokhov wrote:
> 
> > Ok, what do you think about this one?
> >
> > ===================================================================
> >
> > swsusp: disable usermodehelper after generating memory snapshot and
> >         before resuming devices, so when device fails to resume we
> >         won't try to call hotplug - userspace stopped anyway.
> 
> Hm, shouldn't we disable it before we start to freeze processes? We don't
> want any more processes trying to start up after we've taken care of
> them..
> 

Can't a device be removed (for any reason) _while_ we are freezing
processes? I think freeszing code will properly deal with it... What
about suspend semantics - if suspend fails do we say the device should
be operational or the system should attempt to re-initialize? I.e. we
are not doing suspend after all - can we still drop messages on the
floor? After all, we still have ability to run coldplug after failed
suspend.

I frankly am not sure at what point to disable usermode helper. Or
maybe we need to have a list of pending events and suspend khelper_wq
while suspending.

-- 
Dmitry

Re: Re: swsusp 'disk' fails in bk-current - intel_agp at fault?

[Nigel Cunningham]

[-- Attachment #1: Type: text/plain, Size: 1838 bytes --]

Hi.

On Fri, 2005-04-01 at 02:32, Dmitry Torokhov wrote:
> On Thu, 31 Mar 2005 08:02:44 -0800 (PST), Patrick Mochel
> <mochel-og82NyAXoxR0I81aPRRTmR2eb7JE58TQ@public.gmane.org> wrote:
> > 
> > On Thu, 31 Mar 2005, Dmitry Torokhov wrote:
> > 
> > > Ok, what do you think about this one?
> > >
> > > ===================================================================
> > >
> > > swsusp: disable usermodehelper after generating memory snapshot and
> > >         before resuming devices, so when device fails to resume we
> > >         won't try to call hotplug - userspace stopped anyway.
> > 
> > Hm, shouldn't we disable it before we start to freeze processes? We don't
> > want any more processes trying to start up after we've taken care of
> > them..
> > 
> 
> Can't a device be removed (for any reason) _while_ we are freezing
> processes? I think freeszing code will properly deal with it... What
> about suspend semantics - if suspend fails do we say the device should
> be operational or the system should attempt to re-initialize? I.e. we
> are not doing suspend after all - can we still drop messages on the
> floor? After all, we still have ability to run coldplug after failed
> suspend.
> 
> I frankly am not sure at what point to disable usermode helper. Or
> maybe we need to have a list of pending events and suspend khelper_wq
> while suspending.

FWIW, my solution is purely freezer based. I freeze khelper and in the
freezer code ignore kernel threads in state uninterruptible (which is
where kseriod, eg, will be while it waits for the usermode helper
process (which also gets frozen).

Regards,

Nigel
-- 
Nigel Cunningham
Software Engineer, Canberra, Australia
http://www.cyclades.com
Bus: +61 (2) 6291 9554; Hme: +61 (2) 6292 8028;  Mob: +61 (417) 100 574

Maintainer of Suspend2 Kernel Patches http://suspend2.net


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Re: swsusp 'disk' fails in bk-current - intel_agp at fault?

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1274 bytes --]

Hi!

> > > Ok, what do you think about this one?
> > >
> > > ===================================================================
> > >
> > > swsusp: disable usermodehelper after generating memory snapshot and
> > >         before resuming devices, so when device fails to resume we
> > >         won't try to call hotplug - userspace stopped anyway.
> > 
> > Hm, shouldn't we disable it before we start to freeze processes? We don't
> > want any more processes trying to start up after we've taken care of
> > them..
> > 
> 
> Can't a device be removed (for any reason) _while_ we are freezing
> processes? I think freeszing code will properly deal with it... What
> about suspend semantics - if suspend fails do we say the device should
> be operational or the system should attempt to re-initialize? I.e. we
> are not doing suspend after all - can we still drop messages on the
> floor? After all, we still have ability to run coldplug after failed
> suspend.

I believe we should freeze hotplug before processes. Dropping messages
on the floor should not be a problem, we should just call coldplug
after failed suspend.
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Re: swsusp 'disk' fails in bk-current - intel_agp at fault?

[Nigel Cunningham]

[-- Attachment #1: Type: text/plain, Size: 1575 bytes --]

Hi.

On Fri, 2005-04-01 at 08:18, Pavel Machek wrote:
> Hi!
> 
> > > > Ok, what do you think about this one?
> > > >
> > > > ===================================================================
> > > >
> > > > swsusp: disable usermodehelper after generating memory snapshot and
> > > >         before resuming devices, so when device fails to resume we
> > > >         won't try to call hotplug - userspace stopped anyway.
> > > 
> > > Hm, shouldn't we disable it before we start to freeze processes? We don't
> > > want any more processes trying to start up after we've taken care of
> > > them..
> > > 
> > 
> > Can't a device be removed (for any reason) _while_ we are freezing
> > processes? I think freeszing code will properly deal with it... What
> > about suspend semantics - if suspend fails do we say the device should
> > be operational or the system should attempt to re-initialize? I.e. we
> > are not doing suspend after all - can we still drop messages on the
> > floor? After all, we still have ability to run coldplug after failed
> > suspend.
> 
> I believe we should freeze hotplug before processes. Dropping messages
> on the floor should not be a problem, we should just call coldplug
> after failed suspend.

How will you know which devices to call coldplug for, post resume? (Or
does it figure that out itself somehow?)

Regards,

Nigel
-- 
Nigel Cunningham
Software Engineer, Canberra, Australia
http://www.cyclades.com
Bus: +61 (2) 6291 9554; Hme: +61 (2) 6292 8028;  Mob: +61 (417) 100 574

Maintainer of Suspend2 Kernel Patches http://suspend2.net


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Re: swsusp 'disk' fails in bk-current - intel_agp at fault?

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 2240 bytes --]

Hi,

On Friday, 1 of April 2005 00:28, Nigel Cunningham wrote:
> Hi.
> 
> On Fri, 2005-04-01 at 08:18, Pavel Machek wrote:
> > Hi!
> > 
> > > > > Ok, what do you think about this one?
> > > > >
> > > > > ===================================================================
> > > > >
> > > > > swsusp: disable usermodehelper after generating memory snapshot and
> > > > >         before resuming devices, so when device fails to resume we
> > > > >         won't try to call hotplug - userspace stopped anyway.
> > > > 
> > > > Hm, shouldn't we disable it before we start to freeze processes? We don't
> > > > want any more processes trying to start up after we've taken care of
> > > > them..
> > > > 
> > > 
> > > Can't a device be removed (for any reason) _while_ we are freezing
> > > processes? I think freeszing code will properly deal with it... What
> > > about suspend semantics - if suspend fails do we say the device should
> > > be operational or the system should attempt to re-initialize? I.e. we
> > > are not doing suspend after all - can we still drop messages on the
> > > floor? After all, we still have ability to run coldplug after failed
> > > suspend.
> > 
> > I believe we should freeze hotplug before processes.

I agree.  IMO user space should not be considered as available once we have
started freezing processes, so hotplug should be disabled before.  By the same
token, it should only be enabled after the processes have been restarted
during resume (or after suspend has failed).

BTW, it seems to me that the forking of new processes could be disabled
before we start to freeze the existing ones.

> > Dropping messages on the floor should not be a problem, we should just
> > call coldplug after failed suspend.
> 
> How will you know which devices to call coldplug for, post resume? (Or
> does it figure that out itself somehow?)

I think the drivers that need the hotplug to resume should defer their resume
routines until usermodehelper is enabled (it seems to me that we can use
a completion to handle this).

Greets,
Rafael


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Re: swsusp 'disk' fails in bk-current - intel_agp at fault?

[Stefan Seyfried]

[-- Attachment #1: Type: text/plain, Size: 627 bytes --]

Rafael J. Wysocki wrote:
> Hi,
>> On Fri, 2005-04-01 at 08:18, Pavel Machek wrote:
>> > I believe we should freeze hotplug before processes.
> 
> I agree.  IMO user space should not be considered as available once we have
> started freezing processes, so hotplug should be disabled before.  By the same
> token, it should only be enabled after the processes have been restarted
> during resume (or after suspend has failed).

it has probably to be enabled before the processes are restarted - they
may rightfully assume that hotplug is working.
-- 
seife
                                 Never trust a computer you can't lift.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]