Re: [PATCH] dmaengine: xilinx_dma: Fix CPU stall in xilinx_dma_poll_timeout

["Gupta, Suraj" <suraj.gupta2@amd.com>]

On 4/1/2026 1:57 PM, Alex Bereza wrote:
> [You don't often get email from alex@bereza.email. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Wed Apr 1, 2026 at 7:23 AM CEST, Suraj Gupta wrote:
> 
>>> Rename XILINX_DMA_LOOP_COUNT to XILINX_DMA_POLL_TIMEOUT_US because the
>>> former is incorrect. It is a timeout value for polling various register
>>> bits in microseconds. It is not a loop count. Add a constant
>>> XILINX_DMA_POLL_DELAY_US for delay_us value.
>>
>> Please split this change in a new patch.
> 
> Ok, will send a v2.
> 
>>> Fixes: 7349a69cf312 ("iopoll: Do not use timekeeping in read_poll_timeout_atomic()")
>>
>> This patch doesn't fixes anything in iopoll, please use correct fixes tag.
> 
> Ok, but I'm not sure what would be the correct fixes tag then? I though I need to reference
> 7349a69cf312 in fixes tag because this is the actual change that surfaced the CPU stall issue that I
> want to fix in this driver. I'm fixing the call sites of xilinx_dma_poll_timeout but they were added
> in different commits. Should I add all of them? That would be the following then:
> 
> Fixes: 9495f2648287 ("dmaengine: xilinx_vdma: Use readl_poll_timeout instead of do while loop's")
> Fixes: 676f9c26c330 ("dmaengine: xilinx: fix device_terminate_all() callback for AXI CDMA")
> 
> Three call sites with delay_us=0 were first introduced by 9495f2648287, then 676f9c26c330 added the
> fourth call site when introducing xilinx_cdma_stop_transfer (probably copy paste from
> xilinx_dma_stop_transfer). Would adding these two fixes tags be correct?
> 
>>> Hi, in addition to this patch I also have a question: what is the point
>>> of atomically polling for the HALTED or IDLE bit in the stop_transfer
>>> functions? Does device_terminate_all really need to be callable from
>>> atomic context? If not, one could switch to polling non-atomically and
>>> avoid burning CPU cycles.
>>>
>>
>> dmaengine_terminate_async(), which directly calls device_terminate_all
>> can be called from atomic context.
> 
> Right, thanks! Just for my understanding: I still think there is potential for improvement, because
> from my understanding it would be beneficial to do the waiting for the bits in the status register
> and the freeing of descriptors in xilinx_dma_synchronize. Do I understand correctly that this is
> currently not possible due to how the DMA engine API is structured? To make this possible I think
> the deprecated dmaengine_terminate_all would have to be removed and all users of this API would have
> to be adapted accordingly, correct? So this would be a patch of much larger scope than xilinx_dma
> driver alone.

Yes, your understanding of xilinx_dma_synchronize()and proposed changes 
looks correct.

Regards,
Suraj