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

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The driver improperly applies the `I2C_M_DMA_SAFE` flag to the caller-provided `val_buf`, bypassing I2C bounce buffering and risking direct DMA into the stack or unaligned memory.
- [High] The I2C bus lock is temporarily released between the command write and response read, allowing other clients to interleave traffic and break the MCU's transaction atomicity.
- [Medium] Missing `.use_single_read = true` in the regmap configuration allows unsupported multi-byte reads, violating the MCU's communication protocol.
- [Low] Driver-specific header file is incorrectly placed in the global `include/linux/mfd/` directory, violating subsystem guidelines.
--

--- Patch [3]: [PATCH v7 3/5] mfd: aaeon: Add SRG-IMX8P MCU driver ---
commit ce41fd5bdffca68e9033f19059ca7fbc7576a6fe
Author: Thomas Perrot (Schneider Electric) <[email protected]>
Date:   Tue Jul 7 08:01:29 2026 +0000

    mfd: aaeon: Add SRG-IMX8P MCU driver

This commit adds an MFD driver for the Aaeon SRG-IMX8P embedded controller.
It provides the core I2C interface to communicate with the MCU and registers
child devices for GPIO and watchdog functionality. The driver implements a
custom regmap bus over I2C to match the MCU's fixed 3-byte command format.

> diff --git a/drivers/mfd/aaeon-mcu.c b/drivers/mfd/aaeon-mcu.c
> new file mode 100644
> index 0000000000000..54ac30fd5bed5
> --- /dev/null
> +++ b/drivers/mfd/aaeon-mcu.c
[ ... ]
> @@ -46,36 +46,36 @@ static int aaeon_mcu_regmap_write(void *context, const void *data, size_t count)
> 	struct aaeon_mcu *mcu = context;
> 	struct i2c_client *client = mcu->client;
> 	struct i2c_msg write_msg;
> 	/* The MCU always sends a response byte after each command; discard it. */
> 	struct i2c_msg response_msg;
> 	int ret;
> 
> 	memcpy(mcu->cmd, data, count);
> 
> 	write_msg.addr  = client->addr;
> 	write_msg.flags = I2C_M_DMA_SAFE;
> 	write_msg.buf   = mcu->cmd;
> 	write_msg.len   = count;
> 
> 	response_msg.addr  = client->addr;
> 	response_msg.flags = I2C_M_RD | I2C_M_DMA_SAFE;
> 	response_msg.buf   = mcu->response;
> 	response_msg.len   = 1;
> 
> 	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]
Because two separate i2c_transfer() calls are used to generate the required
STOP condition, the I2C bus lock is temporarily released in between them.

Can another driver on the same I2C bus interleave a transaction between the
command write and the response read, disrupting the MCU state machine?

Should this code use i2c_lock_bus() around both transfers to ensure
atomicity?

> 	if (ret < 0)
> 		return ret;
> 	if (ret != 1)
> 		return -EIO;
> 
> 	return 0;
> }
> 
> static int aaeon_mcu_regmap_read(void *context, const void *reg_buf,
> 				 size_t reg_size, void *val_buf, size_t val_size)
> {
> 	struct aaeon_mcu *mcu = context;
> 	struct i2c_client *client = mcu->client;
> 	struct i2c_msg write_msg;
> 	struct i2c_msg read_msg;
> 	int ret;
> 
> 	/*
> 	 * reg_buf holds the 2-byte big-endian register address [opcode, arg].
> 	 * Append a trailing 0x00 to form the full 3-byte MCU command.
> 	 */
> 	mcu->cmd[0] = ((u8 *)reg_buf)[0];
> 	mcu->cmd[1] = ((u8 *)reg_buf)[1];
> 	mcu->cmd[2] = 0x00;
> 
> 	write_msg.addr  = client->addr;
> 	write_msg.flags = I2C_M_DMA_SAFE;
> 	write_msg.buf   = mcu->cmd;
> 	write_msg.len   = AAEON_MCU_CMD_LEN;
> 
> 	read_msg.addr  = client->addr;
> 	read_msg.flags = I2C_M_RD | I2C_M_DMA_SAFE;
> 	read_msg.buf   = val_buf;
> 	read_msg.len   = val_size;

[Severity: High]
Does applying the I2C_M_DMA_SAFE flag to the caller-provided val_buf risk
direct DMA into unaligned memory or the stack?

If a child driver or the regmap core calls regmap_bulk_read() with a
stack-allocated buffer on a vmapped stack, bypassing the I2C bounce
buffers could lead to a kernel panic or memory corruption.

Would it be better to copy the read data through the DMA-safe mcu->response
buffer instead of passing val_buf directly?

> 
> 	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 aaeon_mcu_regmap_write(), using two separate i2c_transfer() calls
here temporarily releases the I2C bus lock. 

Can this lead to the same interleaving and state machine disruption issues
described above?

> 	if (ret < 0)
> 		return ret;
> 	if (ret != 1)
> 		return -EIO;
> 
> 	return 0;
> }
[ ... ]
> static const struct regmap_config aaeon_mcu_regmap_config = {
> 	.reg_bits          = 16,
> 	.val_bits          = 8,
> 	.reg_format_endian = REGMAP_ENDIAN_BIG,
> 	.max_register      = AAEON_MCU_MAX_REGISTER,
> 	.volatile_reg      = aaeon_mcu_volatile_reg,
> 	.cache_type        = REGCACHE_MAPLE,
> 	.use_single_write  = true,
> };

[Severity: Medium]
Is .use_single_read = true missing from this configuration?

Without it, if a child driver attempts a bulk read, regmap will pass a
multi-byte val_size to the read callback.

Since the MCU protocol expects exactly one response byte per 3-byte command,
could a multi-byte read request lead to I2C transfer errors or invalid data?

[ ... ]
> 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
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Aaeon MCU driver definitions
> + *
> + * Copyright (C) 2026 Bootlin

[Severity: Low]
Should this driver-specific header be kept local to drivers/mfd/ instead of
being placed in the global include/linux/mfd/ directory?

The MFD subsystem guidelines state that headers used only by the parent and
its immediate children should not be placed in the global include path.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3