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

[Paulo Marques]

Dmitry Torokhov wrote:
> Hi,
>
> On 7/12/06, Paulo Marques <pmarques@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/