Re: [PATCH 05/17] Input: gpio-keys - switch to using cleanup functions

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 8/25/24 7:16 AM, Dmitry Torokhov wrote:
> Start using __free() and guard() primitives to simplify the code
> and error handling. This makes the code more compact and error
> handling more robust by ensuring that locks are released in all
> code paths when control leaves critical section and all allocated
> memory is freed.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  drivers/input/keyboard/gpio_keys.c | 44 ++++++++++++------------------
>  1 file changed, 17 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index 9fb0bdcfbf9e..380fe8dab3b0 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -245,23 +245,20 @@ static ssize_t gpio_keys_attr_store_helper(struct gpio_keys_drvdata *ddata,
>  {
>  	int n_events = get_n_events_by_type(type);
>  	const unsigned long *bitmap = get_bm_events_by_type(ddata->input, type);
> -	unsigned long *bits;
>  	ssize_t error;
>  	int i;
>  
> -	bits = bitmap_alloc(n_events, GFP_KERNEL);
> +	unsigned long *bits __free(bitmap) = bitmap_alloc(n_events, GFP_KERNEL);
>  	if (!bits)
>  		return -ENOMEM;
>  
>  	error = bitmap_parselist(buf, bits, n_events);
>  	if (error)
> -		goto out;
> +		return error;
>  
>  	/* First validate */
> -	if (!bitmap_subset(bits, bitmap, n_events)) {
> -		error = -EINVAL;
> -		goto out;
> -	}
> +	if (!bitmap_subset(bits, bitmap, n_events))
> +		return -EINVAL;
>  
>  	for (i = 0; i < ddata->pdata->nbuttons; i++) {
>  		struct gpio_button_data *bdata = &ddata->data[i];
> @@ -271,12 +268,11 @@ static ssize_t gpio_keys_attr_store_helper(struct gpio_keys_drvdata *ddata,
>  
>  		if (test_bit(*bdata->code, bits) &&
>  		    !bdata->button->can_disable) {
> -			error = -EINVAL;
> -			goto out;
> +			return -EINVAL;
>  		}
>  	}
>  
> -	mutex_lock(&ddata->disable_lock);
> +	guard(mutex)(&ddata->disable_lock);
>  
>  	for (i = 0; i < ddata->pdata->nbuttons; i++) {
>  		struct gpio_button_data *bdata = &ddata->data[i];
> @@ -290,11 +286,7 @@ static ssize_t gpio_keys_attr_store_helper(struct gpio_keys_drvdata *ddata,
>  			gpio_keys_enable_button(bdata);
>  	}
>  
> -	mutex_unlock(&ddata->disable_lock);
> -
> -out:
> -	bitmap_free(bits);
> -	return error;
> +	return 0;
>  }
>  
>  #define ATTR_SHOW_FN(name, type, only_disabled)				\
> @@ -470,11 +462,10 @@ static irqreturn_t gpio_keys_irq_isr(int irq, void *dev_id)
>  {
>  	struct gpio_button_data *bdata = dev_id;
>  	struct input_dev *input = bdata->input;
> -	unsigned long flags;
>  
>  	BUG_ON(irq != bdata->irq);
>  
> -	spin_lock_irqsave(&bdata->lock, flags);
> +	guard(spinlock_irqsave)(&bdata->lock);
>  
>  	if (!bdata->key_pressed) {
>  		if (bdata->button->wakeup)
> @@ -497,7 +488,6 @@ static irqreturn_t gpio_keys_irq_isr(int irq, void *dev_id)
>  			      ms_to_ktime(bdata->release_delay),
>  			      HRTIMER_MODE_REL_HARD);
>  out:
> -	spin_unlock_irqrestore(&bdata->lock, flags);
>  	return IRQ_HANDLED;
>  }
>  
> @@ -1062,10 +1052,10 @@ static int gpio_keys_suspend(struct device *dev)
>  		if (error)
>  			return error;
>  	} else {
> -		mutex_lock(&input->mutex);
> +		guard(mutex)(&input->mutex);
> +
>  		if (input_device_enabled(input))
>  			gpio_keys_close(input);
> -		mutex_unlock(&input->mutex);
>  	}
>  
>  	return 0;
> @@ -1075,20 +1065,20 @@ static int gpio_keys_resume(struct device *dev)
>  {
>  	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);
>  	struct input_dev *input = ddata->input;
> -	int error = 0;
> +	int error;
>  
>  	if (device_may_wakeup(dev)) {
>  		gpio_keys_disable_wakeup(ddata);
>  	} else {
> -		mutex_lock(&input->mutex);
> -		if (input_device_enabled(input))
> +		guard(mutex)(&input->mutex);
> +
> +		if (input_device_enabled(input)) {
>  			error = gpio_keys_open(input);
> -		mutex_unlock(&input->mutex);
> +			if (error)
> +				return error;
> +		}
>  	}
>  
> -	if (error)
> -		return error;
> -
>  	gpio_keys_report_state(ddata);
>  	return 0;
>  }