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

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

Hi Dmitry,

On Sun, Oct 27, 2024 at 10:30:36PM -0700, Dmitry Torokhov wrote:
> Hi everyone,
> 
> On Sun, Oct 27, 2024 at 10:02:24AM -0700, Ned T. Crigler wrote:
> > 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 }
> 
> Yes, I did not consider that we might want to re-register the same input
> handler, thank you for alerting me about the regression.
> 
> > > 
> > > 
> > > 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.
> 
> Yes, indeed. I wonder if we can solve this by moving the derived event
> handler method into input_handle structure, like the patch below. 

Your patch fixes the sysrq regression, thanks for the fix!

Tested-by: Ned T. Crigler <crigler@gmail.com>

-- 
Ned T. Crigler