Re: [PATCH v5 07/10] iommu/arm-smmu-v3: Implement pm_runtime & system sleep ops

[Pranjal Shrivastava <praan@google.com>]

On Tue, Mar 10, 2026 at 05:12:15PM -0700, Daniel Mentz wrote:
> On Tue, Mar 10, 2026 at 2:40 PM Pranjal Shrivastava <praan@google.com> wrote:
> >
> > On Tue, Mar 10, 2026 at 02:16:31PM -0700, Daniel Mentz wrote:
> > > On Tue, Mar 10, 2026 at 9:58 AM Pranjal Shrivastava <praan@google.com> wrote:
> > > >
> > > > On Mon, Mar 09, 2026 at 06:45:09PM -0700, Daniel Mentz wrote:
> > > > > On Mon, Jan 26, 2026 at 7:12 AM Pranjal Shrivastava <praan@google.com> wrote:
> > > > > >
> > > > > > +static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
> > > > > > +{
> > > > > > +       struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> > > > > > +       int timeout = ARM_SMMU_SUSPEND_TIMEOUT_US;
> > > > > > +       u32 enables;
> > > > > > +       int ret;
> > > > > > +
> > > > > > +       /* Try to suspend the device, wait for in-flight submissions */
> > > > > > +       do {
> > > > > > +               if (atomic_cmpxchg(&smmu->nr_cmdq_users, 1, 0) == 1)
> > > > >
> > > > >
> > > > > I understand we must follow the rule: do not elide any invalidations
> > > > > while SMMU is still enabled. Otherwise, the following sequence of
> > > > > events might occur:
> > > > >  * The driver clears a table descriptor
> > > > >  * The TLBI command is skipped
> > > > >  * The driver frees the page that backed the translation table
> > > > >  * The page is re-allocated for some unrelated purpose
> > > > >  * SMMU performs a table walk and then descends into the page that has
> > > > > been reallocated, misinterpreting its contents.
> > > > >
> > > > > Can you disable SMMU before setting nr_cmdq_users to 0?
> > > > >
> > > >
> > > > No, that'd be racy, if I disable the smmu but nr_cmdq_users != 0 an
> > > > "owner" thread might believe that the SMMU is still functional and poll
> > > > for consumption.
> > >
> > > I assume that SMMU still processes commands if SMMU_CR0.SMMUEN is
> > > cleared but SMMU_CR0.CMDQEN is set. The arch spec reads: "When
> > > translation is not enabled for a Security state, an SMMU: [...] Can
> > > process commands after the relevant queue pointers are initialized and
> > > SMMU_(*_)CR0.CMDQEN is enabled." Also, the sequence described under
> > > "Arm recommends that software initializing the SMMU performs the
> > > following steps:" suggests this behavior. The existing driver code
> > > also relies on this when it runs CMDQ_OP_CFGI_ALL before setting
> > > SMMU_CR0.SMMUEN.
> > >
> > > >
> > > > I don't think that the SMMU would perform a walk as we first set the
> > > > nr_cmdq_users == 0 to stop command submissions and then immediately set
> > > > GBPA to abort. IF a command submission raced with this, we wait for the
> > >
> > > To ensure we're on the same page: Setting SMMU_GBPA.ABORT doesn't do
> > > anything while SMMU_CR0.SMMUEN is set. SMMU_GBPA only plays a role if
> > > SMMU_CR0.SMMUEN is cleared.
> > >
> >
> > I think you're missing that we *DO* actually set SMMUEN = 0 *with* the
> > GBPA abort, please look at this part of the patch:
> >
> > +       /* Abort all transactions before disable to avoid spurious bypass */
> > +       arm_smmu_update_gbpa(smmu, GBPA_ABORT, 0);
> > +
> > +       /* Disable the SMMU via CR0.EN and all queues except CMDQ */
> > +       enables = CR0_CMDQEN;
> > +       ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0, ARM_SMMU_CR0ACK);
> > +       if (ret) {
> > +               dev_err(smmu->dev, "Timed-out while disabling smmu\n");
> > +               atomic_set(&smmu->nr_cmdq_users, 1);
> > +               return ret;
> > +       }
> >
> > We disable everything except for the CMDQ. Since, everything is disabled
> > at this point, any new commands can safely be elided as the SMMU won't
> > access any config structures as mentioned by the spec.
> >
> > > The moment you set nr_cmdq_users == 0, you potentially start eliding
> > > TBLIs which means that the TLB entries are at risk of becoming stale.
> >
> > Not really, the SMMU isn't accessing anything since SMMUEN=0 and GBPA is
> > set by this point.
> 
> I believe you are assuming that all these things happen atomically
> when they, in fact, happen sequentially. Imagine the CPU takes an
> interrupt after setting nr_cmdq_users==0 but before clearing
> SMMU_CR0.SMMUEN, then we are in a state where nr_cmdq_users==0 but
> SMMUEN is still set.
> 
> >
> > > How about the following sequenc of events:
> > >
> > > * suspend handler sets nr_cmdq_users == 0
> > > * arm_smmu_cmdq_issue_cmdlist() stops updating the producer index
> > > which means that TLBIs are not executed
> > > * (sequence described above)
> > >
> > > Now that I think more about it: I'm concerned that if we set
> > > nr_cmdq_users == 0, the hw prod index will no longer be updated and
> > > the arm_smmu_drain_queues() function will wait for ever, because the
> > > cons index is not catching up.
> > >
> >
> > If we go ahead with the suggestion to completely elide a command
> > submission, if (nr_users_cmdq == 0) return 0; (as discussed in the other
> > thread), then we don't update llq.cons / prod. Then this shouldn't be an
> > issue right?
> 
> I still think we have to clear SMMUEN before setting nr_users_cmdq=0
> 
> >
> > The problem I see in swapping the sequence is::
> >
> > +       /* Abort all transactions before disable to avoid spurious bypass */
> > +       arm_smmu_update_gbpa(smmu, GBPA_ABORT, 0);
> > +
> > +       /* Disable the SMMU via CR0.EN and all queues except CMDQ */
> > +       enables = CR0_CMDQEN; <-- Tear down everything except CMDQ
> > +       ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0, ARM_SMMU_CR0ACK);
> > +       if (ret) {
> > +               dev_err(smmu->dev, "Timed-out while disabling smmu\n");
> > +               atomic_set(&smmu->nr_cmdq_users, 1);
> > +               return ret;
> > +       }
> > +       /* Try to suspend the device, wait for in-flight submissions */
> > +       do {
> > +               if (atomic_cmpxchg(&smmu->nr_cmdq_users, 1, 0) == 1)
> > +                       break;
> > +
> > +               udelay(1);
> > +       } while (--timeout);
> > +
> > +       if (!timeout) {
> > +               dev_warn(smmu->dev, "SMMU in use, aborting suspend\n");
> > +               return -EAGAIN; <--- suspend failed, but we already tore down the smmu
> > +       }
> > +
> > +
> > +       /*
> > +        * At this point the SMMU is completely disabled and won't access
> > +        * any translation/config structures, even speculative accesses
> > +        * aren't performed as per the IHI0070 spec (section 6.3.9.6).
> > +        */
> > +
> > +       /* Wait for the CMDQs to be drained to flush any pending commands */
> > +       ret = arm_smmu_drain_queues(smmu);
> > +       if (ret)
> > +               dev_err(smmu->dev, "Draining queues timed-out..forcing suspend\n");
> > +
> > +       /* Disable everything */
> > +       arm_smmu_device_disable(smmu); <---- Note that this still just disables the CMDQ
> > +       dev_dbg(dev, "Suspended smmu\n");
> > +
> > +       return 0;
> > +}
> > +
> >
> > In the current implementation we first see if we can suspend only then
> > tear down the SMMU (Except for the CMDQ),
> 
> What do you mean by "if we can suspend"? I think we can clear SMMUEN
> even while commands are in-flight.
> 

I mean a race where we set SMMUEN = 0 but we get another CMDQ user, 
since the nr_cmdq_users != 0, we can have multiple users flushing their
commands unless it's back to 1. The problem is how long do we wait for
nr_users = 0 ? If we timeout, we'll have to reset the SMMU as we can't
simply re-enable SMMUEN. 

We can't forcibly set nr_cmdq_user = 0 as we don't know where in the
issue_cmdlist a thread is, forcibly setting it 0 would cause trouble if
a thread is polling on a sync or about to increment the prod_index..

Thanks
Praan