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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] i2c: lockless hardware reset races with ISR after transfer timeout
- [Medium] i2c: integer overflow in CS75xx clock prescaler boundary check
- [Medium] i2c: cs75xx: premature controller reset on multi-master bus due to 1ms timeout
--

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

i2c: Add Cortina Systems CS75xx I2C controller driver

This commit adds a new I2C controller driver for the Cortina Systems CS75xx
SoCs. It supports single-byte transfers with a command register, handles
interrupts upon command completion, and supports arbitration loss retries.

> diff --git a/drivers/i2c/busses/i2c-cs75xx.c b/drivers/i2c/busses/i2c-cs75xx.c
> new file mode 100644
> index 0000000000000..3eca5fb14266e
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-cs75xx.c
[ ... ]
> +static irqreturn_t cs75xx_i2c_isr(int irq, void *dev_id)
> +{
> +	struct cs75xx_i2c *i2c = dev_id;
> +	u32 ctrl, ack;
> +
> +	guard(spinlock)(&i2c->lock);
> +
> +	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);
> +	cs75xx_i2c_process(i2c, ack);
> +
> +	return IRQ_HANDLED;
> +}
[ ... ]
> +static int cs75xx_i2c_xfer_msg(struct cs75xx_i2c *i2c, struct i2c_msg *msg,
> +			       bool last)
> +{
> +	unsigned long time_left;
> +
> +	spin_lock_irq(&i2c->lock);
> +	reinit_completion(&i2c->done);
> +	i2c->msg = msg;
> +	i2c->pos = 0;
> +	i2c->last = last;
> +	i2c->err = 0;
> +	i2c->state = CS75XX_I2C_ADDR;
> +	writel(i2c_8bit_addr_from_msg(msg), i2c->base + CS75XX_I2C_TXR);
> +	writel(CS75XX_I2C_CTRL_START | CS75XX_I2C_CTRL_WRITE,
> +	       i2c->base + CS75XX_I2C_CTRL);
> +	spin_unlock_irq(&i2c->lock);
> +
> +	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_reset(i2c);

[Severity: High]
Is there a race condition between this hardware reset and the interrupt
handler?

When a transfer times out, the lock is dropped before resetting the controller:

CPU1 (Timeout path)
cs75xx_i2c_xfer_msg()
   spin_unlock_irq(&i2c->lock);
   cs75xx_i2c_reset(i2c);
      cs75xx_i2c_hw_init(i2c);
         writel(0, i2c->base + CS75XX_I2C_CFG);  /* Disables the IP */
         usleep_range(5000, 10000);

If a delayed interrupt fires during this sleep window, the ISR will run:

CPU2 (ISR)
cs75xx_i2c_isr()
   guard(spinlock)(&i2c->lock);
   ...
   writel(CS75XX_I2C_INT_DONE, i2c->base + CS75XX_I2C_INT);
   ctrl = readl(i2c->base + CS75XX_I2C_CTRL);
   writel(ctrl, i2c->base + CS75XX_I2C_CTRL);

The ISR grabs the lock and performs multiple MMIO reads and writes before it
checks i2c->state. Can accessing these hardware registers while the block is
disabled or in reset trigger an asynchronous external bus abort or panic?

> +		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;
> +
> +	if (readl_poll_timeout(i2c->base + CS75XX_I2C_ACK, val,
> +			       !(val & CS75XX_I2C_ACK_BUSY), 10, 1000)) {
> +		cs75xx_i2c_reset(i2c);

[Severity: Medium]
Is a 1ms (1000 us) timeout sufficient to wait for the I2C bus to become idle
when an external master is executing a legitimate transaction?

If another master initiates a transaction that takes longer than 1ms (such as
a transfer larger than 11 bytes at 100 kHz), this poll will time out.

This would cause the driver to prematurely perform a hardware reset and fail
the transfer with -EBUSY, potentially disrupting valid multi-master bus
activity.

> +		if (readl(i2c->base + CS75XX_I2C_ACK) & CS75XX_I2C_ACK_BUSY)
> +			return -EBUSY;
> +	}
[ ... ]
> +static int cs75xx_i2c_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	i2c->ref_rate = clk_get_rate(clk);
> +	if (device_property_read_u32(dev, "clock-frequency", &i2c->bus_rate))
> +		i2c->bus_rate = I2C_MAX_STANDARD_MODE_FREQ;
> +	if (!i2c->ref_rate || !i2c->bus_rate ||
> +	    i2c->bus_rate > I2C_MAX_FAST_MODE_FREQ ||
> +	    i2c->ref_rate / (5 * i2c->bus_rate) > FIELD_MAX(CS75XX_I2C_CFG_PRER) + 1)

[Severity: Medium]
Could this integer division allow boundary values that overflow the 16-bit
prescaler field?

Since FIELD_MAX(CS75XX_I2C_CFG_PRER) is 65535, this check permits the integer
division to evaluate to 65536.

However, cs75xx_i2c_hw_init() calculates the actual prescaler using
DIV_ROUND_UP(i2c->ref_rate, 5 * i2c->bus_rate) - 1.

If the division yields 65536 with any non-zero remainder, DIV_ROUND_UP
evaluates to 65537, causing the calculated prescaler to equal 65536 (0x10000).

When this 17-bit value is passed to FIELD_PREP() in cs75xx_i2c_hw_init(), it
gets silently truncated to 0, which would misconfigure the hardware for the
highest possible bus frequency.

Should this boundary check use DIV_ROUND_UP to match the initialization logic?

> +		return dev_err_probe(dev, -EINVAL,
> +				     "unsupported bus frequency %u Hz\n",
> +				     i2c->bus_rate);
> +
> +	cs75xx_i2c_hw_init(i2c);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-b4-cs75xx-s2-v2-0-94f8c0a90cda@gmail.com?part=6