| 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: <20250820122520.c2rvwlrspatvnst4@hu-mojha-hyd.qualcomm.com>
Date: Wed, 20 Aug 2025 17:55:20 +0530
From: Mukesh Ojha <mukesh.ojha@....qualcomm.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
Cc: Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>,
Vikash Garodia <quic_vgarodia@...cinc.com>,
Dikshita Agarwal <quic_dikshita@...cinc.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Mathieu Poirier <mathieu.poirier@...aro.org>,
Abhinav Kumar <abhinav.kumar@...ux.dev>, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org, linux-media@...r.kernel.org,
linux-remoteproc@...r.kernel.org
Subject: Re: [PATCH v2 02/11] soc: qcom: mdtloader: Add context aware
qcom_mdt_pas_load() helper
On Wed, Aug 20, 2025 at 12:48:55PM +0100, Bryan O'Donoghue wrote:
> On 19/08/2025 17:54, 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 require it.
> >
> > 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. This will also help in unifying the API used by
> > remoteproc and non-remoteproc subsystems drivers.
> >
> > Signed-off-by: Mukesh Ojha <mukesh.ojha@....qualcomm.com>
> > ---
> > If this approach is preferred, will convert all subsystem drivers to use the
> > same set of API's using context and completely get away with qcom_mdt_load()
> >
> > -Mukesh
> >
> > drivers/remoteproc/qcom_q6v5_pas.c | 53 ++++++++++++++---------------
> > drivers/soc/qcom/mdt_loader.c | 26 ++++++++++----
> > include/linux/soc/qcom/mdt_loader.h | 22 ++++++------
> > 3 files changed, 56 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> > index 55a7da801183..e376c0338576 100644
> > --- a/drivers/remoteproc/qcom_q6v5_pas.c
> > +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> > @@ -115,8 +115,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_ctx *pas_ctx;
> > + struct qcom_scm_pas_ctx *dtb_pas_ctx;
> > };
> > static void qcom_pas_segment_dump(struct rproc *rproc,
> > @@ -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_metadata);
> > + qcom_scm_pas_metadata_release(pas->pas_ctx->metadata);
> > if (pas->dtb_pas_id)
> > - qcom_scm_pas_metadata_release(&pas->dtb_pas_metadata);
> > + qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> > return 0;
> > }
> > @@ -235,15 +235,8 @@ static int qcom_pas_load(struct rproc *rproc, const struct firmware *fw)
> > return ret;
> > }
> > - 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);
> > - if (ret)
> > - goto release_dtb_firmware;
> > -
> > - ret = qcom_mdt_load_no_init(pas->dev, pas->dtb_firmware, pas->dtb_firmware_name,
> > - pas->dtb_mem_region, pas->dtb_mem_phys,
> > - pas->dtb_mem_size, &pas->dtb_mem_reloc);
> > + ret = qcom_mdt_pas_load(pas->dtb_pas_ctx, pas->dtb_firmware, pas->dtb_firmware_name,
> > + pas->dtb_mem_region, &pas->dtb_mem_reloc);
> > if (ret)
> > goto release_dtb_metadata;
> > }
> > @@ -251,9 +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_metadata);
> > -
> > -release_dtb_firmware:
> > + qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> > release_firmware(pas->dtb_firmware);
> > return ret;
> > @@ -301,14 +292,8 @@ 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);
> > - if (ret)
> > - goto disable_px_supply;
> > -
> > - ret = qcom_mdt_load_no_init(pas->dev, pas->firmware, rproc->firmware,
> > - pas->mem_region, pas->mem_phys, pas->mem_size,
> > - &pas->mem_reloc);
> > + ret = qcom_mdt_pas_load(pas->pas_ctx, pas->firmware, rproc->firmware,
> > + pas->mem_region, &pas->dtb_mem_reloc);
> > if (ret)
> > goto release_pas_metadata;
> > @@ -328,9 +313,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->metadata);
> > if (pas->dtb_pas_id)
> > - qcom_scm_pas_metadata_release(&pas->dtb_pas_metadata);
> > + qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> > /* firmware is used to pass reference from qcom_pas_start(), drop it now */
> > pas->firmware = NULL;
> > @@ -338,9 +323,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->metadata);
> > if (pas->dtb_pas_id)
> > - qcom_scm_pas_metadata_release(&pas->dtb_pas_metadata);
> > + qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> > disable_px_supply:
> > if (pas->px_supply)
> > regulator_disable(pas->px_supply);
> > @@ -774,6 +759,18 @@ 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_ctx_init(pas->dev, pas->pas_id, pas->mem_phys,
> > + pas->mem_size, true);
> > + if (!pas->pas_ctx)
> > + goto remove_ssr_sysmon;
>
> this function already returns -ENOMEM you don't set ret to any particular
> value so if qcom_scm_pas_ctx_init() returned NULL, you would exit your probe
> function here "error out" with returning ret = 0
Ack.
>
> Please ERR_PTR() in qcom_scm_pas_ctx_init() and return the error up the call
> stack via your remove_ssr_sysmon jump label.
Sure.
>
> ---
> bod
--
-Mukesh Ojha
Powered by blists - more mailing lists