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

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-messa=
ge transfer as a subaddress write, silently dropping it or misinterpreting =
read operations.
- [High] The interrupt handler returns IRQ_NONE without clearing the hardwa=
re 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 a=
dapter, breaking concurrent ongoing transfers and client device removal.
- [High] The ISR fails to handle the enabled CLK_STR_TIMEOUT interrupt, lea=
ving 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 control=
ler driver ---
commit 2546eac52465a246476611598e7130d303e3cc7f
Author: Ben Huang <[email protected]>

i2c: nt72676: Add Novatek NT72676 I2C controller driver
   =20
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-nt=
72676.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 =3D dev_id;
> +	struct i2c_msg *msg =3D i2c->current_msg;
> +	unsigned int status =3D nt72676_i2c_readl(i2c, I2C_REG_INTR);
> +	unsigned int clr =3D 0;
> +	int do_complete =3D 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 =3D -ENXIO;
> +		clr |=3D 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 |=3D 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 =3D wait_for_completion_timeout(&i2c->msg_complete, i2c->adapter.ti=
meout);
> +	if (ret =3D=3D 0) {
> +		i2c->error_code =3D -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 =3D 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 anot=
her
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 =3D i2c_get_adapdata(adap);
> +	int ret =3D 0, i =3D 0;
> +	struct i2c_msg *msg =3D NULL;
> +
> +	nvt_i2c_ctrl_init(i2c);
> +
> +	if (num =3D=3D 2) {
> +		nvt_i2c_set_subaddr(i2c, &msgs[0]);
> +		msg =3D &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 ar=
ray
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 =3D nvt_i2c_read(i2c, msg);
> +		else
> +			ret =3D nvt_i2c_write(i2c, msg);
> +	} else {

[ ... ]

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

[ ... ]

> +	ret =3D nvt_i2c_init(i2c);
> +	if (ret)
> +		return ret;
> +
> +	/* Setup I2C adapter */
> +	i2c->adapter.owner =3D THIS_MODULE;
> +	i2c->adapter.algo =3D &nvt_i2c_algo;
> +	i2c->adapter.dev.of_node =3D pdev->dev.of_node;
> +	i2c->adapter.dev.parent =3D &pdev->dev;
> +	i2c->adapter.timeout =3D 3 * HZ;
> +	strscpy(i2c->adapter.name, dev_name(&pdev->dev), sizeof(i2c->adapter.na=
me));
> +	i2c_set_adapdata(&i2c->adapter, i2c);
> +
> +	ret =3D 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 =3D 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().

> +}

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260804033419.6938=
[email protected]?part=3D2