dmaengine: mv_xor: add missing dma_wmb() barriers before descriptor chain start
PATCH 1/1 dmaengine: mv_xor: add missing dma_wmb() barriers before descriptor chain start
The descriptor data written to coherent memory must be visible to the DMA engine before the HW registers that trigger processing are written.
Add a dma_wmb() before mv_chan_set_next_descriptor() in mv_chan_start_new_chain() to order descriptor fields before the descriptor-pointer write.
Add a dma_wmb() after mv_desc_set_next_desc() in mv_chan_handle_chain_append() to order the chain link write before the subsequent check of the current-descriptor register. Without the barrier the hardware may observe stale descriptor data.
--- drivers/dma/mv_xor.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/dma/mv_xor.c +2 -0
@@ -181,6 +181,7 @@ static void mv_chan_start_new_chain(struct mv_xor_chan *mv_chan,dev_dbg (mv_chan_to_devp (mv_chan),"%s %d: sw_desc %p\n" , __func__, __LINE__, sw_desc); +dma_wmb ();/* set the hardware chain */ mv_chan_set_next_descriptor (mv_chan, sw_desc->async_tx.phys);
@@ -409,6 +410,7 @@ mv_xor_tx_submit(struct dma_async_tx_descriptor *tx)/* fix up the hardware chain */ mv_desc_set_next_desc (old_chain_tail, sw_desc->async_tx.phys); +dma_wmb ();/* if the channel is not busy */ if (!mv_chan_is_busy (mv_chan)) {
Signature · 1 line
2.55.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] dma_wmb() is insufficient to enforce Store-Load ordering between the memory write and MMIO read in mv_xor_tx_submit().
- [Medium] dma_wmb() is incorrectly used to order a coherent memory write against a subsequent MMIO write in mv_chan_start_new_chain().
Pre-existing issues:
- [High] Missing memory barrier before linking a new descriptor to an active hardware chain in mv_xor_tx_submit(). --
commit a54d735383cc294f5f7387dfea7d5abca9cdec6f
Author: Rosen Penev <[email protected]>
dmaengine: mv_xor: add missing dma_wmb() barriers before descriptor chain start
This commit adds dma_wmb() barriers in the DMA submission and chain starting paths to ensure descriptor data written to coherent memory is visible to the DMA engine before the hardware registers that trigger processing are written.
drivers/dma/mv_xor.c static void mv_chan_start_new_chain(struct mv_xor_chan *mv_chan, · line 186
Quoted 11 lines
drivers/dma/mv_xor.c +1
-0@@ -181,6 +181,7 @@ static void mv_chan_start_new_chain(struct mv_xor_chan *mv_chan,dev_dbg (mv_chan_to_devp (mv_chan),"%s %d: sw_desc %p\n" , __func__, __LINE__, sw_desc); +dma_wmb ();/* set the hardware chain */ mv_chan_set_next_descriptor (mv_chan, sw_desc->async_tx.phys);
[Severity: Medium]
Does this incorrectly use the memory-to-memory barrier API here?
According to Documentation/memory-barriers.txt, dma_*() barriers do not provide ordering guarantees for accesses to MMIO regions. Here, we are trying to order a coherent memory write against a subsequent relaxed MMIO write inside mv_chan_set_next_descriptor().
Should this use wmb() instead to properly enforce memory-to-MMIO ordering?
[ ... ]
drivers/dma/mv_xor.c mv_xor_tx_submit(struct dma_async_tx_descriptor *tx) · line 411
Quoted 3 lines
drivers/dma/mv_xor.c +0
-0@@ -409,6 +410,7 @@ mv_xor_tx_submit(struct dma_async_tx_descriptor *tx)/* fix up the hardware chain */
[Severity: High]
This isn't a bug introduced by this patch, but does the descriptor link pointer update need a preceding memory barrier?
Before this function is called, the descriptor is initialized with operational parameters. The spin_lock_bh() acquired at the start of mv_xor_tx_submit() only acts as an acquire barrier, which doesn't prevent preceding stores from being reordered after the link write.
Should there be a dma_wmb() placed immediately before mv_desc_set_next_desc() to guarantee that the hardware doesn't follow the new next-descriptor pointer and read an uninitialized descriptor?
drivers/dma/mv_xor.c mv_xor_tx_submit(struct dma_async_tx_descriptor *tx) · line 416
Quoted 5 lines
drivers/dma/mv_xor.c +1
-0mv_desc_set_next_desc (old_chain_tail, sw_desc->async_tx.phys); +dma_wmb ();/* if the channel is not busy */ if (!mv_chan_is_busy (mv_chan)) {
[Severity: High]
Is dma_wmb() sufficient to enforce store-load ordering here?
The patch adds dma_wmb() after mv_desc_set_next_desc() to order the memory store before the MMIO load in mv_chan_is_busy(), which uses readl_relaxed().
However, dma_wmb() strictly guarantees store-store ordering and does not prevent the CPU from executing a subsequent load before a preceding store is globally visible.
If the CPU reorders its MMIO load of the status register to happen before its memory write is globally visible, it could incorrectly read 'busy', causing the CPU to skip kicking the hardware. If the hardware simultaneously fetches the next descriptor right before the CPU writes the new link, it will see a null pointer and stop, leaving the channel stalled indefinitely.
Does this sequence require a full memory barrier like mb() instead?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Quoted 32 lines · Rosen Penev
The descriptor data written to coherent memory must be visible to the DMA engine before the HW registers that trigger processing are written.
Add a
dma_wmb()beforemv_chan_set_next_descriptor()inmv_chan_start_new_chain()to order descriptor fields before the descriptor-pointer write.Add a
dma_wmb()aftermv_desc_set_next_desc()inmv_chan_handle_chain_append()to order the chain link write before the subsequent check of the current-descriptor register. Without the barrier the hardware may observe stale descriptor data.Fixes ff7b04796d98 ("DMA engine driver for Marvell XOR engine")Assisted-by opencode:big-pickleSigned-off-by Rosen Penev <[email protected]>--- drivers/dma/mv_xor.c | 2 ++ 1 file changed, 2 insertions(+)drivers/dma/mv_xor.c +1
-0@@ -181,6 +181,7 @@ static void mv_chan_start_new_chain(struct mv_xor_chan *mv_chan,dev_dbg (mv_chan_to_devp (mv_chan),"%s %d: sw_desc %p\n" , __func__, __LINE__, sw_desc); +dma_wmb ();/* set the hardware chain */ mv_chan_set_next_descriptor (mv_chan, sw_desc->async_tx.phys);
drivers/dma/mv_xor.c mv_xor_tx_submit(struct dma_async_tx_descriptor *tx) · line 413
Quoted 4 lines
drivers/dma/mv_xor.c +1
-0@@ -409,6 +410,7 @@ mv_xor_tx_submit(struct dma_async_tx_descriptor *tx)/* fix up the hardware chain */ mv_desc_set_next_desc (old_chain_tail, sw_desc->async_tx.phys); +dma_wmb ();
mv_chan_set_next_descriptor() implement is wrong, it should use writel() instead of writel_relax().
Frank
Quoted 6 lines
drivers/dma/mv_xor.c +0
-1/* if the channel is not busy */ if (!mv_chan_is_busy (mv_chan)) { --2.55.0