[PATCH v2 0/7] Simplify event handling logic in input core

[PATCH v2 0/7] Simplify event handling logic in input core

[Dmitry Torokhov]

This series tries to untangle somewhat convoluted logic in the input
event processing in the input core by realizing that input handler can
be either a filter, or a handler that handles a single event at a time,
or a handler that can handle a sequence of events, but should not mix
the 3 behaviors in one handler. This allows us to reduce both filter
functionality and single-event handling functionality to batch handling
and have the main event handling path to only deal with
input_handle->events() batch method.

v2: addressed comments from Benjamin:

- added missing patch to remove evdev->event() implementation
- allow not specifying any event handling method to satisfy kgdboc
  handler
- expanded comment on order of running input handlers when passing
  events
- split pre-allocation into 2 patches and moved removal of count check
  into 3rd patch.

Dmitry Torokhov (7):
  Input: evdev - remove ->event() method
  Input: make sure input handlers define only one processing method
  Input: make events() method return number of events processed
  Input: simplify event handling logic
  Input: rearrange input_alloc_device() to prepare for preallocating of vals
  Input: preallocate memory to hold event values
  Input: do not check number of events in input_pass_values()

 drivers/input/evdev.c |  16 +--
 drivers/input/input.c | 230 ++++++++++++++++++++++++++++--------------
 include/linux/input.h |   7 +-
 3 files changed, 163 insertions(+), 90 deletions(-)

-- 
2.45.2.803.g4e1b14247a-goog

[PATCH v2 1/7] Input: evdev - remove ->event() method

[Dmitry Torokhov]

Input core favors ->events() (batch) method over ->event() method
if the former is defined, so there is no point in defining evdev_event()
as it is never called. Remove it.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/evdev.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 51e0c4954600..05abcd45b5d4 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -308,17 +308,6 @@ static void evdev_events(struct input_handle *handle,
 	rcu_read_unlock();
 }
 
-/*
- * Pass incoming event to all connected clients.
- */
-static void evdev_event(struct input_handle *handle,
-			unsigned int type, unsigned int code, int value)
-{
-	struct input_value vals[] = { { type, code, value } };
-
-	evdev_events(handle, vals, 1);
-}
-
 static int evdev_fasync(int fd, struct file *file, int on)
 {
 	struct evdev_client *client = file->private_data;
@@ -1418,7 +1407,6 @@ static const struct input_device_id evdev_ids[] = {
 MODULE_DEVICE_TABLE(input, evdev_ids);
 
 static struct input_handler evdev_handler = {
-	.event		= evdev_event,
 	.events		= evdev_events,
 	.connect	= evdev_connect,
 	.disconnect	= evdev_disconnect,
-- 
2.45.2.803.g4e1b14247a-goog

[PATCH v2 2/7] Input: make sure input handlers define only one processing method

[Dmitry Torokhov]

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..7e4f8824f4fd 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 can 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

[PATCH v2 3/7] Input: make events() method return number of events processed

[Dmitry Torokhov]

In preparation to consolidating filtering and event processing in the
input core change events() method to return number of events processed
by it.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/evdev.c | 6 ++++--
 include/linux/input.h | 7 ++++---
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 05abcd45b5d4..a8ce3d140722 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -288,8 +288,8 @@ static void evdev_pass_values(struct evdev_client *client,
 /*
  * Pass incoming events to all connected clients.
  */
-static void evdev_events(struct input_handle *handle,
-			 const struct input_value *vals, unsigned int count)
+static unsigned int evdev_events(struct input_handle *handle,
+				 struct input_value *vals, unsigned int count)
 {
 	struct evdev *evdev = handle->private;
 	struct evdev_client *client;
@@ -306,6 +306,8 @@ static void evdev_events(struct input_handle *handle,
 			evdev_pass_values(client, vals, count, ev_time);
 
 	rcu_read_unlock();
+
+	return count;
 }
 
 static int evdev_fasync(int fd, struct file *file, int on)
diff --git a/include/linux/input.h b/include/linux/input.h
index c22ac465254b..89a0be6ee0e2 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -275,7 +275,8 @@ struct input_handle;
  *	it may not sleep
  * @events: event sequence handler. This method is being called by
  *	input core with interrupts disabled and dev->event_lock
- *	spinlock held and so it may not sleep
+ *	spinlock held and so it may not sleep. The method must return
+ *	number of events passed to it.
  * @filter: similar to @event; separates normal event handlers from
  *	"filters".
  * @match: called after comparing device's id with handler's id_table
@@ -312,8 +313,8 @@ struct input_handler {
 	void *private;
 
 	void (*event)(struct input_handle *handle, unsigned int type, unsigned int code, int value);
-	void (*events)(struct input_handle *handle,
-		       const struct input_value *vals, unsigned int count);
+	unsigned int (*events)(struct input_handle *handle,
+			       struct input_value *vals, unsigned int count);
 	bool (*filter)(struct input_handle *handle, unsigned int type, unsigned int code, int value);
 	bool (*match)(struct input_handler *handler, struct input_dev *dev);
 	int (*connect)(struct input_handler *handler, struct input_dev *dev, const struct input_device_id *id);
-- 
2.45.2.803.g4e1b14247a-goog

[PATCH v2 4/7] Input: simplify event handling logic

[Dmitry Torokhov]

Streamline event handling code by providing batch implementations for
filtering and event processing and using them in place of the main
event handler, as needed, instead of having complex branching logic
in the middle of the event processing code.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/input.c | 109 ++++++++++++++++++++++++++----------------
 1 file changed, 68 insertions(+), 41 deletions(-)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 7e4f8824f4fd..40a04154f99d 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -99,45 +99,13 @@ static void input_stop_autorepeat(struct input_dev *dev)
 	del_timer(&dev->timer);
 }
 
-/*
- * Pass event first through all filters and then, if event has not been
- * filtered out, through all open handles. This function is called with
- * dev->event_lock held and interrupts disabled.
- */
-static unsigned int input_to_handler(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;
-
-	if (handler->filter) {
-		for (v = vals; v != vals + count; v++) {
-			if (handler->filter(handle, v->type, v->code, v->value))
-				continue;
-			if (end != v)
-				*end = *v;
-			end++;
-		}
-		count = end - vals;
-	}
-
-	if (!count)
-		return 0;
-
-	if (handler->events)
-		handler->events(handle, vals, count);
-	else if (handler->event)
-		for (v = vals; v != vals + count; v++)
-			handler->event(handle, v->type, v->code, v->value);
-
-	return count;
-}
-
 /*
  * Pass values first through all filters and then, if event has not been
- * filtered out, through all open handles. This function is called with
- * dev->event_lock held and interrupts disabled.
+ * filtered out, through all open handles. This order is achieved by placing
+ * filters at the head of the list of handles attached to the device, and
+ * placing regular handles at the tail of the list.
+ *
+ * This function is called with dev->event_lock held and interrupts disabled.
  */
 static void input_pass_values(struct input_dev *dev,
 			      struct input_value *vals, unsigned int count)
@@ -154,11 +122,12 @@ static void input_pass_values(struct input_dev *dev,
 
 	handle = rcu_dereference(dev->grab);
 	if (handle) {
-		count = input_to_handler(handle, vals, count);
+		count = handle->handler->events(handle, vals, count);
 	} else {
 		list_for_each_entry_rcu(handle, &dev->h_list, d_node)
 			if (handle->open) {
-				count = input_to_handler(handle, vals, count);
+				count = handle->handler->events(handle, vals,
+								count);
 				if (!count)
 					break;
 			}
@@ -2537,6 +2506,57 @@ 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
@@ -2554,12 +2574,19 @@ int input_register_handler(struct input_handler *handler)
 	if (error)
 		return error;
 
+	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;
 
-	INIT_LIST_HEAD(&handler->h_list);
-
 	list_add_tail(&handler->node, &input_handler_list);
 
 	list_for_each_entry(dev, &input_dev_list, node)
-- 
2.45.2.803.g4e1b14247a-goog

[PATCH v2 5/7] Input: rearrange input_alloc_device() to prepare for preallocating of vals

[Dmitry Torokhov]

In preparation to have dev->vals memory pre-allocated rearrange
code in input_alloc_device() so that it allows handling multiple
points of failure.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/input.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 40a04154f99d..9981fdfaee9f 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -1982,21 +1982,28 @@ struct input_dev *input_allocate_device(void)
 	struct input_dev *dev;
 
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
-	if (dev) {
-		dev->dev.type = &input_dev_type;
-		dev->dev.class = &input_class;
-		device_initialize(&dev->dev);
-		mutex_init(&dev->mutex);
-		spin_lock_init(&dev->event_lock);
-		timer_setup(&dev->timer, NULL, 0);
-		INIT_LIST_HEAD(&dev->h_list);
-		INIT_LIST_HEAD(&dev->node);
-
-		dev_set_name(&dev->dev, "input%lu",
-			     (unsigned long)atomic_inc_return(&input_no));
-
-		__module_get(THIS_MODULE);
-	}
+	if (!dev)
+		return NULL;
+
+	mutex_init(&dev->mutex);
+	spin_lock_init(&dev->event_lock);
+	timer_setup(&dev->timer, NULL, 0);
+	INIT_LIST_HEAD(&dev->h_list);
+	INIT_LIST_HEAD(&dev->node);
+
+	dev->dev.type = &input_dev_type;
+	dev->dev.class = &input_class;
+	device_initialize(&dev->dev);
+	/*
+	 * From this point on we can no longer simply "kfree(dev)", we need
+	 * to use input_free_device() so that device core properly frees its
+	 * resources associated with the input device.
+	 */
+
+	dev_set_name(&dev->dev, "input%lu",
+		     (unsigned long)atomic_inc_return(&input_no));
+
+	__module_get(THIS_MODULE);
 
 	return dev;
 }
-- 
2.45.2.803.g4e1b14247a-goog

[PATCH v2 6/7] Input: preallocate memory to hold event values

[Dmitry Torokhov]

Preallocate memory for holding event values (input_dev->vals) so that
there is no need to check if it was allocated or not in the event
processing code.

The amount of memory will be adjusted after input device has been fully
set up upon registering device with the input core.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/input.c | 61 +++++++++++++++++++++++++++++++------------
 1 file changed, 44 insertions(+), 17 deletions(-)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 9981fdfaee9f..4e12fa79883e 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -323,9 +323,6 @@ static void input_event_dispose(struct input_dev *dev, int disposition,
 	if ((disposition & INPUT_PASS_TO_DEVICE) && dev->event)
 		dev->event(dev, type, code, value);
 
-	if (!dev->vals)
-		return;
-
 	if (disposition & INPUT_PASS_TO_HANDLERS) {
 		struct input_value *v;
 
@@ -1985,6 +1982,18 @@ struct input_dev *input_allocate_device(void)
 	if (!dev)
 		return NULL;
 
+	/*
+	 * Start with space for SYN_REPORT + 7 EV_KEY/EV_MSC events + 2 spare,
+	 * see input_estimate_events_per_packet(). We will tune the number
+	 * when we register the device.
+	 */
+	dev->max_vals = 10;
+	dev->vals = kcalloc(dev->max_vals, sizeof(*dev->vals), GFP_KERNEL);
+	if (!dev->vals) {
+		kfree(dev);
+		return NULL;
+	}
+
 	mutex_init(&dev->mutex);
 	spin_lock_init(&dev->event_lock);
 	timer_setup(&dev->timer, NULL, 0);
@@ -2344,6 +2353,35 @@ bool input_device_enabled(struct input_dev *dev)
 }
 EXPORT_SYMBOL_GPL(input_device_enabled);
 
+static int input_device_tune_vals(struct input_dev *dev)
+{
+	struct input_value *vals;
+	unsigned int packet_size;
+	unsigned int max_vals;
+
+	packet_size = input_estimate_events_per_packet(dev);
+	if (dev->hint_events_per_packet < packet_size)
+		dev->hint_events_per_packet = packet_size;
+
+	max_vals = dev->hint_events_per_packet + 2;
+	if (dev->max_vals >= max_vals)
+		return 0;
+
+	vals = kcalloc(max_vals, sizeof(*vals), GFP_KERNEL);
+	if (!vals)
+		return -ENOMEM;
+
+	spin_lock_irq(&dev->event_lock);
+	dev->max_vals = max_vals;
+	swap(dev->vals, vals);
+	spin_unlock_irq(&dev->event_lock);
+
+	/* Because of swap() above, this frees the old vals memory */
+	kfree(vals);
+
+	return 0;
+}
+
 /**
  * input_register_device - register device with input core
  * @dev: device to be registered
@@ -2371,7 +2409,6 @@ int input_register_device(struct input_dev *dev)
 {
 	struct input_devres *devres = NULL;
 	struct input_handler *handler;
-	unsigned int packet_size;
 	const char *path;
 	int error;
 
@@ -2399,16 +2436,9 @@ int input_register_device(struct input_dev *dev)
 	/* Make sure that bitmasks not mentioned in dev->evbit are clean. */
 	input_cleanse_bitmasks(dev);
 
-	packet_size = input_estimate_events_per_packet(dev);
-	if (dev->hint_events_per_packet < packet_size)
-		dev->hint_events_per_packet = packet_size;
-
-	dev->max_vals = dev->hint_events_per_packet + 2;
-	dev->vals = kcalloc(dev->max_vals, sizeof(*dev->vals), GFP_KERNEL);
-	if (!dev->vals) {
-		error = -ENOMEM;
+	error = input_device_tune_vals(dev);
+	if (error)
 		goto err_devres_free;
-	}
 
 	/*
 	 * If delay and period are pre-set by the driver, then autorepeating
@@ -2428,7 +2458,7 @@ int input_register_device(struct input_dev *dev)
 
 	error = device_add(&dev->dev);
 	if (error)
-		goto err_free_vals;
+		goto err_devres_free;
 
 	path = kobject_get_path(&dev->dev.kobj, GFP_KERNEL);
 	pr_info("%s as %s\n",
@@ -2458,9 +2488,6 @@ int input_register_device(struct input_dev *dev)
 
 err_device_del:
 	device_del(&dev->dev);
-err_free_vals:
-	kfree(dev->vals);
-	dev->vals = NULL;
 err_devres_free:
 	devres_free(devres);
 	return error;
-- 
2.45.2.803.g4e1b14247a-goog

[PATCH v2 7/7] Input: do not check number of events in input_pass_values()

[Dmitry Torokhov]

Now that the input_dev->vals array is always there we can be assured
that input_pass_values() is always called with a non-0 number of
events. Remove the check.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/input.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 4e12fa79883e..54c57b267b25 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -115,9 +115,6 @@ static void input_pass_values(struct input_dev *dev,
 
 	lockdep_assert_held(&dev->event_lock);
 
-	if (!count)
-		return;
-
 	rcu_read_lock();
 
 	handle = rcu_dereference(dev->grab);
-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH v2 0/7] Simplify event handling logic in input core

[Benjamin Tissoires]

On Jul 03 2024, Dmitry Torokhov wrote:
> This series tries to untangle somewhat convoluted logic in the input
> event processing in the input core by realizing that input handler can
> be either a filter, or a handler that handles a single event at a time,
> or a handler that can handle a sequence of events, but should not mix
> the 3 behaviors in one handler. This allows us to reduce both filter
> functionality and single-event handling functionality to batch handling
> and have the main event handling path to only deal with
> input_handle->events() batch method.
> 
> v2: addressed comments from Benjamin:
> 
> - added missing patch to remove evdev->event() implementation
> - allow not specifying any event handling method to satisfy kgdboc
>   handler
> - expanded comment on order of running input handlers when passing
>   events
> - split pre-allocation into 2 patches and moved removal of count check
>   into 3rd patch.

Thanks for the respin.

This series is:
Reviewed-by: Benjamin Tissoires <bentiss@kernel.org>

Cheers,
Benjamin


> 
> Dmitry Torokhov (7):
>   Input: evdev - remove ->event() method
>   Input: make sure input handlers define only one processing method
>   Input: make events() method return number of events processed
>   Input: simplify event handling logic
>   Input: rearrange input_alloc_device() to prepare for preallocating of vals
>   Input: preallocate memory to hold event values
>   Input: do not check number of events in input_pass_values()
> 
>  drivers/input/evdev.c |  16 +--
>  drivers/input/input.c | 230 ++++++++++++++++++++++++++++--------------
>  include/linux/input.h |   7 +-
>  3 files changed, 163 insertions(+), 90 deletions(-)
> 
> -- 
> 2.45.2.803.g4e1b14247a-goog
> 

Re: [PATCH v2 1/7] Input: evdev - remove ->event() method

[Jeff LaBundy]

Hi Dmitry,

On Wed, Jul 03, 2024 at 02:37:48PM -0700, Dmitry Torokhov wrote:
> Input core favors ->events() (batch) method over ->event() method
> if the former is defined, so there is no point in defining evdev_event()
> as it is never called. Remove it.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Jeff LaBundy <jeff@labundy.com>

> ---
>  drivers/input/evdev.c | 12 ------------
>  1 file changed, 12 deletions(-)
> 
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index 51e0c4954600..05abcd45b5d4 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -308,17 +308,6 @@ static void evdev_events(struct input_handle *handle,
>  	rcu_read_unlock();
>  }
>  
> -/*
> - * Pass incoming event to all connected clients.
> - */
> -static void evdev_event(struct input_handle *handle,
> -			unsigned int type, unsigned int code, int value)
> -{
> -	struct input_value vals[] = { { type, code, value } };
> -
> -	evdev_events(handle, vals, 1);
> -}
> -
>  static int evdev_fasync(int fd, struct file *file, int on)
>  {
>  	struct evdev_client *client = file->private_data;
> @@ -1418,7 +1407,6 @@ static const struct input_device_id evdev_ids[] = {
>  MODULE_DEVICE_TABLE(input, evdev_ids);
>  
>  static struct input_handler evdev_handler = {
> -	.event		= evdev_event,
>  	.events		= evdev_events,
>  	.connect	= evdev_connect,
>  	.disconnect	= evdev_disconnect,
> -- 
> 2.45.2.803.g4e1b14247a-goog
> 

Kind regards,
Jeff LaBundy

Re: [PATCH v2 2/7] Input: make sure input handlers define only one processing method

[Jeff LaBundy]

Hi Dmitry,

On Wed, Jul 03, 2024 at 02:37:49PM -0700, 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>

Reviewed-by: Jeff LaBundy <jeff@labundy.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..7e4f8824f4fd 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 can 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
> 

Kind regards,
Jeff LaBundy

Re: [PATCH v2 3/7] Input: make events() method return number of events processed

[Jeff LaBundy]

Hi Dmitry,

On Wed, Jul 03, 2024 at 02:37:50PM -0700, Dmitry Torokhov wrote:
> In preparation to consolidating filtering and event processing in the
> input core change events() method to return number of events processed
> by it.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Jeff LaBundy <jeff@labundy.com>

> ---
>  drivers/input/evdev.c | 6 ++++--
>  include/linux/input.h | 7 ++++---
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index 05abcd45b5d4..a8ce3d140722 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -288,8 +288,8 @@ static void evdev_pass_values(struct evdev_client *client,
>  /*
>   * Pass incoming events to all connected clients.
>   */
> -static void evdev_events(struct input_handle *handle,
> -			 const struct input_value *vals, unsigned int count)
> +static unsigned int evdev_events(struct input_handle *handle,
> +				 struct input_value *vals, unsigned int count)
>  {
>  	struct evdev *evdev = handle->private;
>  	struct evdev_client *client;
> @@ -306,6 +306,8 @@ static void evdev_events(struct input_handle *handle,
>  			evdev_pass_values(client, vals, count, ev_time);
>  
>  	rcu_read_unlock();
> +
> +	return count;
>  }
>  
>  static int evdev_fasync(int fd, struct file *file, int on)
> diff --git a/include/linux/input.h b/include/linux/input.h
> index c22ac465254b..89a0be6ee0e2 100644
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -275,7 +275,8 @@ struct input_handle;
>   *	it may not sleep
>   * @events: event sequence handler. This method is being called by
>   *	input core with interrupts disabled and dev->event_lock
> - *	spinlock held and so it may not sleep
> + *	spinlock held and so it may not sleep. The method must return
> + *	number of events passed to it.
>   * @filter: similar to @event; separates normal event handlers from
>   *	"filters".
>   * @match: called after comparing device's id with handler's id_table
> @@ -312,8 +313,8 @@ struct input_handler {
>  	void *private;
>  
>  	void (*event)(struct input_handle *handle, unsigned int type, unsigned int code, int value);
> -	void (*events)(struct input_handle *handle,
> -		       const struct input_value *vals, unsigned int count);
> +	unsigned int (*events)(struct input_handle *handle,
> +			       struct input_value *vals, unsigned int count);
>  	bool (*filter)(struct input_handle *handle, unsigned int type, unsigned int code, int value);
>  	bool (*match)(struct input_handler *handler, struct input_dev *dev);
>  	int (*connect)(struct input_handler *handler, struct input_dev *dev, const struct input_device_id *id);
> -- 
> 2.45.2.803.g4e1b14247a-goog
> 

Kind regards,
Jeff LaBundy

Re: [PATCH v2 4/7] Input: simplify event handling logic

[Jeff LaBundy]

Hi Dmitry,

On Wed, Jul 03, 2024 at 02:37:51PM -0700, Dmitry Torokhov wrote:
> Streamline event handling code by providing batch implementations for
> filtering and event processing and using them in place of the main
> event handler, as needed, instead of having complex branching logic
> in the middle of the event processing code.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Jeff LaBundy <jeff@labundy.com>

> ---
>  drivers/input/input.c | 109 ++++++++++++++++++++++++++----------------
>  1 file changed, 68 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 7e4f8824f4fd..40a04154f99d 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -99,45 +99,13 @@ static void input_stop_autorepeat(struct input_dev *dev)
>  	del_timer(&dev->timer);
>  }
>  
> -/*
> - * Pass event first through all filters and then, if event has not been
> - * filtered out, through all open handles. This function is called with
> - * dev->event_lock held and interrupts disabled.
> - */
> -static unsigned int input_to_handler(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;
> -
> -	if (handler->filter) {
> -		for (v = vals; v != vals + count; v++) {
> -			if (handler->filter(handle, v->type, v->code, v->value))
> -				continue;
> -			if (end != v)
> -				*end = *v;
> -			end++;
> -		}
> -		count = end - vals;
> -	}
> -
> -	if (!count)
> -		return 0;
> -
> -	if (handler->events)
> -		handler->events(handle, vals, count);
> -	else if (handler->event)
> -		for (v = vals; v != vals + count; v++)
> -			handler->event(handle, v->type, v->code, v->value);
> -
> -	return count;
> -}
> -
>  /*
>   * Pass values first through all filters and then, if event has not been
> - * filtered out, through all open handles. This function is called with
> - * dev->event_lock held and interrupts disabled.
> + * filtered out, through all open handles. This order is achieved by placing
> + * filters at the head of the list of handles attached to the device, and
> + * placing regular handles at the tail of the list.
> + *
> + * This function is called with dev->event_lock held and interrupts disabled.
>   */
>  static void input_pass_values(struct input_dev *dev,
>  			      struct input_value *vals, unsigned int count)
> @@ -154,11 +122,12 @@ static void input_pass_values(struct input_dev *dev,
>  
>  	handle = rcu_dereference(dev->grab);
>  	if (handle) {
> -		count = input_to_handler(handle, vals, count);
> +		count = handle->handler->events(handle, vals, count);
>  	} else {
>  		list_for_each_entry_rcu(handle, &dev->h_list, d_node)
>  			if (handle->open) {
> -				count = input_to_handler(handle, vals, count);
> +				count = handle->handler->events(handle, vals,
> +								count);
>  				if (!count)
>  					break;
>  			}
> @@ -2537,6 +2506,57 @@ 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
> @@ -2554,12 +2574,19 @@ int input_register_handler(struct input_handler *handler)
>  	if (error)
>  		return error;
>  
> +	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;
>  
> -	INIT_LIST_HEAD(&handler->h_list);
> -
>  	list_add_tail(&handler->node, &input_handler_list);
>  
>  	list_for_each_entry(dev, &input_dev_list, node)
> -- 
> 2.45.2.803.g4e1b14247a-goog
> 

Kind regards,
Jeff LaBundy

Re: [PATCH v2 5/7] Input: rearrange input_alloc_device() to prepare for preallocating of vals

[Jeff LaBundy]

Hi Dmitry,

On Wed, Jul 03, 2024 at 02:37:52PM -0700, Dmitry Torokhov wrote:
> In preparation to have dev->vals memory pre-allocated rearrange
> code in input_alloc_device() so that it allows handling multiple
> points of failure.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Jeff LaBundy <jeff@labundy.com>

> ---
>  drivers/input/input.c | 37 ++++++++++++++++++++++---------------
>  1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 40a04154f99d..9981fdfaee9f 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -1982,21 +1982,28 @@ struct input_dev *input_allocate_device(void)
>  	struct input_dev *dev;
>  
>  	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> -	if (dev) {
> -		dev->dev.type = &input_dev_type;
> -		dev->dev.class = &input_class;
> -		device_initialize(&dev->dev);
> -		mutex_init(&dev->mutex);
> -		spin_lock_init(&dev->event_lock);
> -		timer_setup(&dev->timer, NULL, 0);
> -		INIT_LIST_HEAD(&dev->h_list);
> -		INIT_LIST_HEAD(&dev->node);
> -
> -		dev_set_name(&dev->dev, "input%lu",
> -			     (unsigned long)atomic_inc_return(&input_no));
> -
> -		__module_get(THIS_MODULE);
> -	}
> +	if (!dev)
> +		return NULL;
> +
> +	mutex_init(&dev->mutex);
> +	spin_lock_init(&dev->event_lock);
> +	timer_setup(&dev->timer, NULL, 0);
> +	INIT_LIST_HEAD(&dev->h_list);
> +	INIT_LIST_HEAD(&dev->node);
> +
> +	dev->dev.type = &input_dev_type;
> +	dev->dev.class = &input_class;
> +	device_initialize(&dev->dev);
> +	/*
> +	 * From this point on we can no longer simply "kfree(dev)", we need
> +	 * to use input_free_device() so that device core properly frees its
> +	 * resources associated with the input device.
> +	 */
> +
> +	dev_set_name(&dev->dev, "input%lu",
> +		     (unsigned long)atomic_inc_return(&input_no));
> +
> +	__module_get(THIS_MODULE);
>  
>  	return dev;
>  }
> -- 
> 2.45.2.803.g4e1b14247a-goog
> 

Kind regards,
Jeff LaBundy

Re: [PATCH v2 6/7] Input: preallocate memory to hold event values

[Jeff LaBundy]

Hi Dmitry,

On Wed, Jul 03, 2024 at 02:37:53PM -0700, Dmitry Torokhov wrote:
> Preallocate memory for holding event values (input_dev->vals) so that
> there is no need to check if it was allocated or not in the event
> processing code.
> 
> The amount of memory will be adjusted after input device has been fully
> set up upon registering device with the input core.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Jeff LaBundy <jeff@labundy.com>

> ---
>  drivers/input/input.c | 61 +++++++++++++++++++++++++++++++------------
>  1 file changed, 44 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 9981fdfaee9f..4e12fa79883e 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -323,9 +323,6 @@ static void input_event_dispose(struct input_dev *dev, int disposition,
>  	if ((disposition & INPUT_PASS_TO_DEVICE) && dev->event)
>  		dev->event(dev, type, code, value);
>  
> -	if (!dev->vals)
> -		return;
> -
>  	if (disposition & INPUT_PASS_TO_HANDLERS) {
>  		struct input_value *v;
>  
> @@ -1985,6 +1982,18 @@ struct input_dev *input_allocate_device(void)
>  	if (!dev)
>  		return NULL;
>  
> +	/*
> +	 * Start with space for SYN_REPORT + 7 EV_KEY/EV_MSC events + 2 spare,
> +	 * see input_estimate_events_per_packet(). We will tune the number
> +	 * when we register the device.
> +	 */
> +	dev->max_vals = 10;
> +	dev->vals = kcalloc(dev->max_vals, sizeof(*dev->vals), GFP_KERNEL);
> +	if (!dev->vals) {
> +		kfree(dev);
> +		return NULL;
> +	}
> +
>  	mutex_init(&dev->mutex);
>  	spin_lock_init(&dev->event_lock);
>  	timer_setup(&dev->timer, NULL, 0);
> @@ -2344,6 +2353,35 @@ bool input_device_enabled(struct input_dev *dev)
>  }
>  EXPORT_SYMBOL_GPL(input_device_enabled);
>  
> +static int input_device_tune_vals(struct input_dev *dev)
> +{
> +	struct input_value *vals;
> +	unsigned int packet_size;
> +	unsigned int max_vals;
> +
> +	packet_size = input_estimate_events_per_packet(dev);
> +	if (dev->hint_events_per_packet < packet_size)
> +		dev->hint_events_per_packet = packet_size;
> +
> +	max_vals = dev->hint_events_per_packet + 2;
> +	if (dev->max_vals >= max_vals)
> +		return 0;
> +
> +	vals = kcalloc(max_vals, sizeof(*vals), GFP_KERNEL);
> +	if (!vals)
> +		return -ENOMEM;
> +
> +	spin_lock_irq(&dev->event_lock);
> +	dev->max_vals = max_vals;
> +	swap(dev->vals, vals);
> +	spin_unlock_irq(&dev->event_lock);
> +
> +	/* Because of swap() above, this frees the old vals memory */
> +	kfree(vals);
> +
> +	return 0;
> +}
> +
>  /**
>   * input_register_device - register device with input core
>   * @dev: device to be registered
> @@ -2371,7 +2409,6 @@ int input_register_device(struct input_dev *dev)
>  {
>  	struct input_devres *devres = NULL;
>  	struct input_handler *handler;
> -	unsigned int packet_size;
>  	const char *path;
>  	int error;
>  
> @@ -2399,16 +2436,9 @@ int input_register_device(struct input_dev *dev)
>  	/* Make sure that bitmasks not mentioned in dev->evbit are clean. */
>  	input_cleanse_bitmasks(dev);
>  
> -	packet_size = input_estimate_events_per_packet(dev);
> -	if (dev->hint_events_per_packet < packet_size)
> -		dev->hint_events_per_packet = packet_size;
> -
> -	dev->max_vals = dev->hint_events_per_packet + 2;
> -	dev->vals = kcalloc(dev->max_vals, sizeof(*dev->vals), GFP_KERNEL);
> -	if (!dev->vals) {
> -		error = -ENOMEM;
> +	error = input_device_tune_vals(dev);
> +	if (error)
>  		goto err_devres_free;
> -	}
>  
>  	/*
>  	 * If delay and period are pre-set by the driver, then autorepeating
> @@ -2428,7 +2458,7 @@ int input_register_device(struct input_dev *dev)
>  
>  	error = device_add(&dev->dev);
>  	if (error)
> -		goto err_free_vals;
> +		goto err_devres_free;
>  
>  	path = kobject_get_path(&dev->dev.kobj, GFP_KERNEL);
>  	pr_info("%s as %s\n",
> @@ -2458,9 +2488,6 @@ int input_register_device(struct input_dev *dev)
>  
>  err_device_del:
>  	device_del(&dev->dev);
> -err_free_vals:
> -	kfree(dev->vals);
> -	dev->vals = NULL;
>  err_devres_free:
>  	devres_free(devres);
>  	return error;
> -- 
> 2.45.2.803.g4e1b14247a-goog
> 

Kind regards,
Jeff LaBundy

Re: [PATCH v2 7/7] Input: do not check number of events in input_pass_values()

[Jeff LaBundy]

Hi Dmitry,

On Wed, Jul 03, 2024 at 02:37:54PM -0700, Dmitry Torokhov wrote:
> Now that the input_dev->vals array is always there we can be assured
> that input_pass_values() is always called with a non-0 number of
> events. Remove the check.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Jeff LaBundy <jeff@labundy.com>

> ---
>  drivers/input/input.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 4e12fa79883e..54c57b267b25 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -115,9 +115,6 @@ static void input_pass_values(struct input_dev *dev,
>  
>  	lockdep_assert_held(&dev->event_lock);
>  
> -	if (!count)
> -		return;
> -
>  	rcu_read_lock();
>  
>  	handle = rcu_dereference(dev->grab);
> -- 
> 2.45.2.803.g4e1b14247a-goog
> 

Kind regards,
Jeff LaBundy