Re: [PATCH v11 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:
- [High] Non-atomic I2C read transaction releases the bus lock, making it vulnerable to bus interleaving.
--

commit 0b46526090ca97299b6d37544af48b39e72bb565
Author: Salih Erim <salih.erim@amd.com>

iio: adc: versal-sysmon: add I2C driver

This commit adds an I2C transport driver for the Versal SysMon block,
allowing external masters to read voltage and temperature measurements. It
implements a regmap backend using I2C but does not support events due to
the lack of an interrupt line.

> diff --git a/drivers/iio/adc/versal-sysmon-i2c.c b/drivers/iio/adc/versal-sysmon-i2c.c
> new file mode 100644
> --- /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: High]
Does this sequence allow another device to interleave its I2C traffic on a
shared bus?

Looking at sysmon_i2c_reg_read(), the register read operation is split into
separate i2c_master_send() and i2c_master_recv() calls.

Because i2c_master_send() releases the I2C adapter bus lock before returning,
another kernel thread can perform a bus transaction between these two calls.

If an intervening bus transaction occurs, could it reset the sysmon device's
I2C state machine, discarding the previously written command payload and
corrupting the subsequent read?

Would using i2c_transfer() to perform both the write and read messages as a
single atomic transaction prevent this interleaving?

> +	if (ret < 0)
> +		return ret;
> +	if (ret != sizeof(read_buf))
> +		return -EIO;
[ ... ]

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