[PATCH] remoteproc: qcom_q6v5_pas: Release dtb_firmware on stop

[PATCH] remoteproc: qcom_q6v5_pas: Release dtb_firmware on stop

[Sailesh Nandanavanam]

qcom_pas_load() acquires a reference to pas->dtb_firmware via
request_firmware() whenever the platform defines dtb_pas_id. This
reference is only released on the error path inside qcom_pas_load()
itself, when qcom_mdt_pas_load() fails.

On the success path, pas->dtb_firmware is never released by
qcom_pas_start(), qcom_pas_stop(), qcom_pas_unprepare(), or
qcom_pas_remove(). Since qcom_pas_load() runs on every load/reload
cycle, including subsystem-restart recovery, this leaks a firmware
reference on every successful boot of remoteprocs that use a DTB
co-firmware image (e.g. ADSP/CDSP/MPSS on SM8550, SM8650, SM8750,
X1E80100).

A second, narrower leak window exists if qcom_pas_start() itself
fails after the DTB carveout has been mapped: the remoteproc core
does not invoke .stop for a remoteproc that failed to start, so
qcom_pas_stop() never runs to release the reference in that case.

Release pas->dtb_firmware in both qcom_pas_stop() and at the
unmap_dtb_carveout label in qcom_pas_start(), alongside the existing
teardown of the DTB carveout mapping, and clear the pointer to avoid
holding a stale reference into the next load cycle.

Fixes: 29814986b82e ("remoteproc: qcom_q6v5_pas: add support for dtb co-firmware loading")

Signed-off-by: Sailesh Nandanavanam <saileshnandanavanam@gmail.com>
---
 drivers/remoteproc/qcom_q6v5_pas.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index da27d1d3c9da..040aabbe5860 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -360,8 +360,11 @@ static int qcom_pas_start(struct rproc *rproc)
 		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
 
 unmap_dtb_carveout:
-	if (pas->dtb_pas_id)
+	if (pas->dtb_pas_id) {
 		qcom_pas_unmap_carveout(rproc, pas->dtb_mem_phys, pas->dtb_mem_size);
+		release_firmware(pas->dtb_firmware);
+		pas->dtb_firmware = NULL;
+	}
 disable_px_supply:
 	if (pas->px_supply)
 		regulator_disable(pas->px_supply);
@@ -419,6 +422,8 @@ static int qcom_pas_stop(struct rproc *rproc)
 			dev_err(pas->dev, "failed to shutdown dtb: %d\n", ret);
 
 		qcom_pas_unmap_carveout(rproc, pas->dtb_mem_phys, pas->dtb_mem_size);
+		release_firmware(pas->dtb_firmware);
+		pas->dtb_firmware = NULL;
 	}
 
 	qcom_pas_unmap_carveout(rproc, pas->mem_phys, pas->mem_size);
-- 
2.34.1

Re: [PATCH] remoteproc: qcom_q6v5_pas: Release dtb_firmware on stop

[Mukesh Ojha]

On Mon, Jun 22, 2026 at 09:31:14AM +0530, Sailesh Nandanavanam wrote:
> qcom_pas_load() acquires a reference to pas->dtb_firmware via
> request_firmware() whenever the platform defines dtb_pas_id. This
> reference is only released on the error path inside qcom_pas_load()
> itself, when qcom_mdt_pas_load() fails.
> 
> On the success path, pas->dtb_firmware is never released by
> qcom_pas_start(), qcom_pas_stop(), qcom_pas_unprepare(), or
> qcom_pas_remove(). Since qcom_pas_load() runs on every load/reload
> cycle, including subsystem-restart recovery, this leaks a firmware
> reference on every successful boot of remoteprocs that use a DTB
> co-firmware image (e.g. ADSP/CDSP/MPSS on SM8550, SM8650, SM8750,
> X1E80100).
> 
> A second, narrower leak window exists if qcom_pas_start() itself
> fails after the DTB carveout has been mapped: the remoteproc core
> does not invoke .stop for a remoteproc that failed to start, so
> qcom_pas_stop() never runs to release the reference in that case.

> 
> Release pas->dtb_firmware in both qcom_pas_stop() and at the
> unmap_dtb_carveout label in qcom_pas_start(), alongside the existing
> teardown of the DTB carveout mapping, and clear the pointer to avoid
> holding a stale reference into the next load cycle.
> 
> Fixes: 29814986b82e ("remoteproc: qcom_q6v5_pas: add support for dtb co-firmware loading")
>

Remove the line feed from here..

> Signed-off-by: Sailesh Nandanavanam <saileshnandanavanam@gmail.com>
> ---
>  drivers/remoteproc/qcom_q6v5_pas.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index da27d1d3c9da..040aabbe5860 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -360,8 +360,11 @@ static int qcom_pas_start(struct rproc *rproc)
>  		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
>  
>  unmap_dtb_carveout:
> -	if (pas->dtb_pas_id)
> +	if (pas->dtb_pas_id) {
>  		qcom_pas_unmap_carveout(rproc, pas->dtb_mem_phys, pas->dtb_mem_size);
> +		release_firmware(pas->dtb_firmware);

This is still not solving the issue. What if you have failed just after
the first function qcom_q6v5_prepare() in qcom_pas_start()?

would it be better to move dtb_firmware loading to qcom_pas_start() ?


> +		pas->dtb_firmware = NULL;
> +	}
>  disable_px_supply:
>  	if (pas->px_supply)
>  		regulator_disable(pas->px_supply);
> @@ -419,6 +422,8 @@ static int qcom_pas_stop(struct rproc *rproc)
>  			dev_err(pas->dev, "failed to shutdown dtb: %d\n", ret);
>  
>  		qcom_pas_unmap_carveout(rproc, pas->dtb_mem_phys, pas->dtb_mem_size);
> +		release_firmware(pas->dtb_firmware);
> +		pas->dtb_firmware = NULL;
>  	}
>  
>  	qcom_pas_unmap_carveout(rproc, pas->mem_phys, pas->mem_size);
> -- 
> 2.34.1
> 

-- 
-Mukesh Ojha

[PATCH v2] remoteproc: qcom_q6v5_pas: Fix dtb firmware lifecycle and leak

[Sailesh Nandanavanam]

DTB co-firmware was previously requested and loaded in
qcom_pas_load(), but its lifetime did not match the actual
start/stop lifecycle of the remoteproc. As a result, the firmware
reference could be retained across restart cycles, leading to a
leak for each successful boot.

Additionally, if qcom_pas_start() failed after loading the DTB
firmware, the remoteproc core would not invoke .stop(), leaving
no opportunity to release the associated firmware reference.

Fix this by moving DTB firmware request and loading into
qcom_pas_start(), so that its lifetime is strictly tied to the
remoteproc start sequence.

Update qcom_pas_start() to ensure proper cleanup on all paths:
- release PAS metadata on failure
- release DTB firmware on both success and failure paths
- unmap DTB carveout where applicable

Remove DTB firmware handling from qcom_pas_load(), as it does not
match the correct ownership and lifecycle model.

With this change, request_firmware() and release_firmware() are
properly paired within the start path, avoiding leaks and ensuring
consistent behavior across restart and failure scenarios.

Fixes: 29814986b82e ("remoteproc: qcom_q6v5_pas: add support for dtb co-firmware loading")
Signed-off-by: Sailesh Nandanavanam <saileshnandanavanam@gmail.com>
---
v2:
- Move DTB firmware request/load from qcom_pas_load() to qcom_pas_start()
- Fix firmware reference leak across restart cycles
- Handle start() failure paths where .stop() is not invoked
- Ensure firmware is released on all success and failure paths
- Remove DTB handling from load() and drop release from stop()
---
 drivers/remoteproc/qcom_q6v5_pas.c | 54 ++++++++++++++++--------------
 1 file changed, 29 insertions(+), 25 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index da27d1d3c9da..090f1f09dba3 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -232,28 +232,7 @@ static int qcom_pas_load(struct rproc *rproc, const struct firmware *fw)
 	if (pas->lite_dtb_pas_id)
 		qcom_scm_pas_shutdown(pas->lite_dtb_pas_id);
 
-	if (pas->dtb_pas_id) {
-		ret = request_firmware(&pas->dtb_firmware, pas->dtb_firmware_name, pas->dev);
-		if (ret) {
-			dev_err(pas->dev, "request_firmware failed for %s: %d\n",
-				pas->dtb_firmware_name, ret);
-			return ret;
-		}
-
-		ret = qcom_mdt_pas_load(pas->dtb_pas_ctx, pas->dtb_firmware,
-					pas->dtb_firmware_name, pas->dtb_mem_region,
-					&pas->dtb_mem_reloc);
-		if (ret)
-			goto release_dtb_metadata;
-	}
-
 	return 0;
-
-release_dtb_metadata:
-	qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
-	release_firmware(pas->dtb_firmware);
-
-	return ret;
 }
 
 static void qcom_pas_unmap_carveout(struct rproc *rproc, phys_addr_t mem_phys, size_t size)
@@ -277,9 +256,24 @@ static int qcom_pas_start(struct rproc *rproc)
 	struct qcom_pas *pas = rproc->priv;
 	int ret;
 
+	if (pas->dtb_pas_id) {
+		ret = request_firmware(&pas->dtb_firmware, pas->dtb_firmware_name, pas->dev);
+		if (ret) {
+			dev_err(pas->dev, "request_firmware failed for %s: %d\n",
+					pas->dtb_firmware_name, ret);
+			return ret;
+		}
+
+		ret = qcom_mdt_pas_load(pas->dtb_pas_ctx, pas->dtb_firmware,
+				pas->dtb_firmware_name, pas->dtb_mem_region,
+				&pas->dtb_mem_reloc);
+		if (ret)
+			goto release_dtb_metadata;
+	}
+
 	ret = qcom_q6v5_prepare(&pas->q6v5);
 	if (ret)
-		return ret;
+		goto release_dtb_metadata;
 
 	ret = qcom_pas_pds_enable(pas, pas->proxy_pds, pas->proxy_pd_count);
 	if (ret < 0)
@@ -350,15 +344,17 @@ static int qcom_pas_start(struct rproc *rproc)
 	/* firmware is used to pass reference from qcom_pas_start(), drop it now */
 	pas->firmware = NULL;
 
+	if (pas->dtb_firmware) {
+		release_firmware(pas->dtb_firmware);
+		pas->dtb_firmware = NULL;
+	}
+
 	return 0;
 
 unmap_carveout:
 	qcom_pas_unmap_carveout(rproc, pas->mem_phys, pas->mem_size);
 release_pas_metadata:
 	qcom_scm_pas_metadata_release(pas->pas_ctx);
-	if (pas->dtb_pas_id)
-		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
-
 unmap_dtb_carveout:
 	if (pas->dtb_pas_id)
 		qcom_pas_unmap_carveout(rproc, pas->dtb_mem_phys, pas->dtb_mem_size);
@@ -376,6 +372,14 @@ static int qcom_pas_start(struct rproc *rproc)
 	qcom_pas_pds_disable(pas, pas->proxy_pds, pas->proxy_pd_count);
 disable_irqs:
 	qcom_q6v5_unprepare(&pas->q6v5);
+release_dtb_metadata:
+	if (pas->dtb_pas_id)
+		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
+release_dtb_firmware:
+	if (pas->dtb_firmware) {
+		release_firmware(pas->dtb_firmware);
+		pas->dtb_firmware = NULL;
+	}
 
 	/* firmware is used to pass reference from qcom_pas_start(), drop it now */
 	pas->firmware = NULL;
-- 
2.34.1

Re: [PATCH v2] remoteproc: qcom_q6v5_pas: Fix dtb firmware lifecycle and leak

[Mukesh Ojha]

On Wed, Jun 24, 2026 at 11:23:35PM +0530, Sailesh Nandanavanam wrote:
> DTB co-firmware was previously requested and loaded in
> qcom_pas_load(), but its lifetime did not match the actual
> start/stop lifecycle of the remoteproc. As a result, the firmware
> reference could be retained across restart cycles, leading to a
> leak for each successful boot.
> 
> Additionally, if qcom_pas_start() failed after loading the DTB
> firmware, the remoteproc core would not invoke .stop(), leaving
> no opportunity to release the associated firmware reference.
> 
> Fix this by moving DTB firmware request and loading into
> qcom_pas_start(), so that its lifetime is strictly tied to the
> remoteproc start sequence.
> 
> Update qcom_pas_start() to ensure proper cleanup on all paths:
> - release PAS metadata on failure
> - release DTB firmware on both success and failure paths
> - unmap DTB carveout where applicable
> 
> Remove DTB firmware handling from qcom_pas_load(), as it does not
> match the correct ownership and lifecycle model.
> 
> With this change, request_firmware() and release_firmware() are
> properly paired within the start path, avoiding leaks and ensuring
> consistent behavior across restart and failure scenarios.
> 
> Fixes: 29814986b82e ("remoteproc: qcom_q6v5_pas: add support for dtb co-firmware loading")
> Signed-off-by: Sailesh Nandanavanam <saileshnandanavanam@gmail.com>

Revision of the patch should be sent separately and not as reply
of the earlier revision.


> ---
> v2:
> - Move DTB firmware request/load from qcom_pas_load() to qcom_pas_start()
> - Fix firmware reference leak across restart cycles
> - Handle start() failure paths where .stop() is not invoked
> - Ensure firmware is released on all success and failure paths
> - Remove DTB handling from load() and drop release from stop()
> ---
>  drivers/remoteproc/qcom_q6v5_pas.c | 54 ++++++++++++++++--------------
>  1 file changed, 29 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index da27d1d3c9da..090f1f09dba3 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -232,28 +232,7 @@ static int qcom_pas_load(struct rproc *rproc, const struct firmware *fw)
>  	if (pas->lite_dtb_pas_id)
>  		qcom_scm_pas_shutdown(pas->lite_dtb_pas_id);
>  
> -	if (pas->dtb_pas_id) {
> -		ret = request_firmware(&pas->dtb_firmware, pas->dtb_firmware_name, pas->dev);
> -		if (ret) {
> -			dev_err(pas->dev, "request_firmware failed for %s: %d\n",
> -				pas->dtb_firmware_name, ret);
> -			return ret;
> -		}
> -
> -		ret = qcom_mdt_pas_load(pas->dtb_pas_ctx, pas->dtb_firmware,
> -					pas->dtb_firmware_name, pas->dtb_mem_region,
> -					&pas->dtb_mem_reloc);
> -		if (ret)
> -			goto release_dtb_metadata;
> -	}
> -
>  	return 0;
> -
> -release_dtb_metadata:
> -	qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
> -	release_firmware(pas->dtb_firmware);
> -
> -	return ret;
>  }
>  
>  static void qcom_pas_unmap_carveout(struct rproc *rproc, phys_addr_t mem_phys, size_t size)
> @@ -277,9 +256,24 @@ static int qcom_pas_start(struct rproc *rproc)
>  	struct qcom_pas *pas = rproc->priv;
>  	int ret;
>  
> +	if (pas->dtb_pas_id) {
> +		ret = request_firmware(&pas->dtb_firmware, pas->dtb_firmware_name, pas->dev);
> +		if (ret) {
> +			dev_err(pas->dev, "request_firmware failed for %s: %d\n",
> +					pas->dtb_firmware_name, ret);

Check the align of the line.


> +			return ret;
> +		}
> +
> +		ret = qcom_mdt_pas_load(pas->dtb_pas_ctx, pas->dtb_firmware,
> +				pas->dtb_firmware_name, pas->dtb_mem_region,
> +				&pas->dtb_mem_reloc);

Here, as well.

> +		if (ret)
> +			goto release_dtb_metadata;
> +	}
> +
>  	ret = qcom_q6v5_prepare(&pas->q6v5);
>  	if (ret)
> -		return ret;
> +		goto release_dtb_metadata;
>  
>  	ret = qcom_pas_pds_enable(pas, pas->proxy_pds, pas->proxy_pd_count);
>  	if (ret < 0)
> @@ -350,15 +344,17 @@ static int qcom_pas_start(struct rproc *rproc)
>  	/* firmware is used to pass reference from qcom_pas_start(), drop it now */
>  	pas->firmware = NULL;
>  
> +	if (pas->dtb_firmware) {
> +		release_firmware(pas->dtb_firmware);
> +		pas->dtb_firmware = NULL;
> +	}

you can put this under check pas->dtb_pas_id which is already
there.

> +
>  	return 0;
>  
>  unmap_carveout:
>  	qcom_pas_unmap_carveout(rproc, pas->mem_phys, pas->mem_size);
>  release_pas_metadata:
>  	qcom_scm_pas_metadata_release(pas->pas_ctx);
> -	if (pas->dtb_pas_id)
> -		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
> -
>  unmap_dtb_carveout:
>  	if (pas->dtb_pas_id)
>  		qcom_pas_unmap_carveout(rproc, pas->dtb_mem_phys, pas->dtb_mem_size);
> @@ -376,6 +372,14 @@ static int qcom_pas_start(struct rproc *rproc)
>  	qcom_pas_pds_disable(pas, pas->proxy_pds, pas->proxy_pd_count);
>  disable_irqs:
>  	qcom_q6v5_unprepare(&pas->q6v5);
> +release_dtb_metadata:
> +	if (pas->dtb_pas_id)
> +		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
> +release_dtb_firmware:

who is the user of this label ?


> +	if (pas->dtb_firmware) {

dtb_pas_id check should be fine here..


> +		release_firmware(pas->dtb_firmware);
> +		pas->dtb_firmware = NULL;
> +	}
>  
>  	/* firmware is used to pass reference from qcom_pas_start(), drop it now */
>  	pas->firmware = NULL;
> -- 
> 2.34.1
> 

-- 
-Mukesh Ojha