Re: [PATCH 2/2] remoteproc: qcom: pas: Don't fail attach if the remote has crashed

[Stephan Gerhold <stephan.gerhold@linaro.org>]

On Tue, Oct 06, 2026 at 11:35:50AM +0800, Shawn Guo wrote:
> When qcom_pas_attach() finds the fatal SMP2P bit already set, it reports
> a crash and then fails the attach. The core unwinds the attach while the
> crash work is queued: rproc_boot() drops the power refcount back to 0,
> and __rproc_attach()/rproc_attach() unprepare the subdevices, clean up
> the resource table and disable the IOMMU. Once rproc_boot() releases
> rproc->lock, the crash handler finds the rproc still RPROC_DETACHED,
> marks it RPROC_CRASHED and runs rproc_boot_recovery() on top of that
> torn-down state:
> 
>  - the subdevices are unprepared a second time by rproc_stop(), so SSR
>    and sysmon notifiers see a duplicate shutdown;
>  - qcom_pas_stop() and the subsequent rproc_start() operate on resources
>    that rproc_attach() already released, e.g. iommu_unmap() on a
>    disabled domain for rproc->has_iommu;
>  - recovery leaves the rproc RPROC_RUNNING with power == 0. A later
>    "stop" via sysfs takes power to -1 and returns success without
>    stopping anything, so the remote stays running. A following "start"
>    then boots the firmware again on the running remote and fails to
>    re-add the still registered subdevices:
> 
>   sysfs: cannot create duplicate filename '.../qcom_common.pd-mapper.0'
>   remoteproc remoteproc0: failed to prepare subdevices for adsp: -17
>   remoteproc remoteproc0: Boot failed: -17
> 
> A crash found at attach time is no different from one reported by the
> fatal interrupt right after attaching. Treat it the same way as
> q6v5_fatal_interrupt() does: clear q6v5.running, report the crash and
> let the attach succeed. The core completes the attach with balanced
> bookkeeping, and the crash handler then recovers the subsystem through
> the regular RPROC_ATTACHED -> RPROC_CRASHED path.  With running cleared,
> qcom_q6v5_request_stop() does not wait for a stop ack the dead remote
> cannot send, and handover_issued already prevents a spurious handover
> on stop.
> 

Hmmmm okay, I have never thought of this option, but reusing the
existing crash path as-is by making the attach succeed feels actually
quite clever! I suppose starting all the subdevs is really redundant in
this case, but the crash case is also nothing worth optimizing for.

> Assisted-by: LLM
> Signed-off-by: Shawn Guo <shengchao.guo@oss.qualcomm.com>
> ---
>  drivers/remoteproc/qcom_q6v5_pas.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index 8f3d45c604c9..0ecd7688a54d 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -562,10 +562,14 @@ static int qcom_pas_attach(struct rproc *rproc)
>  		goto disable_running;
>  
>  	if (crash_state) {
> +		/*
> +		 * Complete the attach and let the crash handler recover the
> +		 * subsystem from the attached state, as for a crash reported
> +		 * by the fatal interrupt.
> +		 */
>  		dev_err(pas->dev, "Subsystem has crashed before driver probe\n");
> +		pas->q6v5.running = false;
>  		rproc_report_crash(rproc, RPROC_FATAL_ERROR);
> -		ret = -EINVAL;
> -		goto disable_running;
>  	}

My only nitpick is that it would be nice to fully reuse the normal crash
handling in this case, in particular the crash_reason logging inside
q6v5_fatal_interrupt(). Can you extract that part into a common function
and call it from here?

(I guess you could also call the existing q6v5_fatal_interrupt()
function since it should work as-is, but externally calling an interrupt
handler might be a bit weird...)

Thanks,
Stephan