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

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

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

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