lkml.org 
[lkml]   [2006]   [Jul]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [patch] Re: Magic Alt-SysRq change in 2.6.18-rc1
Dmitry Torokhov wrote:
> Hi,
>
> On 7/12/06, Paulo Marques <pmarques@grupopie.com> 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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-07-13 00:25    [W:0.085 / U:0.364 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site