| 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: <te2o47dxihjsckaigfdhbrbyqxaeqmchmtx5xbx5y2smu6yaja@t7uccvfsxmay>
Date: Thu, 14 Aug 2025 09:52:05 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: "Rob Herring (Arm)" <robh@...nel.org>
Cc: Bjorn Andersson <andersson@...nel.org>,
Mathieu Poirier <mathieu.poirier@...aro.org>,
Shawn Guo <shawnguo@...nel.org>, Sascha Hauer <s.hauer@...gutronix.de>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Fabio Estevam <festevam@...il.com>,
Geert Uytterhoeven <geert+renesas@...der.be>,
Magnus Damm <magnus.damm@...il.com>,
Patrice Chotard <patrice.chotard@...s.st.com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
Arnaud Pouliquen <arnaud.pouliquen@...s.st.com>,
Peng Fan <peng.fan@....com>, linux-remoteproc@...r.kernel.org,
imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
linux-renesas-soc@...r.kernel.org,
linux-stm32@...md-mailman.stormreply.com
Subject: Re: [PATCH v4] remoteproc: Use of_reserved_mem_region_* functions
for "memory-region"
On Wed, Aug 13, 2025 at 04:48:03PM -0500, Rob Herring (Arm) wrote:
> Use the newly added of_reserved_mem_region_to_resource() and
> of_reserved_mem_region_count() functions to handle "memory-region"
> properties.
>
> The error handling is a bit different in some cases. Often
> "memory-region" is optional, so failed lookup is not an error. But then
> an error in of_reserved_mem_lookup() is treated as an error. However,
> that distinction is not really important. Either the region is available
> and usable or it is not. So now, it is just
> of_reserved_mem_region_to_resource() which is checked for an error.
>
> Acked-by: Arnaud Pouliquen <arnaud.pouliquen@...s.st.com>
> Tested-by: Peng Fan <peng.fan@....com> # i.MX93-11x11-EVK for imx_rproc.c
> Signed-off-by: Rob Herring (Arm) <robh@...nel.org>
> ---
> v4:
> - Rebase on v6.17-rc1. qcom_q6v5_pas.c conflicted needing s/adsp/pas/
>
> v3:
> - Rebase on v6.16-rc1. Move TI K3 changes to new common file.
> - Fix double increment of "i" in xlnx_r5
>
> v2:
> - Use strstarts instead of strcmp for resource names as they include
> the unit-address.
> - Drop the unit-address from resource name for imx and st drivers
> ---
> drivers/remoteproc/imx_dsp_rproc.c | 45 ++++++--------
> drivers/remoteproc/imx_rproc.c | 68 ++++++++------------
> drivers/remoteproc/qcom_q6v5_adsp.c | 24 +++-----
> drivers/remoteproc/qcom_q6v5_mss.c | 60 ++++++------------
> drivers/remoteproc/qcom_q6v5_pas.c | 75 +++++++++--------------
> drivers/remoteproc/qcom_q6v5_wcss.c | 25 +++-----
> drivers/remoteproc/qcom_wcnss.c | 23 +++----
> drivers/remoteproc/rcar_rproc.c | 36 +++++------
> drivers/remoteproc/st_remoteproc.c | 41 ++++++-------
> drivers/remoteproc/stm32_rproc.c | 44 ++++++-------
> drivers/remoteproc/ti_k3_common.c | 28 ++++-----
> drivers/remoteproc/ti_k3_dsp_remoteproc.c | 2 +-
> drivers/remoteproc/ti_k3_r5_remoteproc.c | 2 +-
> drivers/remoteproc/xlnx_r5_remoteproc.c | 51 ++++++---------
> 14 files changed, 204 insertions(+), 320 deletions(-)
>
> diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c
> index 94af77baa7a1..a5b7cbb8fe07 100644
> --- a/drivers/remoteproc/qcom_q6v5_adsp.c
> +++ b/drivers/remoteproc/qcom_q6v5_adsp.c
> @@ -625,26 +625,20 @@ static int adsp_init_mmio(struct qcom_adsp *adsp,
>
> static int adsp_alloc_memory_region(struct qcom_adsp *adsp)
> {
> - struct reserved_mem *rmem = NULL;
> - struct device_node *node;
> -
> - node = of_parse_phandle(adsp->dev->of_node, "memory-region", 0);
> - if (node)
> - rmem = of_reserved_mem_lookup(node);
> - of_node_put(node);
> + int ret;
> + struct resource res;
>
> - if (!rmem) {
> + ret = of_reserved_mem_region_to_resource(adsp->dev->of_node, 0, &res);
> + if (!ret) {
> dev_err(adsp->dev, "unable to resolve memory-region\n");
> - return -EINVAL;
> + return ret;
This looks strange. Shouldn't it be `if (ret) {` ?
> }
>
> - adsp->mem_phys = adsp->mem_reloc = rmem->base;
> - adsp->mem_size = rmem->size;
> - adsp->mem_region = devm_ioremap_wc(adsp->dev,
> - adsp->mem_phys, adsp->mem_size);
> + adsp->mem_phys = adsp->mem_reloc = res.start;
> + adsp->mem_size = resource_size(&res);
> + adsp->mem_region = devm_ioremap_resource_wc(adsp->dev, &res);
> if (!adsp->mem_region) {
> - dev_err(adsp->dev, "unable to map memory region: %pa+%zx\n",
> - &rmem->base, adsp->mem_size);
> + dev_err(adsp->dev, "unable to map memory region: %pR\n", &res);
> return -EBUSY;
> }
>
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index 0c0199fb0e68..0fea5f91dd1c 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -1961,8 +1961,8 @@ static int q6v5_init_reset(struct q6v5 *qproc)
> static int q6v5_alloc_memory_region(struct q6v5 *qproc)
> {
> struct device_node *child;
> - struct reserved_mem *rmem;
> - struct device_node *node;
> + struct resource res;
> + int ret;
>
> /*
> * In the absence of mba/mpss sub-child, extract the mba and mpss
> @@ -1970,71 +1970,49 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
> */
> child = of_get_child_by_name(qproc->dev->of_node, "mba");
> if (!child) {
> - node = of_parse_phandle(qproc->dev->of_node,
> - "memory-region", 0);
> + ret = of_reserved_mem_region_to_resource(qproc->dev->of_node, 0, &res);
> } else {
> - node = of_parse_phandle(child, "memory-region", 0);
> + ret = of_reserved_mem_region_to_resource(child, 0, &res);
> of_node_put(child);
> }
>
> - if (!node) {
> - dev_err(qproc->dev, "no mba memory-region specified\n");
> - return -EINVAL;
> - }
> -
> - rmem = of_reserved_mem_lookup(node);
> - of_node_put(node);
> - if (!rmem) {
> + if (ret) {
> dev_err(qproc->dev, "unable to resolve mba region\n");
> - return -EINVAL;
> + return ret;
> }
>
> - qproc->mba_phys = rmem->base;
> - qproc->mba_size = rmem->size;
> + qproc->mba_phys = res.start;
> + qproc->mba_size = resource_size(&res);
>
> if (!child) {
> - node = of_parse_phandle(qproc->dev->of_node,
> - "memory-region", 1);
> + ret = of_reserved_mem_region_to_resource(qproc->dev->of_node, 1, &res);
> } else {
> child = of_get_child_by_name(qproc->dev->of_node, "mpss");
> - node = of_parse_phandle(child, "memory-region", 0);
> + ret = of_reserved_mem_region_to_resource(child, 0, &res);
> of_node_put(child);
> }
>
> - if (!node) {
> - dev_err(qproc->dev, "no mpss memory-region specified\n");
> - return -EINVAL;
> - }
> -
> - rmem = of_reserved_mem_lookup(node);
> - of_node_put(node);
> - if (!rmem) {
> + if (ret) {
> dev_err(qproc->dev, "unable to resolve mpss region\n");
> - return -EINVAL;
> + return ret;
> }
>
> - qproc->mpss_phys = qproc->mpss_reloc = rmem->base;
> - qproc->mpss_size = rmem->size;
> + qproc->mpss_phys = qproc->mpss_reloc = res.start;
> + qproc->mpss_size = resource_size(&res);
>
> if (!child) {
> - node = of_parse_phandle(qproc->dev->of_node, "memory-region", 2);
> + ret = of_reserved_mem_region_to_resource(qproc->dev->of_node, 2, &res);
> } else {
> child = of_get_child_by_name(qproc->dev->of_node, "metadata");
> - node = of_parse_phandle(child, "memory-region", 0);
> + ret = of_reserved_mem_region_to_resource(child, 0, &res);
> of_node_put(child);
> }
>
> - if (!node)
> + if (ret)
> return 0;
Shouldn't we differentiate between an absent region (OK) and an error
during parse.
>
> - rmem = of_reserved_mem_lookup(node);
> - if (!rmem) {
> - dev_err(qproc->dev, "unable to resolve metadata region\n");
> - return -EINVAL;
> - }
> -
> - qproc->mdata_phys = rmem->base;
> - qproc->mdata_size = rmem->size;
> + qproc->mdata_phys = res.start;
> + qproc->mdata_size = resource_size(&res);
>
> return 0;
> }
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index 02e29171cbbe..b3f7209289a6 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -121,7 +121,7 @@ struct qcom_pas {
>
> static void qcom_pas_segment_dump(struct rproc *rproc,
> struct rproc_dump_segment *segment,
> - void *dest, size_t offset, size_t size)
> + void *dest, size_t offset, size_t size)
Irrelevant? (and two next chunks)
> {
> struct qcom_pas *pas = rproc->priv;
> int total_offset;
> @@ -149,7 +149,7 @@ static void qcom_pas_minidump(struct rproc *rproc)
> }
>
> static int qcom_pas_pds_enable(struct qcom_pas *pas, struct device **pds,
> - size_t pd_count)
> + size_t pd_count)
> {
> int ret;
> int i;
> @@ -176,7 +176,7 @@ static int qcom_pas_pds_enable(struct qcom_pas *pas, struct device **pds,
> };
>
> static void qcom_pas_pds_disable(struct qcom_pas *pas, struct device **pds,
> - size_t pd_count)
> + size_t pd_count)
> {
> int i;
>
--
With best wishes
Dmitry
Powered by blists - more mailing lists