Re: [PATCH] firmware: qcom: scm: Fix resource cleanup on probe failure

[Bartosz Golaszewski]

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.

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

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
>
>