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

[zychen <zychennvt@gmail.com>]

sashiko-bot@kernel.org 於 2026/10/16 上午 09:17 寫道:
> 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

As in the previous reply.
When MA35D1 reports `MA35_S_RECE_DATA_ACK`, the byte has already been received and ACKed,
so the driver can no longer NACK it. Propagating the callback error to the next byte is also
incorrect, as `I2C_SLAVE_WRITE_RECEIVED` applies only to the current byte.

> - [High] i2c: zero-message transfers bypass wait logic causing OOB reads or bus lockups

core rejects transfers with `num < 1` in `__i2c_transfer()` and returns `-EINVAL`
before calling the adapter's `master_xfer()` callback. 

> - [Medium] i2c: ma35d1: multi-master target arbitration broken by clearing AA bit

The MA35D1 hardware supports both controller and target modes. When a target
is registered, this driver intentionally clears AA while initiating a controller
transfer, so it does not accept transactions addressed to its target address
during that period. Preserving AA would change the current coexistence behavior
and require validation of the arbitration and target state transitions. We would
prefer to address that as a separate multi-master enhancement in a follow-up patch.
> --
> 
> 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) {
> 

Best regards,
Zi-Yu