[PATCH v2 0/3] spi: tegra210-quad: Fix timeout path status handling and DMA leak

[PATCH v2 0/3] spi: tegra210-quad: Fix timeout path status handling and DMA leak

[Vishwaroop A]

Three fixes in the Tegra QSPI transfer timeout path: the two from v1, plus
one new patch out of Mark's review.

Patch 1 fixes a regression from commit 1ff82c13ea94 ("spi: tegra210-quad:
Cache TRANS_STATUS in ISR for timeout handler"), currently in linux-next
only. When tegra_qspi_handle_timeout() observes a real ISR completion
after draining the workqueue, it still publishes the pre-drain lost-IRQ
FIFO snapshot. That snapshot can hold zeroed error bits if the ISR W1C-ed
the hardware between the two reads, which makes the manual completion
handler downstream accept a transfer that actually reported a FIFO
underflow or overflow. No stable backport is needed because the offending
commit has not been released.

Patch 2 is new in v2. Mark observed that the unmap added by patch 3 can
run for a transfer flagged as DMA that has nothing mapped, which happens
when one chunk of a multi-chunk transfer completes and the next fails in
tegra_qspi_dma_map_xfer(). The same hazard is already reachable through
handle_dma_based_xfer()'s own cleanup, so rather than special-case the new
site this records the mapping state and makes tegra_qspi_dma_unmap_xfer()
a no-op when nothing is outstanding. Failing the transfer on a start
error, the other option Mark offered, is left for a follow-up along with
the mappings leaked by tegra_qspi_start_dma_based_transfer()'s own error
returns. Tagged for stable.

Patch 3 fixes a DMA mapping leak present since the driver was added. The
only unmap lives in handle_dma_based_xfer(), which runs from the interrupt
bottom half, so on a genuine timeout the bottom half never runs and both
transfer paths abort without unmapping. Every real timeout therefore leaks
the streaming mappings for that chunk, gradually exhausting the IOVA space
on systems with an IOMMU in front of QSPI. Tagged for stable, and it has
to be applied after patch 2, which its stable tag records.

Neither patch 2 nor patch 3 applies to a released tree as posted, although
both bugs are older than the code they are written against: patch 2 needs
tqspi->tx_dma_dev and tqspi->rx_dma_dev from commit f469138a77ac, itself
still linux-next only, and patch 3 collides with the WARN_ON() calls added
to the timeout paths after v6.19. I will send adapted backports once this
series lands.

Changes in v2:
 - New patch 2 recording DMA mapping state, so patch 3's unmap cannot run
   against a stale or never-created mapping (Mark Brown).
 - Picked up Breno's Reviewed-by on patch 1.
 - Recorded patch 2 as a stable prerequisite on patch 3's tag.
 - Rebased onto next-20261023; patch 1 and patch 3 code is unchanged.

v1: https://lore.kernel.org/r/20261016132520.3723412-1-va@nvidia.com

Vishwaroop A (3):
  spi: tegra210-quad: don't overwrite refreshed status with lost-IRQ
    snapshot
  spi: tegra210-quad: Track DMA mapping state
  spi: tegra210-quad: Unmap DMA buffers on transfer timeout

 drivers/spi/spi-tegra210-quad.c | 52 ++++++++++++++++++++++++++-------
 1 file changed, 42 insertions(+), 10 deletions(-)


base-commit: 20a33c6b6cbe7e4805dfb6cd68816937443aa148
-- 
2.17.1

[PATCH v2 1/3] spi: tegra210-quad: don't overwrite refreshed status with lost-IRQ snapshot

[Vishwaroop A]

tegra_qspi_handle_timeout() captures a "lost IRQ" FIFO status snapshot
when the ISR cache is empty on entry but QSPI_TRANS_STATUS shows the
transfer completed. After masking the controller IRQ and draining the
workqueue with cancel_work_sync(), it re-loads the ISR cache with
smp_load_acquire(): if the drain observed a real ISR completion, the
refreshed cache holds ISR-published status_reg, tx_status and rx_status
that reflect the true hardware error state at IRQ time.

However, when both cache and snapshot are set, the code keeps the
refreshed value in the local status variable but then unconditionally
publishes the pre-drain lost_fifo_status / lost_tx_status /
lost_rx_status snapshot into tqspi->status_reg / tx_status / rx_status
via the lost_irq_snapshot branch. That snapshot is racy: if the ISR
fires on another CPU between our live QSPI_TRANS_STATUS read and our
live QSPI_FIFO_STATUS snapshot, the ISR W1Cs the hardware error bits
before we sample them, so the snapshot may contain zeroed error
fields. Overwriting the ISR-published values with these zeros makes
the manual completion handler downstream silently accept a transfer
that actually reported a FIFO error.

Clear lost_irq_snapshot when the refreshed cache is non-zero, so the
ISR-published status wins over the pre-drain snapshot in that case.
The lost-IRQ path (refreshed == 0 && live QSPI_RDY set) is unaffected.

Fixes: 1ff82c13ea94 ("spi: tegra210-quad: Cache TRANS_STATUS in ISR for timeout handler")
Reported-by: Carol Soto <csoto@nvidia.com>
Link: https://github.com/NVIDIA/NV-Kernels/pull/579
Signed-off-by: Vishwaroop A <va@nvidia.com>
Reviewed-by: Breno Leitao <leitao@debian.org>
---
 drivers/spi/spi-tegra210-quad.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-tegra210-quad.c b/drivers/spi/spi-tegra210-quad.c
index d8cca3aa276b..4a5933d8112c 100644
--- a/drivers/spi/spi-tegra210-quad.c
+++ b/drivers/spi/spi-tegra210-quad.c
@@ -1260,8 +1260,17 @@ static int tegra_qspi_handle_timeout(struct tegra_qspi *tqspi)
 	 * loaded the cache but before we masked).
 	 */
 	refreshed = smp_load_acquire(&tqspi->trans_status);
-	if (refreshed)
+	if (refreshed) {
 		status = refreshed;
+		/*
+		 * The drain observed a real ISR completion. Prefer the
+		 * ISR-published status_reg / tx_status / rx_status over
+		 * the pre-drain lost-IRQ snapshot, which may have been
+		 * taken after the ISR W1C-ed the hardware error bits and
+		 * therefore holds cleared values.
+		 */
+		lost_irq_snapshot = false;
+	}
 
 	if (!(status & QSPI_RDY)) {
 		retval = -ETIMEDOUT;
-- 
2.17.1

[PATCH v2 2/3] spi: tegra210-quad: Track DMA mapping state

[Vishwaroop A]

tegra_qspi_dma_unmap_xfer() assumes a streaming mapping is always
outstanding and recomputes its length from the live
tqspi->curr_dma_words. Nothing records whether tegra_qspi_dma_map_xfer()
mapped anything, so neither assumption holds.

For a multi-chunk transfer, handle_dma_based_xfer() unmaps the chunk that
just completed, recalculates the parameters for the next chunk, which
overwrites curr_dma_words, and re-arms the hardware. If
tegra_qspi_start_dma_based_transfer() then fails at its
tegra_qspi_dma_map_xfer() call, nothing is mapped. That early return also
sits above the smp_store_release() that clears tqspi->trans_status, so the
previous chunk's QSPI_RDY survives, and the re-arm's return value is never
checked, so handle_dma_based_xfer() returns IRQ_HANDLED without completing
the transfer.

The caller times out, tegra_qspi_handle_timeout() sees the stale QSPI_RDY,
concludes the transfer completed and calls handle_dma_based_xfer() again.
No DMA completion arrives, so num_errors is incremented and
tegra_qspi_dma_unmap_xfer() runs with nothing mapped. t->tx_dma then holds
either the DMA_MAPPING_ERROR sentinel or, if the RX mapping was the one
that failed, the preceding chunk's address that has already been released.
Behind an IOMMU the latter frees an IOVA that may since have been
reallocated.

Record whether a mapping is outstanding and the length that was mapped,
and return early from tegra_qspi_dma_unmap_xfer() when there is nothing to
release. is_dma_mapped is only updated under tqspi->lock, or on the timeout
path after synchronize_irq() and cancel_work_sync() have quiesced the ISR
and the bottom half.

Also skip releasing the TX mapping on the RX failure path unless a TX
mapping was created; t->tx_dma is uninitialised when t->tx_buf is NULL.

Fixes: 921fc1838fb0 ("spi: tegra210-quad: Add support for Tegra210 QSPI controller")
Suggested-by: Mark Brown <broonie@kernel.org>
Cc: stable@vger.kernel.org
Signed-off-by: Vishwaroop A <va@nvidia.com>
---
 drivers/spi/spi-tegra210-quad.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-tegra210-quad.c b/drivers/spi/spi-tegra210-quad.c
index 4a5933d8112c..d74aa46636ec 100644
--- a/drivers/spi/spi-tegra210-quad.c
+++ b/drivers/spi/spi-tegra210-quad.c
@@ -237,6 +237,17 @@ struct tegra_qspi {
 	bool					is_packed;
 	bool					use_dma;
 
+	/*
+	 * Set while a streaming mapping created by
+	 * tegra_qspi_dma_map_xfer() is outstanding, together with the
+	 * length that was handed to dma_map_single(). is_curr_dma_xfer
+	 * only records that the last armed chunk took the DMA path,
+	 * which is not the same thing: a chunk whose mapping failed
+	 * leaves the DMA path flagged with nothing mapped.
+	 */
+	bool					is_dma_mapped;
+	unsigned int				dma_map_len;
+
 	u32					command1_reg;
 	u32					dma_control_reg;
 	u32					def_command1_reg;
@@ -606,24 +617,32 @@ static int tegra_qspi_dma_map_xfer(struct tegra_qspi *tqspi, struct spi_transfer
 	if (t->rx_buf) {
 		t->rx_dma = dma_map_single(tqspi->rx_dma_dev, (void *)rx_buf, len, DMA_FROM_DEVICE);
 		if (dma_mapping_error(tqspi->rx_dma_dev, t->rx_dma)) {
-			dma_unmap_single(tqspi->tx_dma_dev, t->tx_dma, len, DMA_TO_DEVICE);
+			if (t->tx_buf)
+				dma_unmap_single(tqspi->tx_dma_dev, t->tx_dma,
+						 len, DMA_TO_DEVICE);
 			return -ENOMEM;
 		}
 	}
 
+	tqspi->dma_map_len = len;
+	tqspi->is_dma_mapped = true;
+
 	return 0;
 }
 
 static void tegra_qspi_dma_unmap_xfer(struct tegra_qspi *tqspi, struct spi_transfer *t)
 {
-	unsigned int len;
-
-	len = DIV_ROUND_UP(tqspi->curr_dma_words * tqspi->bytes_per_word, 4) * 4;
+	if (!tqspi->is_dma_mapped)
+		return;
 
 	if (t->tx_buf)
-		dma_unmap_single(tqspi->tx_dma_dev, t->tx_dma, len, DMA_TO_DEVICE);
+		dma_unmap_single(tqspi->tx_dma_dev, t->tx_dma,
+				 tqspi->dma_map_len, DMA_TO_DEVICE);
 	if (t->rx_buf)
-		dma_unmap_single(tqspi->rx_dma_dev, t->rx_dma, len, DMA_FROM_DEVICE);
+		dma_unmap_single(tqspi->rx_dma_dev, t->rx_dma,
+				 tqspi->dma_map_len, DMA_FROM_DEVICE);
+
+	tqspi->is_dma_mapped = false;
 }
 
 static int tegra_qspi_start_dma_based_transfer(struct tegra_qspi *tqspi, struct spi_transfer *t)
-- 
2.17.1

[PATCH v2 3/3] spi: tegra210-quad: Unmap DMA buffers on transfer timeout

[Vishwaroop A]

tegra_qspi_start_dma_based_transfer() maps the client TX/RX buffers with
dma_map_single() via tegra_qspi_dma_map_xfer(). The matching unmap only
happens in handle_dma_based_xfer(), which runs from the interrupt bottom
half.

When wait_for_completion_timeout() expires and tegra_qspi_handle_timeout()
cannot establish that the hardware finished, it returns an error without
the bottom half ever having run. Both transfer paths then abort the
transfer with tegra_qspi_dma_stop(), reset the controller and clear
curr_xfer, but never unmap the buffers. Every genuine transfer timeout
therefore leaks the streaming DMA mappings for that chunk, which
gradually exhausts the IOVA space on systems with an IOMMU in front of
QSPI.

Unmap the transfer after stopping DMA on both timeout error paths. The
unmap is placed after tegra_qspi_dma_stop() so the DMA engine is
quiesced before the mappings go away.

The recovery paths that do reach handle_dma_based_xfer() unmap there
themselves. The preceding patch makes tegra_qspi_dma_unmap_xfer() a no-op
once the mapping is released, so this cleanup cannot unmap twice or run
against a chunk that was never mapped.

Fixes: 921fc1838fb0 ("spi: tegra210-quad: Add support for Tegra210 QSPI controller")
Cc: stable@vger.kernel.org # with "spi: tegra210-quad: Track DMA mapping state"
Signed-off-by: Vishwaroop A <va@nvidia.com>
---
 drivers/spi/spi-tegra210-quad.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-tegra210-quad.c b/drivers/spi/spi-tegra210-quad.c
index d74aa46636ec..58c87c45431a 100644
--- a/drivers/spi/spi-tegra210-quad.c
+++ b/drivers/spi/spi-tegra210-quad.c
@@ -1507,10 +1507,12 @@ static int tegra_qspi_combined_seq_xfer(struct tegra_qspi *tqspi,
 					dev_err(tqspi->dev, "transfer timeout\n");
 
 					/* Abort transfer by resetting pio/dma bit */
-					if (tqspi->is_curr_dma_xfer)
+					if (tqspi->is_curr_dma_xfer) {
 						tegra_qspi_dma_stop(tqspi);
-					else
+						tegra_qspi_dma_unmap_xfer(tqspi, xfer);
+					} else {
 						tegra_qspi_pio_stop(tqspi);
+					}
 
 					/* Reset controller if timeout happens */
 					tegra_qspi_reset(tqspi);
@@ -1624,8 +1626,10 @@ static int tegra_qspi_non_combined_seq_xfer(struct tegra_qspi *tqspi,
 				WARN_ON(1);
 				dev_err(tqspi->dev, "transfer timeout\n");
 
-				if (tqspi->is_curr_dma_xfer)
+				if (tqspi->is_curr_dma_xfer) {
 					tegra_qspi_dma_stop(tqspi);
+					tegra_qspi_dma_unmap_xfer(tqspi, xfer);
+				}
 
 				tegra_qspi_handle_error(tqspi);
 				ret = -EIO;
-- 
2.17.1