On Sun, Sep 21, 2025 at 10:40:55PM +0100, Bryan O'Donoghue wrote:
> On 20/09/2025 20:41, Mukesh Ojha wrote:
> > When Secure Peripheral Authentication Service (PAS) method runs on a
> > SoC where Linux runs at EL2 (Gunyah absence) where reset sequences
>
> "i.e. runs without the Gynyah Hypervisor then, reset sequences"
>
> > move to EL3 and Linux need to do some extra stuff before calling PAS
> > SMC calls like creating SHMbridge. So, PAS SMC call need awareness and
> > need handling of things required when Linux run at EL2.
>
> "Therefore the PAS SMC call"
>
> >
> > Currently, remoteproc and non-remoteproc subsystems use different
>
> "Currently remoteproc"
>
> > variants of the MDT loader helper API, primarily due to the handling of
> > the metadata context. Remoteproc subsystems retain metadata context
> > until authentication and reset is done, while non-remoteproc subsystems
> > (e.g., video, graphics, ipa etc.) do not need to retain it and can free
>
> "do not need to retain metadata context"
>
> > the context right inside qcom_scm_pas_init() call based on passed context
> > parameter as NULL.
> >
> > So, in an attempt to unify the metadata API process for both remoteproc
>
> "In an attempt to unify"
>
> > and non-remoteproc subsystems and to make the SMC helper function
> > cleaner whether SoC running with Gunyah presence or absence by introducing
> > a dedicated PAS context initialization and destroy function. Context
> > initialization beforehand would help all SMC function to carry it and do
> > the right thing whether SoC is running with Gunyah presence or absence.
>
> Since you need to do another version of this patch re: below, please tidy up
> the commit log here a bit too.
>
> > Signed-off-by: Mukesh Ojha <[email protected]>
> > ---
> > drivers/firmware/qcom/qcom_scm.c | 53
> > ++++++++++++++++++++++++++++++++++
> > include/linux/firmware/qcom/qcom_scm.h | 11 +++++++
> > 2 files changed, 64 insertions(+)
> >
> > diff --git a/drivers/firmware/qcom/qcom_scm.c
> > b/drivers/firmware/qcom/qcom_scm.c
> > index 3379607eaf94..1c6b4c6f5513 100644
> > --- a/drivers/firmware/qcom/qcom_scm.c
> > +++ b/drivers/firmware/qcom/qcom_scm.c
> > @@ -558,6 +558,59 @@ static void qcom_scm_set_download_mode(u32 dload_mode)
> > dev_err(__scm->dev, "failed to set download mode: %d\n", ret);
> > }
> >
> > +/**
> > + * qcom_scm_pas_ctx_init() - Initialize peripheral authentication service
> > + * context for a given peripheral and it can be
> > + * destroyed with qcom_scm_pas_ctx_destroy() to
> > + * release the context
> > + *
> > + * @dev: PAS firmware device
> > + * @pas_id: peripheral authentication service id
> > + * @mem_phys: Subsystem reserve memory start address
> > + * @mem_size: Subsystem reserve memory size
> > + *
> > + * Upon successful, returns the PAS context or ERR_PTR() of the error
> > otherwise.
> > + */
> > +void *qcom_scm_pas_ctx_init(struct device *dev, u32 pas_id, phys_addr_t
> > mem_phys,
> > + size_t mem_size)
> > +{
> > + struct qcom_scm_pas_ctx *ctx;
> > +
> > + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> > + if (!ctx)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + ctx->dev = dev;
> > + ctx->pas_id = pas_id;
> > + ctx->mem_phys = mem_phys;
> > + ctx->mem_size = mem_size;
> > +
> > + ctx->metadata = kzalloc(sizeof(*ctx->metadata), GFP_KERNEL);
> > + if (!ctx->metadata) {
> > + kfree(ctx);
> > + return ERR_PTR(-ENOMEM);
> > + }
> > +
> > + return ctx;
> > +}
> > +EXPORT_SYMBOL_GPL(qcom_scm_pas_ctx_init);
> > +
> > +/**
> > + * qcom_scm_pas_ctx_destroy() - release PAS context
> > + * @ctx: PAS context
> > + */
> > +void qcom_scm_pas_ctx_destroy(struct qcom_scm_pas_ctx *ctx)
> > +{
> > + kfree(ctx->metadata);
> > + ctx->metadata = NULL;
> > + ctx->dev = NULL;
> > + ctx->pas_id = 0;
> > + ctx->mem_phys = 0;
> > + ctx->mem_size = 0;
> > + kfree(ctx);
> > +}
>
> This looks a bit strange, manually destructing an object you then free. I
> get the argument you might make about use-after-free but, I don't think this
> level of defensive coding is necessary.
I agreed with Pavan in my last version about adding destroy version of
it., otherwise, it looked a bit odd to just do init and forget and not
do corresponding destroy however, I do agree the only place we are going
to do in ->remove() but would not that look nicer to have _destroy() as well ?
>
> > +EXPORT_SYMBOL_GPL(qcom_scm_pas_ctx_destroy);
> > +
> > /**
> > * qcom_scm_pas_init_image() - Initialize peripheral authentication
> > service
> > * state machine for a given peripheral,
> > using the
> > diff --git a/include/linux/firmware/qcom/qcom_scm.h
> > b/include/linux/firmware/qcom/qcom_scm.h
> > index a13f703b16cd..e3e9e9e9077f 100644
> > --- a/include/linux/firmware/qcom/qcom_scm.h
> > +++ b/include/linux/firmware/qcom/qcom_scm.h
> > @@ -72,6 +72,17 @@ struct qcom_scm_pas_metadata {
> > ssize_t size;
> > };
> >
> > +struct qcom_scm_pas_ctx {
> > + struct device *dev;
> > + u32 pas_id;
> > + phys_addr_t mem_phys;
> > + size_t mem_size;
> > + struct qcom_scm_pas_metadata *metadata;
> > +};
> > +
> > +void *qcom_scm_pas_ctx_init(struct device *dev, u32 pas_id, phys_addr_t
> > mem_phys,
> > + size_t mem_size);
> > +void qcom_scm_pas_ctx_destroy(struct qcom_scm_pas_ctx *ctx);
> > int qcom_scm_pas_init_image(u32 pas_id, const void *metadata, size_t size,
> > struct qcom_scm_pas_metadata *ctx);
> > void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx);
> >
> > --
> > 2.50.1
> >
> >
>
> Once fixed.
>
> Reviewed-by: Bryan O'Donoghue <[email protected]>
>
> ---
> bod
--
-Mukesh Ojha
