* [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.