linux-kernel - Re: [Linux-stm32] [PATCH 3/3] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL_Jsq+7ZhMWgbFDvPB+3BG7YfiS9PweybOGNY3r=d40RbGHJA@mail.gmail.com>
Date: Wed, 19 Mar 2025 18:04:30 -0500
From: Rob Herring <robh@...nel.org>
To: Arnaud POULIQUEN <arnaud.pouliquen@...s.st.com>
Cc: Saravana Kannan <saravanak@...gle.com>, 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>, Patrice Chotard <patrice.chotard@...s.st.com>, 
	Maxime Coquelin <mcoquelin.stm32@...il.com>, Alexandre Torgue <alexandre.torgue@...s.st.com>, 
	devicetree@...r.kernel.org, imx@...ts.linux.dev, 
	linux-arm-msm@...r.kernel.org, linux-remoteproc@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com, 
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [Linux-stm32] [PATCH 3/3] remoteproc: Use of_reserved_mem_region_*
 functions for "memory-region"

On Wed, Mar 19, 2025 at 10:26 AM Arnaud POULIQUEN
<arnaud.pouliquen@...s.st.com> wrote:
>
> Hello Rob,
>
> On 3/18/25 00:24, 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.
> >
> > Signed-off-by: Rob Herring (Arm) <robh@...nel.org>
> > ---
> > For v6.16
> >

[...]

> > diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> > index b02b36a3f515..9d2bd8904c49 100644
> > --- a/drivers/remoteproc/stm32_rproc.c
> > +++ b/drivers/remoteproc/stm32_rproc.c
> > @@ -213,52 +213,46 @@ static int stm32_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;
> >       u64 da;
> > -     int index = 0;
> > +     int index = 0, mr = 0;
> >
> >       /* 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(dev, "unable to acquire memory-region\n");
> > -                     return -EINVAL;
> > -             }
> > +     while (1) {
> > +             struct resource res;
> > +             int ret;
> > +
> > +             ret = of_reserved_mem_region_to_resource(np, mr++, &res);
> > +             if (ret)
> > +                     return 0;
> >
> > -             if (stm32_rproc_pa_to_da(rproc, rmem->base, &da) < 0) {
> > -                     of_node_put(it.node);
> > -                     dev_err(dev, "memory region not valid %pa\n",
> > -                             &rmem->base);
> > +             if (stm32_rproc_pa_to_da(rproc, res.start, &da) < 0) {
> > +                     dev_err(dev, "memory region not valid %pR\n", &res);
> >                       return -EINVAL;
> >               }
> >
> >               /*  No need to map vdev buffer */
> > -             if (strcmp(it.node->name, "vdev0buffer")) {
> > +             if (strcmp(res.name, "vdev0buffer")) {
>
> I tested your patches

Thank you.

> The update introduces a regression here. The strcmp function never returns 0.
> Indeed, it.node->name stores the memory region label "vdev0buffer," while
> res.name stores the memory region name "vdev0buffer@...42000."
>
> Several remoteproc drivers may face the same issue as they embed similar code.

Indeed. I confused myself because node 'name' is without the
unit-address, but this is using the full name. I've replaced the
strcmp's with strstarts() to address this. I've updated my branch with
the changes.

Rob