Qualcomm SoCs running with QHEE (Qualcomm Hypervisor Execution
Environment—a library present in the Gunyah hypervisor) utilize the
Peripheral Authentication Service (PAS) from Qualcomm TrustZone (TZ)
also called QTEE(Qualcomm Trusted Execution Environment firmware)
to securely authenticate and reset remote processors via a sequence
of SMC calls such as qcom_scm_pas_init_image(), qcom_scm_pas_mem_setup(),
and qcom_scm_pas_auth_and_reset().

For memory passed to Qualcomm TrustZone, it must either be part of a
pool registered with TZ or be directly registered via SHMbridge SMC
calls.

When QHEE is present, PAS SMC calls from Linux running at EL1 are
trapped by QHEE (running at EL2), which then creates or retrieves memory
from the SHM bridge for both metadata and remoteproc carveout memory
before passing them to TZ. However, when the SoC runs with a
non-QHEE-based hypervisor, Linux must create the SHM bridge for both
metadata (before it is passed to TZ in qcom_scm_pas_init_image()) and
for remoteproc memory (before the call is made to TZ in
qcom_scm_pas_auth_and_reset()).

For the qcom_scm_pas_init_image() call, metadata content must be copied
to a buffer allocated from the SHM bridge before making the SMC call.
This buffer should be freed either immediately after the call or during
the qcom_scm_pas_metadata_release() function, depending on the context
parameter passed to qcom_scm_pas_init_image(). Convert the metadata
context parameter to use PAS context data structure so that it will also
be possible to decide whether to get memory from SHMbridge pool or not.

When QHEE is present, it manages the IOMMU translation context so, in
absence of it device driver will be aware through device tree that its
translation context is managed by Linux and it need to create SHMbridge
before passing any buffer to TZ, So, remote processor driver should
appropriately set ctx->has_iommu to let PAS SMC function to take care of
everything ready for the call to work.

Lets convert qcom_scm_pas_init_image() and qcom_scm_pas_metadata_release()
to have these awareness.

Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
---
 drivers/firmware/qcom/qcom_scm.c       | 71 +++++++++++++++++++++-----
 drivers/remoteproc/qcom_q6v5_pas.c     | 14 ++---
 drivers/soc/qcom/mdt_loader.c          |  4 +-
 include/linux/firmware/qcom/qcom_scm.h |  9 ++--
 4 files changed, 73 insertions(+), 25 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 7827699e277c..301d440f62f3 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -616,6 +616,35 @@ static int __qcom_scm_pas_init_image(u32 peripheral, dma_addr_t mdata_phys,
 	return ret;
 }
 
+static int qcom_scm_pas_prep_and_init_image(struct qcom_scm_pas_ctx *ctx,
+					    const void *metadata, size_t size)
+{
+	struct qcom_scm_pas_metadata *mdt_ctx;
+	struct qcom_scm_res res;
+	phys_addr_t mdata_phys;
+	void *mdata_buf;
+	int ret;
+
+	mdt_ctx = ctx->metadata;
+	mdata_buf = qcom_tzmem_alloc(__scm->mempool, size, GFP_KERNEL);
+	if (!mdata_buf)
+		return -ENOMEM;
+
+	memcpy(mdata_buf, metadata, size);
+	mdata_phys = qcom_tzmem_to_phys(mdata_buf);
+
+	ret = __qcom_scm_pas_init_image(ctx->peripheral, mdata_phys, mdata_buf, size, &res);
+	if (ret < 0 || !mdt_ctx) {
+		qcom_tzmem_free(mdata_buf);
+	} else if (mdt_ctx) {
+		mdt_ctx->ptr = mdata_buf;
+		mdt_ctx->addr.phys_addr = mdata_phys;
+		mdt_ctx->size = size;
+	}
+
+	return ret ? : res.result[0];
+}
+
 /**
  * qcom_scm_pas_init_image() - Initialize peripheral authentication service
  *			       state machine for a given peripheral, using the
@@ -625,7 +654,7 @@ static int __qcom_scm_pas_init_image(u32 peripheral, dma_addr_t mdata_phys,
  *		and optional blob of data used for authenticating the metadata
  *		and the rest of the firmware
  * @size:	size of the metadata
- * @ctx:	optional metadata context
+ * @ctx:	optional pas context
  *
  * Return: 0 on success.
  *
@@ -634,13 +663,19 @@ static int __qcom_scm_pas_init_image(u32 peripheral, dma_addr_t mdata_phys,
  * qcom_scm_pas_metadata_release() by the caller.
  */
 int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
-			    struct qcom_scm_pas_metadata *ctx)
+			    struct qcom_scm_pas_ctx *ctx)
 {
+	struct qcom_scm_pas_metadata *mdt_ctx;
 	struct qcom_scm_res res;
 	dma_addr_t mdata_phys;
 	void *mdata_buf;
 	int ret;
 
+	if (ctx && ctx->has_iommu) {
+		ret = qcom_scm_pas_prep_and_init_image(ctx, metadata, size);
+		return ret;
+	}
+
 	/*
 	 * During the scm call memory protection will be enabled for the meta
 	 * data blob, so make sure it's physically contiguous, 4K aligned and
@@ -663,10 +698,11 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
 	ret = __qcom_scm_pas_init_image(peripheral, mdata_phys, mdata_buf, size, &res);
 	if (ret < 0 || !ctx) {
 		dma_free_coherent(__scm->dev, size, mdata_buf, mdata_phys);
-	} else if (ctx) {
-		ctx->ptr = mdata_buf;
-		ctx->phys = mdata_phys;
-		ctx->size = size;
+	} else if (ctx->metadata) {
+		mdt_ctx = ctx->metadata;
+		mdt_ctx->ptr = mdata_buf;
+		mdt_ctx->addr.dma_addr = mdata_phys;
+		mdt_ctx->size = size;
 	}
 
 	return ret ? : res.result[0];
@@ -675,18 +711,27 @@ EXPORT_SYMBOL_GPL(qcom_scm_pas_init_image);
 
 /**
  * qcom_scm_pas_metadata_release() - release metadata context
- * @ctx:	metadata context
+ * @ctx:	pas context
  */
-void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx)
+void qcom_scm_pas_metadata_release(struct qcom_scm_pas_ctx *ctx)
 {
-	if (!ctx->ptr)
+	struct qcom_scm_pas_metadata *mdt_ctx;
+
+	mdt_ctx = ctx->metadata;
+	if (!mdt_ctx->ptr)
 		return;
 
-	dma_free_coherent(__scm->dev, ctx->size, ctx->ptr, ctx->phys);
+	if (ctx->has_iommu) {
+		qcom_tzmem_free(mdt_ctx->ptr);
+		mdt_ctx->addr.phys_addr = 0;
+	} else {
+		dma_free_coherent(__scm->dev, mdt_ctx->size, mdt_ctx->ptr,
+				  mdt_ctx->addr.dma_addr);
+		mdt_ctx->addr.dma_addr = 0;
+	}
 
-	ctx->ptr = NULL;
-	ctx->phys = 0;
-	ctx->size = 0;
+	mdt_ctx->ptr = NULL;
+	mdt_ctx->size = 0;
 }
 EXPORT_SYMBOL_GPL(qcom_scm_pas_metadata_release);
 
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index e376c0338576..09cada92dfd5 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -209,9 +209,9 @@ static int qcom_pas_unprepare(struct rproc *rproc)
 	 * auth_and_reset() was successful, but in other cases clean it up
 	 * here.
 	 */
-	qcom_scm_pas_metadata_release(pas->pas_ctx->metadata);
+	qcom_scm_pas_metadata_release(pas->pas_ctx);
 	if (pas->dtb_pas_id)
-		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
+		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
 
 	return 0;
 }
@@ -244,7 +244,7 @@ static int qcom_pas_load(struct rproc *rproc, const struct firmware *fw)
 	return 0;
 
 release_dtb_metadata:
-	qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
+	qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
 	release_firmware(pas->dtb_firmware);
 
 	return ret;
@@ -313,9 +313,9 @@ static int qcom_pas_start(struct rproc *rproc)
 		goto release_pas_metadata;
 	}
 
-	qcom_scm_pas_metadata_release(pas->pas_ctx->metadata);
+	qcom_scm_pas_metadata_release(pas->pas_ctx);
 	if (pas->dtb_pas_id)
-		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
+		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
 
 	/* firmware is used to pass reference from qcom_pas_start(), drop it now */
 	pas->firmware = NULL;
@@ -323,9 +323,9 @@ static int qcom_pas_start(struct rproc *rproc)
 	return 0;
 
 release_pas_metadata:
-	qcom_scm_pas_metadata_release(pas->pas_ctx->metadata);
+	qcom_scm_pas_metadata_release(pas->pas_ctx);
 	if (pas->dtb_pas_id)
-		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
+		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
 disable_px_supply:
 	if (pas->px_supply)
 		regulator_disable(pas->px_supply);
diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index 509ff85d9bf6..a1718db91b3e 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -240,7 +240,7 @@ EXPORT_SYMBOL_GPL(qcom_mdt_read_metadata);
  */
 static int __qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
 			       const char *fw_name, int pas_id, phys_addr_t mem_phys,
-			       struct qcom_scm_pas_metadata *ctx)
+			       struct qcom_scm_pas_ctx *ctx)
 {
 	const struct elf32_phdr *phdrs;
 	const struct elf32_phdr *phdr;
@@ -491,7 +491,7 @@ int qcom_mdt_pas_load(struct qcom_scm_pas_ctx *ctx, const struct firmware *fw,
 	int ret;
 
 	ret = __qcom_mdt_pas_init(ctx->dev, fw, firmware, ctx->peripheral,
-				  ctx->mem_phys, ctx->metadata);
+				  ctx->mem_phys, ctx);
 	if (ret)
 		return ret;
 
diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
index a31006fe49a9..bd3417d9c3f9 100644
--- a/include/linux/firmware/qcom/qcom_scm.h
+++ b/include/linux/firmware/qcom/qcom_scm.h
@@ -68,7 +68,10 @@ int qcom_scm_set_remote_state(u32 state, u32 id);
 
 struct qcom_scm_pas_metadata {
 	void *ptr;
-	dma_addr_t phys;
+	union {
+		dma_addr_t dma_addr;
+		phys_addr_t phys_addr;
+	} addr;
 	ssize_t size;
 };
 
@@ -85,8 +88,8 @@ struct qcom_scm_pas_ctx {
 void *qcom_scm_pas_ctx_init(struct device *dev, u32 peripheral, phys_addr_t mem_phys,
 			    size_t mem_size, bool save_mdt_ctx);
 int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
-			    struct qcom_scm_pas_metadata *ctx);
-void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx);
+			    struct qcom_scm_pas_ctx *ctx);
+void qcom_scm_pas_metadata_release(struct qcom_scm_pas_ctx *ctx);
 int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size);
 int qcom_scm_pas_prepare_and_auth_reset(struct qcom_scm_pas_ctx *ctx);
 int qcom_scm_pas_auth_and_reset(u32 peripheral);
-- 
2.50.1

On 19/08/2025 17:54, Mukesh Ojha wrote:
> Qualcomm SoCs running with QHEE (Qualcomm Hypervisor Execution
> Environment—a library present in the Gunyah hypervisor) utilize the
> Peripheral Authentication Service (PAS) from Qualcomm TrustZone (TZ)
> also called QTEE(Qualcomm Trusted Execution Environment firmware)
> to securely authenticate and reset remote processors via a sequence
> of SMC calls such as qcom_scm_pas_init_image(), qcom_scm_pas_mem_setup(),
> and qcom_scm_pas_auth_and_reset().
> 
> For memory passed to Qualcomm TrustZone, it must either be part of a
> pool registered with TZ or be directly registered via SHMbridge SMC
> calls.
> 
> When QHEE is present, PAS SMC calls from Linux running at EL1 are
> trapped by QHEE (running at EL2), which then creates or retrieves memory
> from the SHM bridge for both metadata and remoteproc carveout memory
> before passing them to TZ. However, when the SoC runs with a
> non-QHEE-based hypervisor, Linux must create the SHM bridge for both
> metadata (before it is passed to TZ in qcom_scm_pas_init_image()) and
> for remoteproc memory (before the call is made to TZ in
> qcom_scm_pas_auth_and_reset()).
> 
> For the qcom_scm_pas_init_image() call, metadata content must be copied
> to a buffer allocated from the SHM bridge before making the SMC call.
> This buffer should be freed either immediately after the call or during
> the qcom_scm_pas_metadata_release() function, depending on the context
> parameter passed to qcom_scm_pas_init_image(). Convert the metadata
> context parameter to use PAS context data structure so that it will also
> be possible to decide whether to get memory from SHMbridge pool or not.
> 
> When QHEE is present, it manages the IOMMU translation context so, in
> absence of it device driver will be aware through device tree that its
> translation context is managed by Linux and it need to create SHMbridge
> before passing any buffer to TZ, So, remote processor driver should
> appropriately set ctx->has_iommu to let PAS SMC function to take care of
> everything ready for the call to work.
> 
> Lets convert qcom_scm_pas_init_image() and qcom_scm_pas_metadata_release()
> to have these awareness.

I like the effort in the commit log here but its also a bit too long.

Please go through these paragraphs and try to reduce down the amount of 
text you are generating.

> 
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
>   drivers/firmware/qcom/qcom_scm.c       | 71 +++++++++++++++++++++-----
>   drivers/remoteproc/qcom_q6v5_pas.c     | 14 ++---
>   drivers/soc/qcom/mdt_loader.c          |  4 +-
>   include/linux/firmware/qcom/qcom_scm.h |  9 ++--
>   4 files changed, 73 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 7827699e277c..301d440f62f3 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -616,6 +616,35 @@ static int __qcom_scm_pas_init_image(u32 peripheral, dma_addr_t mdata_phys,
>   	return ret;
>   }
>   
> +static int qcom_scm_pas_prep_and_init_image(struct qcom_scm_pas_ctx *ctx,
> +					    const void *metadata, size_t size)
> +{
> +	struct qcom_scm_pas_metadata *mdt_ctx;
> +	struct qcom_scm_res res;
> +	phys_addr_t mdata_phys;
> +	void *mdata_buf;
> +	int ret;
> +
> +	mdt_ctx = ctx->metadata;
> +	mdata_buf = qcom_tzmem_alloc(__scm->mempool, size, GFP_KERNEL);
> +	if (!mdata_buf)
> +		return -ENOMEM;
> +
> +	memcpy(mdata_buf, metadata, size);
> +	mdata_phys = qcom_tzmem_to_phys(mdata_buf);
> +
> +	ret = __qcom_scm_pas_init_image(ctx->peripheral, mdata_phys, mdata_buf, size, &res);
> +	if (ret < 0 || !mdt_ctx) {

if ret is an error or mdt_ctx is null free the memory

> +		qcom_tzmem_free(mdata_buf);
> +	} else if (mdt_ctx) {

if mdt_ctx is valid do this

> +		mdt_ctx->ptr = mdata_buf;
> +		mdt_ctx->addr.phys_addr = mdata_phys;
> +		mdt_ctx->size = size;
> +	}
> +
> +	return ret ? : res.result[0];

so we can have ctx_mtd valid but return the value at ret but also mtd 
valid and return the res.result[0]

That seems like an odd choice - surely if you are enumerating the 
data-structure the result code we care about is res.result[0] instead of 
ret ?

OK I see this return logic comes from below..

But

drivers/soc/qcom/mdt_loader.c::qcom_mdt_pas_init

ret = qcom_scm_pas_init_image(pas_id, metadata, metadata_len, ctx);
kfree(metadata);
if (ret) {
     /* Invalid firmware metadata */
     dev_err(dev, "error %d initializing firmware %s\n", ret, fw_name);
     goto out;
}

So if ret as returned from your function is > 0 you will leak the memory 
allocated @ mdata_buf ..

Do you expect something else to come along and call 
qcom_scm_pas_metadata_release() ?

> +}
> +
>   /**
>    * qcom_scm_pas_init_image() - Initialize peripheral authentication service
>    *			       state machine for a given peripheral, using the
> @@ -625,7 +654,7 @@ static int __qcom_scm_pas_init_image(u32 peripheral, dma_addr_t mdata_phys,
>    *		and optional blob of data used for authenticating the metadata
>    *		and the rest of the firmware
>    * @size:	size of the metadata
> - * @ctx:	optional metadata context
> + * @ctx:	optional pas context
>    *
>    * Return: 0 on success.
>    *
> @@ -634,13 +663,19 @@ static int __qcom_scm_pas_init_image(u32 peripheral, dma_addr_t mdata_phys,
>    * qcom_scm_pas_metadata_release() by the caller.
>    */
>   int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
> -			    struct qcom_scm_pas_metadata *ctx)
> +			    struct qcom_scm_pas_ctx *ctx)
>   {
> +	struct qcom_scm_pas_metadata *mdt_ctx;
>   	struct qcom_scm_res res;
>   	dma_addr_t mdata_phys;
>   	void *mdata_buf;
>   	int ret;
>   
> +	if (ctx && ctx->has_iommu) {
> +		ret = qcom_scm_pas_prep_and_init_image(ctx, metadata, size);
> +		return ret;
> +	}
> +
>   	/*
>   	 * During the scm call memory protection will be enabled for the meta
>   	 * data blob, so make sure it's physically contiguous, 4K aligned and
> @@ -663,10 +698,11 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
>   	ret = __qcom_scm_pas_init_image(peripheral, mdata_phys, mdata_buf, size, &res);
>   	if (ret < 0 || !ctx) {
>   		dma_free_coherent(__scm->dev, size, mdata_buf, mdata_phys);
> -	} else if (ctx) {
> -		ctx->ptr = mdata_buf;
> -		ctx->phys = mdata_phys;
> -		ctx->size = size;
> +	} else if (ctx->metadata) {
> +		mdt_ctx = ctx->metadata;
> +		mdt_ctx->ptr = mdata_buf;
> +		mdt_ctx->addr.dma_addr = mdata_phys;
> +		mdt_ctx->size = size;
>   	}
>   
>   	return ret ? : res.result[0];

is this return path still valid now that you've functionally decomposed 
into qcom_sm_pas_prep_and_init ?

> @@ -675,18 +711,27 @@ EXPORT_SYMBOL_GPL(qcom_scm_pas_init_image);
>   
>   /**
>    * qcom_scm_pas_metadata_release() - release metadata context
> - * @ctx:	metadata context
> + * @ctx:	pas context
>    */
> -void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx)
> +void qcom_scm_pas_metadata_release(struct qcom_scm_pas_ctx *ctx)
>   {
> -	if (!ctx->ptr)
> +	struct qcom_scm_pas_metadata *mdt_ctx;
> +
> +	mdt_ctx = ctx->metadata;
> +	if (!mdt_ctx->ptr)
>   		return;
>   
> -	dma_free_coherent(__scm->dev, ctx->size, ctx->ptr, ctx->phys);
> +	if (ctx->has_iommu) {
> +		qcom_tzmem_free(mdt_ctx->ptr);
> +		mdt_ctx->addr.phys_addr = 0;
> +	} else {
> +		dma_free_coherent(__scm->dev, mdt_ctx->size, mdt_ctx->ptr,
> +				  mdt_ctx->addr.dma_addr);
> +		mdt_ctx->addr.dma_addr = 0;
> +	}
>   
> -	ctx->ptr = NULL;
> -	ctx->phys = 0;
> -	ctx->size = 0;
> +	mdt_ctx->ptr = NULL;
> +	mdt_ctx->size = 0;
>   }
>   EXPORT_SYMBOL_GPL(qcom_scm_pas_metadata_release);
>   
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index e376c0338576..09cada92dfd5 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -209,9 +209,9 @@ static int qcom_pas_unprepare(struct rproc *rproc)
>   	 * auth_and_reset() was successful, but in other cases clean it up
>   	 * here.
>   	 */
> -	qcom_scm_pas_metadata_release(pas->pas_ctx->metadata);
> +	qcom_scm_pas_metadata_release(pas->pas_ctx);
>   	if (pas->dtb_pas_id)
> -		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> +		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
>   
>   	return 0;
>   }
> @@ -244,7 +244,7 @@ static int qcom_pas_load(struct rproc *rproc, const struct firmware *fw)
>   	return 0;
>   
>   release_dtb_metadata:
> -	qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> +	qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
>   	release_firmware(pas->dtb_firmware);
>   
>   	return ret;
> @@ -313,9 +313,9 @@ static int qcom_pas_start(struct rproc *rproc)
>   		goto release_pas_metadata;
>   	}
>   
> -	qcom_scm_pas_metadata_release(pas->pas_ctx->metadata);
> +	qcom_scm_pas_metadata_release(pas->pas_ctx);
>   	if (pas->dtb_pas_id)
> -		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> +		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
>   
>   	/* firmware is used to pass reference from qcom_pas_start(), drop it now */
>   	pas->firmware = NULL;
> @@ -323,9 +323,9 @@ static int qcom_pas_start(struct rproc *rproc)
>   	return 0;
>   
>   release_pas_metadata:
> -	qcom_scm_pas_metadata_release(pas->pas_ctx->metadata);
> +	qcom_scm_pas_metadata_release(pas->pas_ctx);
>   	if (pas->dtb_pas_id)
> -		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> +		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
>   disable_px_supply:
>   	if (pas->px_supply)
>   		regulator_disable(pas->px_supply);
> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> index 509ff85d9bf6..a1718db91b3e 100644
> --- a/drivers/soc/qcom/mdt_loader.c
> +++ b/drivers/soc/qcom/mdt_loader.c
> @@ -240,7 +240,7 @@ EXPORT_SYMBOL_GPL(qcom_mdt_read_metadata);
>    */
>   static int __qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
>   			       const char *fw_name, int pas_id, phys_addr_t mem_phys,
> -			       struct qcom_scm_pas_metadata *ctx)
> +			       struct qcom_scm_pas_ctx *ctx)
>   {
>   	const struct elf32_phdr *phdrs;
>   	const struct elf32_phdr *phdr;
> @@ -491,7 +491,7 @@ int qcom_mdt_pas_load(struct qcom_scm_pas_ctx *ctx, const struct firmware *fw,
>   	int ret;
>   
>   	ret = __qcom_mdt_pas_init(ctx->dev, fw, firmware, ctx->peripheral,
> -				  ctx->mem_phys, ctx->metadata);
> +				  ctx->mem_phys, ctx);
>   	if (ret)
>   		return ret;
>   
> diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
> index a31006fe49a9..bd3417d9c3f9 100644
> --- a/include/linux/firmware/qcom/qcom_scm.h
> +++ b/include/linux/firmware/qcom/qcom_scm.h
> @@ -68,7 +68,10 @@ int qcom_scm_set_remote_state(u32 state, u32 id);
>   
>   struct qcom_scm_pas_metadata {
>   	void *ptr;
> -	dma_addr_t phys;
> +	union {
> +		dma_addr_t dma_addr;
> +		phys_addr_t phys_addr;
> +	} addr;
>   	ssize_t size;
>   };
>   
> @@ -85,8 +88,8 @@ struct qcom_scm_pas_ctx {
>   void *qcom_scm_pas_ctx_init(struct device *dev, u32 peripheral, phys_addr_t mem_phys,
>   			    size_t mem_size, bool save_mdt_ctx);
>   int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
> -			    struct qcom_scm_pas_metadata *ctx);
> -void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx);
> +			    struct qcom_scm_pas_ctx *ctx);
> +void qcom_scm_pas_metadata_release(struct qcom_scm_pas_ctx *ctx);
>   int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size);
>   int qcom_scm_pas_prepare_and_auth_reset(struct qcom_scm_pas_ctx *ctx);
>   int qcom_scm_pas_auth_and_reset(u32 peripheral);

Please review the error paths here especially WRT to qcom_mdt_pas_init();

---
bod

On Thu, Aug 21, 2025 at 04:23:53PM +0100, Bryan O'Donoghue wrote:
> On 19/08/2025 17:54, Mukesh Ojha wrote:
> > Qualcomm SoCs running with QHEE (Qualcomm Hypervisor Execution
> > Environment—a library present in the Gunyah hypervisor) utilize the
> > Peripheral Authentication Service (PAS) from Qualcomm TrustZone (TZ)
> > also called QTEE(Qualcomm Trusted Execution Environment firmware)
> > to securely authenticate and reset remote processors via a sequence
> > of SMC calls such as qcom_scm_pas_init_image(), qcom_scm_pas_mem_setup(),
> > and qcom_scm_pas_auth_and_reset().
> > 
> > For memory passed to Qualcomm TrustZone, it must either be part of a
> > pool registered with TZ or be directly registered via SHMbridge SMC
> > calls.
> > 
> > When QHEE is present, PAS SMC calls from Linux running at EL1 are
> > trapped by QHEE (running at EL2), which then creates or retrieves memory
> > from the SHM bridge for both metadata and remoteproc carveout memory
> > before passing them to TZ. However, when the SoC runs with a
> > non-QHEE-based hypervisor, Linux must create the SHM bridge for both
> > metadata (before it is passed to TZ in qcom_scm_pas_init_image()) and
> > for remoteproc memory (before the call is made to TZ in
> > qcom_scm_pas_auth_and_reset()).
> > 
> > For the qcom_scm_pas_init_image() call, metadata content must be copied
> > to a buffer allocated from the SHM bridge before making the SMC call.
> > This buffer should be freed either immediately after the call or during
> > the qcom_scm_pas_metadata_release() function, depending on the context
> > parameter passed to qcom_scm_pas_init_image(). Convert the metadata
> > context parameter to use PAS context data structure so that it will also
> > be possible to decide whether to get memory from SHMbridge pool or not.
> > 
> > When QHEE is present, it manages the IOMMU translation context so, in
> > absence of it device driver will be aware through device tree that its
> > translation context is managed by Linux and it need to create SHMbridge
> > before passing any buffer to TZ, So, remote processor driver should
> > appropriately set ctx->has_iommu to let PAS SMC function to take care of
> > everything ready for the call to work.
> > 
> > Lets convert qcom_scm_pas_init_image() and qcom_scm_pas_metadata_release()
> > to have these awareness.
> 
> I like the effort in the commit log here but its also a bit too long.
> 
> Please go through these paragraphs and try to reduce down the amount of text
> you are generating.

I was writing to set context for each commit and for the record and hence, the
repetition of text you would see in some of the lines used.

I will take your suggestion and reduce it.

> 
> > 
> > Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> > ---
> >   drivers/firmware/qcom/qcom_scm.c       | 71 +++++++++++++++++++++-----
> >   drivers/remoteproc/qcom_q6v5_pas.c     | 14 ++---
> >   drivers/soc/qcom/mdt_loader.c          |  4 +-
> >   include/linux/firmware/qcom/qcom_scm.h |  9 ++--
> >   4 files changed, 73 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> > index 7827699e277c..301d440f62f3 100644
> > --- a/drivers/firmware/qcom/qcom_scm.c
> > +++ b/drivers/firmware/qcom/qcom_scm.c
> > @@ -616,6 +616,35 @@ static int __qcom_scm_pas_init_image(u32 peripheral, dma_addr_t mdata_phys,
> >   	return ret;
> >   }
> > +static int qcom_scm_pas_prep_and_init_image(struct qcom_scm_pas_ctx *ctx,
> > +					    const void *metadata, size_t size)
> > +{
> > +	struct qcom_scm_pas_metadata *mdt_ctx;
> > +	struct qcom_scm_res res;
> > +	phys_addr_t mdata_phys;
> > +	void *mdata_buf;
> > +	int ret;
> > +
> > +	mdt_ctx = ctx->metadata;
> > +	mdata_buf = qcom_tzmem_alloc(__scm->mempool, size, GFP_KERNEL);
> > +	if (!mdata_buf)
> > +		return -ENOMEM;
> > +
> > +	memcpy(mdata_buf, metadata, size);
> > +	mdata_phys = qcom_tzmem_to_phys(mdata_buf);
> > +
> > +	ret = __qcom_scm_pas_init_image(ctx->peripheral, mdata_phys, mdata_buf, size, &res);
> > +	if (ret < 0 || !mdt_ctx) {
> 
> if ret is an error or mdt_ctx is null free the memory
> 
> > +		qcom_tzmem_free(mdata_buf);
> > +	} else if (mdt_ctx) {
> 
> if mdt_ctx is valid do this

Nothing change, it is similar to the earlier code.

> 
> > +		mdt_ctx->ptr = mdata_buf;
> > +		mdt_ctx->addr.phys_addr = mdata_phys;
> > +		mdt_ctx->size = size;
> > +	}
> > +
> > +	return ret ? : res.result[0];
> 
> so we can have ctx_mtd valid but return the value at ret but also mtd valid
> and return the res.result[0]
> 
> That seems like an odd choice - surely if you are enumerating the
> data-structure the result code we care about is res.result[0] instead of ret
> ?
> 
> OK I see this return logic comes from below..
> 
> But
> 
> drivers/soc/qcom/mdt_loader.c::qcom_mdt_pas_init
> 
> ret = qcom_scm_pas_init_image(pas_id, metadata, metadata_len, ctx);
> kfree(metadata);
> if (ret) {
>     /* Invalid firmware metadata */
>     dev_err(dev, "error %d initializing firmware %s\n", ret, fw_name);
>     goto out;
> }
> 
> So if ret as returned from your function is > 0 you will leak the memory
> allocated @ mdata_buf ..
> 
> Do you expect something else to come along and call
> qcom_scm_pas_metadata_release() ?

You just identified a bug in the existing code where qcom_mdt_pas_init()
does not call qcom_scm_pas_metadata_release() for firmware image for
failure case from qcom_q6v5_pas().


> 
> > +}
> > +
> >   /**
> >    * qcom_scm_pas_init_image() - Initialize peripheral authentication service
> >    *			       state machine for a given peripheral, using the
> > @@ -625,7 +654,7 @@ static int __qcom_scm_pas_init_image(u32 peripheral, dma_addr_t mdata_phys,
> >    *		and optional blob of data used for authenticating the metadata
> >    *		and the rest of the firmware
> >    * @size:	size of the metadata
> > - * @ctx:	optional metadata context
> > + * @ctx:	optional pas context
> >    *
> >    * Return: 0 on success.
> >    *
> > @@ -634,13 +663,19 @@ static int __qcom_scm_pas_init_image(u32 peripheral, dma_addr_t mdata_phys,
> >    * qcom_scm_pas_metadata_release() by the caller.
> >    */
> >   int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
> > -			    struct qcom_scm_pas_metadata *ctx)
> > +			    struct qcom_scm_pas_ctx *ctx)
> >   {
> > +	struct qcom_scm_pas_metadata *mdt_ctx;
> >   	struct qcom_scm_res res;
> >   	dma_addr_t mdata_phys;
> >   	void *mdata_buf;
> >   	int ret;
> > +	if (ctx && ctx->has_iommu) {
> > +		ret = qcom_scm_pas_prep_and_init_image(ctx, metadata, size);
> > +		return ret;
> > +	}
> > +
> >   	/*
> >   	 * During the scm call memory protection will be enabled for the meta
> >   	 * data blob, so make sure it's physically contiguous, 4K aligned and
> > @@ -663,10 +698,11 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
> >   	ret = __qcom_scm_pas_init_image(peripheral, mdata_phys, mdata_buf, size, &res);
> >   	if (ret < 0 || !ctx) {
> >   		dma_free_coherent(__scm->dev, size, mdata_buf, mdata_phys);
> > -	} else if (ctx) {
> > -		ctx->ptr = mdata_buf;
> > -		ctx->phys = mdata_phys;
> > -		ctx->size = size;
> > +	} else if (ctx->metadata) {
> > +		mdt_ctx = ctx->metadata;
> > +		mdt_ctx->ptr = mdata_buf;
> > +		mdt_ctx->addr.dma_addr = mdata_phys;
> > +		mdt_ctx->size = size;
> >   	}
> >   	return ret ? : res.result[0];
> 
> is this return path still valid now that you've functionally decomposed into
> qcom_sm_pas_prep_and_init ?
> 
> > @@ -675,18 +711,27 @@ EXPORT_SYMBOL_GPL(qcom_scm_pas_init_image);
> >   /**
> >    * qcom_scm_pas_metadata_release() - release metadata context
> > - * @ctx:	metadata context
> > + * @ctx:	pas context
> >    */
> > -void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx)
> > +void qcom_scm_pas_metadata_release(struct qcom_scm_pas_ctx *ctx)
> >   {
> > -	if (!ctx->ptr)
> > +	struct qcom_scm_pas_metadata *mdt_ctx;
> > +
> > +	mdt_ctx = ctx->metadata;
> > +	if (!mdt_ctx->ptr)
> >   		return;
> > -	dma_free_coherent(__scm->dev, ctx->size, ctx->ptr, ctx->phys);
> > +	if (ctx->has_iommu) {
> > +		qcom_tzmem_free(mdt_ctx->ptr);
> > +		mdt_ctx->addr.phys_addr = 0;
> > +	} else {
> > +		dma_free_coherent(__scm->dev, mdt_ctx->size, mdt_ctx->ptr,
> > +				  mdt_ctx->addr.dma_addr);
> > +		mdt_ctx->addr.dma_addr = 0;
> > +	}
> > -	ctx->ptr = NULL;
> > -	ctx->phys = 0;
> > -	ctx->size = 0;
> > +	mdt_ctx->ptr = NULL;
> > +	mdt_ctx->size = 0;
> >   }
> >   EXPORT_SYMBOL_GPL(qcom_scm_pas_metadata_release);
> > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> > index e376c0338576..09cada92dfd5 100644
> > --- a/drivers/remoteproc/qcom_q6v5_pas.c
> > +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> > @@ -209,9 +209,9 @@ static int qcom_pas_unprepare(struct rproc *rproc)
> >   	 * auth_and_reset() was successful, but in other cases clean it up
> >   	 * here.
> >   	 */
> > -	qcom_scm_pas_metadata_release(pas->pas_ctx->metadata);
> > +	qcom_scm_pas_metadata_release(pas->pas_ctx);
> >   	if (pas->dtb_pas_id)
> > -		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> > +		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
> >   	return 0;
> >   }
> > @@ -244,7 +244,7 @@ static int qcom_pas_load(struct rproc *rproc, const struct firmware *fw)
> >   	return 0;
> >   release_dtb_metadata:
> > -	qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> > +	qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
> >   	release_firmware(pas->dtb_firmware);
> >   	return ret;
> > @@ -313,9 +313,9 @@ static int qcom_pas_start(struct rproc *rproc)
> >   		goto release_pas_metadata;
> >   	}
> > -	qcom_scm_pas_metadata_release(pas->pas_ctx->metadata);
> > +	qcom_scm_pas_metadata_release(pas->pas_ctx);
> >   	if (pas->dtb_pas_id)
> > -		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> > +		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
> >   	/* firmware is used to pass reference from qcom_pas_start(), drop it now */
> >   	pas->firmware = NULL;
> > @@ -323,9 +323,9 @@ static int qcom_pas_start(struct rproc *rproc)
> >   	return 0;
> >   release_pas_metadata:
> > -	qcom_scm_pas_metadata_release(pas->pas_ctx->metadata);
> > +	qcom_scm_pas_metadata_release(pas->pas_ctx);
> >   	if (pas->dtb_pas_id)
> > -		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> > +		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
> >   disable_px_supply:
> >   	if (pas->px_supply)
> >   		regulator_disable(pas->px_supply);
> > diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> > index 509ff85d9bf6..a1718db91b3e 100644
> > --- a/drivers/soc/qcom/mdt_loader.c
> > +++ b/drivers/soc/qcom/mdt_loader.c
> > @@ -240,7 +240,7 @@ EXPORT_SYMBOL_GPL(qcom_mdt_read_metadata);
> >    */
> >   static int __qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
> >   			       const char *fw_name, int pas_id, phys_addr_t mem_phys,
> > -			       struct qcom_scm_pas_metadata *ctx)
> > +			       struct qcom_scm_pas_ctx *ctx)
> >   {
> >   	const struct elf32_phdr *phdrs;
> >   	const struct elf32_phdr *phdr;
> > @@ -491,7 +491,7 @@ int qcom_mdt_pas_load(struct qcom_scm_pas_ctx *ctx, const struct firmware *fw,
> >   	int ret;
> >   	ret = __qcom_mdt_pas_init(ctx->dev, fw, firmware, ctx->peripheral,
> > -				  ctx->mem_phys, ctx->metadata);
> > +				  ctx->mem_phys, ctx);
> >   	if (ret)
> >   		return ret;
> > diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
> > index a31006fe49a9..bd3417d9c3f9 100644
> > --- a/include/linux/firmware/qcom/qcom_scm.h
> > +++ b/include/linux/firmware/qcom/qcom_scm.h
> > @@ -68,7 +68,10 @@ int qcom_scm_set_remote_state(u32 state, u32 id);
> >   struct qcom_scm_pas_metadata {
> >   	void *ptr;
> > -	dma_addr_t phys;
> > +	union {
> > +		dma_addr_t dma_addr;
> > +		phys_addr_t phys_addr;
> > +	} addr;
> >   	ssize_t size;
> >   };
> > @@ -85,8 +88,8 @@ struct qcom_scm_pas_ctx {
> >   void *qcom_scm_pas_ctx_init(struct device *dev, u32 peripheral, phys_addr_t mem_phys,
> >   			    size_t mem_size, bool save_mdt_ctx);
> >   int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
> > -			    struct qcom_scm_pas_metadata *ctx);
> > -void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx);
> > +			    struct qcom_scm_pas_ctx *ctx);
> > +void qcom_scm_pas_metadata_release(struct qcom_scm_pas_ctx *ctx);
> >   int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size);
> >   int qcom_scm_pas_prepare_and_auth_reset(struct qcom_scm_pas_ctx *ctx);
> >   int qcom_scm_pas_auth_and_reset(u32 peripheral);
> 
> Please review the error paths here especially WRT to qcom_mdt_pas_init();

Sure, will send the fix patch for the existing bug.

> 
> ---
> bod

-- 
-Mukesh Ojha

On 21/08/2025 18:03, Mukesh Ojha wrote:
>> Please review the error paths here especially WRT to qcom_mdt_pas_init();
> Sure, will send the fix patch for the existing bug.

In which case please structure your next submission with bugfixes coming 
first in the series with Fixes: tags and then apply your changes on top 
as a progressive set of changes so we can apply this fix to -stable.

Don't forget to Cc: stable

---
bod

On Thu, Aug 21, 2025 at 10:33:37PM +0530, Mukesh Ojha wrote:
> On Thu, Aug 21, 2025 at 04:23:53PM +0100, Bryan O'Donoghue wrote:
> > On 19/08/2025 17:54, Mukesh Ojha wrote:
> > > Qualcomm SoCs running with QHEE (Qualcomm Hypervisor Execution
> > > Environment—a library present in the Gunyah hypervisor) utilize the
> > > Peripheral Authentication Service (PAS) from Qualcomm TrustZone (TZ)
> > > also called QTEE(Qualcomm Trusted Execution Environment firmware)
> > > to securely authenticate and reset remote processors via a sequence
> > > of SMC calls such as qcom_scm_pas_init_image(), qcom_scm_pas_mem_setup(),
> > > and qcom_scm_pas_auth_and_reset().
> > > 
> > > For memory passed to Qualcomm TrustZone, it must either be part of a
> > > pool registered with TZ or be directly registered via SHMbridge SMC
> > > calls.
> > > 
> > > When QHEE is present, PAS SMC calls from Linux running at EL1 are
> > > trapped by QHEE (running at EL2), which then creates or retrieves memory
> > > from the SHM bridge for both metadata and remoteproc carveout memory
> > > before passing them to TZ. However, when the SoC runs with a
> > > non-QHEE-based hypervisor, Linux must create the SHM bridge for both
> > > metadata (before it is passed to TZ in qcom_scm_pas_init_image()) and
> > > for remoteproc memory (before the call is made to TZ in
> > > qcom_scm_pas_auth_and_reset()).
> > > 
> > > For the qcom_scm_pas_init_image() call, metadata content must be copied
> > > to a buffer allocated from the SHM bridge before making the SMC call.
> > > This buffer should be freed either immediately after the call or during
> > > the qcom_scm_pas_metadata_release() function, depending on the context
> > > parameter passed to qcom_scm_pas_init_image(). Convert the metadata
> > > context parameter to use PAS context data structure so that it will also
> > > be possible to decide whether to get memory from SHMbridge pool or not.
> > > 
> > > When QHEE is present, it manages the IOMMU translation context so, in
> > > absence of it device driver will be aware through device tree that its
> > > translation context is managed by Linux and it need to create SHMbridge
> > > before passing any buffer to TZ, So, remote processor driver should
> > > appropriately set ctx->has_iommu to let PAS SMC function to take care of
> > > everything ready for the call to work.
> > > 
> > > Lets convert qcom_scm_pas_init_image() and qcom_scm_pas_metadata_release()
> > > to have these awareness.
> > 
> > I like the effort in the commit log here but its also a bit too long.
> > 
> > Please go through these paragraphs and try to reduce down the amount of text
> > you are generating.
> 
> I was writing to set context for each commit and for the record and hence, the
> repetition of text you would see in some of the lines used.
> 
> I will take your suggestion and reduce it.
> 
> > 
> > > 
> > > Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> > > ---
> > >   drivers/firmware/qcom/qcom_scm.c       | 71 +++++++++++++++++++++-----
> > >   drivers/remoteproc/qcom_q6v5_pas.c     | 14 ++---
> > >   drivers/soc/qcom/mdt_loader.c          |  4 +-
> > >   include/linux/firmware/qcom/qcom_scm.h |  9 ++--
> > >   4 files changed, 73 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> > > index 7827699e277c..301d440f62f3 100644
> > > --- a/drivers/firmware/qcom/qcom_scm.c
> > > +++ b/drivers/firmware/qcom/qcom_scm.c
> > > @@ -616,6 +616,35 @@ static int __qcom_scm_pas_init_image(u32 peripheral, dma_addr_t mdata_phys,
> > >   	return ret;
> > >   }
> > > +static int qcom_scm_pas_prep_and_init_image(struct qcom_scm_pas_ctx *ctx,
> > > +					    const void *metadata, size_t size)
> > > +{
> > > +	struct qcom_scm_pas_metadata *mdt_ctx;
> > > +	struct qcom_scm_res res;
> > > +	phys_addr_t mdata_phys;
> > > +	void *mdata_buf;
> > > +	int ret;
> > > +
> > > +	mdt_ctx = ctx->metadata;
> > > +	mdata_buf = qcom_tzmem_alloc(__scm->mempool, size, GFP_KERNEL);
> > > +	if (!mdata_buf)
> > > +		return -ENOMEM;
> > > +
> > > +	memcpy(mdata_buf, metadata, size);
> > > +	mdata_phys = qcom_tzmem_to_phys(mdata_buf);
> > > +
> > > +	ret = __qcom_scm_pas_init_image(ctx->peripheral, mdata_phys, mdata_buf, size, &res);
> > > +	if (ret < 0 || !mdt_ctx) {
> > 
> > if ret is an error or mdt_ctx is null free the memory
> > 
> > > +		qcom_tzmem_free(mdata_buf);
> > > +	} else if (mdt_ctx) {
> > 
> > if mdt_ctx is valid do this
> 
> Nothing change, it is similar to the earlier code.
> 
> > 
> > > +		mdt_ctx->ptr = mdata_buf;
> > > +		mdt_ctx->addr.phys_addr = mdata_phys;
> > > +		mdt_ctx->size = size;
> > > +	}
> > > +
> > > +	return ret ? : res.result[0];
> > 
> > so we can have ctx_mtd valid but return the value at ret but also mtd valid
> > and return the res.result[0]
> > 
> > That seems like an odd choice - surely if you are enumerating the
> > data-structure the result code we care about is res.result[0] instead of ret
> > ?
> > 
> > OK I see this return logic comes from below..
> > 
> > But
> > 
> > drivers/soc/qcom/mdt_loader.c::qcom_mdt_pas_init
> > 
> > ret = qcom_scm_pas_init_image(pas_id, metadata, metadata_len, ctx);
> > kfree(metadata);
> > if (ret) {
> >     /* Invalid firmware metadata */
> >     dev_err(dev, "error %d initializing firmware %s\n", ret, fw_name);
> >     goto out;
> > }
> > 
> > So if ret as returned from your function is > 0 you will leak the memory
> > allocated @ mdata_buf ..
> > 
> > Do you expect something else to come along and call
> > qcom_scm_pas_metadata_release() ?
> 
> You just identified a bug in the existing code where qcom_mdt_pas_init()
> does not call qcom_scm_pas_metadata_release() for firmware image for
> failure case from qcom_q6v5_pas().

This could be bug only if some odd firmware return > 0 response code
even if the SMC was a success.

> 
> 
> > 
> > > +}
> > > +
> > >   /**
> > >    * qcom_scm_pas_init_image() - Initialize peripheral authentication service
> > >    *			       state machine for a given peripheral, using the
> > > @@ -625,7 +654,7 @@ static int __qcom_scm_pas_init_image(u32 peripheral, dma_addr_t mdata_phys,
> > >    *		and optional blob of data used for authenticating the metadata
> > >    *		and the rest of the firmware
> > >    * @size:	size of the metadata
> > > - * @ctx:	optional metadata context
> > > + * @ctx:	optional pas context
> > >    *
> > >    * Return: 0 on success.
> > >    *
> > > @@ -634,13 +663,19 @@ static int __qcom_scm_pas_init_image(u32 peripheral, dma_addr_t mdata_phys,
> > >    * qcom_scm_pas_metadata_release() by the caller.
> > >    */
> > >   int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
> > > -			    struct qcom_scm_pas_metadata *ctx)
> > > +			    struct qcom_scm_pas_ctx *ctx)
> > >   {
> > > +	struct qcom_scm_pas_metadata *mdt_ctx;
> > >   	struct qcom_scm_res res;
> > >   	dma_addr_t mdata_phys;
> > >   	void *mdata_buf;
> > >   	int ret;
> > > +	if (ctx && ctx->has_iommu) {
> > > +		ret = qcom_scm_pas_prep_and_init_image(ctx, metadata, size);
> > > +		return ret;
> > > +	}
> > > +
> > >   	/*
> > >   	 * During the scm call memory protection will be enabled for the meta
> > >   	 * data blob, so make sure it's physically contiguous, 4K aligned and
> > > @@ -663,10 +698,11 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
> > >   	ret = __qcom_scm_pas_init_image(peripheral, mdata_phys, mdata_buf, size, &res);
> > >   	if (ret < 0 || !ctx) {
> > >   		dma_free_coherent(__scm->dev, size, mdata_buf, mdata_phys);
> > > -	} else if (ctx) {
> > > -		ctx->ptr = mdata_buf;
> > > -		ctx->phys = mdata_phys;
> > > -		ctx->size = size;
> > > +	} else if (ctx->metadata) {
> > > +		mdt_ctx = ctx->metadata;
> > > +		mdt_ctx->ptr = mdata_buf;
> > > +		mdt_ctx->addr.dma_addr = mdata_phys;
> > > +		mdt_ctx->size = size;
> > >   	}
> > >   	return ret ? : res.result[0];
> > 
> > is this return path still valid now that you've functionally decomposed into
> > qcom_sm_pas_prep_and_init ?

Yes, should be the same as it was earlier.

I believe, there is a duplication but its worth it to avoid confusion to
among different allocators used here one is DMA and other is TZmem.

> > 
> > > @@ -675,18 +711,27 @@ EXPORT_SYMBOL_GPL(qcom_scm_pas_init_image);
> > >   /**
> > >    * qcom_scm_pas_metadata_release() - release metadata context
> > > - * @ctx:	metadata context
> > > + * @ctx:	pas context
> > >    */
> > > -void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx)
> > > +void qcom_scm_pas_metadata_release(struct qcom_scm_pas_ctx *ctx)
> > >   {
> > > -	if (!ctx->ptr)
> > > +	struct qcom_scm_pas_metadata *mdt_ctx;
> > > +
> > > +	mdt_ctx = ctx->metadata;
> > > +	if (!mdt_ctx->ptr)
> > >   		return;
> > > -	dma_free_coherent(__scm->dev, ctx->size, ctx->ptr, ctx->phys);
> > > +	if (ctx->has_iommu) {
> > > +		qcom_tzmem_free(mdt_ctx->ptr);
> > > +		mdt_ctx->addr.phys_addr = 0;
> > > +	} else {
> > > +		dma_free_coherent(__scm->dev, mdt_ctx->size, mdt_ctx->ptr,
> > > +				  mdt_ctx->addr.dma_addr);
> > > +		mdt_ctx->addr.dma_addr = 0;
> > > +	}
> > > -	ctx->ptr = NULL;
> > > -	ctx->phys = 0;
> > > -	ctx->size = 0;
> > > +	mdt_ctx->ptr = NULL;
> > > +	mdt_ctx->size = 0;
> > >   }
> > >   EXPORT_SYMBOL_GPL(qcom_scm_pas_metadata_release);
> > > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> > > index e376c0338576..09cada92dfd5 100644
> > > --- a/drivers/remoteproc/qcom_q6v5_pas.c
> > > +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> > > @@ -209,9 +209,9 @@ static int qcom_pas_unprepare(struct rproc *rproc)
> > >   	 * auth_and_reset() was successful, but in other cases clean it up
> > >   	 * here.
> > >   	 */
> > > -	qcom_scm_pas_metadata_release(pas->pas_ctx->metadata);
> > > +	qcom_scm_pas_metadata_release(pas->pas_ctx);
> > >   	if (pas->dtb_pas_id)
> > > -		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> > > +		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
> > >   	return 0;
> > >   }
> > > @@ -244,7 +244,7 @@ static int qcom_pas_load(struct rproc *rproc, const struct firmware *fw)
> > >   	return 0;
> > >   release_dtb_metadata:
> > > -	qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> > > +	qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
> > >   	release_firmware(pas->dtb_firmware);
> > >   	return ret;
> > > @@ -313,9 +313,9 @@ static int qcom_pas_start(struct rproc *rproc)
> > >   		goto release_pas_metadata;
> > >   	}
> > > -	qcom_scm_pas_metadata_release(pas->pas_ctx->metadata);
> > > +	qcom_scm_pas_metadata_release(pas->pas_ctx);
> > >   	if (pas->dtb_pas_id)
> > > -		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> > > +		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
> > >   	/* firmware is used to pass reference from qcom_pas_start(), drop it now */
> > >   	pas->firmware = NULL;
> > > @@ -323,9 +323,9 @@ static int qcom_pas_start(struct rproc *rproc)
> > >   	return 0;
> > >   release_pas_metadata:
> > > -	qcom_scm_pas_metadata_release(pas->pas_ctx->metadata);
> > > +	qcom_scm_pas_metadata_release(pas->pas_ctx);
> > >   	if (pas->dtb_pas_id)
> > > -		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> > > +		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
> > >   disable_px_supply:
> > >   	if (pas->px_supply)
> > >   		regulator_disable(pas->px_supply);
> > > diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> > > index 509ff85d9bf6..a1718db91b3e 100644
> > > --- a/drivers/soc/qcom/mdt_loader.c
> > > +++ b/drivers/soc/qcom/mdt_loader.c
> > > @@ -240,7 +240,7 @@ EXPORT_SYMBOL_GPL(qcom_mdt_read_metadata);
> > >    */
> > >   static int __qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
> > >   			       const char *fw_name, int pas_id, phys_addr_t mem_phys,
> > > -			       struct qcom_scm_pas_metadata *ctx)
> > > +			       struct qcom_scm_pas_ctx *ctx)
> > >   {
> > >   	const struct elf32_phdr *phdrs;
> > >   	const struct elf32_phdr *phdr;
> > > @@ -491,7 +491,7 @@ int qcom_mdt_pas_load(struct qcom_scm_pas_ctx *ctx, const struct firmware *fw,
> > >   	int ret;
> > >   	ret = __qcom_mdt_pas_init(ctx->dev, fw, firmware, ctx->peripheral,
> > > -				  ctx->mem_phys, ctx->metadata);
> > > +				  ctx->mem_phys, ctx);
> > >   	if (ret)
> > >   		return ret;
> > > diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
> > > index a31006fe49a9..bd3417d9c3f9 100644
> > > --- a/include/linux/firmware/qcom/qcom_scm.h
> > > +++ b/include/linux/firmware/qcom/qcom_scm.h
> > > @@ -68,7 +68,10 @@ int qcom_scm_set_remote_state(u32 state, u32 id);
> > >   struct qcom_scm_pas_metadata {
> > >   	void *ptr;
> > > -	dma_addr_t phys;
> > > +	union {
> > > +		dma_addr_t dma_addr;
> > > +		phys_addr_t phys_addr;
> > > +	} addr;
> > >   	ssize_t size;
> > >   };
> > > @@ -85,8 +88,8 @@ struct qcom_scm_pas_ctx {
> > >   void *qcom_scm_pas_ctx_init(struct device *dev, u32 peripheral, phys_addr_t mem_phys,
> > >   			    size_t mem_size, bool save_mdt_ctx);
> > >   int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
> > > -			    struct qcom_scm_pas_metadata *ctx);
> > > -void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx);
> > > +			    struct qcom_scm_pas_ctx *ctx);
> > > +void qcom_scm_pas_metadata_release(struct qcom_scm_pas_ctx *ctx);
> > >   int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size);
> > >   int qcom_scm_pas_prepare_and_auth_reset(struct qcom_scm_pas_ctx *ctx);
> > >   int qcom_scm_pas_auth_and_reset(u32 peripheral);
> > 
> > Please review the error paths here especially WRT to qcom_mdt_pas_init();
> 
> Sure, will send the fix patch for the existing bug.

For existing code, consider a bug only if it is buggy firmware.

> 
> > 
> > ---
> > bod
> 
> -- 
> -Mukesh Ojha

-- 
-Mukesh Ojha