[PATCH 15/17] dmaengine: idxd: flush the work queues on every halt type

[Daniel Byrne <daniel1.byrne@intel.com>]

A device halt ends the engines, so nothing in flight completes and a
client waiting on one of those descriptors waits forever. Only the FLR
branch of idxd_halt() flushed the kernel work queues. The software-reset
branch only queued the reinit, which re-enables the queues with the dead
descriptors still on their lists and their slots held, and the
system-reset branch only quiesced the queues.

Flush on every halt type. The flush hands descriptors back, so no
submitter may still own one: a submitter that passed the state check
before the halt holds its descriptor on the pending list while ENQCMDS
retries, and aborts and frees it itself when the submit fails. Every
halt branch therefore kills wq_active and waits for the last submitter
before it flushes, and the abort now runs before the submitter drops
its reference, so the wait covers it. Each queue is handled in one hold
of wq_lock, from the state check through the revive. The software reset
revives the queues afterwards, but only what its own flush killed and
only a queue whose interrupt handle a failed revoke has not
invalidated. That revive comes before the reinit enables the device
again, so the submit checks the state again once it holds the reference
and fails with -EIO while the device is not enabled.

Every killer of wq_active runs under wq_lock and waits for the last
reference before it unlocks, so a reference found dying under the lock
is already dead and the kill is idempotent. A halt branch that does not
revive the queue leaves the reference dead, and the recovery's unbind
and a later halt skip the kill rather than drop the count below zero
and wait forever for a release that cannot come.

The software-reset branch flushes queues that its reinit enables again
without requesting new interrupt handles, which is safe only because
the flush no longer releases them since "dmaengine: idxd: keep the
interrupt handle across terminate_all".

Fixes: f019d7814bce ("dmaengine: idxd: Flush kernel workqueues on Function Level Reset")
Assisted-by: Claude:claude-opus-5
Assisted-by: Claude:claude-opus-5-5
Assisted-by: Claude:claude-fable-5-1
Signed-off-by: Daniel Byrne <daniel1.byrne@intel.com>
---
 drivers/dma/idxd/device.c | 49 ++++++++++++++++++++++++---------
 drivers/dma/idxd/idxd.h   |  2 ++
 drivers/dma/idxd/irq.c    | 57 ++++++++++++++++++++++++++++++++++++---
 drivers/dma/idxd/submit.c | 54 ++++++++++++++++++++++++++++++-------
 4 files changed, 135 insertions(+), 27 deletions(-)

diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index a33c4309f5d4..158c141650e3 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -483,13 +483,32 @@ int idxd_wq_init_percpu_ref(struct idxd_wq *wq)
 }
 EXPORT_SYMBOL_NS_GPL(idxd_wq_init_percpu_ref, "IDXD");
 
-void __idxd_wq_quiesce(struct idxd_wq *wq)
+/*
+ * Kill wq_active and wait for the last submitter to drop its reference.
+ * Every killer runs under wq_lock and waits before it unlocks, so a
+ * reference found dying here was killed and waited out by an earlier
+ * caller, such as a halt flush that did not revive the queue. Killing
+ * it again would drop the count below zero and wait forever. A
+ * reference that percpu_ref_exit() released reads as dying too, so a
+ * queue the device re-enabled behind an unbind is skipped as well.
+ * Returns whether this call killed the reference.
+ */
+bool idxd_wq_kill(struct idxd_wq *wq)
 {
 	lockdep_assert_held(&wq->wq_lock);
+	if (percpu_ref_is_dying(&wq->wq_active))
+		return false;
 	reinit_completion(&wq->wq_resurrect);
 	percpu_ref_kill(&wq->wq_active);
-	complete_all(&wq->wq_resurrect);
 	wait_for_completion(&wq->wq_dead);
+	return true;
+}
+
+void __idxd_wq_quiesce(struct idxd_wq *wq)
+{
+	idxd_wq_kill(wq);
+	/* Let parked submitters retry and fail on the dead reference. */
+	complete_all(&wq->wq_resurrect);
 }
 EXPORT_SYMBOL_NS_GPL(__idxd_wq_quiesce, "IDXD");
 
@@ -1627,14 +1646,15 @@ void idxd_wq_quarantine_free(struct idxd_wq *wq, struct list_head *list)
 
 /*
  * A halt killed every descriptor the device held: the quarantined slots
- * are free, and the fence a failed drain raised can be lifted. Under
- * wq_lock, since an unbind racing the halt may free the pool.
+ * are free, and the fence a failed drain raised can be lifted. The halt
+ * flush calls this under wq_lock, since an unbind racing the halt may
+ * free the pool.
  */
 void idxd_wq_halt_release(struct idxd_wq *wq)
 {
 	LIST_HEAD(dead_descs);
 
-	guard(mutex)(&wq->wq_lock);
+	lockdep_assert_held(&wq->wq_lock);
 
 	if (wq->state != IDXD_WQ_ENABLED || wq->type != IDXD_WQT_KERNEL)
 		return;
@@ -1697,13 +1717,14 @@ void idxd_wq_free_irq(struct idxd_wq *wq)
  * @quarantine: the device keeps running, so the slots of the descriptors
  * it has not finished stay out of use until a drain covers them. A halt
  * has killed them and frees their slots at once.
+ * The caller holds wq_lock.
  */
-void idxd_wq_flush_descs(struct idxd_wq *wq, bool quarantine)
+void __idxd_wq_flush_descs(struct idxd_wq *wq, bool quarantine)
 {
 	struct idxd_irq_entry *ie = &wq->ie;
 	bool killed = false;
 
-	guard(mutex)(&wq->wq_lock);
+	lockdep_assert_held(&wq->wq_lock);
 
 	if (wq->state != IDXD_WQ_ENABLED || wq->type != IDXD_WQT_KERNEL)
 		return;
@@ -1714,12 +1735,8 @@ void idxd_wq_flush_descs(struct idxd_wq *wq, bool quarantine)
 	 * descriptor in the device, where the drain that frees the
 	 * quarantine covers it. A halt flush has done this itself.
 	 */
-	if (quarantine && !percpu_ref_is_dying(&wq->wq_active)) {
-		reinit_completion(&wq->wq_resurrect);
-		percpu_ref_kill(&wq->wq_active);
-		wait_for_completion(&wq->wq_dead);
-		killed = true;
-	}
+	if (quarantine)
+		killed = idxd_wq_kill(wq);
 	idxd_flush_pending_descs(ie, quarantine);
 	if (killed) {
 		percpu_ref_reinit(&wq->wq_active);
@@ -1727,6 +1744,12 @@ void idxd_wq_flush_descs(struct idxd_wq *wq, bool quarantine)
 	}
 }
 
+void idxd_wq_flush_descs(struct idxd_wq *wq, bool quarantine)
+{
+	guard(mutex)(&wq->wq_lock);
+	__idxd_wq_flush_descs(wq, quarantine);
+}
+
 int idxd_wq_request_irq(struct idxd_wq *wq)
 {
 	struct idxd_device *idxd = wq->idxd;
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index 3caf9fc00f83..0ad61bc233e8 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -826,11 +826,13 @@ int idxd_wq_map_portal(struct idxd_wq *wq);
 void idxd_wq_unmap_portal(struct idxd_wq *wq);
 int idxd_wq_set_pasid(struct idxd_wq *wq, int pasid);
 int idxd_wq_disable_pasid(struct idxd_wq *wq);
+bool idxd_wq_kill(struct idxd_wq *wq);
 void __idxd_wq_quiesce(struct idxd_wq *wq);
 void idxd_wq_quiesce(struct idxd_wq *wq);
 int idxd_wq_init_percpu_ref(struct idxd_wq *wq);
 void idxd_wq_free_irq(struct idxd_wq *wq);
 int idxd_wq_request_irq(struct idxd_wq *wq);
+void __idxd_wq_flush_descs(struct idxd_wq *wq, bool quarantine);
 void idxd_wq_flush_descs(struct idxd_wq *wq, bool quarantine);
 void idxd_wq_quarantine_take(struct idxd_wq *wq, struct list_head *list);
 void idxd_wq_quarantine_put_back(struct idxd_wq *wq, struct list_head *list);
diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c
index 1ec3d360eea8..99b2df7fc2de 100644
--- a/drivers/dma/idxd/irq.c
+++ b/drivers/dma/idxd/irq.c
@@ -422,15 +422,61 @@ static void idxd_device_flr(struct work_struct *work)
 		dev_err(&idxd->pdev->dev, "FLR failed\n");
 }
 
-static void idxd_wqs_flush_descs(struct idxd_device *idxd)
+/*
+ * Complete what the halted engines will never finish, or a client waiting
+ * on one of those descriptors waits forever. The flush hands descriptors
+ * back, so no submitter may still own one: a submitter that passed the
+ * state check before the halt can hold a descriptor on the pending list
+ * while its ENQCMDS retries, and it aborts and frees that descriptor
+ * itself when the submit fails. Killing wq_active and waiting for the
+ * last submitter to drop its reference orders the flush after that.
+ * Each queue is handled in one hold of wq_lock, from the state check
+ * through the revive. A driver remove that quiesces the queue in one
+ * hold and disables it in another, as iaa_crypto's does, would otherwise
+ * find the reference dying between the kill and the revive, skip its
+ * own kill, and have the revive undo its quiesce before its disable
+ * frees the queue.
+ * @resurrect: revive the queues afterwards for a reset that keeps them.
+ */
+static void idxd_wqs_halt_flush(struct idxd_device *idxd, bool resurrect)
 {
+	bool killed;
 	int i;
 
 	for (i = 0; i < idxd->max_wqs; i++) {
 		struct idxd_wq *wq = idxd->wqs[i];
 
-		idxd_wq_flush_descs(wq, false);
+		mutex_lock(&wq->wq_lock);
+		if (wq->state != IDXD_WQ_ENABLED || wq->type != IDXD_WQT_KERNEL) {
+			mutex_unlock(&wq->wq_lock);
+			continue;
+		}
+		killed = idxd_wq_kill(wq);
+		__idxd_wq_flush_descs(wq, false);
 		idxd_wq_halt_release(wq);
+
+		if (resurrect) {
+			/*
+			 * Revive only what this flush killed, and only if no
+			 * failed interrupt handle revoke killed it for good,
+			 * which leaves the handle invalid. The state changes
+			 * without the lock only to enabled, in the reinit of
+			 * an earlier halt, or on a queue that an FLR recovery
+			 * has unbound, whose reference reads as dying, so this
+			 * flush did not kill it. A reference an earlier halt
+			 * left dead stays dead, since that halt's recovery
+			 * unbinds the queue. The revive has to stay in this
+			 * hold: moved to the reinit, it would reopen the
+			 * window described above.
+			 */
+			if (killed && wq->ie.int_handle != INVALID_INT_HANDLE)
+				percpu_ref_reinit(&wq->wq_active);
+			complete_all(&wq->wq_resurrect);
+		} else {
+			/* Let blocked submitters retry and fail on the state. */
+			complete_all(&wq->wq_resurrect);
+		}
+		mutex_unlock(&wq->wq_lock);
 	}
 }
 
@@ -442,6 +488,8 @@ static irqreturn_t idxd_halt(struct idxd_device *idxd)
 	if (gensts.state == IDXD_DEVICE_STATE_HALT) {
 		idxd->state = IDXD_DEV_HALTED;
 		if (gensts.reset_type == IDXD_DEVICE_RESET_SOFTWARE) {
+			/* The reinit below re-enables the queues, so revive them. */
+			idxd_wqs_halt_flush(idxd, true);
 			/*
 			 * If we need a software reset, we will throw the work
 			 * on a system workqueue in order to allow interrupts
@@ -460,7 +508,7 @@ static irqreturn_t idxd_halt(struct idxd_device *idxd)
 			 * device command; they are requested again when a work
 			 * queue is re-enabled after the reset.
 			 */
-			idxd_wqs_flush_descs(idxd);
+			idxd_wqs_halt_flush(idxd, false);
 			for (i = 0; i < idxd->max_wqs; i++) {
 				struct idxd_wq *wq = idxd->wqs[i];
 
@@ -474,7 +522,8 @@ static irqreturn_t idxd_halt(struct idxd_device *idxd)
 
 		} else {
 			idxd->state = IDXD_DEV_HALTED;
-			idxd_wqs_quiesce(idxd);
+			/* Same as above: nothing in flight will ever complete. */
+			idxd_wqs_halt_flush(idxd, false);
 			idxd_wqs_unmap_portal(idxd);
 			idxd_device_clear_state(idxd);
 			dev_err(&idxd->pdev->dev,
diff --git a/drivers/dma/idxd/submit.c b/drivers/dma/idxd/submit.c
index fab3e0176cba..cf8536e94c15 100644
--- a/drivers/dma/idxd/submit.c
+++ b/drivers/dma/idxd/submit.c
@@ -132,10 +132,14 @@ static void llist_abort_desc(struct idxd_wq *wq, struct idxd_irq_entry *ie,
 			}
 
 			/*
-			 * A finished descriptor goes on the work list for
-			 * the interrupt thread, as one still in flight does,
-			 * rather than being completed in the submitter's
-			 * context.
+			 * A completion runs a client's callback or another
+			 * driver's handler, and either may submit or terminate
+			 * and so wait for a killer, which waits for this
+			 * submitter's wq_active reference. iaa_crypto's verify
+			 * of a compression may also wait for a free descriptor
+			 * while this submitter holds one. A finished
+			 * descriptor therefore goes on the work list for the
+			 * interrupt thread, as one still in flight does.
 			 */
 			if (d->completion->status)
 				wake = true;
@@ -156,10 +160,16 @@ static void llist_abort_desc(struct idxd_wq *wq, struct idxd_irq_entry *ie,
 
 	/*
 	 * The device never took the descriptor, so its record is still
-	 * clear. The interrupt thread and another submitter's abort sort
-	 * the lists under the list lock and leave one with a clear record
-	 * on a list, so the search finds the descriptor, and once off the
-	 * lists it is the submitter's alone. Marking it aborted before
+	 * clear. While this submitter holds wq_active, only the interrupt
+	 * thread and another submitter's abort may take descriptors off
+	 * the lists, and both sort them under the list lock and leave one
+	 * with a clear record on a list. A flush takes them all, so every
+	 * flusher must kill wq_active and wait for this reference before
+	 * it takes the lists, and nothing may revive the reference before
+	 * the flush runs. The error path of iaa_crypto_probe() breaks the
+	 * rule: it disables a queue it has already published without a
+	 * quiesce. Otherwise the search finds the descriptor, and once off
+	 * the lists it is the submitter's alone. Marking it aborted before
 	 * that would let another context that saw the mark on a list
 	 * complete and free it, and the caller frees it as well.
 	 */
@@ -215,6 +225,22 @@ int idxd_submit_desc(struct idxd_wq *wq, struct idxd_desc *desc)
 			return -ENXIO;
 	}
 
+	/*
+	 * A software-reset halt revives wq_active before its reinit enables
+	 * the device again. A submitter that passed the check above before
+	 * the halt and got its reference after the revive would post to the
+	 * halted device, which drops the descriptor, and nothing would
+	 * complete an interrupt-armed one. The halt stores the state before
+	 * its kill and revives the reference after the wait. The barrier
+	 * pairs with the release in percpu_ref_reinit() and orders this
+	 * read after the tryget that saw the revive.
+	 */
+	smp_rmb();
+	if (idxd->state != IDXD_DEV_ENABLED) {
+		percpu_ref_put(&wq->wq_active);
+		return -EIO;
+	}
+
 	portal = idxd_wq_portal_addr(wq);
 
 	/*
@@ -239,10 +265,18 @@ int idxd_submit_desc(struct idxd_wq *wq, struct idxd_desc *desc)
 	} else {
 		rc = idxd_enqcmds(wq, portal, desc->hw);
 		if (rc < 0) {
-			percpu_ref_put(&wq->wq_active);
-			/* abort operation frees the descriptor */
+			/*
+			 * Abort before dropping the reference: a halt flush
+			 * waits for wq_active to die before it takes the
+			 * pending descriptors, and this one must not still be
+			 * listed when it does. The abort takes the descriptor
+			 * off the lists and runs no callback, which could wait
+			 * for this reference to die. The caller completes or
+			 * frees the descriptor.
+			 */
 			if (ie)
 				llist_abort_desc(wq, ie, desc);
+			percpu_ref_put(&wq->wq_active);
 			return rc;
 		}
 	}
-- 
2.43.0