Re: [PATCH V9 06/11] iio: imu: inv_icm42607: Add Buffer support for icm42607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Regmap write is executed after the hardware power has already been disabled, causing bus timeouts or crashes.
- [High] Missing `__packed` attribute on `struct inv_icm42607_fifo_sensor_data` causes an unaligned memory access fault on strict architectures.
- [High] `st->fifo.count` read from the hardware is not rounded down to a multiple of `packet_size` before reading from the hardware FIFO, permanently desynchronizing the FIFO stream if a partial packet is present.
- [Medium] `get_unaligned(&packet)` is misused to cast a pointer, failing to perform an unaligned read of the actual struct fields.

Pre-existing issues:
- [Critical] System suspend and resume callbacks inherently deadlock against their runtime PM equivalents due to recursive locking of the same non-recursive mutex.
--

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

iio: imu: inv_icm42607: Add Buffer support for icm42607

Add all FIFO parsing and reading functions to support inv_icm42607 hardware.

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.h b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.h
> new file mode 100644
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.h

[ ... ]

> +/* FIFO data packet */
> +struct inv_icm42607_fifo_sensor_data {
> +	__be16 x;
> +	__be16 y;
> +	__be16 z;
> +};

[Severity: High]
Does this struct need a __packed attribute?

Because this struct is embedded inside the __packed packets at odd byte offsets
(like inv_icm42607_fifo_1sensor_packet), leaving it unpacked means the compiler
assumes normal 2-byte alignment for its fields. 

When these fields are later accessed in inv_icm42607_fifo_is_data_valid()
via be16_to_cpu(), could this trigger a fatal alignment fault on architectures
lacking hardware support for unaligned accesses?

> +
> +#define INV_ICM42607_FIFO_DATA_INVALID		-32768

[ ... ]

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c

[ ... ]

> +ssize_t inv_icm42607_fifo_decode_packet(const void *packet, const void **accel,
> +					const void **gyro, const int8_t **temp,
> +					const void **timestamp)
> +{
> +	const struct inv_icm42607_fifo_1sensor_packet *pack1 = get_unaligned(&packet);
> +	const struct inv_icm42607_fifo_2sensors_packet *pack2 = get_unaligned(&packet);

[Severity: Medium]
Are these get_unaligned() calls working as intended?

By taking the address of the pointer argument (&packet), get_unaligned() simply
reads the pointer value off the stack itself. It does not mark the resulting
pointer for safe unaligned access of the actual struct fields in the data
buffer. 

Is there a better way to safely cast these pointers?

> +	u8 header = *((const u8 *)packet);

[ ... ]

> +	/* check and clamp FIFO count value */
> +	if (st->fifo.count == 0)
> +		return 0;
> +
> +	st->fifo.count = min(st->fifo.count, max_count);
> +
> +	/* read all FIFO data in internal buffer */
> +	ret = regmap_noinc_read(st->map, INV_ICM42607_REG_FIFO_DATA,
> +				st->fifo.data, st->fifo.count);

[Severity: High]
Does st->fifo.count need to be rounded down to a multiple of the packet size
before reading from the hardware FIFO?

If a read races with the hardware writing a new packet, the reported count
might include a partial packet. If this unrounded count is passed directly to
regmap_noinc_read(), those partial bytes are permanently popped from the
hardware FIFO.

While the decode loop will safely exit when returning -EIO and discarding the
partial bytes, won't the next interrupt resume reading from the halfway point
of that packet? 

Could this permanently desynchronize the stream by causing the decode function
to interpret random data bytes as headers?

> +	if (ret)
> +		return ret;

[ ... ]

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c

[ ... ]

> @@ ... @@
>  static int inv_icm42607_suspend(struct device *dev)
>  {
>  	struct inv_icm42607_state *st = dev_get_drvdata(dev);
>  	int ret;
>  
>  	guard(mutex)(&st->lock);
>  
>  	if (pm_runtime_suspended(dev))
>  		return 0;
>  
>  	ret = pm_runtime_force_suspend(dev);

[Severity: Critical]
This is a pre-existing issue, but can this system suspend callback deadlock
against its runtime PM equivalent?

In inv_icm42607_suspend(), the driver locks the main state mutex via
guard(mutex)(&st->lock). While holding this lock, it calls
pm_runtime_force_suspend(dev).

If the device is not already suspended, the PM core synchronously executes the
device's runtime suspend callback, inv_icm42607_runtime_suspend(), which
immediately attempts to acquire the exact same st->lock via its own
guard(mutex)(&st->lock). 

Because Linux kernel mutexes are not recursive, does this result in a
guaranteed deadlock on every system suspend transition where the device was
active?

>  	if (ret)
>  		return ret;
>  
> +	if (st->fifo.on) {
> +		ret = regmap_write(st->map, INV_ICM42607_REG_FIFO_CONFIG1,
> +				   INV_ICM42607_FIFO_CONFIG1_BYPASS);

[Severity: High]
Can this regmap_write() timeout or crash the bus by accessing a powered-off
device?

The pm_runtime_force_suspend() call immediately preceding this block delegates
to the driver's runtime suspend callback, which actively powers down the device
by turning off pwr_mgmt0 and disabling the vddio regulator. 

When this new regmap_write() executes, the I/O power to the chip has already
been severed.

> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = inv_icm42607_set_pwr_mgmt0(st, INV_ICM42607_SENSOR_MODE_OFF,
>  					 INV_ICM42607_SENSOR_MODE_OFF,
>  					 false, NULL);

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