Replace explicit mutex_lock() and mutex_unlock() with the guard() macro
for cleaner and safer mutex handling.
Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c | 11 ++----
drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c | 27 ++++++--------
drivers/iio/imu/inv_icm42600/inv_icm42600_core.c | 43 +++++++++-------------
drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c | 11 ++----
4 files changed, 36 insertions(+), 56 deletions(-)
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
index 7a28051330b79098bfa94b8c8c78c2bce20b7230..1e58253b3ddc0028d0899ec04cabbc65c6bf2b72 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
@@ -562,11 +562,10 @@ static int inv_icm42600_accel_write_scale(struct iio_dev *indio_dev,
conf.fs = idx / 2;
pm_runtime_get_sync(dev);
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = inv_icm42600_set_accel_conf(st, &conf, NULL);
- mutex_unlock(&st->lock);
pm_runtime_mark_last_busy(dev);
pm_runtime_put_autosuspend(dev);
@@ -993,13 +992,11 @@ static int inv_icm42600_accel_hwfifo_set_watermark(struct iio_dev *indio_dev,
struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
st->fifo.watermark.accel = val;
ret = inv_icm42600_buffer_update_watermark(st);
- mutex_unlock(&st->lock);
-
return ret;
}
@@ -1012,14 +1009,12 @@ static int inv_icm42600_accel_hwfifo_flush(struct iio_dev *indio_dev,
if (count == 0)
return 0;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = inv_icm42600_buffer_hwfifo_flush(st, count);
if (!ret)
ret = st->fifo.nb.accel;
- mutex_unlock(&st->lock);
-
return ret;
}
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
index 7c4ed981db043b4e8f3967b0802655d34f863954..d052b7069dc5c99dfea634415c107e188994c995 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
@@ -292,9 +292,8 @@ static int inv_icm42600_buffer_preenable(struct iio_dev *indio_dev)
pm_runtime_get_sync(dev);
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
inv_sensors_timestamp_reset(ts);
- mutex_unlock(&st->lock);
return 0;
}
@@ -308,7 +307,7 @@ static int inv_icm42600_buffer_postenable(struct iio_dev *indio_dev)
struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
/* exit if FIFO is already on */
if (st->fifo.on) {
@@ -320,30 +319,29 @@ static int inv_icm42600_buffer_postenable(struct iio_dev *indio_dev)
ret = regmap_set_bits(st->map, INV_ICM42600_REG_INT_SOURCE0,
INV_ICM42600_INT_SOURCE0_FIFO_THS_INT1_EN);
if (ret)
- goto out_unlock;
+ return ret;
/* flush FIFO data */
ret = regmap_write(st->map, INV_ICM42600_REG_SIGNAL_PATH_RESET,
INV_ICM42600_SIGNAL_PATH_RESET_FIFO_FLUSH);
if (ret)
- goto out_unlock;
+ return ret;
/* set FIFO in streaming mode */
ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
INV_ICM42600_FIFO_CONFIG_STREAM);
if (ret)
- goto out_unlock;
+ return ret;
/* workaround: first read of FIFO count after reset is always 0 */
ret = regmap_bulk_read(st->map, INV_ICM42600_REG_FIFO_COUNT, st->buffer, 2);
if (ret)
- goto out_unlock;
+ return ret;
out_on:
/* increase FIFO on counter */
st->fifo.on++;
-out_unlock:
- mutex_unlock(&st->lock);
+
return ret;
}
@@ -352,7 +350,7 @@ static int inv_icm42600_buffer_predisable(struct iio_dev *indio_dev)
struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
/* exit if there are several sensors using the FIFO */
if (st->fifo.on > 1) {
@@ -364,25 +362,24 @@ static int inv_icm42600_buffer_predisable(struct iio_dev *indio_dev)
ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
INV_ICM42600_FIFO_CONFIG_BYPASS);
if (ret)
- goto out_unlock;
+ return ret;
/* flush FIFO data */
ret = regmap_write(st->map, INV_ICM42600_REG_SIGNAL_PATH_RESET,
INV_ICM42600_SIGNAL_PATH_RESET_FIFO_FLUSH);
if (ret)
- goto out_unlock;
+ return ret;
/* disable FIFO threshold interrupt */
ret = regmap_clear_bits(st->map, INV_ICM42600_REG_INT_SOURCE0,
INV_ICM42600_INT_SOURCE0_FIFO_THS_INT1_EN);
if (ret)
- goto out_unlock;
+ return ret;
out_off:
/* decrease FIFO on counter */
st->fifo.on--;
-out_unlock:
- mutex_unlock(&st->lock);
+
return ret;
}
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
index 52f61e90fec3e08effc1d398293bece5413406d1..82e6c8b3fba5b63d4eebb0a68bd88b950dbe881d 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
@@ -441,15 +441,13 @@ int inv_icm42600_debugfs_reg(struct iio_dev *indio_dev, unsigned int reg,
struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
if (readval)
ret = regmap_read(st->map, reg, readval);
else
ret = regmap_write(st->map, reg, writeval);
- mutex_unlock(&st->lock);
-
return ret;
}
@@ -820,20 +818,21 @@ static int inv_icm42600_suspend(struct device *dev)
int accel_conf;
int ret = 0;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
st->suspended.gyro = st->conf.gyro.mode;
st->suspended.accel = st->conf.accel.mode;
st->suspended.temp = st->conf.temp_en;
- if (pm_runtime_suspended(dev))
- goto out_unlock;
+ ret = pm_runtime_suspended(dev);
+ if (ret)
+ return ret;
/* disable FIFO data streaming */
if (st->fifo.on) {
ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
INV_ICM42600_FIFO_CONFIG_BYPASS);
if (ret)
- goto out_unlock;
+ return ret;
}
/* keep chip on and wake-up capable if APEX and wakeup on */
@@ -849,7 +848,7 @@ static int inv_icm42600_suspend(struct device *dev)
if (st->apex.wom.enable) {
ret = inv_icm42600_disable_wom(st);
if (ret)
- goto out_unlock;
+ return ret;
}
accel_conf = INV_ICM42600_SENSOR_MODE_OFF;
}
@@ -857,14 +856,12 @@ static int inv_icm42600_suspend(struct device *dev)
ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
accel_conf, false, NULL);
if (ret)
- goto out_unlock;
+ return ret;
/* disable vddio regulator if chip is sleeping */
if (!wakeup)
regulator_disable(st->vddio_supply);
-out_unlock:
- mutex_unlock(&st->lock);
return ret;
}
@@ -881,10 +878,11 @@ static int inv_icm42600_resume(struct device *dev)
bool wakeup;
int ret = 0;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
- if (pm_runtime_suspended(dev))
- goto out_unlock;
+ ret = pm_runtime_suspended(dev);
+ if (ret)
+ return ret;
/* check wakeup capability */
accel_dev = &st->indio_accel->dev;
@@ -896,7 +894,7 @@ static int inv_icm42600_resume(struct device *dev)
} else {
ret = inv_icm42600_enable_regulator_vddio(st);
if (ret)
- goto out_unlock;
+ return ret;
}
/* restore sensors state */
@@ -904,13 +902,13 @@ static int inv_icm42600_resume(struct device *dev)
st->suspended.accel,
st->suspended.temp, NULL);
if (ret)
- goto out_unlock;
+ return ret;
/* restore APEX features if disabled */
if (!wakeup && st->apex.wom.enable) {
ret = inv_icm42600_enable_wom(st);
if (ret)
- goto out_unlock;
+ return ret;
}
/* restore FIFO data streaming */
@@ -921,8 +919,6 @@ static int inv_icm42600_resume(struct device *dev)
INV_ICM42600_FIFO_CONFIG_STREAM);
}
-out_unlock:
- mutex_unlock(&st->lock);
return ret;
}
@@ -932,19 +928,17 @@ static int inv_icm42600_runtime_suspend(struct device *dev)
struct inv_icm42600_state *st = dev_get_drvdata(dev);
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
/* disable all sensors */
ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
INV_ICM42600_SENSOR_MODE_OFF, false,
NULL);
if (ret)
- goto error_unlock;
+ return ret;
regulator_disable(st->vddio_supply);
-error_unlock:
- mutex_unlock(&st->lock);
return ret;
}
@@ -954,11 +948,10 @@ static int inv_icm42600_runtime_resume(struct device *dev)
struct inv_icm42600_state *st = dev_get_drvdata(dev);
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = inv_icm42600_enable_regulator_vddio(st);
- mutex_unlock(&st->lock);
return ret;
}
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
index 9ba6f13628e6af5e19d047476ae93754f07aa95f..875e48748d70678bcd3963187be6de8c5a2b2fcc 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
@@ -278,11 +278,10 @@ static int inv_icm42600_gyro_write_scale(struct iio_dev *indio_dev,
conf.fs = idx / 2;
pm_runtime_get_sync(dev);
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = inv_icm42600_set_gyro_conf(st, &conf, NULL);
- mutex_unlock(&st->lock);
pm_runtime_mark_last_busy(dev);
pm_runtime_put_autosuspend(dev);
@@ -695,13 +694,11 @@ static int inv_icm42600_gyro_hwfifo_set_watermark(struct iio_dev *indio_dev,
struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
st->fifo.watermark.gyro = val;
ret = inv_icm42600_buffer_update_watermark(st);
- mutex_unlock(&st->lock);
-
return ret;
}
@@ -714,14 +711,12 @@ static int inv_icm42600_gyro_hwfifo_flush(struct iio_dev *indio_dev,
if (count == 0)
return 0;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = inv_icm42600_buffer_hwfifo_flush(st, count);
if (!ret)
ret = st->fifo.nb.gyro;
- mutex_unlock(&st->lock);
-
return ret;
}
--
2.50.1On Fri, Aug 8, 2025 at 5:58 PM Sean Nyekjaer <sean@geanix.com> wrote: > > Replace explicit mutex_lock() and mutex_unlock() with the guard() macro > for cleaner and safer mutex handling. ... > pm_runtime_get_sync(dev); > - mutex_lock(&st->lock); > + guard(mutex)(&st->lock); > > ret = inv_icm42600_set_accel_conf(st, &conf, NULL); > > - mutex_unlock(&st->lock); > pm_runtime_mark_last_busy(dev); > pm_runtime_put_autosuspend(dev); This makes PM calls under the mutex. In some cases it may lead to deadlocks. I think you wanted to use scoped_guard() here and in similar cases. ... > struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev); > int ret; > > - mutex_lock(&st->lock); > + guard(mutex)(&st->lock); > > st->fifo.watermark.accel = val; > ret = inv_icm42600_buffer_update_watermark(st); > > - mutex_unlock(&st->lock); > - > return ret; Now remove ret and use return directly. > } > - mutex_lock(&st->lock); > + guard(mutex)(&st->lock); > > ret = inv_icm42600_buffer_hwfifo_flush(st, count); > if (!ret) > ret = st->fifo.nb.accel; > > - mutex_unlock(&st->lock); > - > return ret; In the similar way as above. ret = _flush(); if (ret) return ret; return ...nb.accel; ... > struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev); > int ret; Now unneeded, just return directly. > - mutex_lock(&st->lock); > + guard(mutex)(&st->lock); > > if (readval) > ret = regmap_read(st->map, reg, readval); > else > ret = regmap_write(st->map, reg, writeval); > > - mutex_unlock(&st->lock); > - > return ret; ... > int ret = 0; Now unneeded assignment. > - mutex_lock(&st->lock); > + guard(mutex)(&st->lock); > > st->suspended.gyro = st->conf.gyro.mode; > st->suspended.accel = st->conf.accel.mode; > st->suspended.temp = st->conf.temp_en; > - if (pm_runtime_suspended(dev)) > - goto out_unlock; > + ret = pm_runtime_suspended(dev); > + if (ret) > + return ret; ... > /* disable vddio regulator if chip is sleeping */ > if (!wakeup) > regulator_disable(st->vddio_supply); > > -out_unlock: > - mutex_unlock(&st->lock); > return ret; Now return 0 to make it clear that this is a success. ... > @@ -881,10 +878,11 @@ static int inv_icm42600_resume(struct device *dev) > bool wakeup; > int ret = 0; Assignment is useless now. > - mutex_lock(&st->lock); > + guard(mutex)(&st->lock); > > - if (pm_runtime_suspended(dev)) > - goto out_unlock; > + ret = pm_runtime_suspended(dev); > + if (ret) > + return ret; ... > -out_unlock: > - mutex_unlock(&st->lock); > return ret; return 0; ? ... > regulator_disable(st->vddio_supply); > > -error_unlock: > - mutex_unlock(&st->lock); > return ret; Ditto. > } ... > int ret; Now useless variable. > > - mutex_lock(&st->lock); > + guard(mutex)(&st->lock); > > ret = inv_icm42600_enable_regulator_vddio(st); > > - mutex_unlock(&st->lock); > return ret; > } ... > int ret; Ditto. > - mutex_lock(&st->lock); > + guard(mutex)(&st->lock); > > st->fifo.watermark.gyro = val; > ret = inv_icm42600_buffer_update_watermark(st); > > - mutex_unlock(&st->lock); > - > return ret; > } ... > - mutex_lock(&st->lock); > + guard(mutex)(&st->lock); > > ret = inv_icm42600_buffer_hwfifo_flush(st, count); > if (!ret) > ret = st->fifo.nb.gyro; Invert conditional and return ret directly. > - mutex_unlock(&st->lock); > - > return ret; -- With Best Regards, Andy Shevchenko
On Fri, Aug 08, 2025 at 11:52:35PM +0100, Andy Shevchenko wrote: > On Fri, Aug 8, 2025 at 5:58 PM Sean Nyekjaer <sean@geanix.com> wrote: > > > > Replace explicit mutex_lock() and mutex_unlock() with the guard() macro > > for cleaner and safer mutex handling. > > ... > > > pm_runtime_get_sync(dev); > > - mutex_lock(&st->lock); > > + guard(mutex)(&st->lock); > > > > ret = inv_icm42600_set_accel_conf(st, &conf, NULL); > > > > - mutex_unlock(&st->lock); > > pm_runtime_mark_last_busy(dev); > > pm_runtime_put_autosuspend(dev); > > This makes PM calls under the mutex. In some cases it may lead to deadlocks. > I think you wanted to use scoped_guard() here and in similar cases. Oh, good catch :) > > ... > > > struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev); > > int ret; > > > > - mutex_lock(&st->lock); > > + guard(mutex)(&st->lock); > > > > st->fifo.watermark.accel = val; > > ret = inv_icm42600_buffer_update_watermark(st); > > > > - mutex_unlock(&st->lock); > > - > > return ret; > > Now remove ret and use return directly. > > > } > > > - mutex_lock(&st->lock); > > + guard(mutex)(&st->lock); > > > > ret = inv_icm42600_buffer_hwfifo_flush(st, count); > > if (!ret) > > ret = st->fifo.nb.accel; > > > > - mutex_unlock(&st->lock); > > - > > return ret; > > In the similar way as above. > > ret = _flush(); > if (ret) > return ret; > > return ...nb.accel; > > ... > > > struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev); > > > int ret; > > Now unneeded, just return directly. > > > - mutex_lock(&st->lock); > > + guard(mutex)(&st->lock); > > > > if (readval) > > ret = regmap_read(st->map, reg, readval); > > else > > ret = regmap_write(st->map, reg, writeval); > > > > - mutex_unlock(&st->lock); > > - > > return ret; > > ... > > > int ret = 0; > > Now unneeded assignment. > > > - mutex_lock(&st->lock); > > + guard(mutex)(&st->lock); > > > > st->suspended.gyro = st->conf.gyro.mode; > > st->suspended.accel = st->conf.accel.mode; > > st->suspended.temp = st->conf.temp_en; > > - if (pm_runtime_suspended(dev)) > > - goto out_unlock; > > + ret = pm_runtime_suspended(dev); > > + if (ret) > > + return ret; > > ... > > > /* disable vddio regulator if chip is sleeping */ > > if (!wakeup) > > regulator_disable(st->vddio_supply); > > > > -out_unlock: > > - mutex_unlock(&st->lock); > > return ret; > > Now return 0 to make it clear that this is a success. > > ... > > > @@ -881,10 +878,11 @@ static int inv_icm42600_resume(struct device *dev) > > bool wakeup; > > int ret = 0; > > Assignment is useless now. > > > - mutex_lock(&st->lock); > > + guard(mutex)(&st->lock); > > > > - if (pm_runtime_suspended(dev)) > > - goto out_unlock; > > + ret = pm_runtime_suspended(dev); > > + if (ret) > > + return ret; > > ... > > > -out_unlock: > > - mutex_unlock(&st->lock); > > return ret; > > return 0; > > ? > > ... > > > regulator_disable(st->vddio_supply); > > > > -error_unlock: > > - mutex_unlock(&st->lock); > > return ret; > > Ditto. > > > } > > ... > > > int ret; > > Now useless variable. > > > > > - mutex_lock(&st->lock); > > + guard(mutex)(&st->lock); > > > > ret = inv_icm42600_enable_regulator_vddio(st); > > > > - mutex_unlock(&st->lock); > > return ret; > > } > > ... > > > int ret; > > Ditto. > > > - mutex_lock(&st->lock); > > + guard(mutex)(&st->lock); > > > > st->fifo.watermark.gyro = val; > > ret = inv_icm42600_buffer_update_watermark(st); > > > > - mutex_unlock(&st->lock); > > - > > return ret; > > } > > ... > > > - mutex_lock(&st->lock); > > + guard(mutex)(&st->lock); > > > > ret = inv_icm42600_buffer_hwfifo_flush(st, count); > > if (!ret) > > ret = st->fifo.nb.gyro; > > Invert conditional and return ret directly. > > > - mutex_unlock(&st->lock); > > - > > return ret; > > -- > With Best Regards, > Andy Shevchenko Thanks for the review. /Sean
© 2016 - 2025 Red Hat, Inc.