Re: [PATCH v5 6/6] firmware: qcom: scm: introduce keep_mdt_buf flag in PAS context

[Bjorn Andersson <andersson@kernel.org>]

On Fri, Jul 24, 2026 at 11:58:58PM +0530, Mukesh Ojha wrote:
> The PAS image initialization path always retains the metadata buffer
> when a valid qcom_scm_pas_context is provided, even if the caller does
> not require it. This implicit behavior leads to unclear buffer ownership
> and forces new users of qcom_mdt_pas_load() to manually release
> metadata, which is error‑ prone and incorrect.
> 
> Add a keep_mdt_buf flag to struct qcom_scm_pas_context to make metadata
> retention explicit.  Metadata buffers are now freed by default and are
> only preserved when this flag is set. qcom_q6v5_pas enables this during
> probe for contexts that require retained metadata for subsequent PAS
> operations, while existing callers continue to work unchanged.
> 

I presume given that this was sent together with the remoteproc patches
that this can't be merged until those other changes has made it into the
tree.

Please confirm if there is an actual dependency here.

> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
>  drivers/firmware/qcom/qcom_scm.c       | 18 +++++++++++++++---
>  drivers/remoteproc/qcom_q6v5_pas.c     |  3 +++
>  include/linux/firmware/qcom/qcom_pas.h |  1 +
>  3 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 16ae42e6c434..54ffec97cc26 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -625,7 +625,7 @@ static int qcom_scm_pas_prep_and_init_image(struct device *dev,
>  	mdata_phys = qcom_tzmem_to_phys(mdata_buf);
>  
>  	ret = __qcom_scm_pas_init_image(dev, ctx->pas_id, mdata_phys, &res);
> -	if (ret < 0)
> +	if (ret < 0 || !ctx->keep_mdt_buf)
>  		qcom_tzmem_free(mdata_buf);
>  	else
>  		ctx->ptr = mdata_buf;
> @@ -664,9 +664,21 @@ static int qcom_scm_pas_init_image(struct device *dev, u32 pas_id,
>  	memcpy(mdata_buf, metadata, size);
>  
>  	ret = __qcom_scm_pas_init_image(dev, pas_id, mdata_phys, &res);
> -	if (ret < 0 || !ctx) {
> +
> +	/*
> +	 * Some clients still pass the PAS context as NULL. Until all clients
> +	 * switch to qcom_mdt_pas_load() and provide a valid PAS context, check
> +	 * for NULL before dereferencing it.

Why don't we fix the problematic clients? There's no point in working
around such things.

> +	 *
> +	 * When a valid context is provided, keep_mdt_buf controls whether the
> +	 * metadata buffer is retained after PAS_INIT. PAS remoteproc subsystems
> +	 * set this flag so metadata persists until auth_and_reset() completes,
> +	 * as TrustZone keeps the buffers locked until then. Other callers leave
> +	 * it unset and metadata is freed immediately after the PAS_INIT call.

Please rewrite this to make it more succinct. It seems you could say the
same thing with just the words "free the metadata on error or if client
didn't request us to keep it".

Regards,
Bjorn

> +	 */
> +	if (ret < 0 || !ctx || !ctx->keep_mdt_buf) {
>  		dma_free_coherent(dev, size, mdata_buf, mdata_phys);
> -	} else if (ctx) {
> +	} else {
>  		ctx->ptr = mdata_buf;
>  		ctx->phys = mdata_phys;
>  		ctx->size = size;
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index 0391e30f42ba..38e6f6019825 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -722,6 +722,7 @@ static int qcom_pas_alloc_memory_region(struct qcom_pas *pas)
>  	if (IS_ERR(pas->pas_ctx))
>  		return PTR_ERR(pas->pas_ctx);
>  
> +	pas->pas_ctx->keep_mdt_buf = true;
>  	if (!pas->dtb_pas_id)
>  		return 0;
>  
> @@ -740,6 +741,8 @@ static int qcom_pas_alloc_memory_region(struct qcom_pas *pas)
>  	if (IS_ERR(pas->dtb_pas_ctx))
>  		return PTR_ERR(pas->dtb_pas_ctx);
>  
> +	pas->dtb_pas_ctx->keep_mdt_buf = true;
> +
>  	return 0;
>  }
>  
> diff --git a/include/linux/firmware/qcom/qcom_pas.h b/include/linux/firmware/qcom/qcom_pas.h
> index fb2ec3be6a16..1d132e89536e 100644
> --- a/include/linux/firmware/qcom/qcom_pas.h
> +++ b/include/linux/firmware/qcom/qcom_pas.h
> @@ -22,6 +22,7 @@ struct qcom_pas_context {
>  	dma_addr_t phys;
>  	ssize_t size;
>  	bool use_tzmem;
> +	bool keep_mdt_buf;
>  };
>  
>  static inline void __iomem *qcom_pas_ctx_map(struct qcom_pas_context *ctx)
> -- 
> 2.53.0
>