Re: [PATCH 0/4] remoteproc: core: Fix the error unwind paths

[Yonghao Zhang <hyz3367@gmail.com>]

On Tue, Sep 29, 2026 at 03:54:49PM +0800, Yonghao Zhang wrote:

Hi Mathieu,

Gentle ping on this series: it makes rproc_free_vring() reset freed
vring entries to FW_RSC_ADDR_ANY, guards the error-path stop() call
in rproc_start() against implementations without stop(), rolls a
failed attach back with detach() when available, and releases the
resources and power references left behind by a failed recovery.

Happy to provide anything else you need.

Thanks,
Yonghao

> The remoteproc core unwinds failed boots, failed attaches and failed
> recoveries in a handful of places, and those paths have drifted over
> the years: an unconditional stop() here, a missing cleanup there, a
> reset value a running remote processor could misread.  This series
> fixes four of them.  The success paths are untouched, and no in-tree
> driver changes behaviour when nothing fails.
> 
> The single-sided ops configuration that two of the patches guard
> against is not hypothetical.  Version 9 of the TI k3 refactor series
> briefly introduced it [1]: ti_k3_dsp registered in IPC-only mode
> with start() kept and stop() cleared, on the reasoning that
> rproc_attach() never invokes start().  The shape sat on the list for
> a month before the rework that finally landed removed the ops
> overrides altogether (commit 41d746b3423a ("remoteproc: k3-dsp: Don't
> override rproc ops in IPC-only mode")).  The point stands: nothing in
> the core would have caught the intermediate version, and a driver
> sent today with the same single-sided ops would pass rproc_validate()
> just the same.
> 
> Patch 3 mostly benefits the attach-on-recovery users (imx_rproc when
> the Mcore resource is owned by another partition, xlnx_r5 when the
> RPU is already running firmware): since attach recovery was introduced
> (commit ba194232edc0 ("remoteproc: Support attach recovery after rproc
> crash")), a failed re-attach was unwound with an unconditional
> ops->stop() -- powering off a processor which, per the feature's own
> contract, "does not need help from Linux to recover... Linux just needs
> to attach", and which zynqmp_r5_rproc_stop() would additionally flip out
> of attach recovery mode by clearing the feature bit.  With the unwind
> detaching instead, the processor keeps running under its original owner
> and the next attempt attaches again; attach()/detach() stay paired
> one-to-one with the attach() calls that succeeded.
> 
>  - 1/4: rproc_free_vring() writes FW_RSC_ADDR_ANY rather than 0 into
>    a freed vring entry, for the callers that reach it while the
>    remote processor is running and the reset lands in the installed
>    resource table.
>  - 2/4: rproc_start() no longer calls stop() unguarded when it
>    unrolls a failed subdevice registration.
>  - 3/4: __rproc_attach() rolls a failed attach back with detach()
>    when available, keeping the resource table accounting and the
>    state consistent, instead of unconditionally stopping a processor
>    that was started by another entity.
>  - 4/4: a failed recovery releases the resources of the boot it was
>    trying to restore and voids the power count, which nothing else
>    can drain once the processor is offline or detached.
> 
> Testing: on an Allwinner T153 (sun8iw22) with an E907 remote core,
> driven by an out-of-tree platform driver for the Allwinner remoteproc
> hardware.
> 
> The error paths of patches 1 and 3 were hit on this hardware rather
> than constructed.  During development, a subdevice registration
> failure during rproc_attach(), unwound by an intermediate version
> that called detach() without the resource table reset, left the
> freed vring entries at da 0 in the installed table: the next
> rproc_attach() failed in rproc_check_carveout_da(), called from
> rproc_alloc_vring(), with "Registered carveout doesn't fit da request"
> That trace motivated the reset bookkeeping of patch 3 and the
> FW_RSC_ADDR_ANY reset value of patch 1; with the series applied, a
> failed attach can simply be retried.
> 
> The failure path of patch 4 was exercised by renaming the firmware
> file once the processor was up and then crashing the remote core:
> recovery fails in request_firmware() as expected, and after the
> file is restored the next rproc_boot() performs a real new boot
> instead of silently returning success on the stale power count,
> with no "already associated to resource table" leftovers from the
> failed recovery.
> 
> [1] [PATCH v9 17/26] remoteproc: k3: Refactor .start rproc ops into
>     common driver, 2025-03-17:
>     https://lore.kernel.org/all/20250317120622.1746415-18-b-padhi@ti.com/
> 
> Yonghao Zhang (4):
>   remoteproc: core: Reset freed vring entries to FW_RSC_ADDR_ANY
>   remoteproc: core: Guard against a missing stop() in rproc_start()
>   remoteproc: core: Roll a failed attach back with detach() when
>     available
>   remoteproc: core: Clean up after a failed recovery
> 
>  drivers/remoteproc/remoteproc_core.c | 144 +++++++++++++++++++++++----
>  1 file changed, 127 insertions(+), 17 deletions(-)
> 
> 
> base-commit: 586a2fadadb160295a93e5d5d32db1a2558a4d3f
> -- 
> 2.34.1
>