The Linux Kernel Mailing List
 help / color / mirror / Atom feed
* [PATCH v5 0/3] spi: tegra210-quad: Improve interrupt handling for loaded systems
@ 2026-07-08  1:12 Vishwaroop A
  2026-07-08  1:12 ` [PATCH v5 1/3] spi: tegra210-quad: Convert to hard IRQ with high-priority workqueue Vishwaroop A
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Vishwaroop A @ 2026-07-08  1:12 UTC (permalink / raw)
  To: Thierry Reding, Jon Hunter, Mark Brown
  Cc: Vishwaroop A, Laxman Dewangan, Sowjanya Komatineni, Breno Leitao,
	Suresh Mangipudi, Krishna Yarlagadda, linux-tegra, linux-spi,
	linux-kernel

The current threaded IRQ implementation in spi-tegra210-quad suffers from
scheduler-induced latency on heavily loaded systems. Because threaded IRQ
handlers are subject to CFS scheduling, they can be delayed long enough to
trigger transfer timeouts even though hardware completes in microseconds.
This results in false timeout errors and WARN_ON splats during normal
operation.

This series addresses the problem in three steps:

1. Convert the threaded IRQ handler to a hard IRQ + high-priority unbound
   workqueue model. The hard IRQ does the minimum: capture FIFO status,
   mask and clear the controller IRQ, then schedule the bottom half. The
   workqueue handler runs in process context (can sleep for DMA
   completion) and can execute on any CPU, avoiding the CPU0 bottleneck
   inherent in threaded IRQs.

2. Cache QSPI_TRANS_STATUS in the ISR before clearing it. This lets the
   timeout handler distinguish between a real hardware timeout (QSPI_RDY
   not set) and a delayed workqueue (QSPI_RDY set), preventing false
   timeout errors when hardware has already completed. Pair the cache
   publication with smp_store_release()/smp_load_acquire() so the timeout
   handler observes a coherent set of cached fields on weakly-ordered
   architectures.

3. Process small PIO transfers (those that complete the whole spi_transfer
   in a single chunk) directly in hard IRQ context, eliminating workqueue
   scheduling latency for TPM-style short reads.

Runtime PM lifetime note (unchanged from v4): the work handler only
touches QSPI MMIO when curr_xfer is non-NULL. While curr_xfer is set,
the transfer thread is blocked in wait_for_completion_timeout() with
the SPI core's runtime PM reference held, so the clocks are guaranteed
on. When the work handler runs late after the timeout path has already
processed the transfer, it sees curr_xfer == NULL and returns without
any MMIO. With this invariant no additional PM reference handoff
between the ISR and the work handler is needed.

Changes since v4 (addressing review by Mark Brown [1]):

  Patch 1 ("Convert to hard IRQ with high-priority workqueue"):
    - Rewrote the tegra_qspi_work_handler() comment to describe the
      new serialisation invariant honestly: with the cancel_work_sync()
      added to tegra_qspi_handle_timeout() in patch 2, the work handler
      is never concurrent with the timeout path. The curr_xfer NULL
      check that remains simply catches the case where the timeout
      path already tore the transfer down before this work got a CPU.
      Addresses Mark's "Can't the timeout handler also be running at
      the same time as this?" comment.
    - Converted the tegra_qspi_isr() header comment to a proper
      kernel-doc block, with @irq / @context_data / Return: fields.
      No functional change.

  Patch 2 ("Cache TRANS_STATUS in ISR for timeout handler"):
    - Serialise tegra_qspi_handle_timeout() against the workqueue.
      Mask the controller IRQ (tegra_qspi_mask_clear_irq()) and then
      cancel_work_sync(&tqspi->irq_work) at the top of the recovery
      path, and re-check curr_xfer under the spinlock afterwards.
      Once cancel_work_sync() returns the bottom half is neither
      running nor pending, so the subsequent
      handle_{cpu,dma}_based_xfer() call from the timeout handler
      cannot race with the same call from the work handler. Addresses
      Mark's "This can be called from both tegra_qspi_work_handler()
      and tegra_qspi_handle_timeout() - I can't see what stops them
      both handling and completing the same transfer simultaneously?"
      review comment.
    - Clear the cached trans_status per chunk. Add
      smp_store_release(&trans_status, 0) immediately before
      tegra_qspi_unmask_irq() in both tegra_qspi_start_cpu_based_
      transfer() and tegra_qspi_start_dma_based_transfer(), so a
      multi-chunk DMA transfer (or the DMA -> PIO tail-chunk
      transition) cannot leave a stale RDY from chunk N in the cache
      when chunk N+1's completion times out. The store happens-before
      the unmask, and the new chunk's IRQ cannot fire until the
      controller register write below the unmask, so ordering with
      smp_load_acquire() in tegra_qspi_handle_timeout() is preserved.
      Addresses Mark's "It looks like the CPU based transfer function
      supports multiple interrupts per transfer ... don't we need to
      clear trans_status when we handle the interrupt as well?" review
      comment.
    - Take tqspi->lock across the ISR's status snapshot and cache
      publish sequence, and move tegra_qspi_mask_clear_irq() inside
      the locked region. This closes the QSPI_INTR_MASK RMW race
      Mark had already flagged on v3: setup_transfer_one()'s
      mask_clear_irq() and the ISR's mask_clear_irq() no longer race
      on the shared INTR_MASK register, and the cached status /
      status_reg fields are published atomically with respect to
      other tqspi->lock holders.

  Patch 3 ("Process small PIO transfers in hard IRQ context"):
    - Comment-only reflows to match the surrounding text and clarify
      that the fastpath cannot recurse into
      tegra_qspi_start_cpu_based_transfer() from hard IRQ context
      (multi-chunk PIO is only set on the final chunk, so the
      continuation path is intrinsically single-chunk).

No functional changes in patch 3.

Changes since v3:

  Patch 1 ("Convert to hard IRQ with high-priority workqueue"):
    - Dropped IRQF_SHARED. Tegra QSPI uses a dedicated GIC SPI line on
      every SoC that uses this driver, so the ISR does not need a
      runtime PM reference. Addresses Mark Brown's "Since we now have
      IRQF_SHARED we need to take a runtime PM reference here" comment.
    - Switched from devm_request_irq() to plain request_irq() in
      probe() and added explicit free_irq() in remove(), in the order:
      spi_unregister_controller -> free_irq -> destroy_workqueue ->
      pm_runtime_dont_use_autosuspend -> pm_runtime_force_suspend ->
      tegra_qspi_deinit_dma. Addresses Mark's "devm + non-devm mix
      seems likely to be racy" comments on probe() and remove().
    - Removed the tegra_qspi_unmask_irq() call from the work_handler
      NULL-bail path. Addresses Mark's "unmask after dropping the lock
      feels like it opens up races" comment. The next transfer's setup
      path re-arms the IRQ.
    - Snapshot tqspi->curr_xfer under tqspi->lock at the top of
      handle_dma_based_xfer() so the (potentially long) DMA waits
      operate on a stable transfer pointer even if the timeout path
      clears curr_xfer concurrently. Integrates cleanly with Breno
      Leitao's recently merged protect-curr_xfer series.

  Patch 2 ("Cache TRANS_STATUS in ISR for timeout handler"):
    - Cached status_reg / tx_status / rx_status / trans_status are now
      published with WRITE_ONCE() and smp_store_release() and consumed
      with smp_load_acquire() in tegra_qspi_handle_timeout(), so a
      timeout handler that observes non-zero trans_status also sees a
      coherent snapshot of the other fields on weakly-ordered
      architectures. Live MMIO fallback via tegra_qspi_readl() only
      runs when the cache is still zero (the ISR never ran).
    - Kept the WRITE_ONCE / dev_warn_ratelimited() / dev_err()
      diagnostics scaffolding around the wait_for_completion_timeout()
      calls in tegra_qspi_{combined,non_combined}_seq_xfer(). With
      the recovery path in place a timeout is no longer a programming
      error; the existing dev_warn_ratelimited("QSPI interrupt
      timeout, but transfer complete") and dev_err("transfer
      timeout") diagnostics are preserved.

  Patch 3 ("Process small PIO transfers in hard IRQ context"):
    - Replaced the previous "curr_dma_words <= QSPI_FIFO_DEPTH" check
      with a tqspi->is_last_pio_chunk scalar computed in
      tegra_qspi_start_cpu_based_transfer() before it unmasks the IRQ.
      Addresses Mark Brown's "Is cur_dma_words always in the same
      units as QSPI_FIFO_DEPTH - I see there's packed transfer support
      in the driver?" comment: the scalar comparison is units-correct
      (cur_pos + curr_dma_words * bytes_per_word >= t->len) and the
      hard-IRQ fastpath no longer dereferences the spi_transfer
      object, so it stays safe against any teardown race that could
      clear curr_xfer concurrently.
    - Fastpath additionally gates on tx_status == 0 && rx_status == 0
      because handle_cpu_based_xfer()'s error path calls
      tegra_qspi_handle_error() -> device_reset(), which can sleep and
      must not run from hard IRQ context.
    - is_curr_dma_xfer and is_last_pio_chunk are written from process
      context (the transfer-start functions) and read lock-free from
      the hard IRQ handler and the workqueue handler, so the writers
      use WRITE_ONCE() and the readers use READ_ONCE() to prevent
      compiler tearing and silence KCSAN.

Changes since v2:
  - Added cancel_work_sync() in remove to flush pending work before
    devm tore down the workqueue (Jon Hunter). v4 has replaced devm
    altogether per Mark Brown's comment, so the explicit teardown now
    relies on free_irq() preventing new work being queued, followed by
    destroy_workqueue() draining what is in-flight.
  - Rewrote patch 2 commit message to describe the race in terms of
    the workqueue model rather than referencing the old threaded IRQ
    (Jon).
  - s/NULLed/cleared/ in code comment (Jon).

Changes since v1:
  - Switched to devm_alloc_workqueue() and devm_request_irq() for
    resource management (Jon Hunter). v4 has since reverted to
    non-devm for the IRQ and workqueue per Mark Brown's review, so
    teardown order can be made explicit.
  - Improved patch 2 commit message to explain the timeout race
    scenario and clarify that the issue pre-exists the workqueue
    conversion (Jon).
  - Removed unnecessary local variable in tegra_qspi_handle_timeout
    (Jon).
  - Moved "workqueue was delayed" comment updates from patch 2 to
    patch 1, since patch 1 introduces the workqueue (Jon).

Tested on Tegra234 (Ferrix P3960) under sustained CPU stress (stress-ng
--cpu $(nproc) --timeout 3600s) while running dd if=/dev/mtd0 of=/dev/null
bs=1M count=100 in a loop; no QSPI timeouts, no softlockups, no lockdep
warnings, and mtd read/write + md5sum round-trip clean.

The series is based on linux-next (next-20260706).

[1] https://lore.kernel.org/linux-spi/cover.1781037385.git.va@nvidia.com/

Vishwaroop A (3):
  spi: tegra210-quad: Convert to hard IRQ with high-priority workqueue
  spi: tegra210-quad: Cache TRANS_STATUS in ISR for timeout handler
  spi: tegra210-quad: Process small PIO transfers in hard IRQ context

 drivers/spi/spi-tegra210-quad.c | 324 ++++++++++++++++++++++++++------
 1 file changed, 265 insertions(+), 59 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-07-09  3:01 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-08  1:12 [PATCH v5 0/3] spi: tegra210-quad: Improve interrupt handling for loaded systems Vishwaroop A
2026-07-08  1:12 ` [PATCH v5 1/3] spi: tegra210-quad: Convert to hard IRQ with high-priority workqueue Vishwaroop A
2026-07-08 12:15   ` Breno Leitao
2026-07-09  3:00     ` Vishwaroop A
2026-07-08  1:12 ` [PATCH v5 2/3] spi: tegra210-quad: Cache TRANS_STATUS in ISR for timeout handler Vishwaroop A
2026-07-08  1:12 ` [PATCH v5 3/3] spi: tegra210-quad: Process small PIO transfers in hard IRQ context Vishwaroop A

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox