linux-kernel - RE: [PATCH V3] dmaengine: dw-axi-dmac: add spinlock for multi-channel DMA

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

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