Re: [PATCH v3 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] i2c: amlogic-a9: missing threshold IRQ disable on timeout in aml_i2c_xfer_msg()
- [Medium] i2c: amlogic-a9: transfer started before IRQ setup in aml_i2c_do_start()
- [Medium] i2c: amlogic-a9: spurious TX FIFO write on completion in aml_i2c_irq()
--

commit ab8bf627f35c4931f8163555b700d4cca01277db
Author: Junyi Zhao <junyi.zhao@amlogic.com>

i2c: amlogic: Add Amlogic A9 I2C controller driver

This commit adds an I2C controller driver for Amlogic A9 SoCs, along with
corresponding Kconfig, Makefile, and MAINTAINERS entries.

> diff --git a/drivers/i2c/busses/i2c-amlogic-a9.c b/drivers/i2c/busses/i2c-amlogic-a9.c
> new file mode 100644
> index 0000000000000..503511734fe26
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-amlogic-a9.c
[ ... ]
> +static void aml_i2c_put_data(struct aml_i2c *i2c, u8 *buf, int len)
> +{
> +	int i;
> +
> +	/* When this I2C module transfers 0 bytes, it must still send at least 1 byte. */
> +	if (!i2c->msg->len) {
> +		writel(0x00, i2c->regs + REG_CFG_TX);
> +		return;
> +	}
> +
> +	for (i = 0; i < len; i++, buf++)
> +		writel(*buf, i2c->regs + REG_CFG_TX);
> +}
> +
> +static void aml_i2c_prepare_xfer(struct aml_i2c *i2c, enum fifo_fill_mode mode)
> +{
> +	bool write = !(i2c->msg->flags & I2C_M_RD);
> +
> +	if (!write)
> +		return;
> +
> +	i2c->count = min(i2c->msg->len - i2c->pos,
> +			 mode == A9_FIFO_FILL_FULL ? A9_I2C_FIFO_DEPTH : A9_I2C_HALF_FIFO);
> +	aml_i2c_put_data(i2c, i2c->msg->buf + i2c->pos, i2c->count);
> +}
> +
> +static irqreturn_t aml_i2c_irq(int irqno, void *dev_id)
> +{
[ ... ]
> +	switch (i2c->state) {
> +	case A9_STATE_READ:
> +		buf = i2c->msg->buf;
> +		rd_len = min(i2c->msg->len - i2c->pos, A9_I2C_FIFO_DEPTH);
> +		for (i = 0; i < rd_len; i++) {
> +			rd_val = readl(i2c->regs + REG_CFG_RX);
> +			buf[i2c->pos++] = rd_val & TX_RX_DATA;
> +		}
> +		/* Rx must clear thresh after read fifo */
> +		clear_irq_status(i2c, A9_RX_THRESH_READ);
> +		break;
> +	case A9_STATE_WRITE:
> +		aml_i2c_prepare_xfer(i2c, A9_FIFO_FILL_HALF);
> +		if ((i2c->pos + A9_I2C_HALF_FIFO) >= i2c->msg->len)
> +			aml_i2c_set_mask(i2c, REG_CFG_IRQ_ENABLE, A9_TX_THRESH_WRITE, 0);
> +		i2c->pos += i2c->count;
> +		/* Tx must clear thresh after write fifo */
> +		clear_irq_status(i2c, A9_TX_THRESH_WRITE);
> +		break;

[Severity: Medium]
When a zero-length write message (i2c->msg->len == 0) is submitted via
aml_i2c_xfer() -> aml_i2c_xfer_msg(), aml_i2c_prepare_xfer() calls
aml_i2c_put_data() and writes 0x00 to REG_CFG_TX before starting the
transfer.

When the transfer finishes and A9_PHY_DONE triggers aml_i2c_irq(), the
A9_STATE_WRITE case runs unconditionally without checking if bytes remain to
be written or if A9_TX_THRESH_WRITE is set. Because i2c->msg->len is still 0,
aml_i2c_prepare_xfer() -> aml_i2c_put_data() writes a second dummy 0x00 byte
to REG_CFG_TX during completion.

Also, given the comment in aml_i2c_put_data() noting that the hardware must
send at least 1 byte when transferring 0 bytes, does a 0-byte transfer
actually transmit a data byte on the bus? If the controller cannot perform
true zero-length transfers, should aml_i2c_quirks set I2C_AQ_NO_ZERO_LEN
since aml_i2c_func() advertises I2C_FUNC_SMBUS_EMUL (which includes SMBus
Quick commands)?

> +	default:
> +		break;
> +	}
> +
> +	if (!(irq_state & A9_TRANS_DONE) ||
> +	    (i2c->state == A9_STATE_READ && i2c->pos < i2c->msg->len))
> +		goto irq_trans_out;
> +
> +complete_irq_out:
> +	i2c->state = A9_STATE_IDLE;
> +	reset_fifo_pos(i2c);
> +	complete(&i2c->done);
> +	aml_i2c_set_mask(i2c, REG_CFG_IRQ_ENABLE, A9_RX_THRESH_READ | A9_TX_THRESH_WRITE, 0);
> +	clear_irq_status(i2c, IRQ_ALL_MASK);
> +irq_trans_out:
> +	spin_unlock(&i2c->lock);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void aml_i2c_do_start(struct aml_i2c *i2c, struct i2c_msg *msg)
> +{
> +	unsigned int reg_val;
> +	unsigned int trans_len;
> +
> +	trans_len = msg->len;
> +	reg_val = (unsigned int)(((msg->addr & 0x7f) << START_SADDR_SHIFT) |
> +				((msg->flags & I2C_M_RD) ? START_READ : 0) |
> +				((trans_len & 0xfff) << START_LEN_SHIFT) | START_START);
> +	aml_i2c_set_mask(i2c, REG_CFG_START,
> +			 START_START | START_SLAVE_ADDR | START_READ | START_LEN, reg_val);
> +	clear_irq_status(i2c, IRQ_ALL_MASK);
> +	/* enable thresh mode */
> +	if (trans_len > A9_I2C_FIFO_DEPTH) {
> +		reg_val = (msg->flags & I2C_M_RD) ? A9_RX_THRESH_READ : A9_TX_THRESH_WRITE;
> +		aml_i2c_set_mask(i2c, REG_CFG_IRQ_ENABLE,
> +				 A9_RX_THRESH_READ | A9_TX_THRESH_WRITE, reg_val);
> +	}
> +}

[Severity: Medium]
In aml_i2c_do_start(), START_START is written to REG_CFG_START before
clear_irq_status(i2c, IRQ_ALL_MASK) is called and before the threshold
interrupts are enabled in REG_CFG_IRQ_ENABLE.

If execution is delayed between writing REG_CFG_START and clearing
REG_CFG_IRQ_STATE (for instance on PREEMPT_RT where spin_lock_irqsave() does
not disable preemption, or due to an NMI or hypervisor preemption), can the
hardware assert A9_NCK_ERROR or A9_PHY_DONE for the new transfer only for
clear_irq_status(i2c, IRQ_ALL_MASK) to immediately wipe it out, causing
aml_i2c_irq() to miss A9_TRANS_DONE and wait_for_completion_timeout() in
aml_i2c_xfer_msg() to time out?

> +
> +static int aml_i2c_xfer_msg(struct aml_i2c *i2c, struct i2c_msg *msg, bool last)
> +{
[ ... ]
> +	if (!time_left) {
> +		spin_lock_irqsave(&i2c->lock, flags);
> +		i2c->state = A9_STATE_IDLE;
> +		clear_irq_status(i2c, IRQ_ALL_MASK);
> +		reset_fifo_pos(i2c);
> +		spin_unlock_irqrestore(&i2c->lock, flags);
> +		return -ETIMEDOUT;
> +	}

[Severity: High]
When a transfer with msg->len > A9_I2C_FIFO_DEPTH (32 bytes) is started,
aml_i2c_do_start() enables A9_RX_THRESH_READ or A9_TX_THRESH_WRITE in
REG_CFG_IRQ_ENABLE.

If that transfer times out in aml_i2c_xfer_msg(), this timeout path resets
i2c->state, clears REG_CFG_IRQ_STATE, and resets the FIFO pointers, but
unlike complete_irq_out in aml_i2c_irq(), it does not disable
A9_RX_THRESH_READ | A9_TX_THRESH_WRITE in REG_CFG_IRQ_ENABLE, nor does it
clear START_START in REG_CFG_START to halt the controller.

When a subsequent transfer with msg->len <= A9_I2C_FIFO_DEPTH runs,
aml_i2c_do_start() skips updating REG_CFG_IRQ_ENABLE:

	if (trans_len > A9_I2C_FIFO_DEPTH) {
		reg_val = (msg->flags & I2C_M_RD) ? A9_RX_THRESH_READ : A9_TX_THRESH_WRITE;
		aml_i2c_set_mask(i2c, REG_CFG_IRQ_ENABLE,
				 A9_RX_THRESH_READ | A9_TX_THRESH_WRITE, reg_val);
	}

Because aml_i2c_irq() dispatches strictly on i2c->state without checking
which bit in irq_state triggered the interrupt, won't a stale
A9_TX_THRESH_WRITE interrupt left enabled from a timed-out write fire during
a subsequent <= 32-byte read (i2c->state == A9_STATE_READ), causing
aml_i2c_irq() to read from an empty REG_CFG_RX FIFO into i2c->msg->buf while
leaving A9_TX_THRESH_WRITE uncleared?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-a9-i2c-v3-0-7b30ccc00533@amlogic.com?part=2