[PATCH v6 0/2] i2c: imx-lpi2c: fix probe error handling and reset controller

[PATCH v6 0/2] i2c: imx-lpi2c: fix probe error handling and reset controller

[carlos.song]

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

During probe, two issues exist in the LPI2C driver:

1. The error paths do not properly unwind all acquired resources on
   failure. Clocks enabled before runtime PM initialization are never
   disabled on certain error paths, when probe fails after runtime PM
   is initialized, the previous error path called pm_runtime_put_sync(),
   however, due to different clock management strategies on different
   SoCs[1] (to avoid deadlocks between the global prepare_lock and runtime
   PM), the callback may only disable clocks without unpreparing them,
   causing an incomplete unwind.

2. The LPI2C controller may retain unexpected state from previous
   boot stages, causing the system to hang during probe.

This series addresses both issues. Patch 1 restructures the probe
error paths to ensure correct resource cleanup on failure. Patch 2
resets the Master and Target controller logic before IRQ registration
to clear any leftover state.

[1] https://lore.kernel.org/all/20251125084718.2156168-1-carlos.song@nxp.com/

Changes for v6:
  - According to Markus Elfring suggestion, use a consistent format
    dev_err_probe to return ret, it help avoid duplicate source code.

Changes for v5:
  - Remove devm_free_irq() and free_irq err label.
  - Change commit log to explain why not using runtime PM to manage clocks
    during the probe phase.

Changes for v4:
  - Split v3 into two patches per reviewer feedback:
    * Patch 1 contains only the error path restructuring.
    * Patch 2 contains only the controller reset and the IRQ
      relocation that is required by the reset ordering.

Changes for v3:
  - Reset the Target logic via LPI2C_SCR in addition to MCR.
  - Replace pm_runtime_put_sync() with pm_runtime_disable() +
    pm_runtime_set_suspended() + pm_runtime_put_noidle() to avoid
    triggering the suspend callback during error recovery.
  - Add clk_disable and free_irq labels for complete error unwinding.
  - Update commit log to cover the SCR reset rationale.

Changes for v2:
  - Jump to rpm_disable instead of returning directly if the IRQ
request fails.

Carlos Song (2):
  i2c: imx-lpi2c: properly unwind resources on probe failure
  i2c: imx-lpi2c: reset controller in probe stage

 drivers/i2c/busses/i2c-imx-lpi2c.c | 42 +++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 12 deletions(-)

-- 
2.43.0

[PATCH v6 1/2] i2c: imx-lpi2c: properly unwind resources on probe failure

[carlos.song]

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

When probe fails at devm_clk_rate_exclusive_get() or clk_get_rate(),
which occur before runtime PM is initialized, the clocks enabled by
clk_bulk_prepare_enable() are never disabled.

When probe fails after runtime PM is initialized, the previous error
path called pm_runtime_put_sync(), which triggers the runtime suspend
callback. However, due to different clock management strategies on
different SoCs[1] (to avoid deadlocks between the global prepare_lock
and runtime PM), the callback may only disable clocks without
unpreparing them, causing an incomplete unwind.

Introduce a new error label 'clk_disable' to explicitly invoke
clk_bulk_disable_unprepare(). Replace pm_runtime_put_sync() with the
sequence of pm_runtime_disable(), pm_runtime_set_suspended() and
pm_runtime_put_noidle() to bypass the runtime suspend callback during
error recovery. During the LPI2C driver probe phase, clock APIs are
used exclusively to manage clocks. Once probing succeeds, clock
management is handed over to the runtime PM core.

[1] https://lore.kernel.org/all/20251125084718.2156168-1-carlos.song@nxp.com/

Signed-off-by: Carlos Song <carlos.song@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index e6c24a9d934d..615d9f7f1147 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -1527,14 +1527,18 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 	 * each transfer
 	 */
 	ret = devm_clk_rate_exclusive_get(&pdev->dev, lpi2c_imx->clks[0].clk);
-	if (ret)
-		return dev_err_probe(&pdev->dev, ret,
-				     "can't lock I2C peripheral clock rate\n");
+	if (ret) {
+		ret = dev_err_probe(&pdev->dev, ret,
+				    "can't lock I2C peripheral clock rate\n");
+		goto clk_disable;
+	}
 
 	lpi2c_imx->rate_per = clk_get_rate(lpi2c_imx->clks[0].clk);
-	if (!lpi2c_imx->rate_per)
-		return dev_err_probe(&pdev->dev, -EINVAL,
-				     "can't get I2C peripheral clock rate\n");
+	if (!lpi2c_imx->rate_per) {
+		ret = dev_err_probe(&pdev->dev, -EINVAL,
+				    "can't get I2C peripheral clock rate\n");
+		goto clk_disable;
+	}
 
 	if (lpi2c_imx->hwdata->need_prepare_unprepare_clk)
 		pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_LONG_TIMEOUT_MS);
@@ -1576,8 +1580,11 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 
 rpm_disable:
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
-	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
+	pm_runtime_put_noidle(&pdev->dev);
+clk_disable:
+	clk_bulk_disable_unprepare(lpi2c_imx->num_clks, lpi2c_imx->clks);
 
 	return ret;
 }
-- 
2.43.0

[PATCH v6 2/2] i2c: imx-lpi2c: reset controller in probe stage

[carlos.song]

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

Reset I2C controller in probe stage to avoid unexpected LPI2C controller
state left from previous stages and hang system boot.

Per the LPI2C reference manual, section 7.1.4 "Controller Control (MCR)"
and 7.1.20 Target Control (SCR), the RST bit (bit 1) description states:

  "The reset takes effect immediately and remains asserted until negated
  by software. There is no minimum delay required before clearing the
  software reset."

Therefore, it is safe to write 0 to MCR and SCR immediately after
asserting the RST bit without any additional delay.

Signed-off-by: Carlos Song <carlos.song@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index 615d9f7f1147..cfc0d108d3c4 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -1510,11 +1510,6 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 	if (ret)
 		lpi2c_imx->bitrate = I2C_MAX_STANDARD_MODE_FREQ;
 
-	ret = devm_request_irq(&pdev->dev, lpi2c_imx->irq, lpi2c_imx_isr, IRQF_NO_SUSPEND,
-			       pdev->name, lpi2c_imx);
-	if (ret)
-		return dev_err_probe(&pdev->dev, ret, "can't claim irq %d\n", lpi2c_imx->irq);
-
 	i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx);
 	platform_set_drvdata(pdev, lpi2c_imx);
 
@@ -1550,6 +1545,22 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
+	/*
+	 * Reset all internal controller registers of both Master and Target
+	 * to avoid effects of previous status.
+	 */
+	writel(MCR_RST, lpi2c_imx->base + LPI2C_MCR);
+	writel(SCR_RST, lpi2c_imx->base + LPI2C_SCR);
+	writel(0, lpi2c_imx->base + LPI2C_MCR);
+	writel(0, lpi2c_imx->base + LPI2C_SCR);
+
+	ret = devm_request_irq(&pdev->dev, lpi2c_imx->irq, lpi2c_imx_isr, IRQF_NO_SUSPEND,
+			       pdev->name, lpi2c_imx);
+	if (ret) {
+		dev_err_probe(&pdev->dev, ret, "can't claim irq %d\n", lpi2c_imx->irq);
+		goto rpm_disable;
+	}
+
 	temp = readl(lpi2c_imx->base + LPI2C_PARAM);
 	lpi2c_imx->txfifosize = 1 << (temp & 0x0f);
 	lpi2c_imx->rxfifosize = 1 << ((temp >> 8) & 0x0f);
-- 
2.43.0

Re: [PATCH v6 2/2] i2c: imx-lpi2c: reset controller in probe stage

[sashiko-bot]

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

New issues:
- [High] The probe error path explicitly disables device clocks while the devres-managed interrupt remains active, risking an external abort.

Pre-existing issues:
- [High] The interrupt handler unconditionally accesses hardware registers without verifying the device's runtime PM state, risking a synchronous external abort.
--

--- Patch [2]: [PATCH v6 2/2] i2c: imx-lpi2c: reset controller in probe stage ---
commit a2b44c86e6f00595e1b33e7174c800f8fdb5e7ca
Author: Carlos Song <carlos.song@nxp.com>

i2c: imx-lpi2c: reset controller in probe stage

This commit resets the I2C controller during the probe stage to prevent
unexpected states left from prior boot stages. It asserts and deasserts
software reset bits in the MCR and SCR registers. It also moves the
IRQ allocation to occur after this reset initialization.

> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index 615d9f7f11471..cfc0d108d3c4a 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c

[ ... ]

> @@ -1550,6 +1545,22 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>  	pm_runtime_set_active(&pdev->dev);
>  	pm_runtime_enable(&pdev->dev);
>  
> +	/*
> +	 * Reset all internal controller registers of both Master and Target
> +	 * to avoid effects of previous status.
> +	 */
> +	writel(MCR_RST, lpi2c_imx->base + LPI2C_MCR);
> +	writel(SCR_RST, lpi2c_imx->base + LPI2C_SCR);
> +	writel(0, lpi2c_imx->base + LPI2C_MCR);
> +	writel(0, lpi2c_imx->base + LPI2C_SCR);
> +
> +	ret = devm_request_irq(&pdev->dev, lpi2c_imx->irq, lpi2c_imx_isr, IRQF_NO_SUSPEND,
> +			       pdev->name, lpi2c_imx);

[Severity: High]
If an error occurs later in lpi2c_imx_probe() (such as in
lpi2c_imx_init_recovery_info() or i2c_add_adapter()), the error path jumps
to clk_disable and calls clk_bulk_disable_unprepare() before returning.

Since devm_request_irq() is used, the interrupt isn't freed until after the
probe function returns.

Does this create a window where the devres-managed IRQ remains active while
the device clocks are explicitly disabled? If a spurious interrupt arrives
during this window, could it cause an external abort?

[Severity: High]
This is a pre-existing issue, but lpi2c_imx_isr() unconditionally reads
hardware registers without verifying the runtime PM state:

drivers/i2c/busses/i2c-imx-lpi2c.c:lpi2c_imx_isr() {
    ...
    u32 scr = readl(lpi2c_imx->base + LPI2C_SCR);
    ...
}

Because IRQF_NO_SUSPEND is used, the interrupt could fire when the device
is in the RPM_SUSPENDED state with clocks disabled, especially for SoCs
where need_request_free_irq is false.

Would it be safer to check the PM state with pm_runtime_get_if_active()
in the interrupt handler before accessing registers, returning IRQ_NONE
if the device is suspended?

> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret, "can't claim irq %d\n", lpi2c_imx->irq);
> +		goto rpm_disable;
> +	}
> +
>  	temp = readl(lpi2c_imx->base + LPI2C_PARAM);

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

Re: [PATCH v6 1/2] i2c: imx-lpi2c: properly unwind resources on probe failure

[sashiko-bot]

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

Pre-existing issues:
- [High] The `lpi2c_imx_remove()` function leaks clock prepare and enable references when unbinding the driver.
- [High] NULL pointer dereference when `devm_clk_bulk_get_all()` returns 0.
- [High] Clock references are permanently leaked in the error path of `lpi2c_runtime_resume()`.
- [Medium] Unprotected hardware register access in the interrupt handler causes a bus fault on spurious interrupts.
- [Low] Unconditional `pm` assignment produces dead code when PM is disabled.
--

--- Patch [1]: [PATCH v6 1/2] i2c: imx-lpi2c: properly unwind resources on probe failure ---
commit 2175c9d5c422256763ff98e4c5aedc0fe0e29c4d
Author: Carlos Song <carlos.song@nxp.com>

i2c: imx-lpi2c: properly unwind resources on probe failure

The commit introduces a new error label to explicitly invoke
clk_bulk_disable_unprepare() on probe failure. It replaces
pm_runtime_put_sync() with pm_runtime_disable(), pm_runtime_set_suspended()
and pm_runtime_put_noidle() to bypass the runtime suspend callback during
error recovery.

> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index e6c24a9d934d7..615d9f7f11471 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -1527,14 +1527,18 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>  	 * each transfer
>  	 */
>  	ret = devm_clk_rate_exclusive_get(&pdev->dev, lpi2c_imx->clks[0].clk);

[Severity: High]
This is a pre-existing issue, but if devm_clk_bulk_get_all() returns 0,
lpi2c_imx->clks remains NULL. Does the array access lpi2c_imx->clks[0].clk
cause a NULL pointer dereference in that case?

> -	if (ret)
> -		return dev_err_probe(&pdev->dev, ret,
> -				     "can't lock I2C peripheral clock rate\n");
> +	if (ret) {
> +		ret = dev_err_probe(&pdev->dev, ret,
> +				    "can't lock I2C peripheral clock rate\n");
> +		goto clk_disable;
> +	}
>  
>  	lpi2c_imx->rate_per = clk_get_rate(lpi2c_imx->clks[0].clk);
> -	if (!lpi2c_imx->rate_per)
> -		return dev_err_probe(&pdev->dev, -EINVAL,
> -				     "can't get I2C peripheral clock rate\n");
> +	if (!lpi2c_imx->rate_per) {
> +		ret = dev_err_probe(&pdev->dev, -EINVAL,
> +				    "can't get I2C peripheral clock rate\n");
> +		goto clk_disable;
> +	}
>  
>  	if (lpi2c_imx->hwdata->need_prepare_unprepare_clk)
>  		pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_LONG_TIMEOUT_MS);
> @@ -1576,8 +1580,11 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>  
>  rpm_disable:
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
> -	pm_runtime_put_sync(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_set_suspended(&pdev->dev);
> +	pm_runtime_put_noidle(&pdev->dev);
> +clk_disable:
> +	clk_bulk_disable_unprepare(lpi2c_imx->num_clks, lpi2c_imx->clks);
>  
>  	return ret;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does lpi2c_imx_remove() safely
clean up the prepared clocks?

static void lpi2c_imx_remove(struct platform_device *pdev)
{
	struct lpi2c_imx_struct *lpi2c_imx = platform_get_drvdata(pdev);

	i2c_del_adapter(&lpi2c_imx->adapter);

	pm_runtime_disable(&pdev->dev);
	pm_runtime_dont_use_autosuspend(&pdev->dev);
}

Calling pm_runtime_disable() without a prior pm_runtime_get_sync() leaves
the device in its current state. If the device was active, the clocks could be
permanently leaked since lpi2c_runtime_suspend() won't execute, and there is
no fallback clk_bulk_disable_unprepare() call here.

[Severity: High]
This is a pre-existing issue, but could a devm_request_irq() failure in
lpi2c_runtime_resume() cause clock references to be leaked?

	if (need_request_free_irq) {
		ret = devm_request_irq(dev, lpi2c_imx->irq, lpi2c_imx_isr, IRQF_NO_SUSPEND,
				       dev_name(dev), lpi2c_imx);
		if (ret) {
			dev_err(dev, "can't claim irq %d\n", lpi2c_imx->irq);
			return ret;
		}
	}

If the IRQ request fails, it appears the function returns the error without
reverting the prior clk_bulk_prepare_enable() operations. Since the PM core
would mark the device as suspended on failure, would the clocks remain enabled
indefinitely?

[Severity: Medium]
This isn't a bug introduced by this patch, but is it possible for the
interrupt handler to access unclocked registers during a spurious interrupt?

static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id)
{
	struct lpi2c_imx_struct *lpi2c_imx = dev_id;

	if (lpi2c_imx->target) {
		u32 scr = readl(lpi2c_imx->base + LPI2C_SCR);
		u32 ssr = readl(lpi2c_imx->base + LPI2C_SSR);

The handler unconditionally reads hardware registers without verifying the
device PM status is active via pm_runtime_get_if_active(). Could this
trigger a bus fault if the device is currently suspended and its clocks are
disabled?

[Severity: Low]
This is a pre-existing issue, but can the PM callbacks become dead code
when power management is disabled?

static struct platform_driver lpi2c_imx_driver = {
	.probe = lpi2c_imx_probe,
	.remove = lpi2c_imx_remove,
	.driver = {
		.name = DRIVER_NAME,
		.of_match_table = lpi2c_imx_of_match,
		.pm = &lpi2c_pm_ops,
	},
};

The .pm assignment does not use the pm_ptr() wrapper, which may prevent the
compiler from safely discarding the PM callbacks when CONFIG_PM is not set.

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