| Message ID | 20200228130747.22905-2-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dmaengine: Initial debugfs support | expand |
On 28-02-20, 15:07, Peter Ujfalusi wrote: > Via the /sys/kernel/debug/dmaengine/summary users can get information > about the DMA devices and the used channels. > > Example output on am654-evm with audio using two channels and after running > dmatest on 4 channels: > > dma0 (285c0000.dma-controller): number of channels: 96 > > dma1 (31150000.dma-controller): number of channels: 267 > dma1chan0 | 2b00000.mcasp:tx > dma1chan1 | 2b00000.mcasp:rx > dma1chan2 | in-use > dma1chan3 | in-use > dma1chan4 | in-use > dma1chan5 | in-use > > For slave channels we can show the device and the channel name a given > channel is requested. > For non slave devices the only information we know is that the channel is > in use. > > DMA drivers can implement the optional dbg_summary_show callback to > provide controller specific information instead of the generic one. > > It is easy to extend the generic dmaengine_summary_show() to print > additional information about the used channels. > > In case DMA drivers want to create additional files under the dmaengine > directory to provide additional debug points, they can use the > dmaengine_get_debugfs_root() to get the dentry of the rootdir. > > I have taken the idea from gpiolib and clk subsystems. > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > --- > drivers/dma/dmaengine.c | 77 +++++++++++++++++++++++++++++++++++++++ > drivers/dma/dmaengine.h | 6 +++ > include/linux/dmaengine.h | 12 +++++- > 3 files changed, 94 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c > index c3b1283b6d31..b5e8b737785c 100644 > --- a/drivers/dma/dmaengine.c > +++ b/drivers/dma/dmaengine.c > @@ -760,6 +760,11 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name) > return chan ? chan : ERR_PTR(-EPROBE_DEFER); > > found: > +#ifdef CONFIG_DEBUG_FS > + chan->dbg_client_name = kasprintf(GFP_KERNEL, "%s:%s", dev_name(dev), > + name); > +#endif > + > chan->name = kasprintf(GFP_KERNEL, "dma:%s", name); > if (!chan->name) > return chan; > @@ -837,6 +842,11 @@ void dma_release_channel(struct dma_chan *chan) > chan->name = NULL; > chan->slave = NULL; > } > + > +#ifdef CONFIG_DEBUG_FS > + kfree(chan->dbg_client_name); > + chan->dbg_client_name = NULL; > +#endif > mutex_unlock(&dma_list_mutex); > } > EXPORT_SYMBOL_GPL(dma_release_channel); > @@ -1562,3 +1572,70 @@ static int __init dma_bus_init(void) > return class_register(&dma_devclass); > } > arch_initcall(dma_bus_init); > + > +#ifdef CONFIG_DEBUG_FS > +#include <linux/debugfs.h> > + > +static struct dentry *rootdir; > + > +struct dentry *dmaengine_get_debugfs_root(void) > +{ > + return rootdir; > +} > +EXPORT_SYMBOL_GPL(dmaengine_get_debugfs_root); > + > +static void dmaengine_dbg_summary_show(struct seq_file *s, > + struct dma_device *dma_dev) > +{ > + struct dma_chan *chan; > + > + list_for_each_entry(chan, &dma_dev->channels, device_node) { > + if (chan->client_count) { > + seq_printf(s, " %-13s| %s", dma_chan_name(chan), > + chan->dbg_client_name ?: "in-use"); > + > + if (chan->router) > + seq_printf(s, " (via router: %s)\n", > + dev_name(chan->router->dev)); > + else > + seq_puts(s, "\n"); > + } > + } > +} > + > +static int dmaengine_summary_show(struct seq_file *s, void *data) > +{ > + struct dma_device *dma_dev = NULL; > + > + mutex_lock(&dma_list_mutex); > + list_for_each_entry(dma_dev, &dma_device_list, global_node) { > + seq_printf(s, "dma%d (%s): number of channels: %u\n", > + dma_dev->dev_id, dev_name(dma_dev->dev), > + dma_dev->chancnt); > + > + if (dma_dev->dbg_summary_show) > + dma_dev->dbg_summary_show(s, dma_dev); > + else > + dmaengine_dbg_summary_show(s, dma_dev); > + > + if (!list_is_last(&dma_dev->global_node, &dma_device_list)) > + seq_puts(s, "\n"); > + } > + mutex_unlock(&dma_list_mutex); > + > + return 0; > +} > +DEFINE_SHOW_ATTRIBUTE(dmaengine_summary); > + > +static int __init dmaengine_debugfs_init(void) > +{ > + rootdir = debugfs_create_dir("dmaengine", NULL); > + > + /* /sys/kernel/debug/dmaengine */ > + debugfs_create_file("summary", 0444, rootdir, NULL, > + &dmaengine_summary_fops); > + return 0; > +} > +late_initcall(dmaengine_debugfs_init); > + > +#endif /* DEBUG_FS */ > diff --git a/drivers/dma/dmaengine.h b/drivers/dma/dmaengine.h > index e8a320c9e57c..72cd7fe33638 100644 > --- a/drivers/dma/dmaengine.h > +++ b/drivers/dma/dmaengine.h > @@ -182,4 +182,10 @@ dmaengine_desc_callback_valid(struct dmaengine_desc_callback *cb) > struct dma_chan *dma_get_slave_channel(struct dma_chan *chan); > struct dma_chan *dma_get_any_slave_channel(struct dma_device *device); > > +#ifdef CONFIG_DEBUG_FS > +#include <linux/debugfs.h> > + > +struct dentry *dmaengine_get_debugfs_root(void); this needs to have an else defined with NULL return so that we dont force users to wrap the code under CONFIG_DEBUG_FS..
Hi Vinod, On 02/03/2020 9.11, Vinod Koul wrote: >> diff --git a/drivers/dma/dmaengine.h b/drivers/dma/dmaengine.h >> index e8a320c9e57c..72cd7fe33638 100644 >> --- a/drivers/dma/dmaengine.h >> +++ b/drivers/dma/dmaengine.h >> @@ -182,4 +182,10 @@ dmaengine_desc_callback_valid(struct dmaengine_desc_callback *cb) >> struct dma_chan *dma_get_slave_channel(struct dma_chan *chan); >> struct dma_chan *dma_get_any_slave_channel(struct dma_device *device); >> >> +#ifdef CONFIG_DEBUG_FS >> +#include <linux/debugfs.h> >> + >> +struct dentry *dmaengine_get_debugfs_root(void); > > this needs to have an else defined with NULL return so that we dont > force users to wrap the code under CONFIG_DEBUG_FS.. Drivers would anyways should have their debugfs related code wrapped within ifdef. There is no point of having the code complied when it can not be used (no debugfs support). But I can add the else case if we really want to: #ifdef CONFIG_DEBUG_FS #include <linux/debugfs.h> struct dentry *dmaengine_get_debugfs_root(void); #else struct dentry; static inline struct dentry *dmaengine_get_debugfs_root(void) { return NULL; } #endif /* CONFIG_DEBUG_FS */ - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 02/03/2020 12.28, Peter Ujfalusi wrote: > Hi Vinod, > > On 02/03/2020 9.11, Vinod Koul wrote: >>> diff --git a/drivers/dma/dmaengine.h b/drivers/dma/dmaengine.h >>> index e8a320c9e57c..72cd7fe33638 100644 >>> --- a/drivers/dma/dmaengine.h >>> +++ b/drivers/dma/dmaengine.h >>> @@ -182,4 +182,10 @@ dmaengine_desc_callback_valid(struct dmaengine_desc_callback *cb) >>> struct dma_chan *dma_get_slave_channel(struct dma_chan *chan); >>> struct dma_chan *dma_get_any_slave_channel(struct dma_device *device); >>> >>> +#ifdef CONFIG_DEBUG_FS >>> +#include <linux/debugfs.h> >>> + >>> +struct dentry *dmaengine_get_debugfs_root(void); >> >> this needs to have an else defined with NULL return so that we dont >> force users to wrap the code under CONFIG_DEBUG_FS.. > > Drivers would anyways should have their debugfs related code wrapped > within ifdef. There is no point of having the code complied when it can > not be used (no debugfs support). > > But I can add the else case if we really want to: > > #ifdef CONFIG_DEBUG_FS > #include <linux/debugfs.h> > > struct dentry *dmaengine_get_debugfs_root(void); > > #else > struct dentry; > static inline struct dentry *dmaengine_get_debugfs_root(void) > { > return NULL; > } > #endif /* CONFIG_DEBUG_FS */ It might be even better if the core creates directories for the dma controllers in dma_async_device_register() and removes the whole directory in dma_async_device_unregister() Then drivers can get their per device root via: #ifdef CONFIG_DEBUG_FS static inline struct dentry * dmaengine_get_debugfs_root(struct dma_device *dma_dev) { return dma_dev->dbg_dev_root; } #else struct dentry; static inline struct dentry * dmaengine_get_debugfs_root(struct dma_device *dma_dev) { return NULL; } #endif /* CONFIG_DEBUG_FS */ - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 02-03-20, 13:53, Peter Ujfalusi wrote: > > > On 02/03/2020 12.28, Peter Ujfalusi wrote: > > Hi Vinod, > > > > On 02/03/2020 9.11, Vinod Koul wrote: > >>> diff --git a/drivers/dma/dmaengine.h b/drivers/dma/dmaengine.h > >>> index e8a320c9e57c..72cd7fe33638 100644 > >>> --- a/drivers/dma/dmaengine.h > >>> +++ b/drivers/dma/dmaengine.h > >>> @@ -182,4 +182,10 @@ dmaengine_desc_callback_valid(struct dmaengine_desc_callback *cb) > >>> struct dma_chan *dma_get_slave_channel(struct dma_chan *chan); > >>> struct dma_chan *dma_get_any_slave_channel(struct dma_device *device); > >>> > >>> +#ifdef CONFIG_DEBUG_FS > >>> +#include <linux/debugfs.h> > >>> + > >>> +struct dentry *dmaengine_get_debugfs_root(void); > >> > >> this needs to have an else defined with NULL return so that we dont > >> force users to wrap the code under CONFIG_DEBUG_FS.. > > > > Drivers would anyways should have their debugfs related code wrapped > > within ifdef. There is no point of having the code complied when it can > > not be used (no debugfs support). > > > > But I can add the else case if we really want to: > > > > #ifdef CONFIG_DEBUG_FS > > #include <linux/debugfs.h> > > > > struct dentry *dmaengine_get_debugfs_root(void); > > > > #else > > struct dentry; > > static inline struct dentry *dmaengine_get_debugfs_root(void) > > { > > return NULL; > > } > > #endif /* CONFIG_DEBUG_FS */ > > It might be even better if the core creates directories for the dma > controllers in dma_async_device_register() and removes the whole > directory in dma_async_device_unregister() hmmm, i think makes sense and dentry can be part of dma_device > > Then drivers can get their per device root via: > #ifdef CONFIG_DEBUG_FS > static inline struct dentry * > dmaengine_get_debugfs_root(struct dma_device *dma_dev) { > return dma_dev->dbg_dev_root; > } right! > #else > struct dentry; > static inline struct dentry * > dmaengine_get_debugfs_root(struct dma_device *dma_dev) > { > return NULL; > } > #endif /* CONFIG_DEBUG_FS */
diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c index c3b1283b6d31..b5e8b737785c 100644 --- a/drivers/dma/dmaengine.c +++ b/drivers/dma/dmaengine.c @@ -760,6 +760,11 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name) return chan ? chan : ERR_PTR(-EPROBE_DEFER); found: +#ifdef CONFIG_DEBUG_FS + chan->dbg_client_name = kasprintf(GFP_KERNEL, "%s:%s", dev_name(dev), + name); +#endif + chan->name = kasprintf(GFP_KERNEL, "dma:%s", name); if (!chan->name) return chan; @@ -837,6 +842,11 @@ void dma_release_channel(struct dma_chan *chan) chan->name = NULL; chan->slave = NULL; } + +#ifdef CONFIG_DEBUG_FS + kfree(chan->dbg_client_name); + chan->dbg_client_name = NULL; +#endif mutex_unlock(&dma_list_mutex); } EXPORT_SYMBOL_GPL(dma_release_channel); @@ -1562,3 +1572,70 @@ static int __init dma_bus_init(void) return class_register(&dma_devclass); } arch_initcall(dma_bus_init); + +#ifdef CONFIG_DEBUG_FS +#include <linux/debugfs.h> + +static struct dentry *rootdir; + +struct dentry *dmaengine_get_debugfs_root(void) +{ + return rootdir; +} +EXPORT_SYMBOL_GPL(dmaengine_get_debugfs_root); + +static void dmaengine_dbg_summary_show(struct seq_file *s, + struct dma_device *dma_dev) +{ + struct dma_chan *chan; + + list_for_each_entry(chan, &dma_dev->channels, device_node) { + if (chan->client_count) { + seq_printf(s, " %-13s| %s", dma_chan_name(chan), + chan->dbg_client_name ?: "in-use"); + + if (chan->router) + seq_printf(s, " (via router: %s)\n", + dev_name(chan->router->dev)); + else + seq_puts(s, "\n"); + } + } +} + +static int dmaengine_summary_show(struct seq_file *s, void *data) +{ + struct dma_device *dma_dev = NULL; + + mutex_lock(&dma_list_mutex); + list_for_each_entry(dma_dev, &dma_device_list, global_node) { + seq_printf(s, "dma%d (%s): number of channels: %u\n", + dma_dev->dev_id, dev_name(dma_dev->dev), + dma_dev->chancnt); + + if (dma_dev->dbg_summary_show) + dma_dev->dbg_summary_show(s, dma_dev); + else + dmaengine_dbg_summary_show(s, dma_dev); + + if (!list_is_last(&dma_dev->global_node, &dma_device_list)) + seq_puts(s, "\n"); + } + mutex_unlock(&dma_list_mutex); + + return 0; +} +DEFINE_SHOW_ATTRIBUTE(dmaengine_summary); + +static int __init dmaengine_debugfs_init(void) +{ + rootdir = debugfs_create_dir("dmaengine", NULL); + + /* /sys/kernel/debug/dmaengine */ + debugfs_create_file("summary", 0444, rootdir, NULL, + &dmaengine_summary_fops); + return 0; +} +late_initcall(dmaengine_debugfs_init); + +#endif /* DEBUG_FS */ diff --git a/drivers/dma/dmaengine.h b/drivers/dma/dmaengine.h index e8a320c9e57c..72cd7fe33638 100644 --- a/drivers/dma/dmaengine.h +++ b/drivers/dma/dmaengine.h @@ -182,4 +182,10 @@ dmaengine_desc_callback_valid(struct dmaengine_desc_callback *cb) struct dma_chan *dma_get_slave_channel(struct dma_chan *chan); struct dma_chan *dma_get_any_slave_channel(struct dma_device *device); +#ifdef CONFIG_DEBUG_FS +#include <linux/debugfs.h> + +struct dentry *dmaengine_get_debugfs_root(void); +#endif + #endif diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index 64461fc64e1b..a0fff2f9f3b5 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -300,6 +300,8 @@ struct dma_router { * @chan_id: channel ID for sysfs * @dev: class device for sysfs * @name: backlink name for sysfs + * @dbg_client_name: slave name for debugfs in format: + * dev_name(requester's dev):channel name, for example: "2b00000.mcasp:tx" * @device_node: used to add this to the device chan list * @local: per-cpu pointer to a struct dma_chan_percpu * @client_count: how many clients are using this channel @@ -318,6 +320,9 @@ struct dma_chan { int chan_id; struct dma_chan_dev *dev; const char *name; +#ifdef CONFIG_DEBUG_FS + char *dbg_client_name; +#endif struct list_head device_node; struct dma_chan_percpu __percpu *local; @@ -805,7 +810,9 @@ struct dma_filter { * called and there are no further references to this structure. This * must be implemented to free resources however many existing drivers * do not and are therefore not safe to unbind while in use. - * + * @dbg_summary_show: optional routine to show contents in debugfs; default code + * will be used when this is omitted, but custom code can show extra, + * controller specific information. */ struct dma_device { struct kref ref; @@ -891,6 +898,9 @@ struct dma_device { struct dma_tx_state *txstate); void (*device_issue_pending)(struct dma_chan *chan); void (*device_release)(struct dma_device *dev); +#ifdef CONFIG_DEBUG_FS + void (*dbg_summary_show)(struct seq_file *s, struct dma_device *dev); +#endif }; static inline int dmaengine_slave_config(struct dma_chan *chan,
Via the /sys/kernel/debug/dmaengine/summary users can get information about the DMA devices and the used channels. Example output on am654-evm with audio using two channels and after running dmatest on 4 channels: dma0 (285c0000.dma-controller): number of channels: 96 dma1 (31150000.dma-controller): number of channels: 267 dma1chan0 | 2b00000.mcasp:tx dma1chan1 | 2b00000.mcasp:rx dma1chan2 | in-use dma1chan3 | in-use dma1chan4 | in-use dma1chan5 | in-use For slave channels we can show the device and the channel name a given channel is requested. For non slave devices the only information we know is that the channel is in use. DMA drivers can implement the optional dbg_summary_show callback to provide controller specific information instead of the generic one. It is easy to extend the generic dmaengine_summary_show() to print additional information about the used channels. In case DMA drivers want to create additional files under the dmaengine directory to provide additional debug points, they can use the dmaengine_get_debugfs_root() to get the dentry of the rootdir. I have taken the idea from gpiolib and clk subsystems. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> --- drivers/dma/dmaengine.c | 77 +++++++++++++++++++++++++++++++++++++++ drivers/dma/dmaengine.h | 6 +++ include/linux/dmaengine.h | 12 +++++- 3 files changed, 94 insertions(+), 1 deletion(-)