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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] i2c: amlogic: unstopped hw transfer 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: missed completion in aml_i2c_irq() on multi-chunk reads
- [Low] i2c: amlogic: use of deprecated master_xfer in aml_i2c_algorithm
- [Low] i2c: amlogic-a9: off-by-one minimum divider check in aml_i2c_set_clk_div()
--

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

i2c: amlogic: Add Amlogic A9 I2C controller driver

This commit adds a driver for the I2C controller found on Amlogic A9 SoCs,
along with the 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..d7f88d6feb05b
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-amlogic-a9.c
> @@ -0,0 +1,431 @@

[ ... ]

> +/*
> + * SCL = clk/b_ratio. if b_ratio<=8, SCL = clk/8
> + */
> +static int aml_i2c_set_clk_div(struct aml_i2c *i2c)
> +{
> +	unsigned long clk_rate = i2c->clk_rate;
> +	unsigned int div;
> +
> +	div = DIV_ROUND_UP(clk_rate, i2c->frequency);
> +
> +	/* clock divider has 12 bits */
> +	if (div >= (1 << 12)) {
> +		dev_err(i2c->dev, "requested bus frequency too low\n");
> +		return -EINVAL;
> +	} else if (div <= 8) {
> +		dev_err(i2c->dev, "requested bus frequency too high\n");
> +		return -EINVAL;
> +	}

[Severity: Low]
In aml_i2c_set_clk_div(), the comment notes that b_ratio <= 8 yields
SCL = clk / 8. Since div == 8 is the minimum divider that produces the exact
clk / 8 frequency without clamping, should this check be div < 8 rather than
div <= 8 so that a valid divider of 8 is not rejected with -EINVAL?

[ ... ]

> +static irqreturn_t aml_i2c_irq(int irqno, void *dev_id)
> +{
> +	struct aml_i2c *i2c = dev_id;
> +	u8 *buf = NULL;
> +	u32 rd_len, i;
> +	u32 rd_val;
> +	u32 irq_state;
> +
> +	spin_lock(&i2c->lock);
> +
> +	irq_state = readl(i2c->regs + REG_CFG_IRQ_STATE);
> +	if (i2c->state == A9_STATE_IDLE) {
> +		dev_warn(i2c->dev, "i2c irq triggered abnormally, irq_state:0x%x,irq_en:0x%x\n",
> +			 irq_state, readl(i2c->regs + REG_CFG_IRQ_ENABLE));
> +		goto complete_irq_out;
> +	}
> +
> +	if (irq_state & A9_TRANS_ERROR) {
> +		i2c->error = -ENXIO;
> +		goto complete_irq_out;
> +	}
> +
> +	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 == i2c->msg->len)
> +			aml_i2c_set_mask(i2c, REG_CFG_IRQ_ENABLE, A9_TX_THRESH_WRITE, 0);
> +		/* Tx must clear thresh after write fifo */
> +		clear_irq_status(i2c, A9_TX_THRESH_WRITE);
> +		break;
> +	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;
> +}

[Severity: Medium]
As reported by Sashiko on v2 [1], can multi-chunk I2C read transfers
(msg->len > A9_I2C_FIFO_DEPTH) miss the transfer completion interrupt in
aml_i2c_irq()?

Unlike A9_STATE_WRITE, which disables A9_TX_THRESH_WRITE once the final bytes
are queued, A9_STATE_READ leaves A9_RX_THRESH_READ enabled and samples
REG_CFG_IRQ_STATE only once upon entry.

When msg->len is a multiple of 32 (for example, 64), the 32nd byte of the
final chunk triggers A9_RX_THRESH_READ before the I2C STOP condition finishes
on the bus. If A9_PHY_DONE asserts while aml_i2c_irq() is draining those 32
bytes from REG_CFG_RX and before A9_RX_THRESH_READ is cleared, the interrupt
line remains continuously asserted without generating a new rising edge, while
aml_i2c_irq() takes goto irq_trans_out because the initial irq_state snapshot
did not have A9_TRANS_DONE set.

Similarly, if both A9_RX_THRESH_READ and A9_PHY_DONE are set when
aml_i2c_irq() enters with more than 32 bytes remaining, aml_i2c_irq() drains
only 32 bytes, clears only A9_RX_THRESH_READ, and exits via irq_trans_out with
i2c->pos < i2c->msg->len. Because A9_PHY_DONE is already asserted and fewer
than 32 bytes remain to re-trigger A9_RX_THRESH_READ, no further interrupt edge
occurs, leaving wait_for_completion_timeout() in aml_i2c_xfer_msg() to time
out with -ETIMEDOUT.

> +
> +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]
As reported by Sashiko on v2 [2], in aml_i2c_do_start(), setting START_START
in REG_CFG_START begins the hardware transfer while A9_ENABLE_IRQ_BIT
(A9_NCK_ERROR | A9_PHY_DONE) is already enabled in REG_CFG_IRQ_ENABLE, before
clear_irq_status(i2c, IRQ_ALL_MASK) is called and before A9_RX_THRESH_READ or
A9_TX_THRESH_WRITE is enabled in REG_CFG_IRQ_ENABLE.

If execution is delayed between writing REG_CFG_START and writing
REG_CFG_IRQ_STATE (for example, due to task preemption under CONFIG_PREEMPT_RT,
vCPU preemption, or NMI/FIQ latency), the controller can assert A9_NCK_ERROR
on an address NACK or A9_PHY_DONE on a short transfer, which is then wiped out
by clear_irq_status(i2c, IRQ_ALL_MASK) before aml_i2c_irq() acquires i2c->lock
and reads REG_CFG_IRQ_STATE. When aml_i2c_irq() runs, it sees
!(irq_state & A9_TRANS_DONE) and exits via irq_trans_out without completing
i2c->done, causing aml_i2c_xfer_msg() to time out.

Should the interrupt status be cleared and the threshold interrupts enabled
before asserting START_START in REG_CFG_START?

> +
> +static int aml_i2c_xfer_msg(struct aml_i2c *i2c, struct i2c_msg *msg, bool last)
> +{
> +	unsigned long time_left, flags;
> +
> +	i2c->msg = msg;
> +	i2c->pos = 0;
> +	i2c->error = 0;
> +
> +	spin_lock_irqsave(&i2c->lock, flags);
> +	i2c->state = (msg->flags & I2C_M_RD) ? A9_STATE_READ : A9_STATE_WRITE;
> +	aml_i2c_prepare_xfer(i2c, A9_FIFO_FILL_FULL);
> +	reinit_completion(&i2c->done);
> +
> +	aml_i2c_set_mask(i2c, REG_CFG_BUS, BUS_NO_STOP, (last ? 0 : BUS_NO_STOP));
> +
> +	/* Start the transfer */
> +	aml_i2c_do_start(i2c, msg);
> +	spin_unlock_irqrestore(&i2c->lock, flags);
> +
> +	time_left = msecs_to_jiffies(I2C_TIMEOUT_MS);
> +	time_left = wait_for_completion_timeout(&i2c->done, time_left);
> +
> +	/*
> +	 * Protect access to i2c struct and registers from interrupt
> +	 * handlers triggered by a transfer terminated after the
> +	 * timeout period
> +	 */
> +
> +	if (!time_left) {
> +		guard(spinlock_irqsave)(&i2c->lock);
> +		i2c->state = A9_STATE_IDLE;
> +		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);
> +		reset_fifo_pos(i2c);
> +		return -ETIMEDOUT;
> +	}
> +
> +	return i2c->error;
> +}

[Severity: High]
As reported by Sashiko on v2 [3], when wait_for_completion_timeout() expires
(!time_left) in aml_i2c_xfer_msg(), this timeout path sets i2c->state to
A9_STATE_IDLE, disables the threshold IRQs, clears REG_CFG_IRQ_STATE, and
calls reset_fifo_pos(i2c), but never clears START_START in REG_CFG_START or
otherwise aborts the active hardware transfer while A9_ENABLE_IRQ_BIT remains
enabled in REG_CFG_IRQ_ENABLE.

If the stalled transfer shifts additional bytes after reset_fifo_pos(i2c), the
hardware FIFO pointers become non-zero for the next message. Furthermore, if
the stalled transfer finishes or NACKs after a subsequent aml_i2c_xfer_msg()
has set i2c->state to A9_STATE_READ or A9_STATE_WRITE, aml_i2c_irq() will
process the stale A9_PHY_DONE or A9_NCK_ERROR interrupt against the new
i2c->msg, corrupting i2c->msg->buf, i2c->pos, and i2c->error and prematurely
completing i2c->done. Even if the late interrupt arrives while i2c->state is
still A9_STATE_IDLE, aml_i2c_irq() jumps to complete_irq_out and calls
complete(&i2c->done) when no transfer is waiting.

Also, this function mixes manual spin_lock_irqsave() / spin_unlock_irqrestore()
with guard(spinlock_irqsave)(&i2c->lock) without directly including
linux/cleanup.h. Could the hardware transfer be explicitly halted on timeout,
the spurious complete(&i2c->done) when A9_STATE_IDLE avoided, and the locking
style made consistent across aml_i2c_xfer_msg()?

[ ... ]

> +static const struct i2c_algorithm aml_i2c_algorithm = {
> +	.master_xfer	= aml_i2c_xfer,
> +	.functionality	= aml_i2c_func,
> +};

[Severity: Low]
In struct i2c_algorithm, master_xfer is marked deprecated in include/linux/i2c.h
in favor of xfer. Could aml_i2c_algorithm initialize .xfer = aml_i2c_xfer
instead?

[1] https://lore.kernel.org/r/sashiko-outbox-181284@kernel.org
[2] https://lore.kernel.org/r/sashiko-outbox-181284@kernel.org
[3] https://lore.kernel.org/r/sashiko-outbox-181284@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-a9-i2c-v5-0-86f2dcd179d8@amlogic.com?part=2