* [PATCH RESEND 0/3] dmaengine: xilinx_dma: Fixes and optimizations for AXIDMA and MCDMA channel management
@ 2026-06-20 20:34 Suraj Gupta
2026-06-20 20:34 ` [PATCH RESEND 1/3] dmaengine: xilinx_dma: Fix channel idle state management in AXIDMA and MCDMA interrupt handlers Suraj Gupta
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Suraj Gupta @ 2026-06-20 20:34 UTC (permalink / raw)
To: vkoul, Frank.Li, michal.simek, linux-kernel
Cc: dmaengine, linux-arm-kernel, srinivas.neeli, dev
This patch series addresses issues and optimizations in the Xilinx
AXI DMA and MCDMA drivers:
1. Fix channel idle state management in the interrupt handlers.
2. Enable transfer chaining by removing unnecessary idle restrictions.
3. Optimize control register writes and channel start logic.
Changes in V2:
- Apply similar fixes and optimizations to MCDMA as well.
- Expand the 1/3 commit description with when the described issue occurs.
Suraj Gupta (3):
dmaengine: xilinx_dma: Fix channel idle state management in AXIDMA and
MCDMA interrupt handlers
dmaengine: xilinx_dma: Enable transfer chaining for AXIDMA and MCDMA
by removing idle restriction
dmaengine: xilinx_dma: Optimize control register write and channel
start logic for AXIDMA and MCDMA in corresponding start_transfer()
drivers/dma/xilinx/xilinx_dma.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH RESEND 1/3] dmaengine: xilinx_dma: Fix channel idle state management in AXIDMA and MCDMA interrupt handlers 2026-06-20 20:34 [PATCH RESEND 0/3] dmaengine: xilinx_dma: Fixes and optimizations for AXIDMA and MCDMA channel management Suraj Gupta @ 2026-06-20 20:34 ` Suraj Gupta 2026-06-20 20:47 ` sashiko-bot 2026-06-20 20:34 ` [PATCH RESEND 2/3] dmaengine: xilinx_dma: Enable transfer chaining for AXIDMA and MCDMA by removing idle restriction Suraj Gupta 2026-06-20 20:34 ` [PATCH RESEND 3/3] dmaengine: xilinx_dma: Optimize control register write and channel start logic for AXIDMA and MCDMA in corresponding start_transfer() Suraj Gupta 2 siblings, 1 reply; 7+ messages in thread From: Suraj Gupta @ 2026-06-20 20:34 UTC (permalink / raw) To: vkoul, Frank.Li, michal.simek, linux-kernel Cc: dmaengine, linux-arm-kernel, srinivas.neeli, dev Fix a race condition in AXIDMA and MCDMA irq handlers where the channel could be incorrectly marked as idle and attempt spurious transfers when descriptors are still being processed. The issue occurs when: 1. Multiple descriptors are queued and active. 2. An interrupt fires after completing some descriptors. 3. xilinx_dma_complete_descriptor() moves completed descriptors to done_list. 4. Channel is marked idle and start_transfer() is called even though active_list still contains unprocessed descriptors. 5. This leads to premature transfer attempts and potential descriptor corruption or missed completions. Only mark the channel as idle and start new transfers when the active list is actually empty, ensuring proper channel state management and avoiding spurious transfer attempts. Fixes: c0bba3a99f07 ("dmaengine: vdma: Add Support for Xilinx AXI Direct Memory Access Engine") Tested-by: Folker Schwesinger <dev@folker-schwesinger.de> Signed-off-by: Suraj Gupta <suraj.gupta2@amd.com> Co-developed-by: Srinivas Neeli <srinivas.neeli@amd.com> Signed-off-by: Srinivas Neeli <srinivas.neeli@amd.com> --- drivers/dma/xilinx/xilinx_dma.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c index 404235c17353..ca396b709742 100644 --- a/drivers/dma/xilinx/xilinx_dma.c +++ b/drivers/dma/xilinx/xilinx_dma.c @@ -1893,8 +1893,10 @@ static irqreturn_t xilinx_mcdma_irq_handler(int irq, void *data) if (status & XILINX_MCDMA_IRQ_IOC_MASK) { spin_lock(&chan->lock); xilinx_dma_complete_descriptor(chan); - chan->idle = true; - chan->start_transfer(chan); + if (list_empty(&chan->active_list)) { + chan->idle = true; + chan->start_transfer(chan); + } spin_unlock(&chan->lock); } @@ -1950,8 +1952,10 @@ static irqreturn_t xilinx_dma_irq_handler(int irq, void *data) XILINX_DMA_DMASR_DLY_CNT_IRQ)) { spin_lock(&chan->lock); xilinx_dma_complete_descriptor(chan); - chan->idle = true; - chan->start_transfer(chan); + if (list_empty(&chan->active_list)) { + chan->idle = true; + chan->start_transfer(chan); + } spin_unlock(&chan->lock); } -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH RESEND 1/3] dmaengine: xilinx_dma: Fix channel idle state management in AXIDMA and MCDMA interrupt handlers 2026-06-20 20:34 ` [PATCH RESEND 1/3] dmaengine: xilinx_dma: Fix channel idle state management in AXIDMA and MCDMA interrupt handlers Suraj Gupta @ 2026-06-20 20:47 ` sashiko-bot 0 siblings, 0 replies; 7+ messages in thread From: sashiko-bot @ 2026-06-20 20:47 UTC (permalink / raw) To: Suraj Gupta; +Cc: vkoul, dmaengine, Frank.Li Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: Pre-existing issues: - [Critical] The patch's intended fix is completely ineffective for MCDMA and CDMA channels due to a pre-existing bug where descriptors are unconditionally completed without checking hardware status. - [High] Cyclic DMA completion callbacks are permanently starved and will never be executed because the cyclic descriptor is moved out of `done_list` before the cleanup tasklet runs. - [High] The MCDMA IRQ handler fails to process descriptor completions when a delay interrupt occurs, leading to indefinite stalls. -- commit bb06990d374374bf16e72c807c81d0d1449ff514 Author: Suraj Gupta <suraj.gupta2@amd.com> dmaengine: xilinx_dma: Fix channel idle state management in AXIDMA and MCDMA interrupt handlers Fix a race condition in AXIDMA and MCDMA irq handlers where the channel could be incorrectly marked as idle and attempt spurious transfers when descriptors are still being processed. > diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c > index 404235c173538..ca396b7097427 100644 > --- a/drivers/dma/xilinx/xilinx_dma.c > +++ b/drivers/dma/xilinx/xilinx_dma.c > @@ -1893,8 +1893,10 @@ static irqreturn_t xilinx_mcdma_irq_handler(int irq, void *data) > if (status & XILINX_MCDMA_IRQ_IOC_MASK) { > spin_lock(&chan->lock); > xilinx_dma_complete_descriptor(chan); > - chan->idle = true; > - chan->start_transfer(chan); > + if (list_empty(&chan->active_list)) { > + chan->idle = true; > + chan->start_transfer(chan); > + } [Severity: Critical] This is a pre-existing issue, but does this list_empty() check actually prevent the race condition for MCDMA channels? Looking at drivers/dma/xilinx/xilinx_dma.c in xilinx_dma_complete_descriptor(), hardware completion status is only validated for AXIDMA: xilinx_dma_complete_descriptor() { ... if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) { struct xilinx_axidma_tx_segment *seg; seg = list_last_entry(&desc->segments, struct xilinx_axidma_tx_segment, node); if (!(seg->hw.status & XILINX_DMA_BD_COMP_MASK) && chan->has_sg) break; } ... } For MCDMA and CDMA channels, descriptors are unconditionally unlinked and moved to done_list. This means active_list is always emptied by xilinx_dma_complete_descriptor(), causing list_empty(&chan->active_list) to always evaluate to true. Doesn't this make the patch's fix ineffective for MCDMA, while also risking data corruption since clients might reuse memory before the hardware actually completes the transfer? [Severity: High] This is a pre-existing issue, but does the MCDMA IRQ handler properly process descriptor completions when a delay interrupt occurs? Unlike the AXIDMA handler which checks both FRM_CNT_IRQ and DLY_CNT_IRQ, if a delay interrupt fires for MCDMA in drivers/dma/xilinx/xilinx_dma.c, the handler only logs a message: xilinx_mcdma_irq_handler() { ... if (status & XILINX_MCDMA_IRQ_DELAY_MASK) { /* * Device takes too long to do the transfer when user requires * responsiveness. */ dev_dbg(chan->dev, "Inter-packet latency too long\n"); } ... } If a delay interrupt occurs without an accompanying IOC interrupt, which happens when the transferred count hasn't reached the coalesce threshold but the timer expires, descriptor completions are never processed. Could this lead to indefinite stalls for the DMA client since the channel becomes unresponsive? > spin_unlock(&chan->lock); > } > > @@ -1950,8 +1952,10 @@ static irqreturn_t xilinx_dma_irq_handler(int irq, void *data) > XILINX_DMA_DMASR_DLY_CNT_IRQ)) { > spin_lock(&chan->lock); > xilinx_dma_complete_descriptor(chan); > - chan->idle = true; > - chan->start_transfer(chan); > + if (list_empty(&chan->active_list)) { > + chan->idle = true; > + chan->start_transfer(chan); > + } [Severity: High] This is a pre-existing issue, but will cyclic DMA completion callbacks ever be executed since start_transfer() is called here? When a cyclic transfer completes, xilinx_dma_complete_descriptor() moves the cyclic descriptor from active_list to done_list. When start_transfer() is invoked immediately after, it preemptively moves the descriptor back into active_list because pending_list is empty in cyclic mode: drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_start_transfer() { ... list_splice_tail_init(&chan->done_list, &chan->active_list); ... } After the IRQ handler returns, the cleanup tasklet executes to iterate over done_list and trigger the callback: drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_chan_desc_cleanup() { ... list_for_each_entry_safe(desc, next, &chan->done_list, node) { struct dmaengine_result result; if (desc->cyclic) { xilinx_dma_chan_handle_cyclic(chan, desc, &flags); ... } Because done_list was already completely emptied by start_transfer(), won't this loop be bypassed entirely, causing the cyclic notification mechanism to fail and clients to stall? > spin_unlock(&chan->lock); > } -- Sashiko AI review · https://sashiko.dev/#/patchset/20260620203417.4000360-1-suraj.gupta2@amd.com?part=1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH RESEND 2/3] dmaengine: xilinx_dma: Enable transfer chaining for AXIDMA and MCDMA by removing idle restriction 2026-06-20 20:34 [PATCH RESEND 0/3] dmaengine: xilinx_dma: Fixes and optimizations for AXIDMA and MCDMA channel management Suraj Gupta 2026-06-20 20:34 ` [PATCH RESEND 1/3] dmaengine: xilinx_dma: Fix channel idle state management in AXIDMA and MCDMA interrupt handlers Suraj Gupta @ 2026-06-20 20:34 ` Suraj Gupta 2026-06-20 20:47 ` sashiko-bot 2026-06-20 20:34 ` [PATCH RESEND 3/3] dmaengine: xilinx_dma: Optimize control register write and channel start logic for AXIDMA and MCDMA in corresponding start_transfer() Suraj Gupta 2 siblings, 1 reply; 7+ messages in thread From: Suraj Gupta @ 2026-06-20 20:34 UTC (permalink / raw) To: vkoul, Frank.Li, michal.simek, linux-kernel Cc: dmaengine, linux-arm-kernel, srinivas.neeli, dev Remove the restrictive idle check in xilinx_dma_start_transfer() and xilinx_mcdma_start_transfer() that prevented new transfers from being queued when the channel was busy. Additionally, only update the CURDESC register when the channel is running in scatter-gather mode and active list is empty to avoid interfering with transfers already in progress. When the active list contains transfers, the hardware tail pointer extension mechanism handles chaining automatically. Tested-by: Folker Schwesinger <dev@folker-schwesinger.de> Signed-off-by: Suraj Gupta <suraj.gupta2@amd.com> Co-developed-by: Srinivas Neeli <srinivas.neeli@amd.com> Signed-off-by: Srinivas Neeli <srinivas.neeli@amd.com> --- drivers/dma/xilinx/xilinx_dma.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c index ca396b709742..35b553ee3205 100644 --- a/drivers/dma/xilinx/xilinx_dma.c +++ b/drivers/dma/xilinx/xilinx_dma.c @@ -1580,9 +1580,6 @@ static void xilinx_dma_start_transfer(struct xilinx_dma_chan *chan) return; } - if (!chan->idle) - return; - head_desc = list_first_entry(&chan->pending_list, struct xilinx_dma_tx_descriptor, node); tail_desc = list_last_entry(&chan->pending_list, @@ -1599,7 +1596,7 @@ static void xilinx_dma_start_transfer(struct xilinx_dma_chan *chan) dma_ctrl_write(chan, XILINX_DMA_REG_DMACR, reg); } - if (chan->has_sg) + if (chan->has_sg && list_empty(&chan->active_list)) xilinx_write(chan, XILINX_DMA_REG_CURDESC, head_desc->async_tx.phys); reg &= ~XILINX_DMA_CR_DELAY_MAX; @@ -1660,9 +1657,6 @@ static void xilinx_mcdma_start_transfer(struct xilinx_dma_chan *chan) if (chan->err) return; - if (!chan->idle) - return; - if (list_empty(&chan->pending_list)) return; @@ -1685,8 +1679,9 @@ static void xilinx_mcdma_start_transfer(struct xilinx_dma_chan *chan) dma_ctrl_write(chan, XILINX_MCDMA_CHAN_CR_OFFSET(chan->tdest), reg); /* Program current descriptor */ - xilinx_write(chan, XILINX_MCDMA_CHAN_CDESC_OFFSET(chan->tdest), - head_desc->async_tx.phys); + if (chan->has_sg && list_empty(&chan->active_list)) + xilinx_write(chan, XILINX_MCDMA_CHAN_CDESC_OFFSET(chan->tdest), + head_desc->async_tx.phys); /* Program channel enable register */ reg = dma_ctrl_read(chan, XILINX_MCDMA_CHEN_OFFSET); -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH RESEND 2/3] dmaengine: xilinx_dma: Enable transfer chaining for AXIDMA and MCDMA by removing idle restriction 2026-06-20 20:34 ` [PATCH RESEND 2/3] dmaengine: xilinx_dma: Enable transfer chaining for AXIDMA and MCDMA by removing idle restriction Suraj Gupta @ 2026-06-20 20:47 ` sashiko-bot 0 siblings, 0 replies; 7+ messages in thread From: sashiko-bot @ 2026-06-20 20:47 UTC (permalink / raw) To: Suraj Gupta; +Cc: dmaengine, vkoul, Frank.Li Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Critical] Removing the channel idle check unconditionally overwrites hardware registers in Direct (non-SG) mode, corrupting active transfers. - [Critical] Appending new transfers to a running Scatter-Gather chain breaks the hardware descriptor chain, as new descriptors are never physically linked in memory. - [High] Updating the interrupt coalesce threshold while the channel is active uses a partial descriptor count, disrupting interrupt generation. -- commit 59ccde00ae0d672019658b79ee8d2055ebb8fa7a Author: Suraj Gupta <suraj.gupta2@amd.com> dmaengine: xilinx_dma: Enable transfer chaining for AXIDMA and MCDMA by removing idle restriction This commit removes the idle check in xilinx_dma_start_transfer() and xilinx_mcdma_start_transfer() to allow queuing new transfers while the channel is busy. It also updates the CURDESC register only when the active list is empty in scatter-gather mode, relying on the hardware tail pointer extension mechanism to handle chaining. > diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c > index ca396b7097427..35b553ee3205c 100644 > --- a/drivers/dma/xilinx/xilinx_dma.c > +++ b/drivers/dma/xilinx/xilinx_dma.c > @@ -1580,9 +1580,6 @@ static void xilinx_dma_start_transfer(struct xilinx_dma_chan *chan) > return; > } > > - if (!chan->idle) > - return; > - [Severity: Critical] Does removing this early return cause active transfers to be corrupted in Direct (non-SG) mode? Without the idle check, xilinx_dma_start_transfer() can proceed while a transfer is running. In Direct DMA mode (has_sg is false), the code falls through and executes: xilinx_write(chan, XILINX_DMA_REG_SRCDSTADDR, xilinx_prep_dma_addr_t(hw->buf_addr)); dma_ctrl_write(chan, XILINX_DMA_REG_BTT, hw->control & chan->xdev->max_buffer_len); Could this unconditionally overwrite the active hardware registers if a transfer is already in progress? Additionally, moving the newly queued descriptor into active_list appears to cause xilinx_dma_complete_descriptor() to prematurely complete newly appended descriptors, because the loop break condition is skipped for non-SG mode: if (!(seg->hw.status & XILINX_DMA_BD_COMP_MASK) && chan->has_sg) break; [Severity: Critical] Can appending new transfers to a running Scatter-Gather chain break the hardware descriptor chain? When the idle check is removed, we update the TAILDESC register to append transfers on-the-fly. However, the Xilinx AXI DMA engine requires SG descriptors to be physically linked in memory via their next_desc pointers. In the driver, descriptor linking occurs in append_desc_queue(): if (list_empty(&chan->pending_list)) goto append; ... tail_desc = list_last_entry(&chan->pending_list, ...); Since start_transfer() moves all queued descriptors to chan->active_list and leaves pending_list empty, a subsequent submission will find an empty pending_list. Will this cause it to skip linking its next_desc pointer to the tail of the actively running descriptor chain in active_list? When the DMA engine reaches the end of the original active chain, it might follow a stale next_desc pointer instead of the physically unlinked new descriptors. > head_desc = list_first_entry(&chan->pending_list, > struct xilinx_dma_tx_descriptor, node); > tail_desc = list_last_entry(&chan->pending_list, > @@ -1599,7 +1596,7 @@ static void xilinx_dma_start_transfer(struct xilinx_dma_chan *chan) > dma_ctrl_write(chan, XILINX_DMA_REG_DMACR, reg); > } > > - if (chan->has_sg) > + if (chan->has_sg && list_empty(&chan->active_list)) [Severity: High] Does updating the interrupt coalesce threshold while the channel is active disrupt interrupt generation? The driver updates the hardware interrupt coalesce threshold using chan->desc_pendingcount before writing to the DMACR register: if (chan->desc_pendingcount <= XILINX_DMA_COALESCE_MAX) { reg &= ~XILINX_DMA_CR_COALESCE_MAX; reg |= chan->desc_pendingcount << XILINX_DMA_CR_COALESCE_SHIFT; } With the removal of the idle check, this function now executes while descriptors are actively running. Since desc_pendingcount is reset to 0 at the end of each start_transfer() call, it now only represents the number of newly submitted descriptors in the latest pending_list batch. Will overwriting the active coalesce threshold with this potentially much smaller partial count disrupt the DMA engine's interrupt generation and cause erratic coalescing behavior? > xilinx_write(chan, XILINX_DMA_REG_CURDESC, > head_desc->async_tx.phys); > reg &= ~XILINX_DMA_CR_DELAY_MAX; [ ... ] > @@ -1660,9 +1657,6 @@ static void xilinx_mcdma_start_transfer(struct xilinx_dma_chan *chan) > if (chan->err) > return; > > - if (!chan->idle) > - return; > - [Severity: Critical] Does MCDMA suffer from the same unlinked descriptor chain vulnerability when TDESC is updated while active? Like xilinx_dma_start_transfer(), appending new transfers to a running Scatter-Gather chain might break the hardware descriptor chain because the new descriptors are not physically linked in memory to the active list. > if (list_empty(&chan->pending_list)) > return; > > @@ -1685,8 +1679,9 @@ static void xilinx_mcdma_start_transfer(struct xilinx_dma_chan *chan) > dma_ctrl_write(chan, XILINX_MCDMA_CHAN_CR_OFFSET(chan->tdest), reg); > > /* Program current descriptor */ > - xilinx_write(chan, XILINX_MCDMA_CHAN_CDESC_OFFSET(chan->tdest), > - head_desc->async_tx.phys); > + if (chan->has_sg && list_empty(&chan->active_list)) [Severity: High] Does this also overwrite the MCDMA CR coalesce field with a partial count while the engine may be active? if (chan->desc_pendingcount <= XILINX_MCDMA_COALESCE_MAX) { reg &= ~XILINX_MCDMA_COALESCE_MASK; reg |= chan->desc_pendingcount << XILINX_MCDMA_COALESCE_SHIFT; } Similar to the AXIDMA case, using desc_pendingcount here while active might disrupt interrupt generation. > + xilinx_write(chan, XILINX_MCDMA_CHAN_CDESC_OFFSET(chan->tdest), > + head_desc->async_tx.phys); > > /* Program channel enable register */ > reg = dma_ctrl_read(chan, XILINX_MCDMA_CHEN_OFFSET); -- Sashiko AI review · https://sashiko.dev/#/patchset/20260620203417.4000360-1-suraj.gupta2@amd.com?part=2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH RESEND 3/3] dmaengine: xilinx_dma: Optimize control register write and channel start logic for AXIDMA and MCDMA in corresponding start_transfer() 2026-06-20 20:34 [PATCH RESEND 0/3] dmaengine: xilinx_dma: Fixes and optimizations for AXIDMA and MCDMA channel management Suraj Gupta 2026-06-20 20:34 ` [PATCH RESEND 1/3] dmaengine: xilinx_dma: Fix channel idle state management in AXIDMA and MCDMA interrupt handlers Suraj Gupta 2026-06-20 20:34 ` [PATCH RESEND 2/3] dmaengine: xilinx_dma: Enable transfer chaining for AXIDMA and MCDMA by removing idle restriction Suraj Gupta @ 2026-06-20 20:34 ` Suraj Gupta 2026-06-20 20:46 ` sashiko-bot 2 siblings, 1 reply; 7+ messages in thread From: Suraj Gupta @ 2026-06-20 20:34 UTC (permalink / raw) To: vkoul, Frank.Li, michal.simek, linux-kernel Cc: dmaengine, linux-arm-kernel, srinivas.neeli, dev Optimize AXI DMA control register programming by consolidating coalesce count and delay configuration into a single register write. Previously, the coalesce count was written separately from the delay configuration, resulting in two register writes. Combine these into one write operation to reduce bus overhead. Additionally, avoid redundant channel starts in xilinx_dma_start_transfer() and xilinx_mcdma_start_transfer() by only calling xilinx_dma_start() when the channel is actually idle. Tested-by: Folker Schwesinger <dev@folker-schwesinger.de> Signed-off-by: Suraj Gupta <suraj.gupta2@amd.com> Co-developed-by: Srinivas Neeli <srinivas.neeli@amd.com> Signed-off-by: Srinivas Neeli <srinivas.neeli@amd.com> --- drivers/dma/xilinx/xilinx_dma.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c index 35b553ee3205..aa3dee0dc2fc 100644 --- a/drivers/dma/xilinx/xilinx_dma.c +++ b/drivers/dma/xilinx/xilinx_dma.c @@ -1593,7 +1593,6 @@ static void xilinx_dma_start_transfer(struct xilinx_dma_chan *chan) reg &= ~XILINX_DMA_CR_COALESCE_MAX; reg |= chan->desc_pendingcount << XILINX_DMA_CR_COALESCE_SHIFT; - dma_ctrl_write(chan, XILINX_DMA_REG_DMACR, reg); } if (chan->has_sg && list_empty(&chan->active_list)) @@ -1604,7 +1603,8 @@ static void xilinx_dma_start_transfer(struct xilinx_dma_chan *chan) reg |= XILINX_DMA_DMAXR_ALL_IRQ_MASK; dma_ctrl_write(chan, XILINX_DMA_REG_DMACR, reg); - xilinx_dma_start(chan); + if (chan->idle) + xilinx_dma_start(chan); if (chan->err) return; @@ -1693,7 +1693,8 @@ static void xilinx_mcdma_start_transfer(struct xilinx_dma_chan *chan) reg |= XILINX_MCDMA_CR_RUNSTOP_MASK; dma_ctrl_write(chan, XILINX_MCDMA_CHAN_CR_OFFSET(chan->tdest), reg); - xilinx_dma_start(chan); + if (chan->idle) + xilinx_dma_start(chan); if (chan->err) return; -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH RESEND 3/3] dmaengine: xilinx_dma: Optimize control register write and channel start logic for AXIDMA and MCDMA in corresponding start_transfer() 2026-06-20 20:34 ` [PATCH RESEND 3/3] dmaengine: xilinx_dma: Optimize control register write and channel start logic for AXIDMA and MCDMA in corresponding start_transfer() Suraj Gupta @ 2026-06-20 20:46 ` sashiko-bot 0 siblings, 0 replies; 7+ messages in thread From: sashiko-bot @ 2026-06-20 20:46 UTC (permalink / raw) To: Suraj Gupta; +Cc: vkoul, dmaengine, Frank.Li Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Pre-existing issues: - [High] Concurrent read-modify-write on shared global MCDMA registers due to lack of a controller-level lock. -- commit ecde894a65e016199009c2bd5c7fcf89cdab08cf Author: Suraj Gupta <suraj.gupta2@amd.com> dmaengine: xilinx_dma: Optimize control register write and channel start logic for AXIDMA and MCDMA in corresponding start_transfer() This commit optimizes AXI DMA control register programming by combining coalesce count and delay configuration into a single register write to reduce bus overhead. It also avoids redundant channel starts by only calling xilinx_dma_start() when the channel is actually idle. > diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c > index 35b553ee3205c..aa3dee0dc2fcb 100644 > --- a/drivers/dma/xilinx/xilinx_dma.c > +++ b/drivers/dma/xilinx/xilinx_dma.c [ ... ] > @@ -1693,7 +1693,8 @@ static void xilinx_mcdma_start_transfer(struct xilinx_dma_chan *chan) > reg |= XILINX_MCDMA_CR_RUNSTOP_MASK; > dma_ctrl_write(chan, XILINX_MCDMA_CHAN_CR_OFFSET(chan->tdest), reg); > > - xilinx_dma_start(chan); > + if (chan->idle) > + xilinx_dma_start(chan); [Severity: High] This is a pre-existing issue, but does the lack of a controller-level lock cause concurrent read-modify-write operations on shared global MCDMA registers to race? In xilinx_mcdma_start_transfer(), just before this diff context, there is an unprotected read-modify-write on the shared XILINX_MCDMA_CHEN_OFFSET register: reg = dma_ctrl_read(chan, XILINX_MCDMA_CHEN_OFFSET); reg |= BIT(chan->tdest); dma_ctrl_write(chan, XILINX_MCDMA_CHEN_OFFSET, reg); Similarly, when xilinx_dma_start() is called here, it performs an unprotected read-modify-write on the shared XILINX_DMA_REG_DMACR register: dma_ctrl_set(chan, XILINX_DMA_REG_DMACR, XILINX_DMA_DMACR_RUNSTOP); These operations appear to be protected only by the per-channel spinlock (chan->lock). If two channels on the same controller execute their start transfer logic concurrently on different CPUs, can the read-modify-write operations race and lead to a channel's enable bit being silently overwritten and lost? -- Sashiko AI review · https://sashiko.dev/#/patchset/20260620203417.4000360-1-suraj.gupta2@amd.com?part=3 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-06-20 20:47 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-06-20 20:34 [PATCH RESEND 0/3] dmaengine: xilinx_dma: Fixes and optimizations for AXIDMA and MCDMA channel management Suraj Gupta 2026-06-20 20:34 ` [PATCH RESEND 1/3] dmaengine: xilinx_dma: Fix channel idle state management in AXIDMA and MCDMA interrupt handlers Suraj Gupta 2026-06-20 20:47 ` sashiko-bot 2026-06-20 20:34 ` [PATCH RESEND 2/3] dmaengine: xilinx_dma: Enable transfer chaining for AXIDMA and MCDMA by removing idle restriction Suraj Gupta 2026-06-20 20:47 ` sashiko-bot 2026-06-20 20:34 ` [PATCH RESEND 3/3] dmaengine: xilinx_dma: Optimize control register write and channel start logic for AXIDMA and MCDMA in corresponding start_transfer() Suraj Gupta 2026-06-20 20:46 ` sashiko-bot
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.