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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] i2c: amlogic-a9: transfer started before IRQ setup in
  aml_i2c_do_start()
- [Medium] i2c: amlogic-a9: stale irq_state and edge IRQ miss in
  aml_i2c_irq()
--

commit 75985e9b577bcde7a76a8a6f844a5a7564d2426a
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 its
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..3e08a0333ceea
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-amlogic-a9.c

[ ... ]

> +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 irq_trans_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)) {
> +		spin_unlock(&i2c->lock);
> +		return IRQ_HANDLED;
> +	}

[Severity: Medium]
Can multi-chunk reads (msg->len > A9_I2C_FIFO_DEPTH) miss the completion
interrupt and time out in aml_i2c_xfer_msg()?

Documentation/devicetree/bindings/i2c/amlogic,a9-i2c.yaml:
        interrupts = <0 164 1>;

The interrupt is rising-edge-triggered (IRQ_TYPE_EDGE_RISING), so the
controller's enabled interrupt output must drop to 0 and transition back to 1
to latch a new interrupt in the GIC.

In aml_i2c_irq(), irq_state is sampled once before draining up to 32 bytes
from REG_CFG_RX.  On a 64-byte read, the 32nd byte of the second chunk asserts
A9_RX_THRESH_READ before the final NACK and STOP finish on the bus.  If
A9_PHY_DONE asserts while the handler is reading REG_CFG_RX, prior to
clear_irq_status(i2c, A9_RX_THRESH_READ), the interrupt output stays
continuously high across the clear without generating a new rising edge, while
aml_i2c_irq() checks the stale irq_state snapshot and returns without
completing i2c->done.

Similarly, if both A9_RX_THRESH_READ and A9_PHY_DONE are already set in
irq_state while more than 32 bytes remain to be read, aml_i2c_irq() reads 32
bytes, clears only A9_RX_THRESH_READ, and returns early because i2c->pos <
i2c->msg->len.  Because A9_PHY_DONE remains asserted in REG_CFG_IRQ_STATE and
fewer than 32 tail bytes remain in the FIFO, no new rising edge is generated
and wait_for_completion_timeout() hangs for 500 ms before returning
-ETIMEDOUT.

> +
> +complete_irq_out:
> +	i2c->state = A9_STATE_IDLE;
> +	reset_fifo_pos(i2c);
> +	aml_i2c_set_mask(i2c, REG_CFG_IRQ_ENABLE, A9_RX_THRESH_READ | A9_TX_THRESH_WRITE, 0);
> +	complete(&i2c->done);
> +irq_trans_out:
> +	clear_irq_status(i2c, IRQ_ALL_MASK);
> +	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]
Does writing START_START to REG_CFG_START before clearing IRQ_ALL_MASK and
enabling threshold interrupts race with hardware completion or NACK?

If execution is delayed between writing REG_CFG_START and calling
clear_irq_status(i2c, IRQ_ALL_MASK) (for example due to vCPU preemption or
PREEMPT_RT spinlock preemption), an address NACK or short transfer can assert
A9_NCK_ERROR or A9_PHY_DONE and then have its status bits wiped out by
clear_irq_status().  When aml_i2c_irq() then runs, it reads irq_state == 0,
misses A9_TRANS_ERROR or A9_TRANS_DONE, and returns without completing
i2c->done, causing wait_for_completion_timeout() to time out.  Clearing
REG_CFG_IRQ_STATE and configuring REG_CFG_IRQ_ENABLE before asserting
START_START avoids this window.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-a9-i2c-v8-0-1e9d2682f86b@amlogic.com?part=2

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic