Re: [PATCH v4 2/2] i2c: Add Cortina Systems CS75xx I2C controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] i2c: cs75xx: CS75XX_I2C_ACK_AL handled before CS75XX_I2C_IDLE
  in cs75xx_i2c_process()
- [Medium] i2c: cs75xx: Kconfig depends on undefined symbol
  ARCH_CORTINA_ACCESS
- [Low] i2c: cs75xx: unused CS75XX_I2C_CTRL_DONE macro in
  cs75xx_i2c_isr()
--

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

i2c: Add Cortina Systems CS75xx I2C controller driver

This commit adds an I2C controller driver for the Cortina Systems CS75xx
("Bi-Wire") controller.

> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 275aaa7f35833..db1b218a66a23 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -550,6 +550,17 @@ config I2C_CPM
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-cpm.
>  
> +config I2C_CS75XX
> +	tristate "Cortina Systems CS75xx I2C controller"
> +	depends on ARCH_CORTINA_ACCESS || COMPILE_TEST

[Severity: Medium]
Does ARCH_CORTINA_ACCESS exist in Kconfig?  No Kconfig file in the tree
defines this symbol, so CONFIG_I2C_CS75XX cannot be enabled unless
CONFIG_COMPILE_TEST is set.

[ ... ]

> diff --git a/drivers/i2c/busses/i2c-cs75xx.c b/drivers/i2c/busses/i2c-cs75xx.c
> new file mode 100644
> index 0000000000000..a7aec20ea5cf1
> --- /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 (i2c->state == CS75XX_I2C_STOP) {
> +		cs75xx_i2c_finish(i2c, i2c->err);
> +		return;
> +	}
> +
> +	if (ack & CS75XX_I2C_ACK_AL) {
> +		cs75xx_i2c_abort(i2c, -EAGAIN);
> +		return;
> +	}
> +
> +	switch (i2c->state) {
> +	case CS75XX_I2C_ADDR:
> +		if (ack & CS75XX_I2C_ACK_NACK) {
> +			cs75xx_i2c_abort(i2c, -ENXIO);
> +			return;
> +		}
> +		cs75xx_i2c_next_byte(i2c);
> +		break;
> +
> +	case CS75XX_I2C_WRITE:
> +		if (ack & CS75XX_I2C_ACK_NACK) {
> +			/* The last byte of the last message carried the stop */
> +			if (i2c->pos == msg->len && i2c->last)
> +				cs75xx_i2c_finish(i2c, -EIO);
> +			else
> +				cs75xx_i2c_abort(i2c, -EIO);
> +			return;
> +		}
> +		if (i2c->pos == msg->len) {
> +			cs75xx_i2c_finish(i2c, 0);
> +			return;
> +		}
> +		cs75xx_i2c_next_byte(i2c);
> +		break;
> +
> +	case CS75XX_I2C_READ:
> +		msg->buf[i2c->pos++] = readl(i2c->base + CS75XX_I2C_RXR);
> +		if (i2c->pos == msg->len) {
> +			cs75xx_i2c_finish(i2c, 0);
> +			return;
> +		}
> +		cs75xx_i2c_next_byte(i2c);
> +		break;
> +
> +	case CS75XX_I2C_IDLE:
> +	case CS75XX_I2C_STOP:
> +		/* Late interrupt of a transfer that timed out */
> +		break;
> +	}
> +}

[Severity: Medium]
Can a late interrupt with CS75XX_I2C_ACK_AL set trigger cs75xx_i2c_abort()
while i2c->state is CS75XX_I2C_IDLE?

When cs75xx_i2c_xfer_msg() times out, it sets i2c->state to CS75XX_I2C_IDLE
and drops i2c->lock before cs75xx_i2c_reset() calls disable_irq().  If a
late interrupt arrives in that window with CS75XX_I2C_ACK_AL set,
cs75xx_i2c_process() checks CS75XX_I2C_ACK_AL before checking for
CS75XX_I2C_IDLE, writes CS75XX_I2C_CTRL_STOP to the hardware and leaves
i2c->state in CS75XX_I2C_STOP across cs75xx_i2c_reset().  In addition, case
CS75XX_I2C_STOP in the switch statement is unreachable because
cs75xx_i2c_process() already returns early for CS75XX_I2C_STOP.

> +
> +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);

[Severity: Low]
Should cs75xx_i2c_isr() write CS75XX_I2C_CTRL_DONE instead of writing back
the entire CS75XX_I2C_CTRL register?  CS75XX_I2C_CTRL_DONE is defined at the
top of the file but never used, and writing back all of ctrl risks
re-asserting any non-self-clearing control bits in CS75XX_I2C_CTRL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115-cs75xx-i2c-v4-0-4b16eea98a78@gmail.com?part=2