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 <[email protected]> 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 <[email protected]> --- 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
