[PATCH v5 04/15] iommu/arm-smmu-v3: Drain in-flight fault events on domain detach

[Jason Gunthorpe]

> [ ... 73 lines skipped ... ]
> +static int arm_smmu_wait_for_queue_drained(struct arm_smmu_device *smmu,
> +					   struct arm_smmu_queue *q,
> +					   bool until_empty)
> +{

Nothing uses until_empty = false ?

Is that for the power management series? How does it make sense?
Shouldn't we already know the queue is not seeing new entries in that
case?

> +	ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_POLL_TIMEOUT_US);
> +	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_DIFF(&q->llq, cons, prod);
> +
> +	while (true) {
> +		u32 prev, undrained;
> +		bool expired;
> +
> +		/*
> +		 * Sample the deadline ahead of the queue state it judges, but
> +		 * break only after the exit conditions below, so a queue that
> +		 * drained during a long preemption still exits with a success.
> +		 */
> +		expired = ktime_compare(ktime_get(), timeout) > 0;
> +
> +		/* Accumulate the entries consumed since the last poll */
> +		prev = cons;
> +		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);

I'm not sure how this all can work, the queue is running on its own
with some other CPU handling interrupts.

You can't do this sort of Q_DIFF math unless you've somehow guaranteed
one side of the queue is stable for this logic. If both pointers are
moving forward then the points pointers can progress and wrap without
this noticing that happened. That will lock up.

Can you just replace this whole function with:

static void arm_smmu_irq_thread_fence(struct arm_smmu_device *smmu,
				      unsigned int irq)
{
	if (!irq)
		return;
	irq_wake_thread(irq, smmu);
	synchronize_irq(irq);
}

?

This forces the thread to run and waits for it to finish. Since the
thread fully drains the queue at the moment it starts, that should be
sufficient?

But I wonder if the point of this has been lost? Prior to calling the
driver attach functions the core code already changes the xarray:

	curr = xa_cmpxchg(&group->pasid_array, pasid, NULL,
			  XA_ZERO_ENTRY, GFP_KERNEL);

That immediately makes the threaded IRQ safe since it calls
iommu_attach_handle_get() which now fails.

So all that is needed is to synchronize_irq() to make sure the irq
thread sees the xa update

Then to flush the workqueue that iommu_report_device_fault() pushes
into.

We don't need to do anything with the HW queue.

-- 
Jason