[PATCH v7 2/2] i2c: amlogic: Add Amlogic A9 I2C controller driver
sashiko-bot at kernel.org
sashiko-bot at kernel.org
Thu Nov 5 08:11:34 PST 2026
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] i2c: amlogic: unstopped hw transfer on timeout in aml_i2c_xfer_msg()
- [High] i2c: amlogic-a9: race between complete() and reinit_completion() on timeout
- [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 c27eb9a87c5b273047db1979d97f8c36648a9931
Author: Junyi Zhao <junyi.zhao at 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..d282f275d4d32
> --- /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));
> + clear_irq_status(i2c, IRQ_ALL_MASK);
> + spin_unlock(&i2c->lock);
> + return IRQ_HANDLED;
> + }
> +
> + 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]
As reported by Sashiko on v2 [1], in aml_i2c_irq(), can multi-chunk I2C read
transfers (where msg->len > A9_I2C_FIFO_DEPTH) miss the transfer completion
interrupt and time out in wait_for_completion_timeout()?
In Documentation/devicetree/bindings/i2c/amlogic,a9-i2c.yaml, the controller
interrupt is configured as rising-edge triggered (IRQ_TYPE_EDGE_RISING), which
requires a 0-to-1 signal transition to latch a new pending interrupt in the
GIC.
For a read transfer with msg->len > A9_I2C_FIFO_DEPTH, aml_i2c_do_start()
enables A9_RX_THRESH_READ in REG_CFG_IRQ_ENABLE alongside A9_PHY_DONE and
A9_NCK_ERROR. Unlike A9_STATE_WRITE, which disables A9_TX_THRESH_WRITE once
the final bytes are written to the TX FIFO, A9_STATE_READ never disables
A9_RX_THRESH_READ when the remaining bytes fit in a single FIFO chunk.
Because irq_state = readl(i2c->regs + REG_CFG_IRQ_STATE) is sampled only once
at the start of aml_i2c_irq() before reading up to 32 bytes from REG_CFG_RX:
1. When msg->len is a multiple of 32, the 32nd byte of the final chunk can
trigger A9_RX_THRESH_READ before the I2C NACK and STOP condition finish on the
bus. If A9_PHY_DONE asserts in REG_CFG_IRQ_STATE during the loop reading
REG_CFG_RX before clear_irq_status(i2c, A9_RX_THRESH_READ) runs, the
controller interrupt output stays continuously high without generating a new
rising edge, while aml_i2c_irq() checks the stale irq_state snapshot and
returns early without completing i2c->done even though i2c->pos ==
i2c->msg->len.
2. Similarly, if both A9_RX_THRESH_READ and A9_PHY_DONE are asserted in
REG_CFG_IRQ_STATE when more than 32 bytes remain (so i2c->pos < i2c->msg->len
after reading one 32-byte chunk), aml_i2c_irq() clears only A9_RX_THRESH_READ
and returns early with A9_PHY_DONE still asserted and fewer than 32 bytes left
in the RX FIFO. Because A9_PHY_DONE remains asserted continuously, no new
0-to-1 edge occurs to re-trigger the interrupt handler for the remaining
bytes.
> +
> +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);
> + clear_irq_status(i2c, IRQ_ALL_MASK);
> + spin_unlock(&i2c->lock);
> + complete(&i2c->done);
> +
> + 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]
As reported by Sashiko on v2 [2], in aml_i2c_do_start(), could starting the
hardware transfer before clearing REG_CFG_IRQ_STATE and enabling the FIFO
threshold interrupts lose a fast completion or NACK status bit?
In aml_i2c_xfer() -> aml_i2c_xfer_msg() -> aml_i2c_do_start(),
aml_i2c_set_mask() writes START_START to REG_CFG_START 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.
If execution is delayed between writing REG_CFG_START and writing
REG_CFG_IRQ_STATE (for example due to task preemption while holding spinlock_t
on PREEMPT_RT, vCPU preemption, or interrupt/firmware latency), the hardware
can assert A9_NCK_ERROR or A9_PHY_DONE and have it immediately 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() then runs, it sees irq_state
== 0, misses A9_TRANS_ERROR, touches REG_CFG_RX or REG_CFG_TX unconditionally,
and exits at !(irq_state & A9_TRANS_DONE) without calling complete(&i2c->done),
causing wait_for_completion_timeout() in aml_i2c_xfer_msg() to hang for 500 ms
and return -ETIMEDOUT.
Should clear_irq_status(i2c, IRQ_ALL_MASK) and the REG_CFG_IRQ_ENABLE setup
happen 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);
[Severity: High]
Can a late interrupt from a completing transfer race with the timeout path and
a subsequent call to reinit_completion(&i2c->done) in aml_i2c_xfer_msg()?
In aml_i2c_irq(), the complete_irq_out path sets i2c->state = A9_STATE_IDLE,
releases i2c->lock via spin_unlock(&i2c->lock), and then calls
complete(&i2c->done) outside i2c->lock. Meanwhile, aml_i2c_xfer_msg()
initializes i2c->msg, i2c->pos, and i2c->error before acquiring i2c->lock and
calling reinit_completion(&i2c->done).
If a transfer finishes on CPU 1 just as wait_for_completion_timeout() expires
on CPU 0:
CPU 1 (aml_i2c_irq) CPU 0 (aml_i2c_xfer_msg)
------------------- ------------------------
spin_unlock(&i2c->lock);
/* !time_left timeout path */
spin_lock_irqsave(&i2c->lock, flags);
...
spin_unlock_irqrestore(&i2c->lock, flags);
return -ETIMEDOUT;
/* next message starts */
i2c->msg = msg;
spin_lock_irqsave(&i2c->lock, flags);
i2c->state = A9_STATE_READ;
reinit_completion(&i2c->done);
aml_i2c_do_start(i2c, msg);
spin_unlock_irqrestore(&i2c->lock, flags);
wait_for_completion_timeout(&i2c->done, ...);
complete(&i2c->done);
In this case, CPU 1's delayed complete(&i2c->done) wakes
wait_for_completion_timeout() immediately while the new transfer is still
active in hardware. That allows aml_i2c_xfer_msg() to return early while
aml_i2c_irq() can still access i2c->msg and write to i2c->msg->buf after the
caller frees or pops the buffer, or while the next message in aml_i2c_xfer()
overwrites i2c->msg, i2c->pos, and i2c->error without holding i2c->lock.
Should complete(&i2c->done) be called while holding i2c->lock, and should
i2c->msg, i2c->pos, and i2c->error be accessed under i2c->lock?
> +
> + 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) {
> + spin_lock_irqsave(&i2c->lock, flags);
> + 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);
> + spin_unlock_irqrestore(&i2c->lock, flags);
> + return -ETIMEDOUT;
> + }
[Severity: High]
As reported by Sashiko on v2 [3], when wait_for_completion_timeout() expires
in aml_i2c_xfer_msg(), does the timeout cleanup leave the hardware transfer
running on the bus?
In the !time_left block, the driver resets i2c->state = A9_STATE_IDLE,
disables A9_RX_THRESH_READ | A9_TX_THRESH_WRITE in REG_CFG_IRQ_ENABLE, clears
REG_CFG_IRQ_STATE, and calls reset_fifo_pos(i2c), but it never clears
START_START in REG_CFG_START (or otherwise halts the controller) while
A9_ENABLE_IRQ_BIT (A9_NCK_ERROR | A9_PHY_DONE) remains enabled in
REG_CFG_IRQ_ENABLE.
If a stalled bus transfer continues shifting bytes after reset_fifo_pos(i2c),
it will advance the hardware FIFO pointers before the next transfer calls
aml_i2c_prepare_xfer(). Furthermore, if the stalled transfer finishes
(A9_PHY_DONE) or NACKs (A9_NCK_ERROR) after a subsequent call to
aml_i2c_xfer_msg() has set i2c->state to A9_STATE_READ or A9_STATE_WRITE and
released i2c->lock, aml_i2c_irq() will service that stale interrupt against
the new i2c->msg, corrupting i2c->msg->buf, i2c->pos, and i2c->error and
prematurely completing i2c->done.
Should the timeout path clear START_START in REG_CFG_START to abort the active
hardware transfer under i2c->lock?
[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/20261104-a9-i2c-v7-0-b31f2b53faf4@amlogic.com?part=2
More information about the linux-amlogic
mailing list