-mm: PM=y, VT=n doesn't compile

-mm: PM=y, VT=n doesn't compile

[Adrian Bunk]

swsusp-pm-refuse-to-suspend-devices-if-wrong-console-is-active.patch
causes the following compile error with CONFIG_PM=y, CONFIG_VT=n:

<--  snip  -->

...
  LD      .tmp_vmlinux1
drivers/built-in.o: In function `device_suspend': undefined reference to `fg_console'
drivers/built-in.o: In function `device_suspend': undefined reference to `vc_cons'
make: *** [.tmp_vmlinux1] Error 1

<--  snip  -->

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: -mm: PM=y, VT=n doesn't compile

[Andrew Morton]

Adrian Bunk <bunk@stusta.de> wrote:
>
>  swsusp-pm-refuse-to-suspend-devices-if-wrong-console-is-active.patch
>  causes the following compile error with CONFIG_PM=y, CONFIG_VT=n:
> 
>  <--  snip  -->
> 
>  ...
>    LD      .tmp_vmlinux1
>  drivers/built-in.o: In function `device_suspend': undefined reference to `fg_console'
>  drivers/built-in.o: In function `device_suspend': undefined reference to `vc_cons'
>  make: *** [.tmp_vmlinux1] Error 1

Right, thanks.  I'll drop that patch.

Guys, please don't go poking around in the vt subsystems's internal data
structures from within power management code.

Write a nice little helper function, put that in vt.c, give it a static
inline `return 0' stub in the header file, then call that.

That's Kernel Programming 101, no?

Re: -mm: PM=y, VT=n doesn't compile

[Rafael J. Wysocki]

On Saturday 18 March 2006 02:50, Andrew Morton wrote:
> Adrian Bunk <bunk@stusta.de> wrote:
> >
> >  swsusp-pm-refuse-to-suspend-devices-if-wrong-console-is-active.patch
> >  causes the following compile error with CONFIG_PM=y, CONFIG_VT=n:
> > 
> >  <--  snip  -->
> > 
> >  ...
> >    LD      .tmp_vmlinux1
> >  drivers/built-in.o: In function `device_suspend': undefined reference to `fg_console'
> >  drivers/built-in.o: In function `device_suspend': undefined reference to `vc_cons'
> >  make: *** [.tmp_vmlinux1] Error 1
> 
> Right, thanks.  I'll drop that patch.
> 
> Guys, please don't go poking around in the vt subsystems's internal data
> structures from within power management code.
> 
> Write a nice little helper function, put that in vt.c, give it a static
> inline `return 0' stub in the header file, then call that.

OK, I will, but the changes in kernel/power/user.c in that patch are necessary
for the userland suspend we're working on right now.

Could you please accept the following patch instead of that one?

---
From: "Rafael J. Wysocki" <rjw@sisk.pl>

Remove the console-switching code from the suspend part of the swsusp
userland interface and let the userland tools switch the console.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/user.c |    3 ---
 1 files changed, 3 deletions(-)

Index: linux-2.6.16-rc6-mm2/kernel/power/user.c
===================================================================
--- linux-2.6.16-rc6-mm2.orig/kernel/power/user.c
+++ linux-2.6.16-rc6-mm2/kernel/power/user.c
@@ -138,12 +138,10 @@ static int snapshot_ioctl(struct inode *
 		if (data->frozen)
 			break;
 		down(&pm_sem);
-		pm_prepare_console();
 		disable_nonboot_cpus();
 		if (freeze_processes()) {
 			thaw_processes();
 			enable_nonboot_cpus();
-			pm_restore_console();
 			error = -EBUSY;
 		}
 		up(&pm_sem);
@@ -157,7 +155,6 @@ static int snapshot_ioctl(struct inode *
 		down(&pm_sem);
 		thaw_processes();
 		enable_nonboot_cpus();
-		pm_restore_console();
 		up(&pm_sem);
 		data->frozen = 0;
 		break;

Re: -mm: PM=y, VT=n doesn't compile

[Rafael J. Wysocki]

Hi,

On Friday 17 March 2006 18:18, Adrian Bunk wrote:
> swsusp-pm-refuse-to-suspend-devices-if-wrong-console-is-active.patch
> causes the following compile error with CONFIG_PM=y, CONFIG_VT=n:
> 
> <--  snip  -->
> 
> ...
>   LD      .tmp_vmlinux1
> drivers/built-in.o: In function `device_suspend': undefined reference to `fg_console'
> drivers/built-in.o: In function `device_suspend': undefined reference to `vc_cons'
> make: *** [.tmp_vmlinux1] Error 1
> 
> <--  snip  -->

Sorry, I overlooked your message yesterday.  [Actually Andrew turned my
attention to it by dropping the patch from -mm. ;-)]

Fixed version of the patch is appended.

Greetings,
Rafael

---
From: "Rafael J. Wysocki" <rjw@sisk.pl>

1) Remove the console-switching code from the suspend part of the swsusp
   userland interface and let the userland tools switch the console.

2) It is unsafe to suspend devices if the hardware is controlled by X. 
   Add an extra check to prevent this from happening.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/suspend.c |   17 +++++++++++++++++
 kernel/power/user.c          |    3 ---
 2 files changed, 17 insertions(+), 3 deletions(-)

Index: linux-2.6.16-rc6-mm2/drivers/base/power/suspend.c
===================================================================
--- linux-2.6.16-rc6-mm2.orig/drivers/base/power/suspend.c
+++ linux-2.6.16-rc6-mm2/drivers/base/power/suspend.c
@@ -8,6 +8,9 @@
  *
  */
 
+#include <linux/vt_kern.h>
+#include <linux/kbd_kern.h>
+#include <linux/console.h>
 #include <linux/device.h>
 #include <linux/kallsyms.h>
 #include <linux/pm.h>
@@ -65,6 +68,17 @@ int suspend_device(struct device * dev, 
 	return error;
 }
 
+#ifdef CONFIG_VT
+static inline int is_suspend_console_safe(void)
+{
+	/* It is unsafe to suspend devices while X has control of the
+	 * hardware. Make sure we are running on a kernel-controlled console.
+	 */
+	return vc_cons[fg_console].d->vc_mode == KD_TEXT;
+}
+#else
+#define is_suspend_console_safe()	1
+#endif
 
 /**
  *	device_suspend - Save state and stop all devices in system.
@@ -85,6 +99,9 @@ int device_suspend(pm_message_t state)
 {
 	int error = 0;
 
+	if (!is_suspend_console_safe())
+		return -EINVAL;
+
 	down(&dpm_sem);
 	down(&dpm_list_sem);
 	while (!list_empty(&dpm_active) && error == 0) {
Index: linux-2.6.16-rc6-mm2/kernel/power/user.c
===================================================================
--- linux-2.6.16-rc6-mm2.orig/kernel/power/user.c
+++ linux-2.6.16-rc6-mm2/kernel/power/user.c
@@ -138,12 +138,10 @@ static int snapshot_ioctl(struct inode *
 		if (data->frozen)
 			break;
 		down(&pm_sem);
-		pm_prepare_console();
 		disable_nonboot_cpus();
 		if (freeze_processes()) {
 			thaw_processes();
 			enable_nonboot_cpus();
-			pm_restore_console();
 			error = -EBUSY;
 		}
 		up(&pm_sem);
@@ -157,7 +155,6 @@ static int snapshot_ioctl(struct inode *
 		down(&pm_sem);
 		thaw_processes();
 		enable_nonboot_cpus();
-		pm_restore_console();
 		up(&pm_sem);
 		data->frozen = 0;
 		break;

Re: -mm: PM=y, VT=n doesn't compile

[Andrew Morton]

"Rafael J. Wysocki" <rjw@sisk.pl> wrote:
>
> --- linux-2.6.16-rc6-mm2.orig/drivers/base/power/suspend.c
>  +++ linux-2.6.16-rc6-mm2/drivers/base/power/suspend.c
>  @@ -8,6 +8,9 @@
>    *
>    */
>   
>  +#include <linux/vt_kern.h>
>  +#include <linux/kbd_kern.h>
>  +#include <linux/console.h>
>   #include <linux/device.h>
>   #include <linux/kallsyms.h>
>   #include <linux/pm.h>
>  @@ -65,6 +68,17 @@ int suspend_device(struct device * dev, 
>   	return error;
>   }
>   
>  +#ifdef CONFIG_VT
>  +static inline int is_suspend_console_safe(void)
>  +{
>  +	/* It is unsafe to suspend devices while X has control of the
>  +	 * hardware. Make sure we are running on a kernel-controlled console.
>  +	 */
>  +	return vc_cons[fg_console].d->vc_mode == KD_TEXT;
>  +}

Please implement this inside the vt subsystem, not the pm subsystem.  That way

a) It gets to be called "console_is_in_text_mode()", or
   "vt_not_running_X()" or something, which is something someone else might
   want to know.  

b) People who work on vt code don't need to keep an eye on a hunk of pm
   code at the same time.

c) You won't need all those includes.

>  +#else
>  +#define is_suspend_console_safe()	1

And this can go in console.h

>  +#endif

Re: -mm: PM=y, VT=n doesn't compile

[Rafael J. Wysocki]

On Saturday 18 March 2006 20:48, Andrew Morton wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> >
> > --- linux-2.6.16-rc6-mm2.orig/drivers/base/power/suspend.c
> >  +++ linux-2.6.16-rc6-mm2/drivers/base/power/suspend.c
> >  @@ -8,6 +8,9 @@
> >    *
> >    */
> >   
> >  +#include <linux/vt_kern.h>
> >  +#include <linux/kbd_kern.h>
> >  +#include <linux/console.h>
> >   #include <linux/device.h>
> >   #include <linux/kallsyms.h>
> >   #include <linux/pm.h>
> >  @@ -65,6 +68,17 @@ int suspend_device(struct device * dev, 
> >   	return error;
> >   }
> >   
> >  +#ifdef CONFIG_VT
> >  +static inline int is_suspend_console_safe(void)
> >  +{
> >  +	/* It is unsafe to suspend devices while X has control of the
> >  +	 * hardware. Make sure we are running on a kernel-controlled console.
> >  +	 */
> >  +	return vc_cons[fg_console].d->vc_mode == KD_TEXT;
> >  +}
> 
> Please implement this inside the vt subsystem, not the pm subsystem.  That way
> 
> a) It gets to be called "console_is_in_text_mode()", or
>    "vt_not_running_X()" or something, which is something someone else might
>    want to know.  
> 
> b) People who work on vt code don't need to keep an eye on a hunk of pm
>    code at the same time.
> 
> c) You won't need all those includes.

I'm working on this now.  [For some obscure reason I received your reply to
Adrian only a couple of minutes ago.]

Still I'd like to separate it from the console-related changes in
kernel/power/user.c that are needed for the userland suspend, so I'd like
to split the dropped patch in two.

I've already sent the kernel/power/user.c changes in a separate patch,
and I'll send the drivers/base/power/suspend.c changes when I test them
(in a while).

Re: -mm: PM=y, VT=n doesn't compile

[Rafael J. Wysocki]

On Saturday 18 March 2006 20:58, Rafael J. Wysocki wrote:
> On Saturday 18 March 2006 20:48, Andrew Morton wrote:
> > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > >
> > > --- linux-2.6.16-rc6-mm2.orig/drivers/base/power/suspend.c
> > >  +++ linux-2.6.16-rc6-mm2/drivers/base/power/suspend.c
> > >  @@ -8,6 +8,9 @@
> > >    *
> > >    */
> > >   
> > >  +#include <linux/vt_kern.h>
> > >  +#include <linux/kbd_kern.h>
> > >  +#include <linux/console.h>
> > >   #include <linux/device.h>
> > >   #include <linux/kallsyms.h>
> > >   #include <linux/pm.h>
> > >  @@ -65,6 +68,17 @@ int suspend_device(struct device * dev, 
> > >   	return error;
> > >   }
> > >   
> > >  +#ifdef CONFIG_VT
> > >  +static inline int is_suspend_console_safe(void)
> > >  +{
> > >  +	/* It is unsafe to suspend devices while X has control of the
> > >  +	 * hardware. Make sure we are running on a kernel-controlled console.
> > >  +	 */
> > >  +	return vc_cons[fg_console].d->vc_mode == KD_TEXT;
> > >  +}
> > 
> > Please implement this inside the vt subsystem, not the pm subsystem.  That way
> > 
> > a) It gets to be called "console_is_in_text_mode()", or
> >    "vt_not_running_X()" or something, which is something someone else might
> >    want to know.  
> > 
> > b) People who work on vt code don't need to keep an eye on a hunk of pm
> >    code at the same time.
> > 
> > c) You won't need all those includes.
> 
> I'm working on this now.  [For some obscure reason I received your reply to
> Adrian only a couple of minutes ago.]
> 
> Still I'd like to separate it from the console-related changes in
> kernel/power/user.c that are needed for the userland suspend, so I'd like
> to split the dropped patch in two.
> 
> I've already sent the kernel/power/user.c changes in a separate patch,
> and I'll send the drivers/base/power/suspend.c changes when I test them
> (in a while).

OK, here they go.

---
From: "Rafael J. Wysocki" <rjw@sisk.pl>

It is unsafe to suspend devices if the hardware is controlled by X. 
Add an extra check to prevent this from happening.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/suspend.c |    5 ++++-
 drivers/char/vt.c            |    8 ++++++++
 include/linux/vt_kern.h      |    5 +++++
 3 files changed, 17 insertions(+), 1 deletion(-)

Index: linux-2.6.16-rc6-mm2/drivers/base/power/suspend.c
===================================================================
--- linux-2.6.16-rc6-mm2.orig/drivers/base/power/suspend.c
+++ linux-2.6.16-rc6-mm2/drivers/base/power/suspend.c
@@ -8,6 +8,7 @@
  *
  */
 
+#include <linux/vt_kern.h>
 #include <linux/device.h>
 #include <linux/kallsyms.h>
 #include <linux/pm.h>
@@ -65,7 +66,6 @@ int suspend_device(struct device * dev, 
 	return error;
 }
 
-
 /**
  *	device_suspend - Save state and stop all devices in system.
  *	@state:		Power state to put each device in.
@@ -85,6 +85,9 @@ int device_suspend(pm_message_t state)
 {
 	int error = 0;
 
+	if (!is_console_suspend_safe())
+		return -EINVAL;
+
 	down(&dpm_sem);
 	down(&dpm_list_sem);
 	while (!list_empty(&dpm_active) && error == 0) {
Index: linux-2.6.16-rc6-mm2/drivers/char/vt.c
===================================================================
--- linux-2.6.16-rc6-mm2.orig/drivers/char/vt.c
+++ linux-2.6.16-rc6-mm2/drivers/char/vt.c
@@ -3234,6 +3234,14 @@ void vcs_scr_writew(struct vc_data *vc, 
 	}
 }
 
+int is_console_suspend_safe(void)
+{
+	/* It is unsafe to suspend devices while X has control of the
+	 * hardware. Make sure we are running on a kernel-controlled console.
+	 */
+	return vc_cons[fg_console].d->vc_mode == KD_TEXT;
+}
+
 /*
  *	Visible symbols for modules
  */
Index: linux-2.6.16-rc6-mm2/include/linux/vt_kern.h
===================================================================
--- linux-2.6.16-rc6-mm2.orig/include/linux/vt_kern.h
+++ linux-2.6.16-rc6-mm2/include/linux/vt_kern.h
@@ -73,6 +73,11 @@ int con_copy_unimap(struct vc_data *dst_
 int vt_waitactive(int vt);
 void change_console(struct vc_data *new_vc);
 void reset_vc(struct vc_data *vc);
+#ifdef CONFIG_VT
+int is_console_suspend_safe(void);
+#else
+static inline int is_console_suspend_safe(void) { return 1; }
+#endif
 
 /*
  * vc_screen.c shares this temporary buffer with the console write code so that

Re: -mm: PM=y, VT=n doesn't compile

[Pavel Machek]

Hi!

> OK, here they go.

ACK.

> Index: linux-2.6.16-rc6-mm2/include/linux/vt_kern.h
> ===================================================================
> --- linux-2.6.16-rc6-mm2.orig/include/linux/vt_kern.h
> +++ linux-2.6.16-rc6-mm2/include/linux/vt_kern.h
> @@ -73,6 +73,11 @@ int con_copy_unimap(struct vc_data *dst_
>  int vt_waitactive(int vt);
>  void change_console(struct vc_data *new_vc);
>  void reset_vc(struct vc_data *vc);
> +#ifdef CONFIG_VT
> +int is_console_suspend_safe(void);

I guess I'd just do static inline ... and put code here, but both
versions are okay.

> +#else
> +static inline int is_console_suspend_safe(void) { return 1; }
> +#endif
>  
>  /*
>   * vc_screen.c shares this temporary buffer with the console write code so that

-- 
42:             i < SampleTable.Length;