| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <fbea7d45-bf92-4f6b-a464-0f7a6f921bde@baylibre.com>
Date: Fri, 5 Sep 2025 14:45:52 -0500
From: David Lechner <dlechner@...libre.com>
To: Sean Nyekjaer <sean@...nix.com>,
Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@....com>, rafael@...nel.org,
Jonathan Cameron <jic23@...nel.org>, Nuno Sá
<nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-pm@...r.kernel.org, Jean-Baptiste Maneyrol <jmaneyrol@...ensense.com>,
Jonathan Cameron <Jonathan.Cameron@...wei.com>
Subject: Re: [PATCH v3 5/5] iio: imu: inv_icm42600: use guard() to release
mutexes
On 9/1/25 2:49 AM, Sean Nyekjaer wrote:
> Replace explicit mutex_lock() and mutex_unlock() with the guard() macro
> for cleaner and safer mutex handling.
>
> Signed-off-by: Sean Nyekjaer <sean@...nix.com>
> ---
> drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c | 25 +++------
> drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c | 27 ++++-----
> drivers/iio/imu/inv_icm42600/inv_icm42600_core.c | 65 +++++++++-------------
> drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c | 20 +++----
> 4 files changed, 55 insertions(+), 82 deletions(-)
>
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> index 48014b61ced335eb2c8549cfc2e79ccde1934308..fbed6974ef04ac003c9b7bd38f87bd77f4b55509 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> @@ -561,11 +561,11 @@ 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);
>
> - ret = inv_icm42600_set_accel_conf(st, &conf, NULL);
> + scoped_guard(mutex, &st->lock) {
> + ret = inv_icm42600_set_accel_conf(st, &conf, NULL);
> + }
Don't need braces here.
>
> - mutex_unlock(&st->lock);
> pm_runtime_put_autosuspend(dev);
>
> return ret;
...
> @@ -299,7 +298,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) {
> @@ -311,30 +310,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++;
I would be tempted to get rid of out_on as well even if we have to repeat
`st->fifo.on++;` in two places.
> -out_unlock:
> - mutex_unlock(&st->lock);
> +
> return ret;
Can just return 0 here and simplify if (st->fifo.on).
> }
>
> @@ -343,7 +341,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) {
> @@ -355,25 +353,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;
Same comments apply here.
> }
>
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index 4bf436c46f1cfd7e7e1bb911d94a0a566d63e791..4db8bc68075a30c59e6e358bb0b73b1e6b9175ea 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -439,18 +439,13 @@ int inv_icm42600_debugfs_reg(struct iio_dev *indio_dev, unsigned int reg,
> unsigned int writeval, unsigned int *readval)
> {
> 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);
> + return regmap_read(st->map, reg, readval);
> else
Don't need the `else` anymore.
> - ret = regmap_write(st->map, reg, writeval);
> -
> - mutex_unlock(&st->lock);
> -
> - return ret;
> + return regmap_write(st->map, reg, writeval);
> }
>
> static int inv_icm42600_set_conf(struct inv_icm42600_state *st,
> @@ -820,22 +815,23 @@ static int inv_icm42600_suspend(struct device *dev)
> struct device *accel_dev;
> bool wakeup;
> int accel_conf;
> - int ret = 0;
> + int ret;
>
> - 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;
pm_runtime_suspended() returns a bool, so this doesn't make sense.
Probably should be:
if (pm_runtime_suspended(dev))
return 0;
>
> /* 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 */
> @@ -851,7 +847,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;
> }
> @@ -859,15 +855,13 @@ 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;
> + return 0;
> }
>
> /*
> @@ -881,12 +875,13 @@ static int inv_icm42600_resume(struct device *dev)
> struct inv_icm42600_sensor_state *accel_st = iio_priv(st->indio_accel);
> struct device *accel_dev;
> bool wakeup;
> - int ret = 0;
> + int ret;
>
> - 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;
same here.
>
> /* check wakeup capability */
> accel_dev = &st->indio_accel->dev;
...
> @@ -690,13 +690,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);
Can return directly now.
>
> - mutex_unlock(&st->lock);
> -
> return ret;
> }
>
Powered by blists - more mailing lists