Re: [PATCH v2 4/8] Add Advantech EIO I2C driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 18 potential issue(s) to consider:
- [Critical] Reading SMBus block data via an `unsigned int *` cast causes out-of-bounds memory writes and stack corruption.
- [Critical] The driver does not validate the userspace-provided block length for SMBus block writes, allowing an out-of-bounds read of kernel memory.
- [High] Premature exit in `i2c_xfer` causes silent truncation of multi-message I2C transactions.
- [High] Local mutex `pnp_mutex` fails to protect the shared MFD-level PNP configuration sequence from concurrent access by other child drivers.
- [High] Misuse of `regmap_update_bits` (`eio_reg_or`) on W1C status registers and with 0 masks acts as a no-op, failing to clear hardware errors.
- [High] Off-by-one boundary check strictly rejects valid maximum-length (32-byte) SMBus block transfers.
- [High] The 10-bit I2C addressing implementation is structurally flawed, losing the R/W bit, corrupting the address, and omitting Repeated Start.
- [High] Uninitialized stack variables in `smb_access` are returned to userspace if regmap reads fail.
- [High] SMBus read emulation over I2C incorrectly marks the command byte phase as a read operation.
- [High] The read phase for `I2C_SMBUS_BLOCK_PROC_CALL` is completely omitted, dropping the response data.
- [High] Destructive bitwise operation wipes out `SMB_REG_HC` configuration during Block Process Call.
- [Medium] Incorrect endianness handling in SMBus Word Data and Process Call transfers corrupts data on big-endian architectures.
- [Medium] The hardware is left stuck in PNP Extended Configuration Mode if any register access fails during `load_i2c`.
- [Medium] Clock configuration logic is structurally disjoint when the requested frequency equals exactly 30 kHz (`I2C_THRESHOLD_SCLH`), resulting in massive under-clocking.
- [Medium] Polling loops can falsely trigger timeout errors because they evaluate expiration before executing the final register read.
- [Low] The block write formatting logic in `smbus_xfer` copies one byte past the end of the payload buffer.
- [Low] Module parameters for configuring I2C and SMBus frequencies are declared but completely ignored by the driver logic.
- [Low] Driver-specific header file is inappropriately placed in the global MFD include directory.
--

--- Patch [4]: [PATCH v2 4/8] Add Advantech EIO I2C driver ---
commit 0b05c6638fad42a025d75120571d2def944319c3
Author: Ramiro Oliveira <ramiro.oliveira@advantech.com>

    Add Advantech EIO I2C driver
    
    This commit adds the driver to control the Advantech EIO I2C block, this
    block is included in the Advantech EIO MFD.

> diff --git a/drivers/i2c/busses/i2c-eio.c b/drivers/i2c/busses/i2c-eio.c
> new file mode 100644
> index 0000000000000..56a89ca18f328
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-eio.c
> @@ -0,0 +1,1135 @@
> +// SPDX-License-Identifier: GPL-2.0-only
[ ... ]
> +#include <linux/device.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/eio.h>

[Severity: Low]
Does this include violate MFD architectural guidelines? The subsystem guide
specifies that driver-specific header files should be kept local to
drivers/mfd/ and not placed in the global include/linux/mfd/ directory if
they are only used by the parent and its immediate children.

[ ... ]
> +struct eio_i2c_dev {
> +	struct device *dev;
> +	struct device *mfd;
> +	struct regmap *regmap;
> +	struct mutex pnp_mutex; /* Mutex for PNP acces */

[Severity: High]
Since this mutex is initialized locally within the I2C child driver, does it
fail to protect the shared MFD-level PNP configuration registers (0x299/0x29A)
from concurrent access by other sibling drivers (like GPIO or HWMON)?

[ ... ]
> +static int wait_busy(struct eio_i2c_chan *i2c_chan)
> +{
> +	ktime_t time_end = ktime_add_us(ktime_get(), I2C_TIMEOUT);
> +	unsigned int reg = eio_chan_reg(i2c_chan, I2C_REG_STAT, SMB_REG_HS);
> +	unsigned int target = eio_chan_reg(i2c_chan, I2C_STAT_BUSY, SMB_HS_BUSY);
> +	unsigned int val;
> +	int cnt = 0;
> +
> +	do {
> +		fsleep(cnt++);
> +
> +		if (ktime_after(ktime_get(), time_end)) {
> +			dev_err(eio_dev(i2c_chan), "Wait I2C bus busy timeout\n");
> +			return -ETIME;
> +		}
> +
> +		if (eio_reg_read(i2c_chan, reg, &val))
> +			return -EIO;
> +
> +	} while (val & target);

[Severity: Medium]
Could this polling loop falsely return a timeout? If the thread experiences
latency during the sleep and the timer expires, the loop will exit with an
error without performing a final register read to check if the hardware
actually completed the operation.

[ ... ]
> +static void i2c_clear(struct eio_i2c_chan *i2c_chan)
> +{
> +	if (is_i2c(i2c_chan)) {
> +		eio_reg_write(i2c_chan, I2C_REG_STAT, 0xFF);
> +	} else {
> +		eio_reg_or(i2c_chan, SMB_REG_HS,  0xA9);
> +		eio_reg_or(i2c_chan, SMB_REG_HS2, 0x4C);
> +	}
> +}

[Severity: High]
Does using a read-modify-write operation (eio_reg_or -> regmap_update_bits)
work for clearing Write-1-to-Clear (W1C) bits? If the bits are already set
in the hardware, reading them and writing back the same value will often be
skipped by regmap, leaving the errors uncleared.

[ ... ]
> +static int wait_write_done(struct eio_i2c_chan *i2c_chan, bool no_ack)
> +{
> +	ktime_t time_end = ktime_add_us(ktime_get(), I2C_TIMEOUT);
> +	unsigned int val = 0;
> +	int cnt = 0;
> +	unsigned int reg = eio_chan_reg(i2c_chan, I2C_REG_STAT, SMB_REG_HS);
> +	unsigned int target = eio_chan_reg(i2c_chan, I2C_STAT_TXDONE, SMB_HS_TX_DONE);
> +
> +	do {
> +		fsleep(cnt++);
> +		if (ktime_after(ktime_get(), time_end)) {
> +			if (is_i2c(i2c_chan)) {
> +				eio_reg_or(i2c_chan, I2C_REG_STAT, 0);

[Severity: High]
Is it possible this fails to clear the status flags? Passing a mask of 0 to
regmap_update_bits will result in no bits being changed, and the write will
likely be optimized out entirely.

[ ... ]
> +static int set_freq(struct eio_i2c_chan *i2c_chan, int freq)
> +{
> +	u8 pre1, pre2;
> +	u16 speed;
> +	unsigned int reg1 = eio_chan_reg(i2c_chan, I2C_REG_PRESCALE1, SMB_REG_HPRESCALE1);
> +	unsigned int reg2 = eio_chan_reg(i2c_chan, I2C_REG_PRESCALE2, SMB_REG_HPRESCALE2);
> +
> +	dev_dbg(eio_dev(i2c_chan), "set freq: %dkHz\n", freq);
> +	if (freq > I2C_FREQ_MAX || freq < I2C_FREQ_MIN) {
> +		dev_err(eio_dev(i2c_chan), "Invalid i2c freq: %d\n", freq);
> +		return -EINVAL;
> +	}
> +
> +	speed = (freq < I2C_THRESHOLD_SCLH) ? I2C_SCLH_LOW : I2C_SCLH_HIGH;
> +
> +	pre1 = (u8)(CHIP_CLK / speed);
> +	pre2 = (u8)((speed / freq) - 1);
> +
> +	if (freq > I2C_THRESHOLD_SCLH)
> +		pre2 |= I2C_SCL_FAST_MODE;

[Severity: Medium]
What happens if the requested frequency is exactly 30 kHz (I2C_THRESHOLD_SCLH)?
The first check (< 30) evaluates to false, making speed 2500. However, the
second check (> 30) also evaluates to false, failing to set the FAST_MODE bit.
Will this combination drastically underclock the bus?

[ ... ]
> +static int smb_access(struct eio_i2c_chan *i2c_chan, u8 addr, bool is_read, u8 cmd,
> +		      int size, union i2c_smbus_data *data)
> +{
> +	int i, tmp, ret = 0;
[ ... ]
> +	case I2C_SMBUS_BLOCK_DATA:
> +		dev_dbg(eio_dev(i2c_chan), "I2C_SMBUS_BLOCK_DATA\n");
> +		if (is_read)
> +			break;
> +
> +		/* Program command type */
> +		eio_reg_read(i2c_chan, SMB_REG_HC, (unsigned int *)&tmp);
> +		tmp &= ~(0x07 << SMB_HC_CMD_SHIFT);
> +		tmp |=  (size << SMB_HC_CMD_SHIFT);
> +		eio_reg_write(i2c_chan, SMB_REG_HC, tmp);
> +
> +		/* Force write for payload stage */
> +		eio_reg_write(i2c_chan, SMB_REG_HADDR, addr & ~0x01);
> +
> +		/* Reset internal buffer index pointer */
> +		eio_reg_and(i2c_chan, SMB_REG_HC2, (int)~SMB_HC2_E32B);
> +		eio_reg_or(i2c_chan, SMB_REG_HC2, SMB_HC2_E32B);
> +
> +		/* Write length + data */
> +		eio_reg_write(i2c_chan, SMB_REG_HD0, data->block[0]);
> +		for (i = 1; i <= data->block[0]; i++)
> +			eio_reg_write(i2c_chan, SMB_REG_HBLOCK, data->block[i]);

[Severity: Critical]
Does this loop need to validate the length of data->block[0] against
I2C_SMBUS_BLOCK_MAX before writing? Without bounds checking, this may blindly
read past the union's memory boundary and dump kernel stack contents onto the
I2C bus.

> +		break;
> +
> +	case I2C_SMBUS_BLOCK_PROC_CALL:
> +		/* Set command type field */
> +		eio_reg_and(i2c_chan, SMB_REG_HC, (0x07 << SMB_HC_CMD_SHIFT));

[Severity: High]
Does this bitwise operation wipe out configuration flags? Because eio_reg_and
uses regmap_update_bits with an inverted mask, providing only the command bits
implies all other active control bits in the register will be zeroed out.

> +		eio_reg_write(i2c_chan, SMB_REG_HD0, data->block[0]);
> +
> +		/* Reset buffer index */
> +		eio_reg_and(i2c_chan, SMB_REG_HC2, (int)~SMB_HC2_E32B);
> +		eio_reg_or(i2c_chan, SMB_REG_HC2, SMB_HC2_E32B);
> +
> +		for (i = 1; i <= data->block[0]; i++)
> +			eio_reg_write(i2c_chan, SMB_REG_HBLOCK, data->block[i]);

[Severity: Critical]
Similar to the block data write above, does data->block[0] need bounds checking
here to prevent out-of-bounds kernel memory reads?

[ ... ]
> +	switch (size) {
> +	case I2C_SMBUS_QUICK:
> +		dev_dbg(eio_dev(i2c_chan), "I2C_SMBUS_QUICK\n");
> +		break;
> +
> +	case I2C_SMBUS_BYTE:
> +	case I2C_SMBUS_BYTE_DATA:
> +		if (is_read) {
> +			unsigned int v;
> +
> +			dev_dbg(eio_dev(i2c_chan), "I2C_SMBUS_BYTE/I2C_SMBUS_BYTE_DATA\n");
> +			eio_reg_read(i2c_chan, SMB_REG_HD0, &v);
> +			data->block[0] = (u8)v;

[Severity: High]
If eio_reg_read fails (e.g. from a hardware error), the error is ignored and
the uninitialized stack variable 'v' will be copied into data->block[0]. Will
this leak uninitialized kernel memory to userspace during an ioctl?

> +			dev_dbg(eio_dev(i2c_chan), "read %X\n", data->block[0]);
> +		}
> +		break;
> +
> +	case I2C_SMBUS_WORD_DATA: {
> +		unsigned int v0, v1;
> +
> +		if (is_read) {
> +			dev_dbg(eio_dev(i2c_chan), "I2C_SMBUS_WORD_DATA\n");
> +			eio_reg_read(i2c_chan, SMB_REG_HD0, &v0);
> +			eio_reg_read(i2c_chan, SMB_REG_HD1, &v1);
> +			data->block[0] = (u8)v0;
> +			data->block[1] = (u8)v1;

[Severity: Medium]
Is it safe to assign word bytes directly into the data->block byte array?
Because data->block shares a union with the 16-bit data->word, this mappings
changes based on host endianness. Will this corrupt word values on big-endian
architectures?

[Severity: High]
Also, just like the byte read case, an error in eio_reg_read will cause the
uninitialized stack variables v0 and v1 to be returned to the client.

> +		}
> +		break;
> +	}
> +
> +	case I2C_SMBUS_PROC_CALL: {
> +		unsigned int v0, v1;
> +
> +		dev_dbg(eio_dev(i2c_chan), "I2C_SMBUS_PROC_CALL\n");
> +		eio_reg_read(i2c_chan, SMB_REG_HD0, &v0);
> +		eio_reg_read(i2c_chan, SMB_REG_HD1, &v1);
> +		data->block[0] = (u8)v0;
> +		data->block[1] = (u8)v1;

[Severity: Medium]
Similar endianness handling issue exists here for Process Call responses.

[Severity: High]
And once again, uninitialized memory leaks are possible if the reads fail.

> +		break;
> +	}
> +
> +	case I2C_SMBUS_BLOCK_DATA:
> +		if (!is_read)
> +			break;
> +
> +		dev_dbg(eio_dev(i2c_chan), "I2C_SMBUS_BLOCK_DATA\n");
> +		eio_reg_read(i2c_chan, SMB_REG_HD0, (unsigned int *)&len);
> +		len = min(len, I2C_SMBUS_BLOCK_MAX);
> +		data->block[0] = len;
> +
> +		for (i = 1; i <= len; i++)
> +			eio_reg_read(i2c_chan, SMB_REG_HBLOCK,
> +				     (unsigned int *)&data->block[i]);

[Severity: Critical]
Is it safe to cast the 8-bit array slot pointer &data->block[i] to a 32-bit
unsigned int * ? When regmap_read populates the result, it will write 32 bits,
causing it to overwrite up to 3 bytes past the end of the union i2c_smbus_data
structure on the final iterations, corrupting the stack.

> +		break;
> +
> +	default:
> +		ret = -EINVAL;
> +		goto exit;
> +	}

[Severity: High]
Is the read phase for I2C_SMBUS_BLOCK_PROC_CALL intentionally missing? The
switch statement falls through to default, throwing away the response data and
returning an error.

[ ... ]
> +static int i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int nmsgs)
> +{
[ ... ]
> +	for (msg = 0; msg < nmsgs; msg++) {
> +		int is_read = msgs[msg].flags & I2C_M_RD;
> +		bool no_ack = msgs[msg].flags & I2C_M_IGNORE_NAK;
> +
> +		dev_dbg(eio_dev(i2c_chan), "message %d len=%d\n", msg, msgs[msg].len);
> +
> +		if (!msgs[msg].len)
> +			let_stop(i2c_chan);
> +
> +		if (msgs[msg].flags & I2C_M_TEN) {
> +			addr = eio_enc_10bit_addr(msgs[msg].addr);
> +			addr |= is_read;
> +			dev_dbg(eio_dev(i2c_chan), "10-bit addr: %X\n", addr);
> +
> +			ret = write_addr(i2c_chan, addr >> 8, no_ack);
> +			if (!ret)
> +				ret = write_data(i2c_chan, addr & 0x7F, no_ack);

[Severity: High]
Is this 10-bit addressing logic accurate? ORing the read flag into the lower
byte, dropping the upper bit via a shift, and masking the bottom bits seems to
corrupt the address format. Additionally, shouldn't there be a Repeated Start
phase explicitly generated for reads?

> +		} else {
> +			addr = eio_enc_7bit_addr(msgs[msg].addr);
> +			addr |= is_read;
> +			dev_dbg(eio_dev(i2c_chan), "7-bit addr: %X\n", addr);
> +
> +			ret = write_addr(i2c_chan, addr, no_ack);
> +		}
> +
> +		if (ret)
> +			goto exit;
> +
> +		if (!msgs[msg].len)
> +			goto exit;

[Severity: High]
Will exiting here prematurely abort multi-message transfers? For 0-length probe
messages, this breaks out of the loop over nmsgs, failing to process any
subsequent messages while still returning success.

> +
> +		if (is_read)
> +			ret = eio_trigger_read(i2c_chan, (u32 *)&dummy);
> +
> +		/* Transmit all messages */
> +		for (data = 0; data < msgs[msg].len; data++) {
> +			if (msgs[msg].flags & I2C_M_RD) {
> +				bool last = (msgs[msg].len == data + 1);
> +
> +				if (last)
> +					let_stop(i2c_chan);
> +
> +				ret = read_data(i2c_chan, &msgs[msg].buf[data]);
> +				dev_dbg(eio_dev(i2c_chan), "I2C read[%d] = %x\n",
> +					data, msgs[msg].buf[data]);
> +
> +				/* Don't stop twice */
> +				if (last && ret == 0)
> +					goto exit;

[Severity: High]
Similarly, doesn't this goto break out of the nmsgs loop entirely? If a read
message is followed by further messages in a single I2C transaction, those
remaining messages will be silently ignored.

[ ... ]
> +static int smbus_xfer(struct i2c_adapter *adap, u16 addr,
> +		      u16 flags, char is_read, u8 cmd,
> +		      int size, union i2c_smbus_data *data)
> +{
[ ... ]
> +	if (data) {
> +		buf[0] = cmd;
> +		/* FIX: preserve other flags; only toggle I2C_M_RD */
> +		msgs[0].flags = is_read ? (flags | I2C_M_RD) : (flags & ~I2C_M_RD);

[Severity: High]
Does this break the semantic structure of an SMBus read transaction? Setting
I2C_M_RD on the first message (the command byte phase) means it will be sent to
the device as an I2C read rather than an I2C write, causing emulated SMBus reads
to fail entirely.

[ ... ]
> +	case I2C_SMBUS_I2C_BLOCK_DATA:
> +	case I2C_SMBUS_I2C_BLOCK_BROKEN:
> +		dev_dbg(eio_dev(i2c_chan), "I2C_SMBUS_I2C_BLOCK_(DATA/BROKEN) on I2C len=%d\n",
> +			data->block[0]);
> +		if (!data)
> +			return -EINVAL;
> +		msgs[0].len = is_read ? 1 : data->block[0] + 1;
> +		msgs[1].len = data->block[0];
> +		msgs[1].buf = data->block + 1;
> +		if (msgs[0].len >= I2C_SMBUS_BLOCK_MAX ||
> +		    msgs[1].len >= I2C_SMBUS_BLOCK_MAX)
> +			return -EINVAL;

[Severity: High]
Does this boundary check strictly reject valid 32-byte payloads? Since
I2C_SMBUS_BLOCK_MAX is 32, using >= instead of > causes perfectly valid
maximum-length transfers to fail with -EINVAL.

> +		if (!is_read)
> +			memcpy(buf + 1, data->block + 1, msgs[0].len);

[Severity: Low]
Since msgs[0].len is set to data->block[0] + 1, won't copying msgs[0].len bytes
from data->block + 1 pull one extra uninitialized byte from past the end of the
populated payload?

[ ... ]
> +static int load_i2c(struct device *dev, enum eio_chan_id id,
> +		    struct eio_i2c_chan *i2c_chan)
> +{
[ ... ]
> +	/* Read channel I/O base via shared PNP window */
> +	mutex_lock(&eio_i2c->pnp_mutex);
> +	if (regmap_write(map, REG_PNP_INDEX, REG_EXT_MODE_ENTER) ||
> +	    regmap_write(map, REG_PNP_INDEX, REG_EXT_MODE_ENTER) ||
> +	    regmap_write(map, REG_PNP_INDEX, REG_LDN) ||
> +	    regmap_write(map, REG_PNP_DATA,  ldn) ||
> +	    regmap_write(map, REG_PNP_INDEX, REG_BASE_HI) ||
> +	    regmap_read(map,  REG_PNP_DATA,  &base_hi) ||
> +	    regmap_write(map, REG_PNP_INDEX, REG_BASE_LO) ||
> +	    regmap_read(map,  REG_PNP_DATA,  &base_lo) ||
> +	    regmap_write(map, REG_PNP_INDEX, REG_EXT_MODE_EXIT)) {
> +		mutex_unlock(&eio_i2c->pnp_mutex);
> +		dev_err(dev, "error read/write I2C[%d] IO port\n", id);
> +		return -EIO;
> +	}

[Severity: Medium]
If any of these hardware accesses fail, the logical OR sequence short-circuits.
Doesn't this mean the REG_EXT_MODE_EXIT command is never sent, leaving the Super
I/O controller permanently stuck in configuration mode?

> +	mutex_unlock(&eio_i2c->pnp_mutex);
> +
> +	base = (base_hi << 8) | base_lo;
> +	if (base == 0xFFFF || base == 0) {
> +		dev_dbg(dev, "i2c[%d] base addr=%#x (not in-use)\n", id, base);
> +		return -ENODEV;
> +	}
> +
> +	dev_dbg(dev, "i2c[%d] base addr=%#x\n", id, base);
> +
> +	/* Bind channel (no per-chan dev) */
> +	i2c_chan->base = (u16)base;
> +	i2c_chan->id   = id;
> +
> +	/* Per-channel frequency policy */
> +	if (i2c_chan->freq_override != USE_DEFAULT)
> +		set_freq(i2c_chan, i2c_chan->freq_override);

[Severity: Low]
Does this actually have an effect? It appears i2c_chan->freq_override is
unconditionally initialized to USE_DEFAULT in the probe function just before
this is called, rendering the check dead code and ignoring user-provided module
parameters.

[ ... ]
> +static int eio_i2c_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	for (ch = EIO_I2C0; ch < MAX_I2C_SMB; ch++) {
> +		struct eio_i2c_chan *i2c_chan;
> +
> +		i2c_chan = devm_kzalloc(dev, sizeof(*i2c_chan), GFP_KERNEL);
> +		if (!i2c_chan) {
> +			ret = -ENOMEM;
> +			break;
> +		}
> +
> +		i2c_chan->parent = eio_i2c;
> +		i2c_chan->freq_override = USE_DEFAULT;

[Severity: Low]
This initialization unconditionally overrides any module parameters provided by
the system administrator.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-upstream-v2-v2-0-76e5e41026db@advantech.com?part=4