Re: [PATCH v8 11/12] iommu/arm-smmu-v3: Invoke pm_runtime before hw access

[Pranjal Shrivastava <praan@google.com>]

On Wed, Jun 03, 2026 at 01:28:19PM -0700, Daniel Mentz wrote:
> On Mon, Jun 1, 2026 at 2:59 PM Pranjal Shrivastava <praan@google.com> wrote:
> > @@ -2361,8 +2394,33 @@ static irqreturn_t arm_smmu_handle_gerror(struct arm_smmu_device *smmu)
> >  static irqreturn_t arm_smmu_gerror_handler(int irq, void *dev)
> >  {
> >         struct arm_smmu_device *smmu = dev;
> > +       irqreturn_t ret;
> > +
> > +       /*
> > +        * Global Errors are only processed if the SMMU is active.
> > +        *
> > +        * If the STOP_FLAG is set (can_elide == true), the hardware is
> > +        * either already disabled or in the process of being disabled.
> > +        * Any errors captured during the quiesce/drain phase will be
> > +        * handled by the explicit arm_smmu_handle_gerror() call at the
> > +        * end of arm_smmu_runtime_suspend() callback. On resume, the
> > +        * STOP_FLAG is cleared before interrupts are re-enabled, ensuring
> > +        * no valid errors are missed.
> > +        *
> > +        * A lockless check is favoured here over a dynamic PM core check
> > +        * since the runtime_pm_get_if_active would return false during
> > +        * transient states like RPM_RESUMING & ignore level-triggered
> > +        * interrupts.
> > +        */
> > +       if (arm_smmu_cmdq_can_elide(smmu)) {
> > +               dev_err(smmu->dev,
> > +                       "Ignoring gerror interrupt because the SMMU is suspended\n");
> > +               return IRQ_NONE;
> > +       }
> 
> Have you considered using arm_smmu_rpm_get() here instead?
> I can see two issues with the currenlty proposal:
>  * Returning IRQ_NONE when an interrupt is indeed active and needs to
> be handled. This might be interpreted as a spurious interrupt
>  * Nothing is preventing the suspend handler from running while
> arm_smmu_gerror_handler is in the middle of handling an interrupt
> 
> I understand that using arm_smmu_rpm_get() also has downsides,
> including an unnecessary resume operation when the SMMU is already in
> RPM_SUSPENDING state. However, using arm_smmu_rpm_get() would make it
> easier to ensure correctness.
> 

I don't think using arm_smmu_rpm_get() here is possible..

GERROR is registered as a hard IRQ handler, so calling rpm_get (which
can sleep) would be wrong.

Regarding the race, the STOP_FLAG is set at the very beginning of the
suspend sequence. If an IRQ fires after that, we return IRQ_NONE and
let the explicit arm_smmu_handle_gerror() call at the end of
runtime_suspend catch and clear it. After CMDQEN, PRIQEN, EVTQEN &
SMMUEN are all cleared, getting a Gerror should be treated as spurious

That said, I understand your concerns about a real IRQ being interpreted
as a spurious one, and creating an IRQ storm since the gerror register
isn't really written. I have 2 ideas here:

1. We could have a "suspended" flag and check it with can_elide here:
arm_smmu_cmdq_can_elide() && is_suspended() to correctly return IRQ_NONE

2. We could explicitly disable Gerror in IRQ_CTRL write after setting
the CMDQ_STOP_FLAG. Even if there are Gerrors during the CMDQ drain,
we'll catcup to those at the end of our suspend callback.

I'm more inclined towards 2 as it prevents potential races (execution of
an IRQ handler with handle_gerror calls at the end of the suspend).

WDYT?

Thanks.
Praan