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

[PATCH v6] 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.

Add new helper function scd30_trigger_handler_helper() containing
the critical section for scd30_trigger_handler().

In addition, small refactor to replace "?:" operator with regular
if/else returns.

Signed-off-by: Maxwell Doose <m32285159@gmail.com>
---
 v2:
  - Fix callback issue as noted by Jonathan v1.
  - Refactor critical section of scd30_trigger_handler() into helper
    called scd30_trigger_handler_helper_locked().
  - Revert removal of goto in scd30_trigger_handler().

 v3:
  - Tune up helper to return early on failure condition per Jonathan's
    suggestion.

 v4:
  - Forgot to commit changes listed in v3, now fixed.

 v5:
  - Remove unneeded comment for scd30_trigger_handler_helper() per
    Jonathan's suggestion.
  - Change name of arr_size to scan_data_size per Jonathan's suggestion.
  - Change type of scan_data_size to size_t per Jonathan's suggestion.
  - Added blank line between return and the rest of the function body in
    scd30_trigger_handler_helper().

 v6:
  - Fix issue where names for scd30_trigger_handler_helper() did not
    match per Jonathan.

 drivers/iio/chemical/scd30_core.c | 66 ++++++++++++++++++++-----------
 1 file changed, 42 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/chemical/scd30_core.c b/drivers/iio/chemical/scd30_core.c
index a665fcb78806..3949c27b9a3e 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);
@@ -579,24 +587,34 @@ static irqreturn_t scd30_irq_thread_handler(int irq, void *priv)
 	return IRQ_HANDLED;
 }
 
+static int scd30_trigger_handler_helper(struct iio_dev *indio_dev, int *scan_data,
+					size_t scan_data_size)
+{
+	struct scd30_state *state = iio_priv(indio_dev);
+	int ret;
+
+	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, scan_data_size);
+
+	return ret;
+}
+
 static irqreturn_t scd30_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
-	struct scd30_state *state = iio_priv(indio_dev);
 	struct {
 		int data[SCD30_MEAS_COUNT];
 		aligned_s64 ts;
 	} scan = { };
 	int ret;
 
-	mutex_lock(&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);
+	ret = scd30_trigger_handler_helper(indio_dev, scan.data, sizeof(scan.data));
 	if (ret)
 		goto out;
 
-- 
2.54.0

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

[Joshua Crofts]

On Mon, 1 Jun 2026 at 01:38, 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.
>
> Add new helper function scd30_trigger_handler_helper() containing
> the critical section for scd30_trigger_handler().
>
> In addition, small refactor to replace "?:" operator with regular
> if/else returns.
>
> Signed-off-by: Maxwell Doose <m32285159@gmail.com>
> ---
>  v2:
>   - Fix callback issue as noted by Jonathan v1.
>   - Refactor critical section of scd30_trigger_handler() into helper
>     called scd30_trigger_handler_helper_locked().
>   - Revert removal of goto in scd30_trigger_handler().
>
>  v3:
>   - Tune up helper to return early on failure condition per Jonathan's
>     suggestion.
>
>  v4:
>   - Forgot to commit changes listed in v3, now fixed.
>
>  v5:
>   - Remove unneeded comment for scd30_trigger_handler_helper() per
>     Jonathan's suggestion.
>   - Change name of arr_size to scan_data_size per Jonathan's suggestion.
>   - Change type of scan_data_size to size_t per Jonathan's suggestion.
>   - Added blank line between return and the rest of the function body in
>     scd30_trigger_handler_helper().
>
>  v6:
>   - Fix issue where names for scd30_trigger_handler_helper() did not
>     match per Jonathan.
>
>  drivers/iio/chemical/scd30_core.c | 66 ++++++++++++++++++++-----------
>  1 file changed, 42 insertions(+), 24 deletions(-)

FWIW,

Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>

-- 
Kind regards

CJD

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

[Markus Elfring]

> 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, small refactor to replace "?:" operator with regular
> if/else returns.

How does such an information fit to a known patch requirement?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.1-rc5#n81


…
> +++ 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);
…

How do you think about to preserve lock scopes by using scoped_guard() calls?

Regards,
Markus

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

[Maxwell Doose]

Hi Markus,

On Mon, 1 Jun 2026 17:38:44 +0200
Markus Elfring <Markus.Elfring@web.de> 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, small refactor to replace "?:" operator with regular
> > if/else returns.  
> 
> How does such an information fit to a known patch requirement?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.1-rc5#n81
> 

Good point. However, I think that it's such a small change that nobody
is going to care that much. We'll see what Jonathan has to say, I
suppose.

> 
> …
> > +++ 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);  
> …
> 
> How do you think about to preserve lock scopes by using scoped_guard() calls?
> 

Generally scoped_guard() (at least in my experience in iio) is frowned
upon because of the hidden for loop that can make normal things (e.g.,
break, continue) really weird, plus guard()() with {} is also (in my
experience) more popular over scoped_guard(). In the past helpers have
also been liked over both as well.

-- 
best regards,
max

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

[Joshua Crofts]

On Mon, 1 Jun 2026 at 18:52, Maxwell Doose <m32285159@gmail.com> wrote:
> > How does such an information fit to a known patch requirement?
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.1-rc5#n81
> >
>
> Good point. However, I think that it's such a small change that nobody
> is going to care that much. We'll see what Jonathan has to say, I
> suppose.

Considering the patch went through six iterations without anyone
mentioning this, I'd say this point is moot.

-- 
Kind regards

CJD

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

[Andy Shevchenko]

On Mon, Jun 01, 2026 at 11:52:34AM -0500, Maxwell Doose wrote:
> On Mon, 1 Jun 2026 17:38:44 +0200
> Markus Elfring <Markus.Elfring@web.de> wrote:

…

> > > -	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);  
> > …
> > 
> > How do you think about to preserve lock scopes by using scoped_guard() calls?
> 
> Generally scoped_guard() (at least in my experience in iio) is frowned
> upon because of the hidden for loop that can make normal things (e.g.,
> break, continue) really weird, plus guard()() with {} is also (in my
> experience) more popular over scoped_guard(). In the past helpers have
> also been liked over both as well.

This is a slow path and printing under the lock can actually narrow the window
for value to change which means that it increases the chance the user gets
actual value and not the stale one.

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Sun, 31 May 2026 18:38:28 -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.
> 
> Add new helper function scd30_trigger_handler_helper() containing
> the critical section for scd30_trigger_handler().
> 
> In addition, small refactor to replace "?:" operator with regular
> if/else returns.
> 
> Signed-off-by: Maxwell Doose <m32285159@gmail.com>
Applied. Thanks,

J