lkml.org 
[lkml]   [2010]   [Dec]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 06/13] DMAENGINE: driver for the ARM PL080/PL081 PrimeCells
    From
    On Thu, Dec 23, 2010 at 5:17 PM, Linus Walleij
    <linus.ml.walleij@gmail.com> wrote:
    > 2010/12/23 Dan Williams <dan.j.williams@intel.com>:
    >> On Wed, Dec 22, 2010 at 4:29 AM, Russell King - ARM Linux
    >> <linux@arm.linux.org.uk> wrote:
    >>> 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.
    >>
    >> This is listed in the dmaengine documentation [1], but I obviously
    >> missed this before merging.  This also would have been caught by
    >> lockdep as required by SubmitChecklist.
    >
    > Yeah, my bad. I'll get better at this... :-(
    > (I blame it partially on inaccessible hardware, sob sob. I do like to
    > run lockdep.)
    >
    >> It looks like this driver needs a full scrub
    >> which seems unreasonable to complete and test over the holidays before
    >> .37 lands.  Linus we either need to mark this "depends on BROKEN" or
    >> revert it.
    >
    > Isn't it really as simple as to release the spinlock during callbacks?
    > That lock is only intended to protect the plchan variables, not to block
    > anyone from queueing new stuff during the callback (as happens now).
    >
    > It can release that lock, make a callback where a new descriptor
    > gets queued, and then take it again and start looking at the queue,
    > at which point it discovers the new desc and process it.
    >
    > So something like this:
    >
    >
    > From: Linus Walleij <linus.walleij@stericsson.com>
    > Date: Thu, 23 Dec 2010 09:06:14 +0100
    > Subject: [PATCH] dma: release pl08x channel lock during callback
    >
    > The spinlock is not really safeguarding any resources during the
    > callback, so let's release it before that and take it back
    > afterwards so as to avoid deadlocks.
    >
    > Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
    > ---
    >  drivers/dma/amba-pl08x.c |    7 +++++--
    >  1 files changed, 5 insertions(+), 2 deletions(-)
    >
    > diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
    > index b605cc9..7879a22 100644
    > --- a/drivers/dma/amba-pl08x.c
    > +++ b/drivers/dma/amba-pl08x.c
    > @@ -1651,8 +1651,11 @@ static void pl08x_tasklet(unsigned long data)
    >                /*
    >                 * Callback to signal completion
    >                 */
    > -               if (callback)
    > -                       callback(callback_param);
    > +               if (callback) {
    > +                        spin_unlock(&plchan->lock);
    > +                        callback(callback_param);
    > +                        spin_lock(&plchan->lock);
    > +                }
    How about adding completed requests to a list and go on to do important
    channel management stuff, and do callbacks at the end after dropping the lock.
    As in pl330_tasklet of drivers/dma/pl330.c
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2010-12-23 09:33    [W:8.465 / U:0.052 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site