diff mbox series

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

Message ID 20240701060553.869989-5-dmitry.torokhov@gmail.com (mailing list archive)
State Superseded
Headers show
Series Simplify event handling logic in input core | expand

Commit Message

Dmitry Torokhov July 1, 2024, 6:05 a.m. UTC 
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(-)

Comments

Benjamin Tissoires July 1, 2024, 7:53 a.m. UTC | #1 
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
>
diff mbox series

Patch

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;