lkml.org 
[lkml]   [2010]   [Dec]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 06/13] DMAENGINE: driver for the ARM PL080/PL081 PrimeCells
On Tue, Dec 21, 2010 at 06:20:37PM +0000, Russell King - ARM Linux wrote:
> Having just looked at this while trying to undo the DMA API abuses
> in the PL011 UART driver, I'm getting rather frustrated with this
> code.
> What's wrong with the PL08x DMA engine driver? Well, in the PL011
> UART driver, you do this:
>
> +static void pl011_dma_tx_callback(void *data)
> +{
> ...
> + /* Refill the TX if the buffer is not empty */
> + if (!uart_circ_empty(xmit)) {
> + ret = pl011_dma_tx_refill(uap);
> ...
> +static int pl011_dma_tx_refill(struct uart_amba_port *uap)
> +{
> ...
> + /* Prepare the scatterlist */
> + desc = chan->device->device_prep_slave_sg(chan,
> + &dmatx->scatter,
> + 1,
> + DMA_TO_DEVICE,
> + DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> ...
> + /* Some data to go along to the callback */
> + desc->callback = pl011_dma_tx_callback;
> + desc->callback_param = uap;
>
> Note that this calls the channel device_prep_slave_sg() from the
> callback. This seems reasonable.
>
> Right, now let's look at this driver (from the latest kernel):
>
> static void pl08x_tasklet(unsigned long data)
> {
> ...
> spin_lock(&plchan->lock);
> ...
> dma_async_tx_callback callback =
> plchan->at->tx.callback;
> void *callback_param =
> plchan->at->tx.callback_param;
> ...
> /*
> * Callback to signal completion
> */
> if (callback)
> callback(callback_param);
>
> Note that the callback is called with the channel lock held.
>
> struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
> struct dma_chan *chan, struct scatterlist *sgl,
> unsigned int sg_len, enum dma_data_direction direction,
> unsigned long flags)
> {
> ...
> ret = pl08x_prep_channel_resources(plchan, txd);
> if (ret)
> return NULL;
> /*
> * NB: the channel lock is held at this point so tx_submit()
> * must be called in direct succession.
> */
>
> XXXXXXXX DEADLOCK XXXXXXXX

Having found one of my ARM platforms which actually supports DMA (such
a rare thing - neither my Realview EB nor Versatile Express supports it),
I can finally see about run-time checking some of this.

BUG: spinlock lockup on CPU#0, sh/417, c1870a08
Backtrace:
[<c0038880>] (dump_backtrace+0x0/0x10c) from [<c02c20e0>] (dump_stack+0x18/0x1c) r7:c104e000 r6:c1870a08 r5:00000000 r4:00000000
[<c02c20c8>] (dump_stack+0x0/0x1c) from [<c017b520>] (do_raw_spin_lock+0x118/0x154)
[<c017b408>] (do_raw_spin_lock+0x0/0x154) from [<c02c4b98>] (_raw_spin_lock_irqsave+0x54/0x60)
[<c02c4b44>] (_raw_spin_lock_irqsave+0x0/0x60) from [<c01f5828>] (pl08x_prep_channel_resources+0x718/0x8b4)
r7:c1870a08 r6:00000001 r5:c19c3560 r4:ffd04010
[<c01f5110>] (pl08x_prep_channel_resources+0x0/0x8b4) from [<c01f5bb4>] (pl08x_prep_slave_sg+0x120/0x19c)
[<c01f5a94>] (pl08x_prep_slave_sg+0x0/0x19c) from [<c01be7a0>] (pl011_dma_tx_refill+0x164/0x224)
r8:00000020 r7:c03978c0 r6:c1850000 r5:c1847e00 r4:c1847f40
[<c01be63c>] (pl011_dma_tx_refill+0x0/0x224) from [<c01bf1c8>] (pl011_dma_tx_callback+0x7c/0xc4)
[<c01bf14c>] (pl011_dma_tx_callback+0x0/0xc4) from [<c01f4d34>] (pl08x_tasklet+0x60/0x368)
r5:c18709a0 r4:00000000
[<c01f4cd4>] (pl08x_tasklet+0x0/0x368) from [<c004d978>] (tasklet_action+0xa0/0x100)
[<c004d8d8>] (tasklet_action+0x0/0x100) from [<c004e01c>] (__do_softirq+0xa0/0x170)
r8:00000006 r7:00000001 r6:00000018 r5:c104e000 r4:00000100
[<c004df7c>] (__do_softirq+0x0/0x170) from [<c004e140>] (irq_exit+0x54/0x9c)
[<c004e0ec>] (irq_exit+0x0/0x9c) from [<c0029080>] (asm_do_IRQ+0x80/0xa0)
[<c0029000>] (asm_do_IRQ+0x0/0xa0) from [<c00345b8>] (__irq_svc+0x38/0xa0)
As described above in my code analysis, pl08x_tasklet takes the spinlock,
calls pl011_dma_tx_callback and eventually back to pl08x_prep_slave_sg
and pl08x_prep_channel_resources which then try to take the spinlock
again, leading to deadlock.


\
 
 \ /
  Last update: 2010-12-22 13:31    [W:1.721 / U:0.232 seconds]
©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site