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

> 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?

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),

I think this would NOT be a problem if we just bail out of issue_cmdlist
early as discussed on the other thread if (nr_cmdq_users == 0) return 0;
that way, we don't end up in a "fake command" consumption situation
which could lead to the cons never catching up while draining the queue.

Thanks
Praan