[PATCH] Add a new VT mode which is like VT_PROCESS but doesn't require a VT_RELDISP ioctl call

[PATCH] Add a new VT mode which is like VT_PROCESS but doesn't require a VT_RELDISP ioctl call

[atrigent]

This new VT mode (VT_PROCESS_AUTO) does everything that VT_PROCESS does
except that it doesn't wait for a VT_RELDISP ioctl before switching
away from a VT with that mode.

If the X server eventually uses this new mode, debugging and crash
recovery should become easier. This is because even when currently in
the VT of a frozen X server it would still be possible to switch out
by doing SysRq-r and then CTRL-<number of a text vt>, sshing in and
doing chvt <number of a text vt>, or any other method of VT switching.
The general concensus on #xorg-devel seems to be that it should be
safe to use this with X now that we have KMS.

Signed-off-by: Ari Entlich <atrigent@ccs.neu.edu>
---
This should apply to Linus' tree. It has been compile tested only - I just
want to see whether I'm on the right track/whether there's anything big I
overlooked/whether there are any obvious mistakes here.

I was advised to CC Linus and akpm because of the subsystem that this
touches. I hope that was appropriate.

Thanks!

 drivers/char/vt_ioctl.c |   39 ++++++++++++++++++++-------------------
 include/linux/vt.h      |    1 +
 2 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/char/vt_ioctl.c b/drivers/char/vt_ioctl.c
index 6aa1028..87778dc 100644
--- a/drivers/char/vt_ioctl.c
+++ b/drivers/char/vt_ioctl.c
@@ -888,7 +888,7 @@ int vt_ioctl(struct tty_struct *tty, struct file * file,
 			ret = -EFAULT;
 			goto out;
 		}
-		if (tmp.mode != VT_AUTO && tmp.mode != VT_PROCESS) {
+		if (tmp.mode != VT_AUTO && tmp.mode != VT_PROCESS && tmp.mode != VT_PROCESS_AUTO) {
 			ret = -EINVAL;
 			goto out;
 		}
@@ -1622,7 +1622,7 @@ static void complete_change_console(struct vc_data *vc)
 	 * telling it that it has acquired. Also check if it has died and
 	 * clean up (similar to logic employed in change_console())
 	 */
-	if (vc->vt_mode.mode == VT_PROCESS) {
+	if (vc->vt_mode.mode == VT_PROCESS || vc->vt_mode.mode == VT_PROCESS_AUTO) {
 		/*
 		 * Send the signal as privileged - kill_pid() will
 		 * tell us if the process has gone or something else
@@ -1682,7 +1682,7 @@ void change_console(struct vc_data *new_vc)
 	 * vt to auto control.
 	 */
 	vc = vc_cons[fg_console].d;
-	if (vc->vt_mode.mode == VT_PROCESS) {
+	if (vc->vt_mode.mode == VT_PROCESS || vc->vt_mode.mode == VT_PROCESS_AUTO) {
 		/*
 		 * Send the signal as privileged - kill_pid() will
 		 * tell us if the process has gone or something else
@@ -1693,27 +1693,28 @@ void change_console(struct vc_data *new_vc)
 		 */
 		vc->vt_newvt = new_vc->vc_num;
 		if (kill_pid(vc->vt_pid, vc->vt_mode.relsig, 1) == 0) {
+			if(vc->vt_mode.mode == VT_PROCESS)
+				/*
+				 * It worked. Mark the vt to switch to and
+				 * return. The process needs to send us a
+				 * VT_RELDISP ioctl to complete the switch.
+				 */
+				return;
+		} else {
 			/*
-			 * It worked. Mark the vt to switch to and
-			 * return. The process needs to send us a
-			 * VT_RELDISP ioctl to complete the switch.
+			 * The controlling process has died, so we revert back to
+			 * normal operation. In this case, we'll also change back
+			 * to KD_TEXT mode. I'm not sure if this is strictly correct
+			 * but it saves the agony when the X server dies and the screen
+			 * remains blanked due to KD_GRAPHICS! It would be nice to do
+			 * this outside of VT_PROCESS but there is no single process
+			 * to account for and tracking tty count may be undesirable.
 			 */
-			return;
+			reset_vc(vc);
 		}
 
 		/*
-		 * The controlling process has died, so we revert back to
-		 * normal operation. In this case, we'll also change back
-		 * to KD_TEXT mode. I'm not sure if this is strictly correct
-		 * but it saves the agony when the X server dies and the screen
-		 * remains blanked due to KD_GRAPHICS! It would be nice to do
-		 * this outside of VT_PROCESS but there is no single process
-		 * to account for and tracking tty count may be undesirable.
-		 */
-		reset_vc(vc);
-
-		/*
-		 * Fall through to normal (VT_AUTO) handling of the switch...
+		 * Fall through to normal (VT_AUTO and VT_PROCESS_AUTO) handling of the switch...
 		 */
 	}
 
diff --git a/include/linux/vt.h b/include/linux/vt.h
index d5dd0bc..dedf419 100644
--- a/include/linux/vt.h
+++ b/include/linux/vt.h
@@ -27,6 +27,7 @@ struct vt_mode {
 #define VT_SETMODE	0x5602	/* set mode of active vt */
 #define		VT_AUTO		0x00	/* auto vt switching */
 #define		VT_PROCESS	0x01	/* process controls switching */
+#define		VT_PROCESS_AUTO 0x02	/* process is notified of switching */
 #define		VT_ACKACQ	0x02	/* acknowledge switch */
 
 struct vt_stat {
-- 
1.6.4.4

Re: [PATCH] Add a new VT mode which is like VT_PROCESS but doesn't require a VT_RELDISP ioctl call

[Alan Cox]

> If the X server eventually uses this new mode, debugging and crash
> recovery should become easier. This is because even when currently in
> the VT of a frozen X server it would still be possible to switch out
> by doing SysRq-r and then CTRL-<number of a text vt>, sshing in and
> doing chvt <number of a text vt>, or any other method of VT switching.
> The general concensus on #xorg-devel seems to be that it should be
> safe to use this with X now that we have KMS.
> 
> Signed-off-by: Ari Entlich <atrigent@ccs.neu.edu>

If KMS doesn't need to mode switch then does it need any of this stuff -
can it not just switch to a console and use it ? Otherwise please use a
different value than ACKACQ (just for clarity) and conceptually it looks
fine to me - I just question if it's really needed as KMS based X not
having to play with magic locking stuff would be cleaner still.

Alan

Re: [PATCH] Add a new VT mode which is like VT_PROCESS but doesn't require a VT_RELDISP ioctl call

[Ari G. Entlich]

----- "Alan Cox" <alan@lxorguk.ukuu.org.uk> wrote:
> If KMS doesn't need to mode switch then does it need any of this stuff-
> can it not just switch to a console and use it ? Otherwise please use a
> different value than ACKACQ (just for clarity) and conceptually it looks
> fine to me - I just question if it's really needed as KMS based X not
> having to play with magic locking stuff would be cleaner still.

I'm not positive I understand what you're saying. Are you asking why we can't
just use VT_AUTO now? That's something I investigated, and there are a couple
reasons why it wouldn't be suitable:

1. VT_AUTO doesn't send signals to anything when a VT switch happens, precisely
   because VT_AUTO is supposed to be used in the case where there's nothing to
   send signals TO (i.e. the VT is managed by the kernel). The X server still
   needs to know about VT switches to turn input devices off and such.
2. The kernel ignores VT switches when switching away from a VT which is in
   KD_GRAPHICS + VT_AUTO mode. I'm not sure what the reason for this is, and
   I'm also not sure why it doesn't ignore switches TO these sorts of VTs, but
   that's the reality and my impression is that it's generally not a good idea
   to change these sorts of things when they've been in place for this long.

Is this what you were asking about? Also, in terms of what you said about
VT_ACKACQ, it wouldn't really make sense for there to be a new VT_ACKACQ value,
because VT_ACKACQ is something which gets passed to a VT_RELDISP, and VT_RELDISP
isn't needed at all in this new mode.

I hope that clarifies things.

Thanks!

Ari

Re: [PATCH] Add a new VT mode which is like VT_PROCESS but doesn't require a VT_RELDISP ioctl call

[Alan Cox]

> 1. VT_AUTO doesn't send signals to anything when a VT switch happens, precisely
>    because VT_AUTO is supposed to be used in the case where there's nothing to
>    send signals TO (i.e. the VT is managed by the kernel). The X server still
>    needs to know about VT switches to turn input devices off and such.

Ok..

> VT_ACKACQ, it wouldn't really make sense for there to be a new VT_ACKACQ value,
> because VT_ACKACQ is something which gets passed to a VT_RELDISP, and VT_RELDISP
> isn't needed at all in this new mode.

I don't want to change the existing values as they are somewhat visible
to user space.

> I hope that clarifies things.

Yes. You could use the VT_EVENT facility for the switch monitoring but
the asynchronous nature of the reporting probably isn't what is needed
for input device switching etc.

Looks fine to me - just bump the value and resubmit

Re: [PATCH] Add a new VT mode which is like VT_PROCESS but doesn't require a VT_RELDISP ioctl call

[Ari G. Entlich]

----- "Alan Cox" <alan@lxorguk.ukuu.org.uk> wrote:
> I don't want to change the existing values as they are somewhat visible
> to user space.

Sorry if I was unclear - I wasn't talking about changing the value, I was
just saying that VT_ACKACQ and VT_PROCESS_AUTO are used in different
contexts, so it shouldn't matter that they have the same value. One thing
that probably would be nice though would be to move the VT_ACKACQ define
to a different place in vt.h (probably after the VT_RELDISP define).

> Yes. You could use the VT_EVENT facility for the switch monitoring but
> the asynchronous nature of the reporting probably isn't what is needed
> for input device switching etc.

Yeah, it looks like the X server would have to be constantly blocking
inside a VT_WAITEVENT ioctl in order to use that, and then it wouldn't
be getting anything else done. :-/

Ari

Re: [PATCH] Add a new VT mode which is like VT_PROCESS but doesn't require a VT_RELDISP ioctl call

[Alan Cox]

> Sorry if I was unclear - I wasn't talking about changing the value, I was
> just saying that VT_ACKACQ and VT_PROCESS_AUTO are used in different
> contexts, so it shouldn't matter that they have the same value. One thing
> that probably would be nice though would be to move the VT_ACKACQ define
> to a different place in vt.h (probably after the VT_RELDISP define).

That would just as well. It just needs to be obvious for future
maintainers that the overlap doesn't matter

Re: [PATCH] Add a new VT mode which is like VT_PROCESS but doesn't require a VT_RELDISP ioctl call

[Ari Entlich]

----- "Alan Cox" <alan@lxorguk.ukuu.org.uk> wrote:
> That would just as well. It just needs to be obvious for future
> maintainers that the overlap doesn't matter

Alright, cool. Should I submit two patches (one for the define move
and one for the actual patch), or would one patch be fine?

Ari