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 16 potential issue(s) to consider:
- [Critical] Timeout handling in I2C read/write lacks synchronization, leaving a dangling pointer accessible by the interrupt handler.
- [Critical] Unbinding the driver causes a NULL pointer dereference if `bus-enable` is false or missing.
- [Critical] The `clock-frequency` DT property expects kHz and causes a divide-by-zero panic if correctly configured.
- [Critical] Control register fields are bitwise ORed without clearing previous state, allowing I2C target isolation bypass.
- [High] The `master_xfer` implementation unilaterally drops or misroutes the first message of any 2-message transfer.
- [High] Virtual memory mapping leaked during initialization due to missing `iounmap()`.
- [High] The driver advertises SMBus Quick support but explicitly rejects 0-length messages.
- [High] The ISR returns `IRQ_NONE` without clearing device interrupts if no transfer is active, causing an unhandled interrupt storm.
- [High] Device interrupts are not disabled during driver removal, risking a shared IRQ storm.
- [High] The driver interacts with hardware registers without ensuring clocks and power domains are enabled.
- [Medium] The driver hardcodes physical memory addresses to access system-level control registers.
- [Medium] The driver parses custom, non-standard Device Tree properties to handle core subsystem state.
- [Medium] Unconditional assignment of PM callbacks causes dead code when PM features are disabled.
- [Medium] The commit message contains a corporate confidentiality notice.
- [Medium] Device tree node reference acquired via `of_node_get()` is never released.
- [Medium] The driver uses `writel()`/`readl()` with manual byte-packing for FIFO access instead of stream accessors.
--

commit 57029ad0ef0636d7917520387b5868dcfc696f11
Author: SP_ISW1_AT@novatek.com.tw <SP_ISW1_AT@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 basic read/write functions in Standard and Fast modes.

> CONFIDENTIALITY NOTICE: This message, including attachments, contains information which may be confidential and privileged, and is intended only for use by the addressees designated above. Unless you are the intended recipient, any use, copying, disclosure, or distribution is prohibited. If you have received the message in error, please immediately delete the message and destroy all copies thereof and notify the sender by reply email. 本郵件及任何附件均屬機密，僅供其上指定地址之收件人使用。除非您是指定之收件人，否則請勿使用、複製、揭露或散布本郵件之任何部份。若您錯誤地收到此郵件，請立即回覆電子郵件通知寄件人，並請完全刪除且銷毀本郵件及其複本。

This isn't a regression, but the commit message contains a corporate
confidentiality notice.

> diff --git a/drivers/i2c/busses/i2c-nt726xx.c b/drivers/i2c/busses/i2c-nt726xx.c
> new file mode 100644
> index 0000000000000..b0e79722a85b0
> --- /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);

[Severity: Medium]
Is it safe to hardcode physical memory addresses like STBC_PASSWORD to access
system-level control registers, rather than using standard Device Tree
bindings or resource management?

> +		writel(readl(reg_tmp) | STBC_PASSWORD_DATA1, reg_tmp);
> +		writel(readl(reg_tmp) | STBC_PASSWORD_DATA2, reg_tmp);
> +
> +		reg_tmp = ioremap(STBC_KEYPASS, 4);

[Severity: High]
Does this code leak the first virtual memory mapping? The pointer assigned
to reg_tmp from ioremap is overwritten here without a corresponding iounmap(),
creating a persistent leak.

[ ... ]
> +static void nvt_i2c_set_clk(struct nvt_i2c_bus *i2c)
> +{
> +	unsigned int duty = 0;
> +	unsigned int clk_div = 0;
> +	unsigned int source_speed = i2c->stbc_i2c ?
> +		i2c->comp_data->stbc_clock : i2c->comp_data->sys_clock;
> +
> +	clk_div = (source_speed / i2c->bus_clk_rate) / 1000;

[Severity: Critical]
Does this assume the clock-frequency property is in kHz?

The device tree standard mandates it is specified in Hertz. If a standard
value like 400000 is provided, could this integer division result in 0,
locking up the hardware or causing a divide-by-zero kernel panic?

[ ... ]
> +static void nvt_i2c_set_subaddr(struct nvt_i2c_bus *i2c, struct i2c_msg *msg)
> +{
[ ... ]
> +		for (i = 0; i < msg->len; i++)
> +			subaddr |= msg->buf[i] << (8 * (msg->len - 1 - i));

[Severity: High]
Could this read from uninitialized memory if the first message in a
transfer is a read command?

[ ... ]
> +static void nvt_i2c_write_fifo(struct nvt_i2c_bus *i2c,
> +			       unsigned int fifo_reg,
> +			       const unsigned char *buf,
> +			       unsigned int buf_offset,
> +			       unsigned int length)
> +{
> +	unsigned int reg_idx = 0, copy_bytes = 0, j = 0, value = 0;
> +
> +	while (length > 0) {
> +		value = 0;
> +		copy_bytes = length >= FIFO_WORD_BYTES ? FIFO_WORD_BYTES : length;
> +		for (j = 0; j < copy_bytes; j++)
> +			value |= ((unsigned int)buf[buf_offset + j]) << (j * 8);
> +
> +		writel(value, i2c->base + fifo_reg + reg_idx * 4);

[Severity: Medium]
Would it be better to use stream accessors like writesl() instead of manually
byte-packing into a writel() call? The standard register accessors perform
implicit byte-swapping which may be unintended for FIFO data streams.

[ ... ]
> +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 is active but i2c->current_msg is NULL, could returning
IRQ_NONE without clearing the hardware interrupt flag cause an unhandled
interrupt storm on a shared, level-triggered line?

[ ... ]
> +	} else if (status & I2C_IRQ_TX_EMPTY) {
> +		if (i2c->remaining > 0)
> +			nvt_i2c_handle(i2c, msg, 0);

[Severity: Critical]
Can this result in a use-after-free regression if the wait_for_completion
expires?

If wait_for_completion_timeout() times out in nvt_i2c_write(), it sets
i2c->current_msg to NULL and returns, leaving the stack-allocated msg
to be freed. If an interrupt fires at that exact moment, the asynchronous
ISR might have already copied i2c->current_msg to the local msg variable
before it was cleared, leading to nvt_i2c_handle() accessing freed
stack memory.

[ ... ]
> +static int nvt_i2c_check_msg(const struct i2c_msg *msg)
> +{
> +	if (!msg || !msg->buf || msg->len == 0 || msg->len > 4096)
> +		return -EINVAL;

[Severity: High]
Will this reject 0-length messages explicitly? The adapter advertises
I2C_FUNC_SMBUS_EMUL later on, which implies support for SMBus Quick
commands that rely on 0-length transactions.

[ ... ]
> +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: Critical]
Does this bitwise-OR the target address without first clearing the existing
address bits in the control mask?

During a sequence of multiple messages in a single transfer, the previous
address might not be cleared, causing subsequent messages to transmit a
corrupted, logically-combined address on the bus. This could potentially
bypass I2C target isolation checks.

[ ... ]
> +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 unilaterally drop the first message of any 2-message transfer and
treat it as a hardware subaddress? This could break standard 2-message I2C
sequences, such as interacting with two separate targets or consecutive writes.

[ ... ]
> +static int nvt_i2c_parse_dts(struct nvt_i2c_bus *i2c)
> +{
[ ... ]
> +	/* read DTS(nvt,hwmods) and set bus number */
> +	ret = of_property_read_string(np, "nvt,hwmods", &hwmods_val);
[ ... ]
> +	/* read DTS(bus-enable) */
> +	ret = of_property_read_u32(np, "bus-enable", &i2c->bus_enable);

[Severity: Medium]
Is there a reason to parse custom properties like nvt,hwmods and bus-enable
instead of relying on standard I2C core bindings like aliases and the
standard status = "disabled" flag?

[ ... ]
> +static int nvt_i2c_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ret = nvt_i2c_parse_dts(i2c);
> +	if (ret || !i2c->bus_enable)
> +		return ret;

[Severity: Critical]
If bus-enable is false, this returns 0 (success) before calling
platform_set_drvdata(). Could this cause a NULL pointer dereference
later during unbind when nvt_i2c_remove() fetches the empty drvdata?

> +
> +	nvt_i2c_init(i2c);

[Severity: High]
Does nvt_i2c_init() access hardware registers before ensuring the system bus
clocks and I2C power domain are enabled? Accessing a clock-gated or
powered-down peripheral can trigger an imprecise external abort.

[ ... ]
> +	i2c->adapter.dev.of_node = of_node_get(pdev->dev.of_node);

[Severity: Medium]
Is this of_node reference ever released? There doesn't appear to be a
matching of_node_put() in the teardown paths, which would cause a memory
leak across bind/unbind cycles.

[ ... ]
> +static void nvt_i2c_remove(struct platform_device *pdev)
> +{
> +	struct nvt_i2c_bus *i2c = platform_get_drvdata(pdev);
> +
> +	i2c_del_adapter(&i2c->adapter);
> +}

[Severity: High]
Should device hardware interrupts be disabled before unregistering the
adapter and removing the driver? Since a shared IRQ line is requested,
an interrupt fired during or after teardown could cause an unhandled
interrupt storm.

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

[Severity: Medium]
Would it be better to wrap these callback assignments with pm_sleep_ptr()
to avoid generating dead code when PM features are 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: Medium]
Similarly, should the pm structure pointer be wrapped with pm_ptr()?

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