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.
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;
};
