Re: [PATCH 2/2] remoteproc: qcom: q6v5: Ignore stale handover IRQ delivered on attach

[Shawn Guo]

On Fri, Jul 31, 2026 at 03:22:15PM +0200, Stephan Gerhold wrote:
> On Fri, Jul 31, 2026 at 09:02:20PM +0800, Shawn Guo wrote:
> > On Fri, Jul 31, 2026 at 10:42:20AM +0200, Stephan Gerhold wrote:
> > > On Fri, Jul 31, 2026 at 10:56:54AM +0800, Shawn Guo wrote:
> > > > Commit bb7c5d6f5b41 ("remoteproc: qcom: q6v5: Make handover IRQ one-shot")
> > > > dropped the check that ignored a handover interrupt after handover
> > > > had already been issued, relying instead on disabling the IRQ after
> > > > its first delivery to make it one-shot.
> > > > 
> > > > That is not sufficient for qcom_pas_attach(): when attaching to a
> > > > remote processor that was already booted by the bootloader, handover
> > > > has already happened out-of-band, before this driver ever probed.
> > > > qcom_pas_attach() sets handover_issued = true and unmasks the
> > > > handover IRQ to keep the enable/disable tracking consistent, but the
> > > > transition that signals handover is latched at the interrupt
> > > > controller while masked, so unmasking still delivers that one IRQ.
> > > > 
> > > > Since this driver instance never ran qcom_pas_start() for that boot,
> > > > it never took the proxy power-domain/clock/regulator votes that
> > > > q6v5->handover() releases. Running the handover callback for this
> > > > stale, already-accounted-for signal disables those votes without a
> > > > matching enable, producing:
> > > > 
> > > >   genpd genpd:0:4c00000.remoteproc: Runtime PM usage count underflow!
> > > >   genpd genpd:1:4c00000.remoteproc: Runtime PM usage count underflow!
> > > > 
> > > > Restore the early-return when handover_issued is already set, so a
> > > > stale IRQ delivered on attach is dropped after being disabled instead
> > > > of re-running the handover callback.
> > > > 
> > > > Fixes: bb7c5d6f5b41 ("remoteproc: qcom: q6v5: Make handover IRQ one-shot")
> > > > Assisted-by: Claude:claude-sonnet-5
> > > > Signed-off-by: Shawn Guo <shengchao.guo@xxxxxxxxxxxxxxxx>
> > > > ---
> > > >  drivers/remoteproc/qcom_q6v5.c | 17 +++++++++++++++--
> > > >  1 file changed, 15 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
> > > > index 10fa38f264c8..e715083846aa 100644
> > > > --- a/drivers/remoteproc/qcom_q6v5.c
> > > > +++ b/drivers/remoteproc/qcom_q6v5.c
> > > > @@ -210,10 +210,23 @@ static irqreturn_t q6v5_handover_interrupt(int irq, void *data)
> > > >  {
> > > >  	struct qcom_q6v5 *q6v5 = data;
> > > >  
> > > > -	q6v5->handover_issued = true;
> > > > -
> > > >  	qcom_q6v5_handover_irq_disable(q6v5, false);
> > > >  
> > > > +	/*
> > > > +	 * When attaching to an already-running remote processor,
> > > > +	 * handover_issued is set before the IRQ is unmasked, since the
> > > > +	 * handover already happened out-of-band, before this driver probed.
> > > > +	 * The transition that signals it is latched at the interrupt
> > > > +	 * controller while masked, so unmasking still delivers this one
> > > > +	 * IRQ. Ignore it: this driver instance never enabled the resources
> > > > +	 * (proxy PDs, clocks, etc.) that the handover callback would tear
> > > > +	 * down.
> > > > +	 */
> > > > +	if (q6v5->handover_issued)
> > > > +		return IRQ_HANDLED;
> > > > +
> > > 
> > > The goal of Abel's patch was to have the handover interrupt unmasked
> > > only when needed. If you just bail out here and do nothing then there
> > > was no need to unmask it in the first place. Can you just drop enabling
> > > the handover IRQ in qcom_pas_attach()?
> > 
> > Hi Stephen,
> > 
> > Thanks for the comment!
> > 
> > Although it seems working in my limited testing, I'm not sure it's
> > entirely safe to drop the handover IRQ enable in qcom_pas_attach().
> > 
> > The handover IRQ is requested with IRQF_TRIGGER_RISING. When we attach
> > to a remote processor the bootloader already booted, that rising edge
> > happened while the IRQ was masked. Masking doesn't prevent the edge
> > from being latched at the interrupt controller -- it just defers
> > delivery. The pending bit sits there until the IRQ is unmasked and
> > actually taken; unmasking alone doesn't clear it.
> > 
> > If qcom_pas_attach() never unmasks the IRQ, that stale pending edge
> > doesn't go away -- it just waits for the next unmask, which is the
> > next real qcom_q6v5_prepare() (i.e. the next stop/start cycle). But by
> > then handover_issued has already been reset to false for that new
> > boot (qcom_q6v5_prepare() does this before re-enabling the IRQ), so
> > the stale edge would be indistinguishable from a genuine handover for
> > the new cycle. It could run q6v5->handover() and tear down proxy
> > PDs/clocks/interconnect immediately on prepare, before the firmware
> > for that cycle has even signaled ready.
> > 
> > So the enable_irq() in attach() isn't dead weight -- it's there to
> > deliberately unmask and flush that stale latched edge right away,
> > while handover_issued is still true from attach(), so the IRQ fires
> > once, gets caught by the one-shot ISR, is dropped harmlessly (because
> > handover_issued is already set), and the IRQ ends up masked again.
> > Removing the enable would just defer the spurious handover to a later
> > and more damaging point instead of eliminating it.
> > 
> > If you agree with the theory above, I can follow up with a comment
> > update in q6v5_handover_interrupt() to make this rationale explicit,
> > so it's clear that the attach-time enable exists to flush the latched
> > edge.
> > 
> 
> It's a fair point (that I also considered before writing my original
> comment), but I think it does not apply here.
> 
> There is no real interrupt controller for the SMP2P interrupts, all the
> handling happens in software inside drivers/soc/qcom/smp2p.c. The driver
> does not implement any latching. If the interrupt is masked, the bit in
> entry->irq_enabled is not set and qcom_smp2p_notify_in() simply skips
> signaling the event.

You are right! I will send v2 to just drop handover IRQ enable in
qcom_pas_attach().

Thank you for pushing on this, Stephen!

Shawn