Re: [PATCH v6 3/4] iio: accel: sca3000: use lock guards

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 18 Jun 2025 00:12:18 -0300
Andrew Ijano <andrew.ijano@gmail.com> wrote:

> Use guard() and scoped_guard() for handling mutex lock instead of
> manually locking and unlocking the mutex. This prevents forgotten
> locks due to early exits and remove the need of gotos.

Please add extra description for where you have pushed locks up
a level in the call tree and why that is fine to do.

> 
> Signed-off-by: Andrew Ijano <andrew.lopes@alumni.usp.br>
> Co-developed-by: Gustavo Bastos <gustavobastos@usp.br>
> Signed-off-by: Gustavo Bastos <gustavobastos@usp.br>
> Suggested-by: Jonathan Cameron <jic23@kernel.org>

A few comments inline.

Thanks

Jonathan

> @@ -756,28 +747,23 @@ static int sca3000_write_raw(struct iio_dev *indio_dev,
>  			     int val, int val2, long mask)
>  {
>  	struct sca3000_state *st = iio_priv(indio_dev);
> -	int ret;
>  
>  	switch (mask) {
> -	case IIO_CHAN_INFO_SAMP_FREQ:
> +	case IIO_CHAN_INFO_SAMP_FREQ: {
> +		guard(mutex)(&st->lock);
As below

>  		if (val2)
>  			return -EINVAL;
> -		mutex_lock(&st->lock);
> -		ret = sca3000_write_raw_samp_freq(st, val);
> -		mutex_unlock(&st->lock);
> -		return ret;
> -	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> +		return sca3000_write_raw_samp_freq(st, val);
> +	}
> +	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: {
> +		guard(mutex)(&st->lock);
>  		if (val2)
>  			return -EINVAL;
> -		mutex_lock(&st->lock);

You can keep the old ordering here.  It doesn't matter much but
easier to be sure about a patch that makes no functional change.

> -		ret = sca3000_write_3db_freq(st, val);
> -		mutex_unlock(&st->lock);
> -		return ret;
> +		return sca3000_write_3db_freq(st, val);
> +	}

>  
>  /**
> @@ -1021,9 +1001,8 @@ static irqreturn_t sca3000_event_handler(int irq, void *private)
>  	 * Could lead if badly timed to an extra read of status reg,
>  	 * but ensures no interrupt is missed.
>  	 */
> -	mutex_lock(&st->lock);
> +	guard(mutex)(&st->lock);

This is a very large increase in scope.  Use scoped_guard() here instead to avoid
holding the lock over a whole load of code that doesn't need it.

>  	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_STATUS_ADDR));
> -	mutex_unlock(&st->lock);
>  	if (ret)
>  		goto done;
>  

>  }

>  
>  static inline
> @@ -1247,23 +1211,19 @@ int __sca3000_hw_ring_state_set(struct iio_dev *indio_dev, bool state)
>  	struct sca3000_state *st = iio_priv(indio_dev);
>  	int ret;
>  
> -	mutex_lock(&st->lock);
>  	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
>  	if (ret)
> -		goto error_ret;
> +		return ret;
> +
>  	if (state) {
>  		dev_info(&indio_dev->dev, "supposedly enabling ring buffer\n");
> -		ret = sca3000_write_reg(st,
> +		return sca3000_write_reg(st,
>  			SCA3000_REG_MODE_ADDR,
>  			ret | SCA3000_REG_MODE_RING_BUF_ENABLE);

This indent was already nasty so as we are touching the code good to clean it up.
For cases like this we can be more relaxed and if it helps readability go a little
over 80 chars (I think this will be 80 ish)

		return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
					 ret | SCA3000_REG_MODE_RING_BUF_ENABLE);


> -	} else
> -		ret = sca3000_write_reg(st,
> -			SCA3000_REG_MODE_ADDR,
> -			ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE);
> -error_ret:
> -	mutex_unlock(&st->lock);
> -
> -	return ret;
> +	}
> +	return sca3000_write_reg(st,
> +		SCA3000_REG_MODE_ADDR,
> +		ret & ~SCA3000_REG_MODE_RING_BUF_ENABLE);
>  }
>
>  
>  static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
> @@ -1307,22 +1259,18 @@ static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
>  	int ret;
>  	struct sca3000_state *st = iio_priv(indio_dev);
>  
> +	guard(mutex)(&st->lock);

See comment at the top - Making this change is fine but call it out in the patch
description as it isn't simple change to using guards, but instead to holding
the lock for longer.  Change is fine but point it out as it needs
more review than the mechanical changes.

>  	ret = __sca3000_hw_ring_state_set(indio_dev, 0);
>  	if (ret)
>  		return ret;
>  
>  	/* Disable the 50% full interrupt */
> -	mutex_lock(&st->lock);
> -
>  	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
>  	if (ret)
> -		goto unlock;
> -	ret = sca3000_write_reg(st,
> +		return ret;
> +	return sca3000_write_reg(st,
>  				SCA3000_REG_INT_MASK_ADDR,
>  				ret & ~SCA3000_REG_INT_MASK_RING_HALF);

As below.

> -unlock:
> -	mutex_unlock(&st->lock);
> -	return ret;
>  }

> @@ -1386,13 +1334,9 @@ static int sca3000_clean_setup(struct sca3000_state *st)
>  	 */
>  	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
>  	if (ret)
> -		goto error_ret;
> -	ret = sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> +		return ret;
> +	return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
>  				ret & SCA3000_MODE_PROT_MASK);

As below.

> -
> -error_ret:
> -	mutex_unlock(&st->lock);
> -	return ret;
>  }
>  
>  static const struct iio_info sca3000_info = {
> @@ -1470,19 +1414,16 @@ static int sca3000_stop_all_interrupts(struct sca3000_state *st)
>  {
>  	int ret;
>  
> -	mutex_lock(&st->lock);
> +	guard(mutex)(&st->lock);
>  	ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
>  	if (ret)
> -		goto error_ret;
> +		return ret;
>  
> -	ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
> +	return sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
This adds a character to this line which means that either the indent of
the following lines was previously wrong, or it is now.
I think you need to add a space to the following lines.

Check for other similar cases.

>  				ret &
>  				~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
>  				  SCA3000_REG_INT_MASK_RING_HALF |
>  				  SCA3000_REG_INT_MASK_ALL_INTS));