linux-kernel - Re: [PATCH 2/7] iio: dac: ad5592r: use lock guards

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: <20250407201202.7c74a39d@jic23-huawei>
Date: Mon, 7 Apr 2025 20:12:02 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
 <Michael.Hennerich@...log.com>, Linus Walleij <linus.walleij@...aro.org>,
 Cosmin Tanislav <cosmin.tanislav@...log.com>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org, Bartosz
 Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH 2/7] iio: dac: ad5592r: use lock guards

On Mon, 07 Apr 2025 09:18:10 +0200
Bartosz Golaszewski <brgl@...ev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> 
> Use lock guards from linux/cleanup.h to simplify the code and remove
> some labels.
> 
> Note that we need to initialize some variables even though it's not
> technically required as scoped_guards() are implemented as for loops.
That is always a tiny bit irritating. Thanks for calling it out.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> ---
>  drivers/iio/dac/ad5592r-base.c | 119 ++++++++++++++++-------------------------
>  1 file changed, 47 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad5592r-base.c b/drivers/iio/dac/ad5592r-base.c
> index fe4c35689d4d..bbe3b52c6a12 100644
> --- a/drivers/iio/dac/ad5592r-base.c
> +++ b/drivers/iio/dac/ad5592r-base.c

Just one case I'd prefer done a tiny bit differently so as to alway do
early returns rather than very nearly always.


>  	udelay(250);
> @@ -249,45 +235,43 @@ static int ad5592r_set_channel_modes(struct ad5592r_state *st)
>  		}
>  	}
>  
> -	mutex_lock(&st->lock);
> +	guard(mutex)(&st->lock);
>  
>  	/* Pull down unused pins to GND */
>  	ret = ops->reg_write(st, AD5592R_REG_PULLDOWN, pulldown);
>  	if (ret)
> -		goto err_unlock;
> +		return ret;
>  
>  	ret = ops->reg_write(st, AD5592R_REG_TRISTATE, tristate);
>  	if (ret)
> -		goto err_unlock;
> +		return ret;
>  
>  	/* Configure pins that we use */
>  	ret = ops->reg_write(st, AD5592R_REG_DAC_EN, dac);
>  	if (ret)
> -		goto err_unlock;
> +		return ret;
>  
>  	ret = ops->reg_write(st, AD5592R_REG_ADC_EN, adc);
>  	if (ret)
> -		goto err_unlock;
> +		return ret;
>  
>  	ret = ops->reg_write(st, AD5592R_REG_GPIO_SET, st->gpio_val);
>  	if (ret)
> -		goto err_unlock;
> +		return ret;
>  
>  	ret = ops->reg_write(st, AD5592R_REG_GPIO_OUT_EN, st->gpio_out);
>  	if (ret)
> -		goto err_unlock;
> +		return ret;
>  
>  	ret = ops->reg_write(st, AD5592R_REG_GPIO_IN_EN, st->gpio_in);
>  	if (ret)
> -		goto err_unlock;
> +		return ret;
>  
>  	/* Verify that we can read back at least one register */
>  	ret = ops->reg_read(st, AD5592R_REG_ADC_EN, &read_back);
>  	if (!ret && (read_back & 0xff) != adc)
>  		ret = -EIO;
		return -EIO; 
for this one.

>  
> -err_unlock:
> -	mutex_unlock(&st->lock);
>  	return ret;
return 0; here

>  }