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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Double pm_runtime_disable() due to mixing devres and manual goto cleanup on probe error paths.
- [High] Missing NULL checks and synchronization for `i2c->target` leads to a kernel panic in the target ISR.
- [High] Data race on transfer state fields (`i2c->msg`, `i2c->msg_num`) leading to spurious interrupts and transfer timeouts.
- [Medium] Handling bus errors or spurious interrupts while the controller is idle permanently disables target mode.
--

--- Patch [2]: [PATCH v7 2/3] i2c: ma35d1: Add Nuvoton MA35D1 I2C driver support ---
commit 035b6bb91f9d1b731210ec6e2aa45306a8f7f5f8
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..858cfc2a873fb
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-ma35d1.c

[ ... ]

> +static void ma35d1_i2c_stop(struct ma35d1_i2c *i2c, int ret)
> +{
> +	ma35d1_i2c_write_ctl(i2c, MA35_CTL_STO_SI);

[Severity: Medium]
Does handling bus errors or spurious interrupts permanently disable target
mode?

ma35d1_i2c_write_ctl() clears the MA35_CTL_STA_SI_AA bits before applying the
new control mask. Passing only MA35_CTL_STO_SI (or MA35_CTL_SI in the
spurious interrupt handler) leaves the MA35_CTL_AA bit cleared.

Will this prevent the I2C controller from acknowledging its own slave address
until the next controller transfer is initiated?

[ ... ]

> +static void ma35d1_i2c_irq_target_trx(struct ma35d1_i2c *i2c,
> +				      unsigned long i2c_status)
> +{
> +	unsigned char byte;
> +
> +	switch (i2c_status) {
> +	case MA35_S_RECE_ARB_LOST:
> +		/*
> +		 * Arbitration lost during SLA+R/W (Address) phase.
> +		 * The hardware automatically switches to Target mode as our
> +		 * own slave address was detected.
> +		 */
> +		i2c->err = -EAGAIN;
> +		ma35d1_i2c_controller_complete(i2c);
> +		i2c_slave_event(i2c->target, I2C_SLAVE_WRITE_REQUESTED, &byte);

[Severity: High]
Is it possible for this to dereference a NULL pointer?

If an interrupt fires before a target is registered, or if
ma35d1_unreg_target() clears i2c->target concurrently without a
synchronize_irq() barrier, i2c_slave_event() could attempt to use a NULL
i2c->target.

[ ... ]

> +static void ma35d1_i2c_irq_controller_trx(struct ma35d1_i2c *i2c,
> +					  unsigned long i2c_status)
> +{
> +	unsigned char byte;
> +
> +	/* Ensuring messages are absolutely valid in Controller mode. */
> +	if (unlikely(!i2c->msg)) {
> +		dev_err(i2c->dev, "Spurious controller interrupt (status: 0x%02lx)\n", i2c_status);
> +		ma35d1_i2c_write_ctl(i2c, MA35_CTL_SI);

[Severity: Medium]
As mentioned above, applying MA35_CTL_SI without MA35_CTL_AA strips the
assert acknowledge bit, which could silently break target mode.

[ ... ]

> +static int ma35d1_i2c_doxfer(struct ma35d1_i2c *i2c, struct i2c_msg *msgs,
> +			     int num)
> +{
> +	unsigned long timeout;
> +	unsigned int val;
> +	int ret, err;
> +
> +	i2c->msg = msgs;
> +	i2c->msg_num = num;
> +	i2c->msg_ptr = 0;
> +	i2c->msg_idx = 0;
> +	i2c->err = 0;

[Severity: High]
Are these transfer state variables safe from concurrent modification?

If an interrupt fires (such as target arbitration lost) while
ma35d1_i2c_doxfer() is setting up the transfer, the controller complete path
could execute:

ma35d1_i2c_controller_complete()
    i2c->msg = NULL;
    i2c->msg_num = 0;

This could unconditionally clear the state variables, leading to a
corrupted state and transfer timeouts. Should this setup be protected by
i2c->lock or have local interrupts disabled?

[ ... ]

> +static void ma35d1_i2c_pm_cleanup(void *data)
> +{
> +	struct device *dev = data;
> +
> +	pm_runtime_get_sync(dev);
> +	pm_runtime_disable(dev);
> +	pm_runtime_put_noidle(dev);
> +}

[ ... ]

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

[ ... ]

> +rpm_put:
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_suspended(dev);
> +	return ret;
> +}

[Severity: High]
Does this execute pm_runtime_disable() twice on error paths?

Since devm_add_action_or_reset() registers ma35d1_i2c_pm_cleanup() to run
automatically on probe failure, explicitly jumping to the rpm_put label will
call pm_runtime_disable() a second time.

Could this leave the device's disable_depth counter permanently incremented
if the driver undergoes probe deferral?

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