Re: [PATCH v4 2/8] iommu/arm-smmu-v3: Add a helper to drain cmd queues

[Pranjal Shrivastava <praan@google.com>]

On Mon, Nov 17, 2025 at 07:48:12PM -0800, Daniel Mentz wrote:
> On Mon, Nov 17, 2025 at 11:15 AM Pranjal Shrivastava <praan@google.com> wrote:
> > +int arm_smmu_queue_poll_until_empty(struct arm_smmu_device *smmu,
> > +                                   struct arm_smmu_queue *q)
> 
> Since this function is only ever used with the Command Queue (cmdq),
> could you simplify the signature by obtaining the queue pointer
> directly from &smmu->cmdq.q within the function?
> 

Ack. I'll pick up the cmdq from the smmu ptr.

> > +{
> > +       struct arm_smmu_queue_poll qp;
> > +       struct arm_smmu_ll_queue *llq = &q->llq;
> > +       int ret = 0;
> 
> I believe you don't need to initialize this to 0. It won't be used
> uninitialized.
> 

Ack.

> > +
> > +       queue_poll_init(smmu, &qp);
> > +       do {
> > +               if (queue_empty(llq))
> > +                       break;
> > +
> > +               ret = queue_poll(&qp);
> > +               WRITE_ONCE(llq->cons, readl_relaxed(q->cons_reg));
> > +
> > +       } while (!ret);
> > +
> > +       return ret;
> > +}
> > +
> > +static int arm_smmu_drain_queues(struct arm_smmu_device *smmu)
> > +{
> > +       struct arm_smmu_cmdq *cmdq = &smmu->cmdq;
> > +       unsigned long flags;
> > +       int ret;
> > +
> > +       /*
> > +        * Since this is only called from the suspend callback, we
> > +        * should be able to acquire the exclusive lock without failing.
> > +        */
> > +       arm_smmu_cmdq_exclusive_trylock_irqsave(cmdq, flags);
> 
> If you assume that we're the exclusive user and then ignore the return
> value of arm_smmu_cmdq_exclusive_trylock_irqsave, I'm wondering if you
> can just skip the locking entirely.
> 

Correct, with this approach we can be sure that we are the only owner as
we'll be executing this much after setting the nr_users = 0;

> > +       ret = arm_smmu_queue_poll_until_empty(smmu, &smmu->cmdq.q);
> > +       arm_smmu_cmdq_exclusive_unlock_irqrestore(cmdq, flags);
> > +
> > +       if (ret)
> > +               return ret;
> > +
> > +       return 0;
> 
> Can you just always "return ret;". Wouldn't that be the same?
> 

We can, since we're suspending anyway even if this times out.

> > +}
> > +

Thanks,
Praan