Magic SysRq +# in 2.4.9-ac/2.4.10-pre12

Magic SysRq +# in 2.4.9-ac/2.4.10-pre12

[Randy.Dunlap]

(and maybe earlier...)

Simple problems grow...

Keith Owens has already noted one problem in sysrq.c (2.4.10-pre12).

Beginning:

I have an IBM model KB-9910 keyboard.  When I use
Alt+SysRQ+number (number: 0...9) on it to change the
console loglevel, only keys 5 and 6 have the desired
effect.  I used showkey -s to view the scancodes from
the other <number> keys, but showkey didn't display
anything for them.  Any other suggestions?


For now, I'm just using different (non-number) keys
to modify the loglevel.

Anyway, in looking at SysRq loglevel handling in
2.4.9-ac (and 2.4.10-pre12), I see that it has been modified
quite a bit.  Looks extensible, which can be good.
However, looking over it gave me several nagging questions
and problems.

1.  Was this stuff tested?  How ???

It always sets console_loglevel and then restores
console_loglevel from orig_log_level, so Alt+SysRq+#
handling is severely broken.

If someone (Crutcher ?) wants to patch it, that's fine.
If I patched it, I would just add a
  next_loglevel = -1;
at the beginning of __handle_sysrq_nolock() and then
let the loglevel handler(s) set next_loglevel.
If next_loglevel != -1 at the end of __handle_sysrq_nolock(),
set console_loglevel to next_loglevel.

2.  I'd really prefer to see callers use
register_sysrq_key() and unregister_sysrq_key() so that they
can get/use return values, and not the lower-level functions
"__sysrq*" functions that are EXPORTed in sysrq.c.
I don't see a good reason to EXPORT all of these functions.

E.g., arch/ppc64/start/xmon.c calls __sysrq_put_key_op('x', ...).
It doesn't know (and cannot know) whether this call succeeded
or not.

3.  And the sysrq_key_table[] (comments) should end with
w, x, y, z, not with w, x, w, z.


~Randy

You can't do anything without having to do something else first. 
         -- Belefant's Law

Re: Magic SysRq +# in 2.4.9-ac/2.4.10-pre12

[Randy.Dunlap]

"Randy.Dunlap" wrote:
> 
> 1.  Was this stuff tested?  How ???
> 
> It always sets console_loglevel and then restores
> console_loglevel from orig_log_level, so Alt+SysRq+#
> handling is severely broken.
> 
> If someone (Crutcher ?) wants to patch it, that's fine.
> If I patched it, I would just add a
>   next_loglevel = -1;
> at the beginning of __handle_sysrq_nolock() and then
> let the loglevel handler(s) set next_loglevel.
> If next_loglevel != -1 at the end of __handle_sysrq_nolock(),
> set console_loglevel to next_loglevel.


I'll post a patch for these after I test it (soon).

~Randy

Re: Magic SysRq +# in 2.4.9-ac/2.4.10-pre12

[Alan Cox]

> Anyway, in looking at SysRq loglevel handling in
> 2.4.9-ac (and 2.4.10-pre12), I see that it has been modified
> quite a bit.  Looks extensible, which can be good.
> However, looking over it gave me several nagging questions
> and problems.
> 
> 1.  Was this stuff tested?  How ???

Its been in the ac tree for about 6 months and in several distro shippings

> If someone (Crutcher ?) wants to patch it, that's fine.
> If I patched it, I would just add a
>   next_loglevel = -1;
> at the beginning of __handle_sysrq_nolock() and then
> let the loglevel handler(s) set next_loglevel.
> If next_loglevel != -1 at the end of __handle_sysrq_nolock(),
> set console_loglevel to next_loglevel.

Send me a patch and cc Linus/Crutcher

Re: Magic SysRq +# in 2.4.9-ac/2.4.10-pre12

[Randy.Dunlap]

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

Alan Cox wrote:
> 
> > Anyway, in looking at SysRq loglevel handling in
> > 2.4.9-ac (and 2.4.10-pre12), I see that it has been modified
> > quite a bit.  Looks extensible, which can be good.
> > However, looking over it gave me several nagging questions
> > and problems.
> >
> > If someone (Crutcher ?) wants to patch it, that's fine.
> > If I patched it, I would just add a
> >   next_loglevel = -1;
> > at the beginning of __handle_sysrq_nolock() and then
> > let the loglevel handler(s) set next_loglevel.
> > If next_loglevel != -1 at the end of __handle_sysrq_nolock(),
> > set console_loglevel to next_loglevel.
> 
> Send me a patch and cc Linus/Crutcher

Here's a patch to fix SysRq handling of console_loglevel.

Please apply to 2.4.10-pre12 and to 2.4.9-ac12.

Thanks,
~Randy

[-- Attachment #2: sysrq-ll.patch --]
[-- Type: text/plain, Size: 4432 bytes --]

--- linux/drivers/char/sysrq.c.org	Mon Sep 17 10:15:48 2001
+++ linux/drivers/char/sysrq.c	Wed Sep 19 10:15:12 2001
@@ -39,6 +39,8 @@
 /* Whether we react on sysrq keys or just ignore them */
 int sysrq_enabled = 1;
 
+static int entry_loglevel, next_loglevel;
+
 /* Machine specific power off function */
 void (*sysrq_power_off)(void);
 
@@ -47,13 +49,13 @@
 		struct kbd_struct *kbd, struct tty_struct *tty) {
 	int i;
 	i = key - '0';
-	console_loglevel = 7;
 	printk("%d\n", i);
-	console_loglevel = i;
-}	
+	next_loglevel = i;
+}
+
 static struct sysrq_key_op sysrq_loglevel_op = {
 	handler:	sysrq_handle_loglevel,
-	help_msg:	"loglevel0-8",
+	help_msg:	"loglevel0-9",
 	action_msg:	"Loglevel set to ",
 };
 
@@ -66,6 +68,7 @@
 		do_SAK(tty);
 	reset_vc(fg_console);
 }
+
 static struct sysrq_key_op sysrq_SAK_op = {
 	handler:	sysrq_handle_SAK,
 	help_msg:	"saK",
@@ -137,9 +140,6 @@
 /* do_emergency_sync helper function */
 static void go_sync(struct super_block *sb, int remount_flag)
 {
-	int orig_loglevel;
-	orig_loglevel = console_loglevel;
-	console_loglevel = 7;
 	printk(KERN_INFO "%sing device %s ... ",
 	       remount_flag ? "Remount" : "Sync",
 	       kdevname(sb->s_dev));
@@ -178,7 +178,6 @@
 		fsync_dev(sb->s_dev);
 		printk("OK\n");
 	}
-	console_loglevel = orig_loglevel;
 }
 /*
  * Emergency Sync or Unmount. We cannot do it directly, so we set a special
@@ -192,7 +191,6 @@
 void do_emergency_sync(void) {
 	struct super_block *sb;
 	int remount_flag;
-	int orig_loglevel;
 
 	lock_kernel();
 	remount_flag = (emergency_sync_scheduled == EMERG_REMOUNT);
@@ -211,11 +209,7 @@
 			go_sync(sb, remount_flag);
 
 	unlock_kernel();
-
-	orig_loglevel = console_loglevel;
-	console_loglevel = 7;
 	printk(KERN_INFO "Done.\n");
-	console_loglevel = orig_loglevel;
 }
 
 static void sysrq_handle_sync(int key, struct pt_regs *pt_regs,
@@ -223,6 +217,7 @@
 	emergency_sync_scheduled = EMERG_SYNC;
 	wakeup_bdflush(0);
 }
+
 static struct sysrq_key_op sysrq_sync_op = {
 	handler:	sysrq_handle_sync,
 	help_msg:	"Sync",
@@ -250,6 +245,7 @@
 	if (pt_regs)
 		show_regs(pt_regs);
 }
+
 static struct sysrq_key_op sysrq_showregs_op = {
 	handler:	sysrq_handle_showregs,
 	help_msg:	"showPc",
@@ -261,6 +257,7 @@
 		struct kbd_struct *kbd, struct tty_struct *tty) {
 	show_state();
 }
+
 static struct sysrq_key_op sysrq_showstate_op = {
 	handler:	sysrq_handle_showstate,
 	help_msg:	"showTasks",
@@ -272,6 +269,7 @@
 		struct kbd_struct *kbd, struct tty_struct *tty) {
 	show_mem();
 }
+
 static struct sysrq_key_op sysrq_showmem_op = {
 	handler:	sysrq_handle_showmem,
 	help_msg:	"showMem",
@@ -303,8 +301,9 @@
 static void sysrq_handle_term(int key, struct pt_regs *pt_regs,
 		struct kbd_struct *kbd, struct tty_struct *tty) {
 	send_sig_all(SIGTERM, 0);
-	console_loglevel = 8;
+	next_loglevel = 8;
 }
+
 static struct sysrq_key_op sysrq_term_op = {
 	handler:	sysrq_handle_term,
 	help_msg:	"tErm",
@@ -314,8 +313,9 @@
 static void sysrq_handle_kill(int key, struct pt_regs *pt_regs,
 		struct kbd_struct *kbd, struct tty_struct *tty) {
 	send_sig_all(SIGKILL, 0);
-	console_loglevel = 8;
+	next_loglevel = 8;
 }
+
 static struct sysrq_key_op sysrq_kill_op = {
 	handler:	sysrq_handle_kill,
 	help_msg:	"kIll",
@@ -325,8 +325,9 @@
 static void sysrq_handle_killall(int key, struct pt_regs *pt_regs,
 		struct kbd_struct *kbd, struct tty_struct *tty) {
 	send_sig_all(SIGKILL, 1);
-	console_loglevel = 8;
+	next_loglevel = 8;
 }
+
 static struct sysrq_key_op sysrq_killall_op = {
 	handler:	sysrq_handle_killall,
 	help_msg:	"killalL",
@@ -381,7 +382,7 @@
 /* v */	NULL,
 /* w */	NULL,
 /* x */	NULL,
-/* w */	NULL,
+/* y */	NULL,
 /* z */	NULL
 };
 
@@ -434,6 +435,7 @@
 
 void handle_sysrq(int key, struct pt_regs *pt_regs,
 		  struct kbd_struct *kbd, struct tty_struct *tty) {
+
 	if (!sysrq_enabled)
 		return;
 
@@ -451,13 +453,13 @@
 void __handle_sysrq_nolock(int key, struct pt_regs *pt_regs,
 		  struct kbd_struct *kbd, struct tty_struct *tty) {
 	struct sysrq_key_op *op_p;
-	int orig_log_level;
 	int i, j;
 	
 	if (!sysrq_enabled)
 		return;
 
-	orig_log_level = console_loglevel;
+	entry_loglevel = console_loglevel;
+	next_loglevel = -1;
 	console_loglevel = 7;
 	printk(KERN_INFO "SysRq : ");
 
@@ -476,7 +478,7 @@
 		}
 		printk ("\n");
 	}
-	console_loglevel = orig_log_level;
+	console_loglevel = (next_loglevel == -1) ? entry_loglevel : next_loglevel;
 }
 
 EXPORT_SYMBOL(handle_sysrq);

Re: Magic SysRq +# in 2.4.9-ac/2.4.10-pre12

[Erik Mouw]

On Wed, Sep 19, 2001 at 08:56:13AM -0700, Randy.Dunlap wrote:
> I have an IBM model KB-9910 keyboard.  When I use
> Alt+SysRQ+number (number: 0...9) on it to change the
> console loglevel, only keys 5 and 6 have the desired
> effect.  I used showkey -s to view the scancodes from
> the other <number> keys, but showkey didn't display
> anything for them.  Any other suggestions?

Same over here with an IBM PS/2 keyboard that originally came with an
IBM PS2 model 55SX. The IBM keyboard is connected to an Asus M8300
laptop. The keyboard of that laptop has the interesting "feature" that
Alt-SysRQ-m sets the loglevel to 0, and Alt-SysRQ-[suob] also set the
loglevel to a different value instead of doing their job.


Erik

-- 
J.A.K. (Erik) Mouw, Information and Communication Theory Group, Department
of Electrical Engineering, Faculty of Information Technology and Systems,
Delft University of Technology, PO BOX 5031,  2600 GA Delft, The Netherlands
Phone: +31-15-2783635  Fax: +31-15-2781843  Email: J.A.K.Mouw@its.tudelft.nl
WWW: http://www-ict.its.tudelft.nl/~erik/

Re: Magic SysRq +# in 2.4.9-ac/2.4.10-pre12

[Randy.Dunlap]

Erik Mouw wrote:
> 
> On Wed, Sep 19, 2001 at 08:56:13AM -0700, Randy.Dunlap wrote:
> > I have an IBM model KB-9910 keyboard.  When I use
> > Alt+SysRQ+number (number: 0...9) on it to change the
> > console loglevel, only keys 5 and 6 have the desired
> > effect.  I used showkey -s to view the scancodes from
> > the other <number> keys, but showkey didn't display
> > anything for them.  Any other suggestions?
> 
> Same over here with an IBM PS/2 keyboard that originally came with an
> IBM PS2 model 55SX. The IBM keyboard is connected to an Asus M8300
> laptop. The keyboard of that laptop has the interesting "feature" that
> Alt-SysRQ-m sets the loglevel to 0, and Alt-SysRQ-[suob] also set the
> loglevel to a different value instead of doing their job.

I'm having this (my same) problem on a different test system/keyboard,
and I'm beginning to think that it's not a keyboard problem,
but I don't have any evidence of that one way or the other.

I've tested 2.4.2, 2.4.5, 2.4.6, 2.4.7, 2.4.9, and 2.4.10-pre,
and all exhibit the same problem.

~Randy

Re: Magic SysRq +# in 2.4.9-ac/2.4.10-pre12

[Randy.Dunlap]

"Randy.Dunlap" wrote:
> 
> Erik Mouw wrote:
> >
> > On Wed, Sep 19, 2001 at 08:56:13AM -0700, Randy.Dunlap wrote:
> > > I have an IBM model KB-9910 keyboard.  When I use
> > > Alt+SysRQ+number (number: 0...9) on it to change the
> > > console loglevel, only keys 5 and 6 have the desired
> > > effect.  I used showkey -s to view the scancodes from
> > > the other <number> keys, but showkey didn't display
> > > anything for them.  Any other suggestions?
> >
> > Same over here with an IBM PS/2 keyboard that originally came with an
> > IBM PS2 model 55SX. The IBM keyboard is connected to an Asus M8300
> > laptop. The keyboard of that laptop has the interesting "feature" that
> > Alt-SysRQ-m sets the loglevel to 0, and Alt-SysRQ-[suob] also set the
> > loglevel to a different value instead of doing their job.
> 
> I'm having this (my same) problem on a different test system/keyboard,
> and I'm beginning to think that it's not a keyboard problem,
> but I don't have any evidence of that one way or the other.
> 
> I've tested 2.4.2, 2.4.5, 2.4.6, 2.4.7, 2.4.9, and 2.4.10-pre,
> and all exhibit the same problem.

My Logitech Y-SG13 keyboard works nicely with Alt+SysRq+#,
so I won't go after other ghosts.

~Randy

Re: Magic SysRq +# in 2.4.9-ac/2.4.10-pre12

[Erik Mouw]

On Wed, Sep 19, 2001 at 10:34:27AM -0700, Randy.Dunlap wrote:
> Erik Mouw wrote:
> > Same over here with an IBM PS/2 keyboard that originally came with an
> > IBM PS2 model 55SX. The IBM keyboard is connected to an Asus M8300
> > laptop. The keyboard of that laptop has the interesting "feature" that
> > Alt-SysRQ-m sets the loglevel to 0, and Alt-SysRQ-[suob] also set the
> > loglevel to a different value instead of doing their job.
> 
> I'm having this (my same) problem on a different test system/keyboard,
> and I'm beginning to think that it's not a keyboard problem,
> but I don't have any evidence of that one way or the other.
> 
> I've tested 2.4.2, 2.4.5, 2.4.6, 2.4.7, 2.4.9, and 2.4.10-pre,
> and all exhibit the same problem.

Two other data points:

Desktop PC (Asus P5A motherboard, AMD K6/333) and Happy Hacking Lite
keyboard: only Alt-SysRQ-[sm] work, all other don't (unmount, off, and
reboot not tested, though). Kernel is 2.4.9-ac10.

LART StrongARM SA1100 board, serial console (sa1100 driver), kernel
2.4.9-ac9-rmk1-np1 (latest stable release for StrongARM machines):
everything works.

The evidence with the ARM board points in the direction of the keyboard
driver. I don't have a null modem cable right here so I can't test the
desktop with a serial console, but that would also give an interesting
data point.


Erik

PS: my laptop also runs 2.4.9-ac10

-- 
J.A.K. (Erik) Mouw, Information and Communication Theory Group, Department
of Electrical Engineering, Faculty of Information Technology and Systems,
Delft University of Technology, PO BOX 5031,  2600 GA Delft, The Netherlands
Phone: +31-15-2783635  Fax: +31-15-2781843  Email: J.A.K.Mouw@its.tudelft.nl
WWW: http://www-ict.its.tudelft.nl/~erik/

Re: Magic SysRq +# in 2.4.9-ac/2.4.10-pre12

[Vojtech Pavlik]

On Wed, Sep 19, 2001 at 08:56:13AM -0700, Randy.Dunlap wrote:
> (and maybe earlier...)
> 
> Simple problems grow...
> 
> Keith Owens has already noted one problem in sysrq.c (2.4.10-pre12).
> 
> Beginning:
> 
> I have an IBM model KB-9910 keyboard.  When I use
> Alt+SysRQ+number (number: 0...9) on it to change the
> console loglevel, only keys 5 and 6 have the desired
> effect.  I used showkey -s to view the scancodes from
> the other <number> keys, but showkey didn't display
> anything for them.  Any other suggestions?

Most likely the keyboard scanning matrix doesn't allow that combination.
Quite a large number of keyboards doesn't allow multiple keys pressed
(except for shift, ctrl, alt, which are separate) at once.

-- 
Vojtech Pavlik
SuSE Labs

Re: Magic SysRq +# in 2.4.9-ac/2.4.10-pre12

[Crutcher Dunnavant]

> 2.  I'd really prefer to see callers use
> register_sysrq_key() and unregister_sysrq_key() so that they
> can get/use return values, and not the lower-level functions
> "__sysrq*" functions that are EXPORTed in sysrq.c.
> I don't see a good reason to EXPORT all of these functions.

So would I, however, the lower interface is there so that modules can
restructure the table in more complex ways, allowing for sub-menus.

The really good answer here is to add registration functions for the top
level handler, so that sub handlers can just claim the top level events
without mucking with the table, and then restore the table handler
later. This allows really modeful handlers, with submenus, and
potentially even key entry. An example would be a handler to kill a
specific process.

I'm also looking at a patch from Amazon which allows sysrq to be
'sticky', to get arround bad keyboards and VTs, and allows which key the
magic key is to be setable, to get arround VTs lacking sysrq entirely. 

I am reviewing the things I apparently horked, and this amazon stuff
(which is very small) at the moment. Expect a pair of patches tomorrow,
or late tonight.

Ps. I am very embarassed about the log-level stuff.

-- 
Crutcher        <crutcher@datastacks.com>
GCS d--- s+:>+:- a-- C++++$ UL++++$ L+++$>++++ !E PS+++ PE Y+ PGP+>++++
    R-(+++) !tv(+++) b+(++++) G+ e>++++ h+>++ r* y+>*$

Re: Magic SysRq +# in 2.4.9-ac/2.4.10-pre12

[Pavel Machek]

Hi!

> > 2.  I'd really prefer to see callers use
> > register_sysrq_key() and unregister_sysrq_key() so that they
> > can get/use return values, and not the lower-level functions
> > "__sysrq*" functions that are EXPORTed in sysrq.c.
> > I don't see a good reason to EXPORT all of these functions.
> 
> So would I, however, the lower interface is there so that modules can
> restructure the table in more complex ways, allowing for sub-menus.

This is kernel, and sysrq was designed to be debug tool. It turned out
to be more successfull than expected...

Just keep in mind its a debug tool. If you need hierarchical submenus,
then you are probably not using it as debug tool, right?
								Pavel
-- 
I'm pavel@ucw.cz. "In my country we have almost anarchy and I don't care."
Panos Katsaloulis describing me w.r.t. patents at discuss@linmodems.org

Re: Magic SysRq +# in 2.4.9-ac/2.4.10-pre12

[Crutcher Dunnavant]

++ 26/09/01 22:38 +0200 - Pavel Machek:
> Hi!
> 
> > > 2.  I'd really prefer to see callers use
> > > register_sysrq_key() and unregister_sysrq_key() so that they
> > > can get/use return values, and not the lower-level functions
> > > "__sysrq*" functions that are EXPORTed in sysrq.c.
> > > I don't see a good reason to EXPORT all of these functions.
> > 
> > So would I, however, the lower interface is there so that modules can
> > restructure the table in more complex ways, allowing for sub-menus.
> 
> This is kernel, and sysrq was designed to be debug tool. It turned out
> to be more successfull than expected...
> 
> Just keep in mind its a debug tool. If you need hierarchical submenus,
> then you are probably not using it as debug tool, right?
> 								Pavel

Wrong. If I have heirarchal menus, then I can have debug code for many
parts of the kernel, and _detailed_ debug code for any given part, in
the sysrq handlers simultaneously.

-- 
Crutcher        <crutcher@datastacks.com>
GCS d--- s+:>+:- a-- C++++$ UL++++$ L+++$>++++ !E PS+++ PE Y+ PGP+>++++
    R-(+++) !tv(+++) b+(++++) G+ e>++++ h+>++ r* y+>*$

Re: Magic SysRq +# in 2.4.9-ac/2.4.10-pre12

[Crutcher Dunnavant]

++ 19/09/01 08:56 -0700 - Randy.Dunlap:
> (and maybe earlier...)
> 
> Simple problems grow...
> 
> Keith Owens has already noted one problem in sysrq.c (2.4.10-pre12).
> 
> Beginning:
> 
> I have an IBM model KB-9910 keyboard.  When I use
> Alt+SysRQ+number (number: 0...9) on it to change the
> console loglevel, only keys 5 and 6 have the desired
> effect.  I used showkey -s to view the scancodes from
> the other <number> keys, but showkey didn't display
> anything for them.  Any other suggestions?
> 
> 
> For now, I'm just using different (non-number) keys
> to modify the loglevel.
> 
> Anyway, in looking at SysRq loglevel handling in
> 2.4.9-ac (and 2.4.10-pre12), I see that it has been modified
> quite a bit.  Looks extensible, which can be good.
> However, looking over it gave me several nagging questions
> and problems.
> 
> 1.  Was this stuff tested?  How ???
> 
> It always sets console_loglevel and then restores
> console_loglevel from orig_log_level, so Alt+SysRq+#
> handling is severely broken.
> 
> If someone (Crutcher ?) wants to patch it, that's fine.
> If I patched it, I would just add a
>   next_loglevel = -1;
> at the beginning of __handle_sysrq_nolock() and then
> let the loglevel handler(s) set next_loglevel.
> If next_loglevel != -1 at the end of __handle_sysrq_nolock(),
> set console_loglevel to next_loglevel.

I'm looking real close at this right now, and there are a couple of
problems, and a simple, but ugly solution.

The entire reason that console_loglevel is touched _after_ the call to
the second level handler is actually for the loglevel handler's
printout. I was trying to minimize change in the display, but horked it.

Here is the problem.

SysRq events use action messages which get printed by the top level
handler before calling the second level handler, the call line is:

        orig_log_level = console_loglevel;
        console_loglevel = 7;
        printk(KERN_INFO "SysRq : ");

        op_p = __sysrq_get_key_op(key);
	...
        printk ("%s", op_p->action_msg);
        op_p->handler(key, pt_regs, kbd, tty);
	...
        console_loglevel = orig_log_level;


The killer here is the fact that the action message format string does
not carry a newline, allowing people to register strings which leave the
printk state open. The loglevel handler then fills in the loglevel, and
closes the printk state.

There was a time when I thought that was a good idea.

Go ahead, laugh.

Anyway, that sort of unresolved state is bad, and is the source of all
of this song and dance. I think the right answer is to force handlers to
open their own calls to printk, and to keep whats going on with the
console_loglevel and printk buffer nice and clean.

The cost is that messages like this:

SysRq : Loglevel switched to X

will have to become more like this:

SysRq : Loglevel
Loglevel switched to X


Again, appologies, and a patch is forthcoming.

-- 
Crutcher        <crutcher@datastacks.com>
GCS d--- s+:>+:- a-- C++++$ UL++++$ L+++$>++++ !E PS+++ PE Y+ PGP+>++++
    R-(+++) !tv(+++) b+(++++) G+ e>++++ h+>++ r* y+>*$

[PATCH] Magic SysRq loglevel fix.

[Crutcher Dunnavant]

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

Attached is a fix for this part of the sysrq code.

++ 21/09/01 17:08 -0400 - Crutcher Dunnavant:
> ++ 19/09/01 08:56 -0700 - Randy.Dunlap:
> > It always sets console_loglevel and then restores
> > console_loglevel from orig_log_level, so Alt+SysRq+#
> > handling is severely broken.
> > 
> > If someone (Crutcher ?) wants to patch it, that's fine.
> > If I patched it, I would just add a
> >   next_loglevel = -1;
> > at the beginning of __handle_sysrq_nolock() and then
> > let the loglevel handler(s) set next_loglevel.
> > If next_loglevel != -1 at the end of __handle_sysrq_nolock(),
> > set console_loglevel to next_loglevel.
> 
> I'm looking real close at this right now, and there are a couple of
> problems, and a simple, but ugly solution.
> 
> The entire reason that console_loglevel is touched _after_ the call to
> the second level handler is actually for the loglevel handler's
> printout. I was trying to minimize change in the display, but horked it.
> 
> Here is the problem.
> 
> SysRq events use action messages which get printed by the top level
> handler before calling the second level handler, the call line is:
> 
>         orig_log_level = console_loglevel;
>         console_loglevel = 7;
>         printk(KERN_INFO "SysRq : ");
> 
>         op_p = __sysrq_get_key_op(key);
> 	...
>         printk ("%s", op_p->action_msg);
>         op_p->handler(key, pt_regs, kbd, tty);
> 	...
>         console_loglevel = orig_log_level;
> 
> 
> The killer here is the fact that the action message format string does
> not carry a newline, allowing people to register strings which leave the
> printk state open. The loglevel handler then fills in the loglevel, and
> closes the printk state.
> 
> There was a time when I thought that was a good idea.
> 
> Go ahead, laugh.
> 
> Anyway, that sort of unresolved state is bad, and is the source of all
> of this song and dance. I think the right answer is to force handlers to
> open their own calls to printk, and to keep whats going on with the
> console_loglevel and printk buffer nice and clean.
> 
> The cost is that messages like this:
> 
> SysRq : Loglevel switched to X
> 
> will have to become more like this:
> 
> SysRq : Loglevel
> Loglevel switched to X
> 
> 
> Again, appologies, and a patch is forthcoming.

And here is the patch.

-- 
Crutcher        <crutcher@datastacks.com>
GCS d--- s+:>+:- a-- C++++$ UL++++$ L+++$>++++ !E PS+++ PE Y+ PGP+>++++
    R-(+++) !tv(+++) b+(++++) G+ e>++++ h+>++ r* y+>*$

[-- Attachment #2: patch-2.4.10-pre13-sysrq_log_level --]
[-- Type: text/plain, Size: 3559 bytes --]

--- linux-2.4.10-pre13/drivers/char/sysrq.c.sysrq_log_level	Fri Sep 21 17:25:45 2001
+++ linux-2.4.10-pre13/drivers/char/sysrq.c	Fri Sep 21 17:25:55 2001
@@ -47,13 +47,13 @@
 	int i;
 	i = key - '0';
 	console_loglevel = 7;
-	printk("%d\n", i);
+	printk("Loglevel set to %d\n", i);
 	console_loglevel = i;
 }	
 static struct sysrq_key_op sysrq_loglevel_op = {
 	handler:	sysrq_handle_loglevel,
 	help_msg:	"loglevel0-8",
-	action_msg:	"Loglevel set to ",
+	action_msg:	"Changing Loglevel",
 };
 
 
@@ -68,7 +68,7 @@
 static struct sysrq_key_op sysrq_SAK_op = {
 	handler:	sysrq_handle_SAK,
 	help_msg:	"saK",
-	action_msg:	"SAK\n",
+	action_msg:	"SAK",
 };
 #endif
 
@@ -82,7 +82,7 @@
 static struct sysrq_key_op sysrq_unraw_op = {
 	handler:	sysrq_handle_unraw,
 	help_msg:	"unRaw",
-	action_msg:	"Keyboard mode set to XLATE\n",
+	action_msg:	"Keyboard mode set to XLATE",
 };
 
 
@@ -94,7 +94,7 @@
 static struct sysrq_key_op sysrq_reboot_op = {
 	handler:	sysrq_handle_reboot,
 	help_msg:	"reBoot",
-	action_msg:	"Resetting\n",
+	action_msg:	"Resetting",
 };
 
 
@@ -225,7 +225,7 @@
 static struct sysrq_key_op sysrq_sync_op = {
 	handler:	sysrq_handle_sync,
 	help_msg:	"Sync",
-	action_msg:	"Emergency Sync\n",
+	action_msg:	"Emergency Sync",
 };
 
 static void sysrq_handle_mountro(int key, struct pt_regs *pt_regs,
@@ -236,7 +236,7 @@
 static struct sysrq_key_op sysrq_mountro_op = {
 	handler:	sysrq_handle_mountro,
 	help_msg:	"Unmount",
-	action_msg:	"Emergency Remount R/0\n",
+	action_msg:	"Emergency Remount R/0",
 };
 
 /* END SYNC SYSRQ HANDLERS BLOCK */
@@ -252,7 +252,7 @@
 static struct sysrq_key_op sysrq_showregs_op = {
 	handler:	sysrq_handle_showregs,
 	help_msg:	"showPc",
-	action_msg:	"Show Regs\n",
+	action_msg:	"Show Regs",
 };
 
 
@@ -263,7 +263,7 @@
 static struct sysrq_key_op sysrq_showstate_op = {
 	handler:	sysrq_handle_showstate,
 	help_msg:	"showTasks",
-	action_msg:	"Show State\n",
+	action_msg:	"Show State",
 };
 
 
@@ -274,7 +274,7 @@
 static struct sysrq_key_op sysrq_showmem_op = {
 	handler:	sysrq_handle_showmem,
 	help_msg:	"showMem",
-	action_msg:	"Show Memory\n",
+	action_msg:	"Show Memory",
 };
 
 /* SHOW SYSRQ HANDLERS BLOCK */
@@ -307,7 +307,7 @@
 static struct sysrq_key_op sysrq_term_op = {
 	handler:	sysrq_handle_term,
 	help_msg:	"tErm",
-	action_msg:	"Terminate All Tasks\n",
+	action_msg:	"Terminate All Tasks",
 };
 
 static void sysrq_handle_kill(int key, struct pt_regs *pt_regs,
@@ -318,7 +318,7 @@
 static struct sysrq_key_op sysrq_kill_op = {
 	handler:	sysrq_handle_kill,
 	help_msg:	"kIll",
-	action_msg:	"Kill All Tasks\n",
+	action_msg:	"Kill All Tasks",
 };
 
 static void sysrq_handle_killall(int key, struct pt_regs *pt_regs,
@@ -329,7 +329,7 @@
 static struct sysrq_key_op sysrq_killall_op = {
 	handler:	sysrq_handle_killall,
 	help_msg:	"killalL",
-	action_msg:	"Kill All Tasks (even init)\n",
+	action_msg:	"Kill All Tasks (even init)",
 };
 
 /* END SIGNAL SYSRQ HANDLERS BLOCK */
@@ -462,8 +462,9 @@
 
         op_p = __sysrq_get_key_op(key);
         if (op_p) {
-                printk ("%s", op_p->action_msg);
-                op_p->handler(key, pt_regs, kbd, tty);
+		printk ("%s\n", op_p->action_msg);
+		console_loglevel = orig_log_level;
+		op_p->handler(key, pt_regs, kbd, tty);
 	} else {
 		printk("HELP : ");
 		/* Only print the help msg once per handler */
@@ -474,8 +475,8 @@
 				printk ("%s ", sysrq_key_table[i]->help_msg);
 		}
 		printk ("\n");
+		console_loglevel = orig_log_level;
 	}
-	console_loglevel = orig_log_level;
 }
 
 EXPORT_SYMBOL(handle_sysrq);

Re: [PATCH] Magic SysRq loglevel fix.

[Randy.Dunlap]

Crutcher Dunnavant wrote:
> 
> Attached is a fix for this part of the sysrq code.
> 
> ++ 21/09/01 17:08 -0400 - Crutcher Dunnavant:
> > ++ 19/09/01 08:56 -0700 - Randy.Dunlap:
> > > It always sets console_loglevel and then restores
> > > console_loglevel from orig_log_level, so Alt+SysRq+#
> > > handling is severely broken.
> > >
> > > If someone (Crutcher ?) wants to patch it, that's fine.
> > > If I patched it, I would just add a
> > >   next_loglevel = -1;
> > > at the beginning of __handle_sysrq_nolock() and then
> > > let the loglevel handler(s) set next_loglevel.
> > > If next_loglevel != -1 at the end of __handle_sysrq_nolock(),
> > > set console_loglevel to next_loglevel.
> >
> > I'm looking real close at this right now, and there are a couple of
> > problems, and a simple, but ugly solution.
> >
> > The entire reason that console_loglevel is touched _after_ the call to
> > the second level handler is actually for the loglevel handler's
> > printout. I was trying to minimize change in the display, but horked it.
> >
> > Here is the problem.
> >
> > SysRq events use action messages which get printed by the top level
> > handler before calling the second level handler, the call line is:
> >
> >         orig_log_level = console_loglevel;
> >         console_loglevel = 7;
> >         printk(KERN_INFO "SysRq : ");
> >
> >         op_p = __sysrq_get_key_op(key);
> >       ...
> >         printk ("%s", op_p->action_msg);
> >         op_p->handler(key, pt_regs, kbd, tty);
> >       ...
> >         console_loglevel = orig_log_level;
> >
> >
> > The killer here is the fact that the action message format string does
> > not carry a newline, allowing people to register strings which leave the
> > printk state open. The loglevel handler then fills in the loglevel, and
> > closes the printk state.

/me switches to a decent keyboard to test with.

No, the killer is that console_loglevel is restored from
orig_log_level after having been modified in the loglevel
handler.

> > There was a time when I thought that was a good idea.
> >
> > Go ahead, laugh.

Nah, I don't want to laugh at it.  More like cry at it.
It's set me back from other work by too many hours already.

> > Anyway, that sort of unresolved state is bad, and is the source of all
> > of this song and dance. I think the right answer is to force handlers to
> > open their own calls to printk, and to keep whats going on with the
> > console_loglevel and printk buffer nice and clean.
> >
> > The cost is that messages like this:
> >
> > SysRq : Loglevel switched to X
> >
> > will have to become more like this:
> >
> > SysRq : Loglevel
> > Loglevel switched to X

Yes, that's ugly and shouldn't be done.

I made an OK state machine (previous and next states) of it.
Your patch moves restoring console_loglevel to a place
that makes sense.

Bottom line:  I don't care which code goes into the sysrq.c,
but I hope that you (and others) learn to do some basic testing
before unleashing it.  I don't expect all Linux kernel code
to be thoroughly tested before it is added to a kernel,
especially for areas like VM and file systems.
But some basic level of testing should have been done on it,
and I can't tell that it was done.

There is still room for some more/small improvements here.  Nothing
earth-shattering.  For example, go_sync() and do_emergency_sync()
don't need to save console_loglevel or set it to 7 (they have both
already been done in __handle_sysrq_nolock()).
My patch eliminated this cruft.

~Randy

Re: [PATCH] Magic SysRq loglevel fix.

[Crutcher Dunnavant]

++ 21/09/01 18:25 -0700 - Randy.Dunlap:
> Crutcher Dunnavant wrote:
> > 
> > Attached is a fix for this part of the sysrq code.
> > 
> > ++ 21/09/01 17:08 -0400 - Crutcher Dunnavant:
> > > ++ 19/09/01 08:56 -0700 - Randy.Dunlap:
> > > > It always sets console_loglevel and then restores
> > > > console_loglevel from orig_log_level, so Alt+SysRq+#
> > > > handling is severely broken.
> > > >
> > > > If someone (Crutcher ?) wants to patch it, that's fine.
> > > > If I patched it, I would just add a
> > > >   next_loglevel = -1;
> > > > at the beginning of __handle_sysrq_nolock() and then
> > > > let the loglevel handler(s) set next_loglevel.
> > > > If next_loglevel != -1 at the end of __handle_sysrq_nolock(),
> > > > set console_loglevel to next_loglevel.
> > >
> > > I'm looking real close at this right now, and there are a couple of
> > > problems, and a simple, but ugly solution.
> > >
> > > The entire reason that console_loglevel is touched _after_ the call to
> > > the second level handler is actually for the loglevel handler's
> > > printout. I was trying to minimize change in the display, but horked it.
> > >
> > > Here is the problem.
> > >
> > > SysRq events use action messages which get printed by the top level
> > > handler before calling the second level handler, the call line is:
> > >
> > >         orig_log_level = console_loglevel;
> > >         console_loglevel = 7;
> > >         printk(KERN_INFO "SysRq : ");
> > >
> > >         op_p = __sysrq_get_key_op(key);
> > >       ...
> > >         printk ("%s", op_p->action_msg);
> > >         op_p->handler(key, pt_regs, kbd, tty);
> > >       ...
> > >         console_loglevel = orig_log_level;
> > >
> > >
> > > The killer here is the fact that the action message format string does
> > > not carry a newline, allowing people to register strings which leave the
> > > printk state open. The loglevel handler then fills in the loglevel, and
> > > closes the printk state.
> 
> /me switches to a decent keyboard to test with.
> 
> No, the killer is that console_loglevel is restored from
> orig_log_level after having been modified in the loglevel
> handler.

But the _reason_ is that the action message does not carry a new line.


> Bottom line:  I don't care which code goes into the sysrq.c,
> but I hope that you (and others) learn to do some basic testing
> before unleashing it.  I don't expect all Linux kernel code
> to be thoroughly tested before it is added to a kernel,
> especially for areas like VM and file systems.
> But some basic level of testing should have been done on it,
> and I can't tell that it was done.

A whole lot of basic testing was done. The locking and the tables and
the registration all work correctly. I screwed up the loglevel stuff,
and it took 6 months for anyone to notice. Some people think that the
registration functions should be valid to call even when
CONFIG_MAGIC_SYSRQ is not defined, but this is a style thing.

Waving your hands and saying "Why wasn't this tested!!!" over and over
is annoying, and doesn't accomplish anything.

> There is still room for some more/small improvements here.  Nothing
> earth-shattering.  For example, go_sync() and do_emergency_sync()
> don't need to save console_loglevel or set it to 7 (they have both
> already been done in __handle_sysrq_nolock()).
> My patch eliminated this cruft.

No. go_sync() and do_emergency_sync() should NOT make assumptions about
the nature of console_loglevel, they should behave as they have been.
The commingling of loglevel settings was wrong in the first place.

-- 
Crutcher        <crutcher@datastacks.com>
GCS d--- s+:>+:- a-- C++++$ UL++++$ L+++$>++++ !E PS+++ PE Y+ PGP+>++++
    R-(+++) !tv(+++) b+(++++) G+ e>++++ h+>++ r* y+>*$

Re: Magic SysRq +# in 2.4.9-ac/2.4.10-pre12

[Randy.Dunlap]

Crutcher Dunnavant wrote:
> 
> I'm looking real close at this right now, and there are a couple of
> problems, and a simple, but ugly solution.
> 
> The entire reason that console_loglevel is touched _after_ the call to
> the second level handler is actually for the loglevel handler's
> printout. I was trying to minimize change in the display, but horked it.
> 
> Here is the problem.
> 
> SysRq events use action messages which get printed by the top level
> handler before calling the second level handler, the call line is:
> 
>         orig_log_level = console_loglevel;
>         console_loglevel = 7;
>         printk(KERN_INFO "SysRq : ");
> 
>         op_p = __sysrq_get_key_op(key);
>         ...
>         printk ("%s", op_p->action_msg);
>         op_p->handler(key, pt_regs, kbd, tty);
>         ...
>         console_loglevel = orig_log_level;
> 
> The killer here is the fact that the action message format string does
> not carry a newline, allowing people to register strings which leave the
> printk state open. The loglevel handler then fills in the loglevel, and
> closes the printk state.
> 
> There was a time when I thought that was a good idea.
> 
> Go ahead, laugh.
> 
> Anyway, that sort of unresolved state is bad, and is the source of all
> of this song and dance. I think the right answer is to force handlers to
> open their own calls to printk, and to keep whats going on with the
> console_loglevel and printk buffer nice and clean.
> 
> The cost is that messages like this:
> 
> SysRq : Loglevel switched to X
> 
> will have to become more like this:
> 
> SysRq : Loglevel
> Loglevel switched to X
> 
> Again, appologies, and a patch is forthcoming.

I've posted a patch (and copied you on it).
It's in 2.4.9-ac13.
You are welcome to post patches anyway, of course.

~Randy

Re: Magic SysRq +# in 2.4.9-ac/2.4.10-pre12

[Crutcher Dunnavant]

++ 21/09/01 14:42 -0700 - Randy.Dunlap:
> Crutcher Dunnavant wrote:
> I've posted a patch (and copied you on it).
> It's in 2.4.9-ac13.
> You are welcome to post patches anyway, of course.

I've looked at that, and I really think that the commingling of the
loglevel settings is just wrong. The patch I just threw up is much
simpler, and does everthing needed.

-- 
Crutcher        <crutcher@datastacks.com>
GCS d--- s+:>+:- a-- C++++$ UL++++$ L+++$>++++ !E PS+++ PE Y+ PGP+>++++
    R-(+++) !tv(+++) b+(++++) G+ e>++++ h+>++ r* y+>*$