Re: [PATCH v12 3/5] iio: adc: versal-sysmon: add I2C driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Non-atomic I2C read sequence allows bus interleaving, potentially corrupting hardware state or readings.
--

commit 9ab4d88815ee621205c4b3e8a0bb6032534c3baa
Author: Salih Erim <salih.erim@amd.com>

iio: adc: versal-sysmon: add I2C driver

Add an I2C transport driver for the Versal SysMon block. The SysMon
provides an I2C slave interface that allows an external master to
read voltage and temperature measurements through the same register
map used by the MMIO path.

> diff --git a/drivers/iio/adc/versal-sysmon-i2c.c b/drivers/iio/adc/versal-sysmon-i2c.c
> new file mode 100644
> index 0000000000000..e9a7629159ab1
> --- /dev/null
> +++ b/drivers/iio/adc/versal-sysmon-i2c.c

[ ... ]

> +static int sysmon_i2c_reg_read(void *context, unsigned int reg,
> +			       unsigned int *val)
> +{
> +	struct i2c_client *client = context;
> +	u8 write_buf[8] = { };
> +	u8 read_buf[4];
> +	int ret;
> +
> +	put_unaligned_le16(reg >> 2, &write_buf[SYSMON_I2C_REG_OFS]);
> +	write_buf[SYSMON_I2C_INSTR_OFS] = SYSMON_I2C_INSTR_READ;
> +
> +	ret = i2c_master_send(client, write_buf, sizeof(write_buf));
> +	if (ret < 0)
> +		return ret;
> +	if (ret != sizeof(write_buf))
> +		return -EIO;
> +
> +	ret = i2c_master_recv(client, read_buf, sizeof(read_buf));

[Severity: Medium]
Does this split I2C read sequence allow bus interleaving?

Calling i2c_master_send() followed by i2c_master_recv() releases the I2C
adapter lock and generates a STOP condition on the bus between the two calls.

If another I2C transaction is scheduled during this window (for example, from
userspace via /dev/i2c-* or from another driver sharing the bus), it could
interleave with this read sequence.

This breaks the atomicity of the command/response pair, potentially returning
corrupted data or breaking the hardware's state machine.

Could a single i2c_transfer() call with two messages be used here to
generate a Repeated START, or i2c_lock_bus() be used if a STOP condition is
strictly required by the hardware?

> +	if (ret < 0)
> +		return ret;
> +	if (ret != sizeof(read_buf))
> +		return -EIO;
> +
> +	*val = get_unaligned_le32(read_buf);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624143722.4047649-1-salih.erim@amd.com?part=3