Re: [PATCH 3/4] remoteproc: core: Roll a failed attach back with detach() when available

[Mathieu Poirier]

On Wed, 21 Oct 2026 at 20:20, Yonghao Zhang <hyz3367@xxxxxxxxx> wrote:
>
> On Mon, Oct 19, 2026 at 11:14:58AM -0600, Mathieu Poirier wrote:
> > @Arnaud Pouliquen - please review this patch to make sure it doesn't break
> > STM32.
> >
> > On Tue, Sep 29, 2026 at 03:54:52PM +0800, Yonghao Zhang wrote:
> > > When the subdevice registration that follows ops->attach() fails,
> > > __rproc_attach() rolls back with an unconditional ops->stop() call.
> > > That is wrong on three counts.
> > >
> >
> > It isn't wrong, simply implemented during a time where any kind of fault in the
> > attach()/detach() scenario called for a stop().  I would certainly appreciate a
> > less reproachful tone.
> >
> >
> > > An attach-only implementation, one without stop() such as
> > > commit 1168af40b1ad ("remoteproc: k3-r5: Add support for IPC-only
> > > mode for all R5Fs"), dereferences NULL right there.  A processor that
> > > is being attached to was started by another entity and is not ours to
> > > power off: detach() is the matching undo of attach(), and stop()
> > > should only be used as a last resort, when there is no detach() or
> > > it fails.  This is reachable today: when the re-attach of an
> > > RPROC_FEAT_ATTACH_ON_RECOVERY processor fails (imx_rproc and
> > > xlnx_r5 use the feature), the unwind stops a processor which,
> > > per the feature's own contract, "does not need help from Linux to
> > > recover... Linux just needs to attach".  And the unwind leaves
> > > the accounting inconsistent -- no resource table bookkeeping
> > > is done, unlike on the rproc_stop() and __rproc_detach() paths,
> > > and a processor powered off through the fallback keeps its
> > > RPROC_DETACHED state, so the next rproc_boot() tries to attach to a
> > > core that is no longer running.
> > >
> >
> > Remove the above paragraph.
> >
> > > Roll the attach back with detach() first, along with the same
> > > rproc_reset_rsc_table_on_detach() bookkeeping __rproc_detach() does,
> > > and fall back to stop() only when detach() is unavailable or failed.
> > > A failed resource table reset does not abort the unwind: this is an
> > > error path, and detaching from the remote processor, or powering it
> > > off as the last resort, takes precedence over the bookkeeping.  A
> > > successful fallback moves the processor to RPROC_OFFLINE so the next
> > > boot reloads firmware instead of attaching to a dead core;
> > > implementations with neither handler keep the processor running and
> > > untouched, which is all an attach-only core needs.  The rollback is
> > > factored into rproc_unwind_attach().
> > >
> > > The unwind runs the same resource table resets as the detach and
> > > stop paths, which free clean_table and leave a cached copy of the
> > > installed table in rproc->cached_table.  Make rproc_attach()'s error
> > > cleanup, which runs right after, null clean_table after freeing it
> > > and release that copy along with table_ptr, or a failed attach
> > > double-frees clean_table and leaks the copy.
> > >
> > > Fixes: d848a4819d85 ("remoteproc: Introducing function rproc_attach()")
> > > Signed-off-by: Yonghao Zhang <hyz3367@xxxxxxxxx>
> > > ---
> > >  drivers/remoteproc/remoteproc_core.c | 65 ++++++++++++++++++++++++++--
> > >  1 file changed, 62 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > > index 19e0ea3e7240..c52212a1d180 100644
> > > --- a/drivers/remoteproc/remoteproc_core.c
> > > +++ b/drivers/remoteproc/remoteproc_core.c
> > > @@ -1348,6 +1348,59 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> > >     return ret;
> > >  }
> > >
> > > +static int rproc_reset_rsc_table_on_detach(struct rproc *rproc);
> > > +static int rproc_reset_rsc_table_on_stop(struct rproc *rproc);
> > > +
> > > +/*
> > > + * Undo an attach whose subdevice registration failed.  The remote
> > > + * processor was started by another entity and is not ours to power
> > > + * off, so roll back with detach() when available and fall back to
> > > + * stop() when there is no detach() or when it failed.  stop() is
> > > + * the only rollback that does not rely on the remote side.  A
> > > + * processor powered off through the fallback is marked RPROC_OFFLINE,
> > > + * so the next boot reloads firmware instead of attaching to a dead
> > > + * core; with neither handler there is nothing to roll back with and
> > > + * the processor is left running.
> > > + *
> > > + * The resource table resets are best-effort: when one fails, the
> > > + * unwind carries on with detach()/stop() anyway.  Unlike
> > > + * __rproc_detach() and rproc_stop(), which bail out before touching
> > > + * the processor when their reset fails, this is already an error
> > > + * path, and detaching the remote processor -- or, failing that,
> > > + * powering it off -- is the minimum it must still deliver.
> > > + */
> > > +static void rproc_unwind_attach(struct rproc *rproc)
> > > +{
> > > +   struct device *dev = &rproc->dev;
> > > +   int ret;
> > > +
> > > +   if (rproc->ops->detach) {
> > > +           ret = rproc_reset_rsc_table_on_detach(rproc);
> > > +           if (ret)
> > > +                   dev_err(dev, "can't reset rsc table on detach: %d\n",
> > > +                           ret);
> > > +
> > > +           ret = rproc->ops->detach(rproc);
> > > +           if (!ret)
> > > +                   return;
> > > +
> > > +           dev_err(dev, "can't detach from rproc %s: %d\n",
> > > +                   rproc->name, ret);
> > > +   }
> > > +
> > > +   if (rproc->ops->stop) {
> > > +           ret = rproc_reset_rsc_table_on_stop(rproc);
> > > +           if (ret)
> > > +                   dev_err(dev, "can't reset rsc table on stop: %d\n",
> > > +                           ret);
> > > +
> > > +           if (rproc->ops->stop(rproc))
> > > +                   dev_err(dev, "can't stop rproc %s\n", rproc->name);
> > > +           else
> > > +                   rproc->state = RPROC_OFFLINE;
> > > +   }
> > > +}
> >
> >
> > Please remove all the dev_err() above.  An error messages are already generate
> > in __rproc_attach() when something goes wrong.
> >
> > > +
> > >  static int __rproc_attach(struct rproc *rproc)
> > >  {
> > >     struct device *dev = &rproc->dev;
> > > @@ -1373,7 +1426,7 @@ static int __rproc_attach(struct rproc *rproc)
> > >     if (ret) {
> > >             dev_err(dev, "failed to probe subdevices for %s: %d\n",
> > >                     rproc->name, ret);
> > > -           goto stop_rproc;
> > > +           goto unwind_attach;
> > >     }
> > >
> > >     rproc->state = RPROC_ATTACHED;
> > > @@ -1382,8 +1435,8 @@ static int __rproc_attach(struct rproc *rproc)
> > >
> > >     return 0;
> > >
> > > -stop_rproc:
> > > -   rproc->ops->stop(rproc);
> > > +unwind_attach:
> > > +   rproc_unwind_attach(rproc);
> > >  unprepare_subdevices:
> > >     rproc_unprepare_subdevices(rproc);
> > >  out:
> > > @@ -1562,6 +1615,7 @@ static int rproc_reset_rsc_table_on_detach(struct rproc *rproc)
> > >      * rproc_set_rsc_table().
> > >      */
> > >     kfree(rproc->clean_table);
> > > +   rproc->clean_table = NULL;
> >
> > This change is not needed.
> >
>
> Hi Mathieu,
>
> Thanks for the review.  I'll adjust the changelog, drop the dev_err()
> calls and remove the table_ptr assignments.
>
> I traced both assignments again and I'd like to keep them.  The one in
> rproc_reset_rsc_table_on_detach() is what keeps the pointer from being
> freed twice:
>
>   __rproc_attach() -> rproc_unwind_attach() ->
>   rproc_reset_rsc_table_on_detach() -> kfree(clean_table)
>
> Then __rproc_attach() returns, rproc_attach() jumps to
> clean_up_resources, which does kfree(rproc->clean_table) again on the
> same pointer. This holds whether detach() succeeds or fails: On
> failure, rproc_reset_rsc_table_on_stop() takes the cached_table early
> exit and leaves clean_table alone.
>

Ok

> The assignments in rproc_reset_rsc_table_on_stop() matters when
> rproc_reset_rsc_table_on_detach() bails out at its kmemdup() with
> -ENOMEM, before freeing clean_table, and detach() then fails. The
> stop() fallback can then make its own copy and be the one taht frees
> clean_table, and clean_up_resources() would free it again. With no
> detach() at all, clean_table is already NULL, so these two paths are
> the only ones that matter.
>

Ok

> I'll wait for Arnaud's feedback on STM32 before sending the v2.
>
> Thanks,
> Yonghao
>
> > >
> > >     return 0;
> > >  }
> > > @@ -1597,6 +1651,7 @@ static int rproc_reset_rsc_table_on_stop(struct rproc *rproc)
> > >      * won't be needed.  Allocated in rproc_set_rsc_table().
> > >      */
> > >     kfree(rproc->clean_table);
> > > +   rproc->clean_table = NULL;
> >
> >
> > This change is not needed.
> >
> > >
> > >  out:
> > >     /*
> > > @@ -1675,6 +1730,10 @@ static int rproc_attach(struct rproc *rproc)
> > >     /* release HW resources if needed */
> > >     rproc_unprepare_device(rproc);
> > >     kfree(rproc->clean_table);
> > > +   rproc->clean_table = NULL;
> > > +   kfree(rproc->cached_table);
> > > +   rproc->cached_table = NULL;
> > > +   rproc->table_ptr = NULL;
> >
> > ->table_ptr doesn't need to be set to NULL.
> >
> > Thanks,
> > Mathieu
> >
> > >  disable_iommu:
> > >     rproc_disable_iommu(rproc);
> > >     return ret;
> > > --
> > > 2.34.1
> > >