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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dmaengine: idxd: idxd_cmd_reset_state() clears hold in idxd_cmd_hold()
- [Medium] dmaengine: idxd: idxd_cmd_hold() skips hold when IDXD_FLAG_CMD_TIMEDOUT is set
--

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

dmaengine: idxd: hold device commands off while the reset re-creates its interrupts

This commit introduces idxd_cmd_hold() and idxd_cmd_unhold() to wait for
any running device command and hold IDXD_FLAG_CMD_RUNNING across the
interrupt teardown and re-setup window in idxd_pci_probe_alloc() during
reset recovery.

> diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
> index d3b95593a6867..222c201669bdd 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;
> +	}

[Severity: Medium]
What happens in idxd_cmd_hold() if IDXD_FLAG_CMD_TIMEDOUT is set when
wait_event_lock_irq() returns?

When a command times out in idxd_cmd_exec(), IDXD_FLAG_CMD_RUNNING remains
set alongside IDXD_FLAG_CMD_TIMEDOUT. Because IDXD_FLAG_CMD_RUNNING is still
set, idxd_cmd_hold() skips setting held = true and returns false, and
idxd_pci_probe_alloc() proceeds to tear down and re-create interrupts
without a hold.

In idxd_cleanup_interrupts(), idxd_mask_error_interrupts() only masks error
interrupts in GENCTRL and does not mask IDXD_INTC_CMD. If a late command
completion interrupt arrives in idxd_misc_thread() before free_irq()
finishes (or if idxd_cmd_reset_state() runs concurrently):

drivers/dma/idxd/irq.c:idxd_misc_thread() {
    ...
		} else if (test_bit(IDXD_FLAG_CMD_TIMEDOUT, &idxd->flags)) {
			__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");
		}
    ...
}

Wouldn't this clear both IDXD_FLAG_CMD_RUNNING and IDXD_FLAG_CMD_TIMEDOUT
and wake idxd->cmd_waitq while the MSI-X vectors and misc interrupt are
still being torn down and re-allocated, allowing waiting or newly issued
commands to be submitted while interrupts are absent?

> +	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);
> +}

[Severity: High]
If a concurrent halt recovery in idxd_device_reinit() forgets the hold
while idxd_pci_probe_alloc() is between idxd_cleanup_interrupts() and
idxd_setup_interrupts(), won't the command started after
idxd_cmd_reset_state() still lose its completion interrupt and time out?

Specifically, idxd_cmd_hold() overloads IDXD_FLAG_CMD_RUNNING to block
new commands, while idxd_cmd_reset_state() unconditionally clears it:

drivers/dma/idxd/device.c:idxd_cmd_reset_state() {
    ...
	__clear_bit(IDXD_FLAG_CMD_RUNNING, &idxd->flags);
	clear_bit(IDXD_FLAG_CMD_TIMEDOUT, &idxd->flags);
	wake_up(&idxd->cmd_waitq);
    ...
}

And idxd_device_reinit() immediately issues a reset command right after
resetting the command state:

drivers/dma/idxd/irq.c:idxd_device_reinit() {
    ...
	idxd_cmd_reset_state(idxd);
	rc = idxd_device_reset(idxd);
    ...
}

If idxd_device_reinit() runs while the misc interrupt and MSI-X vectors
are freed during the reset re-probe in idxd_pci_probe_alloc(),
idxd_cmd_reset_state() clears IDXD_FLAG_CMD_RUNNING and wakes
idxd->cmd_waitq. Because pci_restore_state() in idxd_reset_done() has
already cleared the hardware halt state, idxd_device_reset() ->
idxd_cmd_exec() (along with any woken waiters) will submit a command while
the misc interrupt is torn down.

Although idxd_cmd_unhold() avoids clobbering the new command's state when
idxd->cmd_seq != seq, hasn't that command already been submitted while
interrupts were absent, causing it to lose its completion interrupt, time
out after 30 seconds, and fence the device?

[ ... ]

> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> index a037a6e763622..312b4670b3e82 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",

[ ... ]

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