linux-kernel - Re: [PATCH 06/13] DMAENGINE: driver for the ARM PL080/PL081 PrimeCells

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 23 Dec 2010 12:30:28 +0000
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Linus Walleij <linus.ml.walleij@...il.com>
Cc:	Dan Williams <dan.j.williams@...el.com>,
	Viresh Kumar <viresh.kumar@...com>,
	Kukjin Kim <kgene.kim@...sung.com>, yuanyabin1978@...a.com,
	linux-kernel@...r.kernel.org, Ben Dooks <ben-linux@...ff.org>,
	Peter Pearse <peter.pearse@....com>,
	linux-arm-kernel@...ts.infradead.org,
	Alessandro Rubini <rubini@...pv.it>
Subject: Re: [PATCH 06/13] DMAENGINE: driver for the ARM PL080/PL081
	PrimeCells

On Thu, Dec 23, 2010 at 09:17:07AM +0100, Linus Walleij wrote:
> 2010/12/23 Dan Williams <dan.j.williams@...el.com>:
> > 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.

Is it actually safe to do this?  The answer seems to be no - if we happen
to terminate all transfers (as your PL011 uart code does) when we fail to
setup a new DMA transaction, then bad stuff happens due to this:

                /*
                 * Device callbacks should NOT clear
                 * the current transaction on the channel
                 * Linus: sometimes they should?
                 */
                if (!plchan->at)
                        BUG();

We really need a saner approach here - maybe the list approach described
by Jassie.

> 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);

Plus, of course, that tasklets run with IRQs enabled.  This means we're
taking this spinlock in an interruptible context.  If we have some other
path which also takes this lock from an IRQ context, then we're asking
for deadlock.  See my previous mails on this subject.

I'm currently splitting my dirty patch, and attacking this driver to
clean up some of it into a more reasonable shape, so this is one area
which I'm going to be sorting out.

Patches later.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/