Update the the dmatest driver so that it handles duplicate DMA channels
properly.

When a DMA client is notified of an available DMA channel, it must check if it
has already allocated resources for that channel.  If so, it should return
DMA_DUP.  This can happen, for example, if a DMA driver calls
dma_async_device_register() more than once.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 drivers/dma/dmatest.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index a08d197..2689d90 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -321,10 +321,15 @@ static void dmatest_cleanup_channel(struct dmatest_chan *dtc)
 
 static enum dma_state_client dmatest_add_channel(struct dma_chan *chan)
 {
-	struct dmatest_chan	*dtc;
+	struct dmatest_chan	*dtc, *_dtc;
 	struct dmatest_thread	*thread;
 	unsigned int		i;
 
+	/* Have we already been told about this channel? */
+	list_for_each_entry_safe(dtc, _dtc, &dmatest_channels, node)
+		if (dtc->chan == chan)
+			return DMA_DUP;
+
 	dtc = kmalloc(sizeof(struct dmatest_chan), GFP_ATOMIC);
 	if (!dtc) {
 		pr_warning("dmatest: No memory for %s\n", chan->dev.bus_id);
-- 
1.5.5

--

[ message continues ]

Acked-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>

...though I don't really see why you have to use
list_for_entry_safe()...

Thanks a lot!

Haavard
--

[ message continues ]

You use it in dmatest_remove_channel(), so I figured I'd do the same thing.


-- 
Timur Tabi
Linux kernel developer at Freescale
--

[ message continues ]

dmatest_remove_channel() may call list_del() on the current entry, so it
has to. As long as you're not altering the list, the _safe() variant
shouldn't be necessary.

Haavard
--

[ message continues ]

On Thu, 18 Sep 2008 10:21:19 -0500

hm.  A few lines after that GFP_ATOMIC the driver does a GFP_KERNEL
allocation.

One of them is incorrect.  The interface is undocumented (natch), but I
assume that GFP_KERNEL is the one to use here.

--- a/drivers/dma/dmatest.c~a
+++ a/drivers/dma/dmatest.c
@@ -330,7 +330,7 @@ static enum dma_state_client dmatest_add
 		if (dtc->chan == chan)
 			return DMA_DUP;
 
-	dtc = kmalloc(sizeof(struct dmatest_chan), GFP_ATOMIC);
+	dtc = kmalloc(sizeof(struct dmatest_chan), GFP_KERNEL);
 	if (!dtc) {
 		pr_warning("dmatest: No memory for %s\n", chan->dev.bus_id);
 		return DMA_NAK;
_

--

[ message continues ]

You're right -- there's no reason to use GFP_ATOMIC there. I messed
around with various kinds of locks before I realized that it was
already protected by a mutex.

Haavard
--

[ message continues ]

On Fri, Sep 19, 2008 at 4:31 PM, Andrew Morton

The interface is documented, although not the locking (natch), at
include/linux/dmaengine.h:229.  When I fix this up is there a
canonical location to document a callback interface rather than at the
callback's typedef?

Thanks,
Dan
--

[ message continues ]

Personally I like to see at at the definition site, so that's within
the struct which contains the function pointer.  eg:

struct address_space_operations {
	int (*writepage)(struct page *page, struct writeback_control *wbc);
	int (*readpage)(struct file *, struct page *);
	void (*sync_page)(struct page *);

	/* Write back some dirty pages from this mapping. */
	int (*writepages)(struct address_space *, struct writeback_control *);

	/* Set a page dirty.  Return true if this dirtied it */
	int (*set_page_dirty)(struct page *page);

(sadly incomplete, but we tried)
--

[ message continues ]