[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <TYAPR06MB2239EF3AECFB14AA7AB3888E8C949@TYAPR06MB2239.apcprd06.prod.outlook.com>
Date: Sun, 9 Apr 2023 15:47:15 +0000
From: Libing Lei <libing.lei@...uarmicro.com>
To: Chen Yu <yu.c.chen@...el.com>
CC: "Eugeniy.Paltsev@...opsys.com" <Eugeniy.Paltsev@...opsys.com>,
"vkoul@...nel.org" <vkoul@...nel.org>,
"dmaengine@...r.kernel.org" <dmaengine@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH V3] dmaengine: dw-axi-dmac: add spinlock for multi-channel
DMA
On 2023-04-07 at 14:48:56 +0800, Chen Yu <yu.c.chen@...el.com> wrote:
> On 2023-04-04 at 23:39:45 +0800, Libing Lei wrote:
> > When multiple channels operate simultaneously, the common register shared
> > by all channels could be set to an unexpected value due to race conditions
> Does the channel mean the "chip" in the following patch? I mean, each channel
> has 1 chip plugged in? I asked this question because I'm trying to figure out the
> protection scope of the introduced spinlock.
No,chip doesn't means "channel" but the dmac chip,which points to some
common registers for all channels.
> > on SMP machines, resulting in a disaster that DMAC cannot work from then
> > on:
> >
> > cpu0 cpu1
> > dw_axi_dma_interrupt()
> > axi_dma_irq_disable(chip)
> > ...
> > axi_chan_block_xfer_start()
> > axi_dma_enable()
> > val = axi_dma_ioread32(chip, DMAC_CFG)
> > axi_dma_irq_enable(chip)
> > axi_dma_iowrite32(chip, DMAC_CFG, val)
> >
> > As a result, the global interrupt enable bit INT_EN in the DMAC_CFG
> > register is eventually cleared and the DMAC will no longer generates
> > interrupts.
> >
> > The error scenario is as follows:
> >
> > [ 63.483688] dmatest: dma0chan1-copy: result #18: 'test timed out' with
> > src_off=0xc2 dst_off=0x27b len=0x3a54 (0)
> > [ 63.483693] dmatest: dma0chan2-copy: result #18: 'test timed out' with
> > src_off=0x239 dst_off=0xfc9 len=0x213a (0)
> > [ 63.483696] dmatest: dma0chan0-copy: result #19: 'test timed out' with
> > src_off=0x5d1 dst_off=0x231 len=0x395e (0)
> >
> > a spinlock is added to fix it up.
> >
> Although it is unlikely to introduce ABBA lock, maybe enabling the
> CONFIG_LOCKDEP to have a double check on this.
Thanks for your good suggestion!I have enabled the CONFIG_LOCKDEP in my
testcase:
/ # ls /proc/lockdep*
/proc/lockdep /proc/lockdep_chains /proc/lockdep_stats
> > Signed-off-by: Libing Lei <libing.lei@...uarmicro.com>
> The patch looks good to me.
>
> thanks,
> Chenyu
>
It's my pleasure.
Best Regards,
Libing.Lei
Powered by blists - more mailing lists