| 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: <CAL_JsqJGtQTVdaJ99DKiqbo3GfxmU7V6QjroTxHi7gR53Dfe-Q@mail.gmail.com>
Date: Mon, 24 Nov 2025 11:34:02 -0600
From: Rob Herring <robh@...nel.org>
To: Mathieu Poirier <mathieu.poirier@...aro.org>
Cc: Bjorn Andersson <andersson@...nel.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 v6] remoteproc: Use of_reserved_mem_region_* functions for "memory-region"
On Thu, Nov 13, 2025 at 9:32 AM Mathieu Poirier
<mathieu.poirier@...aro.org> wrote:
>
> On Wed, Nov 12, 2025 at 10:59:42AM -0600, Rob Herring wrote:
> > On Wed, Nov 12, 2025 at 9:43 AM Mathieu Poirier
> > <mathieu.poirier@...aro.org> wrote:
> > >
> > > On Tue, 11 Nov 2025 at 12:59, Rob Herring <robh@...nel.org> wrote:
> > > >
> > > > On Tue, Nov 11, 2025 at 10:38:05AM -0700, Mathieu Poirier wrote:
> > > > > Hi Rob,
> > > > >
> > > > > Please see may comment for st_remoteproc.c
> > > > >
> > > > > On Fri, Oct 31, 2025 at 12:59:22PM -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.
> >
> > [...]
> >
> > > > > > diff --git a/drivers/remoteproc/st_remoteproc.c b/drivers/remoteproc/st_remoteproc.c
> > > > > > index e6566a9839dc..043348366926 100644
> > > > > > --- a/drivers/remoteproc/st_remoteproc.c
> > > > > > +++ b/drivers/remoteproc/st_remoteproc.c
> > > > > > @@ -120,40 +120,37 @@ static int st_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
> > > > > > struct device *dev = rproc->dev.parent;
> > > > > > struct device_node *np = dev->of_node;
> > > > > > struct rproc_mem_entry *mem;
> > > > > > - struct reserved_mem *rmem;
> > > > > > - struct of_phandle_iterator it;
> > > > > > - int index = 0;
> > > > > > -
> > > > > > - 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(dev, "unable to acquire memory-region\n");
> > > > > > - return -EINVAL;
> > > > > > - }
> > > > > > + int index = 0, mr = 0;
> > > > > > +
> > > > > > + while (1) {
> > > > > > + struct resource res;
> > > > > > + int ret;
> > > > > > +
> > > > > > + ret = of_reserved_mem_region_to_resource(np, mr++, &res);
> > > > > > + if (ret)
> > > > > > + return 0;
> > > > >
> > > > > The original code calls rproc_elf_load_rsc_table() [1] after iterating through
> > > > > the memory region, something that won't happen with the above.
> > > >
> > > > Indeed. it needs the following incremental change. It is slightly
> > > > different in that rproc_elf_load_rsc_table() is not called if
> > > > 'memory-region' is missing, but the binding says that's required.
> > > >
> > > > 8<--------------------------------------------------
> > > >
> > > > diff --git a/drivers/remoteproc/st_remoteproc.c b/drivers/remoteproc/st_remoteproc.c
> > > > index 043348366926..cb09c244fdb5 100644
> > > > --- a/drivers/remoteproc/st_remoteproc.c
> > > > +++ b/drivers/remoteproc/st_remoteproc.c
> > > > @@ -120,15 +120,19 @@ static int st_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
> > > > struct device *dev = rproc->dev.parent;
> > > > struct device_node *np = dev->of_node;
> > > > struct rproc_mem_entry *mem;
> > > > - int index = 0, mr = 0;
> > > > + int index = 0;
> > > >
> > > > while (1) {
> > > > struct resource res;
> > > > int ret;
> > > >
> > > > - ret = of_reserved_mem_region_to_resource(np, mr++, &res);
> > > > - if (ret)
> > > > - return 0;
> > > > + ret = of_reserved_mem_region_to_resource(np, index, &res);
> > > > + if (ret) {
> > > > + if (index)
> > > > + break;
> > > > + else
> > > > + return ret;
> > > > + }
> > >
> > > This looks brittle and I'm not sure it would work.
> > >
> > > Going back to the original implementation, the only time we want to
> > > "break" is when @index is equal to the amount of memory regions _and_
> > > ret is -EINVAL. Any other condition should return.
> >
> > @index equal to number of entries returns -ENODEV, so that condition
> > is impossible. We can simply it to this:
> >
> > if (ret == -ENODEV && index)
> > break;
> > else
> > return ret;
>
> To me this needs to be:
>
> entries = of_reserved_mem_region_count(np);
Ideally, we try to avoid parsing the same property twice. The places
we count and then read the property again are when we need to allocate
an array of the right size in between. But if that puts this patch to
bed finally, then fine.
>
> ...
> ...
>
> if (ret == -ENODEV && index == entries)
> break;
> else
> return ret;
>
> But taking a step back, it might even be easier to go from a while() to a for(),
> the same way you did in imx_rproc_addr_init().
>
> >
> > If you want to keep the prior behavior when 'memory-region' is
> > missing, then '&& index' can be removed, but I think that was wrong
> > behavior.
> >
> > Rob
Powered by blists - more mailing lists