linux-kernel - Re: [PATCH 3/3] iio: dac: ad5592r: localize locks only where needed in ad5592r_read

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] [day] [month] [year] [list]

Message-ID: <20200917194233.57b548be@archlinux>
Date:   Thu, 17 Sep 2020 19:42:33 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Alexandru Ardelean <alexandru.ardelean@...log.com>
Cc:     <linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] iio: dac: ad5592r: localize locks only where needed
 in ad5592r_read_raw()

On Mon, 6 Jul 2020 14:02:59 +0300
Alexandru Ardelean <alexandru.ardelean@...log.com> wrote:

> Since there was a recently discovered issue with these locks, it probably
> makes sense to cleanup the code a bit, to prevent it from being used as an
> example/reference.
> 
> This change moves the lock only where it is explicitly needed to protect
> resources from potential concurrent accesses.
> 
> It also reworks the switch statements to do direct returns vs caching the
> return value on a variable.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@...log.com>
Applied these two to the togreg branch of iio.git

Thanks,

Jonathan

> ---
>  drivers/iio/dac/ad5592r-base.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad5592r-base.c b/drivers/iio/dac/ad5592r-base.c
> index 7ea79e9bfa1d..f697b20efb6e 100644
> --- a/drivers/iio/dac/ad5592r-base.c
> +++ b/drivers/iio/dac/ad5592r-base.c
> @@ -380,32 +380,32 @@ static int ad5592r_read_raw(struct iio_dev *iio_dev,
>  
>  	switch (m) {
>  	case IIO_CHAN_INFO_RAW:
> -		mutex_lock(&st->lock);
> -
>  		if (!chan->output) {
> +			mutex_lock(&st->lock);
>  			ret = st->ops->read_adc(st, chan->channel, &read_val);
> +			mutex_unlock(&st->lock);
>  			if (ret)
> -				goto unlock;
> +				return ret;
>  
>  			if ((read_val >> 12 & 0x7) != (chan->channel & 0x7)) {
>  				dev_err(st->dev, "Error while reading channel %u\n",
>  						chan->channel);
> -				ret = -EIO;
> -				goto unlock;
> +				return -EIO;
>  			}
>  
>  			read_val &= GENMASK(11, 0);
>  
>  		} else {
> +			mutex_lock(&st->lock);
>  			read_val = st->cached_dac[chan->channel];
> +			mutex_unlock(&st->lock);
>  		}
>  
>  		dev_dbg(st->dev, "Channel %u read: 0x%04hX\n",
>  				chan->channel, read_val);
>  
>  		*val = (int) read_val;
> -		ret = IIO_VAL_INT;
> -		break;
> +		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
>  		*val = ad5592r_get_vref(st);
>  
> @@ -425,11 +425,13 @@ static int ad5592r_read_raw(struct iio_dev *iio_dev,
>  			mult = !!(st->cached_gp_ctrl &
>  				AD5592R_REG_CTRL_ADC_RANGE);
>  
> +		mutex_unlock(&st->lock);
> +
>  		*val *= ++mult;
>  
>  		*val2 = chan->scan_type.realbits;
> -		ret = IIO_VAL_FRACTIONAL_LOG2;
> -		break;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
>  	case IIO_CHAN_INFO_OFFSET:
>  		ret = ad5592r_get_vref(st);
>  
> @@ -439,15 +441,13 @@ static int ad5592r_read_raw(struct iio_dev *iio_dev,
>  			*val = (-34365 * 25) / ret;
>  		else
>  			*val = (-75365 * 25) / ret;
> -		ret =  IIO_VAL_INT;
> -		break;
> +
> +		mutex_unlock(&st->lock);
> +
> +		return IIO_VAL_INT;
>  	default:
>  		return -EINVAL;
>  	}
> -
> -unlock:
> -	mutex_unlock(&st->lock);
> -	return ret;
>  }
>  
>  static int ad5592r_write_raw_get_fmt(struct iio_dev *indio_dev,