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

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

[Maxwell Doose]

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>
---
 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
@@ -368,11 +368,13 @@ static ssize_t calibration_auto_enable_show(struct device *dev, struct device_at
 	int ret;
 	u16 val;
 
-	mutex_lock(&state->lock);
-	ret = scd30_command_read(state, CMD_ASC, &val);
-	mutex_unlock(&state->lock);
+	guard(mutex)(&state->lock);
 
-	return ret ?: sysfs_emit(buf, "%d\n", val);
+	ret = scd30_command_read(state, CMD_ASC, &val);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%d\n", val);
 }
 
 static ssize_t calibration_auto_enable_store(struct device *dev, struct device_attribute *attr,
@@ -387,11 +389,13 @@ static ssize_t calibration_auto_enable_store(struct device *dev, struct device_a
 	if (ret)
 		return ret;
 
-	mutex_lock(&state->lock);
-	ret = scd30_command_write(state, CMD_ASC, val);
-	mutex_unlock(&state->lock);
+	guard(mutex)(&state->lock);
 
-	return ret ?: len;
+	ret = scd30_command_write(state, CMD_ASC, val);
+	if (ret)
+		return ret;
+
+	return len;
 }
 
 static ssize_t calibration_forced_value_show(struct device *dev, struct device_attribute *attr,
@@ -402,11 +406,13 @@ static ssize_t calibration_forced_value_show(struct device *dev, struct device_a
 	int ret;
 	u16 val;
 
-	mutex_lock(&state->lock);
-	ret = scd30_command_read(state, CMD_FRC, &val);
-	mutex_unlock(&state->lock);
+	guard(mutex)(&state->lock);
 
-	return ret ?: sysfs_emit(buf, "%d\n", val);
+	ret = scd30_command_read(state, CMD_FRC, &val);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%d\n", val);
 }
 
 static ssize_t calibration_forced_value_store(struct device *dev, struct device_attribute *attr,
@@ -424,11 +430,13 @@ static ssize_t calibration_forced_value_store(struct device *dev, struct device_
 	if (val < SCD30_FRC_MIN_PPM || val > SCD30_FRC_MAX_PPM)
 		return -EINVAL;
 
-	mutex_lock(&state->lock);
-	ret = scd30_command_write(state, CMD_FRC, val);
-	mutex_unlock(&state->lock);
+	guard(mutex)(&state->lock);
 
-	return ret ?: len;
+	ret = scd30_command_write(state, CMD_FRC, val);
+	if (ret)
+		return ret;
+
+	return len;
 }
 
 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);
+
 	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)
+		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;
 }
-- 
2.54.0

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

[Jonathan Cameron]

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;
>  }