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

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

[Maxwell Doose]

On Fri, May 15, 2026 at 9:13 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> 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 ;)

Gah, (of course its the callbacks!) will fix for v2. At least it's a
pattern that I can recognize now (I still need to take a deep deep
dive into the IIO API).

> > ---
> >  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.
>

Will get that helper in for the v2. TBH would've used scoped_guard()
but in my opinion indentation would get screwed up at that point so
I'll go with the helper.

>
> 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.
>

Alrighty, sounds good.

best regards,
max

(ps, will be away for weekend so unfortunately I won't be able to
respond to email, as I won't have access to my computer, and my
phone's email client is crap :( )

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