Re: [PATCH v8 3/5] mfd: aaeon: Add SRG-IMX8P MCU driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The driver unconditionally sets the I2C_M_DMA_SAFE flag on the caller-provided val_buf in aaeon_mcu_regmap_read, bypassing the I2C core's bounce buffering. If a child driver calls regmap_bulk_read() with a stack-allocated buffer for a volatile register (e.g., GPIO inputs), regmap passes the stack buffer directly to aaeon_mcu_regmap_read. This results in the I2C host controller performing DMA directly into a stack memory address, leading to cacheline sharing corruption or a kernel panic (especially on VMAP_STACK configurations).
- [High] The driver implements the MCU's command-response sequence using two separate i2c_transfer() calls without explicitly locking the I2C bus. Since i2c_transfer() internally acquires and releases the I2C adapter lock per call, there is a race window between the command write and the response read. If another device driver on the same I2C bus initiates a transfer during this window, its transaction will be physically interleaved on the bus, disrupting the MCU's state machine.
- [Low] The driver-specific header file aaeon-mcu.h is incorrectly placed in the global include/linux/mfd/ directory, violating MFD subsystem guidelines.
--

--- Patch [3]: [PATCH v8 3/5] mfd: aaeon: Add SRG-IMX8P MCU driver ---
commit 83256074ae86bd1d0d3b7e6e2bf9afe879bd3c73
Author: Thomas Perrot (Schneider Electric) <thomas.perrot@bootlin.com>

mfd: aaeon: Add SRG-IMX8P MCU driver

This patch adds a Multi-Function Device (MFD) driver for the Aaeon
SRG-IMX8P embedded controller. It implements a custom regmap bus over
I2C to match the MCU's command format and registers child devices.

> diff --git a/drivers/mfd/aaeon-mcu.c b/drivers/mfd/aaeon-mcu.c
> new file mode 100644
> index 0000000000000..c06c99d02d648
> --- /dev/null
> +++ b/drivers/mfd/aaeon-mcu.c

[ ... ]

> +static int aaeon_mcu_regmap_write(void *context, const void *data, size_t count)
> +{

[ ... ]

> +	ret = i2c_transfer(client->adapter, &write_msg, 1);
> +	if (ret < 0)
> +		return ret;
> +	if (ret != 1)
> +		return -EIO;
> +
> +	ret = i2c_transfer(client->adapter, &response_msg, 1);

[Severity: High]
Since i2c_transfer() internally acquires and releases the I2C adapter lock
per call, does issuing two separate transfers here leave a race window?

If another device driver on the same I2C bus initiates a transfer during
this window, its transaction could be physically interleaved on the bus.
Would it be safer to use i2c_lock_bus() and __i2c_transfer() to maintain
the lock across both the command write and response read?

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

[ ... ]

> +static int aaeon_mcu_regmap_read(void *context, const void *reg_buf,
> +				 size_t reg_size, void *val_buf, size_t val_size)
> +{

[ ... ]

> +	read_msg.addr  = client->addr;
> +	read_msg.flags = I2C_M_RD | I2C_M_DMA_SAFE;
> +	read_msg.buf   = val_buf;

[Severity: High]
Can val_buf point to a stack-allocated buffer?

When a child driver calls regmap_bulk_read() for a volatile register (like
GPIO inputs), regmap might pass a stack-allocated buffer directly to this
function. By unconditionally setting I2C_M_DMA_SAFE, doesn't this force
the I2C core to bypass bounce buffering?

If the I2C host controller then performs DMA directly into a stack memory
address, this could lead to cacheline sharing corruption or a kernel
panic, especially on VMAP_STACK configurations.

> +	read_msg.len   = val_size;
> +
> +	ret = i2c_transfer(client->adapter, &write_msg, 1);
> +	if (ret < 0)
> +		return ret;
> +	if (ret != 1)
> +		return -EIO;
> +
> +	ret = i2c_transfer(client->adapter, &read_msg, 1);

[Severity: High]
Similar to the write path above, does releasing the I2C adapter lock
between the write command and the read response risk interleaving transfers
with other devices on the same I2C bus?

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

[ ... ]

> diff --git a/include/linux/mfd/aaeon-mcu.h b/include/linux/mfd/aaeon-mcu.h
> new file mode 100644
> index 0000000000000..3a1aeec85d60b
> --- /dev/null
> +++ b/include/linux/mfd/aaeon-mcu.h

[Severity: Low]
This isn't a bug, but should this driver-specific header be placed locally
in drivers/mfd/ instead of the global include/linux/mfd/ directory?

The MFD subsystem guidelines state that driver-specific header files used
only by the parent and its immediate children should be kept local to
drivers/mfd/.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731-dev-b4-aaeon-mcu-driver-v8-0-9ab00e80df2a@bootlin.com?part=3