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 <[email protected]> > --- > 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
