[PATCH 00/17] [PATCH 00/17] dmaengine: idxd: fixes found by an in-kernel polling client

[PATCH 00/17] [PATCH 00/17] dmaengine: idxd: fixes found by an in-kernel polling client

[Daniel Byrne]

These are fixes for bugs in the idxd driver that an in-kernel polling
client exposed while sharing DSA work queues between many io_uring rings
(that client is to be posted separately as "io_uring: DMA-engine copy offload
for buffered file I/O"). They apply to v7.3 on their own and do not
depend on that series. Each patch says which released kernels it
affects; the ones that fit stable are tagged.

In short:
 - the event-log fault path dereferences a NULL workqueue on a kernel WQ
   (stable, 6.4+), plus a hardening of the user WQ's workqueue teardown;
 - a memcpy longer than the work queue's transfer limit is accepted at
   prep and fails silently in hardware (stable, 5.10+);
 - a channel is listed before it is complete, and a failed registration
   leaves a freed channel on the list (stable, 5.13+);
 - the device driver is unbound after its work queues were freed;
 - terminate_all releases the work queue's interrupt handle for good
   (stable, 7.0+);
 - device commands wait forever, and the interrupt-handle release writes
   over a running command; the device disable misreads the fail-fast
   status as "already disabled"; the device is disabled after its
   queues' software state is gone; a reset loses the command interrupt
   it re-creates;
 - terminate_all frees descriptor slots the device still holds, and
   drains and resets report a timeout as a completed drain;
 - a descriptor whose submission failed is freed twice (stable, 5.14+);
 - an iaa_crypto descriptor is completed as a dmaengine transaction on
   abort and flush (stable, 6.8+);
 - software- and system-reset halts never flush the work queues, a halt
   flush strips the callbacks of live clients, and the interrupt-handle
   revoke runs without the queue lock.

Tested on a Granite Rapids system with four DSA devices: crc32c-verified
DMA I/O from the io_uring client under WQ unbind, live unbind and rebind,
driver unbind, PCI FLR and PCI driver unbind and rebind, on a v7.2-based
tree, clean under KASAN, DMA_API_DEBUG and lockdep; the v7.3 rebase is
conflict-free and build-tested per patch.

Daniel Byrne (17):
  dmaengine: idxd: don't queue EVL fault work for kernel WQs
  dmaengine: idxd: detach the user fault workqueue under the event log
    lock
  dmaengine: idxd: refuse a memcpy longer than the WQ max transfer size
  dmaengine: idxd: list a new channel only once it is complete
  dmaengine: idxd: unbind the device driver before its work queues go
    away
  dmaengine: idxd: keep the interrupt handle across terminate_all
  dmaengine: idxd: bound device command waits and fail fast after a
    timeout
  dmaengine: idxd: compare the device disable status code whole
  dmaengine: idxd: quarantine terminate_all's descriptor slots until the
    device has drained
  dmaengine: idxd: return -ETIMEDOUT from drains and resets and leak
    what the device may still write
  dmaengine: idxd: disable the device before its work queues are
    unregistered
  dmaengine: idxd: hold device commands off while the reset re-creates
    its interrupts
  dmaengine: idxd: fix the double free of a descriptor whose submission
    failed
  dmaengine: idxd: complete an iaa_crypto descriptor through its own
    driver on abort and flush
  dmaengine: idxd: flush the work queues on every halt type
  dmaengine: idxd: keep client callbacks on a halt flush and cancel
    resubmissions after a terminate
  dmaengine: idxd: run the interrupt-handle revoke under the queue lock

 drivers/dma/dmaengine.c   |  41 +++
 drivers/dma/idxd/cdev.c   |  62 +++-
 drivers/dma/idxd/device.c | 588 ++++++++++++++++++++++++++++++++++----
 drivers/dma/idxd/dma.c    |  60 +++-
 drivers/dma/idxd/idxd.h   |  53 +++-
 drivers/dma/idxd/init.c   |  64 ++++-
 drivers/dma/idxd/irq.c    | 258 +++++++++++++----
 drivers/dma/idxd/submit.c | 158 ++++++++--
 include/linux/dmaengine.h |   2 +
 9 files changed, 1109 insertions(+), 177 deletions(-)


base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
-- 
2.43.0

[PATCH 01/17] dmaengine: idxd: don't queue EVL fault work for kernel WQs

[Daniel Byrne]

The event log fault path queues work on wq->wq, the workqueue that
only the user (cdev) driver creates for its work queues. A completion
record fault logged for a kernel work queue, as a dmaengine or
iaa_crypto queue is, finds that pointer NULL and queue_work()
dereferences it.

The fault worker only serves user contexts: it copies the completion
record through the PASID xarray that cdev opens populate. On a kernel
work queue there is nothing to do, so log the entry ratelimited and
drop it. Test the queue's type and not the pointer alone, since a user
probe that failed leaves the pointer dangling.

Fixes: c40bd7d9737b ("dmaengine: idxd: process user page faults for completion record")
Cc: stable@vger.kernel.org # 6.4+
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/irq.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c
index 6a25e1fd0e62..820b1ff69fb6 100644
--- a/drivers/dma/idxd/irq.c
+++ b/drivers/dma/idxd/irq.c
@@ -331,10 +331,25 @@ static void process_evl_entry(struct idxd_device *idxd,
 			if (!entry_head->rcr && status == DSA_COMP_DRAIN_EVL)
 				return;
 
+			struct idxd_wq *wq = idxd->wqs[entry_head->wq_idx];
+
+			/*
+			 * The fault worker only does user-context work,
+			 * copying the completion record through the wq's
+			 * PASID xarray, which is populated by cdev opens
+			 * alone. A kernel WQ has no cdev workqueue and no
+			 * user contexts, so queueing could only warn and
+			 * drop the record. Note it and move on.
+			 */
+			if (!is_idxd_wq_user(wq) || !wq->wq) {
+				dev_warn_ratelimited(dev,
+					"EVL fault on kernel wq %d, status %#x\n",
+					wq->id, status);
+				return;
+			}
+
 			fault = kmem_cache_alloc(idxd->evl_cache, GFP_ATOMIC);
 			if (fault) {
-				struct idxd_wq *wq = idxd->wqs[entry_head->wq_idx];
-
 				fault->wq = wq;
 				fault->status = status;
 				memcpy(&fault->entry, entry_head, ent_size);
-- 
2.43.0

[PATCH 02/17] dmaengine: idxd: detach the user fault workqueue under the event log lock

[Daniel Byrne]

The event log walk tests wq->wq and queues fault work on it, and the
drain of a closing context's PASID tests it and drains it, both under
evl->lock. The user driver's remove destroyed the workqueue without
that lock, so a walk could queue on a workqueue being destroyed, and
the probe's error path destroyed it without clearing the pointer, so a
later walk or drain found a dangling one.

Detach and destroy the workqueue under evl->lock, in one helper that
the remove and the probe error path share, and clear the pointer in
both. A walk or a drain that finds it gone knows that the fault work
it would have waited for has run.

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/cdev.c | 27 ++++++++++++++++++++++-----
 drivers/dma/idxd/irq.c  |  5 ++++-
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
index 82b07cf942ef..168888adccac 100644
--- a/drivers/dma/idxd/cdev.c
+++ b/drivers/dma/idxd/cdev.c
@@ -592,6 +592,26 @@ void idxd_wq_del_cdev(struct idxd_wq *wq)
 	put_device(cdev_dev(idxd_cdev));
 }
 
+/*
+ * The event log walk tests the fault workqueue and queues on it, and the
+ * drain of a closing context's PASID tests it and drains it, both under
+ * the log's lock, which neither the probe nor the remove otherwise takes.
+ * The workqueue is therefore detached and destroyed under that lock. A
+ * walk or a drain that finds it gone knows that its work has run.
+ */
+static void idxd_user_wq_destroy_fault_wq(struct idxd_wq *wq)
+{
+	struct idxd_evl *evl = wq->idxd->evl;
+
+	if (evl)
+		mutex_lock(&evl->lock);
+	wq->type = IDXD_WQT_NONE;
+	destroy_workqueue(wq->wq);
+	wq->wq = NULL;
+	if (evl)
+		mutex_unlock(&evl->lock);
+}
+
 static int idxd_user_drv_probe(struct idxd_dev *idxd_dev)
 {
 	struct device *dev = &idxd_dev->conf_dev;
@@ -653,8 +673,7 @@ static int idxd_user_drv_probe(struct idxd_dev *idxd_dev)
 err_cdev:
 	idxd_drv_disable_wq(wq);
 err:
-	destroy_workqueue(wq->wq);
-	wq->type = IDXD_WQT_NONE;
+	idxd_user_wq_destroy_fault_wq(wq);
 wq_err:
 	mutex_unlock(&wq->wq_lock);
 	return rc;
@@ -667,9 +686,7 @@ static void idxd_user_drv_remove(struct idxd_dev *idxd_dev)
 	mutex_lock(&wq->wq_lock);
 	idxd_wq_del_cdev(wq);
 	idxd_drv_disable_wq(wq);
-	wq->type = IDXD_WQT_NONE;
-	destroy_workqueue(wq->wq);
-	wq->wq = NULL;
+	idxd_user_wq_destroy_fault_wq(wq);
 	mutex_unlock(&wq->wq_lock);
 }
 
diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c
index 820b1ff69fb6..647969920a06 100644
--- a/drivers/dma/idxd/irq.c
+++ b/drivers/dma/idxd/irq.c
@@ -339,7 +339,10 @@ static void process_evl_entry(struct idxd_device *idxd,
 			 * PASID xarray, which is populated by cdev opens
 			 * alone. A kernel WQ has no cdev workqueue and no
 			 * user contexts, so queueing could only warn and
-			 * drop the record. Note it and move on.
+			 * drop the record. Note it and move on. The user
+			 * driver detaches the workqueue under evl->lock,
+			 * which this walk holds, so the one tested here is
+			 * the one queued on.
 			 */
 			if (!is_idxd_wq_user(wq) || !wq->wq) {
 				dev_warn_ratelimited(dev,
-- 
2.43.0

[PATCH 03/17] dmaengine: idxd: refuse a memcpy longer than the WQ max transfer size

[Daniel Byrne]

idxd_dma_submit_memcpy() checks the length only against the device
limit from GENCAP. Since commit d7aad5550eca ("dmaengine: idxd: add
support for configurable max wq xfer size"), WQCFG is programmed with
the work queue's own max_transfer_size instead, and that is the limit
the hardware enforces. Since commit 92452a72ebdf ("dmaengine: idxd: set
defaults for wq configs"), that limit defaults to 2 MiB.

A copy longer than the work queue's limit is therefore accepted at prep
time and then fails in hardware with DSA_COMP_XFER_ERANGE. A client that
sets only tx->callback cannot see that error, so the copy looks
complete. Also check the work queue's limit, so that prep returns NULL
and the caller can fall back.

On a Granite Rapids DSA with the default 2 MiB work queue limit and a
device limit of 2 GiB, dmatest with norandom=1 and test_buf_size=4194304
reports a data error for every copy on v7.2: the destination is never
written, yet each copy completes. With this change the same test fails
at prep, and copies of 1 MiB and 2 MiB pass as before.

Fixes: d7aad5550eca ("dmaengine: idxd: add support for configurable max wq xfer size")
Cc: stable@vger.kernel.org # 5.10+
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Daniel Byrne <daniel1.byrne@intel.com>
---
 drivers/dma/idxd/dma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/idxd/dma.c b/drivers/dma/idxd/dma.c
index 9937b671f637..c116416e10e5 100644
--- a/drivers/dma/idxd/dma.c
+++ b/drivers/dma/idxd/dma.c
@@ -115,7 +115,8 @@ idxd_dma_submit_memcpy(struct dma_chan *c, dma_addr_t dma_dest,
 	if (wq->state != IDXD_WQ_ENABLED)
 		return NULL;
 
-	if (len > idxd->max_xfer_bytes)
+	/* The work queue's configured limit is what the hardware enforces. */
+	if (len > idxd->max_xfer_bytes || len > wq->max_xfer_bytes)
 		return NULL;
 
 	op_flag_setup(flags, &desc_flags);
-- 
2.43.0

Re: [PATCH 03/17] dmaengine: idxd: refuse a memcpy longer than the WQ max transfer size

[Frank Li]

On Mon, Oct 19, 2026 at 08:06:16AM -0700, Daniel Byrne wrote:
> [You don't often get email from daniel1.byrne@intel.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> idxd_dma_submit_memcpy() checks the length only against the device
> limit from GENCAP. Since commit d7aad5550eca ("dmaengine: idxd: add
> support for configurable max wq xfer size"), WQCFG is programmed with
> the work queue's own max_transfer_size instead, and that is the limit
> the hardware enforces. Since commit 92452a72ebdf ("dmaengine: idxd: set
> defaults for wq configs"), that limit defaults to 2 MiB.
>
> A copy longer than the work queue's limit is therefore accepted at prep
> time and then fails in hardware with DSA_COMP_XFER_ERANGE. A client that
> sets only tx->callback cannot see that error, so the copy looks
> complete. Also check the work queue's limit, so that prep returns NULL
> and the caller can fall back.
>
> On a Granite Rapids DSA with the default 2 MiB work queue limit and a
> device limit of 2 GiB, dmatest with norandom=1 and test_buf_size=4194304
> reports a data error for every copy on v7.2: the destination is never
> written, yet each copy completes. With this change the same test fails
> at prep, and copies of 1 MiB and 2 MiB pass as before.
>
> Fixes: d7aad5550eca ("dmaengine: idxd: add support for configurable max wq xfer size")
> Cc: stable@vger.kernel.org # 5.10+
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Daniel Byrne <daniel1.byrne@intel.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/dma/idxd/dma.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/idxd/dma.c b/drivers/dma/idxd/dma.c
> index 9937b671f637..c116416e10e5 100644
> --- a/drivers/dma/idxd/dma.c
> +++ b/drivers/dma/idxd/dma.c
> @@ -115,7 +115,8 @@ idxd_dma_submit_memcpy(struct dma_chan *c, dma_addr_t dma_dest,
>         if (wq->state != IDXD_WQ_ENABLED)
>                 return NULL;
>
> -       if (len > idxd->max_xfer_bytes)
> +       /* The work queue's configured limit is what the hardware enforces. */
> +       if (len > idxd->max_xfer_bytes || len > wq->max_xfer_bytes)
>                 return NULL;
>
>         op_flag_setup(flags, &desc_flags);
> --
> 2.43.0
>

[PATCH 04/17] dmaengine: idxd: list a new channel only once it is complete

[Daniel Byrne]

idxd_register_dma_channel() links a new channel into the dma_device's
channel list before it registers the channel, and sets the channel's
work queue only after that. The dma_device is registered when the
device is enabled, before any of its work queues, and a channel
request walks the channel list of every registered device under
dma_list_mutex and may hand out any listed channel without a client.
A request that runs while a work queue is being enabled can therefore
take the half-built channel, and the allocation callback dereferences
its NULL work queue with dma_list_mutex held, after which every
channel request and release hangs. The link runs without that mutex
as well, so it races the walkers.

When the registration fails, the channel is freed but left on the
list, where the next request, the next channel's link and the device's
unregistration all reach it. Registration can fail: the core frees a
channel's id before it removes the channel's sysfs device. The channel
of a work queue enabled while another queue of the same device is
being disabled can therefore be handed an id whose sysfs name still
exists, and its registration then fails with -EEXIST.

Add dma_async_device_channel_add(), which registers a channel and only
then links it into its device's list, under dma_list_mutex, and use
it. The driver sets the channel's work queue and initializes its
descriptors before the call, so a request finds the channel complete
or not at all, and a channel that failed to register is never listed.
An enable that hits the -EEXIST race still fails, but leaves nothing
on the list.

A new helper rather than a change to dma_async_device_channel_register():
that function's contract requires the caller to have linked the channel
into the device's list already, which virt-dma's vchan_init() does for
stm32-dma3, the only other caller. That driver has the same window
between the link and the registration and is left for a follow-up.

The helper serves DMA_PRIVATE devices such as idxd's only, since the
channel list of a public device is also walked under RCU alone and its
channels hold references for the dmaengine_get() users, which the
helper does not take. The unbind still unlinks the channel without the
mutex. A later change, which reworks the unbind for channels that
clients still hold, unlinks it under the mutex.

The hang is reachable since the dma_device registration moved to the
device enable in v5.13, so this is for stable too.

Fixes: 8f47d1a5e545 ("dmaengine: idxd: connect idxd to dmaengine subsystem")
Fixes: 397862855619 ("dmaengine: idxd: fix dma device lifetime")
Cc: stable@vger.kernel.org # 5.13+
Assisted-by: Claude:claude-fable-5-1
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Daniel Byrne <daniel1.byrne@intel.com>
---
 drivers/dma/dmaengine.c   | 41 +++++++++++++++++++++++++++++++++++++++
 drivers/dma/idxd/dma.c    | 12 +++++++++---
 include/linux/dmaengine.h |  2 ++
 3 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index c71763047126..ced78c985d1c 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -1155,6 +1155,47 @@ int dma_async_device_channel_register(struct dma_device *device,
 }
 EXPORT_SYMBOL_GPL(dma_async_device_channel_register);
 
+/**
+ * dma_async_device_channel_add - register a channel and make it available
+ * @device: the dmaengine device, DMA_PRIVATE and registered already
+ * @chan: the channel, not on @device's channel list
+ * @name: the channel's name, or NULL for the default one
+ *
+ * Channel requests walk the channel list of every registered device under
+ * dma_list_mutex and may hand out any listed channel that has no client.
+ * A channel added to a registered device is therefore linked to the list
+ * only once it is registered, and under that mutex, so that a request
+ * never finds it half initialized. The driver's own state behind the
+ * channel must be complete before the call. A channel that fails to
+ * register is not linked.
+ *
+ * Only a DMA_PRIVATE device may add channels this way. The channel list
+ * of a public device is also walked under RCU alone, and its channels
+ * hold references for the dmaengine_get() users, which this function
+ * does not take.
+ *
+ * Return: 0 on success or a negative errno.
+ */
+int dma_async_device_channel_add(struct dma_device *device,
+				 struct dma_chan *chan, const char *name)
+{
+	int rc;
+
+	if (WARN_ON_ONCE(!dma_has_cap(DMA_PRIVATE, device->cap_mask)))
+		return -EINVAL;
+
+	rc = __dma_async_device_channel_register(device, chan, name);
+	if (rc < 0)
+		return rc;
+
+	mutex_lock(&dma_list_mutex);
+	list_add_tail(&chan->device_node, &device->channels);
+	dma_channel_rebalance();
+	mutex_unlock(&dma_list_mutex);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dma_async_device_channel_add);
+
 static void __dma_async_device_channel_unregister(struct dma_device *device,
 						  struct dma_chan *chan)
 {
diff --git a/drivers/dma/idxd/dma.c b/drivers/dma/idxd/dma.c
index c116416e10e5..defd1af74c0a 100644
--- a/drivers/dma/idxd/dma.c
+++ b/drivers/dma/idxd/dma.c
@@ -279,7 +279,7 @@ static int idxd_register_dma_channel(struct idxd_wq *wq)
 
 	chan = &idxd_chan->chan;
 	chan->device = dma;
-	list_add_tail(&chan->device_node, &dma->channels);
+	idxd_chan->wq = wq;
 
 	for (i = 0; i < wq->num_descs; i++) {
 		struct idxd_desc *desc = wq->descs[i];
@@ -288,14 +288,20 @@ static int idxd_register_dma_channel(struct idxd_wq *wq)
 		desc->txd.tx_submit = idxd_dma_tx_submit;
 	}
 
-	rc = dma_async_device_channel_register(dma, chan, NULL);
+	/*
+	 * The dma_device is registered already, so a channel request may
+	 * take the channel as soon as it is on the device's list. The core
+	 * links it only once it is registered, and everything a client
+	 * reaches through it is set up by then. What follows serves the
+	 * unbind, which cannot run before this probe returns.
+	 */
+	rc = dma_async_device_channel_add(dma, chan, NULL);
 	if (rc < 0) {
 		kfree(idxd_chan);
 		return rc;
 	}
 
 	wq->idxd_chan = idxd_chan;
-	idxd_chan->wq = wq;
 	get_device(wq_confdev(wq));
 
 	return 0;
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index fe33a20abc61..4178248611d9 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -1751,6 +1751,8 @@ static inline int dmaengine_desc_free(struct dma_async_tx_descriptor *desc)
 int dma_async_device_register(struct dma_device *device);
 int dmaenginem_async_device_register(struct dma_device *device);
 void dma_async_device_unregister(struct dma_device *device);
+int dma_async_device_channel_add(struct dma_device *device,
+				 struct dma_chan *chan, const char *name);
 int dma_async_device_channel_register(struct dma_device *device,
 				      struct dma_chan *chan,
 				      const char *name);
-- 
2.43.0

[PATCH 05/17] dmaengine: idxd: unbind the device driver before its work queues go away

[Daniel Byrne]

idxd_remove() unregisters the work queue conf_devs first, and each
queue is freed by its conf_dev release. Only then does it unregister
the device's own conf_dev, whose driver remove, idxd_device_drv_remove(),
walks idxd->wqs and reads wq->state to disable the queues still
active. On a PCI unbind of an enabled device that is a read of freed
memory for every queue, which KASAN reports as a slab-use-after-free.
The usual administrative flow disables the device first and never hits
it.

Release the device driver before the queues are unregistered, so its
remove sees them while they exist. The conf_dev is unregistered where it
was, now without a driver bound, and the reference taken there only
keeps the memory alive for the remaining cleanup.

That remove also releases the driver of every bound queue, not only of
the queues that read enabled. A halt's clear_state leaves a bound queue
disabled, and skipping it would unregister the dma_device while that
queue's channel is still registered, so the queue's own unbind later
freed the channel and the device a second time.

The devices on the dsa bus stay open to a sysfs bind until they are
unregistered, and the bus does not take the parent's lock, so the PCI
device lock that the remove holds does not exclude a bind. The early
unbind leaves the device reading enabled while its queues' drivers are
released, and disabled before it is unregistered, which a queue probe
and the device probe accept respectively. The remove therefore marks
the device as going away before it unbinds the driver, and the device
probe and the queue enable, which every queue driver's probe calls,
refuse a device so marked, as the driver core did once the devices
were unregistered.

Fixes: 745e92a6d816 ("dmaengine: idxd: idxd: move remove() bits for idxd 'struct device' to device.c")
Assisted-by: Claude:claude-fable-5-1
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Daniel Byrne <daniel1.byrne@intel.com>
---
 drivers/dma/idxd/device.c | 26 +++++++++++++++++++++++---
 drivers/dma/idxd/idxd.h   |  1 +
 drivers/dma/idxd/init.c   | 28 +++++++++++++++++++++++-----
 3 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index 131138483b87..a83b5da88f8c 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -1415,6 +1415,16 @@ int idxd_drv_enable_wq(struct idxd_wq *wq)
 
 	lockdep_assert_held(&wq->wq_lock);
 
+	/*
+	 * The device remove releases the queues' drivers before it disables
+	 * the device, so the device still reads enabled meanwhile.
+	 */
+	if (test_bit(IDXD_FLAG_REMOVING, &idxd->flags)) {
+		idxd->cmd_status = IDXD_SCMD_DEV_NOT_ENABLED;
+		rc = -ENODEV;
+		goto err;
+	}
+
 	if (idxd->state != IDXD_DEV_ENABLED) {
 		idxd->cmd_status = IDXD_SCMD_DEV_NOT_ENABLED;
 		goto err;
@@ -1563,6 +1573,9 @@ int idxd_device_drv_probe(struct idxd_dev *idxd_dev)
 	struct idxd_device *idxd = idxd_dev_to_idxd(idxd_dev);
 	int rc = 0;
 
+	if (test_bit(IDXD_FLAG_REMOVING, &idxd->flags))
+		return -ENODEV;
+
 	/*
 	 * Device should be in disabled state for the idxd_drv to load. If it's in
 	 * enabled state, then the device was altered outside of driver's control.
@@ -1623,9 +1636,16 @@ void idxd_device_drv_remove(struct idxd_dev *idxd_dev)
 		struct idxd_wq *wq = idxd->wqs[i];
 		struct device *wq_dev = wq_confdev(wq);
 
-		if (wq->state == IDXD_WQ_DISABLED)
-			continue;
-		dev_warn(dev, "Active wq %d on disable %s.\n", i, dev_name(wq_dev));
+		/*
+		 * A halt's clear_state leaves a bound queue DISABLED while
+		 * its driver still holds the queue's channel and resources.
+		 * Release every bound driver, not only the enabled queues',
+		 * before the dma_device below frees their channels. For a
+		 * queue without a driver the release is a no-op.
+		 */
+		if (wq->state != IDXD_WQ_DISABLED)
+			dev_warn(dev, "Active wq %d on disable %s.\n", i,
+				 dev_name(wq_dev));
 		device_release_driver(wq_dev);
 	}
 
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index ce78b9a7c641..2343f0367d62 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -269,6 +269,7 @@ enum idxd_device_flag {
 	IDXD_FLAG_CMD_RUNNING,
 	IDXD_FLAG_PASID_ENABLED,
 	IDXD_FLAG_USER_PASID_ENABLED,
+	IDXD_FLAG_REMOVING,	/* PCI remove under way, refuse binds */
 };
 
 struct idxd_dma_dev {
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index 4b827a329756..4443d23fde5a 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -1267,13 +1267,31 @@ static void idxd_remove(struct pci_dev *pdev)
 {
 	struct idxd_device *idxd = pci_get_drvdata(pdev);
 
+	/*
+	 * The devices on the dsa bus stay registered, and so open to a
+	 * sysfs bind, until the unregisters below. A bind during the
+	 * removal would re-probe the device or enable one of its queues
+	 * while they are torn down, and the PCI device lock held here does
+	 * not exclude it, so the probes test this first. The command path
+	 * changes IDXD_FLAG_CMD_RUNNING in the same word with non-atomic
+	 * bit operations under cmd_lock, which would lose a bit set
+	 * concurrently, so the flag is set under that lock too.
+	 */
+	spin_lock_irq(&idxd->cmd_lock);
+	set_bit(IDXD_FLAG_REMOVING, &idxd->flags);
+	spin_unlock_irq(&idxd->cmd_lock);
+	/*
+	 * The idxd sub-driver's remove walks idxd->wqs to disable the
+	 * active queues, and each wq is freed with its conf_dev. Unbind
+	 * that driver while the queues still exist.
+	 */
+	device_release_driver(idxd_confdev(idxd));
 	idxd_unregister_devices(idxd);
 	/*
-	 * When ->release() is called for the idxd->conf_dev, it frees all the memory related
-	 * to the idxd context. The driver still needs those bits in order to do the rest of
-	 * the cleanup. However, we do need to unbound the idxd sub-driver. So take a ref
-	 * on the device here to hold off the freeing while allowing the idxd sub-driver
-	 * to unbind.
+	 * When ->release() is called for the idxd->conf_dev, it frees all
+	 * the memory related to the idxd context. The driver still needs
+	 * those bits in order to do the rest of the cleanup, so take a ref
+	 * on the device here to hold off the freeing until that is done.
 	 */
 	get_device(idxd_confdev(idxd));
 	device_unregister(idxd_confdev(idxd));
-- 
2.43.0

[PATCH 06/17] dmaengine: idxd: keep the interrupt handle across terminate_all

[Daniel Byrne]

idxd_wq_flush_descs(), the device_terminate_all() implementation,
released the work queue's interrupt handle and cleared its permission
entry after flushing the pending descriptors. The handle is only
requested again when the driver rebinds. On a live channel every
descriptor submitted with a completion interrupt after a terminate
therefore fails with DSA_COMP_INT_HANDLE_INVAL, and since no
interrupt is raised for such a descriptor the client never learns of
it and waits forever. A single dmaengine_terminate_sync() on an error
path, which the API allows, disabled the channel for good.

Flush the pending descriptors only. Releasing the handle and clearing
the permission entry belong to idxd_wq_free_irq(), which already does
both when the interrupt is torn down. idxd_wq_flush_descs() is also the
flush of the FLR halt path, which relied on it to mark the handles
invalid. The FLR revokes every handle, so that path now marks them
invalid itself, without a device command, and the teardown after the
reset skips the release of a handle the reset has already revoked.

At this point idxd_flush_pending_descs() still frees the slots of
in-flight interrupt-armed descriptors while the device may hold them.
A later patch in this series, "dmaengine: idxd: quarantine
terminate_all's descriptor slots until the device has drained",
addresses that.

Fixes: 2a93f5747d0e ("dmaengine: idxd: Flush all pending descriptors")
Cc: stable@vger.kernel.org # 7.0+
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 |  7 +------
 drivers/dma/idxd/irq.c    | 16 +++++++++++++---
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index a83b5da88f8c..a461cae7a409 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -1336,7 +1336,7 @@ void idxd_wq_free_irq(struct idxd_wq *wq)
 	free_irq(ie->vector, ie);
 	idxd_flush_pending_descs(ie);
 
-	/* The interrupt might have been already released by FLR */
+	/* FLR revokes every handle; the halt path marks them invalid. */
 	if (ie->int_handle == INVALID_INT_HANDLE)
 		return;
 
@@ -1351,7 +1351,6 @@ void idxd_wq_free_irq(struct idxd_wq *wq)
 void idxd_wq_flush_descs(struct idxd_wq *wq)
 {
 	struct idxd_irq_entry *ie = &wq->ie;
-	struct idxd_device *idxd = wq->idxd;
 
 	guard(mutex)(&wq->wq_lock);
 
@@ -1359,10 +1358,6 @@ void idxd_wq_flush_descs(struct idxd_wq *wq)
 		return;
 
 	idxd_flush_pending_descs(ie);
-	if (idxd->request_int_handles)
-		idxd_device_release_int_handle(idxd, ie->int_handle, IDXD_IRQ_MSIX);
-	idxd_device_clear_perm_entry(idxd, ie);
-	ie->int_handle = INVALID_INT_HANDLE;
 }
 
 int idxd_wq_request_irq(struct idxd_wq *wq)
diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c
index 647969920a06..915304ee7099 100644
--- a/drivers/dma/idxd/irq.c
+++ b/drivers/dma/idxd/irq.c
@@ -442,13 +442,23 @@ static irqreturn_t idxd_halt(struct idxd_device *idxd)
 			INIT_WORK(&idxd->work, idxd_device_reinit);
 			queue_work(idxd->wq, &idxd->work);
 		} else if (gensts.reset_type == IDXD_DEVICE_RESET_FLR) {
+			int i;
+
 			idxd->state = IDXD_DEV_HALTED;
 			idxd_mask_error_interrupts(idxd);
-			/* Flush all pending descriptors, and disable
-			 * interrupts, they will be re-enabled when FLR
-			 * concludes.
+			/*
+			 * Flush all pending descriptors. The FLR revokes every
+			 * interrupt handle, so mark them invalid here without a
+			 * device command; they are requested again when a work
+			 * queue is re-enabled after the reset.
 			 */
 			idxd_wqs_flush_descs(idxd);
+			for (i = 0; i < idxd->max_wqs; i++) {
+				struct idxd_wq *wq = idxd->wqs[i];
+
+				if (wq->type == IDXD_WQT_KERNEL)
+					WRITE_ONCE(wq->ie.int_handle, INVALID_INT_HANDLE);
+			}
 			dev_dbg(&idxd->pdev->dev,
 				"idxd halted, doing FLR. After FLR, configs are restored\n");
 			INIT_WORK(&idxd->work, idxd_device_flr);
-- 
2.43.0

[PATCH 07/17] dmaengine: idxd: bound device command waits and fail fast after a timeout

[Daniel Byrne]

idxd_cmd_exec() waits forever, twice: first for a previous command to
finish, then for the interrupt that completes its own command. A device
command that never completes therefore leaves the caller in an
uninterruptible sleep, and every subsequent command joins the queue
behind it. Observed with a DRAIN_WQ issued from cdev release while a
descriptor sat blocked on a page fault that could not be serviced: the
releasing process became unkillable, every later process exit queued
behind it in D state, and the reboot hung until a power cycle.

Bound both waits at 30 seconds; a drain of a loaded work queue is the
slowest legitimate command and healthy commands complete in
microseconds. Only the owner of a command can tell that it is stuck. A
caller waiting for the running command restarts its budget whenever
another command starts running and gives up when the owner marks the
device, so healthy commands queued behind one another never add up to
a false timeout. On its own timeout the owner sets
IDXD_FLAG_CMD_TIMEDOUT, so waiters and later commands fail fast with
IDXD_CMDSTS_HW_ERR, and leaves IDXD_FLAG_CMD_RUNNING set, since the
device may still be executing the command and writing another to the
command register would be undefined. The command completion lives on
the owner's stack, so it is detached under cmd_lock on timeout, and
the interrupt handler checks under that lock that it is still set.

A timeout fences the device: every command fails fast and nothing is
freed. A late completion interrupt clears both flags, so a device that
was merely slow is usable again, and so do a PCI reset and the recovery
from a halt, which also forget the running command and wake its owner
instead of letting it sleep out the timeout. The owner checks under
cmd_lock that the command is still its own before it reads the status;
a forgotten command is reported failed and marks nothing. There is no
sysfs lever to clear the state.

The interrupt-handle release polled the command register under
cmd_lock, which idxd_cmd_exec() does not hold while its command runs,
so it could be written over a running command, and it spun unbounded
with preemption off. Send it through idxd_cmd_exec() like the request.

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 | 157 +++++++++++++++++++++++++++++++++-----
 drivers/dma/idxd/idxd.h   |   3 +
 drivers/dma/idxd/init.c   |   9 +++
 drivers/dma/idxd/irq.c    |  25 +++++-
 4 files changed, 175 insertions(+), 19 deletions(-)

diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index a461cae7a409..5ebc2c1a3859 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -12,6 +12,9 @@
 #include "idxd.h"
 #include "registers.h"
 
+/* generous bound: drain of a loaded WQ is the slowest legitimate command */
+#define IDXD_CMD_TIMEOUT_MS	30000
+
 static void idxd_cmd_exec(struct idxd_device *idxd, int cmd_code, u32 operand,
 			  u32 *status);
 static void idxd_device_wqs_clear_state(struct idxd_device *idxd);
@@ -498,7 +501,7 @@ static void idxd_cmd_exec(struct idxd_device *idxd, int cmd_code, u32 operand,
 	union idxd_command_reg cmd;
 	DECLARE_COMPLETION_ONSTACK(done);
 	u32 stat;
-	unsigned long flags;
+	unsigned long flags, left;
 
 	if (idxd_device_is_halted(idxd)) {
 		dev_warn(&idxd->pdev->dev, "Device is HALTED!\n");
@@ -507,21 +510,69 @@ static void idxd_cmd_exec(struct idxd_device *idxd, int cmd_code, u32 operand,
 		return;
 	}
 
+	if (test_bit(IDXD_FLAG_CMD_TIMEDOUT, &idxd->flags)) {
+		dev_warn(&idxd->pdev->dev,
+			 "Not sending command %#x: an earlier device command timed out, device needs a reset\n",
+			 cmd_code);
+		if (status)
+			*status = IDXD_CMDSTS_HW_ERR;
+		return;
+	}
+
 	memset(&cmd, 0, sizeof(cmd));
 	cmd.cmd = cmd_code;
 	cmd.operand = operand;
 	cmd.int_req = 1;
 
 	spin_lock_irqsave(&idxd->cmd_lock, flags);
-	wait_event_lock_irq(idxd->cmd_waitq,
-			    !test_bit(IDXD_FLAG_CMD_RUNNING, &idxd->flags),
-			    idxd->cmd_lock);
+	/*
+	 * Wait for the running command, if any. Only its owner can tell
+	 * that it is stuck, so this wait does not declare a timeout of its
+	 * own: its budget covers one owner and restarts whenever another
+	 * command starts running, and it ends early when the owner gives
+	 * up and marks the device. Several healthy commands queued behind
+	 * one another therefore never add up to a false timeout.
+	 */
+	while (test_bit(IDXD_FLAG_CMD_RUNNING, &idxd->flags)) {
+		u32 seq = idxd->cmd_seq;
+
+		if (test_bit(IDXD_FLAG_CMD_TIMEDOUT, &idxd->flags)) {
+			spin_unlock_irqrestore(&idxd->cmd_lock, flags);
+			dev_err(&idxd->pdev->dev,
+				"not sending command %#x: the running device command timed out, device needs a reset\n",
+				cmd_code);
+			if (status)
+				*status = IDXD_CMDSTS_HW_ERR;
+			return;
+		}
+		if (!wait_event_lock_irq_timeout(idxd->cmd_waitq,
+				!test_bit(IDXD_FLAG_CMD_RUNNING, &idxd->flags) ||
+				test_bit(IDXD_FLAG_CMD_TIMEDOUT, &idxd->flags) ||
+				idxd->cmd_seq != seq,
+				idxd->cmd_lock,
+				msecs_to_jiffies(IDXD_CMD_TIMEOUT_MS))) {
+			/*
+			 * The owner started before this wait and has the
+			 * same budget, so its own timeout is due; it marks
+			 * the device and wakes this queue. Give up without
+			 * marking anything here.
+			 */
+			spin_unlock_irqrestore(&idxd->cmd_lock, flags);
+			dev_err(&idxd->pdev->dev,
+				"timed out waiting for the running device command before %#x\n",
+				cmd_code);
+			if (status)
+				*status = IDXD_CMDSTS_HW_ERR;
+			return;
+		}
+	}
 
 	dev_dbg(&idxd->pdev->dev, "%s: sending cmd: %#x op: %#x\n",
 		__func__, cmd_code, operand);
 
 	idxd->cmd_status = 0;
 	__set_bit(IDXD_FLAG_CMD_RUNNING, &idxd->flags);
+	idxd->cmd_seq++;
 	idxd->cmd_done = &done;
 	iowrite32(cmd.bits, idxd->reg_base + IDXD_CMD_OFFSET);
 
@@ -530,9 +581,57 @@ static void idxd_cmd_exec(struct idxd_device *idxd, int cmd_code, u32 operand,
 	 * the command completes via interrupt.
 	 */
 	spin_unlock_irqrestore(&idxd->cmd_lock, flags);
-	wait_for_completion(&done);
-	stat = ioread32(idxd->reg_base + IDXD_CMDSTS_OFFSET);
+	left = wait_for_completion_timeout(&done,
+				msecs_to_jiffies(IDXD_CMD_TIMEOUT_MS));
 	spin_lock(&idxd->cmd_lock);
+	/*
+	 * A reset, or the recovery from a halt, forgot this command
+	 * meanwhile, and another may be running by now. Forgetting the
+	 * command completes the wait, but it can also happen between the
+	 * interrupt's completion and this lock, or after the wait ran out,
+	 * so ownership is checked whichever way the wait ended. The device
+	 * is not stuck on this command, and the command state belongs to
+	 * whatever runs now. Mark nothing and leave that state alone, even
+	 * if this command completed just before the reset or the halt: its
+	 * status can no longer be trusted.
+	 */
+	if (idxd->cmd_done != &done) {
+		spin_unlock(&idxd->cmd_lock);
+		dev_err(&idxd->pdev->dev,
+			"device command %#x was ended by a reset\n",
+			cmd_code);
+		if (status)
+			*status = IDXD_CMDSTS_HW_ERR;
+		return;
+	}
+	/* A timeout, unless the interrupt beat the lock. */
+	if (!left && !completion_done(&done)) {
+		/*
+		 * The device did not complete the command. Detach the
+		 * on-stack completion so a late interrupt cannot touch it,
+		 * and leave IDXD_FLAG_CMD_RUNNING set: the device may still
+		 * be executing, so no further commands may be issued.
+		 * IDXD_FLAG_CMD_TIMEDOUT makes waiters and future callers
+		 * fail fast; a late completion interrupt or a device reset
+		 * clears both flags.
+		 */
+		idxd->cmd_done = NULL;
+		set_bit(IDXD_FLAG_CMD_TIMEDOUT, &idxd->flags);
+		wake_up(&idxd->cmd_waitq);
+		spin_unlock(&idxd->cmd_lock);
+		dev_err(&idxd->pdev->dev,
+			"device command %#x timed out, device needs a reset\n",
+			cmd_code);
+		if (status)
+			*status = IDXD_CMDSTS_HW_ERR;
+		return;
+	}
+	/*
+	 * Read the status while this command still owns the register: no
+	 * other command is written before RUNNING is cleared below.
+	 */
+	stat = ioread32(idxd->reg_base + IDXD_CMDSTS_OFFSET);
+	idxd->cmd_done = NULL;
 	if (status)
 		*status = stat;
 	idxd->cmd_status = stat & GENMASK(7, 0);
@@ -543,6 +642,33 @@ static void idxd_cmd_exec(struct idxd_device *idxd, int cmd_code, u32 operand,
 	spin_unlock(&idxd->cmd_lock);
 }
 
+/*
+ * Forget the running command, whether its owner still waits for it or
+ * has given up on it. Called when the device has been reset or has
+ * halted, so the command can no longer be executing. A waiting owner is
+ * woken instead of sleeping out its timeout. It finds that it no longer
+ * owns the command and returns without touching the command state,
+ * which belongs to whatever command runs next.
+ */
+void idxd_cmd_reset_state(struct idxd_device *idxd)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&idxd->cmd_lock, flags);
+	/*
+	 * The completion is on the owner's stack. It is valid while
+	 * cmd_done points to it, since the owner clears cmd_done, or sees
+	 * that it points elsewhere, under cmd_lock before it returns.
+	 */
+	if (idxd->cmd_done)
+		complete(idxd->cmd_done);
+	idxd->cmd_done = NULL;
+	__clear_bit(IDXD_FLAG_CMD_RUNNING, &idxd->flags);
+	clear_bit(IDXD_FLAG_CMD_TIMEDOUT, &idxd->flags);
+	wake_up(&idxd->cmd_waitq);
+	spin_unlock_irqrestore(&idxd->cmd_lock, flags);
+}
+
 int idxd_device_enable(struct idxd_device *idxd)
 {
 	struct device *dev = &idxd->pdev->dev;
@@ -644,31 +770,20 @@ int idxd_device_release_int_handle(struct idxd_device *idxd, int handle,
 {
 	struct device *dev = &idxd->pdev->dev;
 	u32 operand, status;
-	union idxd_command_reg cmd;
 
 	if (!(idxd->hw.cmd_cap & BIT(IDXD_CMD_RELEASE_INT_HANDLE)))
 		return -EOPNOTSUPP;
 
 	dev_dbg(dev, "release int handle, handle %d\n", handle);
 
-	memset(&cmd, 0, sizeof(cmd));
 	operand = handle & GENMASK(15, 0);
 
 	if (irq_type == IDXD_IRQ_IMS)
 		operand |= CMD_INT_HANDLE_IMS;
 
-	cmd.cmd = IDXD_CMD_RELEASE_INT_HANDLE;
-	cmd.operand = operand;
-
 	dev_dbg(dev, "cmd: %u operand: %#x\n", IDXD_CMD_RELEASE_INT_HANDLE, operand);
 
-	spin_lock(&idxd->cmd_lock);
-	iowrite32(cmd.bits, idxd->reg_base + IDXD_CMD_OFFSET);
-
-	while (ioread32(idxd->reg_base + IDXD_CMDSTS_OFFSET) & IDXD_CMDSTS_ACTIVE)
-		cpu_relax();
-	status = ioread32(idxd->reg_base + IDXD_CMDSTS_OFFSET);
-	spin_unlock(&idxd->cmd_lock);
+	idxd_cmd_exec(idxd, IDXD_CMD_RELEASE_INT_HANDLE, operand, &status);
 
 	if ((status & IDXD_CMDSTS_ERR_MASK) != IDXD_CMDSTS_SUCCESS) {
 		dev_dbg(dev, "release int handle failed: %#x\n", status);
@@ -1340,6 +1455,12 @@ void idxd_wq_free_irq(struct idxd_wq *wq)
 	if (ie->int_handle == INVALID_INT_HANDLE)
 		return;
 
+	/*
+	 * The release waits for a running command like any other command.
+	 * It is not sent, or does not complete, only on a halted device or
+	 * after a command timed out, and the reset such a device needs
+	 * revokes the handle. The handle is forgotten in any case.
+	 */
 	if (idxd->request_int_handles)
 		idxd_device_release_int_handle(idxd, ie->int_handle, IDXD_IRQ_MSIX);
 	idxd_device_clear_perm_entry(idxd, ie);
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index 2343f0367d62..221a7302bb0c 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -267,6 +267,7 @@ enum idxd_device_state {
 enum idxd_device_flag {
 	IDXD_FLAG_CONFIGURABLE = 0,
 	IDXD_FLAG_CMD_RUNNING,
+	IDXD_FLAG_CMD_TIMEDOUT,
 	IDXD_FLAG_PASID_ENABLED,
 	IDXD_FLAG_USER_PASID_ENABLED,
 	IDXD_FLAG_REMOVING,	/* PCI remove under way, refuse binds */
@@ -332,6 +333,7 @@ struct idxd_device {
 	spinlock_t dev_lock;	/* spinlock for device */
 	spinlock_t cmd_lock;	/* spinlock for device commands */
 	struct completion *cmd_done;
+	u32 cmd_seq;		/* bumps when a command starts running */
 	struct idxd_group **groups;
 	struct idxd_wq **wqs;
 	struct idxd_engine **engines;
@@ -779,6 +781,7 @@ int idxd_device_enable(struct idxd_device *idxd);
 int idxd_device_disable(struct idxd_device *idxd);
 void idxd_device_reset(struct idxd_device *idxd);
 void idxd_device_clear_state(struct idxd_device *idxd);
+void idxd_cmd_reset_state(struct idxd_device *idxd);
 int idxd_device_config(struct idxd_device *idxd);
 void idxd_device_drain_pasid(struct idxd_device *idxd, int pasid);
 int idxd_device_load_config(struct idxd_device *idxd);
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index 4443d23fde5a..9c47a7b9e48e 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -1057,6 +1057,15 @@ static void idxd_reset_done(struct pci_dev *pdev)
 	struct device *dev;
 	int rc, i;
 
+	/*
+	 * A reset ends any command, including one that had timed out. The
+	 * PCI core resets the device even when the preparation failed and
+	 * the device is not rebound, and it calls this after a reset that
+	 * failed as well, which cannot be told apart here. Like the rebind
+	 * below, this assumes that the reset ran.
+	 */
+	idxd_cmd_reset_state(idxd);
+
 	if (!idxd->idxd_saved)
 		return;
 
diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c
index 915304ee7099..acb901e59a00 100644
--- a/drivers/dma/idxd/irq.c
+++ b/drivers/dma/idxd/irq.c
@@ -35,6 +35,11 @@ static void idxd_device_reinit(struct work_struct *work)
 	struct device *dev = &idxd->pdev->dev;
 	int rc, i;
 
+	/*
+	 * The halt ended whatever command was running, and a command that
+	 * timed out before the halt would otherwise refuse the reset.
+	 */
+	idxd_cmd_reset_state(idxd);
 	idxd_device_reset(idxd);
 	rc = idxd_device_config(idxd);
 	if (rc < 0)
@@ -550,7 +555,25 @@ irqreturn_t idxd_misc_thread(int vec, void *data)
 
 	if (cause & IDXD_INTC_CMD) {
 		val |= IDXD_INTC_CMD;
-		complete(idxd->cmd_done);
+		/*
+		 * cmd_done is cleared under cmd_lock when idxd_cmd_exec()
+		 * gives up on a command: the completion lives on its stack,
+		 * so a late interrupt must not touch it.
+		 */
+		spin_lock(&idxd->cmd_lock);
+		if (idxd->cmd_done) {
+			complete(idxd->cmd_done);
+		} else if (test_bit(IDXD_FLAG_CMD_TIMEDOUT, &idxd->flags)) {
+			/*
+			 * The command its owner gave up on has finished after
+			 * all. The device is usable again, so let the queue go.
+			 */
+			__clear_bit(IDXD_FLAG_CMD_RUNNING, &idxd->flags);
+			clear_bit(IDXD_FLAG_CMD_TIMEDOUT, &idxd->flags);
+			wake_up(&idxd->cmd_waitq);
+			dev_warn(dev, "a timed-out device command completed late\n");
+		}
+		spin_unlock(&idxd->cmd_lock);
 	}
 
 	if (cause & IDXD_INTC_OCCUPY) {
-- 
2.43.0

[PATCH 08/17] dmaengine: idxd: compare the device disable status code whole

[Daniel Byrne]

idxd_device_disable() accepts the command's status when it is success
or when `status & IDXD_CMDSTS_ERR_DIS_DEV_EN` is non-zero. The error
code is a value, not a bit mask, so that test accepts every code that
has bit 0 or bits 4-5 set, and IDXD_CMDSTS_HW_ERR (0x3), the code a
refused or timed-out command reports, is one of them. A disable that
the device refused then read as a device already disabled, after which
the device-level remove cleared the software state and freed memory
that the still-enabled device writes.

Compare the error code whole against the already-disabled code.

Fixes: bfe1d56091c1 ("dmaengine: idxd: Init and probe for Intel data accelerators")
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 | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index 5ebc2c1a3859..41223d2e20af 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -704,9 +704,16 @@ int idxd_device_disable(struct idxd_device *idxd)
 
 	idxd_cmd_exec(idxd, IDXD_CMD_DISABLE_DEVICE, 0, &status);
 
-	/* If the command is successful or if the device was disabled */
+	/*
+	 * The command succeeded, or the device was already disabled. The
+	 * error code is a value, not a bit mask, so compare it whole. A
+	 * refused command reports IDXD_CMDSTS_HW_ERR, which shares a bit
+	 * with the already-disabled code but leaves the device enabled,
+	 * and clearing the software state of an enabled device lets a
+	 * later teardown free memory the device still writes.
+	 */
 	if (status != IDXD_CMDSTS_SUCCESS &&
-	    !(status & IDXD_CMDSTS_ERR_DIS_DEV_EN)) {
+	    (status & IDXD_CMDSTS_ERR_MASK) != IDXD_CMDSTS_ERR_DIS_DEV_EN) {
 		dev_dbg(dev, "%s: err_code: %#x\n", __func__, status);
 		return -ENXIO;
 	}
-- 
2.43.0

[PATCH 09/17] dmaengine: idxd: quarantine terminate_all's descriptor slots until the device has drained

[Daniel Byrne]

idxd_dma_terminate_all() flushes the pending descriptors and frees their
slots while the device still holds them. A late completion record then
lands on the slot's next owner and completes it with the old
descriptor's status, and the unmap data the client attached is released
while the device may still access the memory it maps; with an IOMMU
another mapping can reuse the released address and take the late
access.

Keep those slots out of use instead. terminate_all now waits for the
submitters in flight, so that every descriptor on the lists is in the
device, and the flush moves the descriptors the device has not finished
to a per-queue quarantine, with their unmap data, after it has completed
their cookies. device_synchronize() takes the quarantine, drains the
queue, which covers everything the device held when the drain was
issued, and only then frees the slots and the unmap data. A descriptor
terminated after the take waits for the next synchronize. The halt flush
frees the quarantine itself, since the halt killed every descriptor the
device held, and so does the queue disable once its drain and reset
have completed.

Fixes: 2a93f5747d0e ("dmaengine: idxd: Flush all pending descriptors")
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 | 117 ++++++++++++++++++++++++++++++++++++--
 drivers/dma/idxd/dma.c    |  12 +++-
 drivers/dma/idxd/idxd.h   |  12 +++-
 drivers/dma/idxd/init.c   |   1 +
 drivers/dma/idxd/irq.c    |   3 +-
 5 files changed, 138 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index 41223d2e20af..09475f5ea026 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -118,6 +118,8 @@ int idxd_wq_alloc_resources(struct idxd_wq *wq)
 	if (wq->type != IDXD_WQT_KERNEL)
 		return 0;
 
+	/* A fresh pool: nothing outstanding can land in it. */
+	INIT_LIST_HEAD(&wq->quarantine);
 	num_descs = wq_dedicated(wq) ? wq->size : wq->threshold;
 	wq->num_descs = num_descs;
 
@@ -170,10 +172,18 @@ EXPORT_SYMBOL_NS_GPL(idxd_wq_alloc_resources, "IDXD");
 void idxd_wq_free_resources(struct idxd_wq *wq)
 {
 	struct device *dev = &wq->idxd->pdev->dev;
+	LIST_HEAD(dead_descs);
 
 	if (wq->type != IDXD_WQT_KERNEL)
 		return;
 
+	/*
+	 * The disable frees the pool only after its drain and its reset
+	 * completed, and a failed enable has run nothing, so the device no
+	 * longer reaches what a quarantined descriptor maps.
+	 */
+	idxd_wq_quarantine_take(wq, &dead_descs);
+	idxd_wq_quarantine_free(wq, &dead_descs);
 	free_hw_descs(wq);
 	free_descs(wq);
 	dma_free_coherent(dev, wq->compls_size, wq->compls, wq->compls_addr);
@@ -1391,11 +1401,14 @@ int idxd_device_load_config(struct idxd_device *idxd)
 	return 0;
 }
 
-static void idxd_flush_pending_descs(struct idxd_irq_entry *ie)
+static void idxd_flush_pending_descs(struct idxd_irq_entry *ie,
+				     bool quarantine)
 {
+	struct idxd_wq *wq = ie_to_wq(ie);
 	struct idxd_desc *desc, *itr;
 	struct llist_node *head;
 	LIST_HEAD(flist);
+	LIST_HEAD(qlist);
 	enum idxd_complete_type ctype;
 
 	spin_lock(&ie->list_lock);
@@ -1423,8 +1436,82 @@ static void idxd_flush_pending_descs(struct idxd_irq_entry *ie)
 		tx = &desc->txd;
 		tx->callback = NULL;
 		tx->callback_result = NULL;
+		/*
+		 * A descriptor the device has not finished still has its
+		 * completion record to write. A flush that leaves the
+		 * device running keeps its slot out of use, or the record
+		 * would land on the slot's next owner and complete it with
+		 * this descriptor's status. Its unmap data stays with the
+		 * slot as well, since the device may still access what it
+		 * maps, and an address released now could be mapped again
+		 * for someone else before the late access.
+		 */
+		if (quarantine && ctype == IDXD_COMPLETE_ABORT) {
+			struct dmaengine_unmap_data *unmap = tx->unmap;
+
+			tx->unmap = NULL;
+			idxd_dma_complete_txd(desc, ctype, false, NULL, NULL);
+			tx->unmap = unmap;
+			list_add_tail(&desc->list, &qlist);
+			continue;
+		}
 		idxd_dma_complete_txd(desc, ctype, true, NULL, NULL);
 	}
+
+	if (list_empty(&qlist))
+		return;
+	spin_lock(&ie->list_lock);
+	list_splice_tail(&qlist, &wq->quarantine);
+	spin_unlock(&ie->list_lock);
+}
+
+/*
+ * Take what terminate_all has quarantined so far. A drain covers only
+ * what the device held when it was issued, so a synchronize takes the
+ * quarantine before it drains and frees it if the drain completed. A
+ * descriptor terminated meanwhile waits for the next one. The list lock
+ * serialises this with the flush that adds to the quarantine and with a
+ * halt or a teardown that frees it all.
+ */
+void idxd_wq_quarantine_take(struct idxd_wq *wq, struct list_head *list)
+{
+	spin_lock(&wq->ie.list_lock);
+	list_splice_init(&wq->quarantine, list);
+	spin_unlock(&wq->ie.list_lock);
+}
+
+/*
+ * Nothing the device holds can write their completion records, or reach
+ * the memory their unmap data maps, any more. The unmap data goes before
+ * the slot, which a new owner may take at once.
+ */
+void idxd_wq_quarantine_free(struct idxd_wq *wq, struct list_head *list)
+{
+	struct idxd_desc *desc, *itr;
+
+	list_for_each_entry_safe(desc, itr, list, list) {
+		list_del(&desc->list);
+		dma_descriptor_unmap(&desc->txd);
+		idxd_free_desc(wq, desc);
+	}
+}
+
+/*
+ * A halt killed every descriptor the device held: the quarantined slots
+ * are free. 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);
+
+	if (wq->state != IDXD_WQ_ENABLED || wq->type != IDXD_WQT_KERNEL)
+		return;
+
+	idxd_wq_quarantine_take(wq, &dead_descs);
+	idxd_wq_quarantine_free(wq, &dead_descs);
 }
 
 static void idxd_device_set_perm_entry(struct idxd_device *idxd,
@@ -1456,7 +1543,7 @@ void idxd_wq_free_irq(struct idxd_wq *wq)
 		return;
 
 	free_irq(ie->vector, ie);
-	idxd_flush_pending_descs(ie);
+	idxd_flush_pending_descs(ie, false);
 
 	/* FLR revokes every handle; the halt path marks them invalid. */
 	if (ie->int_handle == INVALID_INT_HANDLE)
@@ -1476,16 +1563,38 @@ void idxd_wq_free_irq(struct idxd_wq *wq)
 	ie->pasid = IOMMU_PASID_INVALID;
 }
 
-void idxd_wq_flush_descs(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.
+ */
+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);
 
 	if (wq->state != IDXD_WQ_ENABLED || wq->type != IDXD_WQT_KERNEL)
 		return;
 
-	idxd_flush_pending_descs(ie);
+	/*
+	 * A submitter between its list add and its portal write holds
+	 * wq_active. Waiting for the reference to die puts every listed
+	 * 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;
+	}
+	idxd_flush_pending_descs(ie, quarantine);
+	if (killed) {
+		percpu_ref_reinit(&wq->wq_active);
+		complete_all(&wq->wq_resurrect);
+	}
 }
 
 int idxd_wq_request_irq(struct idxd_wq *wq)
diff --git a/drivers/dma/idxd/dma.c b/drivers/dma/idxd/dma.c
index defd1af74c0a..196045cb9ef2 100644
--- a/drivers/dma/idxd/dma.c
+++ b/drivers/dma/idxd/dma.c
@@ -199,7 +199,8 @@ static int idxd_dma_terminate_all(struct dma_chan *c)
 {
 	struct idxd_wq *wq = to_idxd_wq(c);
 
-	idxd_wq_flush_descs(wq);
+	/* The device keeps running what the lists held. See synchronize. */
+	idxd_wq_flush_descs(wq, true);
 
 	return 0;
 }
@@ -207,8 +208,17 @@ static int idxd_dma_terminate_all(struct dma_chan *c)
 static void idxd_dma_synchronize(struct dma_chan *c)
 {
 	struct idxd_wq *wq = to_idxd_wq(c);
+	LIST_HEAD(quarantined);
 
+	/*
+	 * dmaengine_synchronize() promises the client that its memory may
+	 * be freed afterwards. The drain keeps that promise, and nothing
+	 * the device held before it is left to write a completion record,
+	 * so the slots terminate_all quarantined are free again.
+	 */
+	idxd_wq_quarantine_take(wq, &quarantined);
 	idxd_wq_drain(wq);
+	idxd_wq_quarantine_free(wq, &quarantined);
 }
 
 int idxd_register_dma_device(struct idxd_device *idxd)
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index 221a7302bb0c..31bc4a506225 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -224,6 +224,13 @@ struct idxd_wq {
 	struct idxd_desc **descs;
 	struct sbitmap_queue sbq;
 	struct idxd_dma_chan *idxd_chan;
+	struct list_head quarantine;	/* descriptors a terminate_all took
+					 * off the lists while the device
+					 * still held them. Their slots, with
+					 * their unmap data, are freed once a
+					 * drain that covers them completes or
+					 * the device stops. Under ie.list_lock.
+					 */
 	char name[WQ_NAME_SIZE + 1];
 	u64 max_xfer_bytes;
 	u32 max_batch_size;
@@ -807,7 +814,10 @@ 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);
+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_free(struct idxd_wq *wq, struct list_head *list);
+void idxd_wq_halt_release(struct idxd_wq *wq);
 
 /* submission */
 int idxd_submit_desc(struct idxd_wq *wq, struct idxd_desc *desc);
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index 9c47a7b9e48e..c4319ac27bfb 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -213,6 +213,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
 		init_waitqueue_head(&wq->err_queue);
 		init_completion(&wq->wq_dead);
 		init_completion(&wq->wq_resurrect);
+		INIT_LIST_HEAD(&wq->quarantine);
 		wq->max_xfer_bytes = WQ_DEFAULT_MAX_XFER;
 		idxd_wq_set_max_batch_size(idxd->data->type, wq, WQ_DEFAULT_MAX_BATCH);
 		idxd_wq_set_init_max_sgl_size(idxd, wq);
diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c
index acb901e59a00..6f58d33b7297 100644
--- a/drivers/dma/idxd/irq.c
+++ b/drivers/dma/idxd/irq.c
@@ -427,7 +427,8 @@ static void idxd_wqs_flush_descs(struct idxd_device *idxd)
 	for (i = 0; i < idxd->max_wqs; i++) {
 		struct idxd_wq *wq = idxd->wqs[i];
 
-		idxd_wq_flush_descs(wq);
+		idxd_wq_flush_descs(wq, false);
+		idxd_wq_halt_release(wq);
 	}
 }
 
-- 
2.43.0

[PATCH 10/17] dmaengine: idxd: return -ETIMEDOUT from drains and resets and leak what the device may still write

[Daniel Byrne]

A device command that timed out, or was refused because an earlier one
did, leaves the device enabled with whatever it holds, and a timeout
must never read as a completed drain: the callers free the memory the
device still writes into, or unbind a PASID the device still issues DMA
with.

idxd_wq_drain(), idxd_wq_reset() and idxd_device_drain_pasid() return
-ETIMEDOUT for such a command, and their callers leak what the device
may still reach. The work queue disable keeps the descriptor and
completion memory, and the cdev release keeps the context and its
PASID binding. idxd_wq_disable() reports a failed drain instead of
resetting the queue, so the dedicated user queue's release keeps its
context as the shared queue's does. idxd_wq_reset() leaves the software
state of a queue whose reset timed out enabled, as idxd_wq_disable()
does after a failed reset: the device still runs the queue on its
configuration, and clearing that state let sysfs accept a new one and a
later bind enable the queue on it. idxd_device_reset() reports a refused
reset instead of clearing the software state of an enabled device, and
the device-level remove that could neither disable nor reset the device
keeps the event log.

A command a halted device never ran is not a timeout. A halted device
runs nothing until it is reset, so a drain or reset refused that way
counts as complete, and the cdev release unbinds its PASID normally.

device_synchronize() cannot keep its promise after a drain that failed.
The channel release path runs it under dma_list_mutex, and a device
that refused the drain offers nothing to wait for, so the driver keeps
its own memory safe instead: the quarantined slots stay out of use and
the queue takes no new descriptor until a later drain completes or a
halt kills what the device holds, so a late completion record lands in
a slot nobody reuses.

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/cdev.c   |  35 ++++++++--
 drivers/dma/idxd/device.c | 138 +++++++++++++++++++++++++++++++-------
 drivers/dma/idxd/dma.c    |  31 +++++++--
 drivers/dma/idxd/idxd.h   |  14 ++--
 drivers/dma/idxd/irq.c    |   4 +-
 drivers/dma/idxd/submit.c |   7 ++
 6 files changed, 190 insertions(+), 39 deletions(-)

diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
index 168888adccac..4cfa77c72719 100644
--- a/drivers/dma/idxd/cdev.c
+++ b/drivers/dma/idxd/cdev.c
@@ -120,30 +120,53 @@ static void idxd_file_dev_release(struct device *dev)
 	struct idxd_user_context *ctx = dev_to_uctx(dev);
 	struct idxd_wq *wq = ctx->wq;
 	struct idxd_device *idxd = wq->idxd;
-	int rc;
+	int rc, drained = 0;
 
 	ida_free(&file_ida, ctx->id);
 
 	/* Wait for in-flight operations to complete. */
 	if (wq_shared(wq)) {
-		idxd_device_drain_pasid(idxd, ctx->pasid);
+		drained = idxd_device_drain_pasid(idxd, ctx->pasid);
 	} else {
 		if (device_user_pasid_enabled(idxd)) {
-			/* The wq disable in the disable pasid function will drain the wq */
+			/*
+			 * The wq disable in the disable pasid function drains
+			 * the wq. A drain that never completed leaves the
+			 * PASID in the queue's configuration with the
+			 * descriptors still in it. A failure after the drain,
+			 * re-enabling the queue for instance, does not.
+			 */
 			rc = idxd_wq_disable_pasid(wq);
-			if (rc < 0)
-				dev_err(dev, "wq disable pasid failed.\n");
+			if (rc < 0) {
+				dev_err(dev, "wq disable pasid failed: %d\n", rc);
+				if (rc == -ETIMEDOUT)
+					drained = rc;
+			}
 		} else {
-			idxd_wq_drain(wq);
+			drained = idxd_wq_drain(wq);
 		}
 	}
 
+	/*
+	 * A drain that never completed leaves this context's descriptors
+	 * in the device, tagged with its PASID. Unbinding the PASID would
+	 * let it be reused by another process while the device can still
+	 * issue DMA with it, so the binding, the PASID lookup entry and the
+	 * context are leaked instead.
+	 */
+	if (drained) {
+		dev_err(dev, "pasid %u: drain timed out, leaking the context and its PASID binding\n",
+			ctx->pasid);
+		goto put_wq;
+	}
+
 	if (ctx->sva) {
 		idxd_cdev_evl_drain_pasid(wq, ctx->pasid);
 		iommu_sva_unbind_device(ctx->sva);
 		idxd_xa_pasid_remove(ctx);
 	}
 	kfree(ctx);
+put_wq:
 	mutex_lock(&wq->wq_lock);
 	idxd_wq_put(wq);
 	mutex_unlock(&wq->wq_lock);
diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index 09475f5ea026..d3b95593a686 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -19,6 +19,7 @@ static void idxd_cmd_exec(struct idxd_device *idxd, int cmd_code, u32 operand,
 			  u32 *status);
 static void idxd_device_wqs_clear_state(struct idxd_device *idxd);
 static void idxd_wq_disable_cleanup(struct idxd_wq *wq);
+static inline bool idxd_device_is_halted(struct idxd_device *idxd);
 static int idxd_wq_config_write(struct idxd_wq *wq);
 
 /* Interrupt control bits */
@@ -119,6 +120,7 @@ int idxd_wq_alloc_resources(struct idxd_wq *wq)
 		return 0;
 
 	/* A fresh pool: nothing outstanding can land in it. */
+	wq->stuck = false;
 	INIT_LIST_HEAD(&wq->quarantine);
 	num_descs = wq_dedicated(wq) ? wq->size : wq->threshold;
 	wq->num_descs = num_descs;
@@ -222,6 +224,7 @@ int idxd_wq_disable(struct idxd_wq *wq, bool reset_config)
 	struct idxd_device *idxd = wq->idxd;
 	struct device *dev = &idxd->pdev->dev;
 	u32 status, operand;
+	int rc;
 
 	dev_dbg(dev, "Disabling WQ %d\n", wq->id);
 
@@ -239,8 +242,14 @@ int idxd_wq_disable(struct idxd_wq *wq, bool reset_config)
 	 * To make sure pending translations are drained before WQ
 	 * attributes are changed, we use a WQ Drain followed by WQ Reset and
 	 * then restore the WQ configuration.
+	 *
+	 * A drain that timed out leaves descriptors in the queue. Report
+	 * it instead of resetting, so that the caller keeps whatever those
+	 * descriptors can still reach, a PASID for instance.
 	 */
-	idxd_wq_drain(wq);
+	rc = idxd_wq_drain(wq);
+	if (rc)
+		return rc;
 
 	operand = BIT(wq->id % 16) | ((wq->id / 16) << 16);
 	idxd_cmd_exec(idxd, IDXD_CMD_RESET_WQ, operand, &status);
@@ -260,36 +269,72 @@ int idxd_wq_disable(struct idxd_wq *wq, bool reset_config)
 	return 0;
 }
 
-void idxd_wq_drain(struct idxd_wq *wq)
+/*
+ * The result of a drain or a reset. IDXD_CMDSTS_HW_ERR stands for a
+ * command that timed out, or was refused after one did, and then the
+ * device may still run what it holds. It is also the device's own code
+ * for a hardware error, after which the specification has software read
+ * GENSTS. Both leave the device enabled unless it halted, and a halted
+ * device runs nothing until it is reset, so what it holds is then as
+ * gone as after a drain and that is not a timeout.
+ */
+static int idxd_cmd_drain_result(struct idxd_device *idxd, u32 status)
+{
+	if (status != IDXD_CMDSTS_HW_ERR || idxd_device_is_halted(idxd))
+		return 0;
+	return -ETIMEDOUT;
+}
+
+/*
+ * Returns -ETIMEDOUT when the command never completed, or was refused
+ * because an earlier one never did, in which case descriptors may still
+ * be in the device and the caller must not free anything it can write
+ * into.
+ */
+int idxd_wq_drain(struct idxd_wq *wq)
 {
 	struct idxd_device *idxd = wq->idxd;
 	struct device *dev = &idxd->pdev->dev;
-	u32 operand;
+	u32 operand, status;
 
 	if (wq->state != IDXD_WQ_ENABLED) {
 		dev_dbg(dev, "WQ %d in wrong state: %d\n", wq->id, wq->state);
-		return;
+		return 0;
 	}
 
 	dev_dbg(dev, "Draining WQ %d\n", wq->id);
 	operand = BIT(wq->id % 16) | ((wq->id / 16) << 16);
-	idxd_cmd_exec(idxd, IDXD_CMD_DRAIN_WQ, operand, NULL);
+	idxd_cmd_exec(idxd, IDXD_CMD_DRAIN_WQ, operand, &status);
+	return idxd_cmd_drain_result(idxd, status);
 }
 
-void idxd_wq_reset(struct idxd_wq *wq)
+int idxd_wq_reset(struct idxd_wq *wq)
 {
 	struct idxd_device *idxd = wq->idxd;
 	struct device *dev = &idxd->pdev->dev;
-	u32 operand;
+	u32 operand, status;
+	int rc;
 
 	if (wq->state != IDXD_WQ_ENABLED) {
 		dev_dbg(dev, "WQ %d in wrong state: %d\n", wq->id, wq->state);
-		return;
+		return 0;
 	}
 
 	operand = BIT(wq->id % 16) | ((wq->id / 16) << 16);
-	idxd_cmd_exec(idxd, IDXD_CMD_RESET_WQ, operand, NULL);
+	idxd_cmd_exec(idxd, IDXD_CMD_RESET_WQ, operand, &status);
+	/*
+	 * A reset that timed out, or was refused after a command did,
+	 * leaves the queue enabled in the device on its configuration.
+	 * Its software state stays enabled too, as after a failed
+	 * idxd_wq_disable(), so that the queue is neither reconfigured nor
+	 * enabled again on a configuration the device does not run until
+	 * a device disable or reset has stopped it.
+	 */
+	rc = idxd_cmd_drain_result(idxd, status);
+	if (rc)
+		return rc;
 	idxd_wq_disable_cleanup(wq);
+	return 0;
 }
 
 int idxd_wq_map_portal(struct idxd_wq *wq)
@@ -732,24 +777,41 @@ int idxd_device_disable(struct idxd_device *idxd)
 	return 0;
 }
 
-void idxd_device_reset(struct idxd_device *idxd)
+/*
+ * Returns -ENXIO when the device refused or never ran the reset. The
+ * software state is cleared only after a reset that ran, since clearing
+ * it for an enabled device lets a later teardown free memory the device
+ * still writes.
+ */
+int idxd_device_reset(struct idxd_device *idxd)
 {
-	idxd_cmd_exec(idxd, IDXD_CMD_RESET_DEVICE, 0, NULL);
+	u32 status;
+
+	idxd_cmd_exec(idxd, IDXD_CMD_RESET_DEVICE, 0, &status);
+	if (status != IDXD_CMDSTS_SUCCESS) {
+		dev_dbg(&idxd->pdev->dev, "%s: err_code: %#x\n", __func__, status);
+		return -ENXIO;
+	}
 	idxd_device_clear_state(idxd);
 	spin_lock(&idxd->dev_lock);
 	idxd_unmask_error_interrupts(idxd);
 	spin_unlock(&idxd->dev_lock);
+	return 0;
 }
 
-void idxd_device_drain_pasid(struct idxd_device *idxd, int pasid)
+/* Returns -ETIMEDOUT when the drain was never run or never completed. */
+int idxd_device_drain_pasid(struct idxd_device *idxd, int pasid)
 {
 	struct device *dev = &idxd->pdev->dev;
-	u32 operand;
+	u32 operand, status;
 
 	operand = pasid;
 	dev_dbg(dev, "cmd: %u operand: %#x\n", IDXD_CMD_DRAIN_PASID, operand);
-	idxd_cmd_exec(idxd, IDXD_CMD_DRAIN_PASID, operand, NULL);
+	idxd_cmd_exec(idxd, IDXD_CMD_DRAIN_PASID, operand, &status);
+	if (idxd_cmd_drain_result(idxd, status))
+		return -ETIMEDOUT;
 	dev_dbg(dev, "pasid %d drained\n", pasid);
+	return 0;
 }
 
 int idxd_device_request_int_handle(struct idxd_device *idxd, int idx, int *handle,
@@ -1480,6 +1542,14 @@ void idxd_wq_quarantine_take(struct idxd_wq *wq, struct list_head *list)
 	spin_unlock(&wq->ie.list_lock);
 }
 
+/* The drain did not complete: the device may still run them. */
+void idxd_wq_quarantine_put_back(struct idxd_wq *wq, struct list_head *list)
+{
+	spin_lock(&wq->ie.list_lock);
+	list_splice(list, &wq->quarantine);
+	spin_unlock(&wq->ie.list_lock);
+}
+
 /*
  * Nothing the device holds can write their completion records, or reach
  * the memory their unmap data maps, any more. The unmap data goes before
@@ -1498,8 +1568,8 @@ 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. 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. Under
+ * wq_lock, since an unbind racing the halt may free the pool.
  */
 void idxd_wq_halt_release(struct idxd_wq *wq)
 {
@@ -1512,6 +1582,7 @@ void idxd_wq_halt_release(struct idxd_wq *wq)
 
 	idxd_wq_quarantine_take(wq, &dead_descs);
 	idxd_wq_quarantine_free(wq, &dead_descs);
+	WRITE_ONCE(wq->stuck, false);
 }
 
 static void idxd_device_set_perm_entry(struct idxd_device *idxd,
@@ -1783,6 +1854,7 @@ void idxd_drv_disable_wq(struct idxd_wq *wq)
 {
 	struct idxd_device *idxd = wq->idxd;
 	struct device *dev = &idxd->pdev->dev;
+	int rc;
 
 	lockdep_assert_held(&wq->wq_lock);
 
@@ -1791,10 +1863,19 @@ void idxd_drv_disable_wq(struct idxd_wq *wq)
 			 wq->id, idxd_wq_refcount(wq));
 
 	idxd_wq_unmap_portal(wq);
-	idxd_wq_drain(wq);
+	rc = idxd_wq_drain(wq);
 	idxd_wq_free_irq(wq);
-	idxd_wq_reset(wq);
-	idxd_wq_free_resources(wq);
+	rc |= idxd_wq_reset(wq);
+	/*
+	 * A drain or reset that never completed leaves descriptors in the
+	 * device, and their completion records live in the memory freed
+	 * here. Leak it; the device writing into freed memory is worse.
+	 */
+	if (rc)
+		dev_err(dev, "wq %d: drain or reset timed out, leaking its descriptor memory\n",
+			wq->id);
+	else
+		idxd_wq_free_resources(wq);
 	percpu_ref_exit(&wq->wq_active);
 	wq->client_count = 0;
 }
@@ -1862,6 +1943,7 @@ void idxd_device_drv_remove(struct idxd_dev *idxd_dev)
 {
 	struct device *dev = &idxd_dev->conf_dev;
 	struct idxd_device *idxd = idxd_dev_to_idxd(idxd_dev);
+	bool stopped;
 	int i;
 
 	for (i = 0; i < idxd->max_wqs; i++) {
@@ -1882,10 +1964,20 @@ void idxd_device_drv_remove(struct idxd_dev *idxd_dev)
 	}
 
 	idxd_unregister_dma_device(idxd);
-	idxd_device_disable(idxd);
-	if (test_bit(IDXD_FLAG_CONFIGURABLE, &idxd->flags))
-		idxd_device_reset(idxd);
-	idxd_device_evl_free(idxd);
+	/*
+	 * A disable or a reset that ran, or found the device disabled or
+	 * halted, leaves nothing in flight. Both are refused after a device
+	 * command timed out, and the device then stays enabled with whatever
+	 * it holds, so what it may still write is kept.
+	 */
+	stopped = !idxd_device_disable(idxd);
+	if (test_bit(IDXD_FLAG_CONFIGURABLE, &idxd->flags) &&
+	    !idxd_device_reset(idxd))
+		stopped = true;
+	if (stopped)
+		idxd_device_evl_free(idxd);
+	else
+		dev_err(dev, "device still enabled after a command timeout, leaking the event log\n");
 }
 
 static enum idxd_dev_type dev_types[] = {
diff --git a/drivers/dma/idxd/dma.c b/drivers/dma/idxd/dma.c
index 196045cb9ef2..1d08856f5c4e 100644
--- a/drivers/dma/idxd/dma.c
+++ b/drivers/dma/idxd/dma.c
@@ -212,13 +212,34 @@ static void idxd_dma_synchronize(struct dma_chan *c)
 
 	/*
 	 * dmaengine_synchronize() promises the client that its memory may
-	 * be freed afterwards. The drain keeps that promise, and nothing
-	 * the device held before it is left to write a completion record,
-	 * so the slots terminate_all quarantined are free again.
+	 * be freed afterwards. A drain that completed keeps that promise,
+	 * and nothing the device held before it is left to write a
+	 * completion record, so the slots terminate_all quarantined are
+	 * free again. A drain that never completed, or that was refused
+	 * because an earlier command never did, leaves the client's
+	 * descriptors in a device that may still run them. The channel
+	 * release path runs this under dma_list_mutex, and a device that
+	 * refused the drain offers nothing to wait for, so waiting longer
+	 * here would hold up every dmaengine user for nothing. The driver
+	 * cannot keep the promise for the client's memory then and says so.
+	 * It keeps its own memory safe: the quarantined slots stay out of
+	 * use and the queue takes no new descriptor until a later drain
+	 * completes or a halt kills what the device holds, so a late
+	 * completion record lands in a slot nobody reuses, and
+	 * idxd_drv_disable_wq() leaks what the device may still write.
+	 * Only a device reset ends the exposure of the client's memory.
 	 */
 	idxd_wq_quarantine_take(wq, &quarantined);
-	idxd_wq_drain(wq);
-	idxd_wq_quarantine_free(wq, &quarantined);
+	if (!idxd_wq_drain(wq)) {
+		idxd_wq_quarantine_free(wq, &quarantined);
+		WRITE_ONCE(wq->stuck, false);
+		return;
+	}
+	idxd_wq_quarantine_put_back(wq, &quarantined);
+	WRITE_ONCE(wq->stuck, true);
+	dev_err_ratelimited(&wq->idxd->pdev->dev,
+			    "wq %d: drain failed. Its descriptors may still complete and it takes no new work until a drain completes or the device is reset\n",
+			    wq->id);
 }
 
 int idxd_register_dma_device(struct idxd_device *idxd)
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index 31bc4a506225..c04a2855aa3f 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -224,6 +224,11 @@ struct idxd_wq {
 	struct idxd_desc **descs;
 	struct sbitmap_queue sbq;
 	struct idxd_dma_chan *idxd_chan;
+	bool stuck;		/* a drain failed with the device enabled: the
+				 * outstanding descriptors may complete at any
+				 * time, so no slot is reused until a later
+				 * drain completes or a halt kills them
+				 */
 	struct list_head quarantine;	/* descriptors a terminate_all took
 					 * off the lists while the device
 					 * still held them. Their slots, with
@@ -786,11 +791,11 @@ void idxd_drv_disable_wq(struct idxd_wq *wq);
 int idxd_device_init_reset(struct idxd_device *idxd);
 int idxd_device_enable(struct idxd_device *idxd);
 int idxd_device_disable(struct idxd_device *idxd);
-void idxd_device_reset(struct idxd_device *idxd);
+int idxd_device_reset(struct idxd_device *idxd);
 void idxd_device_clear_state(struct idxd_device *idxd);
 void idxd_cmd_reset_state(struct idxd_device *idxd);
 int idxd_device_config(struct idxd_device *idxd);
-void idxd_device_drain_pasid(struct idxd_device *idxd, int pasid);
+int idxd_device_drain_pasid(struct idxd_device *idxd, int pasid);
 int idxd_device_load_config(struct idxd_device *idxd);
 int idxd_device_request_int_handle(struct idxd_device *idxd, int idx, int *handle,
 				   enum idxd_interrupt_type irq_type);
@@ -803,8 +808,8 @@ int idxd_wq_alloc_resources(struct idxd_wq *wq);
 void idxd_wq_free_resources(struct idxd_wq *wq);
 int idxd_wq_enable(struct idxd_wq *wq);
 int idxd_wq_disable(struct idxd_wq *wq, bool reset_config);
-void idxd_wq_drain(struct idxd_wq *wq);
-void idxd_wq_reset(struct idxd_wq *wq);
+int idxd_wq_drain(struct idxd_wq *wq);
+int idxd_wq_reset(struct idxd_wq *wq);
 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);
@@ -816,6 +821,7 @@ 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_quarantine_take(struct idxd_wq *wq, struct list_head *list);
+void idxd_wq_quarantine_put_back(struct idxd_wq *wq, struct list_head *list);
 void idxd_wq_quarantine_free(struct idxd_wq *wq, struct list_head *list);
 void idxd_wq_halt_release(struct idxd_wq *wq);
 
diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c
index 6f58d33b7297..e48f7c307ceb 100644
--- a/drivers/dma/idxd/irq.c
+++ b/drivers/dma/idxd/irq.c
@@ -40,7 +40,9 @@ static void idxd_device_reinit(struct work_struct *work)
 	 * timed out before the halt would otherwise refuse the reset.
 	 */
 	idxd_cmd_reset_state(idxd);
-	idxd_device_reset(idxd);
+	rc = idxd_device_reset(idxd);
+	if (rc < 0)
+		goto out;
 	rc = idxd_device_config(idxd);
 	if (rc < 0)
 		goto out;
diff --git a/drivers/dma/idxd/submit.c b/drivers/dma/idxd/submit.c
index 03217041b8b3..26dce0179d30 100644
--- a/drivers/dma/idxd/submit.c
+++ b/drivers/dma/idxd/submit.c
@@ -34,6 +34,13 @@ struct idxd_desc *idxd_alloc_desc(struct idxd_wq *wq, enum idxd_op_type optype)
 
 	if (idxd->state != IDXD_DEV_ENABLED)
 		return ERR_PTR(-EIO);
+	/*
+	 * A failed drain left descriptors the device may still complete.
+	 * No slot is reused until a later drain completes or a halt kills
+	 * them.
+	 */
+	if (READ_ONCE(wq->stuck))
+		return ERR_PTR(-ENXIO);
 
 	sbq = &wq->sbq;
 	idx = sbitmap_queue_get(sbq, &cpu);
-- 
2.43.0

[PATCH 11/17] dmaengine: idxd: disable the device before its work queues are unregistered

[Daniel Byrne]

idxd_remove() unregisters the work queues, and with them their
software state, before idxd_shutdown() disables the device. The
sub-driver's remove leaves the device enabled when a device command has
timed out, and a late completion can clear that fail-fast state at any
time, so the disable in idxd_shutdown() may be the one that runs. It
then clears the software state of queues that no longer exist.

Disable the device right after the sub-driver is unbound, while the
queues still exist. Mark the device disabled once bus mastering is off,
so that nothing reached through the device reads it as enabled, and do
so before the reference taken for the cleanup is dropped: that
reference is the last one, and its release frees the device.

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/init.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index c4319ac27bfb..a037a6e76362 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -1296,6 +1296,13 @@ static void idxd_remove(struct pci_dev *pdev)
 	 * that driver while the queues still exist.
 	 */
 	device_release_driver(idxd_confdev(idxd));
+	/*
+	 * Disable the device while the queues still exist too. The
+	 * sub-driver's remove leaves the device enabled when a command has
+	 * timed out, a late completion can clear that state at any time,
+	 * and the disable below then clears the queues' software state.
+	 */
+	idxd_shutdown(pdev);
 	idxd_unregister_devices(idxd);
 	/*
 	 * When ->release() is called for the idxd->conf_dev, it frees all
@@ -1305,15 +1312,20 @@ static void idxd_remove(struct pci_dev *pdev)
 	 */
 	get_device(idxd_confdev(idxd));
 	device_unregister(idxd_confdev(idxd));
-	idxd_shutdown(pdev);
 	idxd_device_remove_debugfs(idxd);
 	perfmon_pmu_remove(idxd);
 	idxd_cleanup_interrupts(idxd);
 	if (device_pasid_enabled(idxd))
 		idxd_disable_system_pasid(idxd);
 	pci_iounmap(pdev, idxd->reg_base);
-	put_device(idxd_confdev(idxd));
 	pci_disable_device(pdev);
+	/*
+	 * Bus mastering is off, so nothing the device holds can complete.
+	 * The store comes before the reference taken above is dropped:
+	 * that reference is the last one, and its release frees the device.
+	 */
+	idxd->state = IDXD_DEV_DISABLED;
+	put_device(idxd_confdev(idxd));
 }
 
 static struct pci_driver idxd_pci_driver = {
-- 
2.43.0

[PATCH 12/17] dmaengine: idxd: hold device commands off while the reset re-creates its interrupts

[Daniel Byrne]

The re-probe after a reset frees the misc interrupt and the MSI-X
vectors and requests them again. A device command whose completion
interrupt arrives in between is never completed, so its owner times
out and fences the device that was just reset, and the rebind in
idxd_reset_done() then fails.

Wait for the running command before the vectors are freed, since its
interrupt still arrives, and then hold IDXD_FLAG_CMD_RUNNING without an
owner until the interrupts are back, so that commands issued meanwhile
wait as they wait for any running command. A halt's recovery may forget
the hold, so it is released only if no newer command owns the state.

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 | 44 +++++++++++++++++++++++++++++++++++++++
 drivers/dma/idxd/idxd.h   |  2 ++
 drivers/dma/idxd/init.c   | 10 +++++++++
 3 files changed, 56 insertions(+)

diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index d3b95593a686..222c201669bd 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -724,6 +724,50 @@ void idxd_cmd_reset_state(struct idxd_device *idxd)
 	spin_unlock_irqrestore(&idxd->cmd_lock, flags);
 }
 
+/*
+ * Hold device commands off while the re-probe after a reset frees the
+ * interrupts and requests them again. A command whose completion
+ * interrupt comes while the misc vector is gone is never completed, and
+ * its owner would time out and fence the device that was just reset.
+ * The running command, if any, is waited for first: its owner gives up
+ * within its timeout, and the misc vector still takes its interrupt.
+ * RUNNING is then held without an owner, and new commands wait for it
+ * as for any running command. Returns false, holding nothing, when a
+ * command timed out, since commands then fail fast anyway.
+ */
+bool idxd_cmd_hold(struct idxd_device *idxd, u32 *seq)
+{
+	bool held = false;
+
+	spin_lock_irq(&idxd->cmd_lock);
+	wait_event_lock_irq(idxd->cmd_waitq,
+			    !test_bit(IDXD_FLAG_CMD_RUNNING, &idxd->flags) ||
+			    test_bit(IDXD_FLAG_CMD_TIMEDOUT, &idxd->flags),
+			    idxd->cmd_lock);
+	if (!test_bit(IDXD_FLAG_CMD_RUNNING, &idxd->flags)) {
+		__set_bit(IDXD_FLAG_CMD_RUNNING, &idxd->flags);
+		*seq = ++idxd->cmd_seq;
+		held = true;
+	}
+	spin_unlock_irq(&idxd->cmd_lock);
+	return held;
+}
+
+/*
+ * End a hold taken by idxd_cmd_hold(). A halt's recovery may have
+ * forgotten the hold meanwhile and let another command run, which then
+ * has an owner or a newer sequence number, and its state is left alone.
+ */
+void idxd_cmd_unhold(struct idxd_device *idxd, u32 seq)
+{
+	spin_lock_irq(&idxd->cmd_lock);
+	if (idxd->cmd_seq == seq && !idxd->cmd_done) {
+		__clear_bit(IDXD_FLAG_CMD_RUNNING, &idxd->flags);
+		wake_up(&idxd->cmd_waitq);
+	}
+	spin_unlock_irq(&idxd->cmd_lock);
+}
+
 int idxd_device_enable(struct idxd_device *idxd)
 {
 	struct device *dev = &idxd->pdev->dev;
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index c04a2855aa3f..9d88b0d82fee 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -794,6 +794,8 @@ int idxd_device_disable(struct idxd_device *idxd);
 int idxd_device_reset(struct idxd_device *idxd);
 void idxd_device_clear_state(struct idxd_device *idxd);
 void idxd_cmd_reset_state(struct idxd_device *idxd);
+bool idxd_cmd_hold(struct idxd_device *idxd, u32 *seq);
+void idxd_cmd_unhold(struct idxd_device *idxd, u32 seq);
 int idxd_device_config(struct idxd_device *idxd);
 int idxd_device_drain_pasid(struct idxd_device *idxd, int pasid);
 int idxd_device_load_config(struct idxd_device *idxd);
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index a037a6e76362..312b4670b3e8 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -1212,6 +1212,14 @@ int idxd_pci_probe_alloc(struct idxd_device *idxd, struct pci_dev *pdev,
 	}
 
 	if (!alloc_idxd) {
+		bool held;
+		u32 seq;
+
+		/*
+		 * Commands complete through the misc interrupt, which is
+		 * gone until it is requested again.
+		 */
+		held = idxd_cmd_hold(idxd, &seq);
 		/* Release interrupts in the IDXD device. */
 		idxd_cleanup_interrupts(idxd);
 
@@ -1219,6 +1227,8 @@ int idxd_pci_probe_alloc(struct idxd_device *idxd, struct pci_dev *pdev,
 		rc = idxd_setup_interrupts(idxd);
 		if (rc)
 			dev_warn(dev, "IDXD interrupts failed to setup\n");
+		if (held)
+			idxd_cmd_unhold(idxd, seq);
 	}
 
 	dev_info(&pdev->dev, "Intel(R) Accelerator Device (v%x)\n",
-- 
2.43.0

[PATCH 13/17] dmaengine: idxd: fix the double free of a descriptor whose submission failed

[Daniel Byrne]

llist_abort_desc() marks the failed descriptor aborted before it
searches the lists for it. When the interrupt thread has just taken the
pending list into a private batch, the search misses it, the thread
completes and frees the descriptor on seeing the mark, and the caller
frees it again: sbitmap_queue_clear() then stores through the per-cpu
hint of cpu -1, or clears the bit of the slot's next owner, and a thread
that read the record before the mark puts the freed descriptor on the
work list. This needs only interrupt-armed descriptors in flight on a
shared queue whose ENQCMDS fails, as in a guest whose shared queue other
guests keep full.

Sort the pending list under the list lock in the thread, as the abort
does, so a descriptor still waiting on the hardware is always on one of
the lists the abort searches. The device never took a failed
descriptor, so its record stays clear and nothing completes it before
the abort has taken it off the lists; from then on it is the
submitter's alone and needs no mark. The finished descriptors the abort
takes off the pending list go to the work list for the interrupt thread,
which the abort wakes, instead of being completed in the submitter's
context.

Fixes: 6b4b87f2c31a ("dmaengine: idxd: fix submission race window")
Cc: stable@vger.kernel.org # 5.14+
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/irq.c    | 63 ++++++++++++++++++++++-----------------
 drivers/dma/idxd/submit.c | 48 +++++++++++++++--------------
 2 files changed, 62 insertions(+), 49 deletions(-)

diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c
index e48f7c307ceb..1ec3d360eea8 100644
--- a/drivers/dma/idxd/irq.c
+++ b/drivers/dma/idxd/irq.c
@@ -651,31 +651,39 @@ static void irq_process_pending_llist(struct idxd_irq_entry *irq_entry)
 {
 	struct idxd_desc *desc, *t;
 	struct llist_node *head;
+	LIST_HEAD(flist);
 
-	head = llist_del_all(&irq_entry->pending_llist);
-	if (!head)
+	if (llist_empty(&irq_entry->pending_llist))
 		return;
 
+	/*
+	 * Sort under the list lock, as the submit abort does. A descriptor
+	 * still waiting on the hardware reaches the work list within the
+	 * same hold, so the abort of a failed submission, which searches
+	 * both lists under the lock, always finds its own descriptor.
+	 */
+	spin_lock(&irq_entry->list_lock);
+	head = llist_del_all(&irq_entry->pending_llist);
 	llist_for_each_entry_safe(desc, t, head, llnode) {
 		u8 status = desc->completion->status & DSA_COMP_STATUS_MASK;
 
-		if (status) {
-			/*
-			 * Check against the original status as ABORT is software defined
-			 * and 0xff, which DSA_COMP_STATUS_MASK can mask out.
-			 */
-			if (unlikely(desc->completion->status == IDXD_COMP_DESC_ABORT)) {
-				idxd_desc_complete(desc, IDXD_COMPLETE_ABORT, true);
-				continue;
-			}
+		if (status)
+			list_add_tail(&desc->list, &flist);
+		else
+			list_add_tail(&desc->list, &irq_entry->work_list);
+	}
+	spin_unlock(&irq_entry->list_lock);
 
+	list_for_each_entry_safe(desc, t, &flist, list) {
+		list_del(&desc->list);
+		/*
+		 * Check against the original status as ABORT is software
+		 * defined and 0xff, which DSA_COMP_STATUS_MASK can mask out.
+		 */
+		if (unlikely(desc->completion->status == IDXD_COMP_DESC_ABORT))
+			idxd_desc_complete(desc, IDXD_COMPLETE_ABORT, true);
+		else
 			idxd_desc_complete(desc, IDXD_COMPLETE_NORMAL, true);
-		} else {
-			spin_lock(&irq_entry->list_lock);
-			list_add_tail(&desc->list,
-				      &irq_entry->work_list);
-			spin_unlock(&irq_entry->list_lock);
-		}
 	}
 }
 
@@ -727,18 +735,19 @@ irqreturn_t idxd_wq_thread(int irq, void *data)
 	 * submmiter adds all the submitted descriptor after sending it to
 	 * the workqueue. It's a lockless singly linked list. The work_list
 	 * is the common linux double linked list. We are in a scenario of
-	 * multiple producers and a single consumer. The producers are all
-	 * the kernel submitters of descriptors, and the consumer is the
-	 * kernel irq handler thread for the msix vector when using threaded
-	 * irq. To work with the restrictions of llist to remain lockless,
-	 * we are doing the following steps:
+	 * multiple producers and several consumers. The producers are all
+	 * the kernel submitters of descriptors, and they add to the llist
+	 * without a lock. The consumers are the kernel irq handler thread
+	 * for the msix vector when using threaded irq, the abort of a
+	 * failed submission, the interrupt handle abort and the flushes,
+	 * and each empties the llist under the list lock. The thread does
+	 * so in these steps:
 	 * 1. Iterate through the work_list and process any completed
 	 *    descriptor. Delete the completed entries during iteration.
-	 * 2. llist_del_all() from the pending list.
-	 * 3. Iterate through the llist that was deleted from the pending list
-	 *    and process the completed entries.
-	 * 4. If the entry is still waiting on hardware, list_add_tail() to
-	 *    the work_list.
+	 * 2. Under the list lock, llist_del_all() from the pending list and
+	 *    move each entry still waiting on hardware to the work_list and
+	 *    each completed one to a private list.
+	 * 3. Process the entries of the private list outside the lock.
 	 */
 	irq_process_work_list(irq_entry);
 	irq_process_pending_llist(irq_entry);
diff --git a/drivers/dma/idxd/submit.c b/drivers/dma/idxd/submit.c
index 26dce0179d30..5806b1b74a2f 100644
--- a/drivers/dma/idxd/submit.c
+++ b/drivers/dma/idxd/submit.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright(c) 2019 Intel Corporation. All rights rsvd. */
 #include <linux/init.h>
+#include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
@@ -92,12 +93,6 @@ static struct idxd_desc *list_abort_desc(struct idxd_wq *wq, struct idxd_irq_ent
 		}
 	}
 
-	/*
-	 * At this point, the desc needs to be aborted is held by the completion
-	 * handler where it has taken it off the pending list but has not added to the
-	 * work list. It will be cleaned up by the interrupt handler when it sees the
-	 * IDXD_COMP_DESC_ABORT for completion status.
-	 */
 	return NULL;
 }
 
@@ -106,9 +101,8 @@ static void llist_abort_desc(struct idxd_wq *wq, struct idxd_irq_entry *ie,
 {
 	struct idxd_desc *d, *t, *found = NULL;
 	struct llist_node *head;
-	LIST_HEAD(flist);
+	bool wake = false;
 
-	desc->completion->status = IDXD_COMP_DESC_ABORT;
 	/*
 	 * Grab the list lock so it will block the irq thread handler. This allows the
 	 * abort code to locate the descriptor need to be aborted.
@@ -122,10 +116,15 @@ static void llist_abort_desc(struct idxd_wq *wq, struct idxd_irq_entry *ie,
 				continue;
 			}
 
+			/*
+			 * 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.
+			 */
 			if (d->completion->status)
-				list_add_tail(&d->list, &flist);
-			else
-				list_add_tail(&d->list, &ie->work_list);
+				wake = true;
+			list_add_tail(&d->list, &ie->work_list);
 		}
 	}
 
@@ -133,21 +132,26 @@ static void llist_abort_desc(struct idxd_wq *wq, struct idxd_irq_entry *ie,
 		found = list_abort_desc(wq, ie, desc);
 	spin_unlock(&ie->list_lock);
 
-	if (found)
-		idxd_dma_complete_txd(found, IDXD_COMPLETE_ABORT, false,
-				      NULL, NULL);
+	/*
+	 * Their interrupts may have run the thread before they reached the
+	 * work list, so it is woken again.
+	 */
+	if (wake)
+		irq_wake_thread(ie->vector, ie);
 
 	/*
-	 * completing the descriptor will return desc to allocator and
-	 * the desc can be acquired by a different process and the
-	 * desc->list can be modified.  Delete desc from list so the
-	 * list traversing does not get corrupted by the other process.
+	 * 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
+	 * that would let another context that saw the mark on a list
+	 * complete and free it, and the caller frees it as well.
 	 */
-	list_for_each_entry_safe(d, t, &flist, list) {
-		list_del_init(&d->list);
-		idxd_dma_complete_txd(d, IDXD_COMPLETE_ABORT, true,
+	WARN_ON_ONCE(!found);
+	if (found)
+		idxd_dma_complete_txd(desc, IDXD_COMPLETE_ABORT, false,
 				      NULL, NULL);
-	}
 }
 
 /*
-- 
2.43.0

[PATCH 14/17] dmaengine: idxd: complete an iaa_crypto descriptor through its own driver on abort and flush

[Daniel Byrne]

The teardown flush and the submit abort complete every descriptor as a
dmaengine transaction, whichever driver owns the queue. An iaa_crypto
queue is a kernel queue as well, and its interrupt-armed descriptors
carry the crypto request where a dmaengine descriptor has its txd, so
completing one of them as a transaction takes the request pointer for
a cookie and a DMA address for the channel, and either hits the cookie
BUG_ON() or stores through that address. A single iaa_crypto client
reaches this without a halt or a shared channel: in the teardown flush
after a drain that failed or after free_irq() lost a completion
interrupt, and in a guest on every failed interrupt-armed submission to
a shared queue that other guests keep full.

Complete such a descriptor through the completion of the driver that
owns the queue, as the interrupt thread does, and mark one the device
never finished as aborted first so that the driver fails its request.
The txd handling, the callback strip and the quarantine stay with
dmaengine queues. A failed submission of another driver's descriptor is
left entirely to its submitter, which learns of it from the return
value.

The teardown flush frees each descriptor it completes only when that
completion returns, and iaa_crypto's verify allocates from inside one.
A blocking allocation therefore fails at once on a queue whose
wq_active is dead with no revive to come, rather than wait for a slot
only the flush could free. Every submission there fails anyway.

Fixes: 09646c98d0bf ("crypto: iaa - Add irq support for the crypto async interface")
Cc: stable@vger.kernel.org # 6.8+
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 | 23 +++++++++++++++++++----
 drivers/dma/idxd/idxd.h   | 10 ++++++++++
 drivers/dma/idxd/submit.c | 22 +++++++++++++++++++++-
 3 files changed, 50 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index 222c201669bd..a33c4309f5d4 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -1534,10 +1534,25 @@ static void idxd_flush_pending_descs(struct idxd_irq_entry *ie,
 		list_del(&desc->list);
 		ctype = desc->completion->status ? IDXD_COMPLETE_NORMAL : IDXD_COMPLETE_ABORT;
 		/*
-		 * wq is being disabled. Any remaining descriptors are
-		 * likely to be stuck and can be dropped. callback could
-		 * point to code that is no longer accessible, for example
-		 * if dmatest module has been unloaded.
+		 * Another driver's descriptor carries that driver's request
+		 * in place of a txd, and a client may still wait for it, so it
+		 * goes back through that driver's completion. The completion
+		 * reads the outcome from the record, which the device never
+		 * wrote for an unfinished descriptor. Only a dmaengine channel
+		 * terminates, so nothing of such a queue is quarantined.
+		 */
+		if (!idxd_wq_has_txd(wq)) {
+			if (ctype == IDXD_COMPLETE_ABORT)
+				desc->completion->status = IDXD_COMP_DESC_ABORT;
+			idxd_desc_complete(desc, ctype, true);
+			continue;
+		}
+		/*
+		 * wq is being disabled or terminated. Any remaining
+		 * descriptors are likely to be stuck and can be dropped.
+		 * With no client left the callback could point to code that
+		 * is no longer accessible (dmatest unloaded), and a client's
+		 * own terminate must not get it, so it is stripped.
 		 */
 		tx = &desc->txd;
 		tx->callback = NULL;
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index 9d88b0d82fee..3caf9fc00f83 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -755,6 +755,16 @@ void idxd_dma_complete_txd(struct idxd_desc *desc,
 			   enum idxd_complete_type comp_type,
 			   bool free_desc, void *ctx, u32 *status);
 
+/*
+ * Whether the queue's descriptors are dmaengine transactions. Another
+ * driver, such as iaa_crypto, keeps its own context where they keep
+ * their txd.
+ */
+static inline bool idxd_wq_has_txd(struct idxd_wq *wq)
+{
+	return wq_to_idxd_drv(wq)->desc_complete == idxd_dma_complete_txd;
+}
+
 static inline void idxd_desc_complete(struct idxd_desc *desc,
 				      enum idxd_complete_type comp_type,
 				      bool free_desc)
diff --git a/drivers/dma/idxd/submit.c b/drivers/dma/idxd/submit.c
index 5806b1b74a2f..fab3e0176cba 100644
--- a/drivers/dma/idxd/submit.c
+++ b/drivers/dma/idxd/submit.c
@@ -52,6 +52,21 @@ struct idxd_desc *idxd_alloc_desc(struct idxd_wq *wq, enum idxd_op_type optype)
 		return __get_desc(wq, idx, cpu);
 	}
 
+	/*
+	 * A dying reference whose resurrect has completed belongs to a
+	 * queue quiesced for good: no kill in progress will revive it, and
+	 * every submission fails. Waiting for a slot there gains nothing
+	 * and may never end. The teardown flush and the interrupt thread
+	 * free the slot of each descriptor they complete only when its
+	 * completion returns, and iaa_crypto's verify of a compression
+	 * allocates from inside one. The two reads are not atomic, so an
+	 * allocation that races a revive can fail like a request the kill
+	 * caught.
+	 */
+	if (data_race(percpu_ref_is_dying(&wq->wq_active)) &&
+	    completion_done(&wq->wq_resurrect))
+		return ERR_PTR(-ENXIO);
+
 	ws = &sbq->ws[0];
 	for (;;) {
 		sbitmap_prepare_to_wait(sbq, ws, &wait, TASK_INTERRUPTIBLE);
@@ -149,7 +164,12 @@ static void llist_abort_desc(struct idxd_wq *wq, struct idxd_irq_entry *ie,
 	 * complete and free it, and the caller frees it as well.
 	 */
 	WARN_ON_ONCE(!found);
-	if (found)
+	/*
+	 * Another driver's submitter cleans up all of its own: it learns
+	 * of the failure from the return value and its descriptor carries
+	 * its request where a dmaengine descriptor has its txd.
+	 */
+	if (found && idxd_wq_has_txd(wq))
 		idxd_dma_complete_txd(desc, IDXD_COMPLETE_ABORT, false,
 				      NULL, NULL);
 }
-- 
2.43.0

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

[Daniel Byrne]

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

[PATCH 16/17] dmaengine: idxd: keep client callbacks on a halt flush and cancel resubmissions after a terminate

[Daniel Byrne]

idxd_flush_pending_descs() strips every callback unconditionally. That
is right when a work queue is torn down with nobody left to call, since
the callback may point into an unloaded module, and wrong when the
device halts with a client still holding the channel: the client then
waits on descriptors that were quietly completed without it.

Take the choice as a parameter. Teardown paths and a client's own
terminate_all keep stripping; the halt flush keeps the callbacks when
the channel has a client. That flush runs under wq_lock, and a callback
may terminate the channel, which takes that lock, so the flush does not
run the callbacks itself: it marks every descriptor the halted device
never finished as aborted, leaves it on the work list with the finished
ones, and wakes the queue's interrupt thread, which completes them
without the lock. The client sees DMA_TRANS_WRITE_FAILED for a
descriptor the halt killed, as for every abort the driver marks.

llist_abort_desc() has the mirror problem. A failed submission is
reported to the client by idxd_submit_desc()'s return value and the
client frees the descriptor, so a callback on top of that completes it
twice. The driver's own interrupt-handle resubmission has no return
path to the client at all, and there the abort must go through the
callback. Split the submit helper: idxd_submit_desc() keeps the
caller-cleans-up contract and strips the callbacks of a found
descriptor, and the new idxd_resubmit_desc() is what the resubmit
worker uses so that a failure there still reaches the client.

Since "dmaengine: idxd: keep the interrupt handle across terminate_all"
such a resubmission really runs, and a terminate_sync() no longer
covers it. A flush that strips the callbacks therefore completes a
descriptor that reports a revoked handle as aborted instead of
resubmitting it. A descriptor submitted before a terminate can still
reach the resubmit work after the flush, from a work item queued
earlier or from the interrupt thread. terminate_all bumps a per-queue
generation under wq_lock before its kill, an interrupt-armed client
submission stamps it into the descriptor, and a resubmission whose
stamp is older is cancelled once it holds wq_active and completed
without its callback.

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 | 54 ++++++++++++++++++++++++++++++++------
 drivers/dma/idxd/dma.c    | 12 ++++++++-
 drivers/dma/idxd/idxd.h   | 13 +++++++--
 drivers/dma/idxd/irq.c    | 27 +++++++++++--------
 drivers/dma/idxd/submit.c | 55 ++++++++++++++++++++++++++++++++++-----
 5 files changed, 132 insertions(+), 29 deletions(-)

diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index 158c141650e3..5ccdc5ff8d53 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -1527,7 +1527,7 @@ int idxd_device_load_config(struct idxd_device *idxd)
 }
 
 static void idxd_flush_pending_descs(struct idxd_irq_entry *ie,
-				     bool quarantine)
+				     bool keep_callbacks, bool quarantine)
 {
 	struct idxd_wq *wq = ie_to_wq(ie);
 	struct idxd_desc *desc, *itr;
@@ -1543,6 +1543,28 @@ static void idxd_flush_pending_descs(struct idxd_irq_entry *ie,
 			list_add_tail(&desc->list, &ie->work_list);
 	}
 
+	/*
+	 * A halt flush that delivers the callbacks of a dmaengine queue
+	 * leaves its descriptors to the interrupt thread, which runs them
+	 * without wq_lock, as it runs every completion. A callback may
+	 * terminate the channel, which takes that lock, and the halt flush
+	 * holds it. The halted device writes no record any more, so one it
+	 * never finished is marked aborted for the thread to complete.
+	 */
+	if (keep_callbacks && idxd_wq_has_txd(wq)) {
+		bool wake = false;
+
+		list_for_each_entry(desc, &ie->work_list, list) {
+			if (!desc->completion->status)
+				desc->completion->status = IDXD_COMP_DESC_ABORT;
+			wake = true;
+		}
+		spin_unlock(&ie->list_lock);
+		if (wake)
+			irq_wake_thread(ie->vector, ie);
+		return;
+	}
+
 	list_for_each_entry_safe(desc, itr, &ie->work_list, list)
 		list_move_tail(&desc->list, &flist);
 	spin_unlock(&ie->list_lock);
@@ -1571,7 +1593,8 @@ static void idxd_flush_pending_descs(struct idxd_irq_entry *ie,
 		 * descriptors are likely to be stuck and can be dropped.
 		 * With no client left the callback could point to code that
 		 * is no longer accessible (dmatest unloaded), and a client's
-		 * own terminate must not get it, so it is stripped.
+		 * own terminate must not get it, so it is stripped. A halt
+		 * that delivers the callbacks left them to the thread above.
 		 */
 		tx = &desc->txd;
 		tx->callback = NULL;
@@ -1595,7 +1618,14 @@ static void idxd_flush_pending_descs(struct idxd_irq_entry *ie,
 			list_add_tail(&desc->list, &qlist);
 			continue;
 		}
-		idxd_dma_complete_txd(desc, ctype, true, NULL, NULL);
+		/*
+		 * The abort type keeps a record that reports a revoked
+		 * interrupt handle from being resubmitted. The device is done
+		 * with it, and with the handle kept across a terminate the
+		 * copy would run once the client may have freed its memory.
+		 */
+		idxd_dma_complete_txd(desc, IDXD_COMPLETE_ABORT, true, NULL,
+				      NULL);
 	}
 
 	if (list_empty(&qlist))
@@ -1693,7 +1723,7 @@ void idxd_wq_free_irq(struct idxd_wq *wq)
 		return;
 
 	free_irq(ie->vector, ie);
-	idxd_flush_pending_descs(ie, false);
+	idxd_flush_pending_descs(ie, false, false);
 
 	/* FLR revokes every handle; the halt path marks them invalid. */
 	if (ie->int_handle == INVALID_INT_HANDLE)
@@ -1714,12 +1744,19 @@ void idxd_wq_free_irq(struct idxd_wq *wq)
 }
 
 /*
+ * @keep_callbacks: deliver the flushed descriptors' completions to the
+ * client, aborted, or with their hardware status where the device had
+ * already finished them. A client's own terminate_all must not get them
+ * (the dmaengine contract); a device halt with a client still holding the
+ * channel must, or the client waits for them forever. The interrupt
+ * thread delivers them, outside wq_lock.
  * @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 keep_callbacks,
+			   bool quarantine)
 {
 	struct idxd_irq_entry *ie = &wq->ie;
 	bool killed = false;
@@ -1737,17 +1774,18 @@ void __idxd_wq_flush_descs(struct idxd_wq *wq, bool quarantine)
 	 */
 	if (quarantine)
 		killed = idxd_wq_kill(wq);
-	idxd_flush_pending_descs(ie, quarantine);
+	idxd_flush_pending_descs(ie, keep_callbacks, quarantine);
 	if (killed) {
 		percpu_ref_reinit(&wq->wq_active);
 		complete_all(&wq->wq_resurrect);
 	}
 }
 
-void idxd_wq_flush_descs(struct idxd_wq *wq, bool quarantine)
+void idxd_wq_flush_descs(struct idxd_wq *wq, bool keep_callbacks,
+			 bool quarantine)
 {
 	guard(mutex)(&wq->wq_lock);
-	__idxd_wq_flush_descs(wq, quarantine);
+	__idxd_wq_flush_descs(wq, keep_callbacks, quarantine);
 }
 
 int idxd_wq_request_irq(struct idxd_wq *wq)
diff --git a/drivers/dma/idxd/dma.c b/drivers/dma/idxd/dma.c
index 1d08856f5c4e..0a7517cb51f2 100644
--- a/drivers/dma/idxd/dma.c
+++ b/drivers/dma/idxd/dma.c
@@ -199,8 +199,18 @@ static int idxd_dma_terminate_all(struct dma_chan *c)
 {
 	struct idxd_wq *wq = to_idxd_wq(c);
 
+	guard(mutex)(&wq->wq_lock);
+	/*
+	 * A descriptor that completed with a revoked interrupt handle is
+	 * resubmitted by a work item. One submitted before this terminate
+	 * may reach that work after the flush, from the interrupt thread or
+	 * from a work item queued earlier, and is cancelled there rather
+	 * than run with its callback once the client may have freed its
+	 * memory.
+	 */
+	WRITE_ONCE(wq->terminate_gen, wq->terminate_gen + 1);
 	/* The device keeps running what the lists held. See synchronize. */
-	idxd_wq_flush_descs(wq, true);
+	__idxd_wq_flush_descs(wq, false, true);
 
 	return 0;
 }
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index 0ad61bc233e8..b0cfd7101174 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -229,6 +229,9 @@ struct idxd_wq {
 				 * time, so no slot is reused until a later
 				 * drain completes or a halt kills them
 				 */
+	u16 terminate_gen;	/* bumps on every terminate_all, under
+				 * wq_lock before its kill
+				 */
 	struct list_head quarantine;	/* descriptors a terminate_all took
 					 * off the lists while the device
 					 * still held them. Their slots, with
@@ -444,6 +447,9 @@ struct idxd_desc {
 	struct list_head list;
 	int id;
 	int cpu;
+	u16 terminate_gen;	/* the queue's terminate_gen at an
+				 * interrupt-armed submit
+				 */
 	struct idxd_wq *wq;
 };
 
@@ -832,8 +838,10 @@ 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_flush_descs(struct idxd_wq *wq, bool keep_callbacks,
+			   bool quarantine);
+void idxd_wq_flush_descs(struct idxd_wq *wq, bool keep_callbacks,
+			 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);
 void idxd_wq_quarantine_free(struct idxd_wq *wq, struct list_head *list);
@@ -841,6 +849,7 @@ void idxd_wq_halt_release(struct idxd_wq *wq);
 
 /* submission */
 int idxd_submit_desc(struct idxd_wq *wq, struct idxd_desc *desc);
+int idxd_resubmit_desc(struct idxd_wq *wq, struct idxd_desc *desc);
 struct idxd_desc *idxd_alloc_desc(struct idxd_wq *wq, enum idxd_op_type optype);
 int idxd_enqcmds(struct idxd_wq *wq, void __iomem *portal, const void *desc);
 
diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c
index 99b2df7fc2de..4517a09fcf1b 100644
--- a/drivers/dma/idxd/irq.c
+++ b/drivers/dma/idxd/irq.c
@@ -452,7 +452,7 @@ static void idxd_wqs_halt_flush(struct idxd_device *idxd, bool resurrect)
 			continue;
 		}
 		killed = idxd_wq_kill(wq);
-		__idxd_wq_flush_descs(wq, false);
+		__idxd_wq_flush_descs(wq, idxd_wq_refcount(wq) > 0, false);
 		idxd_wq_halt_release(wq);
 
 		if (resurrect) {
@@ -659,22 +659,27 @@ static void idxd_int_handle_resubmit_work(struct work_struct *work)
 	int rc;
 
 	desc->completion->status = 0;
-	rc = idxd_submit_desc(wq, desc);
+	rc = idxd_resubmit_desc(wq, desc);
 	if (rc < 0) {
 		dev_dbg(&wq->idxd->pdev->dev, "Failed to resubmit desc %d to wq %d.\n",
 			desc->id, wq->id);
 		/*
-		 * If the error is not -EAGAIN, it means the submission failed due to wq
-		 * has been killed instead of ENQCMDS failure. Here the driver needs to
-		 * notify the submitter of the failure by reporting abort status.
-		 *
-		 * -EAGAIN comes from ENQCMDS failure. idxd_submit_desc() will handle the
-		 * abort.
+		 * The submission failed because the device is not enabled,
+		 * because the wq has been killed or because ENQCMDS failed,
+		 * and in each case the descriptor is on no list. Here the
+		 * driver needs to notify the submitter of the failure by
+		 * reporting abort status. The submit has dropped its
+		 * wq_active reference by now, so a callback that submits or
+		 * terminates does not wait for it. A descriptor submitted
+		 * before a terminate_all, which cancels its resubmission,
+		 * gets no callback, since its client terminated it.
 		 */
-		if (rc != -EAGAIN) {
-			desc->completion->status = IDXD_COMP_DESC_ABORT;
-			idxd_desc_complete(desc, IDXD_COMPLETE_ABORT, false);
+		if (desc->terminate_gen != READ_ONCE(wq->terminate_gen)) {
+			desc->txd.callback = NULL;
+			desc->txd.callback_result = NULL;
 		}
+		desc->completion->status = IDXD_COMP_DESC_ABORT;
+		idxd_desc_complete(desc, IDXD_COMPLETE_ABORT, false);
 		idxd_free_desc(wq, desc);
 	}
 	kfree(irw);
diff --git a/drivers/dma/idxd/submit.c b/drivers/dma/idxd/submit.c
index cf8536e94c15..469a0cadb984 100644
--- a/drivers/dma/idxd/submit.c
+++ b/drivers/dma/idxd/submit.c
@@ -112,7 +112,7 @@ static struct idxd_desc *list_abort_desc(struct idxd_wq *wq, struct idxd_irq_ent
 }
 
 static void llist_abort_desc(struct idxd_wq *wq, struct idxd_irq_entry *ie,
-			     struct idxd_desc *desc)
+			     struct idxd_desc *desc, bool submitter_cleans)
 {
 	struct idxd_desc *d, *t, *found = NULL;
 	struct llist_node *head;
@@ -175,13 +175,23 @@ static void llist_abort_desc(struct idxd_wq *wq, struct idxd_irq_entry *ie,
 	 */
 	WARN_ON_ONCE(!found);
 	/*
-	 * Another driver's submitter cleans up all of its own: it learns
-	 * of the failure from the return value and its descriptor carries
-	 * its request where a dmaengine descriptor has its txd.
+	 * When the failing submission is a client's tx_submit(), the
+	 * client learns of it from the return value and owns the cleanup,
+	 * and a callback on top of that would complete the descriptor
+	 * twice. A dmaengine descriptor then only has its cookie completed
+	 * and its unmap data released here, with the callbacks stripped.
+	 * Nothing reads its record before the caller frees it, so it is
+	 * not marked. An internal resubmission reports the abort through
+	 * the callback, which its worker runs once the submit has dropped
+	 * its reference. Another driver's submitter cleans up all of its
+	 * own, and only dmaengine descriptors are resubmitted.
 	 */
-	if (found && idxd_wq_has_txd(wq))
+	if (found && submitter_cleans && idxd_wq_has_txd(wq)) {
+		desc->txd.callback = NULL;
+		desc->txd.callback_result = NULL;
 		idxd_dma_complete_txd(desc, IDXD_COMPLETE_ABORT, false,
 				      NULL, NULL);
+	}
 }
 
 /*
@@ -208,7 +218,8 @@ int idxd_enqcmds(struct idxd_wq *wq, void __iomem *portal, const void *desc)
 	return rc;
 }
 
-int idxd_submit_desc(struct idxd_wq *wq, struct idxd_desc *desc)
+static int __idxd_submit_desc(struct idxd_wq *wq, struct idxd_desc *desc,
+			      bool submitter_cleans)
 {
 	struct idxd_device *idxd = wq->idxd;
 	struct idxd_irq_entry *ie = NULL;
@@ -240,6 +251,19 @@ int idxd_submit_desc(struct idxd_wq *wq, struct idxd_desc *desc)
 		percpu_ref_put(&wq->wq_active);
 		return -EIO;
 	}
+	/*
+	 * A resubmission of a descriptor first submitted before a
+	 * terminate_all is cancelled. The terminate bumps the generation
+	 * before its kill, so the barrier above orders this read too. A
+	 * resubmission that read the old one is on the pending list when
+	 * the terminate's flush runs, since the kill waits for it, or has
+	 * failed with its stamp unchanged.
+	 */
+	if (!submitter_cleans &&
+	    desc->terminate_gen != READ_ONCE(wq->terminate_gen)) {
+		percpu_ref_put(&wq->wq_active);
+		return -ECANCELED;
+	}
 
 	portal = idxd_wq_portal_addr(wq);
 
@@ -250,6 +274,12 @@ int idxd_submit_desc(struct idxd_wq *wq, struct idxd_desc *desc)
 	if (desc_flags & IDXD_OP_FLAG_RCI) {
 		ie = &wq->ie;
 		desc->hw->int_handle = ie->int_handle;
+		/*
+		 * A resubmission keeps the stamp of the client's submit, so
+		 * the resubmit work sees every terminate since then.
+		 */
+		if (submitter_cleans)
+			desc->terminate_gen = READ_ONCE(wq->terminate_gen);
 		llist_add(&desc->llnode, &ie->pending_llist);
 	}
 
@@ -275,7 +305,7 @@ int idxd_submit_desc(struct idxd_wq *wq, struct idxd_desc *desc)
 			 * frees the descriptor.
 			 */
 			if (ie)
-				llist_abort_desc(wq, ie, desc);
+				llist_abort_desc(wq, ie, desc, submitter_cleans);
 			percpu_ref_put(&wq->wq_active);
 			return rc;
 		}
@@ -284,4 +314,15 @@ int idxd_submit_desc(struct idxd_wq *wq, struct idxd_desc *desc)
 	percpu_ref_put(&wq->wq_active);
 	return 0;
 }
+/* A client submission: on failure the return value is the notification. */
+int idxd_submit_desc(struct idxd_wq *wq, struct idxd_desc *desc)
+{
+	return __idxd_submit_desc(wq, desc, true);
+}
 EXPORT_SYMBOL_NS_GPL(idxd_submit_desc, "IDXD");
+
+/* The driver's own resubmission: a failure must still call the client back. */
+int idxd_resubmit_desc(struct idxd_wq *wq, struct idxd_desc *desc)
+{
+	return __idxd_submit_desc(wq, desc, false);
+}
-- 
2.43.0

[PATCH 17/17] dmaengine: idxd: run the interrupt-handle revoke under the queue lock

[Daniel Byrne]

The interrupt-handle revoke worker requests each new handle without
wq_lock, so that a reset can end the request, and the queue may be
unbound while it waits. It reads the queue's state without the lock,
submits the drain after unlocking, and on a failed request quiesces the
queue and completes the descriptors that carry the revoked handle
outside the lock, so an unbind can unmap the portal under the drain, or
free the descriptors, the allocator and the driver under the
completions.

The unbind invalidates the handle in the same wq_lock hold that tears
the queue down, so after the request the worker takes wq_lock and goes
on only if the handle is still valid. The handle swap and the drain, or
the quiesce and the completions after a failed request, then run in
that hold. A queue a halt already killed and did not revive is left
dead rather than revived behind the halt.

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/irq.c | 45 +++++++++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 12 deletions(-)

diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c
index 4517a09fcf1b..8217698056bb 100644
--- a/drivers/dma/idxd/irq.c
+++ b/drivers/dma/idxd/irq.c
@@ -115,6 +115,11 @@ static void idxd_abort_invalid_int_handle_descs(struct idxd_irq_entry *ie)
 	struct idxd_desc *d, *t;
 	struct llist_node *head;
 
+	/*
+	 * An unbind frees the descriptors and the allocator under
+	 * wq_lock, so they are completed under it as well.
+	 */
+	lockdep_assert_held(&ie_to_wq(ie)->wq_lock);
 	spin_lock(&ie->list_lock);
 	head = llist_del_all(&ie->pending_llist);
 	if (head) {
@@ -142,6 +147,7 @@ static void idxd_int_handle_revoke(struct work_struct *work)
 	struct pci_dev *pdev = idxd->pdev;
 	struct device *dev = &pdev->dev;
 	int i, new_handle, rc;
+	bool killed;
 
 	if (!idxd->request_int_handles) {
 		kfree(revoke);
@@ -166,15 +172,31 @@ static void idxd_int_handle_revoke(struct work_struct *work)
 			continue;
 
 		rc = idxd_device_request_int_handle(idxd, i, &new_handle, IDXD_IRQ_MSIX);
-		if (rc < 0) {
+		if (rc < 0)
 			dev_warn(dev, "get int handle %d failed: %d\n", i, rc);
+
+		/*
+		 * The request runs without wq_lock, so that a reset can end
+		 * it, and the queue may have been unbound meanwhile. An
+		 * unbind invalidates the handle in the same wq_lock hold
+		 * that unmaps the portal and frees the descriptors, so a
+		 * handle still valid under the lock means that the queue
+		 * still has its portal, its descriptors and its completion
+		 * handler. Everything below, the drain included, stays in
+		 * this hold.
+		 */
+		guard(mutex)(&wq->wq_lock);
+		if (ie->int_handle == INVALID_INT_HANDLE)
+			continue;
+
+		if (rc < 0) {
 			/*
 			 * Failed to acquire new interrupt handle. Kill the WQ
 			 * and release all the pending submitters. The submitters will
 			 * get error return code and handle appropriately.
 			 */
 			ie->int_handle = INVALID_INT_HANDLE;
-			idxd_wq_quiesce(wq);
+			__idxd_wq_quiesce(wq);
 			idxd_abort_invalid_int_handle_descs(ie);
 			continue;
 		}
@@ -193,21 +215,20 @@ static void idxd_int_handle_revoke(struct work_struct *work)
 			continue;
 		}
 
-		mutex_lock(&wq->wq_lock);
-		reinit_completion(&wq->wq_resurrect);
-
-		/* Kill percpu_ref to pause additional descriptor submissions */
-		percpu_ref_kill(&wq->wq_active);
-
-		/* Wait for all submitters quiesce before we change interrupt handle */
-		wait_for_completion(&wq->wq_dead);
+		/*
+		 * Kill percpu_ref to pause additional descriptor submissions
+		 * and wait for all submitters to quiesce before we change
+		 * the interrupt handle. A queue a halt already killed and did
+		 * not revive is left dead.
+		 */
+		killed = idxd_wq_kill(wq);
 
 		ie->int_handle = new_handle;
 
 		/* Revive percpu ref and wake up all the waiting submitters */
-		percpu_ref_reinit(&wq->wq_active);
+		if (killed)
+			percpu_ref_reinit(&wq->wq_active);
 		complete_all(&wq->wq_resurrect);
-		mutex_unlock(&wq->wq_lock);
 
 		/*
 		 * The delay here is to wait for all possible MOVDIR64B that
-- 
2.43.0

Re: [PATCH 00/17] [PATCH 00/17] dmaengine: idxd: fixes found by an in-kernel polling client

[Frank Li]

On Mon, Oct 19, 2026 at 08:06:13AM -0700, Daniel Byrne wrote:
> [You don't often get email from daniel1.byrne@intel.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> These are fixes for bugs in the idxd driver that an in-kernel polling
> client exposed while sharing DSA work queues between many io_uring rings
> (that client is to be posted separately as "io_uring: DMA-engine copy offload
> for buffered file I/O").

where the link, I can't find at

https://lore.kernel.org/all/?q=io_uring%3A+DMA-engine+copy+offload+for+buffered+file+I%2FO

Frank