Hi Neel, Thank you for the patch. On Sat, Jan 22, 2022 at 05:44:07PM +0530, Neel Gandhi wrote: > Protected race condition of virtual DMA channel from channel queue transfer > via vchan spin lock from the caller of xilinx_dpdma_chan_queue_transfer This should explain what the race condition is, as it's not immediately apparent from the patch itself. You can write it as The vchan_next_desc() function, called from xilinx_dpdma_chan_queue_transfer(), must be called with virt_dma_chan.lock held. This isn't correctly handled in all code paths, resulting in a race condition between the .device_issue_pending() handler and the IRQ handler which causes DMA to randomly stop. Fix it by taking the lock around xilinx_dpdma_chan_queue_transfer() calls that are missing it. > Signed-off-by: Neel Gandhi <neel.gandhi@xxxxxxxxxx> > --- > drivers/dma/xilinx/xilinx_dpdma.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/dma/xilinx/xilinx_dpdma.c b/drivers/dma/xilinx/xilinx_dpdma.c > index b0f4948b00a5..7d77a8948e38 100644 > --- a/drivers/dma/xilinx/xilinx_dpdma.c > +++ b/drivers/dma/xilinx/xilinx_dpdma.c > @@ -1102,7 +1102,9 @@ static void xilinx_dpdma_chan_vsync_irq(struct xilinx_dpdma_chan *chan) Adding some context: if (chan->desc.active) vchan_cookie_complete(&chan->desc.active->vdesc); > chan->desc.active = pending; > chan->desc.pending = NULL; > > + spin_lock(&chan->vchan.lock); > xilinx_dpdma_chan_queue_transfer(chan); > + spin_unlock(&chan->vchan.lock); There seems to be one more race condition here. vchan_cookie_complete() is documented as requiring the vchan.lock being held too. Moving the spin_lock() call before that should be enough to fix it. It would probably be useful to revisit locking in this driver, but for now, with the issues pointed out here fixed, this patch should be good enough. Can you submit a v2 ? > > out: > spin_unlock_irqrestore(&chan->lock, flags); > @@ -1495,7 +1497,9 @@ static void xilinx_dpdma_chan_err_task(struct tasklet_struct *t) > XILINX_DPDMA_EINTR_CHAN_ERR_MASK << chan->id); > > spin_lock_irqsave(&chan->lock, flags); > + spin_lock(&chan->vchan.lock); > xilinx_dpdma_chan_queue_transfer(chan); > + spin_unlock(&chan->vchan.lock); > spin_unlock_irqrestore(&chan->lock, flags); > } > -- Regards, Laurent Pinchart
