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 <[email protected]>
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
