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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] I2C transfer buffers are allocated on the stack, which can cause a kernel panic if the underlying I2C adapter driver uses DMA.
- [High] I2C regmap backend is executed in hardirq and spinlock context if an IRQ is erroneously specified in the device tree, causing a 'scheduling while atomic' panic.
--

commit f508dce27e50e63598a76da221a58f66b1a7c5f1
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
> @@ -0,0 +1,134 @@
[ ... ]
> +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];

[Severity: High]
Is it safe to pass these stack-allocated buffers to i2c_master_send() and
i2c_master_recv()?

If the underlying I2C adapter driver uses DMA for the transfer, it will
attempt to map these addresses using the DMA API. With CONFIG_VMAP_STACK
enabled, kernel stacks are placed in vmalloc space which cannot be mapped
for DMA, leading to a kernel panic.

Should these buffers be dynamically allocated, or perhaps embedded in a
device-managed structure? This same pattern appears to be present in
sysmon_i2c_reg_write() as well.

> +	int ret;
> +
> +	put_unaligned_le16(reg >> 2, &write_buf[SYSMON_I2C_REG_OFS]);
> +	write_buf[SYSMON_I2C_INSTR_OFS] = SYSMON_I2C_INSTR_READ;
[ ... ]
> +static int sysmon_i2c_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct regmap *regmap;
> +
> +	regmap = devm_regmap_init(dev, NULL, client, &sysmon_i2c_regmap_config);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	/* I2C has no IRQ connection; events are not supported */
> +	return devm_versal_sysmon_core_probe(dev, regmap);

[Severity: High]
Does this code adequately prevent the core driver from registering an IRQ?

The commit message notes that events are not supported because the I2C regmap
backend cannot be called from atomic context. However,
devm_versal_sysmon_core_probe() unconditionally fetches the IRQ from the
device tree:

devm_versal_sysmon_core_probe()
    irq = fwnode_irq_get(dev_fwnode(dev), 0);
    if (irq > 0) {
        ...
        sysmon_init_interrupt(sysmon, dev, indio_dev, irq);
    }

If a misconfigured device tree provides an interrupts property for the I2C
node, the core driver will successfully register the hardirq handler
sysmon_iio_irq(). When the interrupt fires, the handler takes sysmon->irq_lock
(which is a spinlock) and calls regmap_read(). This routes back to
sysmon_i2c_reg_read(). Because the I2C transfer functions can sleep, this
would result in a 'scheduling while atomic' panic.

Could the I2C probe explicitly inform the core to bypass IRQ initialization,
instead of relying solely on the device tree being configured correctly?

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