Re: [PATCH v33 5/5] i2c: ast2600: Add target mode support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Coalesced STOP and SLAVE_MATCH interrupts incorrectly pass the previous transaction's buffered RX bytes to the new transaction.
- [High] Unconditional enablement of target interrupts and unmasked checks of I2CS_ISR in `ast2600_i2c_controller_xfer()` causes unhandled IRQ storms and permanent -EBUSY deadlocks when no slave is registered.
--

commit 12f36e43a7bec16d6ebecb3d928d5bb09f47373e
Author: Ryan Chen <ryan_chen@aspeedtech.com>
Subject: i2c: ast2600: Add target mode support

This commit adds target mode support to the AST2600 I2C driver.
Features include target interrupt handling, address match, and response logic.
This complements the existing controller-mode support to enable dual-role
capability.

> diff --git a/drivers/i2c/busses/i2c-ast2600.c b/drivers/i2c/busses/i2c-ast2600.c
> --- a/drivers/i2c/busses/i2c-ast2600.c
> +++ b/drivers/i2c/busses/i2c-ast2600.c

[ ... ]

> @@ -348,6 +353,253 @@ static int ast2600_i2c_recover_bus(struct ast2600_i2c_bus *i2c_bus)
> +	case AST2600_I2CS_SLAVE_PENDING | AST2600_I2CS_STOP |
> +	     AST2600_I2CS_TX_NAK | AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE:
> +	case AST2600_I2CS_SLAVE_PENDING | AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_STOP |
> +	     AST2600_I2CS_TX_NAK | AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE:
> +		i2c_slave_event(i2c_bus->target, I2C_SLAVE_STOP, &value);
> +		i2c_slave_event(i2c_bus->target, I2C_SLAVE_WRITE_REQUESTED, &value);
> +		target_rx_len = AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
> +						       AST2600_I2CC_BUFF_CTRL));
> +		for (i = 0; i < target_rx_len; i++) {
> +			value = readb(i2c_bus->buf_base + i2c_bus->buf_size + i);
> +			i2c_slave_event(i2c_bus->target, I2C_SLAVE_WRITE_RECEIVED, &value);
> +		}

[Severity: High]
Does this incorrectly pass the previous transaction's buffered RX bytes to the
new transaction?

The handler in ast2600_i2c_target_packet_buff_irq() emits I2C_SLAVE_STOP and
I2C_SLAVE_WRITE_REQUESTED for the new transaction before delivering the
remaining I2C_SLAVE_WRITE_RECEIVED bytes from the previous transaction.

[ ... ]

> @@ -684,6 +956,21 @@ static int ast2600_i2c_controller_xfer(struct i2c_adapter *adap, struct i2c_msg
>  			return ret;
>  	}
>  
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +	if (READ_ONCE(i2c_bus->target_active))
> +		return -EBUSY;
> +	/*
> +	 * Controller and target share the same buffer register. A target
> +	 * transaction can update buffer state asynchronously via IRQ, so block
> +	 * controller transfers while target is active to avoid buffer corruption.
> +	 */
> +	writel(0, i2c_bus->reg_base + AST2600_I2CS_IER);
> +	if (readl(i2c_bus->reg_base + AST2600_I2CS_ISR) || READ_ONCE(i2c_bus->target_active)) {
> +		writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_IER);
> +		return -EBUSY;
> +	}
> +#endif
> +
>  	i2c_bus->cmd_err = 0;
>  	i2c_bus->msgs = msgs;
>  	i2c_bus->msgs_index = 0;
>  	i2c_bus->msgs_count = num;
>  	WRITE_ONCE(i2c_bus->stop_pending, false);
>  	reinit_completion(&i2c_bus->cmd_complete);
>  	ret = ast2600_i2c_do_start(i2c_bus);
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +	/* avoid race condition target is wait and controller wait 1st target operate */
> +	writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_IER);
> +#endif

[Severity: High]
Will this logic cause unhandled IRQ storms and permanent -EBUSY deadlocks
when no slave is registered?

When CONFIG_I2C_SLAVE is enabled but no slave is bound (i2c_bus->target is
NULL), ast2600_i2c_controller_xfer() unconditionally writes to I2CS_IER to
re-enable target interrupts. If a target hardware event occurs, it will
trigger an interrupt that ast2600_i2c_bus_irq() ignores, leading to an IRQ
storm.

Additionally, because the interrupt is never handled and I2CS_ISR is never
cleared, subsequent calls to ast2600_i2c_controller_xfer() will see a
non-zero I2CS_ISR and permanently abort with -EBUSY.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623-upstream_i2c-v33-0-6d5338fc56ed@aspeedtech.com?part=5