| 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: <aUMNDPDgN7ACvry9@p14s>
Date: Wed, 17 Dec 2025 13:05:32 -0700
From: Mathieu Poirier <mathieu.poirier@...aro.org>
To: Daniel Baluta <daniel.baluta@...il.com>
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 02:57:24PM +0200, Daniel Baluta wrote:
> 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
I have applied this patch.
Thanks,
Mathieu
Powered by blists - more mailing lists