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

[Shawn Guo]

On Fri, Oct 09, 2026 at 04:38:14PM +0200, Stephan Gerhold wrote:
> 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@xxxxxxxxxxxxxxxx>
> > ---
> >  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?

That's a sensible suggestion. Will update.

> (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...)

I share the same view.

Shawn