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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:

New issues:
- [Critical] Aborting an in-flight controller transfer from the target IRQ handler races with controller IRQs, leading to a Use-After-Free.
- [Critical] A Time-Of-Check to Time-Of-Use (TOCTOU) race in `ast2600_i2c_controller_xfer` allows target hardware to corrupt the shared I2C buffer.
- [High] Invalid I2C Slave event ordering: STOP events are emitted before WRITE_REQUESTED and WRITE_RECEIVED data events.
- [High] The `target_active` flag is permanently leaked as true under specific hardware interrupt combinations, deadlocking controller transfers.
- [High] Target interrupts are permanently disabled if a target is unregistered and later re-registered.
- [High] A specific target ISR STOP case stalls the I2C target state machine by failing to trigger the hardware command.
- [High] Unhandled target interrupt states fail to re-arm the hardware, permanently hanging the I2C bus.
- [High] Checking the raw ISR register to block controller transfers permanently bricks the controller due to uncleared W1C bits.
- [Medium] Target interrupts are unconditionally enabled even when no target is registered.

Pre-existing issues:
- [High] In controller mode, the received `xfer_len` is not validated against the remaining bounds of the `msg->buf` array.
--

commit fdd43bfe0f52b1390eb013eb7a0a2e7b1131f43e
Author: Ryan Chen <ryan_chen@aspeedtech.com>

i2c: ast2600: Add target mode support

Add target mode support to the AST2600 I2C driver. This complements the 
existing controller-mode support, enabling dual-role capability.

> diff --git a/drivers/i2c/busses/i2c-ast2600.c b/drivers/i2c/busses/i2c-ast2600.c
> index a06d0483865ed..7e609fdfa1a19 100644
> --- a/drivers/i2c/busses/i2c-ast2600.c
> +++ b/drivers/i2c/busses/i2c-ast2600.c
[ ... ]
> @@ -337,6 +342,249 @@ static int ast2600_i2c_recover_bus(struct ast2600_i2c_bus *i2c_bus)
>  	return ret;
>  }
>  
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +static void ast2600_i2c_target_packet_buff_irq(struct ast2600_i2c_bus *i2c_bus, u32 sts)
> +{
> +	u8 value = 0;
> +	int target_rx_len = 0;
> +	u32 cmd = 0;
> +	int i;
> +
> +	/* due to controller target is common buffer, need force the master stop not issue */
> +	if (readl(i2c_bus->reg_base + AST2600_I2CM_CMD_STS) & GENMASK(15, 0)) {
> +		writel(0, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
> +		writel(0, i2c_bus->reg_base + AST2600_I2CM_IER);
> +		writel(readl(i2c_bus->reg_base + AST2600_I2CM_ISR),
> +		       i2c_bus->reg_base + AST2600_I2CM_ISR);
> +		i2c_bus->cmd_err = -EBUSY;
> +		writel(0, i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
> +		complete(&i2c_bus->cmd_complete);
> +		writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER,
> +		       i2c_bus->reg_base + AST2600_I2CM_IER);
> +	}

[Severity: Critical]
Does calling complete() here race with re-enabling the controller interrupts?

If the controller thread wakes up immediately and frees the locally allocated
msgs array, could a subsequent delayed hardware interrupt cause
ast2600_i2c_controller_packet_irq() to dereference the freed msgs pointer?

> +
> +	/* Handle i2c target timeout condition */
> +	if (sts & AST2600_I2CS_INACTIVE_TO) {
[ ... ]
> +	switch (sts) {
> +	case AST2600_I2CS_SLAVE_PENDING | AST2600_I2CS_WAIT_RX_DMA |
> +		 AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
> +	case AST2600_I2CS_SLAVE_PENDING |
> +		 AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
> +	case AST2600_I2CS_SLAVE_PENDING |
> +		 AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_STOP:
> +		i2c_slave_event(i2c_bus->target, I2C_SLAVE_STOP, &value);
> +		fallthrough;
> +	case AST2600_I2CS_SLAVE_PENDING |
> +		 AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE:

[Severity: High]
Does falling through here emit events in the wrong order?

The I2C slave API expects I2C_SLAVE_STOP to be the final event indicating
the end of a transaction. If we fall through to the block that issues
I2C_SLAVE_WRITE_REQUESTED and I2C_SLAVE_WRITE_RECEIVED, could this corrupt
the state machine of backend devices?

[ ... ]
> +	case AST2600_I2CS_SLAVE_PENDING | AST2600_I2CS_RX_DONE |
> +	     AST2600_I2CS_WAIT_TX_DMA | AST2600_I2CS_STOP:
> +		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);
> +		}
> +		i2c_slave_event(i2c_bus->target, I2C_SLAVE_STOP, &value);
> +		i2c_slave_event(i2c_bus->target, I2C_SLAVE_READ_REQUESTED, &value);
> +		writel(value, i2c_bus->buf_base);
> +		break;

[Severity: High]
Is this case missing an assignment to cmd?

Since cmd defaults to 0 and is not updated here, the driver will not write
a command to AST2600_I2CS_CMD_STS. Could this leave the hardware state machine
hanging or stretching the clock indefinitely?

[ ... ]
> +	default:
> +		dev_dbg(i2c_bus->dev, "unhandled target isr case %x, sts %x\n", sts,
> +			readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));
> +		break;
> +	}
> +
> +	if (cmd)
> +		writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);

[Severity: High]
Does an unhandled interrupt state hang the I2C bus?

If we hit the default case, cmd remains 0. Without writing a valid command
to acknowledge and proceed, will the hardware remain paused and stretch
the clock?

> +
> +	writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_ISR);
> +	readl(i2c_bus->reg_base + AST2600_I2CS_ISR);
> +
> +	if ((sts & AST2600_I2CS_STOP) && !(sts & AST2600_I2CS_SLAVE_PENDING))
> +		WRITE_ONCE(i2c_bus->target_active, false);
> +}

[Severity: High]
Can the target_active flag become permanently stuck as true?

The switch statement above explicitly handles combined interrupt cases where
both AST2600_I2CS_STOP and AST2600_I2CS_SLAVE_PENDING are set. In those
cases, target_active will remain true.

Since a STOP occurred, no further interrupts will arrive for this transaction.
Will this cause all future controller transfers to permanently return -EBUSY?

> +
> +static int ast2600_i2c_target_irq(struct ast2600_i2c_bus *i2c_bus)
> +{
> +	u32 ier = readl(i2c_bus->reg_base + AST2600_I2CS_IER);
> +	u32 isr = readl(i2c_bus->reg_base + AST2600_I2CS_ISR);
[ ... ]
> +	isr &= ~(AST2600_I2CS_ADDR_INDICATE_MASK);
> +
> +	if (AST2600_I2CS_ADDR1_NAK & isr)
> +		isr &= ~AST2600_I2CS_ADDR1_NAK;
> +
> +	if (AST2600_I2CS_ADDR2_NAK & isr)
> +		isr &= ~AST2600_I2CS_ADDR2_NAK;

[Severity: High]
Are these Write-1-to-Clear (W1C) bits being left set in the hardware?

The bits are masked out of the local isr variable but are never written back
to AST2600_I2CS_ISR to clear them in hardware. Because they remain set,
will later checks against AST2600_I2CS_ISR always evaluate to true, blocking
all future controller transfers?

[ ... ]
>  	case AST2600_I2CM_RX_DONE:
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +		/*
> +		 * Workaround for controller/target packet mode enable rx done stuck issue
> +		 * When controller go for first read (RX_DONE), target mode will also effect
> +		 * Then controller will send nack, not operate anymore.
> +		 */
> +		if (readl(i2c_bus->reg_base + AST2600_I2CS_CMD_STS) & AST2600_I2CS_PKT_MODE_EN) {
> +			u32 target_cmd = readl(i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
> +
> +			writel(0, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
> +			writel(target_cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
> +		}
> +		fallthrough;
> +#endif
>  	case AST2600_I2CM_RX_DONE | AST2600_I2CM_NORMAL_STOP:
>  		xfer_len = AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
>  							     AST2600_I2CC_BUFF_CTRL));

[Severity: High]
This is a pre-existing issue, but does xfer_len need bounds checking here?

The xfer_len value is read directly from hardware and used unconditionally in
the copy loop below. If the hardware reports a length longer than the
remaining message size, could it overflow the msg->buf array?

[ ... ]
> @@ -599,12 +867,31 @@ static int ast2600_i2c_controller_xfer(struct i2c_adapter *adap, struct i2c_msg
>  			return ret;
>  	}
>  
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +	if (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) || 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;
>  	reinit_completion(&i2c_bus->cmd_complete);
>  	ret = ast2600_i2c_do_start(i2c_bus);

[Severity: Critical]
Is there a race condition between software checks and the hardware state machine?

Disabling CPU interrupts via AST2600_I2CS_IER does not pause the autonomous
hardware state machine since AST2600_I2CC_SLAVE_EN is still active. Can an
external I2C master address the target right after the check, causing the
hardware to overwrite the shared buffer while ast2600_i2c_do_start() is
preparing the master transfer?

[ ... ]
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +static int ast2600_i2c_reg_target(struct i2c_client *client)
> +{
> +	struct ast2600_i2c_bus *i2c_bus = i2c_get_adapdata(client->adapter);
> +	u32 cmd = TARGET_TRIGGER_CMD;
[ ... ]
> +	writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
> +	/* Set target addr. */
> +	writel(client->addr | AST2600_I2CS_ADDR1_ENABLE,
> +	       i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
> +
> +	return 0;
> +}
> +
> +static int ast2600_i2c_unreg_target(struct i2c_client *client)
> +{
> +	struct ast2600_i2c_bus *i2c_bus = i2c_get_adapdata(client->adapter);
> +	u32 val;
> +
> +	writel(0, i2c_bus->reg_base + AST2600_I2CS_IER);

[Severity: High]
Will target interrupts remain permanently disabled if a target is unbound and
then rebound?

The ast2600_i2c_unreg_target() function disables target interrupts, but
ast2600_i2c_reg_target() does not re-enable them. Does this break target
functionality on subsequent registrations?

[ ... ]
> @@ -784,6 +1138,9 @@ static int ast2600_i2c_probe(struct platform_device *pdev)
>  
>  	writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER,
>  	       i2c_bus->reg_base + AST2600_I2CM_IER);
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +	writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_IER);
> +#endif
>  
>  	ret = i2c_add_adapter(&i2c_bus->adap);

[Severity: Medium]
Is it safe to unconditionally enable target interrupts here?

Since i2c_bus->target is NULL until a target is registered, ast2600_i2c_bus_irq
will ignore target interrupts. If the hardware asserts an interrupt before
a target is bound, could this lead to an unhandled interrupt storm?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528-upstream_i2c-v30-0-5d4f9adc3530@aspeedtech.com?part=5