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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] i2c: ignored return value of I2C_SLAVE_WRITE_RECEIVED breaks flow control
- [High] i2c: zero-message transfers bypass wait logic causing OOB reads or bus lockups
- [Medium] i2c: ma35d1: multi-master target arbitration broken by clearing AA bit
--

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

i2c: ma35d1: Add Nuvoton MA35D1 I2C driver support

This commit adds I2C support for the Nuvoton MA35D1 SoC, including standard,
fast, and fast-plus modes, and provides controller and target functionality.

> diff --git a/drivers/i2c/busses/i2c-ma35d1.c b/drivers/i2c/busses/i2c-ma35d1.c
> new file mode 100644
> index 0000000000000..29bed9eb28f67
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-ma35d1.c
[ ... ]
> @@ -125,12 +125,12 @@ static inline bool ma35d1_is_controller_status(unsigned int status)
>  /*
>   * ma35d1_i2c_write_ctl - Update the I2C control register
>   * @i2c: Pointer to the ma35d1 i2c instance
>   * @ctl: Control bits to set (e.g., MA35_CTL_STA, SI, AA)
>   *
>   * Read-modify-write the CTL0 register while preserving unrelated bits.
>   */
>  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;

[Severity: Medium]
Does this unconditionally clear the Assert Acknowledge (MA35_CTL_AA) bit
unless it is explicitly passed in the ctl argument?

If arbitration is lost during the START or address phase of a transfer, the
controller will read AA as 0 and NACK its own target address, causing the
winning master's transfer to fail.

>  	writel(val, i2c->regs + MA35_CTL0);
>  }
[ ... ]
> @@ -262,17 +262,17 @@ static irqreturn_t ma35d1_i2c_irq_target_trx(struct ma35d1_i2c *i2c,
>  	case MA35_S_TRAN_DATA_NACK:
>  	case MA35_S_RECE_DATA_NACK:
>  		/*
>  		 * Data byte or last data in I2CDAT has been transmitted and NACK received,
>  		 * or previously addressed with own SLA address and NACK returned.
>  		 */
>  		i2c->target_reject = false;
>  		break;
>  
>  	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);

[Severity: High]
Does this code ignore the return value of i2c_slave_event in
ma35d1_i2c_irq_target_trx?

If the backend cannot accept more data, for example when the backend buffer
is full, it returns a non-zero value. Because this is ignored, the driver
unconditionally writes MA35_CTL_SI_AA at the end of the ISR to ACK the
byte, which breaks flow control and allows an external I2C master to
overrun the backend buffer.

Should the return value be checked and i2c->target_reject set to true to
properly NACK the next incoming byte on error?

>  		break;
>  
>  	case MA35_ADDR_TRAN_ARB_LOST:
[ ... ]
> @@ -333,13 +333,13 @@ static irqreturn_t ma35d1_i2c_irq_controller_trx(struct ma35d1_i2c *i2c,
>  	if (unlikely(!i2c->msg)) {
>  		dev_err(i2c->dev, "Spurious controller interrupt (status: 0x%02lx)\n", i2c_status);
>  		ma35d1_i2c_restore_idle(i2c);
>  		return IRQ_HANDLED;
>  	}
>  
>  	switch (i2c_status) {
>  	case MA35_M_START:
>  	case MA35_M_REPEAT_START:
>  		ma35d1_i2c_set_addr(i2c);
>  		ma35d1_i2c_write_ctl(i2c, MA35_CTL_SI);

[Severity: Medium]
Since MA35_CTL_AA is omitted here in ma35d1_i2c_irq_controller_trx, the AA
bit is cleared during address transmission. If arbitration is lost during
SLA+R/W, will the target fallback fail to ACK its own address?

>  		break;
>  
>  	case MA35_M_TRAN_ADDR_ACK:
[ ... ]
> @@ -438,15 +438,15 @@ static int ma35d1_i2c_doxfer(struct ma35d1_i2c *i2c, struct i2c_msg *msgs,
>  			     int num)
>  {
>  	unsigned long timeout;
>  	unsigned int val;
>  	int ret, err;
>  
>  	disable_irq(i2c->irq);
>  	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: Medium]
Will this omit the AA bit and thus clear it when initiating a master
transfer in ma35d1_i2c_doxfer?

>  	enable_irq(i2c->irq);
>  
>  	timeout = wait_event_timeout(i2c->wait, i2c->msg_num == 0, HZ * 5);

[Severity: High]
If a caller invokes i2c_transfer with num = 0 to probe the bus, does this
wait evaluate to true immediately?

When num = 0, i2c->msg_num is set to 0. This causes wait_event_timeout to
complete instantly while the hardware is still initiating the START. If msgs
is a zero-length array pointer or NULL, the subsequent interrupt could
either read out-of-bounds from i2c->msg or fail to assert a STOP condition,
leaving the bus permanently locked up in a START state.

>  	ret = i2c->msg_idx;
>  
>  	if (timeout == 0) {

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