| Message ID | 20240701060553.869989-2-dmitry.torokhov@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Simplify event handling logic in input core | expand |
Hi Dmitry, On Jun 30 2024, Dmitry Torokhov wrote: > Input core expects input handlers to be either filters, or regular > handlers, but not both. Additionally, for regular handlers it does > not make sense to define both single event method and batch method. > > Refuse registering handler if it defines more than one method. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/input/input.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/drivers/input/input.c b/drivers/input/input.c > index fd4997ba263c..8434348faeac 100644 > --- a/drivers/input/input.c > +++ b/drivers/input/input.c > @@ -2517,6 +2517,26 @@ void input_unregister_device(struct input_dev *dev) > } > EXPORT_SYMBOL(input_unregister_device); > > +static int input_handler_check_methods(const struct input_handler *handler) > +{ > + int count = 0; > + > + if (handler->filter) > + count++; > + if (handler->events) > + count++; > + if (handler->event) > + count++; > + > + if (count != 1) { Am I missing some upstream commit? I have the following: in drivers/input/evdev.c: static struct input_handler evdev_handler = { .event = evdev_event, .events = evdev_events, .connect = evdev_connect, .disconnect = evdev_disconnect, .legacy_minors = true, .minor = EVDEV_MINOR_BASE, .name = "evdev", .id_table = evdev_ids, }; So here count should be 2 and evdev would be rejected? And in drivers/tty/serial/kgdboc.c: static struct input_handler kgdboc_reset_handler = { .connect = kgdboc_reset_connect, .disconnect = kgdboc_reset_disconnect, .name = "kgdboc_reset", .id_table = kgdboc_reset_ids, }; here count would be 0 and kgdboc would also be rejected. I agree on the intent of the patch, but these couple of input handlers should be fixed if they are not already. Cheers, Benjamin > + pr_err("%s: only one event processing method should be defined (%s)\n", > + __func__, handler->name); > + return -EINVAL; > + } > + > + return 0; > +} > + > /** > * input_register_handler - register a new input handler > * @handler: handler to be registered > @@ -2530,6 +2550,10 @@ int input_register_handler(struct input_handler *handler) > struct input_dev *dev; > int error; > > + error = input_handler_check_methods(handler); > + if (error) > + return error; > + > error = mutex_lock_interruptible(&input_mutex); > if (error) > return error; > -- > 2.45.2.803.g4e1b14247a-goog >
Hi Benjamin, On Mon, Jul 01, 2024 at 09:36:08AM +0200, Benjamin Tissoires wrote: > Hi Dmitry, > > On Jun 30 2024, Dmitry Torokhov wrote: > > Input core expects input handlers to be either filters, or regular > > handlers, but not both. Additionally, for regular handlers it does > > not make sense to define both single event method and batch method. > > > > Refuse registering handler if it defines more than one method. > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > drivers/input/input.c | 24 ++++++++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/drivers/input/input.c b/drivers/input/input.c > > index fd4997ba263c..8434348faeac 100644 > > --- a/drivers/input/input.c > > +++ b/drivers/input/input.c > > @@ -2517,6 +2517,26 @@ void input_unregister_device(struct input_dev *dev) > > } > > EXPORT_SYMBOL(input_unregister_device); > > > > +static int input_handler_check_methods(const struct input_handler *handler) > > +{ > > + int count = 0; > > + > > + if (handler->filter) > > + count++; > > + if (handler->events) > > + count++; > > + if (handler->event) > > + count++; > > + > > + if (count != 1) { > > Am I missing some upstream commit? I have the following: Thank you for the thorough review! > > in drivers/input/evdev.c: > > static struct input_handler evdev_handler = { > .event = evdev_event, > .events = evdev_events, > .connect = evdev_connect, > .disconnect = evdev_disconnect, > .legacy_minors = true, > .minor = EVDEV_MINOR_BASE, > .name = "evdev", > .id_table = evdev_ids, > }; > > So here count should be 2 and evdev would be rejected? Uh, it looks like I had a patch buried that removed evdev_event as not needed - if a handler has ->events() then input core would favor it even before my patches. > > And in drivers/tty/serial/kgdboc.c: > > static struct input_handler kgdboc_reset_handler = { > .connect = kgdboc_reset_connect, > .disconnect = kgdboc_reset_disconnect, > .name = "kgdboc_reset", > .id_table = kgdboc_reset_ids, > }; > > here count would be 0 and kgdboc would also be rejected. Yes, you are totally right. It looks like we need to allow the "no methods" case. > > I agree on the intent of the patch, but these couple of input handlers > should be fixed if they are not already. Yep, I will address your other comments and resend in a few. Thanks again!
diff --git a/drivers/input/input.c b/drivers/input/input.c index fd4997ba263c..8434348faeac 100644 --- a/drivers/input/input.c +++ b/drivers/input/input.c @@ -2517,6 +2517,26 @@ void input_unregister_device(struct input_dev *dev) } EXPORT_SYMBOL(input_unregister_device); +static int input_handler_check_methods(const struct input_handler *handler) +{ + int count = 0; + + if (handler->filter) + count++; + if (handler->events) + count++; + if (handler->event) + count++; + + if (count != 1) { + pr_err("%s: only one event processing method should be defined (%s)\n", + __func__, handler->name); + return -EINVAL; + } + + return 0; +} + /** * input_register_handler - register a new input handler * @handler: handler to be registered @@ -2530,6 +2550,10 @@ int input_register_handler(struct input_handler *handler) struct input_dev *dev; int error; + error = input_handler_check_methods(handler); + if (error) + return error; + error = mutex_lock_interruptible(&input_mutex); if (error) return error;
Input core expects input handlers to be either filters, or regular handlers, but not both. Additionally, for regular handlers it does not make sense to define both single event method and batch method. Refuse registering handler if it defines more than one method. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/input/input.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)