| 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
| ||
|
Message-ID: <20251015072526.xr2v4co4dg7awwuo@hu-mojha-hyd.qualcomm.com>
Date: Wed, 15 Oct 2025 12:55:26 +0530
From: Mukesh Ojha <mukesh.ojha@....qualcomm.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
Cc: 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>, linux-arm-msm@...r.kernel.org,
linux-remoteproc@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 04/13] remoteproc: pas: Replace metadata context with
PAS context structure
On Tue, Oct 14, 2025 at 09:24:27AM +0100, Bryan O'Donoghue wrote:
> 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@....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.
Old code was correct, I see your point this could be same as before
for ctx check and metadata check could be dropped as we still have user
from qcom_mdt_load() which could pass ctx as NULL.
> > }
> > 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.
You want me to clean the older code, I will do this.
>
> > 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;
> > }
> >
>
--
-Mukesh Ojha
Powered by blists - more mailing lists