linux-kernel - Re: [PATCH RFC v2 1/4] iio: adc: ad7380: do not use iio_device_claim_direct

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: <20241228134934.38e69020@jic23-huawei>
Date: Sat, 28 Dec 2024 13:49:34 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Julien Stephan <jstephan@...libre.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
 <Michael.Hennerich@...log.com>, Nuno Sá <nuno.sa@...log.com>,
 David Lechner <dlechner@...libre.com>, Jonathan Corbet <corbet@....net>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-doc@...r.kernel.org
Subject: Re: [PATCH RFC v2 1/4] iio: adc: ad7380: do not use
 iio_device_claim_direct_scoped anymore

On Tue, 24 Dec 2024 10:34:30 +0100
Julien Stephan <jstephan@...libre.com> wrote:

> Rollback and remove the scoped version of iio_dvice_claim_direct_mode.
> 
> Signed-off-by: Julien Stephan <jstephan@...libre.com>
> ---
>  drivers/iio/adc/ad7380.c | 89 ++++++++++++++++++++++++++++--------------------
>  1 file changed, 53 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> index 4f32cb22f140442b831dc9a4f275e88e4ab2388e..4e26e0e7ac1d5a1c4c67118dbc34f2921fc171a4 100644
> --- a/drivers/iio/adc/ad7380.c
> +++ b/drivers/iio/adc/ad7380.c
> @@ -675,15 +675,21 @@ static const struct regmap_config ad7380_regmap_config = {
>  static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
>  				     u32 writeval, u32 *readval)
>  {
> -	iio_device_claim_direct_scoped(return  -EBUSY, indio_dev) {
> -		struct ad7380_state *st = iio_priv(indio_dev);
> +	struct ad7380_state *st = iio_priv(indio_dev);
> +	int ret;
>  
> -		if (readval)
> -			return regmap_read(st->regmap, reg, readval);
> -		else
> -			return regmap_write(st->regmap, reg, writeval);
> -	}
> -	unreachable();
> +	ret = iio_device_claim_direct_mode(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	if (readval)
> +		ret = regmap_read(st->regmap, reg, readval);
> +	else
> +		ret = regmap_write(st->regmap, reg, writeval);
> +
> +	iio_device_release_direct_mode(indio_dev);
> +
> +	return ret;
>  }
>  
>  /*
> @@ -920,6 +926,7 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
>  {
>  	struct ad7380_state *st = iio_priv(indio_dev);
>  	const struct iio_scan_type *scan_type;
> +	int ret;
>  
>  	scan_type = iio_get_current_scan_type(indio_dev, chan);
>  
> @@ -928,11 +935,16 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (info) {
>  	case IIO_CHAN_INFO_RAW:
> -		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> -			return ad7380_read_direct(st, chan->scan_index,
> -						  scan_type, val);
> -		}
> -		unreachable();
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
> +
> +		ret = ad7380_read_direct(st, chan->scan_index,
> +					 scan_type, val);
> +
> +		iio_device_release_direct_mode(indio_dev);
> +
> +		return ret;
>  	case IIO_CHAN_INFO_SCALE:
>  		/*
>  		 * According to the datasheet, the LSB size is:
> @@ -1024,31 +1036,36 @@ static int ad7380_write_raw(struct iio_dev *indio_dev,
>  		/* always enable resolution boost when oversampling is enabled */
>  		boost = osr > 0 ? 1 : 0;
>  
> -		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> -			ret = regmap_update_bits(st->regmap,
> -					AD7380_REG_ADDR_CONFIG1,
> -					AD7380_CONFIG1_OSR | AD7380_CONFIG1_RES,
> -					FIELD_PREP(AD7380_CONFIG1_OSR, osr) |
> -					FIELD_PREP(AD7380_CONFIG1_RES, boost));
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
>  
> -			if (ret)
> -				return ret;
> +		ret = regmap_update_bits(st->regmap,
> +					 AD7380_REG_ADDR_CONFIG1,
> +					 AD7380_CONFIG1_OSR | AD7380_CONFIG1_RES,
> +					 FIELD_PREP(AD7380_CONFIG1_OSR, osr) |
> +					 FIELD_PREP(AD7380_CONFIG1_RES, boost));
>  
> -			st->oversampling_ratio = val;
> -			st->resolution_boost_enabled = boost;
> -
> -			/*
> -			 * Perform a soft reset. This will flush the oversampling
> -			 * block and FIFO but will maintain the content of the
> -			 * configurable registers.
> -			 */
> -			return regmap_update_bits(st->regmap,
> -					AD7380_REG_ADDR_CONFIG2,
> -					AD7380_CONFIG2_RESET,
> -					FIELD_PREP(AD7380_CONFIG2_RESET,
> -						   AD7380_CONFIG2_RESET_SOFT));
> -		}
> -		unreachable();
> +		if (ret)
> +			goto err;
> +
> +		st->oversampling_ratio = val;
> +		st->resolution_boost_enabled = boost;
> +
> +		/*
> +		 * Perform a soft reset. This will flush the oversampling
> +		 * block and FIFO but will maintain the content of the
> +		 * configurable registers.
> +		 */
> +		ret = regmap_update_bits(st->regmap,
> +					 AD7380_REG_ADDR_CONFIG2,
> +					 AD7380_CONFIG2_RESET,
> +					 FIELD_PREP(AD7380_CONFIG2_RESET,
> +						    AD7380_CONFIG2_RESET_SOFT));
> +err:
Labels within switch statements can become hard to maintainer / read.

Id' suggest factoring out the bits between claim and release as a single
__ad7380_write_oversample() or something along those lines.

Otherwise looks good to me and thanks for doing this.

Jonathan


> +		iio_device_release_direct_mode(indio_dev);
> +
> +		return ret;
>  	default:
>  		return -EINVAL;
>  	}
>