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

[Yonghao Zhang <hyz3367@gmail.com>]

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@gmail.com>
> > ---
> >  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.

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.

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
> >