Re: [PATCH v31 4/5] i2c: ast2600: Add controller driver for AST2600 new register set

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] A 0-length SMBus block read completes without issuing a STOP condition, permanently hanging the I2C bus.
- [High] Returning early in the IRQ handler when `i2c_bus->msgs` is NULL skips clearing the interrupt status, causing an infinite interrupt storm.
- [High] Calling `i2c_transfer` with `num == 0` causes an out-of-bounds access or NULL pointer dereference on the `msgs` array.
- [High] A race condition in the I2C transfer timeout path can lead to a Use-After-Free of the `i2c_bus->msgs` buffer.
- [High] Trailing interrupts unconditionally overwrite previous error codes and improperly resume aborted transactions, leading to Use-After-Free.
- [High] The interrupt handler accesses `i2c_bus->msgs[msgs_index]` without checking if `msgs_index` is within bounds.
- [High] A malformed device tree can cause a divide-by-zero kernel panic by setting `clock-frequency = <0>`.
- [High] The I2C controller interrupt handler fails to clear all packet status bits in the Write-Clear ISR register, leading to stale status bits and spurious timeouts.
- [Medium] Uninitialized variables `clk_div_reg` and `global_ctrl` are used if `regmap_read` fails.
- [Medium] The `AST2600_I2CM_BUS_RECOVER_FAIL` interrupt is not enabled in the interrupt enable register (`IER`), causing bus recovery failures to unnecessarily time out.
--

commit e2c4f0c2ecd8a3e8e1c9e3768ae911afff057121
Author: Ryan Chen <ryan_chen@aspeedtech.com>
Date:   Wed Jun 3 05:50:16 2026 +0000

    i2c: ast2600: Add controller driver for AST2600 new register set
    
    This commit adds an AST2600-specific I2C controller driver implementing the
    new register layout, including support for packet-based transfers. It probes
    based on the presence of the "aspeed,global-regs" device tree property.

> diff --git a/drivers/i2c/busses/i2c-ast2600.c b/drivers/i2c/busses/i2c-ast2600.c
> new file mode 100644
> index 0000000000000..35f38e1f47999
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-ast2600.c
> @@ -0,0 +1,864 @@
[ ... ]
> +static void ast2600_i2c_ac_timing_config(struct ast2600_i2c_bus *i2c_bus)
> +{
> +	unsigned long base_clk[16];
> +	int baseclk_idx = 0;
> +	int divisor = 0;
> +	u32 clk_div_reg;
> +	u32 scl_low;
> +	u32 scl_high;
> +	u32 data;
> +
> +	regmap_read(i2c_bus->global_regs, AST2600_I2CG_CLK_DIV_CTRL, &clk_div_reg);

[Severity: Medium]
If regmap_read() fails for the syscon I/O, does it leave clk_div_reg
uninitialized before using it in the divisor calculation below?

> +
> +	for (int i = 0; i < ARRAY_SIZE(base_clk); i++) {
> +		if (i == 0)
> +			base_clk[i] = i2c_bus->apb_clk;
> +		else if (i < 5)
> +			base_clk[i] = (i2c_bus->apb_clk * 2) /
> +			   (((clk_div_reg >> ((i - 1) * 8)) & GENMASK(7, 0)) + 2);
> +		else
> +			base_clk[i] = base_clk[4] >> (i - 4);
> +
> +		if ((base_clk[i] / i2c_bus->timing_info.bus_freq_hz) <= 32) {

[Severity: High]
If the device tree specifies clock-frequency = <0>, does i2c_parse_fw_timings()
leave bus_freq_hz as 0, resulting in a divide-by-zero kernel panic here?

[ ... ]
> +static int ast2600_i2c_setup_buff_rx(u32 cmd, struct ast2600_i2c_bus *i2c_bus)
> +{
> +	struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];
> +	int xfer_len = msg->len - i2c_bus->controller_xfer_cnt;
> +
> +	cmd |= AST2600_I2CM_PKT_EN | AST2600_I2CM_RX_BUFF_EN | AST2600_I2CM_RX_CMD;
> +
> +	if (cmd & AST2600_I2CM_START_CMD)
> +		cmd |= AST2600_I2CM_PKT_ADDR(msg->addr);
> +
> +	if (msg->flags & I2C_M_RECV_LEN) {
> +		dev_dbg(i2c_bus->dev, "smbus read\n");
> +		xfer_len = 1;
> +	} else if (xfer_len > i2c_bus->buf_size) {
> +		xfer_len = i2c_bus->buf_size;
> +	} else if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) {
> +		cmd |= CONTROLLER_TRIGGER_LAST_STOP;
> +	}

[Severity: High]
If this is an SMBus block read and the peripheral later returns a length of 0,
does the driver complete the message without ever issuing a STOP condition,
leaving the I2C bus permanently busy? The I2C_M_RECV_LEN check bypasses the
CONTROLLER_TRIGGER_LAST_STOP setup for the first byte.

[ ... ]
> +static int ast2600_i2c_do_start(struct ast2600_i2c_bus *i2c_bus)
> +{
> +	struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];

[Severity: High]
If the I2C subsystem calls i2c_transfer() with num == 0, does this code
dereference a NULL pointer or empty array since there are no checks on the
number of messages?

[ ... ]
> +static void ast2600_i2c_controller_packet_irq(struct ast2600_i2c_bus *i2c_bus, u32 sts)
> +{
> +	struct i2c_msg *msg;
> +	int xfer_len;
> +	int i;
> +
> +	if (!i2c_bus->msgs)
> +		return;

[Severity: High]
If a trailing hardware interrupt fires when no transfer is active, does
returning here without acknowledging the interrupt in the Write-Clear register
cause an infinite interrupt storm?

> +	msg = &i2c_bus->msgs[i2c_bus->msgs_index];

[Severity: High]
If a spurious or trailing interrupt arrives after a transfer completes but
before msgs is set to NULL, can msgs_index equal msgs_count here?
If so, does this cause an out-of-bounds pointer calculation and a subsequent
out-of-bounds read in ast2600_i2c_clamp_len()?

> +
> +	sts &= ~AST2600_I2CM_PKT_DONE;
> +	writel(AST2600_I2CM_PKT_DONE, i2c_bus->reg_base + AST2600_I2CM_ISR);

[Severity: High]
Since AST2600_I2CM_ISR is a Write-Clear register, does writing only
AST2600_I2CM_PKT_DONE fail to clear the other status bits contained in sts?
Will those stale bits accumulate and cause subsequent transfers to fall
through the switch statement below and timeout?

[ ... ]
> +static int ast2600_i2c_controller_irq(struct ast2600_i2c_bus *i2c_bus)
> +{
> +	u32 sts = readl(i2c_bus->reg_base + AST2600_I2CM_ISR);
> +	u32 ctrl;
[ ... ]
> +	i2c_bus->cmd_err = ast2600_i2c_irq_err_to_errno(sts);

[Severity: High]
If an error like arbitration loss occurs, and the hardware later issues a
trailing NORMAL_STOP interrupt before the process context cleans up, does this
unconditionally overwrite the existing error code and improperly resume the
next step of an already failed transaction?

[ ... ]
> +static int ast2600_i2c_controller_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
[ ... ]
> +	ret = ast2600_i2c_do_start(i2c_bus);
> +	if (ret)
> +		goto controller_out;
> +	timeout = wait_for_completion_timeout(&i2c_bus->cmd_complete, i2c_bus->adap.timeout);
> +	if (timeout == 0) {
> +		u32 ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
[ ... ]
> +		writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER,
> +		       i2c_bus->reg_base + AST2600_I2CM_IER);

[Severity: High]
If a late completion interrupt fires concurrently on another CPU right after
the IER is re-enabled here, but before msgs is set to NULL at the end of this
function, does this create a use-after-free where the interrupt handler
accesses the caller's freed message buffer?

[Severity: Medium]
Should AST2600_I2CM_BUS_RECOVER_FAIL also be enabled in the IER here?
The interrupt handler checks for this flag to return -EPROTO immediately.
Without it, will bus recovery failures silently block until the full software
timeout expires?

[ ... ]
> +static int ast2600_i2c_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	regmap_read(i2c_bus->global_regs, AST2600_I2CG_CTRL, &global_ctrl);
> +	if ((global_ctrl & AST2600_GLOBAL_INIT) != AST2600_GLOBAL_INIT) {

[Severity: Medium]
If regmap_read() returns an error for the syscon I/O, does this evaluate an
uninitialized global_ctrl variable?

[ ... ]
> +	writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER,
> +	       i2c_bus->reg_base + AST2600_I2CM_IER);

[Severity: Medium]
Similar to the timeout path, does the omission of AST2600_I2CM_BUS_RECOVER_FAIL
here cause initial bus recoveries to also hit the full timeout instead of
fast-failing?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-upstream_i2c-v31-0-ba7a02714f22@aspeedtech.com?part=4