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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dmaengine: idxd: premature wq_active revival in idxd_wqs_halt_flush()
--

commit 6dda938d3d45e583931b07390e3058529e447340
Author: Daniel Byrne <daniel1.byrne@intel.com>

dmaengine: idxd: flush the work queues on every halt type

This commit flushes kernel work queues on all device halt types (software
reset, FLR, and system reset) rather than only on FLR. Before flushing, it
kills wq_active and waits for in-flight submitters to finish, reviving the
reference afterwards on software reset.

> diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c
> index 1ec3d360eea8f..99b2df7fc2de0 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
[ ... ]
> diff --git a/drivers/dma/idxd/submit.c b/drivers/dma/idxd/submit.c
> index fab3e0176cbae..cf8536e94c15e 100644
> --- a/drivers/dma/idxd/submit.c
> +++ b/drivers/dma/idxd/submit.c
[ ... ]
> @@ -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);

[Severity: High]
Can reviving wq->wq_active in idxd_wqs_halt_flush() before
idxd_device_reinit() runs allow idxd_submit_desc() to post descriptors to a
work queue that is still disabled in hardware?

On a software-reset halt, idxd_halt() calls idxd_wqs_halt_flush(idxd, true),
which reinitializes wq->wq_active and completes wq->wq_resurrect, and then
queues idxd_device_reinit(). In idxd_submit_desc(), the second check only
verifies idxd->state != IDXD_DEV_ENABLED after acquiring wq->wq_active.

However, in idxd_device_reinit():

drivers/dma/idxd/irq.c:idxd_device_reinit() {
	...
	rc = idxd_device_enable(idxd);
	if (rc < 0)
		goto out;

	for (i = 0; i < idxd->max_wqs; i++) {
		if (test_bit(i, idxd->wq_enable_map)) {
			struct idxd_wq *wq = idxd->wqs[i];

			rc = idxd_wq_enable(wq);
			if (rc < 0) {
				clear_bit(i, idxd->wq_enable_map);
				dev_warn(dev, "Unable to re-enable wq %s\n",
					 dev_name(wq_confdev(wq)));
			}
		}
	}
}

idxd_device_enable() sets idxd->state = IDXD_DEV_ENABLED before the loop
calls idxd_wq_enable() on each work queue.

Once idxd->state is set to IDXD_DEV_ENABLED, won't submitters on a dedicated
work queue pass both checks in idxd_submit_desc(), add interrupt-armed
descriptors to ie->pending_llist, and call iosubmit_cmds512() before
idxd_wq_enable() has enabled that work queue in hardware, causing the device
to drop the descriptors and leaving waiting clients hung?

In addition, there appear to be two cases where the work queue is never
re-enabled in hardware even after idxd_device_reinit() finishes while
wq->wq_active remains live:

1. If idxd_wq_enable() fails in idxd_device_reinit(), it warns and clears
   wq_enable_map, leaving idxd->state == IDXD_DEV_ENABLED and wq->wq_active
   live on the disabled work queue.

2. If IDXD_FLAG_CONFIGURABLE is not set, idxd_device_reset() ->
   idxd_device_clear_state() does not clear wq->state, so idxd_wq_enable()
   sees wq->state == IDXD_WQ_ENABLED and returns 0 without issuing
   IDXD_CMD_ENABLE_WQ:

drivers/dma/idxd/device.c:idxd_wq_enable() {
	...
	if (wq->state == IDXD_WQ_ENABLED) {
		dev_dbg(dev, "WQ %d already enabled\n", wq->id);
		return 0;
	}

	idxd_cmd_exec(idxd, IDXD_CMD_ENABLE_WQ, wq->id, &status);
	...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019150630.4026341-1-daniel1.byrne@intel.com?part=15