All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Software Suspend: Fix suspend when console is in VT_AUTO/KD_GRAPHICS mode
@ 2007-03-08 22:54 Andrew Johnson
  2007-03-08 23:37 ` Daniel Drake
  2007-03-09  9:08 ` Pavel Machek
  0 siblings, 2 replies; 14+ messages in thread
From: Andrew Johnson @ 2007-03-08 22:54 UTC (permalink / raw)
  To: pavel; +Cc: linux-kernel

Hi Pavel,

When the console is in VT_AUTO/KD_GRAPHICS mode, switching to the
SUSPEND_CONSOLE fails, resulting in vt_waitactive() waiting indefinately
or until the task is interrupted.  The following patch tests if a
console switch can occur in set_console() and returns early if a console
switch is not possible.

Signed-off-by: Andrew Johnson <ajohnson@intrinsyc.com>

diff -rup linux-2.6.20.1/drivers/char/vt.c linux/drivers/char/vt.c
--- linux-2.6.20.1/drivers/char/vt.c	2007-02-19 22:34:32.000000000 -0800
+++ linux/drivers/char/vt.c	2007-03-08 14:15:41.000000000 -0800
@@ -2188,10 +2188,20 @@ static void console_callback(struct work
 	release_console_sem();
 }
 
-void set_console(int nr)
+extern char vt_dont_switch;
+
+int set_console(int nr)
 {
+	struct vc_data *vc = vc_cons[fg_console].d;
+
+	if(!vc_cons_allocated(nr) || vt_dont_switch || vc->vc_mode ==
KD_GRAPHICS) {
+		return -EINVAL;
+	}
+
 	want_console = nr;
 	schedule_console_callback();
+
+	return 0;
 }
 
 struct tty_driver *console_driver;
diff -rup linux-2.6.20.1/drivers/char/vt_ioctl.c
linux/drivers/char/vt_ioctl.c
--- linux-2.6.20.1/drivers/char/vt_ioctl.c	2007-02-19 22:34:32.000000000
-0800
+++ linux/drivers/char/vt_ioctl.c	2007-03-08 14:15:41.000000000 -0800
@@ -34,7 +34,7 @@
 #include <linux/kbd_diacr.h>
 #include <linux/selection.h>
 
-static char vt_dont_switch;
+char vt_dont_switch;
 extern struct tty_driver *console_driver;
 
 #define VT_IS_IN_USE(i)	(console_driver->ttys[i] &&
console_driver->ttys[i]->count)
diff -rup linux-2.6.20.1/include/linux/kbd_kern.h
linux/include/linux/kbd_kern.h
--- linux-2.6.20.1/include/linux/kbd_kern.h	2007-02-19
22:34:32.000000000 -0800
+++ linux/include/linux/kbd_kern.h	2007-03-08 14:15:41.000000000 -0800
@@ -75,7 +75,7 @@ extern int do_poke_blanked_console;
 
 extern void (*kbd_ledfunc)(unsigned int led);
 
-extern void set_console(int nr);
+extern int set_console(int nr);
 extern void schedule_console_callback(void);
 
 static inline void set_leds(void)
diff -rup linux-2.6.20.1/kernel/power/console.c
linux/kernel/power/console.c
--- linux-2.6.20.1/kernel/power/console.c	2007-02-19 22:34:32.000000000
-0800
+++ linux/kernel/power/console.c	2007-03-08 14:15:41.000000000 -0800
@@ -27,7 +27,11 @@ int pm_prepare_console(void)
 		return 1;
 	}
 
-	set_console(SUSPEND_CONSOLE);
+	if (set_console(SUSPEND_CONSOLE)) {
+		/* Unable to change to the new console */
+		release_console_sem();
+		return 1;
+	}
 	release_console_sem();
 
 	if (vt_waitactive(SUSPEND_CONSOLE)) {



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Software Suspend: Fix suspend when console is in VT_AUTO/KD_GRAPHICS mode
  2007-03-08 22:54 [PATCH] Software Suspend: Fix suspend when console is in VT_AUTO/KD_GRAPHICS mode Andrew Johnson
@ 2007-03-08 23:37 ` Daniel Drake
  2007-03-09  5:01   ` Andrew Johnson
  2007-03-09  9:08 ` Pavel Machek
  1 sibling, 1 reply; 14+ messages in thread
From: Daniel Drake @ 2007-03-08 23:37 UTC (permalink / raw)
  To: Andrew Johnson; +Cc: pavel, linux-kernel

Andrew Johnson wrote:
> When the console is in VT_AUTO/KD_GRAPHICS mode, switching to the
> SUSPEND_CONSOLE fails, resulting in vt_waitactive() waiting indefinately
> or until the task is interrupted.  The following patch tests if a
> console switch can occur in set_console() and returns early if a console
> switch is not possible.
> 
> Signed-off-by: Andrew Johnson <ajohnson@intrinsyc.com>
> 
> diff -rup linux-2.6.20.1/drivers/char/vt.c linux/drivers/char/vt.c
> --- linux-2.6.20.1/drivers/char/vt.c	2007-02-19 22:34:32.000000000 -0800
> +++ linux/drivers/char/vt.c	2007-03-08 14:15:41.000000000 -0800
> @@ -2188,10 +2188,20 @@ static void console_callback(struct work
>  	release_console_sem();
>  }
>  
> -void set_console(int nr)
> +extern char vt_dont_switch;
> +
> +int set_console(int nr)
>  {
> +	struct vc_data *vc = vc_cons[fg_console].d;
> +
> +	if(!vc_cons_allocated(nr) || vt_dont_switch || vc->vc_mode ==
> KD_GRAPHICS) {
> +		return -EINVAL;
> +	}
> +
>  	want_console = nr;
>  	schedule_console_callback();
> +
> +	return 0;
>  }

I haven't tested, but I think the above -EINVAL return will break chvt. 
chvt uses the VT_ACTIVATE ioctl which calls set_console(), and it is 
valid for chvt to be used to change away from a graphics-mode console -- 
that shouldn't be an error condition.

Daniel


^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH] Software Suspend: Fix suspend when console is in VT_AUTO/KD_GRAPHICS mode
  2007-03-08 23:37 ` Daniel Drake
@ 2007-03-09  5:01   ` Andrew Johnson
  0 siblings, 0 replies; 14+ messages in thread
From: Andrew Johnson @ 2007-03-09  5:01 UTC (permalink / raw)
  To: Daniel Drake; +Cc: pavel, linux-kernel

Daniel Drake wrote:
> Andrew Johnson wrote:
> > When the console is in VT_AUTO/KD_GRAPHICS mode, switching to the
> > SUSPEND_CONSOLE fails, resulting in vt_waitactive() waiting
indefinately
> > or until the task is interrupted.  The following patch tests if a
> > console switch can occur in set_console() and returns early if a
console
> > switch is not possible.
> >
> > Signed-off-by: Andrew Johnson <ajohnson@intrinsyc.com>
> >
> > diff -rup linux-2.6.20.1/drivers/char/vt.c linux/drivers/char/vt.c
> > --- linux-2.6.20.1/drivers/char/vt.c	2007-02-19
22:34:32.000000000 -
> 0800
> > +++ linux/drivers/char/vt.c	2007-03-08 14:15:41.000000000 -0800
> > @@ -2188,10 +2188,20 @@ static void console_callback(struct work
> >  	release_console_sem();
> >  }
> >
> > -void set_console(int nr)
> > +extern char vt_dont_switch;
> > +
> > +int set_console(int nr)
> >  {
> > +	struct vc_data *vc = vc_cons[fg_console].d;
> > +
> > +	if(!vc_cons_allocated(nr) || vt_dont_switch || vc->vc_mode ==
> > KD_GRAPHICS) {
> > +		return -EINVAL;
> > +	}
> > +
> >  	want_console = nr;
> >  	schedule_console_callback();
> > +
> > +	return 0;
> >  }
> 
> I haven't tested, but I think the above -EINVAL return will break
chvt.
> chvt uses the VT_ACTIVATE ioctl which calls set_console(), and it is
> valid for chvt to be used to change away from a graphics-mode console
--
> that shouldn't be an error condition.

Currently the VT_ACTIVATE ioctl will eventually result in a call to
change_console() which will ignore the change if the console is in
KD_GRAPHICS+VT_AUTO mode.  Thus, chvt should fail.  However, the return
code from set_console() is ignored in vt_ioctl so no error code will
result (which is the same as current behavior).

-- Andrew

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Software Suspend: Fix suspend when console is in VT_AUTO/KD_GRAPHICS mode
  2007-03-08 22:54 [PATCH] Software Suspend: Fix suspend when console is in VT_AUTO/KD_GRAPHICS mode Andrew Johnson
  2007-03-08 23:37 ` Daniel Drake
@ 2007-03-09  9:08 ` Pavel Machek
  2007-03-09 15:34   ` Matthew Garrett
  1 sibling, 1 reply; 14+ messages in thread
From: Pavel Machek @ 2007-03-09  9:08 UTC (permalink / raw)
  To: Andrew Johnson; +Cc: linux-kernel

Hi!

> When the console is in VT_AUTO/KD_GRAPHICS mode, switching to the
> SUSPEND_CONSOLE fails, resulting in vt_waitactive() waiting indefinately
> or until the task is interrupted.  The following patch tests if a
> console switch can occur in set_console() and returns early if a console
> switch is not possible.

Your mailer wraps lines...

How do I reproduce this? Suspending from X works for me...?

> Signed-off-by: Andrew Johnson <ajohnson@intrinsyc.com>
> 
> diff -rup linux-2.6.20.1/drivers/char/vt.c linux/drivers/char/vt.c
> --- linux-2.6.20.1/drivers/char/vt.c	2007-02-19 22:34:32.000000000 -0800
> +++ linux/drivers/char/vt.c	2007-03-08 14:15:41.000000000 -0800
> @@ -2188,10 +2188,20 @@ static void console_callback(struct work
>  	release_console_sem();
>  }
>  
> -void set_console(int nr)
> +extern char vt_dont_switch;
> +
> +int set_console(int nr)
>  {
> +	struct vc_data *vc = vc_cons[fg_console].d;
> +
> +	if(!vc_cons_allocated(nr) || vt_dont_switch || vc->vc_mode ==
> KD_GRAPHICS) {
> +		return -EINVAL;
> +	}
> +

So... if current console is graphical, we leave X accessing the
console... That's bad, because video state is not going to be
restored...?
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Software Suspend: Fix suspend when console is in VT_AUTO/KD_GRAPHICS mode
  2007-03-09  9:08 ` Pavel Machek
@ 2007-03-09 15:34   ` Matthew Garrett
  2007-03-09 16:10     ` Andrew Johnson
                       ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Matthew Garrett @ 2007-03-09 15:34 UTC (permalink / raw)
  To: Pavel Machek; +Cc: Andrew Johnson, linux-kernel

On Fri, Mar 09, 2007 at 10:08:05AM +0100, Pavel Machek wrote:

> So... if current console is graphical, we leave X accessing the
> console... That's bad, because video state is not going to be
> restored...?

A graphical console is not necessarily X. Is there any requirement for 
there to be a single VT that isn't in text mode? The vt switching is 
a hack, we shouldn't make life difficult for people who have their own 
userspace code that's entirely capable of restoring video state on its 
own.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH] Software Suspend: Fix suspend when console is in VT_AUTO/KD_GRAPHICS mode
  2007-03-09 15:34   ` Matthew Garrett
@ 2007-03-09 16:10     ` Andrew Johnson
  2007-03-09 21:13       ` Pavel Machek
  2007-03-09 19:02     ` Andrew Johnson
  2007-03-09 21:11     ` Pavel Machek
  2 siblings, 1 reply; 14+ messages in thread
From: Andrew Johnson @ 2007-03-09 16:10 UTC (permalink / raw)
  To: Matthew Garrett, Pavel Machek; +Cc: linux-kernel

Matthew Garrett wrote: 
> On Fri, Mar 09, 2007 at 10:08:05AM +0100, Pavel Machek wrote:
> 
> > So... if current console is graphical, we leave X accessing the
> > console... That's bad, because video state is not going to be
> > restored...?
> 
> A graphical console is not necessarily X. Is there any requirement for
> there to be a single VT that isn't in text mode? The vt switching is
> a hack, we shouldn't make life difficult for people who have their own
> userspace code that's entirely capable of restoring video state on its
> own.

The problem actually comes about when using Qtopia Phone Edition (QPE)
on a PXA270. QPE puts the console into VT_AUTO+KD_GRAPHICS mode and
writes directly to the framebuffer from then on.  In this mode the
kernel correctly disallows a console change, as QPE is not getting
notification of a console change and thus does not know when to repaint
the screen. 

AFAIK, X uses VT_PROCESS+KD_GRAPHICS mode, so it gets notification of a
change to and from the X console, thus it knows when to repaint the
screen.

I think you can test this by changing the mode of a text console to
KD_GRAPHICS using the KDSETMODE ioctl, then attempting to change to
another text console using chvt.

-- Andrew

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Software Suspend: Fix suspend when console is in VT_AUTO/KD_GRAPHICS mode
  2007-03-09 15:34   ` Matthew Garrett
  2007-03-09 16:10     ` Andrew Johnson
@ 2007-03-09 19:02     ` Andrew Johnson
  2007-03-09 21:19       ` Pavel Machek
  2007-03-09 21:11     ` Pavel Machek
  2 siblings, 1 reply; 14+ messages in thread
From: Andrew Johnson @ 2007-03-09 19:02 UTC (permalink / raw)
  To: Matthew Garrett; +Cc: Pavel Machek, linux-kernel

On Fri, 2007-09-03 at 15:34 +0000, Matthew Garrett wrote:
> On Fri, Mar 09, 2007 at 10:08:05AM +0100, Pavel Machek wrote:
> 
> > So... if current console is graphical, we leave X accessing the
> > console... That's bad, because video state is not going to be
> > restored...?
> 
> A graphical console is not necessarily X. Is there any requirement for 
> there to be a single VT that isn't in text mode? The vt switching is 
> a hack, we shouldn't make life difficult for people who have their own 
> userspace code that's entirely capable of restoring video state on its 
> own.

I realised that the previous patch would disallow a console switch while
running X.  Attached is an updated patch with this scenario fixed.

Another approach might be to fail in vt_waitactive() if a console switch
is not going to occur.

-- Andrew

Signed-off-by: Andrew Johnson <ajohnson@intrinsyc.com>
---
diff -rup linux-2.6.20.1/drivers/char/vt.c linux/drivers/char/vt.c
--- linux-2.6.20.1/drivers/char/vt.c	2007-02-19 22:34:32.000000000 -0800
+++ linux/drivers/char/vt.c	2007-03-09 10:53:32.000000000 -0800
@@ -2188,10 +2188,22 @@ static void console_callback(struct work
 	release_console_sem();
 }
 
-void set_console(int nr)
+extern char vt_dont_switch;
+
+int set_console(int nr)
 {
+	struct vc_data *vc = vc_cons[fg_console].d;
+
+	if(!vc_cons_allocated(nr) || vt_dont_switch || 
+		(vc->vt_mode.mode != VT_PROCESS && vc->vc_mode == KD_GRAPHICS)) {
+
+		return -EINVAL;
+	}
+
 	want_console = nr;
 	schedule_console_callback();
+
+	return 0;
 }
 
 struct tty_driver *console_driver;
diff -rup linux-2.6.20.1/drivers/char/vt_ioctl.c
linux/drivers/char/vt_ioctl.c
--- linux-2.6.20.1/drivers/char/vt_ioctl.c	2007-02-19 22:34:32.000000000
-0800
+++ linux/drivers/char/vt_ioctl.c	2007-03-08 14:15:41.000000000 -0800
@@ -34,7 +34,7 @@
 #include <linux/kbd_diacr.h>
 #include <linux/selection.h>
 
-static char vt_dont_switch;
+char vt_dont_switch;
 extern struct tty_driver *console_driver;
 
 #define VT_IS_IN_USE(i)	(console_driver->ttys[i] &&
console_driver->ttys[i]->count)
diff -rup linux-2.6.20.1/include/linux/kbd_kern.h
linux/include/linux/kbd_kern.h
--- linux-2.6.20.1/include/linux/kbd_kern.h	2007-02-19
22:34:32.000000000 -0800
+++ linux/include/linux/kbd_kern.h	2007-03-08 14:15:41.000000000 -0800
@@ -75,7 +75,7 @@ extern int do_poke_blanked_console;
 
 extern void (*kbd_ledfunc)(unsigned int led);
 
-extern void set_console(int nr);
+extern int set_console(int nr);
 extern void schedule_console_callback(void);
 
 static inline void set_leds(void)
diff -rup linux-2.6.20.1/kernel/power/console.c
linux/kernel/power/console.c
--- linux-2.6.20.1/kernel/power/console.c	2007-02-19 22:34:32.000000000
-0800
+++ linux/kernel/power/console.c	2007-03-08 14:15:41.000000000 -0800
@@ -27,7 +27,11 @@ int pm_prepare_console(void)
 		return 1;
 	}
 
-	set_console(SUSPEND_CONSOLE);
+	if (set_console(SUSPEND_CONSOLE)) {
+		/* Unable to change to the new console */
+		release_console_sem();
+		return 1;
+	}
 	release_console_sem();
 
 	if (vt_waitactive(SUSPEND_CONSOLE)) {



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Software Suspend: Fix suspend when console is in VT_AUTO/KD_GRAPHICS mode
  2007-03-09 15:34   ` Matthew Garrett
  2007-03-09 16:10     ` Andrew Johnson
  2007-03-09 19:02     ` Andrew Johnson
@ 2007-03-09 21:11     ` Pavel Machek
  2 siblings, 0 replies; 14+ messages in thread
From: Pavel Machek @ 2007-03-09 21:11 UTC (permalink / raw)
  To: Matthew Garrett; +Cc: Andrew Johnson, linux-kernel

Hi!

> > So... if current console is graphical, we leave X accessing the
> > console... That's bad, because video state is not going to be
> > restored...?
> 
> A graphical console is not necessarily X. Is there any requirement for 
> there to be a single VT that isn't in text mode? The vt switching is 
> a hack, we shouldn't make life difficult for people who have their own 
> userspace code that's entirely capable of restoring video state on its 
> own.

How does their wonderful userspace code know that it needs to restore
video?

vt switch *does* mean 'kernel now takes over console'.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Software Suspend: Fix suspend when console is in VT_AUTO/KD_GRAPHICS mode
  2007-03-09 16:10     ` Andrew Johnson
@ 2007-03-09 21:13       ` Pavel Machek
  2007-03-09 21:58         ` Andrew Johnson
  0 siblings, 1 reply; 14+ messages in thread
From: Pavel Machek @ 2007-03-09 21:13 UTC (permalink / raw)
  To: Andrew Johnson; +Cc: Matthew Garrett, linux-kernel

Hi!

> > > So... if current console is graphical, we leave X accessing the
> > > console... That's bad, because video state is not going to be
> > > restored...?
> > 
> > A graphical console is not necessarily X. Is there any requirement for
> > there to be a single VT that isn't in text mode? The vt switching is
> > a hack, we shouldn't make life difficult for people who have their own
> > userspace code that's entirely capable of restoring video state on its
> > own.
> 
> The problem actually comes about when using Qtopia Phone Edition (QPE)
> on a PXA270. QPE puts the console into VT_AUTO+KD_GRAPHICS mode and
> writes directly to the framebuffer from then on.  In this mode the
> kernel correctly disallows a console change, as QPE is not getting
> notification of a console change and thus does not know when to repaint
> the screen. 

Fix qpe to use vt_process, instead?

...how does qpe know when to repaint the screen, anyway?

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Software Suspend: Fix suspend when console is in VT_AUTO/KD_GRAPHICS mode
  2007-03-09 19:02     ` Andrew Johnson
@ 2007-03-09 21:19       ` Pavel Machek
  2007-03-10  0:05         ` Andrew Johnson
  0 siblings, 1 reply; 14+ messages in thread
From: Pavel Machek @ 2007-03-09 21:19 UTC (permalink / raw)
  To: Andrew Johnson; +Cc: Matthew Garrett, linux-kernel

Hi!

> > > So... if current console is graphical, we leave X accessing the
> > > console... That's bad, because video state is not going to be
> > > restored...?
> > 
> > A graphical console is not necessarily X. Is there any requirement for 
> > there to be a single VT that isn't in text mode? The vt switching is 
> > a hack, we shouldn't make life difficult for people who have their own 
> > userspace code that's entirely capable of restoring video state on its 
> > own.
> 
> I realised that the previous patch would disallow a console switch while
> running X.  Attached is an updated patch with this scenario fixed.
> 
> Another approach might be to fail in vt_waitactive() if a console switch
> is not going to occur.
> 
> -- Andrew
> 

>  
> -void set_console(int nr)
> +extern char vt_dont_switch;
> +

What does this variable do and why do we want to use it here?

> +int set_console(int nr)
>  {
> +	struct vc_data *vc = vc_cons[fg_console].d;
> +
> +	if(!vc_cons_allocated(nr) || vt_dont_switch || 

'if ('

> +		(vc->vt_mode.mode != VT_PROCESS && vc->vc_mode == KD_GRAPHICS)) {
> +
> +		return -EINVAL;
> +	}
> +

I assume you want ...mode == VT_AUTO here?

And big comment explaining why we want this behaviour?

And another big comment explaining why this will not break existing
set_console() users?

>  	want_console = nr;
>  	schedule_console_callback();
> +
> +	return 0;
>  }
>  
>  struct tty_driver *console_driver;
> diff -rup linux-2.6.20.1/drivers/char/vt_ioctl.c
> linux/drivers/char/vt_ioctl.c
> --- linux-2.6.20.1/drivers/char/vt_ioctl.c	2007-02-19 22:34:32.000000000
> -0800
> +++ linux/drivers/char/vt_ioctl.c	2007-03-08 14:15:41.000000000 -0800
> @@ -34,7 +34,7 @@
>  #include <linux/kbd_diacr.h>
>  #include <linux/selection.h>
>  
> -static char vt_dont_switch;
> +char vt_dont_switch;
>  extern struct tty_driver *console_driver;
>  
>  #define VT_IS_IN_USE(i)	(console_driver->ttys[i] &&
> console_driver->ttys[i]->count)
> diff -rup linux-2.6.20.1/include/linux/kbd_kern.h
> linux/include/linux/kbd_kern.h
> --- linux-2.6.20.1/include/linux/kbd_kern.h	2007-02-19
> 22:34:32.000000000 -0800
> +++ linux/include/linux/kbd_kern.h	2007-03-08 14:15:41.000000000 -0800
> @@ -75,7 +75,7 @@ extern int do_poke_blanked_console;
>  
>  extern void (*kbd_ledfunc)(unsigned int led);
>  
> -extern void set_console(int nr);
> +extern int set_console(int nr);
>  extern void schedule_console_callback(void);
>  
>  static inline void set_leds(void)
> diff -rup linux-2.6.20.1/kernel/power/console.c
> linux/kernel/power/console.c
> --- linux-2.6.20.1/kernel/power/console.c	2007-02-19 22:34:32.000000000
> -0800
> +++ linux/kernel/power/console.c	2007-03-08 14:15:41.000000000 -0800
> @@ -27,7 +27,11 @@ int pm_prepare_console(void)
>  		return 1;
>  	}
>  
> -	set_console(SUSPEND_CONSOLE);
> +	if (set_console(SUSPEND_CONSOLE)) {
> +		/* Unable to change to the new console */

That's not what the comment should say.

It should explain why it is okay to proceed when we can't change to
text console.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Software Suspend: Fix suspend when console is in VT_AUTO/KD_GRAPHICS mode
  2007-03-09 21:13       ` Pavel Machek
@ 2007-03-09 21:58         ` Andrew Johnson
  2007-03-10 22:26           ` Pavel Machek
  0 siblings, 1 reply; 14+ messages in thread
From: Andrew Johnson @ 2007-03-09 21:58 UTC (permalink / raw)
  To: Pavel Machek; +Cc: Matthew Garrett, linux-kernel

On Fri, 2007-09-03 at 21:13 +0000, Pavel Machek wrote:
> Hi!
> 
> > > > So... if current console is graphical, we leave X accessing the
> > > > console... That's bad, because video state is not going to be
> > > > restored...?
> > > 
> > > A graphical console is not necessarily X. Is there any requirement for
> > > there to be a single VT that isn't in text mode? The vt switching is
> > > a hack, we shouldn't make life difficult for people who have their own
> > > userspace code that's entirely capable of restoring video state on its
> > > own.
> > 
> > The problem actually comes about when using Qtopia Phone Edition (QPE)
> > on a PXA270. QPE puts the console into VT_AUTO+KD_GRAPHICS mode and
> > writes directly to the framebuffer from then on.  In this mode the
> > kernel correctly disallows a console change, as QPE is not getting
> > notification of a console change and thus does not know when to repaint
> > the screen. 
> 
> Fix qpe to use vt_process, instead?

The real issue seems to be the assumption in pm_prepare_console() that
set_console() will succeed.  set_console() may fail (resulting in
vt_waitactive waiting for a console switch that can never happen or a
break) for two currently unchecked reasons:

- console is in VT_AUTO+KD_GRAPHICS mode
- vt_dont_switch is set

> ...how does qpe know when to repaint the screen, anyway?

QPE doesn't need to repaint the screen after wake-up - the framebuffer
memory is retained so the PXA270 lcd controller simply displays what was
last on the screen when it is re-enabled.

-- Andrew









^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Software Suspend: Fix suspend when console is in VT_AUTO/KD_GRAPHICS mode
  2007-03-09 21:19       ` Pavel Machek
@ 2007-03-10  0:05         ` Andrew Johnson
  2007-03-10 22:28           ` Pavel Machek
  0 siblings, 1 reply; 14+ messages in thread
From: Andrew Johnson @ 2007-03-10  0:05 UTC (permalink / raw)
  To: Pavel Machek; +Cc: Matthew Garrett, linux-kernel

On Fri, 2007-09-03 at 13:19 -0800, Pavel Machek wrote:
> Hi!
> 
> > 
> > -void set_console(int nr)
> > +extern char vt_dont_switch;
> > +
> 
> What does this variable do and why do we want to use it here?
> 
It's needed in set_console().  Console switch will fail if this is true.

> 'if ('
> 
> > +             (vc->vt_mode.mode != VT_PROCESS && vc->vc_mode ==
> KD_GRAPHICS)) {
> > +
> > +             return -EINVAL;
> > +     }
> > +
> 
> I assume you want ...mode == VT_AUTO here?
> 
> And big comment explaining why we want this behaviour?
> 
> And another big comment explaining why this will not break existing
> set_console() users?
> 
See updated comment in attached patch.

> > -     set_console(SUSPEND_CONSOLE);
> > +     if (set_console(SUSPEND_CONSOLE)) {
> > +             /* Unable to change to the new console */
> 
> That's not what the comment should say.
> 
> It should explain why it is okay to proceed when we can't change to
> text console.
> 
See updated comment in attached patch.  It's really up to the caller to
decide what to do if we can't switch the console - currently all callers
ignore the return code so I assume that it's okay to proceed anyway.

-- Andrew


Signed-off-by: Andrew Johnson <ajohnson@intrinsyc.com>
---
diff -rup linux-2.6.20.1/drivers/char/vt.c linux/drivers/char/vt.c
--- linux-2.6.20.1/drivers/char/vt.c	2007-02-19 22:34:32.000000000 -0800
+++ linux/drivers/char/vt.c	2007-03-09 15:48:29.000000000 -0800
@@ -2188,10 +2188,30 @@ static void console_callback(struct work
 	release_console_sem();
 }
 
-void set_console(int nr)
+extern char vt_dont_switch;
+
+int set_console(int nr)
 {
+	struct vc_data *vc = vc_cons[fg_console].d;
+
+	if(!vc_cons_allocated(nr) || vt_dont_switch || 
+		(vc->vt_mode.mode == VT_AUTO && vc->vc_mode == KD_GRAPHICS)) {
+
+		/* 
+		 * Console switch will fail in console_callback() or 
+		 * change_console() so there is no point scheduling 
+		 * the callback
+		 *
+		 * Existing set_console() users don't check the return
+		 * value so this shouldn't break anything 
+		 */ 
+		return -EINVAL;
+	}
+
 	want_console = nr;
 	schedule_console_callback();
+
+	return 0;
 }
 
 struct tty_driver *console_driver;
diff -rup linux-2.6.20.1/drivers/char/vt_ioctl.c
linux/drivers/char/vt_ioctl.c
--- linux-2.6.20.1/drivers/char/vt_ioctl.c	2007-02-19 22:34:32.000000000
-0800
+++ linux/drivers/char/vt_ioctl.c	2007-03-08 14:15:41.000000000 -0800
@@ -34,7 +34,7 @@
 #include <linux/kbd_diacr.h>
 #include <linux/selection.h>
 
-static char vt_dont_switch;
+char vt_dont_switch;
 extern struct tty_driver *console_driver;
 
 #define VT_IS_IN_USE(i)	(console_driver->ttys[i] &&
console_driver->ttys[i]->count)
diff -rup linux-2.6.20.1/include/linux/kbd_kern.h
linux/include/linux/kbd_kern.h
--- linux-2.6.20.1/include/linux/kbd_kern.h	2007-02-19
22:34:32.000000000 -0800
+++ linux/include/linux/kbd_kern.h	2007-03-08 14:15:41.000000000 -0800
@@ -75,7 +75,7 @@ extern int do_poke_blanked_console;
 
 extern void (*kbd_ledfunc)(unsigned int led);
 
-extern void set_console(int nr);
+extern int set_console(int nr);
 extern void schedule_console_callback(void);
 
 static inline void set_leds(void)
diff -rup linux-2.6.20.1/kernel/power/console.c
linux/kernel/power/console.c
--- linux-2.6.20.1/kernel/power/console.c	2007-02-19 22:34:32.000000000
-0800
+++ linux/kernel/power/console.c	2007-03-09 15:52:32.000000000 -0800
@@ -27,7 +27,15 @@ int pm_prepare_console(void)
 		return 1;
 	}
 
-	set_console(SUSPEND_CONSOLE);
+	if (set_console(SUSPEND_CONSOLE)) {
+		/*
+		 * We're unable to switch to the SUSPEND_CONSOLE. 
+		 * Let the calling function know so it can decide 
+		 * what to do.
+		 */ 
+		release_console_sem();
+		return 1;
+	}
 	release_console_sem();
 
 	if (vt_waitactive(SUSPEND_CONSOLE)) {





^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Software Suspend: Fix suspend when console is in VT_AUTO/KD_GRAPHICS mode
  2007-03-09 21:58         ` Andrew Johnson
@ 2007-03-10 22:26           ` Pavel Machek
  0 siblings, 0 replies; 14+ messages in thread
From: Pavel Machek @ 2007-03-10 22:26 UTC (permalink / raw)
  To: Andrew Johnson; +Cc: Matthew Garrett, linux-kernel

Hi!

> > ...how does qpe know when to repaint the screen, anyway?
> 
> QPE doesn't need to repaint the screen after wake-up - the framebuffer
> memory is retained so the PXA270 lcd controller simply displays what was
> last on the screen when it is re-enabled.

That probably means QPE is broken on machines that do not preserve
framebuffer over suspend :-(.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Software Suspend: Fix suspend when console is in VT_AUTO/KD_GRAPHICS mode
  2007-03-10  0:05         ` Andrew Johnson
@ 2007-03-10 22:28           ` Pavel Machek
  0 siblings, 0 replies; 14+ messages in thread
From: Pavel Machek @ 2007-03-10 22:28 UTC (permalink / raw)
  To: Andrew Johnson; +Cc: Matthew Garrett, linux-kernel

Hi1

> > It should explain why it is okay to proceed when we can't change to
> > text console.
> > 
> See updated comment in attached patch.  It's really up to the caller to
> decide what to do if we can't switch the console - currently all callers
> ignore the return code so I assume that it's okay to proceed anyway.

Ok, I guess the patch is right thing to do after all.  Fix issues
below, append a changelog, and send a patch to lmkl, cc Andrew Morton
and me. Oh and you have my ACK.
								Pavel

> Signed-off-by: Andrew Johnson <ajohnson@intrinsyc.com>
> ---
> diff -rup linux-2.6.20.1/drivers/char/vt.c linux/drivers/char/vt.c
> --- linux-2.6.20.1/drivers/char/vt.c	2007-02-19 22:34:32.000000000 -0800
> +++ linux/drivers/char/vt.c	2007-03-09 15:48:29.000000000 -0800
> @@ -2188,10 +2188,30 @@ static void console_callback(struct work
>  	release_console_sem();
>  }
>  
> -void set_console(int nr)
> +extern char vt_dont_switch;
> +
> +int set_console(int nr)
>  {
> +	struct vc_data *vc = vc_cons[fg_console].d;
> +
> +	if(!vc_cons_allocated(nr) || vt_dont_switch || 

there should be space between "if" and "(".

> diff -rup linux-2.6.20.1/drivers/char/vt_ioctl.c
> linux/drivers/char/vt_ioctl.c
> --- linux-2.6.20.1/drivers/char/vt_ioctl.c	2007-02-19 22:34:32.000000000
> -0800
> +++ linux/drivers/char/vt_ioctl.c	2007-03-08 14:15:41.000000000

And your mailer still wordwraps.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2007-03-10 22:28 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-03-08 22:54 [PATCH] Software Suspend: Fix suspend when console is in VT_AUTO/KD_GRAPHICS mode Andrew Johnson
2007-03-08 23:37 ` Daniel Drake
2007-03-09  5:01   ` Andrew Johnson
2007-03-09  9:08 ` Pavel Machek
2007-03-09 15:34   ` Matthew Garrett
2007-03-09 16:10     ` Andrew Johnson
2007-03-09 21:13       ` Pavel Machek
2007-03-09 21:58         ` Andrew Johnson
2007-03-10 22:26           ` Pavel Machek
2007-03-09 19:02     ` Andrew Johnson
2007-03-09 21:19       ` Pavel Machek
2007-03-10  0:05         ` Andrew Johnson
2007-03-10 22:28           ` Pavel Machek
2007-03-09 21:11     ` Pavel Machek

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.