Re: [PATCH 3/3] i2c: Add i2c-nt726xx.c i2c driver for Novatek NT726xx SoCs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Hardcoded physical addresses mapped via ioremap() without NULL checks, and unprotected read-modify-write on shared global registers.
- [High] Driver breaks generic I2C transfers by unconditionally hijacking any 2-message array.
- [High] The ISR returns IRQ_NONE without clearing the hardware interrupt status when current_msg is NULL.
- [High] Missing synchronization between the transfer timeout path and the ISR leads to a use-after-free.
- [High] Interrupts are enabled in hardware before the IRQ handler is registered, and are leaked enabled on probe failure paths.
- [High] The I2C hardware and interrupts are disabled before the adapter is unregistered in the remove function.
- [Medium] Using a custom Device Tree property to statically assign the I2C bus number.
- [Medium] Device Tree node reference is permanently leaked on the adapter registration error path.
- [Low] Unconditional assignment of PM callbacks causes dead code when PM is disabled.
--

commit 2b999343fc99d6d31a3c1ef33d691ee2f0f37f51
Author: Ben Huang <Ben_Huang@novatek.com.tw>

i2c: Add i2c-nt726xx.c i2c driver for Novatek NT726xx SoCs

This patch introduces support for the I2C bus controller of Novatek
NT726xx SoCs. It performs fundamental read/write functions and supports
Standard-mode and Fast-mode.

> diff --git a/drivers/i2c/busses/i2c-nt726xx.c b/drivers/i2c/busses/i2c-nt726xx.c
> new file mode 100644
> index 0000000000000..921f47b7dd551
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-nt726xx.c
[ ... ]
> +static void nvt_i2c_use_case_feature(struct nvt_i2c_bus *i2c)
> +{
> +	void __iomem *reg_tmp;
> +
> +	if (i2c->stbc_i2c) {
> +		reg_tmp = ioremap(STBC_PASSWORD, 4);
> +		writel(readl(reg_tmp) | STBC_PASSWORD_DATA1, reg_tmp);
> +		writel(readl(reg_tmp) | STBC_PASSWORD_DATA2, reg_tmp);

[Severity: High]
Does this code map a hardcoded physical address without checking the return
value of ioremap()? If the mapping fails, readl() will dereference a NULL
pointer. Additionally, is it safe to perform an unprotected read-modify-write
on these shared global registers, or could it corrupt state if other drivers
concurrently access the Standby Controller?

> +		iounmap(reg_tmp);
[ ... ]
> +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 = readl(i2c->base + I2C_REG_INTR);
> +	unsigned int clr = 0;
> +	int do_complete = 0;
> +
> +	if (!(status & I2C_IRQ_FLAG) || !i2c->current_msg)
> +		return IRQ_NONE;

[Severity: High]
If the hardware asserts an interrupt and i2c->current_msg is NULL, does
this early return skip clearing the hardware interrupt status? The write
to clear the interrupt at the end of the ISR is bypassed, which could leave
the IRQ line asserted and cause an infinite interrupt loop.

> +
> +	if (status & I2C_IRQ_NACK) {
[ ... ]
> +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)
> +		I2C_ERR_LOG("[%s.%d]: write failed (err:%d);"
> +			    " SA[0x%X]\n",
> +			    dev_name(i2c->dev), i2c->adapter.nr, i2c->error_code,
> +			    msg->addr);
> +
> +	i2c->current_msg = NULL;

[Severity: High]
Can a use-after-free happen here if the timeout path races with the ISR?
If a transfer times out, nvt_i2c_reset() is called and current_msg is set
to NULL, but there is no synchronization (like a spinlock or
synchronize_irq()) to wait for a concurrently running ISR to finish. If the
ISR checks current_msg before it is set to NULL, the thread could return and
free msg->buf while the ISR continues to execute nvt_i2c_handle().

> +
> +	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]
Does this code unconditionally intercept all 2-message arrays? By consuming
msgs[0] as a subaddress and only transmitting msgs[1], this breaks standard
I2C semantics if a user-space application or generic kernel driver issues a
regular 2-message transfer. If the first message is a read, could it lead
to reading from an uninitialized buffer and writing it to the register?

> +
> +		if (msg->flags & I2C_M_RD)
[ ... ]
> +static int nvt_i2c_parse_dts(struct nvt_i2c_bus *i2c)
> +{
> +	int ret;
> +	struct device *dev = i2c->dev;
> +	struct device_node *np = dev->of_node;
> +	const char *hwmods_val = NULL;
> +
> +	/* read DTS(novatek,hwmods) and set bus number */
> +	ret = of_property_read_string(np, "novatek,hwmods", &hwmods_val);
> +	if (ret == 0) {
> +		i2c->adapter.nr = nvt_i2c_get_hwmods(hwmods_val);

[Severity: Medium]
Is it appropriate to use a custom Device Tree property like novatek,hwmods
to statically assign the I2C adapter number? The standard Linux mechanism
for assigning bus numbers relies on the aliases node in the Device Tree
rather than custom properties.

> +		if (i2c->adapter.nr >= 0) {
[ ... ]
> +static int nvt_i2c_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ret = nvt_i2c_parse_dts(i2c);
> +	if (ret)
> +		return ret;
> +
> +	nvt_i2c_init(i2c);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		I2C_ERR_LOG("No IRQ resource\n");
> +		return irq;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, irq, nvt_i2c_isr,
> +			       IRQF_SHARED | IRQF_TRIGGER_HIGH, I2C_NAME, i2c);

[Severity: High]
Does nvt_i2c_init() enable hardware interrupts before devm_request_irq()
registers the software handler? If the hardware has a pending interrupt,
the kernel might receive it prematurely. Furthermore, if probe fails on
subsequent calls like devm_request_irq(), are these interrupts left enabled
in the hardware, potentially causing an unhandled interrupt storm on a
shared line?

> +	if (ret) {
> +		I2C_ERR_LOG("devm_request_irq fail\n");
> +		return ret;
> +	}
> +
> +	/* Setup I2C adapter */
> +	i2c->adapter.owner = THIS_MODULE;
> +	i2c->adapter.class = I2C_CLASS_HWMON;
> +	i2c->adapter.algo = &nvt_i2c_algo;
> +	i2c->adapter.dev.of_node = of_node_get(pdev->dev.of_node);
> +	i2c->adapter.dev.parent = &pdev->dev;
> +	i2c->adapter.timeout = 3 * HZ;
> +	strscpy(i2c->adapter.name, I2C_NAME, sizeof(i2c->adapter.name));
> +	i2c_set_adapdata(&i2c->adapter, i2c);
> +
> +	ret = i2c_add_numbered_adapter(&i2c->adapter);
> +	if (ret) {
> +		I2C_ERR_LOG("[%s] failed to add adapter\n", dev_name(&pdev->dev));
> +		return ret;

[Severity: Medium]
If i2c_add_numbered_adapter() fails, does this return path leak the OF node
reference? The node was acquired just above with of_node_get(), and
returning directly here skips calling of_node_put().

> +	}
> +
> +	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);
> +
> +	writel(I2C_IRQ_DISABLE_SETTING, i2c->base + I2C_REG_INTR);
> +	writel(readl(i2c->base + I2C_REG_CTRL) & ~I2C_ENABLE,
> +	       i2c->base + I2C_REG_CTRL);
> +	of_node_put(i2c->adapter.dev.of_node);
> +	i2c_del_adapter(&i2c->adapter);

[Severity: High]
Is the teardown order reversed here? The code disables the I2C hardware and
interrupts before calling i2c_del_adapter(). If an active transaction is
running, this will forcefully break it. Additionally, i2c_del_adapter()
ensures synchronization by draining and blocking transfers, so it should
be called first before shutting down the hardware.

> +}
> +
> +static const struct dev_pm_ops nvt_i2c_pm_ops = {
> +	.resume_early = nvt_i2c_resume,
> +	.suspend_late = nvt_i2c_suspend,
> +};

[Severity: Low]
Are the pm_sleep_ptr() wrappers missing here? Without wrapping the callbacks
in the dev_pm_ops structure, this will result in dead code being compiled
into the kernel when CONFIG_PM_SLEEP is disabled.

> +
> +static struct platform_driver nvt_i2c_driver = {
> +	.probe = nvt_i2c_probe,
> +	.remove = nvt_i2c_remove,
> +	.driver = {
> +		.name = "nvt_i2c",
> +		.owner = THIS_MODULE,
> +		.pm = &nvt_i2c_pm_ops,

[Severity: Low]
Does this assignment need a pm_ptr() wrapper to avoid dead code when PM is
disabled?

> +		.of_match_table = of_match_ptr(nvt_i2c_of_match),
> +	},
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605035614.383387-1-SP_ISW1_AT@novatek.com.tw?part=3