Re: [PATCH V9 09/11] iio: imu: inv_icm42607: Add IRQ for icm42607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Double unlock of `st->lock` on the error path in `inv_icm42607_irq_handler()`.
- [High] The threaded interrupt handler will deadlock on the success path due to recursive locking of `st->lock`.
- [High] The shared `st->fifo` state is accessed and modified without adequate lock protection across the read-and-parse transaction.
- [Medium] Premature exposure of the IIO device to userspace before IRQ initialization is complete.
--

commit a6721434d04928fac24f103459562cfa31a2c977
Author: Chris Morgan <macromorgan@hotmail.com>

iio: imu: inv_icm42607: Add IRQ for icm42607
    
Add IRQ support for the icm42607 driver.

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index 6b623fb679f34..3c91623dffb29 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
[ ... ]
> @@ -331,6 +331,91 @@ static int inv_icm42607_setup(struct inv_icm42607_state *st,
[ ... ]
> +static irqreturn_t inv_icm42607_irq_handler(int irq, void *_data)
> +{
> +	struct inv_icm42607_state *st = _data;
> +	struct device *dev = regmap_get_device(st->map);
> +	unsigned int status;
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +
> +	ret = regmap_read(st->map, INV_ICM42607_REG_INT_STATUS, &status);
> +	if (ret) {
> +		dev_err(dev, "Interrut status read error %d\n", ret);
> +		goto out_unlock;
> +	}
> +
> +	if (status & INV_ICM42607_INT_STATUS_FIFO_FULL)
> +		dev_warn(dev, "FIFO full data lost!\n");
> +
> +	if (status & INV_ICM42607_INT_STATUS_FIFO_THS) {
> +		mutex_unlock(&st->lock);
> +		ret = inv_icm42607_buffer_fifo_read(st, 0);
> +		if (ret) {
> +			dev_err(dev, "FIFO read error %d\n", ret);
> +			goto out_unlock;

[Severity: High]
If inv_icm42607_buffer_fifo_read() fails here, the code jumps to out_unlock
while st->lock is already unlocked. Will this cause a double unlock of the
mutex at the out_unlock label?

> +		}
> +
> +		mutex_lock(&st->lock);

[Severity: High]
Will dropping the lock between the read and parse operations expose a race
window?

If a concurrent call to inv_icm42607_buffer_hwfifo_flush() interleaves during
this unlocked window, it could issue another read and overwrite the contents
of st->fifo.data and the sample counts. When the interrupt handler regains
the lock and calls inv_icm42607_buffer_fifo_parse(), it might parse the
overwritten data instead of the data it originally read.

> +		ret = inv_icm42607_buffer_fifo_parse(st);

[Severity: High]
Can this cause a deadlock on the success path?

inv_icm42607_buffer_fifo_parse() eventually delegates to
inv_icm42607_accel_parse_fifo(), which uses guard(mutex)(&st->lock) to
acquire the exact same lock. Since the lock is already acquired here, won't
this result in recursive locking?

> +		if (ret)
> +			dev_err(dev, "FIFO parsing error %d\n", ret);
> +	}
> +
> +out_unlock:
> +	mutex_unlock(&st->lock);

[Severity: High]
As mentioned above, if we jump here after failing
inv_icm42607_buffer_fifo_read(), the lock is already unlocked, causing a
double unlock error.

> +	return IRQ_HANDLED;
> +}
[ ... ]
> @@ -433,6 +523,11 @@ int inv_icm42607_core_probe(struct regmap *regmap,
>  	if (IS_ERR(st->indio_accel))
>  		return PTR_ERR(st->indio_accel);
>  
> +	/* Initialize interrupt handling */
> +	ret = inv_icm42607_irq_init(st, irq, irq_type, open_drain);

[Severity: Medium]
Is it safe to initialize the interrupt handling after registering the IIO
device?

inv_icm42607_accel_init() registers the device, exposing it to userspace. If
userspace opens the device and attempts to interact with it before the
interrupt is configured, or if inv_icm42607_irq_init() fails causing devres
cleanup, could this lead to issues with an actively exposed device?

> +	if (ret)
> +		return ret;
> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260530031739.109063-1-macroalpha82@gmail.com?part=9