Re: [patch] Re: Magic Alt-SysRq change in 2.6.18-rc1

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Dmitry Torokhov wrote:
Hi,

On 7/12/06, Paulo Marques <[email protected]> wrote:

Ok, I've tested it this time and this new one works as expected. I can
use any of the sequences discussed and I can produce a SysRq every time.
Still, just pressing SysRq or any sequence that doesn't start with
"press Alt -> press SysRq" seems unaffected.

I like this, however:

+       if (keycode == KEY_SYSRQ) {
+               if (down) {
+                       if(sysrq_alt)
+                               sysrq_down = down;
+               } else {
+                       sysrq_down = 0;

Are you sure? This will set sysrq_down only if ALT has already been
pressed. If SysRq does not autorepeat and it is pressed first we won't
ever see sysrq_down. Am I missing something?

This was done on purpose, yes. My first thought was to allow any order, but this could cause strange complications like: if I just press PrintScreen when is it ok to forward that keypress before I press Alt? The Alt afterwards arrives to late to not have messed the user space application already.

Anyway, none of the sequences posted required this, and it is more intuitive to press Alt first anyway.

+
+       if (sysrq_down && sysrq_alt)
+               sysrq_active = 1;
+       else if (!sysrq_down && !sysrq_alt)
+               sysrq_active = 0;
+
+       if (keycode == KEY_SYSRQ && sysrq_active)
+               return;

What about alt? I think that "if (...) sysrq_active = 1;" statement
should go down, below handle_sysrq block.

It can not go down, or when you press Alt and then SysRq, the first SysRq down event will get through without returning.

alt is handled a little higher in that function outside the #IFDEF. This is because emulate_raw should work even if we don't support magic sysrq.

The logic here is pretty simple: if we press Alt and then SysRq we enter sysrq_active mode. We only come out of that mode when we release both keys. Releasing any one of them individually is fine.

Any KEY_SYSRQ repetitions or releases while on this mode are not processed any further.

Oops, I just realised that if I release Alt first and then SysRq, the SysRq release will get through because at that point we're already out of sysrq_active mode. This should be easy to fix, though. If this is a problem, I can send a new patch tomorrow (with some more comments in there, too).

+
+       if (sysrq_active && down && !rep) {
               handle_sysrq(kbd_sysrq_xlate[keycode], regs, tty);
               return;
       }

We also need to check if emulate_raw() needs to be adjusted...

I looked at that very quickly, to be honest, but couldn't understand it entirely. This code:

1087 if (keycode == KEY_SYSRQ && sysrq_alt) {
1088   put_queue(vc, 0x54 | up_flag);
1089   return 0;
1090 }

seems to be supposed to cancel the Alt "press" by sending a Alt "release" to handle the sequence press alt -> press sysrq without affecting the application. However, this is just a guess. I couldn't find out what that magic 0x54 meant or why this would be enough wether we pressed the left or the right alt...

Then there are other things that I don't understand there: I don't see any code to filter out the keys we press ('T','P', etc.) while using SysRq magic if we are in raw mode. emulate_raw will happily call put_queue on them before we have a chance to bail out.

Maybe we should just stop calling emulate_raw while sysrq_active is active. This way, after we press Alt + SysRq, every keypress would be processed as a magic sysrq and not handled by any other code until we release both keys.

Does this sound reasonable?

--
Paulo Marques - www.grupopie.com
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux