Re: [PATCH v4 2/3] i2c: ma35d1: Add Nuvoton MA35D1 I2C driver support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Missing IRQ synchronization on teardown paths causes use-after-free and NULL pointer dereferences.
- [High] Lockless register access and unsafe read-modify-write operations on MA35_CTL0 corrupt the hardware state machine.
- [High] Arbitration lost is handled incorrectly, violating I2C specifications and causing transfer timeouts.
- [High] Explicit pm_runtime_disable() in the probe error path causes a double disable, permanently breaking deferred probes.
- [High] Missing `.remove` callback leaves the I2C clock running and violates the clock API if the module is unloaded while runtime active.
- [High] Clearing MA35_CTL_I2CEN in ma35d1_unreg_target() breaks subsequent master transfers due to PM runtime state mismatch.
- [Medium] The driver falsely advertises I2C_FUNC_PROTOCOL_MANGLING but incorrectly handles I2C_M_IGNORE_NAK and ignores I2C_M_NOSTART.
- [Medium] The system suspend callback leaks power by leaving the I2C clock enabled if the device is active and target mode is inactive.
--

commit bda32059fddeb5d34a52e14ab99c35478baf80a4
Author: Zi-Yu Chen <zychennvt@gmail.com>

    i2c: ma35d1: Add Nuvoton MA35D1 I2C driver support
    
    Add I2C support for Nuvoton MA35D1 SoC. The controller supports standard,
    fast and fast-plus modes, and provides controller/target functionality.

> diff --git a/drivers/i2c/busses/i2c-ma35d1.c b/drivers/i2c/busses/i2c-ma35d1.c
> new file mode 100644
> index 0000000000000..7b6b20c0996d2
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-ma35d1.c
[ ... ]
> +static void ma35d1_i2c_write_CTL(struct ma35d1_i2c *i2c, unsigned int ctl)
> +{
> +	unsigned int val;
> +
> +	val = readl(i2c->regs + MA35_CTL0);
> +	val &= ~(MA35_CTL_STA_SI_AA | MA35_CTL_STO);
> +	val |= ctl;
> +	writel(val, i2c->regs + MA35_CTL0);
> +}

[Severity: High]
Is there a missing lock here? The driver performs unprotected
read-modify-write operations on the shared MA35_CTL0 register. A timeout or
unbind process calling ma35d1_i2c_disable_irq() could race with the
interrupt handler updating state via ma35d1_i2c_write_CTL(), clobbering the
STO or SI bits and locking up the hardware.

[ ... ]
> +static void ma35d1_i2c_disable_irq(struct ma35d1_i2c *i2c)
> +{
> +	unsigned long tmp;
> +
> +	tmp = readl(i2c->regs + MA35_CTL0);
> +	writel(tmp & ~MA35_CTL_INTEN, i2c->regs + MA35_CTL0);
> +}

[Severity: High]
Can this read-modify-write race with the interrupt handler? See the comment
on ma35d1_i2c_write_CTL() above.

[ ... ]
> +static void i2c_ma35d1_irq_target_trx(struct ma35d1_i2c *i2c,
> +				      unsigned long i2c_status)
> +{
[ ... ]
> +	case MA35_S_REPEAT_START_STOP:
> +	/* A STOP or repeated START has been received
> +	 *  while still addressed as Target/Receiver
> +	 */
> +		i2c_slave_event(i2c->target, I2C_SLAVE_STOP, &byte);
> +		break;
> +
> +	default:
> +		dev_err(i2c->dev, "Status 0x%02lx is NOT processed\n",
> +			i2c_status);
> +		break;
> +	}
> +	ma35d1_i2c_write_CTL(i2c, MA35_CTL_SI_AA);
> +}

[Severity: High]
Does this fail to handle target arbitration lost states (0x68, 0xb0)? If
execution falls to the default case, the target backend is never notified of
the transfer start, potentially desynchronizing the target state machine.

[ ... ]
> +static void i2c_ma35d1_irq_controller_trx(struct ma35d1_i2c *i2c,
> +					  unsigned long i2c_status)
> +{
[ ... ]
> +	case MA35_M_TRAN_ADDR_NACK:
> +	case MA35_M_RECE_ADDR_NACK:
> +		/* Controller Transmit Address NACK */
> +		/* 0x20: SLA+W has been transmitted and NACK has been received */
> +		/* 0x48: SLA+R has been transmitted and NACK has been received */
> +		if (i2c->msg->flags & I2C_M_IGNORE_NAK) {
> +			ma35d1_i2c_stop(i2c, 0);
> +		} else {

[Severity: Medium]
Does this correctly implement I2C_M_IGNORE_NAK? NACKs are expected to be
ignored to proceed with transmitting the payload as if an ACK occurred, but
this terminates the transfer completely.

[ ... ]
> +static irqreturn_t ma35d1_i2c_irq(int irqno, void *dev_id)
> +{
> +	struct ma35d1_i2c *i2c = dev_id;
> +	unsigned long status;
> +
> +	status = readl(i2c->regs + MA35_STATUS0);
> +
> +	if (status == MA35_M_ARB_LOST) {
> +		dev_err(i2c->dev, "Arbitration lost\n");
> +		ma35d1_i2c_stop(i2c, -EAGAIN);
> +		goto out;
> +	}

[Severity: High]
Should this issue a STOP condition when arbitration is lost? Generating a
STOP while another master is actively driving the bus violates the I2C
protocol, which can disrupt the winning master's transfer.

[ ... ]
> +static int ma35d1_i2c_doxfer(struct ma35d1_i2c *i2c, struct i2c_msg *msgs,
> +			     int num)
> +{
[ ... ]
> +	ma35d1_i2c_enable_irq(i2c);
> +	ma35d1_i2c_write_CTL(i2c, MA35_CTL_STA_SI);
> +	timeout = wait_event_timeout(i2c->wait, i2c->msg_num == 0, HZ * 5);
> +	ma35d1_i2c_disable_irq(i2c);
> +	ret = i2c->msg_idx;
> +
> +	if (timeout == 0) {
> +		dev_dbg(i2c->dev, "xfer timeout\n");
> +		ret = -ETIMEDOUT;
> +		goto reset;
> +	}

[Severity: High]
Are we missing synchronization with inflight ISRs here? Disabling the IRQ at
the register level doesn't stop an already executing handler. Could the ISR
blindly proceed to dereference the message buffer after it has been freed?
This might need synchronize_irq() after ma35d1_i2c_disable_irq().

[ ... ]
> +static int ma35d1_unreg_target(struct i2c_client *target)
> +{
> +	struct ma35d1_i2c *i2c = i2c_get_adapdata(target->adapter);
> +	unsigned int val;
> +	int ret;
> +
> +	if (!i2c->target)
> +		return -EINVAL;
> +
> +	/* Disable I2C interrupt */
> +	ma35d1_i2c_disable_irq(i2c);
> +
> +	/* Disable I2C */
> +	val = readl(i2c->regs + MA35_CTL0);
> +	val &= ~MA35_CTL_I2CEN;
> +	writel(val, i2c->regs + MA35_CTL0);
> +
> +	i2c->target = NULL;

[Severity: High]
Similar to ma35d1_i2c_doxfer(), does this need synchronize_irq() before
setting i2c->target to NULL? Setting it to NULL while an ISR is concurrently
executing on another CPU could cause a NULL pointer dereference.

> +
> +	ret = pm_runtime_put_sync(i2c->dev);

[Severity: High]
Can clearing MA35_CTL_I2CEN above break subsequent master transfers? If the
autosuspend timer is active, pm_runtime_put_sync() leaves the device in the
RPM_ACTIVE state. A subsequent master transfer could observe RPM_ACTIVE and
skip runtime_resume(), resulting in an -EBUSY timeout because the hardware
was disabled.

[ ... ]
> +/* Declare Our I2C Functionality */
> +static u32 ma35d1_i2c_func(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_PROTOCOL_MANGLING | I2C_FUNC_SMBUS_EMUL;
> +}

[Severity: Medium]
Should this driver advertise I2C_FUNC_PROTOCOL_MANGLING? It unconditionally
issues a START condition (ignoring I2C_M_NOSTART) and handles
I2C_M_IGNORE_NAK incorrectly.

[ ... ]
> +static int ma35d1_i2c_probe(struct platform_device *pdev)
> +{
[ ... ]
> +rpm_put:
> +	pm_runtime_dont_use_autosuspend(dev);
> +	pm_runtime_put_sync(dev);
> +	pm_runtime_disable(dev);
> +	return ret;
> +}

[Severity: High]
Can this cause a double disable of runtime PM? The driver uses
devm_pm_runtime_enable() which attaches a devres cleanup action. Calling
pm_runtime_disable() manually increments the disable_depth, and the
subsequent devres cleanup increments it again, preventing the device from
powering on during deferred probes.

> +static int ma35d1_i2c_suspend(struct device *dev)
> +{
> +	struct ma35d1_i2c *i2c = dev_get_drvdata(dev);
> +	unsigned int val;
> +
> +	/* Prepare for wake-up from I2C events if target mode is active */
> +	if (i2c->target) {
> +		val = readl(i2c->regs + MA35_CTL0);
> +		val |= (MA35_CTL_SI | MA35_CTL_AA);
> +		writel(val, i2c->regs + MA35_CTL0);
> +
> +		/* Setup wake-up control */
> +		writel(0x1, i2c->regs + MA35_WKCTL);
> +
> +		/* Clear pending wake-up flags */
> +		val = readl(i2c->regs + MA35_WKSTS);
> +		writel(val, i2c->regs + MA35_WKSTS);
> +
> +		enable_irq_wake(i2c->irq);
> +
> +		ma35d1_i2c_enable_irq(i2c);
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
Could this leak power by leaving the I2C clock enabled if the device is
active and target mode is inactive? If the device happened to be in the
RPM_ACTIVE state when the system goes to sleep, the clock remains fully
enabled for the duration of the suspend.

[ ... ]
> +static struct platform_driver ma35d1_i2c_driver = {
> +	.probe      = ma35d1_i2c_probe,
> +	.driver     = {
> +		.name   = "ma35d1-i2c",
> +		.of_match_table = ma35d1_i2c_of_match,
> +		.pm = pm_ptr(&ma35d1_i2c_pmops),
> +	},
> +};

[Severity: High]
Is a .remove callback needed here? If the module is unloaded while the device
is in the RPM_ACTIVE state (e.g., within the 5000ms autosuspend window), the
clock remains enabled. During devres unbinding, executing clk_unprepare() on
an enabled clock violates the clock framework constraints.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610030208.2020275-1-zychennvt@gmail.com?part=2