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

Hi Stephan,

Thanks -- and you are right on both counts. Attaching a crashed remoteproc
makes no sense, so my suggested RPROC_FEAT_ATTACH_ON_RECOVERY was the wrong
answer. I also have a correction to make: I wrote that the crash could not be
triggered on demand. That was wrong, debugfs exposes exactly that, and I
should have looked before claiming otherwise.

So I tested your suggestion instead of guessing. Both runs on the same boot,
same trigger, only rproc->recovery_disabled differing.

With recovery_disabled = true
=============================

	# echo disabled > /sys/kernel/debug/remoteproc/remoteproc1/recovery
	# echo 2 > /sys/kernel/debug/remoteproc/remoteproc1/crash

	remoteproc remoteproc1: crash detected in cdsp: type fatal error
	remoteproc remoteproc1: handling crash #1 in cdsp

That is all of it. No recovery attempt, no oops, kernel untainted. The state
goes to "crashed" rather than "offline", which is also more honest: nothing
stopped the DSP, it is simply known to be dead.

Control, recovery re-enabled
============================

	# echo enabled > /sys/kernel/debug/remoteproc/remoteproc1/recovery

	remoteproc remoteproc1: recovering cdsp
	remoteproc remoteproc1: stopped remote processor cdsp
	Unable to handle kernel NULL pointer dereference at virtual address 0
	Internal error: Oops: 0000000086000004 [#1]  SMP
	pc : 0x0
	lr : rproc_start+0xc0/0x164
	Call trace:
	 rproc_trigger_recovery+0x148/0x164
	 rproc_recovery_write+0x16c/0x180

Same fault, same lr, as the spontaneous CDSP crash I reported -- only the
entry point differs, debugfs write instead of rproc_crash_handler_work. The
writing process took a SIGSEGV, since the oops happened in its write() path.

So: recovery_disabled = true fixes it, verified on hardware.

One observation, take it or leave it: rproc_start() calls ops->start without
checking it, and rproc_boot_recovery() does the same for ops->coredump. Even
with the flag set correctly by every driver, an rproc_ops missing .start
remains one debugfs write away from a NULL call. A check there would turn a
driver bug into an error return.

On qcom,broken-reset being controversial and possibly never posted upstream:
understood, and thanks for saying so plainly rather than leaving it implied.
I have written that caveat into my notes next to the workaround, so anyone
picking it up from there knows it is not headed for mainline as it stands.
The machine is a Surface Pro 12in (X1P42100) booted at EL2 via slbounce,
where the DSPs are started by the UEFI driver before ExitBootServices; your
series is what makes audio work there at all, so it has been worth having
even in this state.

Thanks again,
François Roux

--
This reply, and the report before it, were drafted with AI assistance; the
testing and the machine are mine.