Gentle ping on this, it's been about two weeks since v4. Let me know if any further changes are needed, or if there's anything I can clarify.
Thanks, Sailesh On Tue, Aug 4, 2026 at 11:50 PM Sailesh Nandanavanam <[email protected]> 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]> > --- > v4: > - Rebase onto linux-next > - Remove unused int ret from qcom_pas_load() > > v3: > - Remove the unused release_dtb_firmware label > - Release DTB firmware under the existing dtb_pas_id check > > 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 | 47 +++++++++++++++++------------- > 1 file changed, 26 insertions(+), 21 deletions(-) > > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c > b/drivers/remoteproc/qcom_q6v5_pas.c > index ca8e61254c44..864446a484f1 100644 > --- a/drivers/remoteproc/qcom_q6v5_pas.c > +++ b/drivers/remoteproc/qcom_q6v5_pas.c > @@ -231,7 +231,6 @@ static int qcom_pas_unprepare(struct rproc *rproc) > static int qcom_pas_load(struct rproc *rproc, const struct firmware *fw) > { > struct qcom_pas *pas = rproc->priv; > - int ret; > > /* Store firmware handle to be used in qcom_pas_start() */ > pas->firmware = fw; > @@ -241,23 +240,6 @@ static int qcom_pas_load(struct rproc *rproc, const > struct firmware *fw) > if (pas->lite_dtb_pas_id) > qcom_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_reloc); > - if (ret) { > - qcom_pas_metadata_release(pas->dtb_pas_ctx); > - release_firmware(pas->dtb_firmware); > - return ret; > - } > - } > - > return 0; > } > > @@ -282,9 +264,23 @@ 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_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) > @@ -352,6 +348,11 @@ static int qcom_pas_start(struct rproc *rproc) > if (pas->dtb_pas_id) > qcom_pas_metadata_release(pas->dtb_pas_ctx); > > + if (pas->dtb_pas_id) { > + 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; > > @@ -361,8 +362,6 @@ static int qcom_pas_start(struct rproc *rproc) > qcom_pas_unmap_carveout(rproc, pas->mem_phys, pas->mem_size); > release_pas_metadata: > qcom_pas_metadata_release(pas->pas_ctx); > - if (pas->dtb_pas_id) > - qcom_pas_metadata_release(pas->dtb_pas_ctx); > > unmap_dtb_carveout: > if (pas->dtb_pas_id) > @@ -381,6 +380,12 @@ 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_pas_metadata_release(pas->dtb_pas_ctx); > + 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 >
