Re: [PATCH v6] remoteproc: Use of_reserved_mem_region_* functions for "memory-region"

[Rob Herring <robh@kernel.org>]

On Wed, Nov 12, 2025 at 9:43 AM Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
>
> On Tue, 11 Nov 2025 at 12:59, Rob Herring <robh@kernel.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;

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