[PATCH v3] i2c: xiic: restore non-managed runtime PM to fix clk WARN flood

[PATCH v3] i2c: xiic: restore non-managed runtime PM to fix clk WARN flood

[Abdurrahman Hussain]

The devres conversion replaced manual pm_runtime_enable()/disable() with
devm_pm_runtime_set_active_enabled() and dropped the remove-time runtime
PM teardown. The managed release tears runtime PM down in the wrong
order: it calls pm_runtime_dont_use_autosuspend() before
pm_runtime_disable(), i.e. while runtime PM is still enabled, and devres
is LIFO so the devm_clk_get_enabled() release runs afterwards.

At remove(), pm_runtime_put_sync() leaves the device active with the
autosuspend timer armed. Clearing use_autosuspend then makes rpm_idle()
suspend immediately, and xiic_i2c_runtime_suspend() clk_disable()s the
clock. The later devm_clk_get_enabled() release clk_disable_unprepare()s
the already-disabled clock, so clk_core_disable() WARNs ("clkN already
disabled") on every teardown.

Drop the managed helper and restore the non-managed runtime PM setup and
teardown, so runtime PM is enabled once in probe and disabled once in
remove and the clock enable count stays balanced.

Fixes: 50c63491ff26 ("i2c: xiic: switch to devres managed APIs")
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
Changes in v3:
- Drop the managed devm_pm_runtime_set_active_enabled() helper entirely
  and restore the non-managed runtime PM setup (pm_runtime_set_active +
  pm_runtime_enable, with probe error unwinding) and teardown
  (pm_runtime_disable + set_suspended + dont_use_autosuspend), so the
  disable depth stays balanced rather than being disabled twice (Andi).
- Link to v2: https://patch.msgid.link/20260814-i2c-xiic-restore-runtime-pm-teardown-v2-1-7ae5d0c30ff2@nexthop.ai

Changes in v2:
- Move the Signed-off-by into the commit message proper (Andy).
- Link to v1: https://patch.msgid.link/20260813-i2c-xiic-restore-runtime-pm-teardown-v1-1-0e7dfb206790@nexthop.ai

To: Michal Simek <michal.simek@amd.com>
To: Andi Shyti <andi.shyti@kernel.org>
To: Abdurrahman Hussain <abdurrahman@nexthop.ai>
To: Andy Shevchenko <andriy.shevchenko@intel.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-i2c@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/i2c/busses/i2c-xiic.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 3e7735e1dae0..85f3c322b1a2 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -1475,9 +1475,13 @@ static int xiic_i2c_probe(struct platform_device *pdev)
 
 	pm_runtime_set_autosuspend_delay(dev, XIIC_PM_TIMEOUT);
 	pm_runtime_use_autosuspend(dev);
-	ret = devm_pm_runtime_set_active_enabled(dev);
-	if (ret)
-		return ret;
+	/*
+	 * Enable runtime PM by hand: devm_pm_runtime_set_active_enabled()
+	 * tears down in an order that races the devm-enabled clock release and
+	 * makes clk_core_disable() WARN (see xiic_i2c_remove()).
+	 */
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
 
 	/* SCL frequency configuration */
 	i2c->input_clk = clk_get_rate(i2c->clk);
@@ -1489,7 +1493,7 @@ static int xiic_i2c_probe(struct platform_device *pdev)
 	ret = devm_request_threaded_irq(dev, irq, NULL, xiic_process,
 					IRQF_ONESHOT, pdev->name, i2c);
 	if (ret)
-		return ret;
+		goto err_pm_disable;
 
 	i2c->singlemaster = device_property_read_bool(dev, "single-master");
 
@@ -1506,14 +1510,16 @@ static int xiic_i2c_probe(struct platform_device *pdev)
 		i2c->endianness = BIG;
 
 	ret = xiic_reinit(i2c);
-	if (ret)
-		return dev_err_probe(dev, ret, "Cannot xiic_reinit\n");
+	if (ret) {
+		dev_err_probe(dev, ret, "Cannot xiic_reinit\n");
+		goto err_pm_disable;
+	}
 
 	/* add i2c adapter to i2c tree */
 	ret = i2c_add_numbered_adapter(&i2c->adap);
 	if (ret) {
 		xiic_deinit(i2c);
-		return ret;
+		goto err_pm_disable;
 	}
 
 	if (pdata) {
@@ -1526,6 +1532,12 @@ static int xiic_i2c_probe(struct platform_device *pdev)
 		res, irq, i2c->i2c_clk);
 
 	return 0;
+
+err_pm_disable:
+	pm_runtime_disable(dev);
+	pm_runtime_set_suspended(dev);
+
+	return ret;
 }
 
 static void xiic_i2c_remove(struct platform_device *pdev)
@@ -1545,6 +1557,9 @@ static void xiic_i2c_remove(struct platform_device *pdev)
 		xiic_deinit(i2c);
 
 	pm_runtime_put_sync(dev);
+	pm_runtime_disable(dev);
+	pm_runtime_set_suspended(dev);
+	pm_runtime_dont_use_autosuspend(dev);
 }
 
 static const struct dev_pm_ops xiic_dev_pm_ops = {

---
base-commit: d58772d8520c7ef247c4b95c9bd76d3a25da9ff5
change-id: 20260813-i2c-xiic-restore-runtime-pm-teardown-dd0ab1db2c02

Best regards,
--  
Abdurrahman Hussain <abdurrahman@nexthop.ai>

Re: [PATCH v3] i2c: xiic: restore non-managed runtime PM to fix clk WARN flood

[Andy Shevchenko]

On Tue, Aug 18, 2026 at 08:35:10AM -0700, Abdurrahman Hussain wrote:
> The devres conversion replaced manual pm_runtime_enable()/disable() with
> devm_pm_runtime_set_active_enabled() and dropped the remove-time runtime
> PM teardown. The managed release tears runtime PM down in the wrong
> order: it calls pm_runtime_dont_use_autosuspend() before
> pm_runtime_disable(), i.e. while runtime PM is still enabled, and devres
> is LIFO so the devm_clk_get_enabled() release runs afterwards.
> 
> At remove(), pm_runtime_put_sync() leaves the device active with the
> autosuspend timer armed. Clearing use_autosuspend then makes rpm_idle()
> suspend immediately, and xiic_i2c_runtime_suspend() clk_disable()s the
> clock. The later devm_clk_get_enabled() release clk_disable_unprepare()s
> the already-disabled clock, so clk_core_disable() WARNs ("clkN already
> disabled") on every teardown.
> 
> Drop the managed helper and restore the non-managed runtime PM setup and
> teardown, so runtime PM is enabled once in probe and disabled once in
> remove and the clock enable count stays balanced.

...

>  	pm_runtime_set_autosuspend_delay(dev, XIIC_PM_TIMEOUT);
>  	pm_runtime_use_autosuspend(dev);
> -	ret = devm_pm_runtime_set_active_enabled(dev);
> -	if (ret)
> -		return ret;
> +	/*
> +	 * Enable runtime PM by hand: devm_pm_runtime_set_active_enabled()
> +	 * tears down in an order that races the devm-enabled clock release and
> +	 * makes clk_core_disable() WARN (see xiic_i2c_remove()).
> +	 */
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
>  
>  	/* SCL frequency configuration */
>  	i2c->input_clk = clk_get_rate(i2c->clk);

>  	ret = devm_request_threaded_irq(dev, irq, NULL, xiic_process,
>  					IRQF_ONESHOT, pdev->name, i2c);
>  	if (ret)
> -		return ret;
> +		goto err_pm_disable;

This might be problematic now. You need to unwind the IRQ request in non-devm
manner as well. Scenario is that IRQ comes exactly after PM is disabled
in the error path. Is it a problem today? What about tomorrow (assuming some
new chips / code is added)?

The rule of thumb is that, no devm_*() call should be followed by a goto.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] i2c: xiic: restore non-managed runtime PM to fix clk WARN flood

[Andy Shevchenko]

On Wed, Aug 19, 2026 at 09:45:41AM +0300, Andy Shevchenko wrote:
> On Tue, Aug 18, 2026 at 08:35:10AM -0700, Abdurrahman Hussain wrote:

...

> >  	pm_runtime_set_autosuspend_delay(dev, XIIC_PM_TIMEOUT);
> >  	pm_runtime_use_autosuspend(dev);
> > -	ret = devm_pm_runtime_set_active_enabled(dev);
> > -	if (ret)
> > -		return ret;
> > +	/*
> > +	 * Enable runtime PM by hand: devm_pm_runtime_set_active_enabled()
> > +	 * tears down in an order that races the devm-enabled clock release and
> > +	 * makes clk_core_disable() WARN (see xiic_i2c_remove()).
> > +	 */
> > +	pm_runtime_set_active(dev);
> > +	pm_runtime_enable(dev);
> >  
> >  	/* SCL frequency configuration */
> >  	i2c->input_clk = clk_get_rate(i2c->clk);
> 
> >  	ret = devm_request_threaded_irq(dev, irq, NULL, xiic_process,
> >  					IRQF_ONESHOT, pdev->name, i2c);
> >  	if (ret)
> > -		return ret;
> > +		goto err_pm_disable;
> 
> This might be problematic now. You need to unwind the IRQ request in non-devm
> manner as well. Scenario is that IRQ comes exactly after PM is disabled
> in the error path. Is it a problem today? What about tomorrow (assuming some
> new chips / code is added)?

(To be clear: I'm talking about next call(s) that may fail, like xiic_reinit()
 later in the probe.)

> The rule of thumb is that, no devm_*() call should be followed by a goto.

-- 
With Best Regards,
Andy Shevchenko