Message-ID: <CAEnQRZC0WU+MGfZ5z3yJCz==MBCcFG4BUwNs2v1ABOMkiRbPrw@mail.gmail.com>
Date: Wed, 17 Dec 2025 14:57:24 +0200
From: Daniel Baluta <daniel.baluta@...il.com>
To: Mathieu Poirier <mathieu.poirier@...aro.org>
Cc: Daniel Baluta <daniel.baluta@....com>, andersson@...nel.org, m.szyprowski@...sung.com, 
	shawnguo@...nel.org, kernel@...gutronix.de, festevam@...il.com, 
	arnaud.pouliquen@...s.st.com, robh@...nel.org, geert+renesas@...der.be, 
	linux-remoteproc@...r.kernel.org, imx@...ts.linux.dev, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	iuliana.prodan@....com
Subject: Re: [PATCH v2] remoteproc: imx_dsp_rproc: Fix multiple start/stop operations

On Wed, Dec 17, 2025 at 12:13 AM Mathieu Poirier
<mathieu.poirier@...aro.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@....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

Powered by blists - more mailing lists