* [PATCH V3] dmaengine: dw-axi-dmac: add spinlock for multi-channel DMA
@ 2023-04-04 15:39 Libing Lei
2023-04-07 6:48 ` Chen Yu
0 siblings, 1 reply; 3+ messages in thread
From: Libing Lei @ 2023-04-04 15:39 UTC (permalink / raw)
To: Eugeniy.Paltsev, vkoul; +Cc: dmaengine, linux-kernel, yu.c.chen, Libing Lei
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(+)
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 */
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH V3] dmaengine: dw-axi-dmac: add spinlock for multi-channel DMA
2023-04-04 15:39 [PATCH V3] dmaengine: dw-axi-dmac: add spinlock for multi-channel DMA Libing Lei
@ 2023-04-07 6:48 ` Chen Yu
2023-04-09 15:47 ` Libing Lei
0 siblings, 1 reply; 3+ messages in thread
From: Chen Yu @ 2023-04-07 6:48 UTC (permalink / raw)
To: Libing Lei; +Cc: Eugeniy.Paltsev, vkoul, dmaengine, linux-kernel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH V3] dmaengine: dw-axi-dmac: add spinlock for multi-channel DMA
2023-04-07 6:48 ` Chen Yu
@ 2023-04-09 15:47 ` Libing Lei
0 siblings, 0 replies; 3+ messages in thread
From: Libing Lei @ 2023-04-09 15:47 UTC (permalink / raw)
To: Chen Yu; +Cc: Eugeniy.Paltsev, vkoul, dmaengine, linux-kernel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-04-09 15:47 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-04 15:39 [PATCH V3] dmaengine: dw-axi-dmac: add spinlock for multi-channel DMA Libing Lei
2023-04-07 6:48 ` Chen Yu
2023-04-09 15:47 ` Libing Lei
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.