Re: [PATCH] firmware: qcom: scm: Fix resource cleanup on probe failure
From: Mukesh Ojha
Date: Thu Jun 25 2026 - 07:29:57 EST
On Thu, Jun 25, 2026 at 03:06:07AM -0700, Bartosz Golaszewski wrote:
> On Thu, 25 Jun 2026 11:36:44 +0200, Mukesh Ojha
> <mukesh.ojha@xxxxxxxxxxxxxxxx> said:
> > qcom_scm_probe() acquires two non-devres resources that are never
> > released if probe fails or defers after them. of_reserved_mem_device_init()
> > adds an entry to a global list with no devres counterpart, so a retry
> > would add a duplicate entry and leak the original. qcom_tzmem_enable()
> > sets a static qcom_tzmem_dev pointer and may set qcom_tzmem_using_shm_bridge;
> > without cleanup a probe retry finds qcom_tzmem_dev already set and
> > returns -EBUSY, permanently preventing the driver from probing.
> >
> > Introduce err_tzmem and err_rmem goto labels at the end of probe to
> > call qcom_tzmem_disable() and of_reserved_mem_device_release() in the
> > right order, route all subsequent error paths through them, and add
> > qcom_tzmem_disable() to qcom_tzmem.c to clear the static state.
> >
> > Fixes: a33b2579c8d3 ("firmware: qcom: scm: add support for SHM bridge memory carveout")
> > Fixes: 40289e35ca52 ("firmware: qcom: scm: enable the TZ mem allocator")
> > Signed-off-by: Mukesh Ojha <mukesh.ojha@xxxxxxxxxxxxxxxx>
> > ---
>
> Hi!
>
> These are two separate issues, I think you should split the change into two
> patches.
Initially, thought of doing this but modifying the same goto
level again added in the previous patch was not looking nice.
>
> > This is reported on sasiko review as existing issue here
> > https://lore.kernel.org/all/20260624192213.C82691F000E9@xxxxxxxxxxxxxxx/
> > and it can go independently.
> >
> > drivers/firmware/qcom/qcom_scm.c | 42 +++++++++++++++++++++---------
> > drivers/firmware/qcom/qcom_tzmem.c | 7 +++++
> > drivers/firmware/qcom/qcom_tzmem.h | 1 +
> > 3 files changed, 37 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> > index ba5cdeed8a04..cb3d776fa645 100644
> > --- a/drivers/firmware/qcom/qcom_scm.c
> > +++ b/drivers/firmware/qcom/qcom_scm.c
> > @@ -2883,9 +2883,11 @@ static int qcom_scm_probe(struct platform_device *pdev)
> > "Failed to setup the reserved memory region for TZ mem\n");
> >
> > ret = qcom_tzmem_enable(scm->dev);
> > - if (ret)
> > - return dev_err_probe(scm->dev, ret,
> > - "Failed to enable the TrustZone memory allocator\n");
> > + if (ret) {
> > + ret = dev_err_probe(scm->dev, ret,
> > + "Failed to enable the TrustZone memory allocator\n");
> > + goto err_rmem;
> > + }
> >
> > memset(&pool_config, 0, sizeof(pool_config));
> > pool_config.initial_size = 0;
> > @@ -2893,16 +2895,20 @@ static int qcom_scm_probe(struct platform_device *pdev)
> > pool_config.max_size = SZ_256K;
> >
> > scm->mempool = devm_qcom_tzmem_pool_new(scm->dev, &pool_config);
> > - if (IS_ERR(scm->mempool))
> > - return dev_err_probe(scm->dev, PTR_ERR(scm->mempool),
> > - "Failed to create the SCM memory pool\n");
> > + if (IS_ERR(scm->mempool)) {
> > + ret = dev_err_probe(scm->dev, PTR_ERR(scm->mempool),
> > + "Failed to create the SCM memory pool\n");
> > + goto err_tzmem;
> > + }
> >
> > ret = qcom_scm_query_waitq_count(scm);
> > scm->wq_cnt = ret < 0 ? QCOM_SCM_DEFAULT_WAITQ_COUNT : ret;
> > scm->waitq_comps = devm_kcalloc(&pdev->dev, scm->wq_cnt, sizeof(*scm->waitq_comps),
> > GFP_KERNEL);
> > - if (!scm->waitq_comps)
> > - return -ENOMEM;
> > + if (!scm->waitq_comps) {
> > + ret = -ENOMEM;
> > + goto err_tzmem;
> > + }
> >
> > for (i = 0; i < scm->wq_cnt; i++)
> > init_completion(&scm->waitq_comps[i]);
> > @@ -2912,14 +2918,18 @@ static int qcom_scm_probe(struct platform_device *pdev)
> > irq = platform_get_irq_optional(pdev, 0);
> >
> > if (irq < 0) {
> > - if (irq != -ENXIO)
> > - return irq;
> > + if (irq != -ENXIO) {
> > + ret = irq;
> > + goto err_tzmem;
> > + }
> > } else {
> > ret = devm_request_threaded_irq(scm->dev, irq, NULL, qcom_scm_irq_handler,
> > IRQF_ONESHOT, "qcom-scm", scm);
> > - if (ret < 0)
> > - return dev_err_probe(scm->dev, ret,
> > - "Failed to request qcom-scm irq\n");
> > + if (ret < 0) {
> > + ret = dev_err_probe(scm->dev, ret,
> > + "Failed to request qcom-scm irq\n");
> > + goto err_tzmem;
> > + }
> > }
> >
> > /*
> > @@ -2966,6 +2976,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
> > qcom_scm_gunyah_wdt_init(scm);
> >
> > return 0;
> > +
> > +err_tzmem:
> > + qcom_tzmem_disable(scm->dev);
> > +err_rmem:
> > + of_reserved_mem_device_release(scm->dev);
> > + return ret;
> > }
> >
> > static void qcom_scm_shutdown(struct platform_device *pdev)
> > diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c
> > index 0635cbeacfc8..3f2b782f4a94 100644
> > --- a/drivers/firmware/qcom/qcom_tzmem.c
> > +++ b/drivers/firmware/qcom/qcom_tzmem.c
> > @@ -518,6 +518,13 @@ int qcom_tzmem_enable(struct device *dev)
> > }
> > EXPORT_SYMBOL_GPL(qcom_tzmem_enable);
> >
> > +void qcom_tzmem_disable(struct device *dev)
> > +{
> > + qcom_tzmem_using_shm_bridge = false;
> > + qcom_tzmem_dev = NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(qcom_tzmem_disable);
>
> That being said, I think we should just modify qcom_tzmem_enable() to silently
> ignore subsequent calls. It's meant to be called once and stay enabled so I
> suggest just removing the check returning -EBUSY.
So, you meant to let it call twice..even if it is doing.
>
> Bart
>
> > +
> > MODULE_DESCRIPTION("TrustZone memory allocator for Qualcomm firmware drivers");
> > MODULE_AUTHOR("Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>");
> > MODULE_LICENSE("GPL");
> > diff --git a/drivers/firmware/qcom/qcom_tzmem.h b/drivers/firmware/qcom/qcom_tzmem.h
> > index 8fa8a3eb940e..0b0f26d4e22e 100644
> > --- a/drivers/firmware/qcom/qcom_tzmem.h
> > +++ b/drivers/firmware/qcom/qcom_tzmem.h
> > @@ -9,5 +9,6 @@
> > struct device;
> >
> > int qcom_tzmem_enable(struct device *dev);
> > +void qcom_tzmem_disable(struct device *dev);
> >
> > #endif /* __QCOM_TZMEM_PRIV_H */
> > --
> > 2.53.0
> >
> >
--
-Mukesh Ojha