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@xxxxxxxxx>
> ---
> 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
>
