linux-kernel - Re: [PATCH] iInput: adp5588-keys - use guard notation when acquiring mutexes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <pdnij2qolnmbcwtvlsdvkvtua7s7yjw4ms4bc7zyk3tpdr5pou@sga4mhcztfiz>
Date: Mon, 1 Jul 2024 22:28:09 +0200
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: Michael Hennerich <michael.hennerich@...log.com>, 
	Utsav Agarwal <utsav.agarwal@...log.com>, Nuno Sá <noname.nuno@...il.com>, 
	Christophe JAILLET <christophe.jaillet@...adoo.fr>, linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iInput: adp5588-keys - use guard notation when acquiring
 mutexes

Hello Dmitry,

$Subject ~= s/iI/I/

On Mon, Jul 01, 2024 at 10:57:18AM -0700, Dmitry Torokhov wrote:
> This makes the code more compact and error handling more robust.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@...il.com>
> ---
> 
> Sending this out because Utsav is working on the driver so he can rebase
> the work on top of this.
> 
>  drivers/input/keyboard/adp5588-keys.c | 49 ++++++++++-----------------
>  1 file changed, 17 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
> index 1b0279393df4..09bcfc6b9408 100644
> --- a/drivers/input/keyboard/adp5588-keys.c
> +++ b/drivers/input/keyboard/adp5588-keys.c
> @@ -221,15 +221,13 @@ static int adp5588_gpio_get_value(struct gpio_chip *chip, unsigned int off)
>  	unsigned int bit = ADP5588_BIT(kpad->gpiomap[off]);
>  	int val;
>  
> -	mutex_lock(&kpad->gpio_lock);
> +	guard(mutex)(&kpad->gpio_lock);
>  
>  	if (kpad->dir[bank] & bit)
>  		val = kpad->dat_out[bank];
>  	else
>  		val = adp5588_read(kpad->client, GPIO_DAT_STAT1 + bank);
>  
> -	mutex_unlock(&kpad->gpio_lock);
> -
>  	return !!(val & bit);
>  }
>  
> @@ -240,7 +238,7 @@ static void adp5588_gpio_set_value(struct gpio_chip *chip,
>  	unsigned int bank = ADP5588_BANK(kpad->gpiomap[off]);
>  	unsigned int bit = ADP5588_BIT(kpad->gpiomap[off]);
>  
> -	mutex_lock(&kpad->gpio_lock);
> +	guard(mutex)(&kpad->gpio_lock);
>  
>  	if (val)
>  		kpad->dat_out[bank] |= bit;
> @@ -248,8 +246,6 @@ static void adp5588_gpio_set_value(struct gpio_chip *chip,
>  		kpad->dat_out[bank] &= ~bit;
>  
>  	adp5588_write(kpad->client, GPIO_DAT_OUT1 + bank, kpad->dat_out[bank]);
> -
> -	mutex_unlock(&kpad->gpio_lock);
>  }
>  
>  static int adp5588_gpio_set_config(struct gpio_chip *chip,  unsigned int off,
> @@ -259,7 +255,6 @@ static int adp5588_gpio_set_config(struct gpio_chip *chip,  unsigned int off,
>  	unsigned int bank = ADP5588_BANK(kpad->gpiomap[off]);
>  	unsigned int bit = ADP5588_BIT(kpad->gpiomap[off]);
>  	bool pull_disable;
> -	int ret;
>  
>  	switch (pinconf_to_config_param(config)) {
>  	case PIN_CONFIG_BIAS_PULL_UP:
> @@ -272,19 +267,15 @@ static int adp5588_gpio_set_config(struct gpio_chip *chip,  unsigned int off,
>  		return -ENOTSUPP;
>  	}
>  
> -	mutex_lock(&kpad->gpio_lock);
> +	guard(mutex)(&kpad->gpio_lock);
>  
>  	if (pull_disable)
>  		kpad->pull_dis[bank] |= bit;
>  	else
>  		kpad->pull_dis[bank] &= bit;
>  
> -	ret = adp5588_write(kpad->client, GPIO_PULL1 + bank,
> -			    kpad->pull_dis[bank]);
> -
> -	mutex_unlock(&kpad->gpio_lock);
> -
> -	return ret;
> +	return adp5588_write(kpad->client, GPIO_PULL1 + bank,
> +			     kpad->pull_dis[bank]);
>  }
>  
>  static int adp5588_gpio_direction_input(struct gpio_chip *chip, unsigned int off)
> @@ -292,16 +283,11 @@ static int adp5588_gpio_direction_input(struct gpio_chip *chip, unsigned int off
>  	struct adp5588_kpad *kpad = gpiochip_get_data(chip);
>  	unsigned int bank = ADP5588_BANK(kpad->gpiomap[off]);
>  	unsigned int bit = ADP5588_BIT(kpad->gpiomap[off]);
> -	int ret;
>  
> -	mutex_lock(&kpad->gpio_lock);
> +	guard(mutex)(&kpad->gpio_lock);
>  
>  	kpad->dir[bank] &= ~bit;
> -	ret = adp5588_write(kpad->client, GPIO_DIR1 + bank, kpad->dir[bank]);
> -
> -	mutex_unlock(&kpad->gpio_lock);
> -
> -	return ret;
> +	return adp5588_write(kpad->client, GPIO_DIR1 + bank, kpad->dir[bank]);
>  }
>  
>  static int adp5588_gpio_direction_output(struct gpio_chip *chip,
> @@ -310,9 +296,9 @@ static int adp5588_gpio_direction_output(struct gpio_chip *chip,
>  	struct adp5588_kpad *kpad = gpiochip_get_data(chip);
>  	unsigned int bank = ADP5588_BANK(kpad->gpiomap[off]);
>  	unsigned int bit = ADP5588_BIT(kpad->gpiomap[off]);
> -	int ret;
> +	int error;

If you keep ret it's consistent with the other functions in this driver
(at least the one you touched above).

Otherwise looks fine to me,
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)