[PATCH v4] iio: chemical: sps30: Replace manual locking with RAII locking

[PATCH v4] iio: chemical: sps30: Replace manual locking with RAII locking

[Maxwell Doose]

Replace manual mutex_lock() and mutex_unlock() calls with the much newer
guard(mutex)() and scoped_guard() macros to enable RAII patterns,
modernize the driver, and to increase readability.

Add guard(mutex)() into sps30_do_meas() as every caller locks it's call.

Signed-off-by: Maxwell Doose <m32285159@gmail.com>
---
 drivers/iio/chemical/sps30.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/chemical/sps30.c b/drivers/iio/chemical/sps30.c
index a934bf0298dd..c10026509e9c 100644
--- a/drivers/iio/chemical/sps30.c
+++ b/drivers/iio/chemical/sps30.c
@@ -5,6 +5,7 @@
  * Copyright (c) Tomasz Duszynski <tduszyns@gmail.com>
  */
 
+#include <linux/cleanup.h>
 #include <linux/crc8.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
@@ -69,6 +70,8 @@ static int sps30_do_meas(struct sps30_state *state, s32 *data, int size)
 {
 	int i, ret;
 
+	guard(mutex)(&state->lock);
+
 	if (state->state == RESET) {
 		ret = state->ops->start_meas(state);
 		if (ret)
@@ -111,9 +114,7 @@ static irqreturn_t sps30_trigger_handler(int irq, void *p)
 		aligned_s64 ts;
 	} scan;
 
-	mutex_lock(&state->lock);
 	ret = sps30_do_meas(state, scan.data, ARRAY_SIZE(scan.data));
-	mutex_unlock(&state->lock);
 	if (ret)
 		goto err;
 
@@ -136,7 +137,6 @@ static int sps30_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_PROCESSED:
 		switch (chan->type) {
 		case IIO_MASSCONCENTRATION:
-			mutex_lock(&state->lock);
 			/* read up to the number of bytes actually needed */
 			switch (chan->channel2) {
 			case IIO_MOD_PM1:
@@ -152,7 +152,6 @@ static int sps30_read_raw(struct iio_dev *indio_dev,
 				ret = sps30_do_meas(state, data, 4);
 				break;
 			}
-			mutex_unlock(&state->lock);
 			if (ret)
 				return ret;
 
@@ -160,6 +159,7 @@ static int sps30_read_raw(struct iio_dev *indio_dev,
 			*val2 = (data[chan->address] % 100) * 10000;
 
 			return IIO_VAL_INT_PLUS_MICRO;
+
 		default:
 			return -EINVAL;
 		}
@@ -197,9 +197,9 @@ static ssize_t start_cleaning_store(struct device *dev,
 	if (kstrtoint(buf, 0, &val) || val != 1)
 		return -EINVAL;
 
-	mutex_lock(&state->lock);
+	guard(mutex)(&state->lock);
+
 	ret = state->ops->clean_fan(state);
-	mutex_unlock(&state->lock);
 	if (ret)
 		return ret;
 
@@ -215,9 +215,9 @@ static ssize_t cleaning_period_show(struct device *dev,
 	__be32 val;
 	int ret;
 
-	mutex_lock(&state->lock);
+	guard(mutex)(&state->lock);
+
 	ret = state->ops->read_cleaning_period(state, &val);
-	mutex_unlock(&state->lock);
 	if (ret)
 		return ret;
 
@@ -238,12 +238,11 @@ static ssize_t cleaning_period_store(struct device *dev, struct device_attribute
 	    (val > SPS30_AUTO_CLEANING_PERIOD_MAX))
 		return -EINVAL;
 
-	mutex_lock(&state->lock);
+	guard(mutex)(&state->lock);
+
 	ret = state->ops->write_cleaning_period(state, cpu_to_be32(val));
-	if (ret) {
-		mutex_unlock(&state->lock);
+	if (ret)
 		return ret;
-	}
 
 	msleep(20);
 
@@ -256,8 +255,6 @@ static ssize_t cleaning_period_store(struct device *dev, struct device_attribute
 		dev_warn(dev,
 			 "period changed but reads will return the old value\n");
 
-	mutex_unlock(&state->lock);
-
 	return len;
 }
 
-- 
2.54.0

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

[Stepan Ionichev]

The hunk in sps30_read_raw() adds an empty line before "default:" in
the switch, which is unrelated to the RAII conversion and probably
worth dropping.

Also, moving guard(mutex) into sps30_do_meas() changes its contract:
previously callers had to hold state->lock around the call, now the
function takes it itself. A line in the commit log would help future
readers avoid the obvious "I'll just wrap it in mutex_lock()" pitfall.

Stepan

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

[Maxwell Doose]

On Wed, May 20, 2026 at 7:22 AM Stepan Ionichev <sozdayvek@gmail.com> wrote:
>
> The hunk in sps30_read_raw() adds an empty line before "default:" in
> the switch, which is unrelated to the RAII conversion and probably
> worth dropping.
>

First one is a stray change, my bad.


> Also, moving guard(mutex) into sps30_do_meas() changes its contract:
> previously callers had to hold state->lock around the call, now the
> function takes it itself. A line in the commit log would help future
> readers avoid the obvious "I'll just wrap it in mutex_lock()" pitfall.
>
> Stepan

See:
>> Add guard(mutex)() into sps30_do_meas() as every caller locks it's call.

Basic but still denotes that sps30_do_meas() holds the lock.

best regards,
max

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

[Jonathan Cameron]

On Wed, 20 May 2026 11:49:33 -0500
Maxwell Doose <m32285159@gmail.com> wrote:

> On Wed, May 20, 2026 at 7:22 AM Stepan Ionichev <sozdayvek@gmail.com> wrote:
> >
> > The hunk in sps30_read_raw() adds an empty line before "default:" in
> > the switch, which is unrelated to the RAII conversion and probably
> > worth dropping.
> >  
> 
> First one is a stray change, my bad.
> 
> 
> > Also, moving guard(mutex) into sps30_do_meas() changes its contract:
> > previously callers had to hold state->lock around the call, now the
> > function takes it itself. A line in the commit log would help future
> > readers avoid the obvious "I'll just wrap it in mutex_lock()" pitfall.
> >
> > Stepan  
> 
> See:
> >> Add guard(mutex)() into sps30_do_meas() as every caller locks it's call.  
> 
> Basic but still denotes that sps30_do_meas() holds the lock.

I'd tweak to something a tiny bit more detailed like

Move mutex locking (and switch to guard()) into sps30_do_meas() as every
caller takes the lock just for this call.

> 
> best regards,
> max
> 

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

[Maxwell Doose]

On Wed, May 20, 2026 at 2:04 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed, 20 May 2026 11:49:33 -0500
> Maxwell Doose <m32285159@gmail.com> wrote:
>
> > On Wed, May 20, 2026 at 7:22 AM Stepan Ionichev <sozdayvek@gmail.com> wrote:
> > >
> > > The hunk in sps30_read_raw() adds an empty line before "default:" in
> > > the switch, which is unrelated to the RAII conversion and probably
> > > worth dropping.
> > >
> >
> > First one is a stray change, my bad.
> >
> >
> > > Also, moving guard(mutex) into sps30_do_meas() changes its contract:
> > > previously callers had to hold state->lock around the call, now the
> > > function takes it itself. A line in the commit log would help future
> > > readers avoid the obvious "I'll just wrap it in mutex_lock()" pitfall.
> > >
> > > Stepan
> >
> > See:
> > >> Add guard(mutex)() into sps30_do_meas() as every caller locks it's call.
> >
> > Basic but still denotes that sps30_do_meas() holds the lock.
>
> I'd tweak to something a tiny bit more detailed like
>
> Move mutex locking (and switch to guard()) into sps30_do_meas() as every
> caller takes the lock just for this call.
>

Will do, while I'm at it I'll also fix the stray whitespace.

best regards,
max



> >
> > best regards,
> > max
> >
>

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

[Andy Shevchenko]

On Tue, May 19, 2026 at 04:26:02PM -0500, Maxwell Doose wrote:
> Replace manual mutex_lock() and mutex_unlock() calls with the much newer
> guard(mutex)() and scoped_guard() macros to enable RAII patterns,
> modernize the driver, and to increase readability.
> 
> Add guard(mutex)() into sps30_do_meas() as every caller locks it's call.

...

> -	mutex_lock(&state->lock);
> +	guard(mutex)(&state->lock);
> +
>  	ret = state->ops->write_cleaning_period(state, cpu_to_be32(val));
> -	if (ret) {
> -		mutex_unlock(&state->lock);
> +	if (ret)
>  		return ret;
> -	}
>  
>  	msleep(20);

Hmm... msleep() under the mutex?

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Tue, 2 Jun 2026 10:13:02 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Tue, May 19, 2026 at 04:26:02PM -0500, Maxwell Doose wrote:
> > Replace manual mutex_lock() and mutex_unlock() calls with the much newer
> > guard(mutex)() and scoped_guard() macros to enable RAII patterns,
> > modernize the driver, and to increase readability.
> > 
> > Add guard(mutex)() into sps30_do_meas() as every caller locks it's call.  
> 
> ...
> 
> > -	mutex_lock(&state->lock);
> > +	guard(mutex)(&state->lock);
> > +
> >  	ret = state->ops->write_cleaning_period(state, cpu_to_be32(val));
> > -	if (ret) {
> > -		mutex_unlock(&state->lock);
> > +	if (ret)
> >  		return ret;
> > -	}
> >  
> >  	msleep(20);  
> 
> Hmm... msleep() under the mutex?
> 
Device needs to ensure no other access to the hardware whilst a reading
is ongoing.  Mutex being held over that sleep seems appropriate to me
given expectation is that any userspace code that is messing with other
paths that take the lock is shooting itself in the foot and should probably
be doing it's own serialization at a higher level (i.e. not hitting the
sysfs interface from mutliple threads).

Jonathan