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

quoted

+/**
+ * dma_chan_tbl_ent - tracks channel allocations per core/opertion
+ */

Would be conventional to document the fields as well.

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 644a8c8..ebbfe2d 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c

@@ -218,7 +218,8 @@ EXPORT_SYMBOL(dma_sync_wait);
 static dma_cap_mask_t dma_cap_mask_all;
 
 /**
- * dma_chan_tbl_ent - tracks channel allocations per core/opertion
+ * dma_chan_tbl_ent - tracks channel allocations per core/operation
+ * @chan - associated channel for this entry
  */
 struct dma_chan_tbl_ent {
 	struct dma_chan *chan;

quoted

+	for_each_dma_cap_mask(cap, dma_cap_mask_all) {
+		channel_table[cap] = alloc_percpu(struct dma_chan_tbl_ent);
+		if (!channel_table[cap]) {
+			err = 1;

initcalls can return -ve errnos, and that at least would make the
message in do_one_initcall() more meaningful.

@@ -247,7 +247,7 @@ static int __init dma_channel_table_init(void)
 	for_each_dma_cap_mask(cap, dma_cap_mask_all) {
 		channel_table[cap] = alloc_percpu(struct dma_chan_tbl_ent);
 		if (!channel_table[cap]) {
-			err = 1;
+			err = -ENOMEM;
 			break;
 		}
 	}

quoted

+/**
+ * dma_find_channel - find a channel to carry out the operation
+ * @tx_type: transaction type
+ */
+struct dma_chan *dma_find_channel(enum dma_transaction_type tx_type)
+{
+	struct dma_chan *chan;
+	int cpu;
+
+	WARN_ONCE(dmaengine_ref_count == 0,
+		  "client called %s without a reference", __func__);
+
+	cpu = get_cpu();
+	chan = per_cpu_ptr(channel_table[tx_type], cpu)->chan;
+	put_cpu();
+
+	return chan;
+}
+EXPORT_SYMBOL(dma_find_channel);

Strange.  We return the address of a per-cpu variable, but we've
reenabled preemption so this thread can now switch CPUs.  Hence there
must be spinlocking on *chan as well?

There is locking when we call into the driver.  dmaengine is using the
cpuid as a load balancing hint and nothing more.  We loosely try to
maintain cpu-channel affinity, but if preemption occasionally jumbles
these associations clients can tolerate it.  dma_find_channel is called
relatively frequently so these misassociations should be short-lived

quoted

+	if (ret)
+		ret->table_count++;

Undocumented locking for ->table_count.

@@ -313,7 +313,8 @@ EXPORT_SYMBOL(dma_issue_pending_all);
  * @n: nth channel desired
  *
  * Defaults to returning the channel with the desired capability and the
- * lowest reference count when 'n' cannot be satisfied
+ * lowest reference count when 'n' cannot be satisfied.  Must be called
+ * under dma_list_mutex.
  */
 static struct dma_chan *nth_chan(enum dma_transaction_type cap, int n)
 {

quoted

+	/* undo the last distribution */
+	for_each_dma_cap_mask(cap, dma_cap_mask_all)
+		for_each_possible_cpu(cpu)
+			per_cpu_ptr(channel_table[cap], cpu)->chan = NULL;

The number of possible cpus can be larger than the number of online
CPUs.  Is it worth making this code hotplug-aware?

...nobody has missed it from the NET_DMA case so far, but yes, at some
future date.

Regards,
Dan