Re: [PATCH v5 06/10] iommu/arm-smmu-v3: Add a usage counter for cmdq

[Pranjal Shrivastava <praan@google.com>]

On Mon, Mar 09, 2026 at 07:46:24PM +0000, Pranjal Shrivastava wrote:
> On Sun, Mar 08, 2026 at 02:23:03PM -0700, Daniel Mentz wrote:
> > On Mon, Jan 26, 2026 at 7:12 AM Pranjal Shrivastava <praan@google.com> wrote:
> > >
> > > Introduce a biased counter to track the number of active cmdq owners as
> > > a preparatory step for the runtime PM implementation.
> > 
> > Non-owners waiting for their CMD_SYNC to be consumed also appear to
> > increase this counter.
> > 
> 
> Ack, I'll clarify that in the commit message too, any owners or
> non-owners waiting on sync would increment this counter.
> 
> > >
> > > The counter will be used to gate command submission, preventing the
> > > submission of new commands while the device is suspended and deferring
> > > suspend while the command submissions are in-flight.
> > 
> > What does command submission mean in this context? I understand that
> > commands are still added to the queue (until the queue is full). It's
> > only the producer index that is not advanced.
> > 
> 
> Exactly, incrementing the "producer index" is how the command is
> actually submitted to the HW. In case the SMMU is suspended, we'd simply
> add it to the queue but not touch the PROD register in the HW. I can
> clarify this further by saying "preventing the flushing of new commands"
> 
> > >
> > > The counter is biased to a value of 1 during device reset. A cmdq owner
> > > or a thread issuing cmds with sync, increment it before accessing HW
> > > registers and decrements it with release semantics afterwards.
> > >
> > > A value of 1 represents an idle (but active) state. A suspend operation
> > > will set it to from 1 -> 0 representing the suspended state.
> > >
> > > Signed-off-by: Pranjal Shrivastava <praan@google.com>
> > > ---
> > >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 65 +++++++++++++++++----
> > >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  3 +
> > >  2 files changed, 57 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > index 91edcd8922a7..3a8d3c2a8d69 100644
> > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > @@ -805,7 +805,7 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
> > >         u64 cmd_sync[CMDQ_ENT_DWORDS];
> > >         u32 prod;
> > >         unsigned long flags;
> > > -       bool owner;
> > > +       bool owner, has_ref = false;
> > >         struct arm_smmu_ll_queue llq, head;
> > >         int ret = 0;
> > >
> > > @@ -819,8 +819,15 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
> > >
> > >                 while (!queue_has_space(&llq, n + sync)) {
> > >                         local_irq_restore(flags);
> > > +
> > > +                       if (!atomic_inc_not_zero(&smmu->nr_cmdq_users))
> > > +                               /* Device is suspended, don't wait for space */
> > > +                               return 0;
> > > +
> > 
> > Does this mean that we still accumulate commands until the queue is
> > full while in the suspended state and then have SMMU excecute all of
> > these (now irrelevant) commands when the command queue is re-enabled?
> > One might argue that this is wasteful, but I guess it might be
> > technically correct, because it doesn't hurt either.
> > 
> 
> No, we drop any commands that are issued if the command queue doesn't
> have space while it is suspended. There are two reasons for this:
> 
> 1. The arm_smmu_cmdq_poll_until_not_full below would try to perform an
> MMIO access (read_relaxed(cmdq->q.cons_reg) which shouldn't be allowed
> when the SMMU is suspended.
> 
> 2. We usually have 3 types of commands in the SMMUv3:
> 
>  a) Invalidation commands: We clear the entire TLB during resume, so we
>  don't really need to batch these.
> 
>  b) Prefetch commands: Any prefetches are effectively no-ops as when the
>  SMMU is suspended, the resume would effectively read the latest config
> 
>  c) Page responses: These are NOT expected to occur while the SMMU is
>  suspended and we shouldn't be batching them anyway. (The page resp
>  handlers handle such situations and avoid queueing-in commands anyway
>  with this series)
> 
> Thus, any dropped invalidation / prefetch commands don't harm us if the
> SMMU was suspended.
>  
> 
> > >                         if (arm_smmu_cmdq_poll_until_not_full(smmu, cmdq, &llq))
> > >                                 dev_err_ratelimited(smmu->dev, "CMDQ timeout\n");
> > > +
> > > +                       atomic_dec_return_release(&smmu->nr_cmdq_users);
> > >                         local_irq_save(flags);
> > >                 }
> > >
> > > @@ -879,10 +886,35 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
> > >                 arm_smmu_cmdq_poll_valid_map(cmdq, llq.prod, prod);
> > >
> > >                 /*
> > > -                * d. Advance the hardware prod pointer
> > > +                * d. Advance the hardware prod pointer (if smmu is still active)
> > 
> > Consider "(if command queue is still enabled)". Otherwise, what does
> > "active" mean? SMMU and/or command queue enabled?
> > 
> 
> The suspend callback is implemented in a way where we can't have a
> "partial" suspend, i.e. there is no code path where the CMDQ is disabled
> but the SMMU is not. Thus, "active" was supposed to mean The SMMU is
> enabled entirely since there isn't a partial suspend/resume implemented.
> 
> Although, I can update the comment to say (if smmu is still enabled) OR
> (if smmu is NOT suspended).
> 
> > >                  * Control dependency ordering from the entries becoming valid.
> > >                  */
> > > -               writel_relaxed(prod, cmdq->q.prod_reg);
> > > +               if (atomic_inc_not_zero(&smmu->nr_cmdq_users)) {
> > > +                       writel_relaxed(prod, cmdq->q.prod_reg);
> > > +
> > > +                       if (sync) {
> > > +                               has_ref = true;
> > > +                       } else {
> > > +                               /*
> > > +                                * Use release semantics to enforce ordering without a full barrier.
> > > +                                * This ensures the prior writel_relaxed() is ordered/visible
> > > +                                * before the refcount decrement, avoiding the heavy pipeline
> > > +                                * stall of a full wmb().
> > > +                                *
> > > +                                * We need the atomic_dec_return_release() below and the
> > > +                                * atomic_set_release() in step (e) below doesn't suffice.
> > > +                                *
> > > +                                * Specifically, without release semantics on the decrement,
> > > +                                * the CPU is free to reorder the independent atomic_dec_relaxed()
> > > +                                * before the writel_relaxed().
> > > +                                *
> > > +                                * If this happens, the refcount could drop to zero, allowing the PM
> > > +                                * suspend path (running on another CPU) to disable the SMMU before
> > > +                                * the register write completes, resulting in a bus fault.
> > > +                                */
> > > +                               atomic_dec_return_release(&smmu->nr_cmdq_users);
> > > +                       }
> > > +               }
> > >
> > >                 /*
> > >                  * e. Tell the next owner we're done
> > > @@ -894,14 +926,19 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
> > >
> > >         /* 5. If we are inserting a CMD_SYNC, we must wait for it to complete */
> > >         if (sync) {
> > > -               llq.prod = queue_inc_prod_n(&llq, n);
> > > -               ret = arm_smmu_cmdq_poll_until_sync(smmu, cmdq, &llq);
> > > -               if (ret) {
> > > -                       dev_err_ratelimited(smmu->dev,
> > > -                                           "CMD_SYNC timeout at 0x%08x [hwprod 0x%08x, hwcons 0x%08x]\n",
> > > -                                           llq.prod,
> > > -                                           readl_relaxed(cmdq->q.prod_reg),
> > > -                                           readl_relaxed(cmdq->q.cons_reg));
> > > +
> > > +               /* If we are not the owner, check if we're suspended */
> > > +               if (has_ref || atomic_inc_not_zero(&smmu->nr_cmdq_users)) {
> > 
> > Can there be a situation, where we execute this branch and the
> > hardware prod pointer has not been updated, in which case
> > arm_smmu_cmdq_poll_until_sync will wait forever (well, actually, until
> > it times out). Imagine the following series of events:
> >  * nr_cmdq_users==0
> >  * hardware prod pointer update is skipped
> >  * nr_cmdq_users becomes 1
> >  * arm_smmu_cmdq_poll_until_sync waits but commands are not executed
> > by SMMU because hardware prod pointer update wasn't updated
> >  * arm_smmu_device_reset() tries to submit CMDQ_OP_CFGI_ALL, but gets
> > stuck because the queue is full.
> >  * cmdq->q.llq.cons is never updated because the shared lock is held.
> > queue_has_space() returns false, and
> > arm_smmu_cmdq_poll_until_not_full() can't get the exclusive lock
> > 
> 
> Thanks for catching this! IIUC, you mean a race between the resume and
> this code block can cause a deadlock? Something like the following:
> 
> CPU 0					CPU 1
> nr_cmdq_users == 0
> skip hw_prod++
> 					->resume cb
> 					nr_cmdq_user == 1
> nr_cmdq_users == 1
> in the if (sync) part
> arm_smmu_cmdq_poll_until_sync()
> 					CMDQ_OP_CFGI_ALL stuck for space
> 
> I guess we could add a check here if (has_ref == false) but
> nr_cmdq_users > 0, then we could write the prod_reg again.. maybe we'd
> need to factor out a helper function here.. I'll think more about this..
> 
> I guess the simplest thing would be to bail out much before, something
> like if (nr_cmdq_users == 0 && sync) return;
> 
> > > +                       has_ref = true;
> > > +                       llq.prod = queue_inc_prod_n(&llq, n);
> > > +                       ret = arm_smmu_cmdq_poll_until_sync(smmu, cmdq, &llq);
> > 
> > If you skip arm_smmu_cmdq_poll_until_sync() in the nr_cmdq_users==0
> > case, I'm thinking that llq.cons is not updated. If this is the case,
> > is the subsequent WRITE_ONCE(cmdq->q.llq.cons, llq.cons); potentially
> > incorrect?
> > 
> 
> The goal here was to "simulate" a successful command submission while 
> the smmu is suspended (as mentioned by Will earlier), we do everything
> except for touching MMIO here. 
> 
> Since the commands can be one of the three types mentioned above, we
> don't really miss out on anything with the faux-cmd submission here.
> 
> Thus, what if we update llq.prod always and set llq.cons == llq.prod
> condtionally if nr_cmdq_users == 0. 
> 
> Ideally, this should also prevent the `queue_full` scenario as all
> commands are faux-issued instantly. Maybe something like:
> 
> if (sync) {                                                               
>       llq.prod = queue_inc_prod_n(&llq, n);                               
>                                                                           
>       if (has_ref || atomic_inc_not_zero(&smmu->nr_cmdq_users)) {
>           if (!has_ref) {  
>               /* Repair: We were suspended earlier but now we're active.
>                * Kick the hardware to publish our commands. */
>               has_ref = true;
> 	      ... owner logic again ...
> 	          ...
>               writel_relaxed(llq.prod, cmdq->q.prod_reg);                 
>           }
>           ret = arm_smmu_cmdq_poll_until_sync(smmu, cmdq, &llq);
> 
>            if (ret) {
>                // ...                                      
>            }                                                                   
>        } else {
>            /*
>             * Device is suspended. Fast-forward the consumer pointer in memory 
>             * to "drop" these commands and avoid a full queue deadlock.
> 	    * (n + 1 to include the SYNC command)
>             */        
>            llq.cons = queue_inc_prod_n(&llq, n + 1);
>        }                                               
>                                                                        
>        if (!arm_smmu_cmdq_shared_tryunlock(cmdq)) {
>            WRITE_ONCE(cmdq->q.llq.cons, llq.cons);
>            arm_smmu_cmdq_shared_unlock(cmdq);
>        }
>    }
> 
> This could still be racy.. I'll give it another thought..
> 

Still thinking out loud, but maybe we can have something like:

if (owner) {
	....

	if (atomic_inc_not_zero(&smmu->nr_cmdq_users)) {
		writel_relaxes(llq.prod, cmdq->q.prod_reg);
		has_ref = true;
	} else {

		/* SMMU is suspended, fake a consumption */
		WRITE_ONCE(cmdq->q.llq.cons, prod);

		/* Avoid racing with resume */
		sync = false;
	}

	atomic_set_release(&cmdq->owner_prod, prod);
}

if (sync) {

  ... same stuff ..

}

LMK, what you think?

[ snip ]

Thanks,
Praan