Re: [PATCH] iio: chemical: scd30: Replace manual locking with RAII locking

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 14 May 2026 19:26:03 -0500
Maxwell Doose <m32285159@gmail.com> wrote:

> scd30_core.c currently uses manual mutex_lock() and mutex_unlock()
> calls. Replace them with the newer guard(mutex)() for cleaner RAII
> patterns and to improve maintainability.
> 
> In addition, minor refactors in control logic to remove gotos where the
> guard(mutex)() calls would be used, and replace the "?:" operator with
> regular if/else returns.
> 
> Signed-off-by: Maxwell Doose <m32285159@gmail.com>
https://sashiko.dev/#/patchset/20260515002603.19240-1-m32285159%40gmail.com

It caught the main issue. Given that is a fairly common bad pattern
I might have noticed it. But then maybe not. Another win to the bot ;)
> ---
>  drivers/iio/chemical/scd30_core.c | 53 +++++++++++++++++--------------
>  1 file changed, 29 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iio/chemical/scd30_core.c b/drivers/iio/chemical/scd30_core.c
> index a665fcb78806..a24c95874fd3 100644
> --- a/drivers/iio/chemical/scd30_core.c
> +++ b/drivers/iio/chemical/scd30_core.c

>  
>  static IIO_DEVICE_ATTR_RO(sampling_frequency_available, 0);
> @@ -590,19 +598,16 @@ static irqreturn_t scd30_trigger_handler(int irq, void *p)
>  	} scan = { };
>  	int ret;
>  
> -	mutex_lock(&state->lock);
> +	guard(mutex)(&state->lock);

I agree with sashiko here (I might have noticed) that it is in appropriate
to extend the lock scope so far. In particular over the iio_trigger_notify_done().
Depending on exactly what happens in there we might end up with a deadlock.

For this one I'd factor out the reading code and memcpy + lock into a helper
- you can use guard() in that safely without expanding the scope and keep
to the goto pattern for the outer function.

Or just leave this one alone as too complex.

> +
>  	if (!iio_trigger_using_own(indio_dev))
>  		ret = scd30_read_poll(state);
>  	else
>  		ret = scd30_read_meas(state);
>  	memcpy(scan.data, state->meas, sizeof(state->meas));
> -	mutex_unlock(&state->lock);
> -	if (ret)
> -		goto out;
> -
> -	iio_push_to_buffers_with_ts(indio_dev, &scan, sizeof(scan),
> -				    iio_get_time_ns(indio_dev));
> -out:
> +	if (!ret)
Very rarely will I be happy with a change that flips from having the
error path out of line to one where the good path is out of line.
If you do decide to change this one via a helper, keep the goto pattern
for this error handling.


> +		iio_push_to_buffers_with_ts(indio_dev, &scan, sizeof(scan),
> +					    iio_get_time_ns(indio_dev));
>  	iio_trigger_notify_done(indio_dev->trig);
>  	return IRQ_HANDLED;
>  }