Re: [PATCH v3 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 11 potential issue(s) to consider:
- [High] The driver bypasses Device Tree abstractions by mapping hardcoded physical MMIO addresses, and triggers a sleep-in-atomic panic during system resume.
- [High] The master transfer function improperly intercepts 2-message sequences, silently dropping the first message and potentially leaking uninitialized kernel memory to hardware.
- [High] The interrupt handler returns IRQ_NONE without clearing asserted IRQ flags on unhandled events, leading to a hard lockup.
- [High] A race condition between the transfer timeout path and the ISR leads to a use-after-free of the I2C message buffer.
- [High] Hardware interrupts are enabled before the driver state is initialized or the adapter is registered.
- [High] Hardware and interrupts are disabled before unregistering the I2C adapter in `remove`, forcefully stalling ongoing transfers.
- [High] `I2C_REG_CTRL` accumulates target addresses and operation flags across multiple messages without clearing previous states.
- [Medium] Device tree node reference leak in the probe error path.
- [Medium] The driver uses a custom string property 'novatek,hwmods' to assign the I2C adapter number instead of standard DT aliases.
- [Low] PM callbacks and `dev_pm_ops` are unconditionally assigned, producing dead code when PM is disabled.
- [Low] Missing Kconfig OF dependency leads to unused variable warnings.
--

--- Patch [3]: [PATCH v3 3/3] i2c: Add i2c-nt726xx.c I2C driver for Novatek NT726xx SoCs ---
commit fffdbc67de89a90f8a40ad053da0475e89a835d6
Author: Ben Huang <Ben_Huang@novatek.com.tw>
Subject: i2c: Add i2c-nt726xx.c I2C driver for Novatek NT726xx SoCs

This patch introduces the I2C bus controller driver for Novatek NT726xx SoCs,
supporting standard and fast modes.

> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index d354569942803..52ee2c82e9d5e 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -962,6 +962,16 @@ config I2C_NPCM
>  	  controllers.
>  	  Driver can also support slave mode (select I2C_SLAVE).
>  
> +config I2C_NT726XX
> +	tristate "Novatek NT726xx Driver"
> +	default n

[Severity: Low]
Does this need a "depends on OF" directive? Without it, of_match_ptr() can
evaluate to NULL when CONFIG_OF is disabled, causing a compiler warning for
an unused nvt_i2c_of_match variable.

[ ... ]
> diff --git a/drivers/i2c/busses/i2c-nt726xx.c b/drivers/i2c/busses/i2c-nt726xx.c
> new file mode 100644
> index 0000000000000..ac3fd7ccc68cb
> --- /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_PSWD, 4);

[Severity: High]
Is it safe to call ioremap() on a hardcoded physical address here? Since
this is called from nvt_i2c_resume() via the resume_early PM hook,
interrupts might be disabled. ioremap() can sleep, which could trigger a
scheduling-while-atomic panic during early resume.

Also, should this physical address be managed via standard device tree
resources instead?

[ ... ]
> +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 an interrupt fires while current_msg is NULL, this returns IRQ_NONE
without clearing the hardware interrupt status in I2C_REG_INTR. Since the
IRQ is requested as IRQF_TRIGGER_HIGH (level-triggered), won't the
uncleared interrupt keep the IRQ line asserted and lead to an interrupt
storm?

[ ... ]
> +static int nvt_i2c_write(struct nvt_i2c_bus *i2c, struct i2c_msg *msg)
> +{
[ ... ]
> +	ctrl_mask = readl(i2c->base + I2C_REG_CTRL);
> +	ctrl_mask |= (((msg->addr << 1) << 8) | I2C_ENABLE |
> +			I2C_CLOCK_DUTY_ENABLE | I2C_CLOCK_STRETCH_ENABLE |
> +			I2C_MASTER_CLK_STRETCH_ENABLE | I2C_TRIGGER);
> +	writel(ctrl_mask, i2c->base + I2C_REG_CTRL);

[Severity: High]
Since this uses a bitwise OR to set the address and flags (like
I2C_READ_OPERATION) into ctrl_mask, and the previous state is not cleared,
won't subsequent messages bitwise OR their target address with the previous
message's address? Could this also cause a write message to be executed as
a read if the previous message was a read?

> +
> +	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;
> +
> +	return i2c->error_code;
> +}

[Severity: High]
When a transfer times out, this sets i2c->current_msg to NULL and returns,
which allows the caller to free the message buffer. What happens if the ISR
is running concurrently on another CPU and has already cached current_msg?

Could the ISR proceed to access the freed msg->buf, causing a
use-after-free?

[ ... ]
> +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];
> +
> +		if (msg->flags & I2C_M_RD)
> +			ret = nvt_i2c_read(i2c, msg);
> +		else
> +			ret = nvt_i2c_write(i2c, msg);

[Severity: High]
Is it intentional to unconditionally pass msgs[0] to nvt_i2c_set_subaddr()
and only execute the transfer for msgs[1]?

If msgs[0] is actually a read request, its buffer could be uninitialized
memory, which nvt_i2c_set_subaddr() might then read and write to the
hardware register, potentially leaking kernel memory. Additionally, wouldn't
this break the API contract by silently dropping the first message of a
valid 2-message sequence?

[ ... ]
> +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 there a specific reason to use a custom property like 'novatek,hwmods'
for assigning the I2C bus number? Typically, fixed indices for I2C
controllers are assigned using standard device tree aliases, like this:
aliases { i2c0 = &i2c_node; };

[ ... ]
> +static int nvt_i2c_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	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,

[Severity: High]
The nvt_i2c_init() function enables hardware interrupts before the handler
is registered by devm_request_irq(). If the controller already has a
pending interrupt on boot, won't this trigger an immediate interrupt storm,
especially since the ISR returns IRQ_NONE when current_msg is NULL?

[ ... ]
> +	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 error path need to call
of_node_put() to release the reference to i2c->adapter.dev.of_node
acquired earlier via of_node_get()?

[ ... ]
> +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]
By disabling the I2C controller and its interrupts before calling
i2c_del_adapter(), couldn't we interrupt active I2C operations?

If a transfer is ongoing concurrently, disabling the hardware out from under
it will cause it to stall and time out. Should i2c_del_adapter() be called
first to cleanly block new transfers and wait for active ones to finish?

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

[Severity: Low]
Should these callbacks be wrapped with pm_sleep_ptr()? When CONFIG_PM_SLEEP
is disabled, these functions and the struct assignment below will become
dead code and might trigger compiler warnings.

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

[Severity: Low]
Can we wrap the pm_ops pointer with pm_ptr() to ensure it compiles away cleanly
when CONFIG_PM is disabled?

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