linux-kernel - Re: [PATCH v4] remoteproc: Use of_reserved_mem_region

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAMuHMdXk4rsv5YEj5kJ4+tu-n_11rXOHe1zJiy7KYnvBwJZ=eQ@mail.gmail.com>
Date: Thu, 14 Aug 2025 09:52:45 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
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>, 
	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"

Hi Rob,

On Wed, 13 Aug 2025 at 23:48, Rob Herring (Arm) <robh@...nel.org> 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>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert+renesas@...der.be> # rcar

One nit below (which applies to most drivers).

> --- a/drivers/remoteproc/rcar_rproc.c
> +++ b/drivers/remoteproc/rcar_rproc.c
> @@ -52,41 +52,33 @@ static int rcar_rproc_prepare(struct rproc *rproc)
>  {
>         struct device *dev = rproc->dev.parent;
>         struct device_node *np = dev->of_node;
> -       struct of_phandle_iterator it;
>         struct rproc_mem_entry *mem;
> -       struct reserved_mem *rmem;
> +       int i = 0;
>         u32 da;
>
>         /* Register associated reserved memory regions */
> -       of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
> -       while (of_phandle_iterator_next(&it) == 0) {
> -
> -               rmem = of_reserved_mem_lookup(it.node);
> -               if (!rmem) {
> -                       of_node_put(it.node);
> -                       dev_err(&rproc->dev,
> -                               "unable to acquire memory-region\n");
> -                       return -EINVAL;
> -               }
> +       while (1) {
> +               struct resource res;
> +               int ret;
> +
> +               ret = of_reserved_mem_region_to_resource(np, i++, &res);
> +               if (ret)
> +                       return 0;
>
> -               if (rmem->base > U32_MAX) {
> -                       of_node_put(it.node);
> +               if (res.start > U32_MAX)
>                         return -EINVAL;
> -               }
>
>                 /* No need to translate pa to da, R-Car use same map */
> -               da = rmem->base;
> +               da = res.start;
>                 mem = rproc_mem_entry_init(dev, NULL,
> -                                          rmem->base,
> -                                          rmem->size, da,
> +                                          res.start,
> +                                          resource_size(&res), da,
>                                            rcar_rproc_mem_alloc,
>                                            rcar_rproc_mem_release,
> -                                          it.node->name);
> +                                          res.name);
>
> -               if (!mem) {
> -                       of_node_put(it.node);
> +               if (!mem)
>                         return -ENOMEM;
> -               }
>
>                 rproc_add_carveout(rproc, mem);
>         }

The "return 0;" below (out of context) is now unreachable.
It may be wise to remove it, so the compiler will complain when someone
ever adds a break statement, and people are forced to consider what
is the proper value to return.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds