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

[PATCH 0/4] 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.

Dmitry Torokhov (4):
  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: preallocate memory to hold event values

 drivers/input/evdev.c |   6 +-
 drivers/input/input.c | 210 +++++++++++++++++++++++++++---------------
 include/linux/input.h |   7 +-
 3 files changed, 146 insertions(+), 77 deletions(-)

-- 
2.45.2.803.g4e1b14247a-goog

[PATCH 1/4] 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..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;
-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH 1/4] Input: make sure input handlers define only one processing method

[Benjamin Tissoires]

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
> 

Re: [PATCH 1/4] Input: make sure input handlers define only one processing method

[Dmitry Torokhov]

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!

-- 
Dmitry

[PATCH 2/4] 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 51e0c4954600..c8eca8cdb976 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;
 }
 
 /*
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

Re: [PATCH 2/4] Input: make events() method return number of events processed

[Benjamin Tissoires]

On Jun 30 2024, 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>
> ---
>  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 51e0c4954600..c8eca8cdb976 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;
>  }
>  
>  /*
> 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
> 

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

Cheers,
Benjamin

[PATCH 3/4] 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 | 90 ++++++++++++++++++++++++-------------------
 1 file changed, 51 insertions(+), 39 deletions(-)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 8434348faeac..eeb755cb12e7 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -99,41 +99,6 @@ 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
@@ -154,11 +119,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 +2503,47 @@ 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;
+}
+
 /**
  * input_register_handler - register a new input handler
  * @handler: handler to be registered
@@ -2554,12 +2561,17 @@ 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;
+
 	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

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

[Benjamin Tissoires]

On Jun 30 2024, 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>
> ---
>  drivers/input/input.c | 90 ++++++++++++++++++++++++-------------------
>  1 file changed, 51 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 8434348faeac..eeb755cb12e7 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -99,41 +99,6 @@ 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

Nitpick: maybe that comment above input_pass_values() should also be
amended now that the processing is more straightforward?

> @@ -154,11 +119,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 +2503,47 @@ 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;
> +}
> +
>  /**
>   * input_register_handler - register a new input handler
>   * @handler: handler to be registered
> @@ -2554,12 +2561,17 @@ 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;
> +
>  	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
> 

Minor nitpick, but otherwise:

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

Cheers,
Benjamin

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

[Dmitry Torokhov]

On Mon, Jul 01, 2024 at 09:41:22AM +0200, Benjamin Tissoires wrote:
> On Jun 30 2024, Dmitry Torokhov wrote:
> >  /*
> >   * Pass values first through all filters and then, if event has not been
> >   * filtered out, through all open handles. This function is called with
> 
> Nitpick: maybe that comment above input_pass_values() should also be
> amended now that the processing is more straightforward?

I think the comment is still accurate from the higher POV. We do want to
send the event(s) first through all the filters and the remainder is
through the handlers. This is achieved by placing filters at the head of
the list of handles attacher to the device, and placing regular handles
at the tail of the list.

Do you want me to expand the comment?

Thanks.

-- 
Dmitry

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

[Benjamin Tissoires]

On Jul 01 2024, Dmitry Torokhov wrote:
> On Mon, Jul 01, 2024 at 09:41:22AM +0200, Benjamin Tissoires wrote:
> > On Jun 30 2024, Dmitry Torokhov wrote:
> > >  /*
> > >   * Pass values first through all filters and then, if event has not been
> > >   * filtered out, through all open handles. This function is called with
> > 
> > Nitpick: maybe that comment above input_pass_values() should also be
> > amended now that the processing is more straightforward?
> 
> I think the comment is still accurate from the higher POV. We do want to
> send the event(s) first through all the filters and the remainder is
> through the handlers. This is achieved by placing filters at the head of
> the list of handles attacher to the device, and placing regular handles
> at the tail of the list.

Oh right, I missed that in my review.

> 
> Do you want me to expand the comment?

Yeah, maybe add the blurb from above in the comment so we don't ask
further questions next time we revisit that code. And given that you
need to respin the series, it should be all right :)

Cheers,
Benjamin

[PATCH 4/4] 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 | 98 ++++++++++++++++++++++++++++---------------
 1 file changed, 64 insertions(+), 34 deletions(-)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index eeb755cb12e7..b65b645d9478 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -112,9 +112,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);
@@ -320,9 +317,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;
 
@@ -1979,22 +1973,41 @@ 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;
+
+	/*
+	 * 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);
+	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;
 }
 EXPORT_SYMBOL(input_allocate_device);
@@ -2334,6 +2347,34 @@ 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);
+
+	kfree(vals);
+
+	return 0;
+}
+
 /**
  * input_register_device - register device with input core
  * @dev: device to be registered
@@ -2361,7 +2402,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;
 
@@ -2389,16 +2429,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
@@ -2418,7 +2451,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",
@@ -2448,9 +2481,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

Re: [PATCH 4/4] Input: preallocate memory to hold event values

[Benjamin Tissoires]

On Jun 30 2024, 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.

As a general comment on this patch, I think it should be split in 2 or
3:
- reorder input_allocate_device() and introduce the `if (!dev) return`
  statement
- introduce input_device_tune_vals()
- and then preallocate the memory for dev->vals.

I think the code is OK in its current form, but the rewrite in the
middle are giving me a hard time ensuring we are not losing anything in
the change.

> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/input.c | 98 ++++++++++++++++++++++++++++---------------
>  1 file changed, 64 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index eeb755cb12e7..b65b645d9478 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -112,9 +112,6 @@ static void input_pass_values(struct input_dev *dev,
>  
>  	lockdep_assert_held(&dev->event_lock);
>  
> -	if (!count)
> -		return;

This doesn't seem to be related to the commit. Should this be in a
separate one?

> -
>  	rcu_read_lock();
>  
>  	handle = rcu_dereference(dev->grab);
> @@ -320,9 +317,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;
>  
> @@ -1979,22 +1973,41 @@ 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;
> +
> +	/*
> +	 * 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);
> +	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;
>  }
>  EXPORT_SYMBOL(input_allocate_device);
> @@ -2334,6 +2347,34 @@ 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);
> +
> +	kfree(vals);

Maybe add a comment here that we are freeing the *old* value of
dev->vals, not the brand new we just allocated a few lines above.
This made me look at the code a few time and wondered why we just
allocate and free the same data...

Cheers,
Benjamin

> +
> +	return 0;
> +}
> +
>  /**
>   * input_register_device - register device with input core
>   * @dev: device to be registered
> @@ -2361,7 +2402,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;
>  
> @@ -2389,16 +2429,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
> @@ -2418,7 +2451,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",
> @@ -2448,9 +2481,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
>