[PATCH 2.6.23] SysRq: print hotkey info while pressing undef key, try 2

[PATCH 2.6.23] SysRq: print hotkey info while pressing undef key, try 2

[Yi Yang]

Compared with last patch, this has a little change according to 
Andreas and Pavel's feedbacks.

For SysRq, we just can get hot key list from Documentation/sysrq.txt
, but in the most of cases, the user can't access it by hand on
using SysRq to debug, so it is better for SysRq to provide an online
help for the users.

SysRq has already provided a similiar help before this patch, but it
is not so clear that the user doesn't know what happened and what
he/she should do.

In addition, that funtion has a big loop with another big loop
embedded which is very inefficient, it is intended to skip some hot
key help info for such a function as "Changing Loglevel", just print
a help info for this, that is very unnecessary. In fact, the key '0'
 - '8' have different results the user should know.

This patch adds this online help function, it'll print the whole hot
key list and corresponding function descriptions, it can print the new
defined hot key without any changed needed.

The output is below on pressing an undefined hot key before this patch:

SysRq : HELP : loglevel0-8 reBoot Crashdump tErm Full kIll saK showMem
Nice powerOff showPc show-all-timers(Q) unRaw Sync showTasks Unmount
shoW-blocked-tasks

But after applying this patch, the output is:

SysRq : 
this hot key isn't defined.

SysRq Help Information:

Hot Key    Function Description
=======    ====================
   0       Changing Loglevel to this value
   1       Changing Loglevel to this value
   2       Changing Loglevel to this value
   3       Changing Loglevel to this value
   4       Changing Loglevel to this value
   5       Changing Loglevel to this value
   6       Changing Loglevel to this value
   7       Changing Loglevel to this value
   8       Changing Loglevel to this value
   9       Changing Loglevel to this value
   a       Not defined
   b       Resetting
   c       Trigger a crashdump
   d       Not defined
   e       Terminate All Tasks
   f       Manual OOM execution
   g       Not defined
   h       Not defined
   i       Kill All Tasks
   j       Not defined
   k       SAK
   l       Not defined
   m       Show Memory
   n       Nice All RT Tasks
   o       Power Off
   p       Show Regs
   q       Show Pending Timers
   r       Keyboard mode set to XLATE
   s       Emergency Sync
   t       Show State
   u       Emergency Remount R/O
   v       Not defined
   w       Show Blocked State
   x       Not defined
   y       Not defined
   z       Not defined

Signed-off-by: Yi Yang <yang.y.yi@gmail.com>
---
 sysrq.c |   38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)


--- a/drivers/char/sysrq.c	2007-10-16 18:32:58.000000000 +0800
+++ b/drivers/char/sysrq.c	2007-10-17 17:52:45.000000000 +0800
@@ -45,6 +45,16 @@ int __read_mostly __sysrq_enabled = 1;
 
 static int __read_mostly sysrq_always_enabled;
 
+/*
+ * Hot key table SysRq supports
+ */
+static char __read_mostly sysrq_hot_key_table[36] = {
+	'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
+	'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
+	'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't',
+	'u', 'v', 'w', 'x', 'y', 'z'
+};
+
 int sysrq_on(void)
 {
 	return __sysrq_enabled || sysrq_always_enabled;
@@ -81,7 +91,7 @@ static void sysrq_handle_loglevel(int ke
 static struct sysrq_key_op sysrq_loglevel_op = {
 	.handler	= sysrq_handle_loglevel,
 	.help_msg	= "loglevel0-8",
-	.action_msg	= "Changing Loglevel",
+	.action_msg	= "Changing Loglevel to this value",
 	.enable_mask	= SYSRQ_ENABLE_LOG,
 };
 
@@ -410,7 +420,7 @@ void __handle_sysrq(int key, struct tty_
 	spin_lock_irqsave(&sysrq_key_table_lock, flags);
 	orig_log_level = console_loglevel;
 	console_loglevel = 7;
-	printk(KERN_INFO "SysRq : ");
+	printk(KERN_INFO "SysRq : \n");
 
         op_p = __sysrq_get_key_op(key);
         if (op_p) {
@@ -426,18 +436,22 @@ void __handle_sysrq(int key, struct tty_
 			printk("This sysrq operation is disabled.\n");
 		}
 	} else {
-		printk("HELP : ");
-		/* Only print the help msg once per handler */
+		/*
+		 * Print SysRq hot key list
+		 */
+		printk(KERN_INFO "this hot key isn't defined.\n\n");
+		printk(KERN_INFO "SysRq Help Information:\n\n");
+		printk(KERN_INFO "Hot Key    Function Description\n");
+		printk(KERN_INFO "=======    ====================\n");
 		for (i = 0; i < ARRAY_SIZE(sysrq_key_table); i++) {
 			if (sysrq_key_table[i]) {
-				int j;
-
-				for (j = 0; sysrq_key_table[i] !=
-						sysrq_key_table[j]; j++)
-					;
-				if (j != i)
-					continue;
-				printk("%s ", sysrq_key_table[i]->help_msg);
+				printk(KERN_INFO "   %c       %s\n",
+					sysrq_hot_key_table[i],
+					sysrq_key_table[i]->action_msg);
+			} else {
+				printk(KERN_INFO "   %c       %s\n",
+					sysrq_hot_key_table[i],
+					"Not defined");
 			}
 		}
 		printk("\n");

Re: [PATCH 2.6.23] SysRq: print hotkey info while pressing undef key, try 2

[Frans Pop]

Yi Yang wrote:
> Hot Key    Function Description
> =======    ====================
>    0       Changing Loglevel to this value
>    1       Changing Loglevel to this value
>    2       Changing Loglevel to this value
>    3       Changing Loglevel to this value
>    4       Changing Loglevel to this value
>    5       Changing Loglevel to this value
>    6       Changing Loglevel to this value
>    7       Changing Loglevel to this value
>    8       Changing Loglevel to this value
>    9       Changing Loglevel to this value

Wouldn't "Change Loglevel to this value" be more consistent with other
descriptions?
Or maybe even "Change Kernel Loglevel to <value>"; the repeated "this
value" seems somewhat silly.

>    b       Resetting

This is not very descriptive. Maybe "Immediate System Reboot"?

>    c       Trigger a crashdump

Most descriptions are capitalized. So for consistency this should be
"Trigger a Crashdump". (Although my personal preference would be to drop
the capitalization for all descriptions.)

>    p       Show Regs

Should "Regs" be expanded to "Registers"?

>   r       Keyboard mode set to XLATE

For consistency: "Set Keyboard Mode to XLATE"

Cheers,
Frans Pop

Re: [PATCH 2.6.23] SysRq: print hotkey info while pressing undef key, try 2

[Valdis.Kletnieks]

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

On Wed, 17 Oct 2007 23:22:58 +0800, Yi Yang said:

> SysRq has already provided a similiar help before this patch, but it
> is not so clear that the user doesn't know what happened and what
> he/she should do.

The person is in one of two states:

1) He has been told "recreate the problem, hit alt-sysreq-cokebottle,
and send me the results".  He has a mission, and the only feedback he
needs is (a) that he hit cokebottle and not pepsibottle, and (b) the
resulting output.

2) He's already read the file in Documentation/ and just needs a reminder.
In this case, the fact it's only 2 or 3 lines and doesn't scroll other stuff
out of sight is more important.

> In addition, that funtion has a big loop with another big loop
> embedded which is very inefficient, it is intended to skip some hot

You're optimizing code that hopefully never gets executed, and even if
it does, you have the optimization *backwards*.  If you're worried about
the efficiency, trim it down to output 3 lines - do you realize how many
instructions it takes in the VGA and fb drivers to actually *output* all
these lines? (Seriously - I had a 1.6Ghz P4 laptop, where scrolling the
screen with vga=791 actually ran so slowly that it horqued up the timer
initialization code. *That* was a fun bug to figure out..)

> key help info for such a function as "Changing Loglevel", just print
> a help info for this, that is very unnecessary. In fact, the key '0'
>  - '8' have different results the user should know.

And ironically enough, you then output the same exact text for all levels.


> +static char __read_mostly sysrq_hot_key_table[36] = {
> +	'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
> +	'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
> +	'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't',
> +	'u', 'v', 'w', 'x', 'y', 'z'

The lists of 'Not defined' tends to scroll the screen away. The old code
instead focuses on listing the things you *can* do.  If I'm looking at the
help output, I don't care that 'g' is not defined.  I need to be reminded
that 'p' is 'showPc' and D is show-all-locks.

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH 2.6.23] SysRq: print hotkey info while pressing undef key, try 2

[Pavel Machek]

Hi!

> > SysRq has already provided a similiar help before this patch, but it
> > is not so clear that the user doesn't know what happened and what
> > he/she should do.
> 
> The person is in one of two states:
> 
> 1) He has been told "recreate the problem, hit alt-sysreq-cokebottle,
> and send me the results".  He has a mission, and the only feedback he
> needs is (a) that he hit cokebottle and not pepsibottle, and (b) the
> resulting output.
> 
> 2) He's already read the file in Documentation/ and just needs a reminder.
> In this case, the fact it's only 2 or 3 lines and doesn't scroll other stuff
> out of sight is more important.
> 
> > In addition, that funtion has a big loop with another big loop
> > embedded which is very inefficient, it is intended to skip some hot
> 
> You're optimizing code that hopefully never gets executed, and even if
> it does, you have the optimization *backwards*.  If you're worried
> about

Pretty much exact. Thanks for saying this so nicely.
									Pavel

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

Re: [PATCH 2.6.23] SysRq: print hotkey info while pressing undef key, try 2

[Yi Yang]

Valdis.Kletnieks@vt.edu 写道:
> On Wed, 17 Oct 2007 23:22:58 +0800, Yi Yang said:
>
>   
>> SysRq has already provided a similiar help before this patch, but it
>> is not so clear that the user doesn't know what happened and what
>> he/she should do.
>>     
>
> The person is in one of two states:
>
> 1) He has been told "recreate the problem, hit alt-sysreq-cokebottle,
> and send me the results".  He has a mission, and the only feedback he
> needs is (a) that he hit cokebottle and not pepsibottle, and (b) the
> resulting output.
>
> 2) He's already read the file in Documentation/ and just needs a reminder.
> In this case, the fact it's only 2 or 3 lines and doesn't scroll other stuff
> out of sight is more important.
>   
Screen scroll isn't a problem.

"SysRq : HELP : loglevel0-8 reBoot Crashdump tErm Full kIll saK showMem
Nice powerOff showPc show-all-timers(Q) unRaw Sync showTasks Unmount
shoW-blocked-tasks"

The above help information isn't very user-friendly.

>   
>> In addition, that funtion has a big loop with another big loop
>> embedded which is very inefficient, it is intended to skip some hot
>>     
>
> You're optimizing code that hopefully never gets executed, and even if
> it does, you have the optimization *backwards*.  If you're worried about
> the efficiency, trim it down to output 3 lines - do you realize how many
> instructions it takes in the VGA and fb drivers to actually *output* all
> these lines? (Seriously - I had a 1.6Ghz P4 laptop, where scrolling the
> screen with vga=791 actually ran so slowly that it horqued up the timer
> initialization code. *That* was a fun bug to figure out..)
>   
To remove a bad loop is just a plus fix. That loop is really inefficient.
>   
>> key help info for such a function as "Changing Loglevel", just print
>> a help info for this, that is very unnecessary. In fact, the key '0'
>>  - '8' have different results the user should know.
>>     
>
> And ironically enough, you then output the same exact text for all levels.
>   
Yes, only one line for them is better.
>
>   
>> +static char __read_mostly sysrq_hot_key_table[36] = {
>> +	'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
>> +	'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
>> +	'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't',
>> +	'u', 'v', 'w', 'x', 'y', 'z'
>>     
>
> The lists of 'Not defined' tends to scroll the screen away. The old code
> instead focuses on listing the things you *can* do.  If I'm looking at the
> help output, I don't care that 'g' is not defined.  I need to be reminded
> that 'p' is 'showPc' and D is show-all-locks.
>   
You're right, "Not defined" is meaningless for the common users. I'll 
submit a new revision
to fix your concerns.

Thank you very much.

Re: [PATCH 2.6.23] SysRq: print hotkey info while pressing undef key, try 2

[Yi Yang]

Frans Pop 写道:
> Yi Yang wrote:
>   
>> Hot Key    Function Description
>> =======    ====================
>>    0       Changing Loglevel to this value
>>    1       Changing Loglevel to this value
>>    2       Changing Loglevel to this value
>>    3       Changing Loglevel to this value
>>    4       Changing Loglevel to this value
>>    5       Changing Loglevel to this value
>>    6       Changing Loglevel to this value
>>    7       Changing Loglevel to this value
>>    8       Changing Loglevel to this value
>>    9       Changing Loglevel to this value
>>     
>
> Wouldn't "Change Loglevel to this value" be more consistent with other
> descriptions?
> Or maybe even "Change Kernel Loglevel to <value>"; the repeated "this
> value" seems somewhat silly.
>   
I think so, too, only one line should be enough.
>   
>>    b       Resetting
>>     
>
> This is not very descriptive. Maybe "Immediate System Reboot"?
>   
>   
>>    c       Trigger a crashdump
>>     
>
> Most descriptions are capitalized. So for consistency this should be
> "Trigger a Crashdump". (Although my personal preference would be to drop
> the capitalization for all descriptions.)
>   
>   
>>    p       Show Regs
>>     
>
> Should "Regs" be expanded to "Registers"?
>
>   
>>   r       Keyboard mode set to XLATE
>>     
>
> For consistency: "Set Keyboard Mode to XLATE"
>
> Cheers,
> Frans Pop
>
>   
These descriptions are from the callers who are from many subsystems, so
I can't change it.

Re: [PATCH 2.6.23] SysRq: print hotkey info while pressing undef key, try 2

[Pavel Machek]

Hi!

>> 1) He has been told "recreate the problem, hit alt-sysreq-cokebottle,
>> and send me the results".  He has a mission, and the only feedback he
>> needs is (a) that he hit cokebottle and not pepsibottle, and (b) the
>> resulting output.
>>
>> 2) He's already read the file in Documentation/ and just needs a reminder.
>> In this case, the fact it's only 2 or 3 lines and doesn't scroll other 
>> stuff
>> out of sight is more important.
>>   
> Screen scroll isn't a problem.
>
> "SysRq : HELP : loglevel0-8 reBoot Crashdump tErm Full kIll saK showMem
> Nice powerOff showPc show-all-timers(Q) unRaw Sync showTasks Unmount
> shoW-blocked-tasks"
>
> The above help information isn't very user-friendly.

Seems perfectly reasonably to me... parhaps "Full" should be replaced
with "Force-oom"...

>>> In addition, that funtion has a big loop with another big loop
>>> embedded which is very inefficient, it is intended to skip some hot
>>>     
>>
>> You're optimizing code that hopefully never gets executed, and even if
>> it does, you have the optimization *backwards*.  If you're worried about
>> the efficiency, trim it down to output 3 lines - do you realize how many
>> instructions it takes in the VGA and fb drivers to actually *output* all
>> these lines? (Seriously - I had a 1.6Ghz P4 laptop, where scrolling the
>> screen with vga=791 actually ran so slowly that it horqued up the timer
>> initialization code. *That* was a fun bug to figure out..)
>>   
> To remove a bad loop is just a plus fix. That loop is really
> inefficient.

No, it is not a fix and no, it is not inefficient.

>>> +static char __read_mostly sysrq_hot_key_table[36] = {
>>> +	'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
>>> +	'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
>>> +	'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't',
>>> +	'u', 'v', 'w', 'x', 'y', 'z'

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

Re: [PATCH 2.6.23] SysRq: print hotkey info while pressing undef key, try 2

[Crutcher Dunnavant]

Crutcher Dunnavant

On Oct 18, 2007, at 10:10 AM, Pavel Machek <pavel@ucw.cz> wrote:

> Hi!
>
>>> 1) He has been told "recreate the problem, hit alt-sysreq- 
>>> cokebottle,
>>> and send me the results".  He has a mission, and the only feedback  
>>> he
>>> needs is (a) that he hit cokebottle and not pepsibottle, and (b) the
>>> resulting output.
>>>
>>> 2) He's already read the file in Documentation/ and just needs a  
>>> reminder.
>>> In this case, the fact it's only 2 or 3 lines and doesn't scroll  
>>> other
>>> stuff
>>> out of sight is more important.
>>>
>> Screen scroll isn't a problem.
>>
>> "SysRq : HELP : loglevel0-8 reBoot Crashdump tErm Full kIll saK  
>> showMem
>> Nice powerOff showPc show-all-timers(Q) unRaw Sync showTasks Unmount
>> shoW-blocked-tasks"
>>
>> The above help information isn't very user-friendly.
>
> Seems perfectly reasonably to me... parhaps "Full" should be replaced
> with "Force-oom"...
>
>>>> In addition, that funtion has a big loop with another big loop
>>>> embedded which is very inefficient, it is intended to skip some hot
>>>>
>>>
>>> You're optimizing code that hopefully never gets executed, and  
>>> even if
>>> it does, you have the optimization *backwards*.  If you're worried  
>>> about
>>> the efficiency, trim it down to output 3 lines - do you realize  
>>> how many
>>> instructions it takes in the VGA and fb drivers to actually  
>>> *output* all
>>> these lines? (Seriously - I had a 1.6Ghz P4 laptop, where  
>>> scrolling the
>>> screen with vga=791 actually ran so slowly that it horqued up the  
>>> timer
>>> initialization code. *That* was a fun bug to figure out..)
>>>
>> To remove a bad loop is just a plus fix. That loop is really
>> inefficient.
>
> No, it is not a fix and no, it is not inefficient.
>
>>>> +static char __read_mostly sysrq_hot_key_table[36] = {
>>>> +    '0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
>>>> +    'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
>>>> +    'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't',
>>>> +    'u', 'v', 'w', 'x', 'y', 'z'
>
> This has to go.
>                                    Pavel

Wow. I've not looked at this in a long time.
A. This imp of sysrq was designed to be run time registerable by  
modules.
B. Some keys aren't valid on some arches.
C. If you care about the lookup time for one table scan, sysrq can't  
help you anyway.
>
> -- d
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html