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@xxxxxxxxx> 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@xxxxxxxxx>
--
Ned T. Crigler
