drivers/iio/chemical/scd30_core.c | 66 ++++++++++++++++++++----------- 1 file changed, 42 insertions(+), 24 deletions(-)
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_locked() 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().
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..bb8bdea7c833 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;
}
+/* Meant ONLY for scd30_trigger_handler() */
+static int scd30_trigger_handler_helper_locked(struct iio_dev *indio_dev,
+ int *scan_data, int arr_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, arr_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_locked(indio_dev, scan.data, sizeof(scan.data));
if (ret)
goto out;
--
2.54.0On Tue, 19 May 2026 08:19:43 -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_locked() 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>
Hi Maxwell
One of those cases where it made us look at code for first time in a while.
The original handling merrily copied garbage data. Let's clean that up whilst
we are here.
Thanks,
Jonathan
> 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;
> }
>
> +/* Meant ONLY for scd30_trigger_handler() */
> +static int scd30_trigger_handler_helper_locked(struct iio_dev *indio_dev,
> + int *scan_data, int arr_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, arr_size);
Why is that a good thing to do if we are in an error condition?
I've have thought the data wasn't valid.
if (ret)
return ret;
memcpy()..
return 0;
Or is there something useful in that data?
Looks like some less than ideal code in the original driver but
I think it makes sense to clean that up now. Just make sure to put
a note in the patch description.
> + 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_locked(indio_dev, scan.data, sizeof(scan.data));
> if (ret)
> goto out;
>
On Wed, May 20, 2026 at 7:28 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 19 May 2026 08:19:43 -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_locked() 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>
> Hi Maxwell
>
> One of those cases where it made us look at code for first time in a while.
> The original handling merrily copied garbage data. Let's clean that up whilst
> we are here.
>
> Thanks,
>
> Jonathan
> > 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;
> > }
> >
> > +/* Meant ONLY for scd30_trigger_handler() */
> > +static int scd30_trigger_handler_helper_locked(struct iio_dev *indio_dev,
> > + int *scan_data, int arr_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, arr_size);
>
> Why is that a good thing to do if we are in an error condition?
> I've have thought the data wasn't valid.
>
> if (ret)
> return ret;
>
> memcpy()..
>
> return 0;
>
> Or is there something useful in that data?
> Looks like some less than ideal code in the original driver but
> I think it makes sense to clean that up now. Just make sure to put
> a note in the patch description.
>
That's likely a result of me being lazy and copy-pasting some of the
code and touching it up so we can make the stack frame leaner. TBH I
would say moving it may be out of scope for this patch? I don't know,
I think it may be worth leaving since the original code did this. I'll
still check the docs later to see if there is actually something
useful in the data.
best regards,
max
>
> > + 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_locked(indio_dev, scan.data, sizeof(scan.data));
> > if (ret)
> > goto out;
> >
>
The "_locked" suffix in scd30_trigger_handler_helper_locked() reads as "caller holds the lock" by the usual kernel convention, but the helper takes state->lock itself via guard(mutex). Dropping the suffix (or renaming to e.g. scd30_read_meas_data()) would match the actual ownership. Also, the int arr_size argument is forwarded straight to memcpy() so it is really a byte count, not an array length -- size_t bytes would describe it more accurately and avoid a future caller passing ARRAY_SIZE(). Stepan
On Tue, 19 May 2026 at 15:28, 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_locked() 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> > --- LGTM, Sashiko also seems to be alright with it. On another note - this driver sure needs cleaning up. Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com> -- Kind regards CJD
On Tue, May 19, 2026 at 8:41 AM Joshua Crofts <joshua.crofts1@gmail.com> wrote: > > On Tue, 19 May 2026 at 15:28, 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_locked() 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> > > --- > > LGTM, Sashiko also seems to be alright with it. > > On another note - this driver sure needs cleaning up. > Agreed. I've stated previously that it may be a good idea to get some newer kernel devs in here since there's a lot of things worth patching in here (when I say newer kernel devs, I don't mean the absolute beginners. should've at least submitted a patch in staging). best regards, max > Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com> > > -- > Kind regards > > CJD
© 2016 - 2026 Red Hat, Inc.