On Thu, 25 Sept 2025 at 15:12, Peng Fan <[email protected]> wrote: > > 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
Yes, this makes better sense to me! Kind regards Uffe
