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

From: Ben Huang <[email protected]>

On Tue, 04 Aug 2026 03:45:15 +0000, [email protected] wrote:
>> +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.

The potential issue of IRQ storm under the condition !i2c->current_msg will
be fixed by clearing all IRQs.

>> +
>> +	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?

Apologies for loss of handling IRQ I2C_IRQ_CLK_STR_TIMEOUT.
The flow of I2C_IRQ_CLK_STR_TIMEOUT will be added.

>> +	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?

Spin lock for current_msg and the interrupt register will be added.

>> +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?

For the 2-message transfer, the flow of checking msg[0] is a sub-address write and
with the maximum length of 4 bytes (32 bits) will be added.

>> +	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.

nvt_i2c_init() will be moved after successful i2c_add_adapter().
If nvt_i2c_init() fails, no interrupts are enabled and i2c_del_adapter() will be
called for releasing the adapter resources.

>> +
>> +	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().

i2c_del_adapter() will be moved before disabling the I2C controller.


The issues above will be fixed at the next iteration.

Sincerely,
Ben Huang