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

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

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@oss.qualcomm.com>
> > ---
> >  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.

Shawn