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

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

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