[PATCH] spi: spi-pci1xxxx: quiesce DMA engines on transfer timeout

[PATCH] spi: spi-pci1xxxx: quiesce DMA engines on transfer timeout

[Weibin Liu]

If a DMA-mapped transfer times out waiting for DMA_TERM and neither DMA
abort interrupt arrived, pci1xxxx_spi_transfer_with_dma() returns
through the error label while both DMA engines are still enabled, the
done/abort interrupt status is still armed and the transfer state
(p->xfer, p->tx_sgl, p->rx_sgl) is still registered.

The ISRs keep firing afterwards: they reprogram the DMA engines from
p->tx_sgl / p->rx_sgl and signal completion through p->xfer and
p->spi_xfer_done, all of which the SPI core is free to unmap or reuse
once transfer_one() has returned, which is a use-after-free. A late
done interrupt can also start the next transfer queued behind the
failed one.

Stop both DMA engines, drain the pending done/abort interrupt status
and synchronize the DMA IRQs before dropping the in-progress state and
the transfer pointers, and re-enable the engines afterwards so that
subsequent transfers keep working. Also make the DMA ISRs ignore
interrupts while no transfer is in progress so that a stray MSI after
the cleanup can no longer act on the cleared state.

Fixes: 9538edeb72c9 ("spi: mchp-pci1xxxx: DMA support for copying data to and from SPI Buf")
Cc: stable@vger.kernel.org # 6.8+
Signed-off-by: Weibin Liu <liuwb@xiaopeng.com>
---
Reviewer notes:

- The quiesce sequence only runs on the DMA timeout branch, i.e. when
  neither DMA_TERM nor the two abort interrupts arrived; all other
  paths behave as before. Every path still passes through the error
  label, which now additionally drops the stale p->xfer, p->tx_sgl and
  p->rx_sgl pointers.
- Ordering matters: the done/abort status bits are drained under the
  same spinlocks the ISRs use, then p->spi_xfer_in_progress is dropped
  and the DMA IRQs are synchronized before the engines are turned back
  on, so an ISR can never observe a half-cleared state. The ISRs
  additionally return IRQ_NONE while no transfer is in progress, which
  also keeps the interrupt accounting right for stray MSIs after the
  cleanup.
- readl_poll_timeout() waits at most 1 ms per engine; it only runs
  after the transfer has already timed out, so the added latency stays
  on the failure path.

Tested on x86_64: with this patch applied the driver builds, loads and
unloads cleanly; no PCI1XXXX device is available to exercise the DMA
timeout path on hardware.

 drivers/spi/spi-pci1xxxx.c | 56 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/drivers/spi/spi-pci1xxxx.c b/drivers/spi/spi-pci1xxxx.c
index af6ed7849..b296d7f6e 100644
--- a/drivers/spi/spi-pci1xxxx.c
+++ b/drivers/spi/spi-pci1xxxx.c
@@ -542,6 +542,7 @@ static int pci1xxxx_spi_transfer_with_dma(struct spi_controller *spi_ctlr,
 	struct pci1xxxx_spi_internal *p = spi_controller_get_devdata(spi_ctlr);
 	struct pci1xxxx_spi *par = p->parent;
 	dma_addr_t tx_dma_addr = 0;
+	unsigned long flags;
 	int ret = 0;
 	u32 regval;
 
@@ -618,12 +619,58 @@ static int pci1xxxx_spi_transfer_with_dma(struct spi_controller *spi_ctlr,
 			p->dma_aborted_wr = false;
 			ret = -ECANCELED;
 		}
+
+		/*
+		 * A timed out transfer leaves the DMA engines running and
+		 * the done/abort interrupt status armed with p->xfer,
+		 * p->tx_sgl and p->rx_sgl still registered.  Stop the
+		 * engines, drain the pending status and synchronize the
+		 * IRQs before dropping the transfer state, otherwise a
+		 * late interrupt makes the ISRs reprogram the engines
+		 * from or signal completion through memory which the SPI
+		 * core is about to unmap or free.
+		 */
+		writel(SPI_DMA_ENGINE_DIS,
+		       par->dma_offset_bar + SPI_DMA_GLOBAL_WR_ENGINE_EN);
+		writel(SPI_DMA_ENGINE_DIS,
+		       par->dma_offset_bar + SPI_DMA_GLOBAL_RD_ENGINE_EN);
+		if (readl_poll_timeout(par->dma_offset_bar +
+				       SPI_DMA_GLOBAL_WR_ENGINE_EN, regval,
+				       regval == 0x0, 0, USEC_PER_MSEC) ||
+		    readl_poll_timeout(par->dma_offset_bar +
+				       SPI_DMA_GLOBAL_RD_ENGINE_EN, regval,
+				       regval == 0x0, 0, USEC_PER_MSEC))
+			dev_warn(&par->dev->dev,
+				 "SPI DMA engines failed to stop\n");
+
+		spin_lock_irqsave(&par->dma_wr_reg_lock, flags);
+		writel((SPI_DMA_DONE_INT_MASK(p->hw_inst) |
+			SPI_DMA_ABORT_INT_MASK(p->hw_inst)),
+		       par->dma_offset_bar + SPI_DMA_INTR_WR_CLR);
+		spin_unlock_irqrestore(&par->dma_wr_reg_lock, flags);
+		spin_lock_irqsave(&par->dma_rd_reg_lock, flags);
+		writel((SPI_DMA_DONE_INT_MASK(p->hw_inst) |
+			SPI_DMA_ABORT_INT_MASK(p->hw_inst)),
+		       par->dma_offset_bar + SPI_DMA_INTR_RD_CLR);
+		spin_unlock_irqrestore(&par->dma_rd_reg_lock, flags);
+
+		p->spi_xfer_in_progress = false;
+		synchronize_irq(p->irq[1]);
+		synchronize_irq(p->irq[2]);
+
+		writel(SPI_DMA_ENGINE_EN,
+		       par->dma_offset_bar + SPI_DMA_GLOBAL_WR_ENGINE_EN);
+		writel(SPI_DMA_ENGINE_EN,
+		       par->dma_offset_bar + SPI_DMA_GLOBAL_RD_ENGINE_EN);
 		goto error;
 	}
 	ret = 0;
 
 error:
 	p->spi_xfer_in_progress = false;
+	p->xfer = NULL;
+	p->tx_sgl = NULL;
+	p->rx_sgl = NULL;
 
 	return ret;
 }
@@ -695,6 +742,9 @@ static irqreturn_t pci1xxxx_spi_isr_dma_rd(int irq, void *dev)
 	unsigned long flags;
 	u32 regval;
 
+	if (!p->spi_xfer_in_progress)
+		return IRQ_NONE;
+
 	/* Clear the DMA RD INT and start spi xfer*/
 	regval = readl(p->parent->dma_offset_bar + SPI_DMA_INTR_RD_STS);
 	if (regval) {
@@ -723,6 +773,9 @@ static irqreturn_t pci1xxxx_spi_isr_dma_wr(int irq, void *dev)
 	unsigned long flags;
 	u32 regval;
 
+	if (!p->spi_xfer_in_progress)
+		return IRQ_NONE;
+
 	/* Clear the DMA WR INT */
 	regval = readl(p->parent->dma_offset_bar + SPI_DMA_INTR_WR_STS);
 	if (regval) {
@@ -755,6 +808,9 @@ static irqreturn_t pci1xxxx_spi_isr_dma(int irq, void *dev)
 	irqreturn_t spi_int_fired = IRQ_NONE;
 	u32 regval;
 
+	if (!p->spi_xfer_in_progress)
+		return IRQ_NONE;
+
 	/* Clear the SPI GO_BIT Interrupt */
 	regval = readl(p->parent->reg_base + SPI_MST_EVENT_REG_OFFSET(p->hw_inst));
 	if (regval & SPI_INTR) {

base-commit: 72d3fcf802c45d00b300f25b848a93c3a2bd7c7e
prerequisite-patch-id: e48582f6ffe124b3806593af6e22b74682c8a83f
prerequisite-patch-id: 369f74b9a7ecde56141b13ec671f9200345a3bab
prerequisite-patch-id: cbf6f60473c80add5b2cddf22d142f35a4e3c834
prerequisite-patch-id: b35e6ec691cd7a99f1ed86137a0ec101a8510c31
-- 
2.50.1

Re: [PATCH] spi: spi-pci1xxxx: quiesce DMA engines on transfer timeout

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 606 bytes --]

On Tue, Sep 29, 2026 at 04:11:56PM +0800, Weibin Liu wrote:

> Stop both DMA engines, drain the pending done/abort interrupt status
> and synchronize the DMA IRQs before dropping the in-progress state and
> the transfer pointers, and re-enable the engines afterwards so that
> subsequent transfers keep working. Also make the DMA ISRs ignore
> interrupts while no transfer is in progress so that a stray MSI after
> the cleanup can no longer act on the cleared state.

Why is the sensible thing to reset both channels immediately rather than
try to let the other one complete (or time out) if it's active?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: spi-pci1xxxx: quiesce DMA engines on transfer timeout

[Weibin Liu]

On Wed, Oct 14, 2026 at 03:37:15PM +0000, Mark Brown wrote:

> Why is the sensible thing to reset both channels immediately rather than
> try to let the other one complete (or time out) if it's active?

The two engines are not two independent channels that could each
finish their own work: they are the two legs of the same transfer.
The RD engine moves the TX direction (host to device), the WR engine
moves the RX direction (device to host), both are tied together by
dma_completion_count, and complete() only fires on the WR engine's
done for the last rx_sgl.  When one engine is wedged, the other can
never reach the end of the transfer, so "letting it complete" has no
reachable exit in this state machine; it would only run into its own
100 ms timeout.

Waiting would also not be safe: once transfer_one() returns, the SPI
core is free to unmap or reuse the buffers, and an engine that is
still enabled keeps DMAing into that memory.  The timeout path has to
stop the engines itself, and stopping both immediately just ends that
window.  The stop sequence is the one the driver already uses for its
abort paths (disable, poll for the readback to clear, drain this
instance's pending status, synchronize_irq); only the trigger changes,
from an abort interrupt to a timeout with no interrupt at all.

The cost to be upfront about is that the engine enable bits are shared
between the two SPI instances, so a transfer in flight on the peer
instance is interrupted and sees one recoverable -ETIMEDOUT (its own
wait has the same 100 ms bound).  The abort paths write the same
direction-wide enable bits with the same blast radius, so this patch
does not introduce a new category of interference.  The interrupt
enables are likewise shared per direction, which is why the cleanup
drains only this instance's status bits instead of masking the lines.

I'll follow up with a v2 that makes the re-enable conditional: the
engines are turned back on only once both were confirmed to have
stopped, and are otherwise left disabled - matching the abort paths,
where a failed stop returns -ECANCELED and the engines stay disabled.
Unconditionally re-enabling an engine that failed to stop would cancel
the pending stop request.

I don't have this part on a board and QEMU has no model of it.  If
someone at Microchip could exercise the timeout recovery path on
hardware (in particular whether the 1 ms stop poll budget is enough
there), that would be much appreciated.

[PATCH v2] spi: spi-pci1xxxx: quiesce DMA engines on transfer timeout

[Weibin Liu]

If a DMA-mapped transfer times out waiting for DMA_TERM and neither DMA
abort interrupt arrived, pci1xxxx_spi_transfer_with_dma() returns
through the error label while both DMA engines are still enabled, the
done/abort interrupt status is still armed and the transfer state
(p->xfer, p->tx_sgl, p->rx_sgl) is still registered.

The ISRs keep firing afterwards: they reprogram the DMA engines from
p->tx_sgl / p->rx_sgl and signal completion through p->xfer and
p->spi_xfer_done, all of which the SPI core is free to unmap or reuse
once transfer_one() has returned, which is a use-after-free. A late
done interrupt can also start the next transfer queued behind the
failed one.

Stop both DMA engines, drain the pending done/abort interrupt status
and synchronize the DMA IRQs before dropping the in-progress state and
the transfer pointers, and re-enable the engines afterwards so that
subsequent transfers keep working. The engines are only turned back on
once both were confirmed to have stopped: re-enabling an engine that
failed to stop would cancel the pending stop request and let it keep
running into buffers the SPI core is about to unmap or free, which is
exactly the window this patch closes. An engine that failed to stop is
left disabled, as the abort paths already do.

The DMA interrupt status and mask registers are shared between the two
SPI instances, so the cleanup drains only this instance's done/abort
status bits instead of masking the interrupt lines, which would also
suppress the peer instance's completion interrupts.

Also make the DMA ISRs ignore interrupts while no transfer is in
progress so that a stray MSI after the cleanup can no longer act on
the cleared state.

No PCI1XXXX hardware was available for testing and QEMU has no model
of this PCIe part, so the timeout path was exercised with a userspace
replica of the timeout branch and the DMA ISRs, with the register and
interrupt accesses mocked: on the unfixed code both a wedged engine
that clears after the timeout and a late done interrupt touch the
freed transfer state, while with this fix applied neither does and an
engine that failed to stop is left disabled.  The module was also
loaded and unloaded in a QEMU x86_64 guest running a kernel with this
fix applied.

Fixes: 9538edeb72c9 ("spi: mchp-pci1xxxx: DMA support for copying data to and from SPI Buf")
Cc: stable@vger.kernel.org # 6.8+
Signed-off-by: Weibin Liu <liuwb@xiaopeng.com>
---
Changes in v2:

- Keep the engines disabled when the stop poll fails instead of
  re-enabling them unconditionally: re-enabling an engine that failed
  to stop would cancel the pending stop request and reopen the window
  this patch closes.  The engines are now only turned back on once
  both were confirmed to have stopped, which matches the abort paths,
  where a failed stop returns -ECANCELED and the engines are left
  disabled.

- Extend the commit message with why the interrupt lines are not
  masked during the cleanup (the status and mask registers are shared
  with the peer SPI instance on this part) and with a testing note.

 drivers/spi/spi-pci1xxxx.c | 71 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/drivers/spi/spi-pci1xxxx.c b/drivers/spi/spi-pci1xxxx.c
index af6ed78493e3..c50e6841de52 100644
--- a/drivers/spi/spi-pci1xxxx.c
+++ b/drivers/spi/spi-pci1xxxx.c
@@ -542,6 +542,8 @@ static int pci1xxxx_spi_transfer_with_dma(struct spi_controller *spi_ctlr,
 	struct pci1xxxx_spi_internal *p = spi_controller_get_devdata(spi_ctlr);
 	struct pci1xxxx_spi *par = p->parent;
 	dma_addr_t tx_dma_addr = 0;
+	unsigned long flags;
+	bool engines_stopped;
 	int ret = 0;
 	u32 regval;
 
@@ -618,12 +620,72 @@ static int pci1xxxx_spi_transfer_with_dma(struct spi_controller *spi_ctlr,
 			p->dma_aborted_wr = false;
 			ret = -ECANCELED;
 		}
+
+		/*
+		 * A timed out transfer leaves the DMA engines running and
+		 * the done/abort interrupt status armed with p->xfer,
+		 * p->tx_sgl and p->rx_sgl still registered.  Stop the
+		 * engines, drain the pending status and synchronize the
+		 * IRQs before dropping the transfer state, otherwise a
+		 * late interrupt makes the ISRs reprogram the engines
+		 * from or signal completion through memory which the SPI
+		 * core is about to unmap or free.
+		 */
+		writel(SPI_DMA_ENGINE_DIS,
+		       par->dma_offset_bar + SPI_DMA_GLOBAL_WR_ENGINE_EN);
+		writel(SPI_DMA_ENGINE_DIS,
+		       par->dma_offset_bar + SPI_DMA_GLOBAL_RD_ENGINE_EN);
+		engines_stopped =
+			!(readl_poll_timeout(par->dma_offset_bar +
+					     SPI_DMA_GLOBAL_WR_ENGINE_EN,
+					     regval, regval == 0x0, 0,
+					     USEC_PER_MSEC) ||
+			  readl_poll_timeout(par->dma_offset_bar +
+					     SPI_DMA_GLOBAL_RD_ENGINE_EN,
+					     regval, regval == 0x0, 0,
+					     USEC_PER_MSEC));
+		if (!engines_stopped)
+			dev_warn(&par->dev->dev,
+				 "SPI DMA engines failed to stop\n");
+
+		spin_lock_irqsave(&par->dma_wr_reg_lock, flags);
+		writel((SPI_DMA_DONE_INT_MASK(p->hw_inst) |
+			SPI_DMA_ABORT_INT_MASK(p->hw_inst)),
+		       par->dma_offset_bar + SPI_DMA_INTR_WR_CLR);
+		spin_unlock_irqrestore(&par->dma_wr_reg_lock, flags);
+		spin_lock_irqsave(&par->dma_rd_reg_lock, flags);
+		writel((SPI_DMA_DONE_INT_MASK(p->hw_inst) |
+			SPI_DMA_ABORT_INT_MASK(p->hw_inst)),
+		       par->dma_offset_bar + SPI_DMA_INTR_RD_CLR);
+		spin_unlock_irqrestore(&par->dma_rd_reg_lock, flags);
+
+		p->spi_xfer_in_progress = false;
+		synchronize_irq(p->irq[1]);
+		synchronize_irq(p->irq[2]);
+
+		/*
+		 * Turn the engines back on only once both are confirmed
+		 * to have stopped: re-enabling an engine that failed to
+		 * stop would cancel the pending stop request and let it
+		 * run into buffers the SPI core is about to unmap or
+		 * free.  An engine that failed to stop is left disabled,
+		 * as the abort paths already do.
+		 */
+		if (engines_stopped) {
+			writel(SPI_DMA_ENGINE_EN,
+			       par->dma_offset_bar + SPI_DMA_GLOBAL_WR_ENGINE_EN);
+			writel(SPI_DMA_ENGINE_EN,
+			       par->dma_offset_bar + SPI_DMA_GLOBAL_RD_ENGINE_EN);
+		}
 		goto error;
 	}
 	ret = 0;
 
 error:
 	p->spi_xfer_in_progress = false;
+	p->xfer = NULL;
+	p->tx_sgl = NULL;
+	p->rx_sgl = NULL;
 
 	return ret;
 }
@@ -695,6 +757,9 @@ static irqreturn_t pci1xxxx_spi_isr_dma_rd(int irq, void *dev)
 	unsigned long flags;
 	u32 regval;
 
+	if (!p->spi_xfer_in_progress)
+		return IRQ_NONE;
+
 	/* Clear the DMA RD INT and start spi xfer*/
 	regval = readl(p->parent->dma_offset_bar + SPI_DMA_INTR_RD_STS);
 	if (regval) {
@@ -723,6 +788,9 @@ static irqreturn_t pci1xxxx_spi_isr_dma_wr(int irq, void *dev)
 	unsigned long flags;
 	u32 regval;
 
+	if (!p->spi_xfer_in_progress)
+		return IRQ_NONE;
+
 	/* Clear the DMA WR INT */
 	regval = readl(p->parent->dma_offset_bar + SPI_DMA_INTR_WR_STS);
 	if (regval) {
@@ -755,6 +823,9 @@ static irqreturn_t pci1xxxx_spi_isr_dma(int irq, void *dev)
 	irqreturn_t spi_int_fired = IRQ_NONE;
 	u32 regval;
 
+	if (!p->spi_xfer_in_progress)
+		return IRQ_NONE;
+
 	/* Clear the SPI GO_BIT Interrupt */
 	regval = readl(p->parent->reg_base + SPI_MST_EVENT_REG_OFFSET(p->hw_inst));
 	if (regval & SPI_INTR) {
-- 
2.50.1

Re: [PATCH v2] spi: spi-pci1xxxx: quiesce DMA engines on transfer timeout

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 685 bytes --]

On Fri, Oct 16, 2026 at 10:00:36AM +0800, Weibin Liu wrote:
> If a DMA-mapped transfer times out waiting for DMA_TERM and neither DMA
> abort interrupt arrived, pci1xxxx_spi_transfer_with_dma() returns
> through the error label while both DMA engines are still enabled, the
> done/abort interrupt status is still armed and the transfer state
> (p->xfer, p->tx_sgl, p->rx_sgl) is still registered.

Please don't send new patches in reply to old patches or serieses, this
makes it harder for both people and tools to understand what is going
on - it can bury things in mailboxes and make it difficult to keep track
of what current patches are, both for the new patches and the old ones.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] spi: spi-pci1xxxx: quiesce DMA engines on transfer timeout

[Mark Brown]

On Fri, 16 Oct 2026 10:00:36 +0800, Weibin Liu wrote:
> spi: spi-pci1xxxx: quiesce DMA engines on transfer timeout

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-7.4

Thanks!

[1/1] spi: spi-pci1xxxx: quiesce DMA engines on transfer timeout
      https://git.kernel.org/broonie/spi/c/0b591332a2bd

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark