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

[Stephan Gerhold <stephan.gerhold@linaro.org>]

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@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.
> 

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.

We rely on this also for the normal boot case. After the initial
handover is completed, the handover IRQ is disabled by Abel's patch and
then you have the exact same situation as in the attach case. There may
be additional handover IRQs coming in, but they should not get latched
for the next boot of the remoteproc.

Thanks,
Stephan