Re: [PATCH 4/4] remoteproc: core: Clean up after a failed recovery

[Mathieu Poirier]

On Tue, Sep 29, 2026 at 03:54:53PM +0800, Yonghao Zhang wrote:
> When rproc_boot_recovery() fails to bring a crashed processor back
> (the firmware request fails, or rproc_start() fails), it returns
> with the processor stopped but two kinds of state still held.
> 
> The resources of the boot the recovery was trying to restore are
> never released: rproc_stop() does not clean them up, and unlike
> rproc_attach_recovery(), which releases everything when its
> re-attach fails, boot_recovery just returned.  Stale carveout
> entries then fail every later firmware boot at "already associated
> to resource table", until one of those failing boots happens to run
> the cleanup of rproc_fw_boot().
> 
> The power references fare worse: nothing can release them anymore.
> Take a processor with two outstanding rproc_boot() references whose
> recovery stops it and then fails to restart it -- RPROC_OFFLINE,
> with the count still at two.  rproc_shutdown() drops exactly one
> reference per call, and only once past its state gate; the gate
> admits RPROC_RUNNING, RPROC_ATTACHED and RPROC_CRASHED, so an
> offline processor never passes and no amount of shutdown() calls
> releases anything.  With the count still above zero, rproc_boot()
> then short-circuits on atomic_inc_return(&rproc->power) > 1 and
> returns success without doing anything: the users of a dead
> processor are told it is running.  The reference count and the
> state machine are misaligned for good.

Please remove the above 3 paragraphs.

> 
> Release the resources on the failure paths of rproc_boot_recovery(),
> the same way rproc_shutdown() does, and void the power count in
> rproc_trigger_recovery() when the recovery failed without leaving
> the processor crashed, offline or detached.  The service the count
> was tracking is gone, so every outstanding reference is dead, which
> decrementing instead would leave the survivors stranded exactly as
> above.  A processor that is still crashed keeps its references, as
> rproc_shutdown() can still drain them in that state.

Remove the last sentence, starting at "The service the...".

> 
> Fixes: ba194232edc0 ("remoteproc: Support attach recovery after rproc crash")
> Signed-off-by: Yonghao Zhang <hyz3367@xxxxxxxxx>
> ---
>  drivers/remoteproc/remoteproc_core.c | 34 +++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index c52212a1d180..b138680b1905 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1900,7 +1900,7 @@ static int rproc_boot_recovery(struct rproc *rproc)
>  	ret = request_firmware(&firmware_p, rproc->firmware, dev);
>  	if (ret < 0) {
>  		dev_err(dev, "request_firmware failed: %d\n", ret);
> -		return ret;
> +		goto clean_up_resources;
>  	}
>  
>  	/* boot the remote processor up again */
> @@ -1908,6 +1908,24 @@ static int rproc_boot_recovery(struct rproc *rproc)
>  
>  	release_firmware(firmware_p);
>  
> +	if (ret < 0)
> +		goto clean_up_resources;
> +
> +	return 0;
> +
> +clean_up_resources:
> +	/*
> +	 * rproc_stop() has already switched the remote processor off, but
> +	 * unlike rproc_shutdown() nothing releases the resources of the
> +	 * boot this recovery was trying to restore.
> +	 */

Describe the situation as it will be when your patch is applied, not the
situation you are fixing.

> +	rproc_resource_cleanup(rproc);
> +	kfree(rproc->cached_table);
> +	rproc->cached_table = NULL;
> +	rproc->table_ptr = NULL;
> +	/* release HW resources if needed */
> +	rproc_unprepare_device(rproc);
> +	rproc_disable_iommu(rproc);

Use the same sequence as in __rproc_shutdown().

>  	return ret;
>  }
>  
> @@ -1948,6 +1966,20 @@ int rproc_trigger_recovery(struct rproc *rproc)
>  	else
>  		ret = rproc_boot_recovery(rproc);
>  
> +	/*
> +	 * A failed recovery leaves the remote processor in a state from which
> +	 * rproc_shutdown() refuses to release the outstanding power references
> +	 * (RPROC_OFFLINE or RPROC_DETACHED), so every rproc_boot() would
> +	 * free-ride on them and silently do nothing. The service those
> +	 * references were tracking is gone: void them all.  Failures that
> +	 * leave the processor crashed keep the references, as rproc_shutdown()
> +	 * can still drain them in that state.
> +	 */

Again, don't describe the situation you're fixing, describe the situatio once
the fix has been applied.

> +	if (ret && rproc->state != RPROC_CRASHED) {

The only possible states we can be in at this point is either RPROC_OFFLINE or
RPROC_DETACHED.  Make that explicit rather than anything that isn't
RPROC_CRASHED.

I am done reviewing this set.

Thanks,
Mathieu

> +		dev_err(dev, "failed to recover %s: %d\n", rproc->name, ret);
> +		atomic_set(&rproc->power, 0);
> +	}
> +
>  unlock_mutex:
>  	mutex_unlock(&rproc->lock);
>  	return ret;
> -- 
> 2.34.1
>