Re: [PATCH v5 2/3] i2c: nt72676: Add Novatek NT72676 I2C controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] The driver unconditionally treats the first message of any 2-message transfer as a subaddress write, silently dropping it or misinterpreting read operations.
- [High] The interrupt handler returns IRQ_NONE without clearing the hardware interrupt if current_msg is NULL, causing an interrupt storm.
- [High] Lack of synchronization between the I2C transfer timeout path and the interrupt handler leads to a use-after-free of the message buffer.
- [High] The I2C controller hardware is disabled before unregistering the adapter, breaking concurrent ongoing transfers and client device removal.
- [High] The ISR fails to handle the enabled CLK_STR_TIMEOUT interrupt, leaving it asserted and causing an interrupt storm.
- [Medium] Hardware interrupts are left enabled if `i2c_add_adapter` fails during probe.
--

--- Patch [2]: [PATCH v5 2/3] i2c: nt72676: Add Novatek NT72676 I2C controller driver ---
commit 2546eac52465a246476611598e7130d303e3cc7f
Author: Ben Huang <Ben_Huang@novatek.com.tw>

i2c: nt72676: Add Novatek NT72676 I2C controller driver
    
This driver performs the fundamental read/write functions as an I2C
controller and supports Standard-mode and Fast-mode.
Default operation is Standard-mode.

> diff --git a/drivers/i2c/busses/i2c-nt72676.c b/drivers/i2c/busses/i2c-nt72676.c
> new file mode 100644
> index 0000000000000..48750b02537fb
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-nt72676.c

[ ... ]

> +static irqreturn_t nvt_i2c_isr(int irq, void *dev_id)
> +{
> +	struct nvt_i2c_bus *i2c = dev_id;
> +	struct i2c_msg *msg = i2c->current_msg;
> +	unsigned int status = nt72676_i2c_readl(i2c, I2C_REG_INTR);
> +	unsigned int clr = 0;
> +	int do_complete = 0;
> +
> +	if (!(status & I2C_IRQ_FLAG) || !i2c->current_msg)
> +		return IRQ_NONE;

[Severity: High]
Could this early return cause an interrupt storm if a spurious interrupt
fires, or if current_msg is NULL after a timeout? Since it returns
IRQ_NONE without writing to the clear register, the level-triggered
hardware interrupt might remain asserted indefinitely.

> +
> +	if (status & I2C_IRQ_NACK) {
> +		i2c->error_code = -ENXIO;
> +		clr |= I2C_IRQ_NACK << 8;
> +	} else if (status & I2C_IRQ_RX_FULL) {

[Severity: High]
What happens if the hardware asserts I2C_IRQ_CLK_STR_TIMEOUT? The
I2C_IRQ_ENABLE_SETTING macro enables this interrupt (bit 12), but it is
omitted from this if/else chain. Does this mean it won't be added to
the clr variable, leaving it asserted and causing an interrupt storm?

> +		if (i2c->remaining > 0)
> +			nvt_i2c_handle(i2c, msg, true);
> +		clr |= I2C_IRQ_RX_FULL << 8;
> +	} else if (status & I2C_IRQ_TX_EMPTY) {

[ ... ]

> +static int nvt_i2c_write(struct nvt_i2c_bus *i2c, struct i2c_msg *msg)
> +{

[ ... ]

> +	ret = wait_for_completion_timeout(&i2c->msg_complete, i2c->adapter.timeout);
> +	if (ret == 0) {
> +		i2c->error_code = -ETIMEDOUT;
> +		nvt_i2c_reset(i2c);
> +	}
> +	if (i2c->error_code)
> +		dev_err(i2c->dev, "[%s]: write failed (err:%d); SA[0x%X]\n",
> +			i2c->adapter.name, i2c->error_code, msg->addr);
> +
> +	i2c->current_msg = NULL;

[Severity: High]
Can the timeout path race with the interrupt handler here? If the transfer
times out, current_msg is set to NULL and the function returns, leading the
caller to free the message buffer. If the ISR executes concurrently on another
CPU, could it access the freed buffer via current_msg before it gets fully
nullified, causing a use-after-free?

> +
> +	return i2c->error_code;
> +}

[ ... ]

> +static int nvt_i2c_xfer(struct i2c_adapter *adap,
> +			struct i2c_msg msgs[],
> +			int num)
> +{
> +	struct nvt_i2c_bus *i2c = i2c_get_adapdata(adap);
> +	int ret = 0, i = 0;
> +	struct i2c_msg *msg = NULL;
> +
> +	nvt_i2c_ctrl_init(i2c);
> +
> +	if (num == 2) {
> +		nvt_i2c_set_subaddr(i2c, &msgs[0]);
> +		msg = &msgs[1];

[Severity: High]
Is it correct to unconditionally treat the first message of any 2-message
transfer as a subaddress write? If a user-space caller sends a 2-message array
where the first message is a read, has a length > 4, or targets a different
device address, won't this silently drop or misinterpret the first message and
only execute the second one?

> +
> +		if (msg->flags & I2C_M_RD)
> +			ret = nvt_i2c_read(i2c, msg);
> +		else
> +			ret = nvt_i2c_write(i2c, msg);
> +	} else {

[ ... ]

> +static int nvt_i2c_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	ret = nvt_i2c_init(i2c);
> +	if (ret)
> +		return ret;
> +
> +	/* Setup I2C adapter */
> +	i2c->adapter.owner = THIS_MODULE;
> +	i2c->adapter.algo = &nvt_i2c_algo;
> +	i2c->adapter.dev.of_node = pdev->dev.of_node;
> +	i2c->adapter.dev.parent = &pdev->dev;
> +	i2c->adapter.timeout = 3 * HZ;
> +	strscpy(i2c->adapter.name, dev_name(&pdev->dev), sizeof(i2c->adapter.name));
> +	i2c_set_adapdata(&i2c->adapter, i2c);
> +
> +	ret = i2c_add_adapter(&i2c->adapter);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "Failed to add adapter\n");

[Severity: Medium]
Does this error path need to disable the hardware interrupts that were
enabled earlier in nvt_i2c_init()? Returning directly might leave
interrupts enabled on a shared IRQ line.

> +
> +	platform_set_drvdata(pdev, i2c);
> +
> +	return 0;
> +}
> +
> +static void nvt_i2c_remove(struct platform_device *pdev)
> +{
> +	struct nvt_i2c_bus *i2c = platform_get_drvdata(pdev);
> +
> +	nt72676_i2c_writel(I2C_IRQ_DISABLE_SETTING, i2c, I2C_REG_INTR);
> +	nt72676_i2c_writel(nt72676_i2c_readl(i2c, I2C_REG_CTRL) & ~I2C_ENABLE,
> +			   i2c, I2C_REG_CTRL);
> +	i2c_del_adapter(&i2c->adapter);

[Severity: High]
Should the adapter be deleted before disabling the I2C controller hardware?
Disabling interrupts and clocks first could break concurrent ongoing
transfers or client device removal operations that happen during
i2c_del_adapter().

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804033419.693819-1-nina_kuo@novatek.com.tw?part=2