Re: [PATCH 06/11] i2c: Add Cortina Systems CS75xx I2C controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] i2c: ignored NACK on final byte of write transfer
- [High] i2c: hardware busy timeout ignored
- [High] i2c: cs75xx: race between STOP completion IRQ and next transfer
- [High] i2c: cs75xx: ignore NACK on the last byte of a write message
- [Medium] i2c: interrupts permanently disabled after timeout reset
- [Medium] i2c: cs75xx: ignore NACK on the last byte of a write message
--

commit 4bef5e4f9afb2c145bb402508760c2866f3f5a51
Author: Fil Dunsky <filipp.dunsky@gmail.com>

i2c: Add Cortina Systems CS75xx I2C controller driver

The I2C ("Bi-Wire") controller of the Cortina Systems CS75xx SoCs transfers
one byte per command, and raises an interrupt when a command has completed.
Messages are sent with a repeated start between them and a stop after the
last one. Arbitration loss is reported as -EAGAIN and retried by the core.

> diff --git a/drivers/i2c/busses/i2c-cs75xx.c b/drivers/i2c/busses/i2c-cs75xx.c
> index 0000000000000..2d40f099d6715
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-cs75xx.c
[ ... ]
> +static void cs75xx_i2c_process(struct cs75xx_i2c *i2c, u32 ack)
> +{
> +	struct i2c_msg *msg = i2c->msg;
> +
> +	if (ack & CS75XX_I2C_ACK_AL) {
> +		writel(CS75XX_I2C_CTRL_STOP, i2c->base + CS75XX_I2C_CTRL);
> +		cs75xx_i2c_finish(i2c, -EAGAIN);
> +		return;
> +	}
> +
> +	switch (i2c->state) {
> +	case CS75XX_I2C_ADDR:
[ ... ]
> +	case CS75XX_I2C_WRITE:
> +		if (i2c->pos == msg->len) {
> +			cs75xx_i2c_finish(i2c, 0);
> +			return;
> +		}
> +		if (ack & CS75XX_I2C_ACK_NACK) {

[Severity: High]
Does this early return ignore the NACK check for the last byte?

Since i2c->pos is incremented immediately after writing a byte to the TX
register, it equals msg->len during the interrupt that signals completion of
the final byte. If the target device NACKs the last byte, will the driver
falsely report a successful transfer instead of an error?

> +			writel(CS75XX_I2C_CTRL_STOP, i2c->base + CS75XX_I2C_CTRL);
> +			cs75xx_i2c_finish(i2c, -EIO);
> +			return;
> +		}
> +		cs75xx_i2c_next_byte(i2c);
> +		break;
[ ... ]
> +static irqreturn_t cs75xx_i2c_isr(int irq, void *dev_id)
> +{
> +	struct cs75xx_i2c *i2c = dev_id;
> +	u32 ctrl, ack;
> +
> +	if (!(readl(i2c->base + CS75XX_I2C_INT) & CS75XX_I2C_INT_DONE))
> +		return IRQ_NONE;
> +
> +	writel(CS75XX_I2C_INT_DONE, i2c->base + CS75XX_I2C_INT);
> +
> +	/* The done flag is cleared by writing it back */
> +	ctrl = readl(i2c->base + CS75XX_I2C_CTRL);
> +	writel(ctrl, i2c->base + CS75XX_I2C_CTRL);
> +
> +	ack = readl(i2c->base + CS75XX_I2C_ACK);
> +
> +	spin_lock(&i2c->lock);

[Severity: High]
Can the STOP completion interrupt from an error in the previous transfer
race with the next transfer?

If a transfer fails (e.g. arbitration loss) and the ISR commands a STOP, the
waiting thread wakes up and the I2C core immediately retries. The thread
polls for !BUSY in cs75xx_i2c_xfer(), which becomes true exactly when the
STOP completes. The STOP completion simultaneously raises INT_DONE.

The thread proceeds, sets the state to ADDR, starts the new transfer, and
unlocks. Concurrently, the STOP completion ISR executes on another CPU,
reads the stale ACK locklessly, and spins on the lock. When the thread
unlocks, the ISR processes the stale ACK by incorrectly calling
cs75xx_i2c_next_byte(), which overrides the ADDR phase with a WRITE.

Does reading the ACK register locklessly before spin_lock() allow this stale
state to corrupt the newly started transfer?

> +	cs75xx_i2c_process(i2c, ack);
> +	spin_unlock(&i2c->lock);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int cs75xx_i2c_xfer_msg(struct cs75xx_i2c *i2c, struct i2c_msg *msg,
> +			       bool last)
> +{
[ ... ]
> +	time_left = wait_for_completion_timeout(&i2c->done, i2c->adap.timeout);
> +	if (!time_left) {
> +		spin_lock_irq(&i2c->lock);
> +		i2c->state = CS75XX_I2C_IDLE;
> +		spin_unlock_irq(&i2c->lock);
> +		dev_err(i2c->dev, "transfer timed out, resetting the controller\n");
> +		cs75xx_i2c_hw_init(i2c);

[Severity: Medium]
Will this hardware reset leave interrupts permanently disabled?

In cs75xx_i2c_hw_init(), the enable bit is cleared which performs a hardware
reset:

	if (readl(i2c->base + CS75XX_I2C_CFG) & CS75XX_I2C_CFG_EN) {
		writel(0, i2c->base + CS75XX_I2C_CFG);
		usleep_range(5000, 10000);
	}

Since cs75xx_i2c_xfer_msg() calls cs75xx_i2c_hw_init() on a timeout but does
not explicitly restore the interrupt enable (IE) register, will all
subsequent transfers time out as well?

> +		return -ETIMEDOUT;
> +	}
> +
> +	return i2c->err;
> +}
> +
> +static int cs75xx_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> +			   int num)
> +{
> +	struct cs75xx_i2c *i2c = i2c_get_adapdata(adap);
> +	u32 val;
> +	int i, ret;
> +
> +	/*
> +	 * Let the stop condition of the previous transfer complete and drop
> +	 * its interrupt, so that it is not taken for the next address phase.
> +	 */
> +	if (readl_poll_timeout(i2c->base + CS75XX_I2C_ACK, val,
> +			       !(val & CS75XX_I2C_ACK_BUSY), 10, 1000))
> +		dev_dbg(i2c->dev, "bus still busy\n");

[Severity: High]
If the controller remains busy after the timeout, should the error code be
returned instead of proceeding?

By ignoring the return value of readl_poll_timeout(), the execution
continues and writes a START command to the controller while it is still
actively asserting BUSY. Can this violate the I2C protocol and cause
undefined hardware behavior?

> +	writel(CS75XX_I2C_INT_DONE, i2c->base + CS75XX_I2C_INT);
> +
> +	for (i = 0; i < num; i++) {
> +		ret = cs75xx_i2c_xfer_msg(i2c, &msgs[i], i == num - 1);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-b4-cs75xx-s2-v1-0-8a7641b7b79c@gmail.com?part=6