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

Reviewed-by: Jeff LaBundy <jeff@xxxxxxxxxxx>

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