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

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

[Carlos Song (OSS)]

From: Carlos Song <carlos.song@nxp.com>

Mark the I2C adapter as suspended during system suspend to block further
transfers, and resume it on system resume. This prevents potential hangs
when the hardware is powered down but clients still attempt I2C transfers.

Fixes: 358025ac091e ("i2c: imx: make controller available until system suspend_noirq() and from resume_noirq()")
Cc: stable@vger.kernel.org
Signed-off-by: Carlos Song <carlos.song@nxp.com>
---
 drivers/i2c/busses/i2c-imx.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index a208fefd3c3b..3b19d4a424ca 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -1913,6 +1913,34 @@ static int i2c_imx_runtime_resume(struct device *dev)
 	return ret;
 }
 
+static int __maybe_unused i2c_imx_suspend_noirq(struct device *dev)
+{
+	struct imx_i2c_struct *i2c_imx = dev_get_drvdata(dev);
+	int ret;
+
+	ret = pm_runtime_force_suspend(dev);
+	if (ret)
+		return ret;
+
+	i2c_mark_adapter_suspended(&i2c_imx->adapter);
+
+	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;
+
+	i2c_mark_adapter_resumed(&i2c_imx->adapter);
+
+	return 0;
+}
+
 static int i2c_imx_suspend(struct device *dev)
 {
 	/*
@@ -1946,8 +1974,8 @@ static int i2c_imx_resume(struct device *dev)
 }
 
 static const struct dev_pm_ops i2c_imx_pm_ops = {
-	NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
-				  pm_runtime_force_resume)
+	NOIRQ_SYSTEM_SLEEP_PM_OPS(i2c_imx_suspend_noirq,
+				  i2c_imx_resume_noirq)
 	SYSTEM_SLEEP_PM_OPS(i2c_imx_suspend, i2c_imx_resume)
 	RUNTIME_PM_OPS(i2c_imx_runtime_suspend, i2c_imx_runtime_resume, NULL)
 };
-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Race condition: hardware powered down before I2C adapter is marked suspended, risking bus hangs or crashes.
- [High] PM Guideline Violation: Shared IRQ handler accesses hardware registers without checking Runtime PM state.
- [High] PM Guideline Violation: Missing `synchronize_irq()` in `i2c_imx_runtime_suspend()`.
--

commit 3cc1c1c3065630f26db74add04c2d7534a0cff83
Author: Carlos Song <carlos.song@nxp.com>

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 resumes the adapter on system resume to prevent
hangs when clients attempt transfers while hardware is powered down.

> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index a208fefd3c3b3..3b19d4a424ca0 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1913,6 +1913,34 @@ static int i2c_imx_runtime_resume(struct device *dev)
>  	return ret;
>  }
>  
> +static int __maybe_unused i2c_imx_suspend_noirq(struct device *dev)
> +{
> +	struct imx_i2c_struct *i2c_imx = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = pm_runtime_force_suspend(dev);

Could calling pm_runtime_force_suspend() before marking the adapter as
suspended introduce a race condition?

If a concurrent transfer is in progress, this will disable the hardware clock
while the transfer is actively accessing registers.

> +	if (ret)
> +		return ret;
> +
> +	i2c_mark_adapter_suspended(&i2c_imx->adapter);

Since i2c_mark_adapter_suspended() attempts to acquire the adapter bus lock,
would it be safer to call it before pm_runtime_force_suspend() to synchronize
and prevent new transfers?

If pm_runtime_force_suspend() fails after marking the adapter suspended,
i2c_mark_adapter_resumed() could then be called to roll back the state.

> +
> +	return 0;
> +}

[ ... ]

While reviewing this code, I also noticed a couple of pre-existing issues
related to Power Management and shared IRQs in the driver.

Does the shared IRQ handler access hardware registers without checking the
Runtime PM state?

drivers/i2c/busses/i2c-imx.c:i2c_imx_probe() {
    ...
    request_irq(..., IRQF_SHARED, ...)
    ...
}

If another device sharing the same interrupt line fires an IRQ while the I2C
controller is runtime-suspended, the I2C interrupt handler will execute and
read from powered-down hardware. Could pm_runtime_get_if_active() be used in
the ISR to verify the state first?


Also, does i2c_imx_runtime_suspend() need to call synchronize_irq()?

Because the interrupt is requested with IRQF_SHARED, disabling the hardware
clock without synchronizing first could cause an invalid register read/write
if the ISR is concurrently running on another CPU.

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