dmaengine redux continued
Subsystems (2)
cover dmaengine redux continued
Here are a few more dmaengine cleanups targeted for 2.6.29. The bulk of the change is fixing the dmaengine sysfs implementation.
It has been pushed out to the 'upstream' tree of async_tx.git[1] and will move to -next shortly.
---
Dan Williams (4):
dmaengine: bump initcall level to arch_initcall
dmaengine: advertise all channels on a device to dma_filter_fn
dmaengine: use idr for registering dma device numbers
dmaengine: add a release for dma class devices and dependent infrastructure
drivers/dca/dca-core.c | 2 - drivers/dma/dmaengine.c | 169 ++++++++++++++++++++++++++++++++------------- drivers/dma/dmatest.c | 14 ++-- drivers/dma/dw_dmac.c | 91 +++++++++++++----------- drivers/dma/fsldma.c | 2 - include/linux/dmaengine.h | 23 +++++- 6 files changed, 201 insertions(+), 100 deletions(-)
Signature · 1 line
[1] git://git.kernel.org/pub/scm/linux/kernel/git/djbw/async_tx.git upstream
PATCH 1/4 dmaengine: add a release for dma class devices and dependent infrastructure
Resolves:
WARNING: at drivers/base/core.c:122 device_release+0x4d/0x52()
Device 'dma0chan0' does not have a release() function, it is broken and must be fixed.
The dma_chan_dev object is introduced to gear-match sysfs kobject and dmaengine channel lifetimes. When a channel is removed access to the sysfs entries return -ENODEV until the kobject can be released.
The bulk of the change is updates to existing code to handle the extra layer of indirection between a dma_chan and its struct device.
--- drivers/dma/dmaengine.c | 108 +++++++++++++++++++++++++++++++++++---------- drivers/dma/dmatest.c | 14 +++--- drivers/dma/dw_dmac.c | 91 +++++++++++++++++++++----------------- drivers/dma/fsldma.c | 2 - include/linux/dmaengine.h | 19 +++++++- 5 files changed, 158 insertions(+), 76 deletions(-)
drivers/dma/dmaengine.c +84 -24
@@ -64,36 +64,75 @@ static long dmaengine_ref_count;/* --- sysfs implementation --- */ +/** +* dev_to_dma_chan - convert a device pointer to the its sysfs container object +* @dev - device node +* +* Must be called under dma_list_mutex +*/ +static struct dma_chan *dev_to_dma_chan (struct device *dev) +{ +struct dma_chan_dev *chan_dev; + + chan_dev =container_of (dev,typeof (*chan_dev), device); +return chan_dev->chan; +} +static ssize_t show_memcpy_count (struct device *dev,struct device_attribute *attr,char *buf) { -struct dma_chan *chan =to_dma_chan (dev); +struct dma_chan *chan;unsigned long count =0 ;int i; +int err; -for_each_possible_cpu (i) - count +=per_cpu_ptr (chan->local, i)->memcpy_count; +mutex_lock (&dma_list_mutex); + chan =dev_to_dma_chan (dev); +if (chan) { +for_each_possible_cpu (i) + count +=per_cpu_ptr (chan->local, i)->memcpy_count; + err =sprintf (buf,"%lu\n" , count); + }else + err = -ENODEV ; +mutex_unlock (&dma_list_mutex); -return sprintf (buf,"%lu\n" , count); +return err; }static ssize_t show_bytes_transferred (struct device *dev,struct device_attribute *attr,char *buf) { -struct dma_chan *chan =to_dma_chan (dev); +struct dma_chan *chan;unsigned long count =0 ;int i; +int err; -for_each_possible_cpu (i) - count +=per_cpu_ptr (chan->local, i)->bytes_transferred; +mutex_lock (&dma_list_mutex); + chan =dev_to_dma_chan (dev); +if (chan) { +for_each_possible_cpu (i) + count +=per_cpu_ptr (chan->local, i)->bytes_transferred; + err =sprintf (buf,"%lu\n" , count); + }else + err = -ENODEV ; +mutex_unlock (&dma_list_mutex); -return sprintf (buf,"%lu\n" , count); +return err; }static ssize_t show_in_use (struct device *dev,struct device_attribute *attr,char *buf) { -struct dma_chan *chan =to_dma_chan (dev); +struct dma_chan *chan; +int err; -return sprintf (buf,"%d\n" , chan->client_count); +mutex_lock (&dma_list_mutex); + chan =dev_to_dma_chan (dev); +if (chan) + err =sprintf (buf,"%d\n" , chan->client_count); +else + err = -ENODEV ; +mutex_unlock (&dma_list_mutex); + +return err; }static struct device_attribute dma_attrs[] = {
@@ -103,9 +142,18 @@ static struct device_attribute dma_attrs[] = { __ATTR_NULL }; +static void chan_dev_release (struct device *dev) +{ +struct dma_chan_dev *chan_dev; + + chan_dev =container_of (dev,typeof (*chan_dev), device); +kfree (chan_dev); +} +static struct class dma_devclass = { .name ="dma" , .dev_attrs = dma_attrs, + .dev_release = chan_dev_release, };/* --- client and device registration --- */
@@ -415,12 +463,12 @@ static struct dma_chan *private_candidate(dma_cap_mask_t *mask, struct dma_deviclist_for_each_entry (chan, &dev->channels, device_node) {if (!__dma_chan_satisfies_mask (chan, mask)) {pr_debug ("%s: %s wrong capabilities\n" , - __func__,dev_name (&chan->dev)); + __func__,dma_chan_name (chan));continue ; }if (chan->client_count) {pr_debug ("%s: %s busy\n" , - __func__,dev_name (&chan->dev)); + __func__,dma_chan_name (chan));continue ; } ret = chan;
@@ -466,22 +514,22 @@ struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, vif (err == -ENODEV ) {pr_debug ("%s: %s module removed\n" , __func__, -dev_name (&chan->dev)); +dma_chan_name (chan));list_del_rcu (&device->global_node); }else if (err)pr_err ("dmaengine: failed to get %s: (%d)" , -dev_name (&chan->dev), err); +dma_chan_name (chan), err);else break ; }else pr_debug ("%s: %s filter said false\n" , - __func__,dev_name (&chan->dev)); + __func__,dma_chan_name (chan)); chan =NULL ; }mutex_unlock (&dma_list_mutex);pr_debug ("%s: %s (%s)\n" , __func__, chan ?"success" :"fail" , - chan ?dev_name (&chan->dev) :NULL ); + chan ?dma_chan_name (chan) :NULL );return chan; }
@@ -521,7 +569,7 @@ void dmaengine_get(void)break ; }else if (err)pr_err ("dmaengine: failed to get %s: (%d)" , -dev_name (&chan->dev), err); +dma_chan_name (chan), err); } }mutex_unlock (&dma_list_mutex);
@@ -594,14 +642,20 @@ int dma_async_device_register(struct dma_device *device) chan->local =alloc_percpu (typeof (*chan->local));if (chan->local ==NULL )continue ; + chan->dev =kzalloc (sizeof (*chan->dev),GFP_KERNEL ); +if (chan->dev ==NULL ) { +free_percpu (chan->local); +continue ; + } chan->chan_id = chancnt++; - chan->dev.class = &dma_devclass; - chan->dev.parent = device->dev; -dev_set_name (&chan->dev,"dma%dchan%d" , + chan->dev->device.class = &dma_devclass; + chan->dev->device.parent = device->dev; + chan->dev->chan = chan; +dev_set_name (&chan->dev->device,"dma%dchan%d" , device->dev_id, chan->chan_id); - rc =device_register (&chan->dev); + rc =device_register (&chan->dev->device);if (rc) {free_percpu (chan->local); chan->local =NULL ;
@@ -638,7 +692,10 @@ err_out:list_for_each_entry (chan, &device->channels, device_node) {if (chan->local ==NULL )continue ; -device_unregister (&chan->dev); +mutex_lock (&dma_list_mutex); + chan->dev->chan =NULL ; +mutex_unlock (&dma_list_mutex); +device_unregister (&chan->dev->device);free_percpu (chan->local); }return rc;
@@ -665,7 +722,10 @@ void dma_async_device_unregister(struct dma_device *device)WARN_ONCE (chan->client_count,"%s called while %d clients hold a reference\n" , __func__, chan->client_count); -device_unregister (&chan->dev); +mutex_lock (&dma_list_mutex); + chan->dev->chan =NULL ; +mutex_unlock (&dma_list_mutex); +device_unregister (&chan->dev->device); } }EXPORT_SYMBOL (dma_async_device_unregister);
@@ -838,7 +898,7 @@ dma_wait_for_async_tx(struct dma_async_tx_descriptor *tx)return DMA_SUCCESS ;WARN_ONCE (tx->parent,"%s: speculatively walking dependency chain for" -" %s\n" , __func__,dev_name (&tx->chan->dev)); +" %s\n" , __func__,dma_chan_name (tx->chan));/* poll through the dependency chain, return when tx is complete */ do {
drivers/dma/dmatest.c +7 -7
@@ -80,7 +80,7 @@ static bool dmatest_match_channel(struct dma_chan *chan) {if (test_channel[0 ] =='\0' )return true; -return strcmp (dev_name (&chan->dev), test_channel) ==0 ; +return strcmp (dma_chan_name (chan), test_channel) ==0 ; }static bool dmatest_match_device (struct dma_device *device)
@@ -325,7 +325,7 @@ static int dmatest_add_channel(struct dma_chan *chan) dtc =kmalloc (sizeof (struct dmatest_chan ),GFP_KERNEL );if (!dtc) { -pr_warning ("dmatest: No memory for %s\n" ,dev_name (&chan->dev)); +pr_warning ("dmatest: No memory for %s\n" ,dma_chan_name (chan));return -ENOMEM ; }
@@ -336,16 +336,16 @@ static int dmatest_add_channel(struct dma_chan *chan) thread =kzalloc (sizeof (struct dmatest_thread ),GFP_KERNEL );if (!thread) {pr_warning ("dmatest: No memory for %s-test%u\n" , -dev_name (&chan->dev), i); +dma_chan_name (chan), i);break ; } thread->chan = dtc->chan;smp_wmb (); thread->task =kthread_run (dmatest_func, thread,"%s-test%u" , -dev_name (&chan->dev), i); +dma_chan_name (chan), i);if (IS_ERR (thread->task)) {pr_warning ("dmatest: Failed to run thread %s-test%u\n" , -dev_name (&chan->dev), i); +dma_chan_name (chan), i);kfree (thread);break ; }
@@ -355,7 +355,7 @@ static int dmatest_add_channel(struct dma_chan *chan)list_add_tail (&thread->node, &dtc->threads); } -pr_info ("dmatest: Started %u threads using %s\n" , i,dev_name (&chan->dev)); +pr_info ("dmatest: Started %u threads using %s\n" , i,dma_chan_name (chan));list_add_tail (&dtc->node, &dmatest_channels); nr_channels++;
@@ -408,7 +408,7 @@ static void __exit dmatest_exit(void)list_del (&dtc->node);dmatest_cleanup_channel (dtc);pr_debug ("dmatest: dropped channel %s\n" , -dev_name (&dtc->chan->dev)); +dma_chan_name (dtc->chan));dma_release_channel (dtc->chan); } }
drivers/dma/dw_dmac.c +50 -41
291 lines not on this page · raw message
drivers/dma/fsldma.c +1 -1
@@ -822,7 +822,7 @@ static int __devinit fsl_dma_chan_probe(struct fsl_dma_device *fdev,*/ WARN_ON (fdev->feature != new_fsl_chan->feature); - new_fsl_chan->dev = &new_fsl_chan->common.dev; + new_fsl_chan->dev = &new_fsl_chan->common.dev->device; new_fsl_chan->reg_base =ioremap (new_fsl_chan->reg.start, new_fsl_chan->reg.end - new_fsl_chan->reg.start +1 );
include/linux/dmaengine.h +16 -3
@@ -113,7 +113,7 @@ struct dma_chan_percpu {* @device: ptr to the dma device who supplies this channel, always !%NULL * @cookie: last cookie value returned to client * @chan_id: channel ID for sysfs -* @class_dev: class device for sysfs +* @dev: class device for sysfs * @refcount: kref, used in "bigref" slow-mode * @slow_ref: indicates that the DMA channel is free * @rcu: the DMA channel's RCU head
@@ -128,7 +128,7 @@ struct dma_chan {/* sysfs */ int chan_id; -struct device dev; +struct dma_chan_dev *dev;struct list_head device_node;struct dma_chan_percpu *local;
@@ -136,7 +136,20 @@ struct dma_chan {int table_count; }; -#define to_dma_chan (p) container_of(p, struct dma_chan, dev) +/** +* struct dma_chan_dev - relate sysfs device node to backing channel device +* @chan - driver channel device +* @device - sysfs device +*/ +struct dma_chan_dev { +struct dma_chan *chan; +struct device device; +}; + +static inline const char *dma_chan_name (struct dma_chan *chan) +{ +return dev_name (&chan->dev->device); +}void dma_chan_cleanup (struct kref *kref);
Re: [PATCH 1/4] dmaengine: add a release for dma class devices and dependent infrastructure
Quoted 8 lines · On Wed, 03 Dec 2008 17:46:46 -0700 Dan Williams <[email protected]>…
Resolves:
WARNING: at drivers/base/core.c:122 device_release+0x4d/0x52()Device 'dma0chan0' does not have a
release()function, it is broken and must be fixed.The
dma_chan_devobject is introduced to gear-match sysfs kobject and dmaengine channel lifetimes. When a channel is removed access to the sysfs entries return-ENODEVuntil the kobject can be released.
Quoted 7 lines
The bulk of the change is updates to existing code to handle the extra layer of indirection between a
dma_chanand its struct device.Reported-by Alexander Beregalov <[email protected]>Cc Stephen Hemminger <[email protected]>Cc Haavard Skinnemoen <[email protected]>Signed-off-by Dan Williams <[email protected]>
Much better thanks.
PATCH 2/4 dmaengine: use idr for registering dma device numbers
In reply to Dan WilliamsThis brings some predictability to dma device numbers, i.e. an rmmod/insmod cycle may now result in /sys/class/dma/dma0chan0 being restored rather than /sys/class/dma/dma1chan0 appearing.
--- drivers/dma/dmaengine.c | 26 ++++++++++++++++++++++++-- include/linux/dmaengine.h | 4 ++++ 2 files changed, 28 insertions(+), 2 deletions(-)
drivers/dma/dmaengine.c +24 -2
@@ -57,10 +57,12 @@#include <linux/mutex.h> #include <linux/jiffies.h> #include <linux/rculist.h> +#include <linux/idr.h> static DEFINE_MUTEX (dma_list_mutex);static LIST_HEAD (dma_device_list);static long dmaengine_ref_count; +static struct idr dma_idr;/* --- sysfs implementation --- */
@@ -147,6 +149,11 @@ static void chan_dev_release(struct device *dev)struct dma_chan_dev *chan_dev; chan_dev =container_of (dev,typeof (*chan_dev), device); +if (atomic_dec_and_test (chan_dev->idr_ref)) { +mutex_lock (&dma_list_mutex); +idr_remove (&dma_idr, chan_dev->dev_id); +mutex_unlock (&dma_list_mutex); + }kfree (chan_dev); }
@@ -604,9 +611,9 @@ EXPORT_SYMBOL(dmaengine_put);*/ int dma_async_device_register (struct dma_device *device) { -static int id;int chancnt =0 , rc;struct dma_chan * chan; +atomic_t *idr_ref;if (!device)return -ENODEV ;
@@ -633,9 +640,20 @@ int dma_async_device_register(struct dma_device *device)BUG_ON (!device->device_issue_pending);BUG_ON (!device->dev); + idr_ref =kmalloc (sizeof (*idr_ref),GFP_KERNEL ); +if (!idr_ref) +return -ENOMEM ; +atomic_set (idr_ref,0 ); + idr_retry: +if (!idr_pre_get (&dma_idr,GFP_KERNEL )) +return -ENOMEM ;mutex_lock (&dma_list_mutex); - device->dev_id = id++; + rc =idr_get_new (&dma_idr,NULL , &device->dev_id);mutex_unlock (&dma_list_mutex); +if (rc == -EAGAIN ) + goto idr_retry; +else if (rc !=0 ) +return rc;/* represent channels in sysfs. Probably want devs too */ list_for_each_entry (chan, &device->channels, device_node) {
@@ -652,6 +670,9 @@ int dma_async_device_register(struct dma_device *device) chan->dev->device.class = &dma_devclass; chan->dev->device.parent = device->dev; chan->dev->chan = chan; + chan->dev->idr_ref = idr_ref; + chan->dev->dev_id = device->dev_id; +atomic_inc (idr_ref);dev_set_name (&chan->dev->device,"dma%dchan%d" , device->dev_id, chan->chan_id);
@@ -964,6 +985,7 @@ EXPORT_SYMBOL_GPL(dma_run_dependencies);static int __init dma_bus_init (void ) { +idr_init (&dma_idr);mutex_init (&dma_list_mutex);return class_register (&dma_devclass); }
include/linux/dmaengine.h +4 -0
@@ -140,10 +140,14 @@ struct dma_chan {* struct dma_chan_dev - relate sysfs device node to backing channel device * @chan - driver channel device * @device - sysfs device +* @dev_id - parent dma_device dev_id +* @idr_ref - reference count to gate release of dma_device dev_id */ struct dma_chan_dev {struct dma_chan *chan;struct device device; +int dev_id; +atomic_t *idr_ref; };static inline const char *dma_chan_name (struct dma_chan *chan)
PATCH 3/4 dmaengine: advertise all channels on a device to dma_filter_fn
In reply to Dan WilliamsAllow dma_filter_fn routines to disambiguate multiple channels on a device rather than assuming that all channels on a device are equal.
--- drivers/dma/dmaengine.c | 33 +++++++++++++-------------------- 1 files changed, 13 insertions(+), 20 deletions(-)
drivers/dma/dmaengine.c +13 -20
@@ -452,10 +452,10 @@ static void dma_channel_rebalance(void) } } -static struct dma_chan *private_candidate (dma_cap_mask_t *mask,struct dma_device *dev) +static struct dma_chan *private_candidate (dma_cap_mask_t *mask,struct dma_device *dev, +dma_filter_fn fn,void *fn_param) {struct dma_chan *chan; -struct dma_chan *ret =NULL ;/* devices with multiple channels need special handling as we need to * ensure that all channels are either private or public.
@@ -478,11 +478,15 @@ static struct dma_chan *private_candidate(dma_cap_mask_t *mask, struct dma_devic __func__, dma_chan_name(chan));continue ; } - ret = chan; -break ; +if (fn && !fn (chan, fn_param)) { +pr_debug ("%s: %s filter said false\n" , + __func__,dma_chan_name (chan)); +continue ; + } +return chan; } -return ret; +return NULL ; }/**
@@ -495,22 +499,13 @@ struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, v {struct dma_device *device, *_d;struct dma_chan *chan =NULL ; -bool ack;int err;/* Find a channel */ mutex_lock (&dma_list_mutex);list_for_each_entry_safe (device, _d, &dma_device_list, global_node) { - chan =private_candidate (mask, device); -if (!chan) -continue ; - -if (fn) - ack =fn (chan, fn_param); -else - ack = true; - -if (ack) { + chan =private_candidate (mask, device, fn, fn_param); +if (chan) {/* Found a suitable channel, try to grab, prep, and * return it. We first set DMA_PRIVATE to disable * balance_ref_count as this channel will not be
@@ -528,10 +523,8 @@ struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, v dma_chan_name(chan), err); else break; - }else -pr_debug ("%s: %s filter said false\n" , - __func__,dma_chan_name (chan)); - chan =NULL ; + chan =NULL ; + } }mutex_unlock (&dma_list_mutex);
PATCH 4/4 dmaengine: bump initcall level to arch_initcall
In reply to Dan WilliamsThere are dmaengine users that would like to register dma devices at subsys_initcall time to ensure channels are available by device_initcall time.
--- drivers/dca/dca-core.c | 2 +- drivers/dma/dmaengine.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
drivers/dca/dca-core.c +1 -1
@@ -270,6 +270,6 @@ static void __exit dca_exit(void)dca_sysfs_exit (); } -subsys_initcall (dca_init); +arch_initcall (dca_init);module_exit (dca_exit);
drivers/dma/dmaengine.c +2 -2
@@ -316,7 +316,7 @@ static int __init dma_channel_table_init(void)return err; } -subsys_initcall (dma_channel_table_init); +arch_initcall (dma_channel_table_init);/** * dma_find_channel - find a channel to carry out the operation
@@ -982,6 +982,6 @@ static int __init dma_bus_init(void)mutex_init (&dma_list_mutex);return class_register (&dma_devclass); } -subsys_initcall (dma_bus_init); +arch_initcall (dma_bus_init);
Hi Dan,
Quoted 6 lines · Dan Williams
Here are a few more dmaengine cleanups targeted for 2.6.29. The bulk of the change is fixing the dmaengine sysfs implementation.
It has been pushed out to the 'upstream' tree of async_tx.git[1] and will move to -next shortly.
Quoted 7 lines
---
Dan Williams (4):
dmaengine: bump initcall level toarch_initcall
dmaengine: advertise all channels on a device todma_filter_fn
dmaengine: use idr for registering dma device numbers
dmaengine: add a release for dma class devices and dependent infrastructure
What about the last hunk from the patch at
http://marc.info/?l=linux-arm-kernel&m=122607472721145&w=2
? Can we add this tx_free pointer? Would you like me to submit it as a separate patch? I'd like to be able to reuse tx-descriptors in clients, so I then have to explicitly release them in the dmaengine device (controller) driver.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
Quoted 9 lines · Guennadi Liakhovetski
What about the last hunk from the patch at
http://marc.info/?l=linux-arm-kernel&m=122607472721145&w=2
? Can we add this
tx_freepointer? Would you like me to submit it as a separate patch? I'd like to be able to reuse tx-descriptors in clients, so I then have to explicitly release them in the dmaengine device (controller) driver.
I took a look but the description does not explain why the driver needs an explicit release. In what cases is the implied release, when the descriptor is acked && done, insufficient?
Quoted 12 lines · Dan Williams
Quoted 9 lines · Guennadi Liakhovetski
What about the last hunk from the patch at
http://marc.info/?l=linux-arm-kernel&m=122607472721145&w=2
? Can we add this
tx_freepointer? Would you like me to submit it as a separate patch? I'd like to be able to reuse tx-descriptors in clients, so I then have to explicitly release them in the dmaengine device (controller) driver.
Quoted 3 lines
I took a look but the description does not explain why the driver needs an explicit release. In what cases is the implied release, when the descriptor is acked && done, insufficient?
v4l. I think, the proper moment to free tx-descriptors is at VIDIOC_DQBUF time. I.e., there is an explicit ioctl from user-space: "free buffers now". That's when I want to call .tx_free().
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer