Re: [PATCH] remoteproc: qcom_q6v5_pas: Drop early_boot flag for Nord ADSP

[Shawn Guo <shengchao.guo@oss.qualcomm.com>]

On Mon, Oct 05, 2026 at 09:34:03AM +0200, Stephan Gerhold wrote:
> On Sun, Oct 04, 2026 at 11:26:31PM +0800, Shawn Guo wrote:
> > On Thu, Oct 01, 2026 at 10:42:39AM +0200, Konrad Dybcio wrote:
> > > On 9/30/26 4:07 AM, Shawn Guo wrote:
> > > > Qualcomm NHLOS team changes XBL for Nord IOT/Embedded variant, leaving
> > > > ADSP to be powered up by Linux remoteproc, so that Nord IQ10 Qualcomm
> > > > Linux (QLI) behavior gets aligned with IQ8/9. Drop early_boot flag from
> > > > Nord ADSP for that purpose.
> > > > 
> > > > Signed-off-by: Shawn Guo <shengchao.guo@oss.qualcomm.com>
> > > > ---
> > > 
> > > What happens if this patch is absent?
> > 
> > ADSP never comes up:
> > 
> >   remoteproc remoteproc0: attaching to adsp
> >   remoteproc remoteproc0: can't attach to rproc adsp: -19
> > 
> > Since XBL no longer boots ADSP, the remote never publishes its SMP2P
> > inbound item. The smp2p entry stays unmapped, and irq_get_irqchip_state()
> > on fatal_irq returns -ENODEV. qcom_pas_attach() treats that as a hard
> > error rather than falling back to a firmware boot.
> > 
> > Even the existing fallback (ready bit clear -> RPROC_OFFLINE) doesn't
> > work: rproc_boot() takes the attach branch, sees the error and returns
> > without loading firmware.
> > 
> > > 
> > > When the early_boot path was first introduced I was really hoping
> > > that this behavior could be made unconditional and Linux would
> > > figure out if the rproc may be active by virtue of it sending
> > > back signals or not, unfortunately that hasn't made it into the
> > > tree..
> > 
> > The difficulty is with the positive signal. As Stephan pointed out [1],
> > the ready bit is not cleared when the remote is stopped or force-shutdown,
> > so a stop followed by rmmod/modprobe of qcom_q6v5_pas makes the driver
> > attach to a remote that is not running. Without ping-pong or a PAS query
> > for the remote state, I don't think we can reliably say that a remote
> > *is* running.
> > 
> > The negative signal is reliable though. The -ENODEV from
> > irq_get_irqchip_state() means the remote has never populated its SMP2P
> > entry since cold boot, so it cannot be running. We can fall back to a
> > firmware boot in that case. early_boot then becomes a hint that the
> > bootloader *may* have started the remote, which matches Nord ADSP:
> > it is started by XBL on the Auto variant and by Linux remoteproc on
> > the IoT variant.
> > 
> > I'll drop this patch and send two patches instead:
> > 
> > - remoteproc: core: continue to the firmware boot path when .attach()
> >   leaves the rproc in RPROC_OFFLINE
> 
> I don't think you need this change in the remoteproc core. In the
> current upstream state - without the ping-pong implementation - the
> stop/shutdown/ready detection should remain static during the
> initialization of qcom_q6v5_pas. The boot firmware has either started
> it, stopped it, or never started it at all. Those signals should not
> change state until we take some action.
> 
> IMO the proper solution is to move the checks inside qcom_pas_attach()
> to the probe function and never mark the remoteproc as RPROC_DETACHED in
> the first place if we already know it was not started.
> 
> The fatal/crash IRQ checks can probably remain inside qcom_pas_attach(),
> I would assume the remoteproc core does not handle the case where a
> remoteproc appears immediately in RPROC_CRASHED state.

Great input, Stephan! I agree it's way better and cleaner to fix
the problem by not touching remoteproc core.

> BTW the issue you are running into was pointed out by Sashiko during the
> review of the original patch, see the second comment here:
> https://sashiko.dev/#/patchset/20260623-knp-soccp-v7-5-1ec7bb5c9fec%40oss.qualcomm.com?part=5

Ah, it seems we should take Sashiko more seriously!

> The first comment from Sashiko about the broken crash handling during
> attach could also still be valid. I pointed out the same problem
> multiple times during the review process [1]. Unfortunately, Jingyi
> never fully addressed it. Eventually, the fixes were moved out into a
> separate series [2] and AFAICT abandoned as soon as the main series was
> merged with all the open problems. I'm quite frustrated about how the
> review process went for this change. :(
> 
> The crash handling may have improved a bit with the fixes Bjorn did
> recently [3], but someone with access to the hardware for testing should
> really go complete the work that should have been done before merging
> the original series and test that *all* the error cases are handled
> correctly (remoteproc running, not running, crashed).

The crash handling seems still buggy, even with Bjorn's fixes.

If attach finds the fatal bit set, it queues the crash work and returns
-EINVAL. The core unwinds the attach before the crash handler can take
rproc->lock. When the handler runs, it finds DETACHED, sets CRASHED
and calls rproc_boot_recovery() on top of a half-torn-down state:

- rproc_boot() has already dropped power back to 0, so after recovery
  the rproc is RUNNING with power == 0;
- the subdevices are unprepared twice, once in the attach error path
  and again in rproc_stop(), so SSR/sysmon notifiers see a duplicate
  shutdown;
- qcom_pas_stop() and rproc_start() run on resources that rproc_attach()
  already released. With has_iommu, that means iommu_unmap() on a disabled
  domain.

I reproduced this on Nord ADSP by forcing crash_state in
qcom_pas_attach(). Recovery itself appears to succeed. But a later sysfs
"stop" takes power to -1 and returns success without stopping anything,
and the state stays "running". A following "start" then boots the firmware
again on top of the running remote:

  sysfs: cannot create duplicate filename '.../qcom_common.pd-mapper.0'
  remoteproc remoteproc0: failed to prepare subdevices for adsp: -17
  remoteproc remoteproc0: Boot failed: -17

> In other words, if you could also do some more testing for the "crashed"
> case while fixing the "not running" case that would be much appreciated!
> 
> With the state of LLMs today, all the discussions and my lengthy
> comments on the original series are probably perfect as verbatim input
> context for an LLM to make it do the dirty work... :-)

Indeed! I will try to fix the crash handling with LLM's help.

> [1]: https://lore.kernel.org/r/aUsUhX8Km275qonq@linaro.org/
> [2]: https://lore.kernel.org/r/20260409-rproc-attach-issue-v1-0-088a1c348e7a@oss.qualcomm.com/
> [3]: https://lore.kernel.org/r/20260723-rproc-rmmod-not-crashing-v1-0-546dfd5de0e6@oss.qualcomm.com/

Thanks much for the detailed pointers to the earlier threads!

Shawn