| Message ID | 20230404153945.916-1-libing.lei@jaguarmicro.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [V3] dmaengine: dw-axi-dmac: add spinlock for multi-channel DMA | expand |
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. > 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. > Signed-off-by: Libing Lei <libing.lei@jaguarmicro.com> The patch looks good to me. thanks, Chenyu
On 2023-04-07 at 14:48:56 +0800, Chen Yu <yu.c.chen@intel.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@jaguarmicro.com> > The patch looks good to me. > > thanks, > Chenyu > It's my pleasure. Best Regards, Libing.Lei
diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c index bf85aa097..7aa53293c 100644 --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c @@ -108,37 +108,49 @@ static inline void axi_chan_config_write(struct axi_dma_chan *chan, static inline void axi_dma_disable(struct axi_dma_chip *chip) { u32 val; + unsigned long flags; + spin_lock_irqsave(&chip->lock, flags); val = axi_dma_ioread32(chip, DMAC_CFG); val &= ~DMAC_EN_MASK; axi_dma_iowrite32(chip, DMAC_CFG, val); + spin_unlock_irqrestore(&chip->lock, flags); } static inline void axi_dma_enable(struct axi_dma_chip *chip) { u32 val; + unsigned long flags; + spin_lock_irqsave(&chip->lock, flags); val = axi_dma_ioread32(chip, DMAC_CFG); val |= DMAC_EN_MASK; axi_dma_iowrite32(chip, DMAC_CFG, val); + spin_unlock_irqrestore(&chip->lock, flags); } static inline void axi_dma_irq_disable(struct axi_dma_chip *chip) { u32 val; + unsigned long flags; + spin_lock_irqsave(&chip->lock, flags); val = axi_dma_ioread32(chip, DMAC_CFG); val &= ~INT_EN_MASK; axi_dma_iowrite32(chip, DMAC_CFG, val); + spin_unlock_irqrestore(&chip->lock, flags); } static inline void axi_dma_irq_enable(struct axi_dma_chip *chip) { u32 val; + unsigned long flags; + spin_lock_irqsave(&chip->lock, flags); val = axi_dma_ioread32(chip, DMAC_CFG); val |= INT_EN_MASK; axi_dma_iowrite32(chip, DMAC_CFG, val); + spin_unlock_irqrestore(&chip->lock, flags); } static inline void axi_chan_irq_disable(struct axi_dma_chan *chan, u32 irq_mask) @@ -177,7 +189,9 @@ static inline u32 axi_chan_irq_read(struct axi_dma_chan *chan) static inline void axi_chan_disable(struct axi_dma_chan *chan) { u32 val; + unsigned long flags; + spin_lock_irqsave(&chan->chip->lock, flags); val = axi_dma_ioread32(chan->chip, DMAC_CHEN); val &= ~(BIT(chan->id) << DMAC_CHAN_EN_SHIFT); if (chan->chip->dw->hdata->reg_map_8_channels) @@ -185,12 +199,15 @@ static inline void axi_chan_disable(struct axi_dma_chan *chan) else val |= BIT(chan->id) << DMAC_CHAN_EN2_WE_SHIFT; axi_dma_iowrite32(chan->chip, DMAC_CHEN, val); + spin_unlock_irqrestore(&chan->chip->lock, flags); } static inline void axi_chan_enable(struct axi_dma_chan *chan) { u32 val; + unsigned long flags; + spin_lock_irqsave(&chan->chip->lock, flags); val = axi_dma_ioread32(chan->chip, DMAC_CHEN); if (chan->chip->dw->hdata->reg_map_8_channels) val |= BIT(chan->id) << DMAC_CHAN_EN_SHIFT | @@ -199,6 +216,7 @@ static inline void axi_chan_enable(struct axi_dma_chan *chan) val |= BIT(chan->id) << DMAC_CHAN_EN_SHIFT | BIT(chan->id) << DMAC_CHAN_EN2_WE_SHIFT; axi_dma_iowrite32(chan->chip, DMAC_CHEN, val); + spin_unlock_irqrestore(&chan->chip->lock, flags); } static inline bool axi_chan_is_hw_enable(struct axi_dma_chan *chan) @@ -1420,6 +1438,8 @@ static int dw_probe(struct platform_device *pdev) if (ret) return ret; + spin_lock_init(&chip->lock); + dw->chan = devm_kcalloc(chip->dev, hdata->nr_channels, sizeof(*dw->chan), GFP_KERNEL); if (!dw->chan) diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac.h b/drivers/dma/dw-axi-dmac/dw-axi-dmac.h index e9d5eb0fd..4a8db437e 100644 --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac.h +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac.h @@ -70,6 +70,7 @@ struct axi_dma_chip { struct clk *core_clk; struct clk *cfgr_clk; struct dw_axi_dma *dw; + spinlock_t lock; }; /* LLI == Linked List Item */
When multiple channels operate simultaneously, the common register shared by all channels could be set to an unexpected value due to race conditions 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. Signed-off-by: Libing Lei <libing.lei@jaguarmicro.com> --- Changes since v2: - fix commit message Changes since v1: - an example and more description for this patch --- .../dma/dw-axi-dmac/dw-axi-dmac-platform.c | 20 +++++++++++++++++++ drivers/dma/dw-axi-dmac/dw-axi-dmac.h | 1 + 2 files changed, 21 insertions(+)