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

[Xianwei Zhao <xianwei.zhao@amlogic.com>]

On 2026/10/22 17:46, sashiko-bot@kernel.org wrote:
> 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?
> 
No. According to the code logic, if the data has not been read 
completely, the condition for completing the operation is not met, so 
the interrupt status bit will not be cleared. Once the data transfer is 
complete, the interrupt will immediately be triggered again, allowing 
the handler to continue reading the data. Finally, once the read 
operation is complete and the condition is satisfied, the completion 
interrupt will be 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)?
> 

Later, I will check the status at the very beginning. If an abnormal 
interrupt occurs, I can skip the processing and clear irq state.

>> +
>> +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?
> 

Will add mask for address.

>> +     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?
> 
I will lock start. This way, there will be no conflict that causes 
interruption.

>> +
>> +     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?
> 

I will take corresponding measures to prevent this from happening.

> [ ... ]
> 
>> +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