[PATCH v2 0/6] remoteproc: imx_rproc: Use device managed API to clean up the driver

[PATCH v2 0/6] remoteproc: imx_rproc: Use device managed API to clean up the driver

[Peng Fan]

This is the 2nd series to cleanup the driver.

Patch 1:
Fix the runtime usage. This is not critical bug fix, so it could be
defered to 6.18.

Patch 2-6:
Use devres managed API to cleanup the error handling path and remove path.

Tested on
i.MX8MP-EVK, i.MX8MM-EVK, i.MX93-11x11-EVK, i.MX8QXP-MEK, and i.MX8ULP-EVK.

There is still a 3rd patchset to do further cleanup. After this patchset
get reviewed, the 3rd patchset will be posted out to list.

Thanks to Daniel and Frank for the internal reviewing.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Changes in v2:
- Address a build warning in patch 4/6
- Add R-b from Frank and Daniel
- Link to v1: https://lore.kernel.org/r/20250917-imx_rproc_c2-v1-0-00ce23dc9c6e@nxp.com

---
Peng Fan (6):
      remoteproc: imx_rproc: Fix runtime PM cleanup order and error handling
      remoteproc: imx_rproc: Use devm_add_action_or_reset() for workqueue cleanup
      remoteproc: imx_rproc: Use devm_add_action_or_reset() for mailbox cleanup
      remoteproc: imx_rproc: Use devm_clk_get_enabled() and simplify cleanup
      remoteproc: imx_rproc: Use devm_add_action_or_reset() for scu cleanup
      remoteproc: imx_rproc: Use devm_rproc_add() helper

 drivers/remoteproc/imx_rproc.c | 128 ++++++++++++++++++-----------------------
 1 file changed, 57 insertions(+), 71 deletions(-)
---
base-commit: c3067c2c38316c3ef013636c93daa285ee6aaa2e
change-id: 20250916-imx_rproc_c2-2b9ad7882f4d

Best regards,
-- 
Peng Fan <peng.fan@nxp.com>

[PATCH v2 1/6] remoteproc: imx_rproc: Fix runtime PM cleanup order and error handling

[Peng Fan]

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.

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.

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.

Fixes: a876a3aacc43 ("remoteproc: imx_rproc: detect and attach to pre-booted remote cores")
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Hiago De Franco <hiago.franco@toradex.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Reviewed-by: Daniel Baluta <daniel.baluta@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index bb25221a4a8987ff427d68e2a5535f0e156b0097..12305f36552fb5265b0953a099ea0d561880e3ff 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -1046,6 +1046,13 @@ static int imx_rproc_sys_off_handler(struct sys_off_data *data)
 	return NOTIFY_DONE;
 }
 
+static void imx_rproc_pm_runtime_put(void *data)
+{
+	struct device *dev = data;
+
+	pm_runtime_put(dev);
+}
+
 static int imx_rproc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1125,12 +1132,23 @@ static int imx_rproc_probe(struct platform_device *pdev)
 	}
 
 	if (dcfg->method == IMX_RPROC_SCU_API) {
-		pm_runtime_enable(dev);
+		ret = devm_pm_runtime_enable(dev);
+		if (ret) {
+			dev_err(dev, "Failed to enable runtime PM, %d\n", ret);
+			goto err_put_clk;
+		}
+
 		ret = pm_runtime_resume_and_get(dev);
 		if (ret) {
 			dev_err(dev, "pm_runtime get failed: %d\n", ret);
 			goto err_put_clk;
 		}
+
+		ret = devm_add_action_or_reset(dev, imx_rproc_pm_runtime_put, dev);
+		if (ret) {
+			dev_err(dev, "Failed to add devm disable pm action: %d\n", ret);
+			goto err_put_clk;
+		}
 	}
 
 	ret = rproc_add(rproc);
@@ -1158,10 +1176,6 @@ static void imx_rproc_remove(struct platform_device *pdev)
 	struct rproc *rproc = platform_get_drvdata(pdev);
 	struct imx_rproc *priv = rproc->priv;
 
-	if (priv->dcfg->method == IMX_RPROC_SCU_API) {
-		pm_runtime_disable(priv->dev);
-		pm_runtime_put(priv->dev);
-	}
 	clk_disable_unprepare(priv->clk);
 	rproc_del(rproc);
 	imx_rproc_put_scu(rproc);

-- 
2.37.1

Re: [PATCH v2 1/6] remoteproc: imx_rproc: Fix runtime PM cleanup order and error handling

[Ulf Hansson]

On Tue, 23 Sept 2025 at 07:17, Peng Fan <peng.fan@nxp.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.

>
> Fixes: a876a3aacc43 ("remoteproc: imx_rproc: detect and attach to pre-booted remote cores")
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Hiago De Franco <hiago.franco@toradex.com>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> Reviewed-by: Daniel Baluta <daniel.baluta@nxp.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>

Kind regards
Uffe

> ---
>  drivers/remoteproc/imx_rproc.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index bb25221a4a8987ff427d68e2a5535f0e156b0097..12305f36552fb5265b0953a099ea0d561880e3ff 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -1046,6 +1046,13 @@ static int imx_rproc_sys_off_handler(struct sys_off_data *data)
>         return NOTIFY_DONE;
>  }
>
> +static void imx_rproc_pm_runtime_put(void *data)
> +{
> +       struct device *dev = data;
> +
> +       pm_runtime_put(dev);
> +}
> +
>  static int imx_rproc_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
> @@ -1125,12 +1132,23 @@ static int imx_rproc_probe(struct platform_device *pdev)
>         }
>
>         if (dcfg->method == IMX_RPROC_SCU_API) {
> -               pm_runtime_enable(dev);
> +               ret = devm_pm_runtime_enable(dev);
> +               if (ret) {
> +                       dev_err(dev, "Failed to enable runtime PM, %d\n", ret);
> +                       goto err_put_clk;
> +               }
> +
>                 ret = pm_runtime_resume_and_get(dev);
>                 if (ret) {
>                         dev_err(dev, "pm_runtime get failed: %d\n", ret);
>                         goto err_put_clk;
>                 }
> +
> +               ret = devm_add_action_or_reset(dev, imx_rproc_pm_runtime_put, dev);
> +               if (ret) {
> +                       dev_err(dev, "Failed to add devm disable pm action: %d\n", ret);
> +                       goto err_put_clk;
> +               }
>         }
>
>         ret = rproc_add(rproc);
> @@ -1158,10 +1176,6 @@ static void imx_rproc_remove(struct platform_device *pdev)
>         struct rproc *rproc = platform_get_drvdata(pdev);
>         struct imx_rproc *priv = rproc->priv;
>
> -       if (priv->dcfg->method == IMX_RPROC_SCU_API) {
> -               pm_runtime_disable(priv->dev);
> -               pm_runtime_put(priv->dev);
> -       }
>         clk_disable_unprepare(priv->clk);
>         rproc_del(rproc);
>         imx_rproc_put_scu(rproc);
>
> --
> 2.37.1
>

Re: [PATCH v2 1/6] remoteproc: imx_rproc: Fix runtime PM cleanup order and error handling

[Peng Fan]

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@nxp.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@nxp.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

Re: [PATCH v2 1/6] remoteproc: imx_rproc: Fix runtime PM cleanup order and error handling

[Ulf Hansson]

On Thu, 25 Sept 2025 at 15:12, Peng Fan <peng.fan@oss.nxp.com> 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 <peng.fan@nxp.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@nxp.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

Yes, this makes better sense to me!

Kind regards
Uffe

[PATCH v2 2/6] remoteproc: imx_rproc: Use devm_add_action_or_reset() for workqueue cleanup

[Peng Fan]

Replace manual destroy_workqueue() calls in error and remove paths with a
devm_add_action_or_reset() helper. Ensure the workqueue is properly
cleaned up with the device lifecycle, and simplify error handling in probe
by removing now-unnecessary labels and cleanup steps.

No functional changes.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Reviewed-by: Daniel Baluta <daniel.baluta@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 12305f36552fb5265b0953a099ea0d561880e3ff..cc776f5d75f1f614943c05250877f17537837068 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -1046,6 +1046,13 @@ static int imx_rproc_sys_off_handler(struct sys_off_data *data)
 	return NOTIFY_DONE;
 }
 
+static void imx_rproc_destroy_workqueue(void *data)
+{
+	struct workqueue_struct *workqueue = data;
+
+	destroy_workqueue(workqueue);
+}
+
 static void imx_rproc_pm_runtime_put(void *data)
 {
 	struct device *dev = data;
@@ -1084,11 +1091,15 @@ static int imx_rproc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	ret = devm_add_action_or_reset(dev, imx_rproc_destroy_workqueue, priv->workqueue);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to add devm destroy workqueue action\n");
+
 	INIT_WORK(&priv->rproc_work, imx_rproc_vq_work);
 
 	ret = imx_rproc_xtr_mbox_init(rproc, true);
 	if (ret)
-		goto err_put_wkq;
+		return ret;
 
 	ret = imx_rproc_addr_init(priv, pdev);
 	if (ret) {
@@ -1165,8 +1176,6 @@ static int imx_rproc_probe(struct platform_device *pdev)
 	imx_rproc_put_scu(rproc);
 err_put_mbox:
 	imx_rproc_free_mbox(rproc);
-err_put_wkq:
-	destroy_workqueue(priv->workqueue);
 
 	return ret;
 }
@@ -1180,7 +1189,6 @@ static void imx_rproc_remove(struct platform_device *pdev)
 	rproc_del(rproc);
 	imx_rproc_put_scu(rproc);
 	imx_rproc_free_mbox(rproc);
-	destroy_workqueue(priv->workqueue);
 }
 
 static const struct imx_rproc_plat_ops imx_rproc_ops_arm_smc = {

-- 
2.37.1

[PATCH v2 3/6] remoteproc: imx_rproc: Use devm_add_action_or_reset() for mailbox cleanup

[Peng Fan]

Convert imx_rproc_free_mbox() to a devm-managed cleanup action using
devm_add_action_or_reset(). Ensure the mailbox resources are freed
automatically with the device lifecycle, simplify error handling and
removing the need for manual cleanup in probe and remove paths.

Also improve error reporting by using dev_err_probe() for consistency and
clarity.

No functional changes.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Reviewed-by: Daniel Baluta <daniel.baluta@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index cc776f5d75f1f614943c05250877f17537837068..e30b61ee39dacc88f9e938f8c6ffe61fef63dbda 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -93,7 +93,7 @@ struct imx_rproc_mem {
 #define ATT_CORE(I)     BIT((I))
 
 static int imx_rproc_xtr_mbox_init(struct rproc *rproc, bool tx_block);
-static void imx_rproc_free_mbox(struct rproc *rproc);
+static void imx_rproc_free_mbox(void *data);
 
 struct imx_rproc {
 	struct device			*dev;
@@ -780,8 +780,9 @@ static int imx_rproc_xtr_mbox_init(struct rproc *rproc, bool tx_block)
 	return 0;
 }
 
-static void imx_rproc_free_mbox(struct rproc *rproc)
+static void imx_rproc_free_mbox(void *data)
 {
+	struct rproc *rproc = data;
 	struct imx_rproc *priv = rproc->priv;
 
 	if (priv->tx_ch) {
@@ -1101,15 +1102,18 @@ static int imx_rproc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = devm_add_action_or_reset(dev, imx_rproc_free_mbox, rproc);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to add devm free mbox action: %d\n", ret);
+
 	ret = imx_rproc_addr_init(priv, pdev);
-	if (ret) {
-		dev_err(dev, "failed on imx_rproc_addr_init\n");
-		goto err_put_mbox;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "failed on imx_rproc_addr_init\n");
 
 	ret = imx_rproc_detect_mode(priv);
 	if (ret)
-		goto err_put_mbox;
+		return dev_err_probe(dev, ret, "failed on detect mode\n");
 
 	ret = imx_rproc_clk_enable(priv);
 	if (ret)
@@ -1174,8 +1178,6 @@ static int imx_rproc_probe(struct platform_device *pdev)
 	clk_disable_unprepare(priv->clk);
 err_put_scu:
 	imx_rproc_put_scu(rproc);
-err_put_mbox:
-	imx_rproc_free_mbox(rproc);
 
 	return ret;
 }
@@ -1188,7 +1190,6 @@ static void imx_rproc_remove(struct platform_device *pdev)
 	clk_disable_unprepare(priv->clk);
 	rproc_del(rproc);
 	imx_rproc_put_scu(rproc);
-	imx_rproc_free_mbox(rproc);
 }
 
 static const struct imx_rproc_plat_ops imx_rproc_ops_arm_smc = {

-- 
2.37.1

[PATCH v2 4/6] remoteproc: imx_rproc: Use devm_clk_get_enabled() and simplify cleanup

[Peng Fan]

Replace separate calls to devm_clk_get() and clk_prepare_enable() with
devm_clk_get_enabled(), which combines clock acquisition and enabling
into a single managed step. Simplify the probe logic and remove the need
for manual clock disable in error and remove paths.

Also, update error handling to eliminate redundant cleanup steps and use
return-based error propagation where appropriate. Improve code clarity and
reduce the chance of resource leaks or incorrect ordering in cleanup paths.

No functional changes.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Reviewed-by: Daniel Baluta <daniel.baluta@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index e30b61ee39dacc88f9e938f8c6ffe61fef63dbda..42014b42bd8d3b46b1a898969f0419e2e6a71528 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -1006,26 +1006,19 @@ static int imx_rproc_clk_enable(struct imx_rproc *priv)
 {
 	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
 	struct device *dev = priv->dev;
-	int ret;
 
 	/* Remote core is not under control of Linux or it is managed by SCU API */
 	if (dcfg->method == IMX_RPROC_NONE || dcfg->method == IMX_RPROC_SCU_API)
 		return 0;
 
-	priv->clk = devm_clk_get(dev, NULL);
-	if (IS_ERR(priv->clk)) {
-		dev_err(dev, "Failed to get clock\n");
-		return PTR_ERR(priv->clk);
-	}
-
 	/*
 	 * clk for M4 block including memory. Should be
 	 * enabled before .start for FW transfer.
 	 */
-	ret = clk_prepare_enable(priv->clk);
-	if (ret) {
+	priv->clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(priv->clk)) {
 		dev_err(dev, "Failed to enable clock\n");
-		return ret;
+		return PTR_ERR(priv->clk);
 	}
 
 	return 0;
@@ -1134,7 +1127,7 @@ static int imx_rproc_probe(struct platform_device *pdev)
 						    imx_rproc_sys_off_handler, rproc);
 		if (ret) {
 			dev_err(dev, "register power off handler failure\n");
-			goto err_put_clk;
+			goto err_put_scu;
 		}
 
 		ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_RESTART_PREPARE,
@@ -1142,7 +1135,7 @@ static int imx_rproc_probe(struct platform_device *pdev)
 						    imx_rproc_sys_off_handler, rproc);
 		if (ret) {
 			dev_err(dev, "register restart handler failure\n");
-			goto err_put_clk;
+			goto err_put_scu;
 		}
 	}
 
@@ -1150,32 +1143,30 @@ static int imx_rproc_probe(struct platform_device *pdev)
 		ret = devm_pm_runtime_enable(dev);
 		if (ret) {
 			dev_err(dev, "Failed to enable runtime PM, %d\n", ret);
-			goto err_put_clk;
+			goto err_put_scu;
 		}
 
 		ret = pm_runtime_resume_and_get(dev);
 		if (ret) {
 			dev_err(dev, "pm_runtime get failed: %d\n", ret);
-			goto err_put_clk;
+			goto err_put_scu;
 		}
 
 		ret = devm_add_action_or_reset(dev, imx_rproc_pm_runtime_put, dev);
 		if (ret) {
 			dev_err(dev, "Failed to add devm disable pm action: %d\n", ret);
-			goto err_put_clk;
+			goto err_put_scu;
 		}
 	}
 
 	ret = rproc_add(rproc);
 	if (ret) {
 		dev_err(dev, "rproc_add failed\n");
-		goto err_put_clk;
+		goto err_put_scu;
 	}
 
 	return 0;
 
-err_put_clk:
-	clk_disable_unprepare(priv->clk);
 err_put_scu:
 	imx_rproc_put_scu(rproc);
 
@@ -1185,9 +1176,7 @@ static int imx_rproc_probe(struct platform_device *pdev)
 static void imx_rproc_remove(struct platform_device *pdev)
 {
 	struct rproc *rproc = platform_get_drvdata(pdev);
-	struct imx_rproc *priv = rproc->priv;
 
-	clk_disable_unprepare(priv->clk);
 	rproc_del(rproc);
 	imx_rproc_put_scu(rproc);
 }

-- 
2.37.1

[PATCH v2 5/6] remoteproc: imx_rproc: Use devm_add_action_or_reset() for scu cleanup

[Peng Fan]

Replace the explicit call to imx_rproc_put_scu() in the remove path with
devm_add_action_or_reset(). Ensure proper cleanup of scu resources and
simplify the code by leveraging the device-managed resource framework.

Additionally:
 - Remove the IMX_RPROC_SCU_API check from imx_rproc_put_scu(), as
   devm_add_action_or_reset() now exclusively handles SCU cleanup.
 - Improve error reporting by using dev_err_probe() for consistency and
   clarity.
 - Drop the err_put_scu label, as it is now redundant due to the updated
   error handling approach.

No functional changes.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Reviewed-by: Daniel Baluta <daniel.baluta@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 56 ++++++++++++++----------------------------
 1 file changed, 19 insertions(+), 37 deletions(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 42014b42bd8d3b46b1a898969f0419e2e6a71528..a53ff186d218f54123e1ce740b0277a6fe95a902 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -796,13 +796,9 @@ static void imx_rproc_free_mbox(void *data)
 	}
 }
 
-static void imx_rproc_put_scu(struct rproc *rproc)
+static void imx_rproc_put_scu(void *data)
 {
-	struct imx_rproc *priv = rproc->priv;
-	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
-
-	if (dcfg->method != IMX_RPROC_SCU_API)
-		return;
+	struct imx_rproc *priv = data;
 
 	if (imx_sc_rm_is_resource_owned(priv->ipc_handle, priv->rsrc_id)) {
 		dev_pm_domain_detach_list(priv->pd_list);
@@ -944,6 +940,10 @@ static int imx_rproc_scu_api_detect_mode(struct rproc *rproc)
 	else
 		priv->core_index = 0;
 
+	ret = devm_add_action_or_reset(dev, imx_rproc_put_scu, priv);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to add action for put scu\n");
+
 	/*
 	 * If Mcore resource is not owned by Acore partition, It is kicked by ROM,
 	 * and Linux could only do IPC with Mcore and nothing else.
@@ -1110,7 +1110,7 @@ static int imx_rproc_probe(struct platform_device *pdev)
 
 	ret = imx_rproc_clk_enable(priv);
 	if (ret)
-		goto err_put_scu;
+		return dev_err_probe(dev, ret, "failed to enable clks\n");
 
 	if (rproc->state != RPROC_DETACHED)
 		rproc->auto_boot = of_property_read_bool(np, "fsl,auto-boot");
@@ -1125,52 +1125,35 @@ static int imx_rproc_probe(struct platform_device *pdev)
 		ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF_PREPARE,
 						    SYS_OFF_PRIO_DEFAULT,
 						    imx_rproc_sys_off_handler, rproc);
-		if (ret) {
-			dev_err(dev, "register power off handler failure\n");
-			goto err_put_scu;
-		}
+		if (ret)
+			return dev_err_probe(dev, ret, "register power off handler failure\n");
 
 		ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_RESTART_PREPARE,
 						    SYS_OFF_PRIO_DEFAULT,
 						    imx_rproc_sys_off_handler, rproc);
-		if (ret) {
-			dev_err(dev, "register restart handler failure\n");
-			goto err_put_scu;
-		}
+		if (ret)
+			return dev_err_probe(dev, ret, "register restart handler failure\n");
 	}
 
 	if (dcfg->method == IMX_RPROC_SCU_API) {
 		ret = devm_pm_runtime_enable(dev);
-		if (ret) {
-			dev_err(dev, "Failed to enable runtime PM, %d\n", ret);
-			goto err_put_scu;
-		}
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
 
 		ret = pm_runtime_resume_and_get(dev);
-		if (ret) {
-			dev_err(dev, "pm_runtime get failed: %d\n", ret);
-			goto err_put_scu;
-		}
+		if (ret)
+			return dev_err_probe(dev, ret, "pm_runtime get failed\n");
 
 		ret = devm_add_action_or_reset(dev, imx_rproc_pm_runtime_put, dev);
-		if (ret) {
-			dev_err(dev, "Failed to add devm disable pm action: %d\n", ret);
-			goto err_put_scu;
-		}
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to add devm disable pm action\n");
 	}
 
 	ret = rproc_add(rproc);
-	if (ret) {
-		dev_err(dev, "rproc_add failed\n");
-		goto err_put_scu;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "rproc_add failed\n");
 
 	return 0;
-
-err_put_scu:
-	imx_rproc_put_scu(rproc);
-
-	return ret;
 }
 
 static void imx_rproc_remove(struct platform_device *pdev)
@@ -1178,7 +1161,6 @@ static void imx_rproc_remove(struct platform_device *pdev)
 	struct rproc *rproc = platform_get_drvdata(pdev);
 
 	rproc_del(rproc);
-	imx_rproc_put_scu(rproc);
 }
 
 static const struct imx_rproc_plat_ops imx_rproc_ops_arm_smc = {

-- 
2.37.1

[PATCH v2 6/6] remoteproc: imx_rproc: Use devm_rproc_add() helper

[Peng Fan]

Replace manual rproc_add() and cleanup logic with devm_rproc_add(), which
ties the remoteproc lifecycle to the device's lifecycle. This simplifies
error handling and ensures proper cleanup.

With no need to invoke rproc_del(), the remove() ops could be removed.

No functional changes.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Reviewed-by: Daniel Baluta <daniel.baluta@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index a53ff186d218f54123e1ce740b0277a6fe95a902..694fbbb2f34061de22a3a815f8a6114159585f9e 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -1149,20 +1149,13 @@ static int imx_rproc_probe(struct platform_device *pdev)
 			return dev_err_probe(dev, ret, "Failed to add devm disable pm action\n");
 	}
 
-	ret = rproc_add(rproc);
+	ret = devm_rproc_add(dev, rproc);
 	if (ret)
 		return dev_err_probe(dev, ret, "rproc_add failed\n");
 
 	return 0;
 }
 
-static void imx_rproc_remove(struct platform_device *pdev)
-{
-	struct rproc *rproc = platform_get_drvdata(pdev);
-
-	rproc_del(rproc);
-}
-
 static const struct imx_rproc_plat_ops imx_rproc_ops_arm_smc = {
 	.start		= imx_rproc_arm_smc_start,
 	.stop		= imx_rproc_arm_smc_stop,
@@ -1288,7 +1281,6 @@ MODULE_DEVICE_TABLE(of, imx_rproc_of_match);
 
 static struct platform_driver imx_rproc_driver = {
 	.probe = imx_rproc_probe,
-	.remove = imx_rproc_remove,
 	.driver = {
 		.name = "imx-rproc",
 		.of_match_table = imx_rproc_of_match,

-- 
2.37.1

Re: [PATCH v2 0/6] remoteproc: imx_rproc: Use device managed API to clean up the driver

[Peng Fan]

Hi Mathieu, Bjorn

On Tue, Sep 23, 2025 at 01:16:32PM +0800, Peng Fan wrote:
>This is the 2nd series to cleanup the driver.
>
>---
>Changes in v2:
>- Address a build warning in patch 4/6
>- Add R-b from Frank and Daniel
>- Link to v1: https://lore.kernel.org/r/20250917-imx_rproc_c2-v1-0-00ce23dc9c6e@nxp.com
>
>---
>Peng Fan (6):
>      remoteproc: imx_rproc: Fix runtime PM cleanup order and error handling
>      remoteproc: imx_rproc: Use devm_add_action_or_reset() for workqueue cleanup
>      remoteproc: imx_rproc: Use devm_add_action_or_reset() for mailbox cleanup
>      remoteproc: imx_rproc: Use devm_clk_get_enabled() and simplify cleanup
>      remoteproc: imx_rproc: Use devm_add_action_or_reset() for scu cleanup
>      remoteproc: imx_rproc: Use devm_rproc_add() helper


Sorry for early ping - I just wanted to check if there's any chance for this
patchset to be included in 6.18, along with the other cleanup patchset [1].

Both patchsets have received Reviewed-by tags, have been tested, and
successfully passed builds (arm64 gcc) with each patch applied incrementally.

[1] https://lore.kernel.org/linux-remoteproc/20250920-imx_rproc_c2-v2-0-3351c4c96df5@nxp.com/T/#ma16bb8a38300f6eb333ee04f00d57805aee3c114

Thanks
Peng

>
> drivers/remoteproc/imx_rproc.c | 128 ++++++++++++++++++-----------------------
> 1 file changed, 57 insertions(+), 71 deletions(-)
>---
>base-commit: c3067c2c38316c3ef013636c93daa285ee6aaa2e
>change-id: 20250916-imx_rproc_c2-2b9ad7882f4d
>
>Best regards,
>-- 
>Peng Fan <peng.fan@nxp.com>
>

Re: [PATCH v2 0/6] remoteproc: imx_rproc: Use device managed API to clean up the driver

[Mathieu Poirier]

On Wed, 24 Sept 2025 at 09:35, Peng Fan <peng.fan@oss.nxp.com> wrote:
>
> Hi Mathieu, Bjorn
>
> On Tue, Sep 23, 2025 at 01:16:32PM +0800, Peng Fan wrote:
> >This is the 2nd series to cleanup the driver.
> >
> >---
> >Changes in v2:
> >- Address a build warning in patch 4/6
> >- Add R-b from Frank and Daniel
> >- Link to v1: https://lore.kernel.org/r/20250917-imx_rproc_c2-v1-0-00ce23dc9c6e@nxp.com
> >
> >---
> >Peng Fan (6):
> >      remoteproc: imx_rproc: Fix runtime PM cleanup order and error handling
> >      remoteproc: imx_rproc: Use devm_add_action_or_reset() for workqueue cleanup
> >      remoteproc: imx_rproc: Use devm_add_action_or_reset() for mailbox cleanup
> >      remoteproc: imx_rproc: Use devm_clk_get_enabled() and simplify cleanup
> >      remoteproc: imx_rproc: Use devm_add_action_or_reset() for scu cleanup
> >      remoteproc: imx_rproc: Use devm_rproc_add() helper
>
>
> Sorry for early ping - I just wanted to check if there's any chance for this
> patchset to be included in 6.18, along with the other cleanup patchset [1].

It seems very unlikely.  I am currently looking into how the PM
runtime framework behaves to address my own questions about this patch
[1].  Furthermore, I am worried about the usage of the device
management framework when it comes to freeing memory.  I will get back
to you with comments on that front when I know we are doing the right
thing with the PM runtime framework.

I dropped the 3rd cleanup patchset.  More than once I asked you to
submit only one patchset at a time and you still refuse to take notice
of my request.

Mathieu

[1]. "remoteproc: imx_rproc: Fix runtime PM cleanup order and error handling"

>
> Both patchsets have received Reviewed-by tags, have been tested, and
> successfully passed builds (arm64 gcc) with each patch applied incrementally.
>
> [1] https://lore.kernel.org/linux-remoteproc/20250920-imx_rproc_c2-v2-0-3351c4c96df5@nxp.com/T/#ma16bb8a38300f6eb333ee04f00d57805aee3c114
>
> Thanks
> Peng
>
> >
> > drivers/remoteproc/imx_rproc.c | 128 ++++++++++++++++++-----------------------
> > 1 file changed, 57 insertions(+), 71 deletions(-)
> >---
> >base-commit: c3067c2c38316c3ef013636c93daa285ee6aaa2e
> >change-id: 20250916-imx_rproc_c2-2b9ad7882f4d
> >
> >Best regards,
> >--
> >Peng Fan <peng.fan@nxp.com>
> >

Re: [PATCH v2 0/6] remoteproc: imx_rproc: Use device managed API to clean up the driver

[Peng Fan]

On Wed, Sep 24, 2025 at 11:10:33AM -0600, Mathieu Poirier wrote:
>On Wed, 24 Sept 2025 at 09:35, Peng Fan <peng.fan@oss.nxp.com> wrote:
>>
...
>> Sorry for early ping - I just wanted to check if there's any chance for this
>> patchset to be included in 6.18, along with the other cleanup patchset [1].
>
>It seems very unlikely.  I am currently looking into how the PM
>runtime framework behaves to address my own questions about this patch
>[1].  Furthermore, I am worried about the usage of the device
>management framework when it comes to freeing memory.  I will get back
>to you with comments on that front when I know we are doing the right
>thing with the PM runtime framework.

I see. Not sure Ulf could help clarify or review, then you might take less
time.

>
>I dropped the 3rd cleanup patchset.  More than once I asked you to
>submit only one patchset at a time and you still refuse to take notice
>of my request.

I apologize - I now recall your earlier request to hold off on submitting
further patches until the table_sz clearing patch was clarified. I
misunderstood and appreciate your patience.

Could you please clarify whether there's a general rule in remoteproc that
developers should only have one patchset or patch under review at a time? If
so, would it be possible to document this guideline in the kernel documentation?
That would help avoid confusion for contributors.

I ask because I have other patches queued that are independent of the current
series, such as:
 - Reintroducing the table_sz clearing
 - Misc cleanup in remoteproc core

I understand you may be busy and have limited bandwidth. Would it be feasible
to offload part of the review work to Bjorn? I rarely see Bjorn reviewing i.MX
patches. Alternatively, could we consider bringing in a third maintainer to
help accelerate the review process?

Thanks again for your time and guidance.

Thanks,
Peng

>
>Mathieu
>
>[1]. "remoteproc: imx_rproc: Fix runtime PM cleanup order and error handling"
>
>>
>> Both patchsets have received Reviewed-by tags, have been tested, and
>> successfully passed builds (arm64 gcc) with each patch applied incrementally.
>>
>> [1] https://lore.kernel.org/linux-remoteproc/20250920-imx_rproc_c2-v2-0-3351c4c96df5@nxp.com/T/#ma16bb8a38300f6eb333ee04f00d57805aee3c114
>>
>> Thanks
>> Peng
>>
>> >
>> > drivers/remoteproc/imx_rproc.c | 128 ++++++++++++++++++-----------------------
>> > 1 file changed, 57 insertions(+), 71 deletions(-)
>> >---
>> >base-commit: c3067c2c38316c3ef013636c93daa285ee6aaa2e
>> >change-id: 20250916-imx_rproc_c2-2b9ad7882f4d
>> >
>> >Best regards,
>> >--
>> >Peng Fan <peng.fan@nxp.com>
>> >
>

Re: [PATCH v2 0/6] remoteproc: imx_rproc: Use device managed API to clean up the driver

[Mathieu Poirier]

On Thu, Sep 25, 2025 at 04:31:15AM +0800, Peng Fan wrote:
> On Wed, Sep 24, 2025 at 11:10:33AM -0600, Mathieu Poirier wrote:
> >On Wed, 24 Sept 2025 at 09:35, Peng Fan <peng.fan@oss.nxp.com> wrote:
> >>
> ...
> >> Sorry for early ping - I just wanted to check if there's any chance for this
> >> patchset to be included in 6.18, along with the other cleanup patchset [1].
> >
> >It seems very unlikely.  I am currently looking into how the PM
> >runtime framework behaves to address my own questions about this patch
> >[1].  Furthermore, I am worried about the usage of the device
> >management framework when it comes to freeing memory.  I will get back
> >to you with comments on that front when I know we are doing the right
> >thing with the PM runtime framework.
> 
> I see. Not sure Ulf could help clarify or review, then you might take less
> time.
>

It is fortunate that time was taken to understand the problem and fix it
correctly.  Otherwise we'd still have a problem and more patches, possibly
wrong as well, would have been needed.
 
> >
> >I dropped the 3rd cleanup patchset.  More than once I asked you to
> >submit only one patchset at a time and you still refuse to take notice
> >of my request.
> 
> I apologize - I now recall your earlier request to hold off on submitting
> further patches until the table_sz clearing patch was clarified. I
> misunderstood and appreciate your patience.
> 
> Could you please clarify whether there's a general rule in remoteproc that
> developers should only have one patchset or patch under review at a time? If
> so, would it be possible to document this guideline in the kernel documentation?
> That would help avoid confusion for contributors.
>

Most people tend to address one problem at a time, especially when subsequent
patchsets have dependencies on the previous ones.  I'm not sure there is a need
to document something like that.
 
> I ask because I have other patches queued that are independent of the current
> series, such as:
>  - Reintroducing the table_sz clearing
>  - Misc cleanup in remoteproc core

I'm fine with those, as long as you address just one proble at any given time.

> 
> I understand you may be busy and have limited bandwidth. Would it be feasible
> to offload part of the review work to Bjorn? I rarely see Bjorn reviewing i.MX
> patches. Alternatively, could we consider bringing in a third maintainer to
> help accelerate the review process?
>

How fast do you want to go?  By and large, I reply to patchsets within a week,
sometimes two when things are busy.  And when I can't meet those standards, I
send out an email to the mailing list with the review order of the patches in
my queue.  What else are you expecting?

Bjorn is maintaining over a dozen subsystems - I stepped forward to maintain
remoteproc/rpmsg to help with the volume.

 
> Thanks again for your time and guidance.
> 
> Thanks,
> Peng
> 
> >
> >Mathieu
> >
> >[1]. "remoteproc: imx_rproc: Fix runtime PM cleanup order and error handling"
> >
> >>
> >> Both patchsets have received Reviewed-by tags, have been tested, and
> >> successfully passed builds (arm64 gcc) with each patch applied incrementally.
> >>
> >> [1] https://lore.kernel.org/linux-remoteproc/20250920-imx_rproc_c2-v2-0-3351c4c96df5@nxp.com/T/#ma16bb8a38300f6eb333ee04f00d57805aee3c114
> >>
> >> Thanks
> >> Peng
> >>
> >> >
> >> > drivers/remoteproc/imx_rproc.c | 128 ++++++++++++++++++-----------------------
> >> > 1 file changed, 57 insertions(+), 71 deletions(-)
> >> >---
> >> >base-commit: c3067c2c38316c3ef013636c93daa285ee6aaa2e
> >> >change-id: 20250916-imx_rproc_c2-2b9ad7882f4d
> >> >
> >> >Best regards,
> >> >--
> >> >Peng Fan <peng.fan@nxp.com>
> >> >
> >

Re: [PATCH v2 0/6] remoteproc: imx_rproc: Use device managed API to clean up the driver

[Peng Fan]

On Thu, Sep 25, 2025 at 09:25:28AM -0600, Mathieu Poirier wrote:
>On Thu, Sep 25, 2025 at 04:31:15AM +0800, Peng Fan wrote:
>
>It is fortunate that time was taken to understand the problem and fix it
>correctly.  Otherwise we'd still have a problem and more patches, possibly
>wrong as well, would have been needed.

Thanks for your caution and patience.

> 
>> >
>
>Most people tend to address one problem at a time, especially when subsequent
>patchsets have dependencies on the previous ones.  I'm not sure there is a need
>to document something like that.

I will not post dependent patches in future.

> 
>> I ask because I have other patches queued that are independent of the current
>> series, such as:
>>  - Reintroducing the table_sz clearing
>>  - Misc cleanup in remoteproc core
>
>I'm fine with those, as long as you address just one proble at any given time.

Thanks. So multiple patchsets/patches from one developer are allowed only if
each patchset/patch focus on a specific area/problem and no dependency.

It is clear to me.

>
>> 
>> I understand you may be busy and have limited bandwidth. Would it be feasible
>> to offload part of the review work to Bjorn? I rarely see Bjorn reviewing i.MX
>> patches. Alternatively, could we consider bringing in a third maintainer to
>> help accelerate the review process?
>>
>
>How fast do you want to go?  By and large, I reply to patchsets within a week,
>sometimes two when things are busy.  And when I can't meet those standards, I
>send out an email to the mailing list with the review order of the patches in
>my queue.  What else are you expecting?

I have been blocked once [1], this make me delay about two months to post out
i.MX95 (: For i.MX95 reviewing cycle, it is not that smooth. Anyway, I get
lesson learned and will follow the rules.

More and more, we(NXP) are taking upstream first. And I have requested Daniel
and Frank(both are very experienced Linux Kernel developer) to help review, so
hope we could move a bit smoothly.

For now,
To imx_rproc.c, the queue:
cleanup patchset2(at here now), cleanup patchset3, i.MX95 support, i.MX943 support

To imx_dsp_rproc.c, the queue:
Drop method, switch to use ops

To remoteproc_core.c, the queue:
Introduce back clearing table_sz.
Use scoped API

Nothing special to expect from you. I get the message that non-relevant
patchsets could be posted out for reviewing, and one or two weeks is good for
me.

Appreciate for your time.

[1] https://lore.kernel.org/linux-remoteproc/CANLsYkyEhhQA5KOsNveGSHUc3ZpckoL-CCHNZ0DZLMNYdNGzdQ@mail.gmail.com/

Thanks,
Peng