Re: [PATCH v2 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-a9: unlocked state init and late IRQ setup in aml_i2c_do_start()
- [High] i2c: amlogic-a9: threshold IRQs left enabled on timeout in aml_i2c_xfer_msg()
- [High] i2c: amlogic-a9: unhandled IRQ and spurious complete() in aml_i2c_irq()
- [High] i2c: amlogic: missing RX drain in aml_i2c_irq() when A9_PHY_DONE is set
- [Medium] i2c: amlogic-a9: unmasked msg->addr in aml_i2c_do_start() corrupts START_LEN
--

commit 10e276126f680fbcf2e402bbde459a690540ef33
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..d4954a4fd88ef
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-amlogic-a9.c
> @@ -0,0 +1,469 @@

[ ... ]

> +static irqreturn_t aml_i2c_irq(int irqno, void *dev_id)
> +{
> +	struct aml_i2c *i2c = dev_id;
> +	u8 *buf = NULL;
> +	int ret;
> +	u32 rd_len, i;
> +	u32 rd_val;
> +
> +	spin_lock(&i2c->lock);
> +	ret = controller_is_error(i2c);
> +	if (ret) {
> +		i2c->error = ret;
> +		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 + 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;
> +	case A9_STATE_IDLE:
> +		dev_warn(i2c->dev, "i2c irq triggered abnormally, irq_state:0x%x,irq_en:0x%x\n",
> +			 readl(i2c->regs + REG_CFG_IRQ_STATE),
> +			 readl(i2c->regs + REG_CFG_IRQ_ENABLE));
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (!controller_is_done(i2c)) {
> +		goto irq_trans_out;
> +	} else {
> +		if (i2c->state == A9_STATE_READ && (i2c->msg->len - i2c->pos))
> +			goto irq_trans_out;
> +	}

[Severity: High]
Can a multi-chunk read (msg->len > A9_I2C_FIFO_DEPTH) hang in
drivers/i2c/busses/i2c-amlogic-a9.c:aml_i2c_irq() if the remaining bytes
finish arriving before A9_RX_THRESH_READ is cleared?

In aml_i2c_xfer() -> aml_i2c_xfer_msg() -> aml_i2c_do_start(), a read with
msg->len > 32 enables A9_RX_THRESH_READ alongside A9_PHY_DONE, with
I2C_RX_THR configured to 32. When the first 32 bytes arrive, A9_RX_THRESH_READ
asserts. If interrupt handling is slightly delayed or the remaining tail bytes
finish arriving while the A9_STATE_READ loop is reading the first 32 bytes
from REG_CFG_RX, A9_PHY_DONE will already be set in REG_CFG_IRQ_STATE before
clear_irq_status(i2c, A9_RX_THRESH_READ) runs.

Because A9_STATE_READ only drains up to A9_I2C_FIFO_DEPTH bytes per call,
i2c->msg->len - i2c->pos remains non-zero, and the controller_is_done(i2c)
branch takes goto irq_trans_out without draining the remaining tail bytes or
completing i2c->done. Since A9_PHY_DONE is already asserted and fewer than 32
bytes remain to re-trigger A9_RX_THRESH_READ, no new rising edge will occur on
the interrupt line, leaving wait_for_completion_timeout() in
aml_i2c_xfer_msg() to time out with -ETIMEDOUT.

Should aml_i2c_irq() drain the remaining bytes in the RX FIFO when
controller_is_done(i2c) is true during A9_STATE_READ before completing the
transfer?

> +
> +complete_irq_out:
> +	reset_fifo_pos(i2c);
> +	complete(&i2c->done);
> +	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);
> +irq_trans_out:
> +	spin_unlock(&i2c->lock);
> +
> +	return IRQ_HANDLED;
> +}

[Severity: High]
What happens in drivers/i2c/busses/i2c-amlogic-a9.c:aml_i2c_irq() when an
interrupt arrives while i2c->state is A9_STATE_IDLE?

First, controller_is_error(i2c) is checked before inspecting i2c->state. If a
late A9_NCK_ERROR interrupt fires after a transfer has already timed out and
transitioned to A9_STATE_IDLE, aml_i2c_irq() sets i2c->error = -ENXIO, jumps
to complete_irq_out, and calls complete(&i2c->done).

Second, in the switch statement, the A9_STATE_IDLE case logs a warning and
breaks into the completion checks:

	if (!controller_is_done(i2c)) {
		goto irq_trans_out;
	}

If a threshold interrupt fires while in A9_STATE_IDLE and controller_is_done()
is false, aml_i2c_irq() jumps to irq_trans_out and returns IRQ_HANDLED without
clearing REG_CFG_IRQ_STATE or masking the threshold interrupt in
REG_CFG_IRQ_ENABLE, leaving the interrupt asserted. Conversely, if
controller_is_done() is true (such as a late A9_PHY_DONE), execution falls
through to complete_irq_out and spuriously signals i2c->done while idle.

Should aml_i2c_irq() check for A9_STATE_IDLE up front, disable threshold
interrupts, clear REG_CFG_IRQ_STATE, and return without calling
complete(&i2c->done)?

> +
> +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 << 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);

[Severity: Medium]
In drivers/i2c/busses/i2c-amlogic-a9.c:aml_i2c_do_start(), could upper bits in
msg->addr corrupt the START_LEN field of REG_CFG_START?

When userspace submits an I2C_RDWR ioctl on /dev/i2c-*, msg->addr is a u16
passed through i2cdev_ioctl_rdwr() -> i2c_transfer() -> aml_i2c_xfer() ->
aml_i2c_xfer_msg() -> aml_i2c_do_start() without masking. Because
msg->addr << START_SADDR_SHIFT is not masked to 7 bits (or START_SLAVE_ADDR)
before being ORed with (trans_len & 0xfff) << START_LEN_SHIFT and written via
aml_i2c_set_mask() under the combined mask START_START | START_SLAVE_ADDR |
START_READ | START_LEN, any bits set in msg->addr[15:11] shift into
START_LEN (GENMASK(23, 12)) and alter the hardware transfer length.

Should msg->addr be masked (for example with 0x7f, since 10-bit addressing is
not advertised in aml_i2c_func()) before shifting by START_SADDR_SHIFT?

> +	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);
> +	}
> +}
> +
> +static int aml_i2c_xfer_msg(struct aml_i2c *i2c, struct i2c_msg *msg, bool last)
> +{
> +	unsigned long time_left, flags;
> +	int ret = 0;
> +
> +	i2c->msg = msg;
> +	i2c->pos = 0;
> +	i2c->count = 0;
> +	i2c->error = 0;
> +
> +	i2c->state = (msg->flags & I2C_M_RD) ? A9_STATE_READ : A9_STATE_WRITE;
> +	spin_lock_irqsave(&i2c->lock, flags);
> +	aml_i2c_prepare_xfer(i2c, A9_FIFO_FILL_FULL);
> +	i2c->pos += i2c->count;
> +	spin_unlock_irqrestore(&i2c->lock, flags);
> +	reinit_completion(&i2c->done);
> +
> +	if (last)
> +		aml_i2c_set_mask(i2c, REG_CFG_BUS, BUS_NO_STOP, 0);
> +	else
> +		aml_i2c_set_mask(i2c, REG_CFG_BUS, BUS_NO_STOP, BUS_NO_STOP);
> +	/* Start the transfer */
> +	aml_i2c_do_start(i2c, msg);

[Severity: High]
Can starting the hardware transfer before clearing IRQ status and enabling
threshold interrupts in drivers/i2c/busses/i2c-amlogic-a9.c:aml_i2c_do_start(),
without holding i2c->lock across transfer setup in aml_i2c_xfer_msg(), race
with aml_i2c_irq()?

In aml_i2c_do_start(), setting START_START in REG_CFG_START kicks off the
hardware transaction before clear_irq_status(i2c, IRQ_ALL_MASK) and before
enabling A9_RX_THRESH_READ or A9_TX_THRESH_WRITE in REG_CFG_IRQ_ENABLE:

	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);
	}

If the task is preempted or interrupted after writing REG_CFG_START, or if the
controller quickly asserts A9_NCK_ERROR (such as on an address NACK) or
A9_PHY_DONE before clear_irq_status() runs, clear_irq_status(i2c, IRQ_ALL_MASK)
can wipe out the newly asserted IRQ status bits and cause
wait_for_completion_timeout() to time out. Similarly, if aml_i2c_irq() runs
and completes on an early NACK before aml_i2c_do_start() updates
REG_CFG_IRQ_ENABLE, the unlocked read-modify-write in aml_i2c_do_start() can
re-enable A9_RX_THRESH_READ or A9_TX_THRESH_WRITE after complete_irq_out
already cleared them.

In addition, in aml_i2c_xfer_msg(), i2c->msg, i2c->pos, i2c->count,
i2c->error, and i2c->state are initialized outside i2c->lock, and i2c->lock is
released before reinit_completion(&i2c->done), REG_CFG_BUS configuration, and
aml_i2c_do_start(). If a late interrupt from a previous timed-out transfer
arrives while i2c->state is already set to A9_STATE_READ or A9_STATE_WRITE,
aml_i2c_irq() can modify i2c->pos and access the new i2c->msg->buf before the
new transfer begins (and if i2c->pos exceeds the new msg->len in
A9_STATE_READ, the signed subtraction i2c->msg->len - i2c->pos in
min(i2c->msg->len - i2c->pos, A9_I2C_FIFO_DEPTH) will wrap to a large u32 in
rd_len and overflow buf[i2c->pos++]).

Could the state initialization, reinit_completion(), bus configuration, IRQ
status clearing, threshold IRQ setup, and START_START trigger all be performed
under a single spin_lock_irqsave(&i2c->lock, flags) critical section, with
clear_irq_status() and REG_CFG_IRQ_ENABLE configured before asserting
START_START?

> +
> +	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
> +	 */
> +
> +	spin_lock_irqsave(&i2c->lock, flags);
> +	if (!time_left) {
> +		i2c->state = A9_STATE_IDLE;
> +		clear_irq_status(i2c, IRQ_ALL_MASK);
> +		reset_fifo_pos(i2c);
> +		ret = -ETIMEDOUT;
> +	}

[Severity: High]
When a transfer with msg->len > A9_I2C_FIFO_DEPTH times out in
drivers/i2c/busses/i2c-amlogic-a9.c:aml_i2c_xfer_msg(), aren't the threshold
interrupts in REG_CFG_IRQ_ENABLE and the active transfer in REG_CFG_START left
enabled?

In aml_i2c_do_start(), transfers with trans_len > A9_I2C_FIFO_DEPTH enable
A9_RX_THRESH_READ or A9_TX_THRESH_WRITE in REG_CFG_IRQ_ENABLE, and only
update REG_CFG_IRQ_ENABLE when trans_len > A9_I2C_FIFO_DEPTH. On normal
completion in aml_i2c_irq(), complete_irq_out clears A9_RX_THRESH_READ |
A9_TX_THRESH_WRITE in REG_CFG_IRQ_ENABLE, but the !time_left timeout path in
aml_i2c_xfer_msg() does not disable those threshold IRQ bits or stop the
transfer in REG_CFG_START.

If a >32-byte write times out and the next transfer is a short read with
msg->len <= 32, wouldn't A9_TX_THRESH_WRITE remain enabled and fire during
A9_STATE_READ, causing aml_i2c_irq() to read from an empty REG_CFG_RX FIFO
into msg->buf?

[ ... ]

> +MODULE_DESCRIPTION("Amlogic I2C Bus driver");
> +MODULE_AUTHOR("Junyi Zhao <junyi.zhao@amlogic.com>");
> +MODULE_LICENSE("GPL");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-a9-i2c-v2-0-45e3cef175bf@amlogic.com?part=2

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