Re: [REGRESSION] disabling and re-enabling magic sysrq fails after kernel 6.11

["Ned T. Crigler" <crigler@gmail.com>]

Hi Peter, Christian,

On Sun, Oct 27, 2024 at 04:37:44PM +0100, Peter Seiderer wrote:
> Hello Ned, Christian, *,
> 
> On Sun, 27 Oct 2024 15:06:09 +0100, Christian Heusel <christian@heusel.eu> wrote:
> 
> > On 24/10/26 07:15PM, Ned T. Crigler wrote:
> > > Hi,
> >
> > Hey Ned,
> >
> > > It looks like starting with kernel 6.11, disabling and re-enabling
> > > magic
> > > sysrq fails with these errors in dmesg:
> > >
> > > kernel: input: input_handler_check_methods: only one event processing
> > > method can be defined (sysrq)
> > > kernel: sysrq: Failed to register input handler, error -22
> > >
> > > after doing:
> > >
> > > # echo 0 > /proc/sys/kernel/sysrq
> > > # echo 438 > /proc/sys/kernel/sysrq
> > > # echo 0 > /proc/sys/kernel/sysrq
> > > # echo 438 > /proc/sys/kernel/sysrq
> > > # echo 0 > /proc/sys/kernel/sysrq
> > > # echo 438 > /proc/sys/kernel/sysrq
> >
> > I have found that this issue is also present in the latest mainline
> > release and bisected it to the following commit:
> >
> >     d469647bafd9 ("Input: simplify event handling logic")
> >
> 
> After the mentioned commit a call sysrq_register_handler() -->
> input_register_handler(&sysrq_handler) with sysrq_handler.filter set
> will result in sysrq_handler.events set to input_handler_events_filter,
> see drivers/input/input.c (line 2607 to 2608):
> 
> 2596 int input_register_handler(struct input_handler *handler)
> 2597 {
> 2598         struct input_dev *dev;
> 2599         int error;
> 2600
> 2601         error = input_handler_check_methods(handler);
> 2602         if (error)
> 2603                 return error;
> 2604
> 2605         INIT_LIST_HEAD(&handler->h_list);
> 2606
> 2607         if (handler->filter)
> 2608                 handler->events = input_handler_events_filter;
> 2609         else if (handler->event)
> 2610                 handler->events = input_handler_events_default;
> 2611         else if (!handler->events)
> 2612                 handler->events = input_handler_events_null;
> 
> So the second call will fail at the check 'input_handler_check_methods(handler)'
> which only allows one method to be set, see drivers/input/input.c:
> 
> 2517 static int input_handler_check_methods(const struct input_handler *handler)
> 2518 {
> 2519         int count = 0;
> 2520
> 2521         if (handler->filter)
> 2522                 count++;
> 2523         if (handler->events)
> 2524                 count++;
> 2525         if (handler->event)
> 2526                 count++;
> 2527
> 2528         if (count > 1) {
> 2529                 pr_err("%s: only one event processing method can be defined      (%s)\n",
> 2530                        __func__, handler->name);
> 2531                 return -EINVAL;
> 2532         }
> 2533
> 2534         return 0;
> 2535 }
> 
> 
> A quick fix/hack for the sysrq case:
> 
> --- a/drivers/tty/sysrq.c
> +++ b/drivers/tty/sysrq.c
> @@ -1045,7 +1045,7 @@ static inline void sysrq_register_handler(void)
>         int error;
> 
>         sysrq_of_get_keyreset_config();
> -
> +       sysrq_handler.events = NULL;
>         error = input_register_handler(&sysrq_handler);
>         if (error)
>                 pr_err("Failed to register input handler, error %d", error);
> lines 1-13/13 (END)
> 
> Regards,
> Peter
> 

Thanks for tracking this down. It seems messy that the mentioned commit
changes input_register_handler to overwrite ->events for an internal purpose,
and callers may expect it to be unchanged, as sysrq does here by reusing
sysrq_handler.

-- 
Ned T. Crigler

> 
> > The additional authors / maintainers have been added to the recipients
> > lists.
> >
> > I hope I didn't overlook any pending fixes.
> >
> > > --
> > > Ned T. Crigler
> >
> > Cheers,
> > Chris
>