Message-ID: <20250925142400.GA18572@nxa18884-linux.ap.freescale.net>
Date: Thu, 25 Sep 2025 22:24:00 +0800
From: Peng Fan <peng.fan@....nxp.com>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: Peng Fan <peng.fan@....com>, Bjorn Andersson <andersson@...nel.org>,
	Mathieu Poirier <mathieu.poirier@...aro.org>,
	Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	Hiago De Franco <hiago.franco@...adex.com>,
	linux-remoteproc@...r.kernel.org, imx@...ts.linux.dev,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Frank Li <Frank.Li@....com>, Daniel Baluta <daniel.baluta@....com>
Subject: Re: [PATCH v2 1/6] remoteproc: imx_rproc: Fix runtime PM cleanup
 order and error handling

Hi Ulf,

Thanks for reviewing this patch.

On Thu, Sep 25, 2025 at 12:18:39PM +0200, Ulf Hansson wrote:
>On Tue, 23 Sept 2025 at 07:17, Peng Fan <peng.fan@....com> wrote:
>>
>> The order of runtime PM API calls in the remove path is wrong.
>> pm_runtime_put() should be called before pm_runtime_disable(), per the
>> runtime PM guidelines. Calling pm_runtime_disable() prematurely can
>> lead to incorrect reference counting and improper device suspend behavior.
>
>This isn't entirely correct as it depends a bit more on the runtime PM
>deployment.
>
>More importantly, even if you would call pm_runtime_put() before the
>call to pm_runtime_disable() doesn't necessarily mean that the device
>becomes runtime suspended, as it can be prevented by user-space for
>example, assuming that is the goal.
>
>To make sure the device is put back into a low power-state, this is
>the typical pattern that is deployed in a driver's ->remove()
>callback.
>
>*) Call pm_runtime_get_sync(), to make sure the device gets the runtime resumed.
>Not needed in this case, as the runtime PM usage count was increased
>during ->probe() and not dropped).
>
>*) Turn off resources that correspond to what the runtime PM callbacks
>in the driver are managing.
>Not needed, as there are no runtime PM callbacks for the driver.
>
>*) Call pm_runtime_disable() and then pm_runtime_put_noidle(). This
>makes sure that when ->remove() is completed, the device is in a low
>power-state and the runtime PM usage count has been restored.
>
>*) If there are PM domains, those are turned off by calling
>dev_pm_domain_detach_list(), or from the driver core (after the
>->remove() callback has been completed) for the single PM domain case.
>
>That said, one could consider converting the pm_runtime_put() here
>into a pm_runtime_put_noidle(), to make it clear that this is only
>about restoring the usage count, but I don't think it's a big deal.
>
>>
>> Additionally, proper cleanup should be done when rproc_add() fails by
>> invoking both pm_runtime_put() and pm_runtime_disable() to avoid leaving
>> the device in an inconsistent power state.
>
>Right, this deserved to be fixed.
>
>>
>> With using devm_pm_runtime_enable() for automatic resource management and
>> introducing a devres-managed cleanup action imx_rproc_pm_runtime_put() to
>> enforce correct PM API usage and simplify error paths, the upper two
>> issues could be fixed. Also print out error log in case of error.
>
>I really don't want to encourage people to use
>devm_pm_runtime_enable(), simply because it's not always a good fit
>when making sure things get turned off in the correct sequence. In
>particular, as it's just about saving one/two lines of code, this
>doesn't make sense to me.
>
>I suggest you follow the similar pattern as I explained above for
>->remove(), for the error path in ->probe() too. So, calling
>pm_runtime_disable() and pm_runtime_put_noidle() should do the trick
>for this too, I think.

I appreciate for your detailed explaination. I intended to drop the remove path
in this patchset :), but need to keep it now. No problem.

Follow your suggestion, I work out one patch, would you please give a look
whether this is good for you?

You could ignore the 'dcfg->method == IMX_RPROC_SCU_API', I will drop
this in the patchset to make the runtime PM apply for all, not just
IMX_RPROC_SCU_API.

Thanks in advance for you guidance and help.

--------------------------------------------------------------------
remoteproc: imx_rproc: Fix runtime PM cleanup and improve remove path

Proper cleanup should be done when rproc_add() fails by invoking both
pm_runtime_disable() and pm_runtime_put_noidle() to avoid leaving the
device in an inconsistent power state.

Fix it by adding pm_runtime_put_noidle() and pm_runtime_disable()
in the error path.

Also Update the remove() callback to use pm_runtime_put_noidle() instead of
pm_runtime_put(), to clearly indicate that only need to restore the usage
count.

Signed-off-by: Peng Fan <peng.fan@....com>
---
 drivers/remoteproc/imx_rproc.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index bb25221a4a89..8424e6ea5569 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -1136,11 +1136,16 @@ static int imx_rproc_probe(struct platform_device *pdev)
 	ret = rproc_add(rproc);
 	if (ret) {
 		dev_err(dev, "rproc_add failed\n");
-		goto err_put_clk;
+		goto err_put_pm;
 	}
 
 	return 0;
 
+err_put_pm:
+	if (dcfg->method == IMX_RPROC_SCU_API) {
+		pm_runtime_disable(dev);
+		pm_runtime_put_noidle(dev);
+	}
 err_put_clk:
 	clk_disable_unprepare(priv->clk);
 err_put_scu:
@@ -1160,7 +1165,7 @@ static void imx_rproc_remove(struct platform_device *pdev)
 
 	if (priv->dcfg->method == IMX_RPROC_SCU_API) {
 		pm_runtime_disable(priv->dev);
-		pm_runtime_put(priv->dev);
+		pm_runtime_put_noidle(priv->dev);
 	}
 	clk_disable_unprepare(priv->clk);
 	rproc_del(rproc);


Thanks,
Peng

Powered by blists - more mailing lists