Re: [PATCH v2] remoteproc: imx_dsp_rproc: Fix multiple start/stop operations

[Mathieu Poirier <mathieu.poirier@linaro.org>]

On Wed, Dec 17, 2025 at 02:57:24PM +0200, Daniel Baluta wrote:
> On Wed, Dec 17, 2025 at 12:13 AM Mathieu Poirier
> <mathieu.poirier@linaro.org> wrote:
> >
> > Good day,
> >
> > On Wed, Dec 10, 2025 at 05:49:06PM +0200, Daniel Baluta wrote:
> > > After commit 67a7bc7f0358 ("remoteproc: Use of reserved_mem_region_*
> > > functions for "memory-region"") following commands with
> > > imx-dsp-rproc started to fail:
> > >
> > > $ echo zephyr.elf > /sys/class/remoteproc/remoteproc0/firmware
> > > $ echo start > /sys/class/remoteproc/remoteproc0/state
> > > $ echo stop > /sys/class/remoteproc/remoteproc0/state
> > > $ echo start > /sys/class/remoteproc/remoteproc0/state #! This fails
> > > -sh: echo: write error: Device or resource busy
> > >
> > > This happens because aforementioned commit replaced devm_ioremap_wc with
> > > devm_ioremap_resource_wc which will "reserve" the memory region with the
> > > first start and then will fail at the second start if the memory
> > > region is already reserved.
> > >
> > > Even partially reverting the faulty commit won't fix the
> > > underlying issue because we map the address in prepare() but we never
> > > unmap it at unprepare(), so we will keep leaking memory regions.
> > >
> > > So, lets use alloc() and release() callbacks for memory carveout
> > > handling. This will nicely map() the memory region at prepare() time
> > > and unmap() it at unprepare().
> > >
> > > Fixes: 67a7bc7f0358 ("remoteproc: Use of_reserved_mem_region_* functions for "memory-region"")
> > > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> > > ---
> > > Changes since v1:
> > >  * https://lore.kernel.org/imx/091a4f29-5435-428a-9a1c-ef82465211cb@nxp.com/T/#t
> > >  * took a different approach and instead of partially reverting the
> > >   faulty patch, used alloc() and release() callbacks to handle memory
> > >   region mapping.
> > >  drivers/remoteproc/imx_dsp_rproc.c | 50 ++++++++++++++++++++----------
> > >  1 file changed, 33 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> > > index 5130a35214c9..83468558e634 100644
> > > --- a/drivers/remoteproc/imx_dsp_rproc.c
> > > +++ b/drivers/remoteproc/imx_dsp_rproc.c
> > > @@ -644,6 +644,32 @@ static void imx_dsp_rproc_free_mbox(struct imx_dsp_rproc *priv)
> > >       mbox_free_channel(priv->rxdb_ch);
> > >  }
> > >
> > > +static int imx_dsp_rproc_mem_alloc(struct rproc *rproc,
> > > +                                struct rproc_mem_entry *mem)
> > > +{
> > > +     struct device *dev = rproc->dev.parent;
> > > +     void *va;
> > > +
> > > +     va = ioremap_wc(mem->dma, mem->len);
> > > +     if (!va) {
> > > +             dev_err(dev, "Unable to map memory region: %pa+%zx\n",
> > > +                     &mem->dma, mem->len);
> > > +             return -ENOMEM;
> > > +     }
> > > +
> > > +     mem->va = va;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int imx_dsp_rproc_mem_release(struct rproc *rproc,
> > > +                                  struct rproc_mem_entry *mem)
> > > +{
> > > +     iounmap(mem->va);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  /**
> > >   * imx_dsp_rproc_add_carveout() - request mailbox channels
> > >   * @priv: private data pointer
> > > @@ -659,7 +685,6 @@ static int imx_dsp_rproc_add_carveout(struct imx_dsp_rproc *priv)
> > >       struct device *dev = rproc->dev.parent;
> > >       struct device_node *np = dev->of_node;
> > >       struct rproc_mem_entry *mem;
> > > -     void __iomem *cpu_addr;
> > >       int a, i = 0;
> > >       u64 da;
> > >
> > > @@ -673,15 +698,10 @@ static int imx_dsp_rproc_add_carveout(struct imx_dsp_rproc *priv)
> > >               if (imx_dsp_rproc_sys_to_da(priv, att->sa, att->size, &da))
> > >                       return -EINVAL;
> > >
> > > -             cpu_addr = devm_ioremap_wc(dev, att->sa, att->size);
> > > -             if (!cpu_addr) {
> > > -                     dev_err(dev, "failed to map memory %p\n", &att->sa);
> > > -                     return -ENOMEM;
> > > -             }
> > > -
> > >               /* Register memory region */
> > > -             mem = rproc_mem_entry_init(dev, (void __force *)cpu_addr, (dma_addr_t)att->sa,
> > > -                                        att->size, da, NULL, NULL, "dsp_mem");
> > > +             mem = rproc_mem_entry_init(dev, NULL, (dma_addr_t)att->sa,
> > > +                                        att->size, da, imx_dsp_rproc_mem_alloc,
> > > +                                        imx_dsp_rproc_mem_release, "dsp_mem");
> >
> > Was there a reason you kept those here rather than moving them to probe() as
> > Iuliana suggested?  Note that I would be fine with this solution since this is
> > how it was before, but if we have to go through a refactoring we may as well
> > take those things into account.
> 
> Tried moving imx_dsp_rproc_add_carveout at probe() time but it doesn't work
> because stop() will clean the carveout list and then the next start() will fail.
> 
> at probe()
> imx_dsp_rproc_add_carveout
> -> rproc_add_carveout (adds allocated carveout to the list).
> 
> at start():
>    -> first start OK
> 
> at stop()
>    -> rproc_shutdown
>        -> rproc_stop
>        -> rproc_resource_cleanup ;//cleans up careveout allocations
> 
> at next start()
>      -> CRASH

I have applied this patch.

Thanks,
Mathieu