linux-kernel - Re: [PATCH v3 04/12] soc: qcom: mdtloader: Add context aware qcom_mdt_pas

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8ec4743e-363c-414a-a309-7aec9b206c9d@kernel.org>
Date: Sun, 21 Sep 2025 22:49:57 +0100
From: Bryan O'Donoghue <bod@...nel.org>
To: Mukesh Ojha <mukesh.ojha@....qualcomm.com>,
 Bjorn Andersson <andersson@...nel.org>,
 Mathieu Poirier <mathieu.poirier@...aro.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Manivannan Sadhasivam <mani@...nel.org>,
 Konrad Dybcio <konradybcio@...nel.org>
Cc: linux-arm-msm@...r.kernel.org, linux-remoteproc@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 04/12] soc: qcom: mdtloader: Add context aware
 qcom_mdt_pas_load() helper

On 20/09/2025 20:41, Mukesh Ojha wrote:
> Currently, remoteproc and non-remoteproc subsystems use different
> variants of the MDT loader helper API, primarily due to the handling of
> the metadata context. Remoteproc subsystems retain this context until
> authentication and reset, while non-remoteproc subsystems (e.g., video,
> graphics) do not need to retain it and it is freed inside
> qcom_scm_pas_init() based on metadata context parameter being NULL.

The above paragraph seems almost 1:1 with the commit log of the previous 
patch.

You'd be better off describing in the commit log what you mean by 
context here. Not a thread context which is the common meaning of 
context. Instead of restating the paragraph from your previous commit, 
give the reader of your commit log new information - for example - what 
do you mean by context here ?

> Add context aware qcom_mdt_pas_load() function which uses context
> returned from qcom_scm_pas_ctx_init() and use it till subsystems
> is out of set. 

"Add a context function qcom_mdt_pas_load() which uses a context 
pointer/handle? return from qcom_scm_pas_init() then use that handle 
until? a subsystem is out of reset ?

Your commit log doesn't match the code below, which simply provides the 
API you will then subsequently build on.


This will also help in unifying the API used by
> remoteproc and non-remoteproc subsystems drivers in future and
> will also help in early setting context to all the PAS SMC function
> to do appropriate things when SoC is running with gunyah hypervisor
> or in absence of it.

Your argument for unification should go into the cover letter - this 
specific commit is adding a function - not performing that unification 
so in my opinion - as a reviewer I should see this argument for 
unification @ the cover-letter level and/or at the commit which 
implements the unification not the commit that adds the API to 
facilitate the unification.
> Signed-off-by: Mukesh Ojha <mukesh.ojha@....qualcomm.com>
> ---
>   drivers/soc/qcom/mdt_loader.c       | 15 +++++++++++++++
>   include/linux/soc/qcom/mdt_loader.h | 10 ++++++++++
>   2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> index a5c80d4fcc36..2ef079797f05 100644
> --- a/drivers/soc/qcom/mdt_loader.c
> +++ b/drivers/soc/qcom/mdt_loader.c
> @@ -486,5 +486,20 @@ int qcom_mdt_load_no_init(struct device *dev, const struct firmware *fw,
>   }
>   EXPORT_SYMBOL_GPL(qcom_mdt_load_no_init);
> 
> +int qcom_mdt_pas_load(struct qcom_scm_pas_ctx *ctx, const struct firmware *fw,
> +		      const char *firmware, void *mem_region, phys_addr_t *reloc_base)
> +{
> +	int ret;
> +
> +	ret = qcom_mdt_pas_init(ctx->dev, fw, firmware, ctx->pas_id,
> +				  ctx->mem_phys, ctx->metadata);
> +	if (ret)
> +		return ret;
> +
> +	return __qcom_mdt_load(ctx->dev, fw, firmware, mem_region, ctx->mem_phys,
> +			       ctx->mem_size, reloc_base);
> +}
> +EXPORT_SYMBOL_GPL(qcom_mdt_pas_load);
> +
>   MODULE_DESCRIPTION("Firmware parser for Qualcomm MDT format");
>   MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/soc/qcom/mdt_loader.h b/include/linux/soc/qcom/mdt_loader.h
> index 8ea8230579a2..36b8b331ce5f 100644
> --- a/include/linux/soc/qcom/mdt_loader.h
> +++ b/include/linux/soc/qcom/mdt_loader.h
> @@ -11,6 +11,7 @@
>   struct device;
>   struct firmware;
>   struct qcom_scm_pas_metadata;
> +struct qcom_scm_pas_ctx;
> 
>   #if IS_ENABLED(CONFIG_QCOM_MDT_LOADER)
> 
> @@ -23,6 +24,9 @@ int qcom_mdt_load(struct device *dev, const struct firmware *fw,
>   		  phys_addr_t mem_phys, size_t mem_size,
>   		  phys_addr_t *reloc_base);
> 
> +int qcom_mdt_pas_load(struct qcom_scm_pas_ctx *ctx, const struct firmware *fw,
> +		      const char *firmware, void *mem_region, phys_addr_t *reloc_base);
> +
>   int qcom_mdt_load_no_init(struct device *dev, const struct firmware *fw,
>   			  const char *fw_name, void *mem_region,
>   			  phys_addr_t mem_phys, size_t mem_size,
> @@ -52,6 +56,12 @@ static inline int qcom_mdt_load(struct device *dev, const struct firmware *fw,
>   	return -ENODEV;
>   }
> 
> +int qcom_mdt_pas_load(struct qcom_scm_pas_ctx *ctx, const struct firmware *fw,
> +		      const char *firmware, void *mem_region, phys_addr_t *reloc_base)
> +{
> +	return -ENODEV;
> +}
> +
>   static inline int qcom_mdt_load_no_init(struct device *dev,
>   					const struct firmware *fw,
>   					const char *fw_name, void *mem_region,
> 
> --
> 2.50.1
> 
>