Re: [BUG] qcom_q6v5_pas: NULL deref in recovery when using attach-only ops (qcom,broken-reset)

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

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