On Tue, Aug 11, 2026 at 07:15:46PM +0200, François Roux wrote:
> Running your remoteproc "attach" series on a Surface Pro 12in (X1P42100) at
> EL2, I hit a kernel oops when the CDSP crashed on its own during a long
> build. I was not trying to break anything -- this was a 28-minute kernel
> compile as a stability test, and the DSP failed unprompted.
>
> Reproducer: any remoteproc using qcom_pas_ops_no_reset (i.e. a node with
> qcom,broken-reset) that crashes at runtime. No user action needed.
>
Thanks for the report. Please note that this is a non-upstream change,
so while I appreciate the report we should not ping the upstream
maintainers about it. They have likely never seen the change.
> What happens
> ============
>
> qcom_q6v5_pas 32300000.remoteproc: fatal error received:
> sleep_statsi.c:537:
> remoteproc remoteproc1: crash detected in cdsp: type fatal error
> remoteproc remoteproc1: handling crash #1 in cdsp
> remoteproc remoteproc1: recovering cdsp
> remoteproc remoteproc1: stopped remote processor cdsp
> Unable to handle kernel NULL pointer dereference at virtual address
> 0000000000000000
> Mem abort info:
> ESR = 0x0000000086000004
> EC = 0x21: IABT (current EL), IL = 32 bits
> FSC = 0x04: level 0 translation fault
> Internal error: Oops: 0000000086000004 [#1] SMP
> CPU: 3 UID: 0 PID: 82168 Comm: kworker/u34:5 Not tainted
> 7.1.0-next-20260626 #10
> Hardware name: Microsoft Corporation Surface Pro 12in 1st Ed with
> Snapdragon
> Workqueue: rproc_recovery_wq rproc_crash_handler_work
> pc : 0x0
> lr : rproc_start+0xc0/0x164
> Call trace:
> rproc_trigger_recovery+0x148/0x164
> rproc_crash_handler_work+0xb4/0xb8
> process_one_work+0x15c/0x29c
> worker_thread+0x18c/0x2e0
> kthread+0x11c/0x13c
> ret_from_fork+0x10/0x20
>
> Analysis
> ========
>
> The link register points at rproc_start+0xc0, and the instruction before it
> is the indirect call:
>
> rproc_start+0xbc: ldr x1, [x0, #16] <- rproc->ops->start
> blr x1 <- x1 == NULL
>
> which is remoteproc_core.c:1292:
>
> ret = rproc->ops->start(rproc);
>
> rproc_start() calls ops->start unconditionally, and qcom_pas_ops_no_reset
> does not provide one:
>
> static const struct rproc_ops qcom_pas_ops_no_reset = {
> .attach = qcom_pas_attach,
> .da_to_va = qcom_pas_da_to_va,
> .stop = qcom_pas_stop,
> .panic = qcom_pas_panic,
> };
>
> The reason that path is reached at all is the branch in
> rproc_trigger_recovery():
>
> if (rproc_has_feature(rproc, RPROC_FEAT_ATTACH_ON_RECOVERY))
> ret = rproc_attach_recovery(rproc);
> else
> ret = rproc_boot_recovery(rproc);
>
> qcom_q6v5_pas.c never calls rproc_set_feature(...,
> RPROC_FEAT_ATTACH_ON_RECOVERY)
> -- in this tree only imx_rproc.c does. So an attach-only remoteproc takes the
> boot path on recovery, which tries to load firmware and start it, neither of
> which it can do.
>
> Suggested direction (untested)
> ==============================
>
> Setting RPROC_FEAT_ATTACH_ON_RECOVERY when qcom_pas_ops_no_reset is selected
> looks like the natural fix, so recovery goes through rproc_attach_recovery().
> Note that path calls __rproc_detach() first, and .detach is also absent from
> qcom_pas_ops_no_reset -- but __rproc_detach() does check for it and returns
> an error rather than dereferencing NULL, so the failure would at least be
> graceful.
>
We can't recover the remoteproc in this case, because we don't know how
to restart it. Attaching a crashed remoteproc (that is no longer
functional) does not make sense either.
I think we should set rproc->recovery_disabled = true to fix this.
Practically speaking though, the whole qcom,broken-reset approach is
kind of controversial and I'm not sure yet if I will ever post it
upstream in the current state.
Thanks,
Stephan
