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 for Android: free password hash cracker in your pocket

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

Message-ID: <AANLkTinOMOkY6AUW8Z6FWMMWU9u0_sNjA2r_31u9-A8v@mail.gmail.com>
Date:	Thu, 23 Dec 2010 09:17:07 +0100
From:	Linus Walleij <linus.ml.walleij@...il.com>
To:	Dan Williams <dan.j.williams@...el.com>
Cc:	Russell King - ARM Linux <linux@....linux.org.uk>,
	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

2010/12/23 Dan Williams <dan.j.williams@...el.com>:
> On Wed, Dec 22, 2010 at 4:29 AM, Russell King - ARM Linux
> <linux@....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@...ricsson.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@...ricsson.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);
+                }

 		/*
 		 * Device callbacks should NOT clear
-- 
1.7.2.3

Yours,
Linus Walleij
--
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/