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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] i2c: amlogic: hardware registers accessed before clock is enabled
- [High] i2c: amlogic: race condition drops completion interrupts
- [High] i2c: amlogic: silent truncation of large transfers
- [High] i2c: amlogic: broken Repeated START for subsequent transfers
- [High] i2c: amlogic: data race on REG_CGF_IRQ_ENABLE
- [High] i2c: amlogic: IRQ storm on transfer timeout
- [High] i2c: amlogic: race in lockless state initialization
- [Medium] i2c: amlogic: crash due to early clock disable in remove
--

commit 5e779e1911cc216efb8179fa9e2910973c79bb74
Author: Junyi Zhao <junyi.zhao@amlogic.com>

i2c: amlogic: Add Amlogic A9 I2C controller driver
    
Add support for the new I2C controller used by Amlogic A9 SoCs.

> diff --git a/drivers/i2c/busses/i2c-amlogic-a9.c b/drivers/i2c/busses/i2c-amlogic-a9.c
> new file mode 100644
> --- /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;
> +	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 STATE_READ:

[ ... ]

> +	case STATE_WRITE:

[ ... ]

> +	case STATE_IDLE:
> +		dev_warn(i2c->dev, "i2c irq triggered abnormally!!!, irq_state:0x%x,irq_en:0x%x\n",
> +			 readl(i2c->regs + REG_CGF_IRQ_STATE),
> +			 readl(i2c->regs + REG_CGF_IRQ_ENABLE));
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (!controller_is_done(i2c)) {
> +		goto irq_trans_out;
> +	} else {

[Severity: High]
Could an unhandled interrupt cause an infinite IRQ storm and CPU lockup here?
When a transfer times out, aml_i2c_xfer_msg() resets the state to STATE_IDLE
but doesn't appear to disable the hardware threshold interrupts. If the
delayed interrupt fires, this handler logs a warning and returns IRQ_HANDLED
without clearing the interrupt source.

[ ... ]

> +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 << 1) |
> +				((msg->flags & I2C_M_RD) ? START_READ : 0) |
> +				((trans_len & 0xfff) << 12) | START_START);

[Severity: High]
Does this silently truncate I2C message lengths greater than 4095 bytes?
The length is masked with 0xfff without returning an error to the caller,
which could result in silent data corruption for large transfers.

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

[Severity: High]
Could clear_irq_status() inadvertently drop a valid completion interrupt?
If the hardware responds extremely fast (for example, generating a NACK on
the address byte) immediately after START_START is written, the completion
interrupt might get cleared before the handler has a chance to run, causing
the transfer to spuriously time out.

> +	/* 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_CGF_IRQ_ENABLE,
> +				 A9_RX_THRESH_READ | A9_TX_THRESH_WRITE, reg_val);

[Severity: High]
Is it safe to perform a read-modify-write on REG_CGF_IRQ_ENABLE here without
holding i2c->lock? If a hardware interrupt asserts immediately after the
transfer starts, a concurrent aml_i2c_irq() running on another CPU might
also modify REG_CGF_IRQ_ENABLE, causing a data race that loses updates.

> +	}
> +}
> +
> +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) ? STATE_READ : STATE_WRITE;
> +	aml_i2c_prepare_xfer(i2c, FIFO_FILL_FULL);
> +	i2c->pos += i2c->count;
> +	reinit_completion(&i2c->done);

[Severity: High]
Are these transfer state variables reinitialized without holding i2c->lock?
If a delayed interrupt from a previous timed-out transfer fires concurrently,
it might read this inconsistent state or falsely complete this new transfer,
potentially causing memory corruption or an invalid state machine transition.

> +
> +	if (last)
> +		aml_i2c_add_stop(i2c);

[Severity: High]
While this clears the BUS_NO_STOP bit for the final message in a transfer,
is there missing logic to restore it to 1 for non-last messages in new
transfers? Without setting it back, subsequent multi-message transfers might
incorrectly emit a STOP between messages, causing devices requiring Repeated
START to fail.

> +	/* Start the transfer */
> +	aml_i2c_do_start(i2c, msg);
> +
> +	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 = STATE_IDLE;
> +		clear_irq_status(i2c, IRQ_ALL_MASK);
> +		reset_fifo_pos(i2c);
> +		ret = -ETIMEDOUT;
> +	}

[Severity: High]
When a transfer times out, is it necessary to also disable the hardware
interrupts in REG_CGF_IRQ_ENABLE? As currently written, the hardware might
still assert a delayed interrupt once this function returns.

[ ... ]

> +static int aml_i2c_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct aml_i2c *i2c;
> +	struct i2c_timings timings;
> +	int irq, ret;

[ ... ]

> +	i2c->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(i2c->clk)) {
> +		dev_err(&pdev->dev, "can't get device clock\n");
> +		return PTR_ERR(i2c->clk);
> +	}
> +
> +	i2c->clk_rate = clk_get_rate(i2c->clk);
> +	if (i2c->clk_rate == 0) {
> +		dev_err(&pdev->dev, "failed to get clk rate\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = aml_i2c_init(i2c);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "fail init i2c\n");
> +		return ret;
> +	}

[Severity: High]
Does aml_i2c_init() attempt to write to hardware registers before the clock
is enabled? clk_prepare_enable() appears to be called further down in this
function. Accessing unclocked registers could trigger a synchronous external
abort on ARM and crash the system.

> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "can't find IRQ\n");
> +		return irq;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, irq, aml_i2c_irq, 0, dev_name(&pdev->dev), i2c);

[Severity: Medium]
Since the interrupt is requested using devres here, it will remain active
until the devres cleanup runs after aml_i2c_remove() returns. Could this
leave a vulnerable window during removal where a spurious interrupt might
attempt to read hardware registers after the clock has been disabled?

> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "can't request IRQ\n");
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(i2c->clk);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "can't prepare clock\n");
> +		return ret;
> +	}
> +
> +	strscpy(i2c->adap.name, "Amlogic I2C adapter",
> +		sizeof(i2c->adap.name));
> +	i2c->adap.owner = THIS_MODULE;
> +	i2c->adap.algo = &aml_i2c_algorithm;
> +	i2c->adap.dev.parent = &pdev->dev;
> +	i2c->adap.dev.of_node = np;
> +	i2c->adap.algo_data = i2c;
> +
> +	ret = i2c_add_adapter(&i2c->adap);

[Severity: High]
Could the silent truncation of large transfers observed in aml_i2c_do_start()
be avoided by setting the quirks pointer on i2c->adap here? Populating the
quirks would allow the I2C core to properly enforce the 4095-byte hardware
limit and return an error to the caller natively.

[ ... ]

> +static void aml_i2c_remove(struct platform_device *pdev)
> +{
> +	struct aml_i2c *i2c = platform_get_drvdata(pdev);
> +
> +	i2c_del_adapter(&i2c->adap);
> +	clk_disable_unprepare(i2c->clk);
> +}

[Severity: Medium]
Is the clock being manually disabled here before the interrupt handler is
cleaned up? As mentioned in aml_i2c_probe(), the devm-managed IRQ is still
active at this point. A spurious interrupt arriving after this line could
crash the system by accessing unclocked registers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-a9-i2c-v1-0-b8ad9b46f4a8@amlogic.com?part=2

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