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

[Bjorn Andersson <andersson@kernel.org>]

On Mon, Aug 03, 2026 at 02:36:45PM +0530, Mukesh Ojha wrote:
> On Sat, Aug 01, 2026 at 11:08:30PM -0500, Bjorn Andersson wrote:
> > 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.
> 
> I had an expectation that Sumit's patches would get picked up completely
> somehow, which would avoid me adding the extra variable keep_mdt_buf to the
> temporary data structure qcom_scm_pas_context. That's the only dependency.
> 

How on earth do you expect me to figure that out!?

Regards,
Bjorn

> > 
> > > 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.
> 
> Was waiting for Sumit's patches to land so that I could convert
> every client to use PAS ctx.
> 
> > 
> > > +	 *
> > > +	 * 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".
> 
> Sure.
> 
> -- 
> -Mukesh Ojha