Re: [PATCH v3] i2c: imx: mark I2C adapter when hardware is powered down

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] If system suspend fails, the I2C slave state machine loses its timeout protection and can permanently hang.

Pre-existing issues:
- [High] The interrupt handler unconditionally accesses hardware registers on a shared IRQ line without checking PM state, which will cause a crash if the device is suspended.
- [High] The `slave_timer` can be re-armed during driver removal, leading to a Use-After-Free.
- [High] The `slave_timer` is not cancelled when a slave is unregistered, potentially causing a NULL pointer dereference.
- [High] The I2C slave mode registers are lost during system suspend and are not restored upon system resume, leaving the slave device non-functional.
--

commit 8b5535f823cf602789b0659057d09e77d12c12d3
Author: Carlos Song [off-list ref]

i2c: imx: mark I2C adapter when hardware is powered down

This commit marks the I2C adapter as suspended during system suspend to block
further transfers. It then resumes the adapter on system resume, preventing
hangs when clients attempt transfers while the hardware is powered down.

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but I have a question about the
slave timer lifecycle during driver removal.

In i2c_imx_remove(), hrtimer_cancel() is called before i2c_del_adapter()
and free_irq(). Because the interrupt is still active, could an incoming
I2C transaction trigger i2c_imx_isr(), process a slave event, and restart
the timer?

After i2c_imx_remove() completes, the i2c_imx structure is freed, which
might result in a use-after-free when the re-armed timer expires.

[Severity: High]
This is a pre-existing issue, but I also have a question about the timer
when a slave is unregistered.

In i2c_imx_unreg_slave(), i2c_imx->slave is set to NULL, but the timer is
not explicitly cancelled. If the timer fires after unregistration, could it
call i2c_slave_event() with a NULL client pointer, leading to a dereference
crash?

+
+	ret = pm_runtime_force_suspend(dev);
+	if (ret) {
+		i2c_mark_adapter_resumed(&i2c_imx->adapter);
+		return ret;
+	}

[Severity: High]
If pm_runtime_force_suspend() fails, we restore the adapter availability
and return an error to abort system suspend, but the slave timer remains
cancelled.

If a slave transaction was ongoing and relied on this timer for timeout
protection, does cancelling it without restoring leave the I2C bus state
machine permanently hung?

+
+	return 0;
+}
+
+static int __maybe_unused i2c_imx_resume_noirq(struct device *dev)
+{
+	struct imx_i2c_struct *i2c_imx = dev_get_drvdata(dev);
+	int ret;
+
+	ret = pm_runtime_force_resume(dev);
+	if (ret)
+		return ret;

[Severity: High]
This is a pre-existing issue, but does pm_runtime_force_resume() properly
restore the slave hardware registers?

During system suspend, the controller may lose its register state (like
IMX_I2C_IADR and IMX_I2C_I2CR) when the hardware domain powers down. Since
the runtime resume callback doesn't re-initialize these registers, will
the controller forget its slave address and lose the slave enable bit,
breaking slave mode until the driver is reloaded?

+
+	i2c_mark_adapter_resumed(&i2c_imx->adapter);
+
+	return 0;
+}

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but since the driver requests
a shared interrupt with IRQF_NO_SUSPEND, is it safe for i2c_imx_isr()
to unconditionally access hardware registers (like IMX_I2C_I2SR) without
checking the PM state?

When the I2C controller is powered down and its clock is disabled during
suspend, a spurious interrupt or one from another device sharing the line
could invoke the ISR.

Since accessing unclocked registers causes a synchronous external abort,
should the ISR use pm_runtime_get_if_active() to prevent a crash?

 	SYSTEM_SLEEP_PM_OPS(i2c_imx_suspend, i2c_imx_resume)
 	RUNTIME_PM_OPS(i2c_imx_runtime_suspend, i2c_imx_runtime_resume, NULL)
 };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520101504.2885873-1-carlos.song@oss.nxp.com?part=1