PDR and SSR callbacks are registred from the controller probe function,
but currently released from the child device's remove function.

In the next commit the controller probe function will be modified such
that the error path will unregister the child device, resulting in a
double free of these resources.

Fixes: 917809e2280b ("slimbus: ngd: Add qcom SLIMBus NGD driver")
Cc: stable@vger.kernel.org
Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
 drivers/slimbus/qcom-ngd-ctrl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/slimbus/qcom-ngd-ctrl.c b/drivers/slimbus/qcom-ngd-ctrl.c
index b34e727bab086c95dc7e760bf1141baac9ccf6a7..09ce3299e15c25b1b9cf6b1559850adf4aa20737 100644
--- a/drivers/slimbus/qcom-ngd-ctrl.c
+++ b/drivers/slimbus/qcom-ngd-ctrl.c
@@ -1685,6 +1685,9 @@ static void qcom_slim_ngd_ctrl_remove(struct platform_device *pdev)
 {
 	struct qcom_slim_ngd_ctrl *ctrl = platform_get_drvdata(pdev);
 
+	pdr_handle_release(ctrl->pdr);
+	qcom_unregister_ssr_notifier(ctrl->notifier, &ctrl->nb);
+
 	qcom_slim_ngd_unregister(ctrl);
 }
 
@@ -1693,8 +1696,6 @@ static void qcom_slim_ngd_remove(struct platform_device *pdev)
 	struct qcom_slim_ngd_ctrl *ctrl = platform_get_drvdata(pdev);
 
 	pm_runtime_disable(&pdev->dev);
-	pdr_handle_release(ctrl->pdr);
-	qcom_unregister_ssr_notifier(ctrl->notifier, &ctrl->nb);
 	qcom_slim_ngd_enable(ctrl, false);
 	qcom_slim_ngd_exit_dma(ctrl);
 	qcom_slim_ngd_qmi_svc_event_deinit(&ctrl->qmi);

-- 
2.51.0

On Mon, Mar 09, 2026 at 11:09:04PM -0500, Bjorn Andersson wrote:
> PDR and SSR callbacks are registred from the controller probe function,
> but currently released from the child device's remove function.
> 
> In the next commit the controller probe function will be modified such
> that the error path will unregister the child device, resulting in a
> double free of these resources.

I'd just say that the foo_remove() should (only) be unwinding what was
done in foo_probe().

With that in place:

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

> 
> Fixes: 917809e2280b ("slimbus: ngd: Add qcom SLIMBus NGD driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> ---
>  drivers/slimbus/qcom-ngd-ctrl.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 

-- 
With best wishes
Dmitry

On Mon, Mar 09, 2026 at 11:09:04PM -0500, Bjorn Andersson wrote:
> PDR and SSR callbacks are registred from the controller probe function,
> but currently released from the child device's remove function.
> 
> In the next commit the controller probe function will be modified such
> that the error path will unregister the child device, resulting in a
> double free of these resources.

Change is fine, Could this not be accommodated in the next commit?

> 
> Fixes: 917809e2280b ("slimbus: ngd: Add qcom SLIMBus NGD driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>

Reviewed-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

> ---
>  drivers/slimbus/qcom-ngd-ctrl.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/slimbus/qcom-ngd-ctrl.c b/drivers/slimbus/qcom-ngd-ctrl.c
> index b34e727bab086c95dc7e760bf1141baac9ccf6a7..09ce3299e15c25b1b9cf6b1559850adf4aa20737 100644
> --- a/drivers/slimbus/qcom-ngd-ctrl.c
> +++ b/drivers/slimbus/qcom-ngd-ctrl.c
> @@ -1685,6 +1685,9 @@ static void qcom_slim_ngd_ctrl_remove(struct platform_device *pdev)
>  {
>  	struct qcom_slim_ngd_ctrl *ctrl = platform_get_drvdata(pdev);
>  
> +	pdr_handle_release(ctrl->pdr);
> +	qcom_unregister_ssr_notifier(ctrl->notifier, &ctrl->nb);
> +
>  	qcom_slim_ngd_unregister(ctrl);
>  }
>  
> @@ -1693,8 +1696,6 @@ static void qcom_slim_ngd_remove(struct platform_device *pdev)
>  	struct qcom_slim_ngd_ctrl *ctrl = platform_get_drvdata(pdev);
>  
>  	pm_runtime_disable(&pdev->dev);
> -	pdr_handle_release(ctrl->pdr);
> -	qcom_unregister_ssr_notifier(ctrl->notifier, &ctrl->nb);
>  	qcom_slim_ngd_enable(ctrl, false);
>  	qcom_slim_ngd_exit_dma(ctrl);
>  	qcom_slim_ngd_qmi_svc_event_deinit(&ctrl->qmi);
> 
> -- 
> 2.51.0
> 

-- 
-Mukesh Ojha

On Tue, Mar 10, 2026 at 01:09:33PM +0530, Mukesh Ojha wrote:
> On Mon, Mar 09, 2026 at 11:09:04PM -0500, Bjorn Andersson wrote:
> > PDR and SSR callbacks are registred from the controller probe function,
> > but currently released from the child device's remove function.
> > 
> > In the next commit the controller probe function will be modified such
> > that the error path will unregister the child device, resulting in a
> > double free of these resources.
> 
> Change is fine, Could this not be accommodated in the next commit?
> 

The problem solved by patch 4 relates to the oreder that we're acquiring
the resources in probe and how the error handling of that works.

If I squash the two patches, it seems that I would have a lengthy commit
message talking about that part and then a "also, while at it move the
unregister from X to Y because...".

I.e. it doesn't feel like the same logical change to me.

Please let me know if you disagree.

Regards,
Bjorn

> > 
> > Fixes: 917809e2280b ("slimbus: ngd: Add qcom SLIMBus NGD driver")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> 
> Reviewed-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> 
> > ---
> >  drivers/slimbus/qcom-ngd-ctrl.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/slimbus/qcom-ngd-ctrl.c b/drivers/slimbus/qcom-ngd-ctrl.c
> > index b34e727bab086c95dc7e760bf1141baac9ccf6a7..09ce3299e15c25b1b9cf6b1559850adf4aa20737 100644
> > --- a/drivers/slimbus/qcom-ngd-ctrl.c
> > +++ b/drivers/slimbus/qcom-ngd-ctrl.c
> > @@ -1685,6 +1685,9 @@ static void qcom_slim_ngd_ctrl_remove(struct platform_device *pdev)
> >  {
> >  	struct qcom_slim_ngd_ctrl *ctrl = platform_get_drvdata(pdev);
> >  
> > +	pdr_handle_release(ctrl->pdr);
> > +	qcom_unregister_ssr_notifier(ctrl->notifier, &ctrl->nb);
> > +
> >  	qcom_slim_ngd_unregister(ctrl);
> >  }
> >  
> > @@ -1693,8 +1696,6 @@ static void qcom_slim_ngd_remove(struct platform_device *pdev)
> >  	struct qcom_slim_ngd_ctrl *ctrl = platform_get_drvdata(pdev);
> >  
> >  	pm_runtime_disable(&pdev->dev);
> > -	pdr_handle_release(ctrl->pdr);
> > -	qcom_unregister_ssr_notifier(ctrl->notifier, &ctrl->nb);
> >  	qcom_slim_ngd_enable(ctrl, false);
> >  	qcom_slim_ngd_exit_dma(ctrl);
> >  	qcom_slim_ngd_qmi_svc_event_deinit(&ctrl->qmi);
> > 
> > -- 
> > 2.51.0
> > 
> 
> -- 
> -Mukesh Ojha

On Mon, Mar 23, 2026 at 09:36:49PM -0500, Bjorn Andersson wrote:
> On Tue, Mar 10, 2026 at 01:09:33PM +0530, Mukesh Ojha wrote:
> > On Mon, Mar 09, 2026 at 11:09:04PM -0500, Bjorn Andersson wrote:
> > > PDR and SSR callbacks are registred from the controller probe function,
> > > but currently released from the child device's remove function.
> > > 
> > > In the next commit the controller probe function will be modified such
> > > that the error path will unregister the child device, resulting in a
> > > double free of these resources.
> > 
> > Change is fine, Could this not be accommodated in the next commit?
> > 
> 
> The problem solved by patch 4 relates to the oreder that we're acquiring
> the resources in probe and how the error handling of that works.
> 
> If I squash the two patches, it seems that I would have a lengthy commit
> message talking about that part and then a "also, while at it move the
> unregister from X to Y because...".
> 
> I.e. it doesn't feel like the same logical change to me.
> 
> Please let me know if you disagree.

Agree..

I was not sure about these text usage "next commit" and "will be modified",
Hence, was asking if we can accomodate it in one commit as both are Cced to
stable


> 
> Regards,
> Bjorn
> 
> > > 
> > > Fixes: 917809e2280b ("slimbus: ngd: Add qcom SLIMBus NGD driver")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > 
> > Reviewed-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> > 
> > > ---
> > >  drivers/slimbus/qcom-ngd-ctrl.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/slimbus/qcom-ngd-ctrl.c b/drivers/slimbus/qcom-ngd-ctrl.c
> > > index b34e727bab086c95dc7e760bf1141baac9ccf6a7..09ce3299e15c25b1b9cf6b1559850adf4aa20737 100644
> > > --- a/drivers/slimbus/qcom-ngd-ctrl.c
> > > +++ b/drivers/slimbus/qcom-ngd-ctrl.c
> > > @@ -1685,6 +1685,9 @@ static void qcom_slim_ngd_ctrl_remove(struct platform_device *pdev)
> > >  {
> > >  	struct qcom_slim_ngd_ctrl *ctrl = platform_get_drvdata(pdev);
> > >  
> > > +	pdr_handle_release(ctrl->pdr);
> > > +	qcom_unregister_ssr_notifier(ctrl->notifier, &ctrl->nb);
> > > +
> > >  	qcom_slim_ngd_unregister(ctrl);
> > >  }
> > >  
> > > @@ -1693,8 +1696,6 @@ static void qcom_slim_ngd_remove(struct platform_device *pdev)
> > >  	struct qcom_slim_ngd_ctrl *ctrl = platform_get_drvdata(pdev);
> > >  
> > >  	pm_runtime_disable(&pdev->dev);
> > > -	pdr_handle_release(ctrl->pdr);
> > > -	qcom_unregister_ssr_notifier(ctrl->notifier, &ctrl->nb);
> > >  	qcom_slim_ngd_enable(ctrl, false);
> > >  	qcom_slim_ngd_exit_dma(ctrl);
> > >  	qcom_slim_ngd_qmi_svc_event_deinit(&ctrl->qmi);
> > > 
> > > -- 
> > > 2.51.0
> > > 
> > 
> > -- 
> > -Mukesh Ojha

-- 
-Mukesh Ojha