* Re: [PATCH] dmaengine: xilinx_dma: Use readl_poll_timeout() instead of atomic variant
2026-08-03 13:20 ` [PATCH] dmaengine: xilinx_dma: Use readl_poll_timeout() instead of atomic variant Tomi Valkeinen
@ 2026-08-03 13:41 ` sashiko-bot
2026-08-03 14:39 ` Tomi Valkeinen
2026-08-04 7:12 ` Alex Bereza
2 siblings, 0 replies; 5+ messages in thread
From: sashiko-bot @ 2026-08-03 13:41 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: vkoul, Frank.Li, dmaengine
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch replaces `readl_poll_timeout_atomic()` with `readl_poll_timeout()` in an atomic context, violating the `iopoll.h` API contract and introducing a risk of infinite loops and severe latency.
--
--- Patch [1]: [PATCH] dmaengine: xilinx_dma: Use readl_poll_timeout() instead of atomic variant ---
commit f549bf1ac3f30db8e280af9c3f9f7f1f4d4ac1b5
Author: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
dmaengine: xilinx_dma: Use readl_poll_timeout() instead of atomic variant
This commit switches xilinx_dma_poll_timeout() from the atomic to the
regular readl_poll_timeout() macro. The commit message states that this
is safe because zero delay means it never sleeps.
> Calling xilinx_dma_poll_timeout() from atomic context is still safe even
> after this change, as readl_poll_timeout() with zero delay never sleeps.
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -530,7 +530,7 @@ struct xilinx_dma_device {
> #define to_dma_tx_descriptor(tx) \
> container_of(tx, struct xilinx_dma_tx_descriptor, async_tx)
> #define xilinx_dma_poll_timeout(chan, reg, val, cond, delay_us, timeout_us) \
> - readl_poll_timeout_atomic(chan->xdev->regs + chan->ctrl_offset + reg, \
> + readl_poll_timeout(chan->xdev->regs + chan->ctrl_offset + reg, \
> val, cond, delay_us, timeout_us)
[Severity: High]
Is the assertion in the commit message accurate for all hardware
configurations, or could this regression lead to a hard lockup during DMA
transfers or error resets?
While it is true that readl_poll_timeout() does not sleep with a zero delay,
it still relies on ktime_get() for timekeeping. The API documentation in
iopoll.h explicitly forbids calling readl_poll_timeout() from atomic context
if timeout_us is used.
This macro is called with interrupts disabled in multiple paths. For example,
xilinx_dma_issue_pending() calls start_transfer() under spin_lock_irqsave(),
and the irq handler also calls it under a spinlock:
drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_irq_handler() {
...
spin_lock(&chan->lock);
xilinx_dma_complete_descriptor(chan);
chan->idle = true;
chan->start_transfer(chan);
...
}
If ktime_get() relies on timer interrupts to advance the clock on certain
hardware configurations, and those interrupts are disabled, would this cause
the polling loop to hang indefinitely if the hardware condition is never met?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-xilinx-dma-poll-fix-v1-1-892302c4847d@ideasonboard.com?part=1
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] dmaengine: xilinx_dma: Use readl_poll_timeout() instead of atomic variant
2026-08-03 13:20 ` [PATCH] dmaengine: xilinx_dma: Use readl_poll_timeout() instead of atomic variant Tomi Valkeinen
2026-08-03 13:41 ` sashiko-bot
@ 2026-08-03 14:39 ` Tomi Valkeinen
2026-08-04 7:12 ` Alex Bereza
2 siblings, 0 replies; 5+ messages in thread
From: Tomi Valkeinen @ 2026-08-03 14:39 UTC (permalink / raw)
To: Vinod Koul, Frank Li, Michal Simek
Cc: dmaengine, linux-arm-kernel, linux-kernel, Vishal Sagar
Hi,
On 03/08/2026 16:20, Tomi Valkeinen wrote:
> All callers of xilinx_dma_poll_timeout() pass a zero delay and a 1
> second timeout (XILINX_DMA_LOOP_COUNT). With a zero delay,
> readl_poll_timeout_atomic() does not measure wall clock time, and the
> actual timeout can be much more than 1 second.
>
> This is very visible when a channel reset never completes, e.g. with a
> VDMA whose stream-side clock isn't running. Probe appears to hang, and
> only minutes later fails with:
>
> xilinx-vdma a0050000.dma: reset timeout, cr 4, sr 0
> xilinx-vdma a0050000.dma: Reset channel failed
>
> Switch to readl_poll_timeout(), which checks the deadline against
> ktime_get().
>
> Calling xilinx_dma_poll_timeout() from atomic context is still safe even
> after this change, as readl_poll_timeout() with zero delay never sleeps.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
> drivers/dma/xilinx/xilinx_dma.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index 404235c17353..6e8c786147f7 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -530,7 +530,7 @@ struct xilinx_dma_device {
> #define to_dma_tx_descriptor(tx) \
> container_of(tx, struct xilinx_dma_tx_descriptor, async_tx)
> #define xilinx_dma_poll_timeout(chan, reg, val, cond, delay_us, timeout_us) \
> - readl_poll_timeout_atomic(chan->xdev->regs + chan->ctrl_offset + reg, \
> + readl_poll_timeout(chan->xdev->regs + chan->ctrl_offset + reg, \
> val, cond, delay_us, timeout_us)
As commented by Sashiko, readl_poll_timeout API doc says "Must not be
called from atomic context if sleep_us or timeout_us are used".
Maybe a better fix is to continue using readl_poll_timeout_atomic, but
use a small (non-zero) delay_us, and decrease the timeout_us. Using a
one second timeout when calling from atomic context doesn't sound very
good in any case.
Tomi
^ permalink raw reply [flat|nested] 5+ messages in thread