From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
To: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>,
Bjorn Andersson <andersson@kernel.org>,
Mathieu Poirier <mathieu.poirier@linaro.org>,
Rob Herring <robh@kernel.org>,
Krzysztof Kozlowski <krzk+dt@kernel.org>,
Conor Dooley <conor+dt@kernel.org>,
Manivannan Sadhasivam <mani@kernel.org>,
Konrad Dybcio <konradybcio@kernel.org>
Cc: linux-arm-msm@vger.kernel.org, linux-remoteproc@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 04/13] remoteproc: pas: Replace metadata context with PAS context structure
Date: Tue, 14 Oct 2025 09:24:27 +0100 [thread overview]
Message-ID: <27b9a906-3348-4b75-b5e4-12edad91b93f@linaro.org> (raw)
In-Reply-To: <20251013-kvm_rprocv5-v5-4-d609ed766061@oss.qualcomm.com>
On 13/10/2025 11:03, Mukesh Ojha wrote:
> As a superset of the existing metadata context, the PAS context
> structure enables both remoteproc and non-remoteproc subsystems to
> better support scenarios where the SoC runs with or without the Gunyah
> hypervisor. To reflect this, relevant SCM and metadata functions are
> updated to incorporate PAS context awareness.
>
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
> drivers/firmware/qcom/qcom_scm.c | 32 ++++++++++++++++-------------
> drivers/remoteproc/qcom_q6v5_pas.c | 37 ++++++++++++++++++++++++----------
> drivers/soc/qcom/mdt_loader.c | 4 ++--
> include/linux/firmware/qcom/qcom_scm.h | 4 ++--
> include/linux/soc/qcom/mdt_loader.h | 6 +++---
> 5 files changed, 51 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 6d22b2ac7880..b11a21797d46 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -600,7 +600,7 @@ EXPORT_SYMBOL_GPL(qcom_scm_pas_context_init);
> * 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.
> *
> @@ -609,8 +609,9 @@ EXPORT_SYMBOL_GPL(qcom_scm_pas_context_init);
> * qcom_scm_pas_metadata_release() by the caller.
> */
> int qcom_scm_pas_init_image(u32 pas_id, const void *metadata, size_t size,
> - struct qcom_scm_pas_metadata *ctx)
> + struct qcom_scm_pas_context *ctx)
> {
> + struct qcom_scm_pas_metadata *mdt_ctx;
> dma_addr_t mdata_phys;
> void *mdata_buf;
> int ret;
> @@ -661,10 +662,11 @@ int qcom_scm_pas_init_image(u32 pas_id, const void *metadata, size_t size,
> out:
> 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 && ctx->metadata) {
> + mdt_ctx = ctx->metadata;
> + mdt_ctx->ptr = mdata_buf;
> + mdt_ctx->phys = mdata_phys;
> + mdt_ctx->size = size;
Suspicious looking code..
The second check for ctx is redundant as it cannot ever be false. You have
if (ret < 0 || !ctx) {
} else if (ctx && ctx->mdatadata) {
}
The old code was wrong but, that's no reason to continue to be wrong.
Is it currently possible for ctx to be true but ctx->metadata to be false..
void *qcom_scm_pas_context_init(struct device *dev, u32 pas_id,
phys_addr_t mem_phys,
size_t mem_size)
{
struct qcom_scm_pas_context *ctx;
ctx = devm_kzalloc(dev, 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 = devm_kzalloc(dev, sizeof(*ctx->metadata),
GFP_KERNEL);
if (!ctx->metadata)
return ERR_PTR(-ENOMEM);
return ctx;
}
EXPORT_SYMBOL_GPL(qcom_scm_pas_context_init);
No.
Lets fix the ctx checking logic in this code as we modify the patch.
> }
>
> return ret ? : res.result[0];
> @@ -673,18 +675,20 @@ 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_context *ctx)
> {
> - if (!ctx->ptr)
> - return;
> + struct qcom_scm_pas_metadata *mdt_ctx;
>
> - dma_free_coherent(__scm->dev, ctx->size, ctx->ptr, ctx->phys);
> + mdt_ctx = ctx->metadata;
> + if (!mdt_ctx->ptr)
> + return;
>
> - ctx->ptr = NULL;
> - ctx->phys = 0;
> - ctx->size = 0;
> + dma_free_coherent(__scm->dev, mdt_ctx->size, mdt_ctx->ptr, mdt_ctx->phys);
> + mdt_ctx->ptr = NULL;
> + mdt_ctx->phys = 0;
> + mdt_ctx->size = 0;
> }
If we have established that mdt_ctx->ptr is the fulcurm of truth for
this data then setting ->phys and ->size to anything after setting ->ptr
= NULL are wasted bus cycles.
> 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 158bcd6cc85c..e9dcab94ea0c 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -117,8 +117,8 @@ struct qcom_pas {
> struct qcom_rproc_ssr ssr_subdev;
> struct qcom_sysmon *sysmon;
>
> - struct qcom_scm_pas_metadata pas_metadata;
> - struct qcom_scm_pas_metadata dtb_pas_metadata;
> + struct qcom_scm_pas_context *pas_ctx;
> + struct qcom_scm_pas_context *dtb_pas_ctx;
> };
>
> static void qcom_pas_segment_dump(struct rproc *rproc,
> @@ -211,9 +211,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_metadata);
> + qcom_scm_pas_metadata_release(pas->pas_ctx);
> if (pas->dtb_pas_id)
> - qcom_scm_pas_metadata_release(&pas->dtb_pas_metadata);
> + qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
>
> return 0;
> }
> @@ -241,7 +241,7 @@ static int qcom_pas_load(struct rproc *rproc, const struct firmware *fw)
>
> ret = qcom_mdt_pas_init(pas->dev, pas->dtb_firmware, pas->dtb_firmware_name,
> pas->dtb_pas_id, pas->dtb_mem_phys,
> - &pas->dtb_pas_metadata);
> + pas->dtb_pas_ctx);
> if (ret)
> goto release_dtb_firmware;
>
> @@ -255,7 +255,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_metadata);
> + qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
>
> release_dtb_firmware:
> release_firmware(pas->dtb_firmware);
> @@ -306,7 +306,7 @@ static int qcom_pas_start(struct rproc *rproc)
> }
>
> ret = qcom_mdt_pas_init(pas->dev, pas->firmware, rproc->firmware, pas->pas_id,
> - pas->mem_phys, &pas->pas_metadata);
> + pas->mem_phys, pas->pas_ctx);
> if (ret)
> goto disable_px_supply;
>
> @@ -332,9 +332,9 @@ static int qcom_pas_start(struct rproc *rproc)
> goto release_pas_metadata;
> }
>
> - qcom_scm_pas_metadata_release(&pas->pas_metadata);
> + qcom_scm_pas_metadata_release(pas->pas_ctx);
> if (pas->dtb_pas_id)
> - qcom_scm_pas_metadata_release(&pas->dtb_pas_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;
> @@ -342,9 +342,9 @@ static int qcom_pas_start(struct rproc *rproc)
> return 0;
>
> release_pas_metadata:
> - qcom_scm_pas_metadata_release(&pas->pas_metadata);
> + qcom_scm_pas_metadata_release(pas->pas_ctx);
> if (pas->dtb_pas_id)
> - qcom_scm_pas_metadata_release(&pas->dtb_pas_metadata);
> + qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
> disable_px_supply:
> if (pas->px_supply)
> regulator_disable(pas->px_supply);
> @@ -779,6 +779,21 @@ static int qcom_pas_probe(struct platform_device *pdev)
> }
>
> qcom_add_ssr_subdev(rproc, &pas->ssr_subdev, desc->ssr_name);
> +
> + pas->pas_ctx = qcom_scm_pas_context_init(pas->dev, pas->pas_id, pas->mem_phys,
> + pas->mem_size);
> + if (IS_ERR(pas->pas_ctx)) {
> + ret = PTR_ERR(pas->pas_ctx);
> + goto remove_ssr_sysmon;
> + }
> +
> + pas->dtb_pas_ctx = qcom_scm_pas_context_init(pas->dev, pas->dtb_pas_id,
> + pas->dtb_mem_phys, pas->dtb_mem_size);
> + if (IS_ERR(pas->dtb_pas_ctx)) {
> + ret = PTR_ERR(pas->dtb_pas_ctx);
> + goto remove_ssr_sysmon;
> + }
> +
> ret = rproc_add(rproc);
> if (ret)
> goto remove_ssr_sysmon;
> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> index a5c80d4fcc36..fe35038c5342 100644
> --- a/drivers/soc/qcom/mdt_loader.c
> +++ b/drivers/soc/qcom/mdt_loader.c
> @@ -234,13 +234,13 @@ EXPORT_SYMBOL_GPL(qcom_mdt_read_metadata);
> * @fw_name: name of the firmware, for construction of segment file names
> * @pas_id: PAS identifier
> * @mem_phys: physical address of allocated memory region
> - * @ctx: PAS metadata context, to be released by caller
> + * @ctx: PAS context, ctx->metadata to be released by caller
> *
> * Returns 0 on success, negative errno otherwise.
> */
> 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_context *ctx)
> {
> const struct elf32_phdr *phdrs;
> const struct elf32_phdr *phdr;
> diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
> index 75dec515c5d2..7c58d26ede24 100644
> --- a/include/linux/firmware/qcom/qcom_scm.h
> +++ b/include/linux/firmware/qcom/qcom_scm.h
> @@ -83,8 +83,8 @@ struct qcom_scm_pas_context {
> void *qcom_scm_pas_context_init(struct device *dev, u32 pas_id, phys_addr_t mem_phys,
> size_t mem_size);
> 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);
> + struct qcom_scm_pas_context *ctx);
> +void qcom_scm_pas_metadata_release(struct qcom_scm_pas_context *ctx);
> int qcom_scm_pas_mem_setup(u32 pas_id, phys_addr_t addr, phys_addr_t size);
> int qcom_scm_pas_auth_and_reset(u32 pas_id);
> int qcom_scm_pas_shutdown(u32 pas_id);
> diff --git a/include/linux/soc/qcom/mdt_loader.h b/include/linux/soc/qcom/mdt_loader.h
> index 8ea8230579a2..07c278841816 100644
> --- a/include/linux/soc/qcom/mdt_loader.h
> +++ b/include/linux/soc/qcom/mdt_loader.h
> @@ -10,14 +10,14 @@
>
> struct device;
> struct firmware;
> -struct qcom_scm_pas_metadata;
> +struct qcom_scm_pas_context;
>
> #if IS_ENABLED(CONFIG_QCOM_MDT_LOADER)
>
> ssize_t qcom_mdt_get_size(const struct firmware *fw);
> 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 *pas_metadata_ctx);
> + struct qcom_scm_pas_context *pas_ctx);
> int qcom_mdt_load(struct device *dev, const struct firmware *fw,
> const char *fw_name, int pas_id, void *mem_region,
> phys_addr_t mem_phys, size_t mem_size,
> @@ -39,7 +39,7 @@ static inline ssize_t qcom_mdt_get_size(const struct firmware *fw)
>
> static inline 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 *pas_metadata_ctx)
> + struct qcom_scm_pas_context *pas_ctx)
> {
> return -ENODEV;
> }
>
next prev parent reply other threads:[~2025-10-14 8:24 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-13 10:03 [PATCH v5 00/13] Peripheral Image Loader support for Qualcomm SoCs running Linux host at EL2 Mukesh Ojha
2025-10-13 10:03 ` [PATCH v5 01/13] dt-bindings: remoteproc: qcom,pas: Add iommus property Mukesh Ojha
2025-10-13 10:03 ` [PATCH v5 02/13] firmware: qcom_scm: Rename peripheral as pas_id Mukesh Ojha
2025-10-13 10:03 ` [PATCH v5 03/13] firmware: qcom_scm: Introduce PAS context initialization helper function Mukesh Ojha
2025-10-13 10:03 ` [PATCH v5 04/13] remoteproc: pas: Replace metadata context with PAS context structure Mukesh Ojha
2025-10-14 8:24 ` Bryan O'Donoghue [this message]
2025-10-15 7:25 ` Mukesh Ojha
2025-10-13 10:03 ` [PATCH v5 05/13] soc: qcom: mdtloader: Add PAS context aware qcom_mdt_pas_load() function Mukesh Ojha
2025-10-13 12:56 ` Mukesh Ojha
2025-10-13 10:03 ` [PATCH v5 06/13] soc: qcom: mdtloader: Remove qcom_mdt_pas_init() from exported symbols Mukesh Ojha
2025-10-13 10:03 ` [PATCH v5 07/13] firmware: qcom_scm: Add a prep version of auth_and_reset function Mukesh Ojha
2025-10-13 10:03 ` [PATCH v5 08/13] firmware: qcom_scm: Simplify qcom_scm_pas_init_image() Mukesh Ojha
2025-10-13 10:03 ` [PATCH v5 09/13] firmware: qcom_scm: Add SHM bridge handling for PAS when running without QHEE Mukesh Ojha
2025-10-13 10:03 ` [PATCH v5 10/13] firmware: qcom_scm: Add qcom_scm_pas_get_rsc_table() to get resource table Mukesh Ojha
2025-10-13 10:03 ` [PATCH v5 11/13] remoteproc: pas: Extend parse_fw callback to fetch resources via SMC call Mukesh Ojha
2025-10-13 10:03 ` [PATCH v5 12/13] remoteproc: qcom: pas: Enable Secure PAS support with IOMMU managed by Linux Mukesh Ojha
2025-10-13 10:03 ` [PATCH v5 13/13] arm64: dts: qcom: Add EL2 overlay for Lemans Mukesh Ojha
2025-10-22 10:03 ` [PATCH v5 00/13] Peripheral Image Loader support for Qualcomm SoCs running Linux host at EL2 Mukesh Ojha
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=27b9a906-3348-4b75-b5e4-12edad91b93f@linaro.org \
--to=bryan.odonoghue@linaro.org \
--cc=andersson@kernel.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=konradybcio@kernel.org \
--cc=krzk+dt@kernel.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-remoteproc@vger.kernel.org \
--cc=mani@kernel.org \
--cc=mathieu.poirier@linaro.org \
--cc=mukesh.ojha@oss.qualcomm.com \
--cc=robh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).