| Message ID | 1370724988-16426-1-git-send-email-shc_work@mail.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Alexander, On Sat, Jun 8, 2013 at 5:56 PM, Alexander Shiyan <shc_work@mail.ru> wrote: > ================================= > [ INFO: inconsistent lock state ] > 3.10.0-rc4-next-20130607-00006-gf5bbfe3-dirty #59 Not tainted > --------------------------------- > inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage. > swapper/1 [HC0[0]:SC1[1]:HE1:SE0] takes: > (&(&imxdma->lock)->rlock){?.-...}, at: [<c0230200>] imxdma_tasklet+0x1c/0x138 > {IN-HARDIRQ-W} state was registered at: I think the proper fix is to replace spin_lock/spin_unlock with spin_lock_bh/spin_unlock_bh inside imxdma_tasklet.
On Sun, Jun 09, 2013 at 12:56:28AM +0400, Alexander Shiyan wrote: > @@ -625,8 +613,9 @@ static void imxdma_tasklet(unsigned long data) > struct imxdma_channel *imxdmac = (void *)data; > struct imxdma_engine *imxdma = imxdmac->imxdma; > struct imxdma_desc *desc; > + unsigned long flags; > > - spin_lock(&imxdma->lock); > + spin_lock_irqsave(&imxdma->lock, flags); > > if (list_empty(&imxdmac->ld_active)) { > /* Someone might have called terminate all */ > @@ -663,7 +652,7 @@ static void imxdma_tasklet(unsigned long data) > __func__, imxdmac->channel); > } > out: > - spin_unlock(&imxdma->lock); > + spin_unlock_irqrestore(&imxdma->lock, flags); So, I just peeked at this driver. Who is reviewing DMA engine drivers and why aren't they reviewing stuff properly. static void imxdma_tasklet(unsigned long data) { ... spin_lock(&imxdma->lock); ... if (desc->desc.callback) desc->desc.callback(desc->desc.callback_param); ... out: spin_unlock(&imxdma->lock); } This stuff is well known and even documented: For slave DMA, the subsequent transaction may not be available for submission prior to callback function being invoked, so slave DMA callbacks are permitted to prepare and submit a new transaction. For cyclic DMA, a callback function may wish to terminate the DMA via dmaengine_terminate_all(). Therefore, it is important that DMA engine drivers drop any locks before calling the callback function which may cause a deadlock. Note that callbacks will always be invoked from the DMA engines tasklet, never from interrupt context. Note the 3rd paragraph - and note that the IMX driver violates this by holding that spinlock. Changing that spinlock to be an irq-saving type just makes the problem worse. What's more is it takes this same lock in the submit and issue_pending functions - so this is potential deadlock territory by the mere fact that a callback function _can_ invoke these two functions. It also makes use of __memzero. Don't. Use memset(). Doesn't check the return value of clk_prepare_enable(). Mixes up accessors and direct accesses: imxdmac->sg_list[i].dma_address = dma_addr; sg_dma_len(&imxdmac->sg_list[i]) = period_len; The first should be accessed via sg_dma_address(). Reimplements much of virt-dma.c/.h - maybe it should use this support instead? As an added benefit you'll be able to start the next queued request without the tasklet and get better throughput as a result - and process all completed transfers upon tasklet invocation.
On Sat, Jun 08, 2013 at 06:07:36PM -0300, Fabio Estevam wrote: > Hi Alexander, > > On Sat, Jun 8, 2013 at 5:56 PM, Alexander Shiyan <shc_work@mail.ru> wrote: > > ================================= > > [ INFO: inconsistent lock state ] > > 3.10.0-rc4-next-20130607-00006-gf5bbfe3-dirty #59 Not tainted > > --------------------------------- > > inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage. > > swapper/1 [HC0[0]:SC1[1]:HE1:SE0] takes: > > (&(&imxdma->lock)->rlock){?.-...}, at: [<c0230200>] imxdma_tasklet+0x1c/0x138 > > {IN-HARDIRQ-W} state was registered at: > > I think the proper fix is to replace spin_lock/spin_unlock with > spin_lock_bh/spin_unlock_bh inside imxdma_tasklet. No, _bh versions are the tasklet-disabling versions. You're already inside tasklet context inside the tasklet itself. What it's complaining about is that the tasklet takes a non-IRQ safe lock, which _is_ taken in IRQ context. So, what can happen is this: - tasklet executes - tasklet takes the lock - interrupt occurs - interrupt handler takes the same lock *deadlock* but as I've just pointed out, there's a number of things wrong with this driver, including one serious problem with the way the tasklet is written, and merely changing the lock type doesn't fix it. It needs a redesign.
On Sat, Jun 08, 2013 at 10:15:57PM +0100, Russell King - ARM Linux wrote: > On Sun, Jun 09, 2013 at 12:56:28AM +0400, Alexander Shiyan wrote: > > @@ -625,8 +613,9 @@ static void imxdma_tasklet(unsigned long data) > > struct imxdma_channel *imxdmac = (void *)data; > > struct imxdma_engine *imxdma = imxdmac->imxdma; > > struct imxdma_desc *desc; > > + unsigned long flags; > > > > - spin_lock(&imxdma->lock); > > + spin_lock_irqsave(&imxdma->lock, flags); > > > > if (list_empty(&imxdmac->ld_active)) { > > /* Someone might have called terminate all */ > > @@ -663,7 +652,7 @@ static void imxdma_tasklet(unsigned long data) > > __func__, imxdmac->channel); > > } > > out: > > - spin_unlock(&imxdma->lock); > > + spin_unlock_irqrestore(&imxdma->lock, flags); > > So, I just peeked at this driver. Who is reviewing DMA engine drivers > and why aren't they reviewing stuff properly. mea culpa, yes its documeneted well. Let me fix it up... > > static void imxdma_tasklet(unsigned long data) > { > ... > spin_lock(&imxdma->lock); > ... > if (desc->desc.callback) > desc->desc.callback(desc->desc.callback_param); > ... > out: > spin_unlock(&imxdma->lock); > } > > This stuff is well known and even documented: > > For slave DMA, the subsequent transaction may not be available > for submission prior to callback function being invoked, so > slave DMA callbacks are permitted to prepare and submit a new > transaction. > > For cyclic DMA, a callback function may wish to terminate the > DMA via dmaengine_terminate_all(). > > Therefore, it is important that DMA engine drivers drop any > locks before calling the callback function which may cause a > deadlock. > > Note that callbacks will always be invoked from the DMA > engines tasklet, never from interrupt context. > > Note the 3rd paragraph - and note that the IMX driver violates this > by holding that spinlock. Changing that spinlock to be an irq-saving > type just makes the problem worse. > > What's more is it takes this same lock in the submit and issue_pending > functions - so this is potential deadlock territory by the mere fact > that a callback function _can_ invoke these two functions. > > It also makes use of __memzero. Don't. Use memset(). > > Doesn't check the return value of clk_prepare_enable(). > > Mixes up accessors and direct accesses: > imxdmac->sg_list[i].dma_address = dma_addr; > sg_dma_len(&imxdmac->sg_list[i]) = period_len; > The first should be accessed via sg_dma_address(). > > Reimplements much of virt-dma.c/.h - maybe it should use this support > instead? As an added benefit you'll be able to start the next queued > request without the tasklet and get better throughput as a result - > and process all completed transfers upon tasklet invocation. -- ~Vinod
Hi,
I'm the initiator of the bug-report[1] ("ARM: imx27: dmaengine:
imx-dma: SD-Card: copy hangs forever") which led to this thread.
Are there any updates yet?
[1]: http://www.spinics.net/lists/arm-kernel/msg250758.html
Thanks
-- Christoph
On Mon, Jul 15, 2013 at 10:13:59AM +0200, Christoph Fritz wrote: > Hi, > > I'm the initiator of the bug-report[1] ("ARM: imx27: dmaengine: > imx-dma: SD-Card: copy hangs forever") which led to this thread. > > Are there any updates yet? I should have a fix for you pretty soon. Obviosuly it would be untested on my part as I lack the hardware ~Vinod
On Mon, 2013-07-15 at 15:31 +0530, Vinod Koul wrote: > On Mon, Jul 15, 2013 at 10:13:59AM +0200, Christoph Fritz wrote: > > Hi, > > > > I'm the initiator of the bug-report[1] ("ARM: imx27: dmaengine: > > imx-dma: SD-Card: copy hangs forever") which led to this thread. > > > > Are there any updates yet? > I should have a fix for you pretty soon. Obviosuly it would be untested on my > part as I lack the hardware No problem, I'll do that. Thanks -- Christoph
================================= [ INFO: inconsistent lock state ] 3.10.0-rc4-next-20130607-00006-gf5bbfe3-dirty #59 Not tainted --------------------------------- inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage. swapper/1 [HC0[0]:SC1[1]:HE1:SE0] takes: (&(&imxdma->lock)->rlock){?.-...}, at: [<c0230200>] imxdma_tasklet+0x1c/0x138 {IN-HARDIRQ-W} state was registered at: [<c004dbe8>] __lock_acquire+0xa74/0x1a64 [<c004f0a4>] lock_acquire+0x64/0x78 [<c0428d9c>] _raw_spin_lock+0x34/0x44 [<c0230584>] dma_irq_handler+0x7c/0x250 [<c005c520>] handle_irq_event_percpu+0x50/0x1c8 [<c005c6d4>] handle_irq_event+0x3c/0x5c [<c005e944>] handle_level_irq+0x8c/0xe8 [<c005bf34>] generic_handle_irq+0x20/0x30 [<c000958c>] handle_IRQ+0x30/0x84 [<c0008710>] avic_handle_irq+0x34/0x54 [<c000bb24>] __irq_svc+0x44/0x74 [<c01fbd78>] ida_get_new_above+0x74/0x1c4 [<c00f4084>] sysfs_new_dirent+0x60/0xf8 [<c00f454c>] create_dir+0x28/0xc0 [<c00f48e4>] sysfs_create_dir+0x90/0xf4 [<c01fc6cc>] kobject_add_internal+0x90/0x1d8 [<c01fc858>] kobject_init_and_add+0x44/0x6c [<c025c9cc>] bus_add_driver+0x74/0x230 [<c025e324>] driver_register+0x78/0x14c [<c054a7d4>] do_one_initcall+0x50/0x158 [<c054a9c4>] kernel_init_freeable+0xe8/0x1ac [<c041f6e0>] kernel_init+0x8/0xe4 [<c0008dc0>] ret_from_fork+0x14/0x34 irq event stamp: 232290 hardirqs last enabled at (232290): [<c001cc08>] tasklet_action+0x30/0xdc hardirqs last disabled at (232289): [<c001cbf0>] tasklet_action+0x18/0xdc softirqs last enabled at (232196): [<c001c548>] __do_softirq+0x174/0x1e0 softirqs last disabled at (232287): [<c001c9a0>] irq_exit+0xa0/0xdc other info that might help us debug this: Possible unsafe locking scenario: CPU0 ---- lock(&(&imxdma->lock)->rlock); <Interrupt> lock(&(&imxdma->lock)->rlock); *** DEADLOCK *** 1 lock held by swapper/1: #0: (sysfs_ino_lock){+.+...}, at: [<c00f4074>] sysfs_new_dirent+0x50/0xf8 stack backtrace: CPU: 0 PID: 1 Comm: swapper Not tainted 3.10.0-rc4-next-20130607-00006-gf5bbfe3-dirty #59 [<c000c5f0>] (unwind_backtrace+0x0/0xf0) from [<c000b028>] (show_stack+0x10/0x14) [<c000b028>] (show_stack+0x10/0x14) from [<c0422380>] (print_usage_bug.part.26+0x220/0x288) [<c0422380>] (print_usage_bug.part.26+0x220/0x288) from [<c004b814>] (mark_lock+0x288/0x668) [<c004b814>] (mark_lock+0x288/0x668) from [<c004d720>] (__lock_acquire+0x5ac/0x1a64) [<c004d720>] (__lock_acquire+0x5ac/0x1a64) from [<c004f0a4>] (lock_acquire+0x64/0x78) [<c004f0a4>] (lock_acquire+0x64/0x78) from [<c0428d9c>] (_raw_spin_lock+0x34/0x44) [<c0428d9c>] (_raw_spin_lock+0x34/0x44) from [<c0230200>] (imxdma_tasklet+0x1c/0x138) [<c0230200>] (imxdma_tasklet+0x1c/0x138) from [<c001cc50>] (tasklet_action+0x78/0xdc) [<c001cc50>] (tasklet_action+0x78/0xdc) from [<c001c4c0>] (__do_softirq+0xec/0x1e0) [<c001c4c0>] (__do_softirq+0xec/0x1e0) from [<c001c9a0>] (irq_exit+0xa0/0xdc) [<c001c9a0>] (irq_exit+0xa0/0xdc) from [<c0009590>] (handle_IRQ+0x34/0x84) [<c0009590>] (handle_IRQ+0x34/0x84) from [<c0008710>] (avic_handle_irq+0x34/0x54) [<c0008710>] (avic_handle_irq+0x34/0x54) from [<c000bb24>] (__irq_svc+0x44/0x74) Signed-off-by: Alexander Shiyan <shc_work@mail.ru> --- drivers/dma/imx-dma.c | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/drivers/dma/imx-dma.c b/drivers/dma/imx-dma.c index ff2aab9..65fe00a 100644 --- a/drivers/dma/imx-dma.c +++ b/drivers/dma/imx-dma.c @@ -318,12 +318,9 @@ static void imxdma_enable_hw(struct imxdma_desc *d) struct imxdma_channel *imxdmac = to_imxdma_chan(d->desc.chan); struct imxdma_engine *imxdma = imxdmac->imxdma; int channel = imxdmac->channel; - unsigned long flags; dev_dbg(imxdma->dev, "%s channel %d\n", __func__, channel); - local_irq_save(flags); - imx_dmav1_writel(imxdma, 1 << channel, DMA_DISR); imx_dmav1_writel(imxdma, imx_dmav1_readl(imxdma, DMA_DIMR) & ~(1 << channel), DMA_DIMR); @@ -342,27 +339,23 @@ static void imxdma_enable_hw(struct imxdma_desc *d) } } - local_irq_restore(flags); } static void imxdma_disable_hw(struct imxdma_channel *imxdmac) { struct imxdma_engine *imxdma = imxdmac->imxdma; int channel = imxdmac->channel; - unsigned long flags; dev_dbg(imxdma->dev, "%s channel %d\n", __func__, channel); if (imxdma_hw_chain(imxdmac)) del_timer(&imxdmac->watchdog); - local_irq_save(flags); imx_dmav1_writel(imxdma, imx_dmav1_readl(imxdma, DMA_DIMR) | (1 << channel), DMA_DIMR); imx_dmav1_writel(imxdma, imx_dmav1_readl(imxdma, DMA_CCR(channel)) & ~CCR_CEN, DMA_CCR(channel)); imx_dmav1_writel(imxdma, 1 << channel, DMA_DISR); - local_irq_restore(flags); } static void imxdma_watchdog(unsigned long data) @@ -519,7 +512,6 @@ static int imxdma_xfer_desc(struct imxdma_desc *d) { struct imxdma_channel *imxdmac = to_imxdma_chan(d->desc.chan); struct imxdma_engine *imxdma = imxdmac->imxdma; - unsigned long flags; int slot = -1; int i; @@ -527,7 +519,6 @@ static int imxdma_xfer_desc(struct imxdma_desc *d) switch (d->type) { case IMXDMA_DESC_INTERLEAVED: /* Try to get a free 2D slot */ - spin_lock_irqsave(&imxdma->lock, flags); for (i = 0; i < IMX_DMA_2D_SLOTS; i++) { if ((imxdma->slots_2d[i].count > 0) && ((imxdma->slots_2d[i].xsr != d->x) || @@ -537,10 +528,8 @@ static int imxdma_xfer_desc(struct imxdma_desc *d) slot = i; break; } - if (slot < 0) { - spin_unlock_irqrestore(&imxdma->lock, flags); + if (slot < 0) return -EBUSY; - } imxdma->slots_2d[slot].xsr = d->x; imxdma->slots_2d[slot].ysr = d->y; @@ -549,7 +538,6 @@ static int imxdma_xfer_desc(struct imxdma_desc *d) imxdmac->slot_2d = slot; imxdmac->enabled_2d = true; - spin_unlock_irqrestore(&imxdma->lock, flags); if (slot == IMX_DMA_2D_SLOT_A) { d->config_mem &= ~CCR_MSEL_B; @@ -625,8 +613,9 @@ static void imxdma_tasklet(unsigned long data) struct imxdma_channel *imxdmac = (void *)data; struct imxdma_engine *imxdma = imxdmac->imxdma; struct imxdma_desc *desc; + unsigned long flags; - spin_lock(&imxdma->lock); + spin_lock_irqsave(&imxdma->lock, flags); if (list_empty(&imxdmac->ld_active)) { /* Someone might have called terminate all */ @@ -663,7 +652,7 @@ static void imxdma_tasklet(unsigned long data) __func__, imxdmac->channel); } out: - spin_unlock(&imxdma->lock); + spin_unlock_irqrestore(&imxdma->lock, flags); } static int imxdma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, @@ -677,11 +666,12 @@ static int imxdma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, switch (cmd) { case DMA_TERMINATE_ALL: - imxdma_disable_hw(imxdmac); - spin_lock_irqsave(&imxdma->lock, flags); + + imxdma_disable_hw(imxdmac); list_splice_tail_init(&imxdmac->ld_active, &imxdmac->ld_free); list_splice_tail_init(&imxdmac->ld_queue, &imxdmac->ld_free); + spin_unlock_irqrestore(&imxdma->lock, flags); return 0; case DMA_SLAVE_CONFIG: