On Wed, Oct 28, 2026 at 2:48 PM Raul E Rangel <[email protected]> wrote: > > Hi James, > > > @@ -37,26 +40,35 @@ static void __dma_tx_complete(void *param) > > struct uart_8250_port *p = param; > > struct uart_8250_dma *dma = p->dma; > > > > guard(uart_port_lock_irqsave)(&p->port); > > > > - if (dma->shutting_down) > > + if (serial8250_tx_dma_blocked(dma)) > > return; > > - > > - if (!dma->tx_running) > > - return; > > - > > serial8250_tx_dma_complete(p); > > } > > Removing `if (!dma->tx_running) return;` and relying solely on > dmaengine_synchronize(dma->txchan) to drain completion callbacks breaks on > DMAengine drivers used with 8250 UARTs that do not implement > .device_synchronize (where dmaengine_synchronize() is a no-op), including: > - drivers/dma/sun6i-dma.c (Allwinner H616, cited in the commit message) > - drivers/dma/dw/core.c (dw_dmac) > - drivers/dma/pl330.c > - drivers/dma/mediatek/mtk-uart-apdma.c
The provider-side fixes are posted separately for DesignWare [0], sun6i [1] and MediaTek [2]. The v8 consumer changes require those callback-drain guarantees on the affected controllers; they cannot compensate for a missing provider synchronization implementation. > > On those controllers, if Transfer #1's completion tasklet (vchan_complete, > dw_dma_tasklet, or pl330_tasklet) has already dequeued the completed > descriptor > on another CPU, dropped the channel lock, and is waiting on port->lock at the > start of __dma_tx_complete() when uart_flush_buffer() -> > serial8250_tx_dma_flush() runs: > 1. serial8250_tx_dma_flush_work() calls dmaengine_synchronize() (which returns > immediately because .device_synchronize is NULL), acquires port->lock, > clears dma->tx_terminating = false, and calls serial8250_tx_dma_complete(p) > -> uart_xmit_unblock(&p->port). > 2. If no new transfer has started yet (dma->tx_running == 0), Transfer #1's > in-flight callback acquires port->lock, sees > !serial8250_tx_dma_blocked(dma), > and runs serial8250_tx_dma_complete(p) a second time even though > dma->tx_running is 0. > 3. If a writer has already started Transfer #2 (dma->tx_running == 1, > dma->tx_flushed == false), Transfer #1's stale callback runs > serial8250_tx_dma_complete(p) while Transfer #2 is still active in > hardware, > syncing tx_addr for CPU, advancing xmit_fifo by Transfer #2's tx_size, and > submitting Transfer #3 concurrently with Transfer #2. > > In addition to implementing .device_synchronize in the affected DMAengine > drivers, __dma_tx_complete() (and omap_8250_dma_tx_complete() for symmetry) > should keep `if (!dma->tx_running) return;` and check > `dmaengine_tx_status(dma->txchan, dma->tx_cookie, NULL) == DMA_COMPLETE` > (matching dma_rx_complete()). I have not added this check. It could reject some stale callbacks, but dma->tx_cookie may already identify a replacement transfer, rather than the transfer whose callback is executing. It therefore cannot replace draining old callbacks before releasing or reusing storage. There is also a complication with the existing classic DW provider: its status path can scan descriptors and invoke completion callbacks. Calling it from the callback with the UART port lock held therefore risks re-entry. The DW series [0] removes that behavior, but callback draining is still required independently of a status check. The intended guarantee is that termination and callback draining finish before a replacement transfer can reuse the canceled transfer's storage. [0] https://lore.kernel.org/r/20261023-submit-dw-dma-synchronize-v1-v3-0-c62bf048f...@gmail.com/ [1] https://lore.kernel.org/r/20261023-submit-sun6i-dma-lifetime-next-v1-v3-8-ef1acb0be...@gmail.com/ [2] https://lore.kernel.org/r/20261027-submit-mtk-uart-dma-synchronize-v1-v3-0-aad7a6b3c...@gmail.com/ > We modeled the v7 TX and RX DMA cancellation state machines in TLA+ to verify > both the counterexample traces above and the fix: > https://github.com/ismell/linux-8250-dma-tla I extended this a bit as well: https://github.com/ismell/linux-8250-dma-tla/pull/1 One issue in general I'm frequently running into is that there's such a large amount of correctness issues with this driver that it's tricky to find good boundaries to split fixes up into. > > Thanks, > Raul
