Re: [RFC PATCH v2 02/10] iommu/arm-smmu-v3: Add a helper to drain all queues

[Pranjal Shrivastava <praan@google.com>]

On Sun, May 04, 2025 at 01:28:13PM -0700, Daniel Mentz wrote:
> On Fri, Apr 18, 2025 at 4:34 PM Pranjal Shrivastava <praan@google.com> wrote:
> > +static int arm_smmu_queue_poll_until_empty(struct arm_smmu_device *smmu,
> > +                                          struct arm_smmu_queue *q)
> > +{
> > +       struct arm_smmu_queue_poll qp;
> > +       struct arm_smmu_ll_queue *llq = &q->llq;
> > +       int ret = 0;
> > +
> > +       queue_poll_init(smmu, &qp);
> > +       llq->val = READ_ONCE(q->llq.val);
> > +       do {
> > +               if (queue_empty(llq))
> > +                       break;
> > +
> > +               ret = queue_poll(&qp);
> > +               llq->cons = readl(q->cons_reg);
> 
>  A readl_relaxed() is probably sufficient. 

Ack. I referred to __arm_smmu_cmdq_poll_until_consumed() which uses
readl, but I agree that this can be _relaxed as for the former we had to
handle a case where multiple threads are polling on their own CMD_SYNCs.

> Also, in other places, they
> use WRITE_ONCE to write to llq->cons. Compare with
> 
> WRITE_ONCE(cmdq->q.llq.cons, readl_relaxed(cmdq->q.cons_reg));
> 
> in arm_smmu_cmdq_poll_until_not_full.
> 

Yes, I believe that's to avoid store-tearing and re-ordering when we
have multiple threads potentially polling on the llq.cons

However, I don't think that's going to be the case in the suspend
callback as we'd atmost have the page_response responding to a stall
event and issuing a CMD_RESUME (without a sync) which won't poll.

Although, if we're looking to use this function in other context apart
from suspend, I guess there's not much of a harm in using WRITE_ONCE()..

> > +       } while (!ret);
> > +
> > +       return ret;
> > +}
> > +
> > +static int arm_smmu_drain_queues(struct arm_smmu_device *smmu)
> > +{
> > +       int ret;
> > +
> > +       /* cmdq */
> > +       arm_smmu_cmdq_shared_lock(&smmu->cmdq);
> 
> I'm afraid this might not be a correct usage
> arm_smmu_cmdq_shared_lock. I understand that acquiring the shared lock
> means that you want to prevent any other thread from updating
> cmdq->q.llq.cons, which I believe is not what you're looking for here.
> If you want to participate in this locking protocol, I believe you
> need to acquire the exclusive lock.
> 

Ack. 
However, I believe there's no such need to acquire any lock here given
the fact that `arm_smmu_drain_queues` isn't expected to be called from
anywhere else except the suspend callback... What do you think ?

The only case that I can think of that could possibly contend with this
is the evtq_thread handling a stalled transaction with a CMD_RESUME
which could be handled by first draining the evtq.

The callers which plan to use arm_smmu_cmdq_poll_until_empty apart from
this could use the appropriate locking, I can add a comment about that.

> This being said, I believe you need some other mechanism to ensure
> that no new commands are submitted to the command queue. Otherwise,
> the command queue could become non-empty after you return from
> arm_smmu_queue_poll_until_empty.
> 

I think the fact that we're calling this from the suspend callback
itself ensures that there aren't any clients actively issuing commands?

The only exception to this could be the other two queues having events
which require to be responded with either CMD_RESUME or CMD_PRI_RESP.
For example: a stall event that requires us to respond with a CMD_RESUME.

We could drain the other queues before the cmdq in that case.

> >
> > +       ret = arm_smmu_queue_poll_until_empty(smmu, &smmu->cmdq.q);
> > +       arm_smmu_cmdq_shared_unlock(&smmu->cmdq);
> > +       if (ret)
> > +               return ret;
> 
> This is basically a command queue timeout. Consider printing an error
> message similar to "CMD_SYNC timeout at [...]" in
> arm_smmu_cmdq_issue_cmdlist.
> 

We're already printing that in the arm_smmu_runtime_suspend:
	dev_err(smmu->dev, "Draining queues timed-out.. retry later\n");

Are you suggesting to print it specifically for cmdq with the prod/cons?

> > +
> > +       /* evtq */
> > +       ret = arm_smmu_queue_poll_until_empty(smmu, &smmu->evtq.q);
> 
> I'm not sure if you can use your arm_smmu_queue_poll_until_empty
> function on the event queue. I understand that in the case of the
> event queue where the driver is the consumer, the driver's version of
> llq->cons is always more up-to-date than the SMMU_EVTQ_CONS register.
> In fact, I'm afraid you might move the consumer index backwards with
> "llq->cons = readl(q->cons_reg);", because llq->cons is more current
> than SMMU_EVTQ_CONS.
> 

That's a good catch! Thanks! This also true for priq, I think we can
have a dedicated function for draining the cmdq as above and another one
for draining priq and evtq.

> I'm thinking about the following proposal:
>  * Drain only the command queue. Use a local copy for llq->cons. Don't
> write to cmdq->q.cons_reg

We're not writing to cmdq->q.cons_reg anywhere? And we can't rely on the
llq->cons copy since it's ONLY updated when the last command was issued
with a "sync" which isn't guaranteed to be the case always.

>  * Don't bother draining the other queues

I don't think that would be appropriate, we can't leave a stalled
transaction before suspending and should handle all Paging requests via
PRI. In fact in the v1, Robin mentioned[1] quite the opposite. 

>  * Backup consumer and producer indices *after* disabling the queues
> in SMMU_CR0. This would involve writing to cmdq->q.cons_reg

So... basically handle the pending stalls and paging requests when we
resume later? 

Also, why would we write to the cmdq->q.cons_reg if it's going to be
lost with the power-down? If at all, we'd have to read the prod/cons_reg
values back into the SW copies. Am I missing something here?


Thanks,
Praan

[1]
https://lore.kernel.org/all/20250319004254.2547950-4-praan@google.com/T/#m6f479d70d666252a657fb3ff57ff1764267a892d