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

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

[Ned T. Crigler]

Hi,

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

-- 
Ned T. Crigler

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

[Christian Heusel]

[-- Attachment #1: Type: text/plain, Size: 967 bytes --]

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")

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Peter Seiderer]

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


> 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

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

[Ned T. Crigler]

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
> 

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

[Dmitry Torokhov]

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. 

Thanks.

-- 
Dmitry


diff --git a/drivers/input/input.c b/drivers/input/input.c
index 3c321671793f..3d2cc13e1f32 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -119,12 +119,12 @@ static void input_pass_values(struct input_dev *dev,
 
 	handle = rcu_dereference(dev->grab);
 	if (handle) {
-		count = handle->handler->events(handle, vals, count);
+		count = handle->handle_events(handle, vals, count);
 	} else {
 		list_for_each_entry_rcu(handle, &dev->h_list, d_node)
 			if (handle->open) {
-				count = handle->handler->events(handle, vals,
-								count);
+				count = handle->handle_events(handle, vals,
+							      count);
 				if (!count)
 					break;
 			}
@@ -2537,57 +2537,6 @@ static int input_handler_check_methods(const struct input_handler *handler)
 	return 0;
 }
 
-/*
- * An implementation of input_handler's events() method that simply
- * invokes handler->event() method for each event one by one.
- */
-static unsigned int input_handler_events_default(struct input_handle *handle,
-						 struct input_value *vals,
-						 unsigned int count)
-{
-	struct input_handler *handler = handle->handler;
-	struct input_value *v;
-
-	for (v = vals; v != vals + count; v++)
-		handler->event(handle, v->type, v->code, v->value);
-
-	return count;
-}
-
-/*
- * An implementation of input_handler's events() method that invokes
- * handler->filter() method for each event one by one and removes events
- * that were filtered out from the "vals" array.
- */
-static unsigned int input_handler_events_filter(struct input_handle *handle,
-						struct input_value *vals,
-						unsigned int count)
-{
-	struct input_handler *handler = handle->handler;
-	struct input_value *end = vals;
-	struct input_value *v;
-
-	for (v = vals; v != vals + count; v++) {
-		if (handler->filter(handle, v->type, v->code, v->value))
-			continue;
-		if (end != v)
-			*end = *v;
-		end++;
-	}
-
-	return end - vals;
-}
-
-/*
- * An implementation of input_handler's events() method that does nothing.
- */
-static unsigned int input_handler_events_null(struct input_handle *handle,
-					      struct input_value *vals,
-					      unsigned int count)
-{
-	return count;
-}
-
 /**
  * input_register_handler - register a new input handler
  * @handler: handler to be registered
@@ -2607,13 +2556,6 @@ int input_register_handler(struct input_handler *handler)
 
 	INIT_LIST_HEAD(&handler->h_list);
 
-	if (handler->filter)
-		handler->events = input_handler_events_filter;
-	else if (handler->event)
-		handler->events = input_handler_events_default;
-	else if (!handler->events)
-		handler->events = input_handler_events_null;
-
 	error = mutex_lock_interruptible(&input_mutex);
 	if (error)
 		return error;
@@ -2687,6 +2629,75 @@ int input_handler_for_each_handle(struct input_handler *handler, void *data,
 }
 EXPORT_SYMBOL(input_handler_for_each_handle);
 
+/*
+ * An implementation of input_handle's handle_events() method that simply
+ * invokes handler->event() method for each event one by one.
+ */
+static unsigned int input_handle_events_default(struct input_handle *handle,
+						struct input_value *vals,
+						unsigned int count)
+{
+	struct input_handler *handler = handle->handler;
+	struct input_value *v;
+
+	for (v = vals; v != vals + count; v++)
+		handler->event(handle, v->type, v->code, v->value);
+
+	return count;
+}
+
+/*
+ * An implementation of input_handle's handle_events() method that invokes
+ * handler->filter() method for each event one by one and removes events
+ * that were filtered out from the "vals" array.
+ */
+static unsigned int input_handle_events_filter(struct input_handle *handle,
+					       struct input_value *vals,
+					       unsigned int count)
+{
+	struct input_handler *handler = handle->handler;
+	struct input_value *end = vals;
+	struct input_value *v;
+
+	for (v = vals; v != vals + count; v++) {
+		if (handler->filter(handle, v->type, v->code, v->value))
+			continue;
+		if (end != v)
+			*end = *v;
+		end++;
+	}
+
+	return end - vals;
+}
+
+/*
+ * An implementation of input_handle's handle_events() method that does nothing.
+ */
+static unsigned int input_handle_events_null(struct input_handle *handle,
+					     struct input_value *vals,
+					     unsigned int count)
+{
+	return count;
+}
+
+/*
+ * Sets up appropriate handle->event_handler based on the input_handler
+ * associated with the handle.
+ */
+static void input_handle_setup_event_handler(struct input_handle *handle)
+{
+	struct input_handler *handler = handle->handler;
+
+	if (handler->filter)
+		handle->handle_events = input_handle_events_filter;
+	else if (handler->event)
+		handle->handle_events = input_handle_events_default;
+	else if (handler->events)
+		handle->handle_events = handler->events;
+	else
+		handle->handle_events = input_handle_events_null;
+}
+
 /**
  * input_register_handle - register a new input handle
  * @handle: handle to register
@@ -2704,6 +2715,7 @@ int input_register_handle(struct input_handle *handle)
 	struct input_dev *dev = handle->dev;
 	int error;
 
+	input_handle_setup_event_handler(handle);
 	/*
 	 * We take dev->mutex here to prevent race with
 	 * input_release_device().
diff --git a/include/linux/input.h b/include/linux/input.h
index 89a0be6ee0e2..cd866b020a01 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -339,12 +339,16 @@ struct input_handler {
  * @name: name given to the handle by handler that created it
  * @dev: input device the handle is attached to
  * @handler: handler that works with the device through this handle
+ * @handle_events: event sequence handler. It is set up by the input core
+ *	according to event handling method specified in the @handler. See
+ *	input_handle_setup_event_handler().
+ *	This method is being called by the input core with interrupts disabled
+ *	and dev->event_lock spinlock held and so it may not sleep.
  * @d_node: used to put the handle on device's list of attached handles
  * @h_node: used to put the handle on handler's list of handles from which
  *	it gets events
  */
 struct input_handle {
-
 	void *private;
 
 	int open;
@@ -353,6 +357,10 @@ struct input_handle {
 	struct input_dev *dev;
 	struct input_handler *handler;
 
+	unsigned int (*handle_events)(struct input_handle *handle,
+				      struct input_value *vals,
+				      unsigned int count);
+
 	struct list_head	d_node;
 	struct list_head	h_node;
 };

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

[Peter Seiderer]

Hello Dmitry, Ned, *,

On Sun, 27 Oct 2024 22:30:36 -0700, Dmitry Torokhov <dmitry.torokhov@gmail.com> 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.

Yes, seems reasonable (and works for the sysrq case), you can add my

	Tested-by: Peter Seiderer <ps.report@gmx.net>

Regards,
Peter


>
> Thanks.
>

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

[Ned T. Crigler]

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

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

[Christian Heusel]

[-- Attachment #1: Type: text/plain, Size: 1950 bytes --]

On 24/10/27 10:30PM, 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")
> > > >
> > > 
> 
> 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)
> > > <snip>

Thanks for the patch, are you looking to fix this in a more general way
or do you plan on just sending in the above patch? (or did this already
happen and I just missed it? :p)

Cheers,
Chris

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Dmitry Torokhov]

Hi Christian,

On Mon, Nov 04, 2024 at 01:06:40PM +0100, Christian Heusel wrote:
> On 24/10/27 10:30PM, 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")
> > > > >
> > > > 
> > 
> > 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)
> > > > <snip>
> 
> Thanks for the patch, are you looking to fix this in a more general way
> or do you plan on just sending in the above patch? (or did this already
> happen and I just missed it? :p)

I applied the patch that I proposed and Peter and Ned tested. I am not
sure about what you mean by fixing this in a more general way, could you
please elaborate?

Thanks.

-- 
Dmitry