Re: [PATCH v11 2/3] i2c: ma35d1: Add Nuvoton MA35D1 I2C driver support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] i2c: ma35d1: data race in transfer initialization corrupts state machine
- [High] i2c: ma35d1: TOCTOU race in target registration leads to NULL pointer dereference
- [Medium] i2c: ma35d1: ignored return value in target receive allows buffer overrun
- [Medium] i2c: ma35d1: inconsistent IO accessors for MA35_DAT register
- [Medium] i2c: ma35d1: resource leak on suspend error path leaves wake-up enabled
- [Medium] i2c: ma35d1: multi-master target mode broken due to unconditional AA bit clear
--

commit deb266e66e11836eee266a00ecd16f32efa64cf8
Author: Zi-Yu Chen <zychennvt@gmail.com>

i2c: ma35d1: Add Nuvoton MA35D1 I2C driver support
    
Add I2C support for Nuvoton MA35D1 SoC.
The controller supports standard, fast and fast-plus modes,
and provides controller/target functionality.

> diff --git a/drivers/i2c/busses/i2c-ma35d1.c b/drivers/i2c/busses/i2c-ma35d1.c
> new file mode 100644
> index 0000000000000..2a4985dc22c6e
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-ma35d1.c
[ ... ]
> +static void ma35d1_i2c_write_ctl(struct ma35d1_i2c *i2c, unsigned int ctl)
> +{
> +	unsigned int val;
> +
> +	guard(spinlock_irqsave)(&i2c->lock);
> +	val = readl(i2c->regs + MA35_CTL0);
> +	val &= ~(MA35_CTL_SI_AA);
> +	val |= ctl;
> +	writel(val, i2c->regs + MA35_CTL0);
> +}

[Severity: Medium]
Will this unconditionally clear the Assert Acknowledge (AA) bit during
controller transfers?

When ma35d1_i2c_doxfer() calls this with just MA35_CTL_STA, the AA bit is
omitted and stripped from the register. In a multi-master environment, if
arbitration is lost and another controller addresses this device as a
target, won't it fail to ACK its target address and break the multi-master
target fallback mechanism?

[ ... ]
> +static irqreturn_t ma35d1_i2c_irq_target_trx(struct ma35d1_i2c *i2c,
> +					     unsigned long i2c_status)
> +{
> +	unsigned char byte = 0;
> +	int ret;
> +
> +	switch (i2c_status) {
[ ... ]
> +	case MA35_S_RECE_DATA_ACK:
> +		/*
> +		 * Previously address with own SLA address Data has been received;
> +		 * ACK has been returned
> +		 */
> +		byte = readb(i2c->regs + MA35_DAT);
> +		i2c_slave_event(i2c->target, I2C_SLAVE_WRITE_RECEIVED, &byte);
> +		break;

[Severity: Medium]
Is it possible for i2c_slave_event() to return an error here when the backend
buffer is full? 

By ignoring the return value, target_reject remains false, causing the
controller to blindly ACK subsequent bytes and potentially overrun the backend
buffer instead of NACKing the next byte.

[ ... ]
> +static irqreturn_t ma35d1_i2c_irq_controller_trx(struct ma35d1_i2c *i2c,
> +						 unsigned long i2c_status)
> +{
> +	unsigned char byte;
[ ... ]
> +	case MA35_M_TRAN_ADDR_ACK:
> +	case MA35_M_TRAN_DATA_ACK:
> +		/* SLA+W has been transmitted and ACK has been received */
> +		if (i2c_status == MA35_M_TRAN_ADDR_ACK) {
> +			if (is_last_msg(i2c) && i2c->msg->len == 0) {
> +				ma35d1_i2c_stop(i2c, 0);
> +				return IRQ_HANDLED;
> +			}
> +		}
> +
> +		if (!is_msgend(i2c)) {
> +			byte = i2c->msg->buf[i2c->msg_ptr++];
> +			writel(byte, i2c->regs + MA35_DAT);
> +			ma35d1_i2c_write_ctl(i2c, MA35_CTL_SI);
> +		} else if (!is_last_msg(i2c)) {

[Severity: Medium]
Why is 32-bit writel() used to write data but 8-bit readb() used to read
from the exact same MA35_DAT register below? 

If the ARM SoC's peripheral bus dictates 32-bit aligned accesses, could
readb() trigger a bus fault or return invalid data? Both paths should use
consistent accessor sizes (e.g. readl(reg) & 0xff).

[ ... ]
> +	case MA35_M_RECE_DATA_ACK:
> +	case MA35_M_RECE_DATA_NACK:
> +		/* DATA has been received and ACK/NACK has been received */
> +		byte = readb(i2c->regs + MA35_DAT);
> +		i2c->msg->buf[i2c->msg_ptr++] = byte;
[ ... ]
> +static int ma35d1_i2c_doxfer(struct ma35d1_i2c *i2c, struct i2c_msg *msgs,
> +			     int num)
> +{
> +	unsigned long timeout;
> +	unsigned int val;
> +	int ret, err;
> +
> +	i2c->msg = msgs;
> +	i2c->msg_num = num;
> +	i2c->msg_ptr = 0;
> +	i2c->msg_idx = 0;
> +	i2c->err = 0;
> +
> +	ma35d1_i2c_write_ctl(i2c, MA35_CTL_STA);

[Severity: High]
Does this expose the transfer state to the active interrupt handler before
setup is fully complete and protected? 

If a spurious interrupt or bus error fires during this window, the ISR
could concurrently modify these fields. The ISR might set msg_num=0 and
err=-EIO, which the process context then overwrites with err=0 before
sending a START, potentially leaving the hardware hanging.

[ ... ]
> +static int ma35d1_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> +			   int num)
> +{
> +	struct ma35d1_i2c *i2c = i2c_get_adapdata(adap);
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(i2c->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ma35d1_i2c_doxfer(i2c, msgs, num);
> +
> +	if (i2c->target)
> +		ma35d1_i2c_write_ctl(i2c, MA35_CTL_AA);
> +	pm_runtime_put_autosuspend(i2c->dev);
> +
> +	return ret;
> +}

[Severity: High]
Is there a TOCTOU race condition here?

If ma35d1_unreg_target() clears i2c->target and calls synchronize_irq()
right after ma35d1_i2c_xfer() checks it but before it sets the AA bit, this
could enable target mode interrupts while i2c->target is NULL. A subsequent
target transaction would then fire the ISR and dereference the NULL pointer.

[ ... ]
> +static int ma35d1_unreg_target(struct i2c_client *target)
> +{
> +	struct ma35d1_i2c *i2c = i2c_get_adapdata(target->adapter);
> +	int ret;
> +
> +	if (!i2c->target)
> +		return -EINVAL;
> +
> +	writel(0, i2c->regs + MA35_ADDR0);
> +	/* Clear AA bit */
> +	ma35d1_i2c_write_ctl(i2c, 0);
> +
> +	synchronize_irq(i2c->irq);
> +	i2c->target = NULL;
[ ... ]
> +static int ma35d1_i2c_suspend(struct device *dev)
> +{
> +	struct ma35d1_i2c *i2c = dev_get_drvdata(dev);
> +	unsigned int val;
> +	int ret;
> +
> +	/* Prepare for wake-up from I2C events if target mode is active */
> +	if (i2c->target) {
> +		ma35d1_i2c_write_ctl(i2c, MA35_CTL_AA);
> +
> +		/* Setup wake-up control */
> +		writel(0x1, i2c->regs + MA35_WKCTL);
> +
> +		/* Clear pending wake-up flags */
> +		val = readl(i2c->regs + MA35_WKSTS);
> +		writel(val, i2c->regs + MA35_WKSTS);
> +
> +		ret = enable_irq_wake(i2c->irq);
> +		if (ret)
> +			return ret;
> +		return 0;
> +	}
> +	return pm_runtime_force_suspend(dev);
> +}

[Severity: Medium]
If enable_irq_wake() fails, this returns early without clearing MA35_WKCTL. 

Will this leave the hardware wake-up control active during normal runtime,
potentially causing spurious wakeups or interrupt storms?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001075101.489660-1-zychennvt@gmail.com?part=2