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

[PATCH v4 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, and pm_runtime_put_sync() may
   trigger the runtime suspend callback before resources are fully
   set up.

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.

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 | 52 +++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 16 deletions(-)

-- 
2.43.0

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

[carlos.song]

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

When probe fails after clk_bulk_prepare_enable() succeeds but before
runtime PM is initialized, the enabled clocks are never disabled.
Additionally, calling pm_runtime_put_sync() in the error path can
trigger the runtime suspend callback, which may attempt to disable
clocks that have not been fully set up, leading to potential issues
during error unwinding.

Introduce two new error labels: clk_disable to explicitly invoke
clk_bulk_disable_unprepare(), and free_irq to release the IRQ via
devm_free_irq(). 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. Update all goto targets so that each failure site
releases only the resources acquired up to that point.

Signed-off-by: Carlos Song <carlos.song@nxp.com>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index cd4da50c4dd9..fbb9c0b0a99c 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -1520,21 +1520,25 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 
 	ret = clk_bulk_prepare_enable(lpi2c_imx->num_clks, lpi2c_imx->clks);
 	if (ret)
-		return ret;
+		goto free_irq;
 
 	/*
 	 * Lock the parent clock rate to avoid getting parent clock upon
 	 * 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) {
+		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,13 @@ 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);
+free_irq:
+	devm_free_irq(&pdev->dev, lpi2c_imx->irq, lpi2c_imx);
 
 	return ret;
 }
-- 
2.43.0

Re: [PATCH v4 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] DMA channels allocated during probe are never released on probe failure or device removal.
- [High] The driver requests its IRQ before enabling peripheral clocks and initializing waitqueues, creating a race condition that can crash the system.
- [High] Clock prepare and enable counts are permanently leaked on driver unload.
- [High] The interrupt handler accesses hardware registers without verifying the device's PM runtime state.
- [High] The atomic transfer callback `lpi2c_imx_xfer_atomic` calls a sleeping Runtime PM function, violating atomic context constraints.
--

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

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

When probe fails after clk_bulk_prepare_enable() succeeds but before
runtime PM is initialized, the enabled clocks are never disabled.
Additionally, calling pm_runtime_put_sync() in the error path can
trigger the runtime suspend callback, which may attempt to disable
clocks that have not been fully set up, leading to potential issues
during error unwinding.

> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index a01c236964819..540dc6cb664eb 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
[ ... ]
> @@ -1565,8 +1569,13 @@ 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);
> +free_irq:
> +	devm_free_irq(&pdev->dev, lpi2c_imx->irq, lpi2c_imx);

[Severity: High]
This is a pre-existing issue, but does explicitly placing clk_disable before
free_irq expose a window where an interrupt can arrive and cause a fault
during cleanup?

If a pending interrupt fires after clocks are disabled but before the interrupt
is freed, lpi2c_imx_isr() in drivers/i2c/busses/i2c-imx-lpi2c.c will execute
and attempt to read hardware registers while clocks are disabled.

[Severity: High]
This isn't a regression introduced by this patch, but are dma channels leaked
if probe fails after lpi2c_dma_init() succeeds?

The driver requests channels using dma_request_chan() in lpi2c_dma_init().
If a later probe step fails (such as i2c_add_adapter()), the error unwinding
path skips releasing the channels.

Additionally, lpi2c_imx_remove() in drivers/i2c/busses/i2c-imx-lpi2c.c appears
to lack cleanup, which could permanently leak two channels every time the
driver unbinds.

[Severity: High]
This is a pre-existing issue, but are clock prepare and enable counts retained
on driver unload?

Because lpi2c_imx_remove() in drivers/i2c/busses/i2c-imx-lpi2c.c only calls
pm_runtime_disable() without explicit clock unprepare or disable operations,
the actual clock counts might never be decremented. Devres will drop the clock
handles, but the clocks could remain enabled preventing deep sleep states.

[Severity: High]
This is a pre-existing issue, but could the interrupt handler access hardware
registers while the device is in a suspended state?

According to subsystem guidelines, interrupt handlers must use
pm_runtime_get_if_active() before accessing registers. Because the interrupt
is not explicitly disabled at the controller during runtime suspend, a spurious
or delayed interrupt might fire in lpi2c_imx_isr() while clocks are gated.

[Severity: High]
This isn't a regression introduced by this patch, but is it possible for the
atomic transfer callback to sleep?

lpi2c_imx_xfer_atomic() in drivers/i2c/busses/i2c-imx-lpi2c.c delegates to
lpi2c_imx_xfer_common(), which then calls lpi2c_imx_master_enable(). This path
executes pm_runtime_resume_and_get(). If the device is suspended, the power
management runtime core will attempt to acquire a mutex and sleep in an atomic
context where interrupts are disabled.

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

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

[Frank Li]

On Tue, Jun 09, 2026 at 05:51:18PM +0800, carlos.song@oss.nxp.com wrote:
> [You don't often get email from carlos.song@oss.nxp.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> From: Carlos Song <carlos.song@nxp.com>
>
> When probe fails after clk_bulk_prepare_enable() succeeds but before
> runtime PM is initialized, the enabled clocks are never disabled.
> Additionally, calling pm_runtime_put_sync() in the error path can
> trigger the runtime suspend callback, which may attempt to disable
> clocks that have not been fully set up, leading to potential issues
> during error unwinding.
>
> Introduce two new error labels: clk_disable to explicitly invoke
> clk_bulk_disable_unprepare(), and free_irq to release the IRQ via
> devm_free_irq(). 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. Update all goto targets so that each failure site
> releases only the resources acquired up to that point.
>
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
> ---
>  drivers/i2c/busses/i2c-imx-lpi2c.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index cd4da50c4dd9..fbb9c0b0a99c 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -1520,21 +1520,25 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>
>         ret = clk_bulk_prepare_enable(lpi2c_imx->num_clks, lpi2c_imx->clks);
>         if (ret)
> -               return ret;
> +               goto free_irq;

If you use runtime pm, you should not manually manange clock again.

generally method is

	devm_clk_get()
	devm_runtime_pm_enable()

	call runtime_pm_get_sync(), \\there are PM AQUIRE help macro to help
elimiate goto branch.
	... // if need clock enable to do some works.
	call runtime_pm_put()

	...

and needn't call devm_free_irq().

Frank


>
>         /*
>          * Lock the parent clock rate to avoid getting parent clock upon
>          * 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) {
> +               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,13 @@ 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);
> +free_irq:
> +       devm_free_irq(&pdev->dev, lpi2c_imx->irq, lpi2c_imx);
>
>         return ret;
>  }
> --
> 2.43.0
>
>

[PATCH v4 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>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 33 ++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index fbb9c0b0a99c..93f263bf27b2 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -1510,17 +1510,12 @@ 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);
 
 	ret = clk_bulk_prepare_enable(lpi2c_imx->num_clks, lpi2c_imx->clks);
 	if (ret)
-		goto free_irq;
+		return dev_err_probe(&pdev->dev, ret, "can't get LPI2C clock\n");
 
 	/*
 	 * Lock the parent clock rate to avoid getting parent clock upon
@@ -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);
@@ -1558,19 +1569,19 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 	ret = lpi2c_imx_init_recovery_info(lpi2c_imx, pdev);
 	/* Give it another chance if pinctrl used is not ready yet */
 	if (ret == -EPROBE_DEFER)
-		goto rpm_disable;
+		goto free_irq;
 
 	/* Init DMA */
 	ret = lpi2c_dma_init(&pdev->dev, phy_addr);
 	if (ret) {
 		if (ret == -EPROBE_DEFER)
-			goto rpm_disable;
+			goto free_irq;
 		dev_info(&pdev->dev, "use pio mode\n");
 	}
 
 	ret = i2c_add_adapter(&lpi2c_imx->adapter);
 	if (ret)
-		goto rpm_disable;
+		goto free_irq;
 
 	pm_runtime_put_autosuspend(&pdev->dev);
 
@@ -1578,6 +1589,8 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 
 	return 0;
 
+free_irq:
+	devm_free_irq(&pdev->dev, lpi2c_imx->irq, lpi2c_imx);
 rpm_disable:
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
@@ -1585,8 +1598,6 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 	pm_runtime_put_noidle(&pdev->dev);
 clk_disable:
 	clk_bulk_disable_unprepare(lpi2c_imx->num_clks, lpi2c_imx->clks);
-free_irq:
-	devm_free_irq(&pdev->dev, lpi2c_imx->irq, lpi2c_imx);
 
 	return ret;
 }
-- 
2.43.0

Re: [PATCH v4 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:

Pre-existing issues:
- [High] DMA channels requested via `dma_request_chan()` are never released, leading to a permanent resource leak upon driver unbind or probe failure.
- [High] Device removal cancels pending async suspend without disabling hardware clocks, leaving clocks permanently enabled.
--

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

i2c: imx-lpi2c: reset controller in probe stage

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

> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index 540dc6cb664eb..ccc7384323e03 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
[ ... ]
> @@ -1547,19 +1558,19 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>  	ret = lpi2c_imx_init_recovery_info(lpi2c_imx, pdev);
>  	/* Give it another chance if pinctrl used is not ready yet */
>  	if (ret == -EPROBE_DEFER)
> -		goto rpm_disable;
> +		goto free_irq;
>  
>  	/* Init DMA */
>  	ret = lpi2c_dma_init(&pdev->dev, phy_addr);
>  	if (ret) {
>  		if (ret == -EPROBE_DEFER)
> -			goto rpm_disable;
> +			goto free_irq;
>  		dev_info(&pdev->dev, "use pio mode\n");
>  	}
>  
>  	ret = i2c_add_adapter(&lpi2c_imx->adapter);
>  	if (ret)
> -		goto rpm_disable;
> +		goto free_irq;

[Severity: High]
This is a pre-existing issue, but does this error path permanently leak
the DMA channels?

If i2c_add_adapter() fails, we jump to the free_irq label, but nothing
releases the TX and RX channels allocated via dma_request_chan() in
lpi2c_dma_init().

It looks like lpi2c_imx_remove() also entirely omits dma_release_channel()
cleanup during driver unbind:

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);
}

Will this eventually exhaust the system's DMA channels after repeated probe
failures or driver unbinds?

>  
>  	pm_runtime_put_autosuspend(&pdev->dev);

[Severity: High]
This is also a pre-existing issue, but does lpi2c_imx_remove() leave
hardware clocks permanently enabled if the device is active when unbound?

When lpi2c_imx_remove() calls pm_runtime_disable(), it synchronously cancels
any pending asynchronous idle/suspend requests (like the autosuspend timer
started here at the end of probe), but leaves the device in its current
power state.

If the device is in RPM_ACTIVE during unbind, the clocks remain enabled. Is
a manual check or a call to pm_runtime_put_sync() needed before disabling
PM runtime to avoid a silent resource leak?

>  
> @@ -1567,6 +1578,8 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> +free_irq:
> +	devm_free_irq(&pdev->dev, lpi2c_imx->irq, lpi2c_imx);
>  rpm_disable:
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);

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