Re: [PATCH v3 03/13] iommu/arm-smmu-v3: Drain in-flight fault events on domain detach

[Nicolin Chen <nicolinc@nvidia.com>]

On Thu, Sep 03, 2026 at 12:18:33PM -0700, Jonathan Cameron wrote:
> > +/**
> > + * arm_smmu_drain_queue - Drain an SMMU queue
> > + * @smmu: the SMMU device
> > + * @q: the queue to drain
> > + * @until_empty: target selection
> > + *
> > + * With @until_empty == true (for CMDQ), exit once the queue is observed empty:
> > + *
> > + *   cons0                cons                                prod
> > + *     |                   |                                   |
> > + *  ---+###################+=====================+=============+--->
> > + *                         |<--------- undrained==0? --------->|
>                                                     ^
> What is the + indicating?  Seems where prod0 that isn't relevant here
> would have been - that is a little confusing so maybe drop?

Yes. I drew the other graph first that has prod0, and forgot to
drop it here with prod0 after I copied.

> > + *
> > + * With @until_empty == false (for EVTQ/PRIQ), exit once "drained" reaches its
> > + * target: "pending" (i.e. prod0 - cons0, frozen at the entry time):
> > + *
> > + *   cons0                cons                 prod0         (prod)
> > + *     |<---- drained ---->|                     |             |
> > + *  ---+###################+=====================+=============+--->
> > + *     |<--------------- pending --------------->|
> > + *
> > + * Note that a drained entry is dequeued, but not necessarily handled: the
> > + * EVTQ/PRIQ callers must follow up with a synchronize_irq() to wait for the
> > + * threaded IRQ handler to finish handling the dequeued entries.
> > + *
> > + * Context: Process context; may sleep.
> > + * Return: 0 on success or a negative errno on timeout.
> > + */
> > +static int arm_smmu_drain_queue(struct arm_smmu_device *smmu,
> > +				struct arm_smmu_queue *q, bool until_empty)
> 
> That name suggests this is doing the draining rather than waiting
> for it to happen elsewhere.

I changed it to "arm_smmu_wait_for_queue_drained".

> > +{
> > +	ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_POLL_TIMEOUT_US);
> > +	u32 cons, prod, prev, undrained;
> > +	u32 drained = 0, pending;
> 
> Pet irritation. Prefer splitting the elements that assign and those
> that don't onto seeprate lines.  Here that just means moving pending
> up one line.

Done.

+       u32 cons, prod, pending;
+       u32 drained = 0;

> > +
> > +	might_sleep();
> > +
> > +	cons = readl_relaxed(q->cons_reg);
> > +	prod = readl_relaxed(q->prod_reg);
> > +	/* The exit target: the number of entries in the queue at entry */
> > +	pending = Q_POS(&q->llq, prod - cons);
> 
> Applying a macro called Q_POS to a difference is a bit confusing to
> me given the output isn't a position of anything. Maybe just needs
> a wrapper Q_DIFF(q->llq, prod, cons)  Can use Q_POS underneath
> but avoid that naming out here well away from the macro definitions.

Sure:

+/* Entries between two positions, i.e. how far @b leads @a */
+#define Q_DIFF(llq, a, b)              Q_POS(llq, (b) - (a))

[...]

+       /* The exit target: the number of entries in the queue at entry */
+       pending = Q_DIFF(&q->llq, cons, prod);

[...]

+               cons = readl_relaxed(q->cons_reg);
+               drained += Q_DIFF(&q->llq, prev, cons);
+
+               prod = readl_relaxed(q->prod_reg);
+               undrained = Q_DIFF(&q->llq, cons, prod);


> > +
> > +	while (true) {
> 
> Maybe pull defintion of prev and undrained in here so it is clear
> they aren't state maintained across iternations.

Done.

+               u32 prev, undrained;

> > +		/* Accumulate the entries consumed since the last poll */
> > +		prev = cons;
> > +		cons = readl_relaxed(q->cons_reg);
> > +		drained += Q_POS(&q->llq, cons - prev);
> > +
> > +		prod = readl_relaxed(q->prod_reg);
> > +		undrained = Q_POS(&q->llq, prod - cons);
> > +
> > +		/* Exit on an empty queue, regardless of until_empty */
> > +		if (!undrained)
> 
> Given you don't use undrained again (maybe in later patches, in which
> case ignore me.)
> 		if (Q_DIFF(&q->llq, prod, cons) == 0)
> perhaps.  This one entirely up to you as maybe the named local does
> help with readability a little.

It's indeed for readability: this matches the graph in the kdocs.

> > +			return 0;
> > +
> > +		/* Snapshot mode: exit once the pending entries are drained */
> > +		if (!until_empty && drained >= pending)
> > +			return 0;
> > +
> > +		/*
> > +		 * A timeout means the consumer might be stuck. In theory, if it
> > +		 * moves 2 * qsize entries or more within a single poll interval
> > +		 * Q_POS() would wrap and undercount drained: that could trigger
> > +		 * a spurious warning too, if the queue was never once observed
> > +		 * empty. Yet, that much consumption in such a short interval is
> > +		 * unrealistic. WARN it only, as a stuck consumer is a real bug.
> 
> I don't like 'unrealisitic' based defenses (even though I agree it is pretty
> unlikely).  Is there a way to bound this?  Maybe future systems will
> be much quicker.

I can't see one..

I think this is already an extreme corner case. Sashiko found it,
FWIW.

If you have a better word than "unrealistic", I can change that.

> > +		 */
> > +		if (WARN_ON(ktime_compare(ktime_get(), timeout) > 0))
> 
> Why WARN_ON here then a dev_warn_ratelimited() below? 

I dropped WARN_ON.

+               if (ktime_compare(ktime_get(), timeout) > 0)
+                       break;

> > +		/* The consumer might be a threaded IRQ handler. Yield to it */
> > +		usleep_range(100, 200);
> 
> fsleep() perhaps then we don't get to argue why that slack.

Done.

+               /* The consumer might be a threaded IRQ handler. Yield to it */
+               fsleep(100);

Thanks
Nicolin