Re: [PATCH v3 03/12] firmware: qcom_scm: Introduce PAS context initialization and destroy helper

Bryan O'Donoghue
2025-09-21T21:40:59Z

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 <mukesh.ojha@oss.qualcomm.com>
> ---
> 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.

> +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 <bryan.odonoghue@linaro.org>

---
bod