Thanks for the review.

On Fri, 2008-11-14 at 23:08 -0700, Andrew Morton wrote: 

quoted

+static struct module *dma_chan_to_owner(struct dma_chan *chan)
+{
+	return chan->device->dev->driver->owner;
+}

Has this all been tested with CONFIG_MODULES=n?

It works, the only thing that changes is that ->owner is always NULL. 

It looks like we have a lot of unneeded code if CONFIG_MODULES=n. 
However that might not be a case which is worth bothering about.

We still need all the other reference counting machinery to identify
busy channels.

quoted

+/**
+ * balance_ref_count - catch up the channel reference count
+ */
+static void balance_ref_count(struct dma_chan *chan)

Forgot to kerneldocument the argument.

yup

quoted

+{
+	struct module *owner = dma_chan_to_owner(chan);
+
+	while (chan->client_count < dmaengine_ref_count) {
+		__module_get(owner);
+		chan->client_count++;
+	}
+}

The locking for ->client_count is undocumented.

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index dce6d00..9396891 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c

@@ -129,6 +129,9 @@ static struct module *dma_chan_to_owner(struct dma_chan *chan)
 
 /**
  * balance_ref_count - catch up the channel reference count
+ * @chan - channel to balance ->client_count versus dmaengine_ref_count
+ *
+ * balance_ref_count must be called under dma_list_mutex
  */
 static void balance_ref_count(struct dma_chan *chan)
 {

quoted

+/**
+ * dma_chan_get - try to grab a dma channel's parent driver module
+ * @chan - channel to grab
+ */
+static int dma_chan_get(struct dma_chan *chan)
+{
+	int err = -ENODEV;
+	struct module *owner = dma_chan_to_owner(chan);
+
+	if (chan->client_count) {
+		__module_get(owner);
+		err = 0;
+	} else if (try_module_get(owner))
+		err = 0;

I wonder if try_module_get() could be used in both cases (migt not make
sense to do so though).

Yes, but I like how it documents the assumption that the backing module
has been referenced when client_count is non-zero.

quoted

+	if (err == 0)
+		chan->client_count++;

Locking for this?

@@ -146,6 +146,8 @@ static void balance_ref_count(struct dma_chan *chan)
 /**
  * dma_chan_get - try to grab a dma channel's parent driver module
  * @chan - channel to grab
+ *
+ * Must be called under dma_list_mutex
  */
 static int dma_chan_get(struct dma_chan *chan)
 {

quoted

+	/* allocate upon first client reference */
+	if (chan->client_count == 1 && err == 0) {
+		int desc = chan->device->device_alloc_chan_resources(chan, NULL);
+
+		if (desc < 0) {
+			chan->client_count = 0;
+			module_put(owner);
+			err = -ENOMEM;

Shouldn't we just propagate the ->device_alloc_chan_resources() return value?

Yes, this is broken.

@@ -165,12 +165,11 @@ static int dma_chan_get(struct dma_chan *chan)
 
 	/* allocate upon first client reference */
 	if (chan->client_count == 1 && err == 0) {
-		int desc = chan->device->device_alloc_chan_resources(chan);
+		err = chan->device->device_alloc_chan_resources(chan);
 
-		if (desc < 0) {
+		if (err < 0) {
 			chan->client_count = 0;
 			module_put(owner);
-			err = -ENOMEM;
 		} else if (!dma_has_cap(DMA_PRIVATE, chan->device->cap_mask))
 			balance_ref_count(chan);
 	}

quoted

+		} else
+			balance_ref_count(chan);
+	}
+
+	return err;
+}
+
+static void dma_chan_put(struct dma_chan *chan)
+{
+	if (!chan->client_count)
+		return; /* this channel failed alloc_chan_resources */

Or we had a bug ;)

...hopefully caught by the BUG_ON(dmaengine_ref_count < 0) in
dmaengine_put().

quoted

+	chan->client_count--;

Undocumented locking..

@@ -178,6 +178,12 @@ static int dma_chan_get(struct dma_chan *chan)
 	return err;
 }
 
+/**
+ * dma_chan_put - drop a reference to a dma channel's parent driver module
+ * @chan - channel to release
+ *
+ * Must be called under dma_list_mutex
+ */
 static void dma_chan_put(struct dma_chan *chan)
 {
 	if (!chan->client_count)

Regards,
Dan