[PATCH] dma-debug: add dumping facility via debugfs

[PATCH] dma-debug: add dumping facility via debugfs

[Corentin Labbe]

While debugging a DMA mapping leak, I needed to access
debug_dma_dump_mappings() but easily from user space.

This patch adds a /sys/kernel/debug/dma-api/dump file which contain all
current DMA mapping.

Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
---
 kernel/dma/debug.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
index 23cf5361bcf1..9253382f5729 100644
--- a/kernel/dma/debug.c
+++ b/kernel/dma/debug.c
@@ -144,6 +144,7 @@ static struct dentry *num_free_entries_dent __read_mostly;
 static struct dentry *min_free_entries_dent __read_mostly;
 static struct dentry *nr_total_entries_dent __read_mostly;
 static struct dentry *filter_dent           __read_mostly;
+static struct dentry *dump_dent             __read_mostly;
 
 /* per-driver filter related state */
 
@@ -840,6 +841,47 @@ static const struct file_operations filter_fops = {
 	.llseek = default_llseek,
 };
 
+static int dump_read(struct seq_file *seq, void *v)
+{
+	int idx;
+
+	for (idx = 0; idx < HASH_SIZE; idx++) {
+		struct hash_bucket *bucket = &dma_entry_hash[idx];
+		struct dma_debug_entry *entry;
+		unsigned long flags;
+
+		spin_lock_irqsave(&bucket->lock, flags);
+
+		list_for_each_entry(entry, &bucket->list, list) {
+			seq_printf(seq,
+				   "%s %s %s idx %d P=%llx N=%lx D=%llx L=%llx %s %s\n",
+				   dev_name(entry->dev),
+				   dev_driver_string(entry->dev),
+				   type2name[entry->type], idx,
+				   phys_addr(entry), entry->pfn,
+				   entry->dev_addr, entry->size,
+				   dir2name[entry->direction],
+				   maperr2str[entry->map_err_type]);
+		}
+
+		spin_unlock_irqrestore(&bucket->lock, flags);
+	}
+	return 0;
+}
+
+static int dump_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, dump_read, inode->i_private);
+}
+
+static const struct file_operations dump_fops = {
+	.owner = THIS_MODULE,
+	.open = dump_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+};
+
 static int dma_debug_fs_init(void)
 {
 	dma_debug_dent = debugfs_create_dir("dma-api", NULL);
@@ -894,6 +936,11 @@ static int dma_debug_fs_init(void)
 	if (!filter_dent)
 		goto out_err;
 
+	dump_dent = debugfs_create_file("dump", 0444,
+					dma_debug_dent, NULL, &dump_fops);
+	if (!dump_dent)
+		goto out_err;
+
 	return 0;
 
 out_err:
-- 
2.19.2

Re: [PATCH] dma-debug: add dumping facility via debugfs

[Robin Murphy]

On 16/01/2019 13:44, Corentin Labbe wrote:
> While debugging a DMA mapping leak, I needed to access
> debug_dma_dump_mappings() but easily from user space.
> 
> This patch adds a /sys/kernel/debug/dma-api/dump file which contain all
> current DMA mapping.
> 
> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> ---
>   kernel/dma/debug.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 47 insertions(+)
> 
> diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
> index 23cf5361bcf1..9253382f5729 100644
> --- a/kernel/dma/debug.c
> +++ b/kernel/dma/debug.c
> @@ -144,6 +144,7 @@ static struct dentry *num_free_entries_dent __read_mostly;
>   static struct dentry *min_free_entries_dent __read_mostly;
>   static struct dentry *nr_total_entries_dent __read_mostly;
>   static struct dentry *filter_dent           __read_mostly;
> +static struct dentry *dump_dent             __read_mostly;
>   
>   /* per-driver filter related state */
>   
> @@ -840,6 +841,47 @@ static const struct file_operations filter_fops = {
>   	.llseek = default_llseek,
>   };
>   
> +static int dump_read(struct seq_file *seq, void *v)
> +{
> +	int idx;
> +
> +	for (idx = 0; idx < HASH_SIZE; idx++) {
> +		struct hash_bucket *bucket = &dma_entry_hash[idx];
> +		struct dma_debug_entry *entry;
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&bucket->lock, flags);
> +
> +		list_for_each_entry(entry, &bucket->list, list) {
> +			seq_printf(seq,
> +				   "%s %s %s idx %d P=%llx N=%lx D=%llx L=%llx %s %s\n",
> +				   dev_name(entry->dev),
> +				   dev_driver_string(entry->dev),
> +				   type2name[entry->type], idx,
> +				   phys_addr(entry), entry->pfn,
> +				   entry->dev_addr, entry->size,
> +				   dir2name[entry->direction],
> +				   maperr2str[entry->map_err_type]);
> +		}
> +
> +		spin_unlock_irqrestore(&bucket->lock, flags);
> +	}
> +	return 0;
> +}

It's a shame that this is pretty much a duplication of 
debug_dma_dump_mappings(), but there seems no straightforward way to 
define one in terms of the other :/

(although given that we'd rather not have that weird external interface 
anyway, maybe "now you can use debugfs instead" might be justification 
enough for cleaning it up...)

> +
> +static int dump_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, dump_read, inode->i_private);
> +}
> +
> +static const struct file_operations dump_fops = {
> +	.owner = THIS_MODULE,
> +	.open = dump_open,
> +	.read = seq_read,
> +	.llseek = seq_lseek,
> +	.release = single_release,
> +};
> +

DEFINE_SHOW_ATTRIBUTE()?

Robin.

>   static int dma_debug_fs_init(void)
>   {
>   	dma_debug_dent = debugfs_create_dir("dma-api", NULL);
> @@ -894,6 +936,11 @@ static int dma_debug_fs_init(void)
>   	if (!filter_dent)
>   		goto out_err;
>   
> +	dump_dent = debugfs_create_file("dump", 0444,
> +					dma_debug_dent, NULL, &dump_fops);
> +	if (!dump_dent)
> +		goto out_err;
> +
>   	return 0;
>   
>   out_err:
> 

Re: [PATCH] dma-debug: add dumping facility via debugfs

[Christoph Hellwig]

On Wed, Jan 16, 2019 at 06:10:13PM +0000, Robin Murphy wrote:
> It's a shame that this is pretty much a duplication of 
> debug_dma_dump_mappings(), but there seems no straightforward way to define 
> one in terms of the other :/

We could always play some macro magic, but I think that is worse than
duplicating a little bit of functionality.

Btw, the dev argument to debug_dma_dump_mappings is always NULL and
could be removed..

> (although given that we'd rather not have that weird external interface 
> anyway, maybe "now you can use debugfs instead" might be justification 
> enough for cleaning it up...)

One argument against that is the system might be in a shape where you
can't easily read a file when it is in that shape.  The argument for
that is that in many systems the full list of mappings might overwhelm
the kernel log.

Adding Joerg, who originally wrote the code in case he has an opinion.

>
>> +
>> +static int dump_open(struct inode *inode, struct file *file)
>> +{
>> +	return single_open(file, dump_read, inode->i_private);
>> +}
>> +
>> +static const struct file_operations dump_fops = {
>> +	.owner = THIS_MODULE,
>> +	.open = dump_open,
>> +	.read = seq_read,
>> +	.llseek = seq_lseek,
>> +	.release = single_release,
>> +};
>> +
>
> DEFINE_SHOW_ATTRIBUTE()?
>
> Robin.
>
>>   static int dma_debug_fs_init(void)
>>   {
>>   	dma_debug_dent = debugfs_create_dir("dma-api", NULL);
>> @@ -894,6 +936,11 @@ static int dma_debug_fs_init(void)
>>   	if (!filter_dent)
>>   		goto out_err;
>>   +	dump_dent = debugfs_create_file("dump", 0444,
>> +					dma_debug_dent, NULL, &dump_fops);
>> +	if (!dump_dent)
>> +		goto out_err;
>> +
>>   	return 0;
>>     out_err:
>>
---end quoted text---

Re: [PATCH] dma-debug: add dumping facility via debugfs

[Joerg Roedel]

On Fri, Jan 18, 2019 at 12:35:43PM +0100, Christoph Hellwig wrote:
> On Wed, Jan 16, 2019 at 06:10:13PM +0000, Robin Murphy wrote:
> > It's a shame that this is pretty much a duplication of 
> > debug_dma_dump_mappings(), but there seems no straightforward way to define 
> > one in terms of the other :/
> 
> We could always play some macro magic, but I think that is worse than
> duplicating a little bit of functionality.

I havn't checked in detail, can seq_buf be used somehow to write a
function that fits both cases?

> Btw, the dev argument to debug_dma_dump_mappings is always NULL and
> could be removed..

This function was also written as a debug-helper for driver developers.
As such it might not make it into the final driver, but the developer
might want to use it to dump the mappings for her device only. So I'd
like to keep the parameter, even when it is always NULL for in-kernel
uses.

> > (although given that we'd rather not have that weird external interface 
> > anyway, maybe "now you can use debugfs instead" might be justification 
> > enough for cleaning it up...)
> 
> One argument against that is the system might be in a shape where you
> can't easily read a file when it is in that shape.  The argument for
> that is that in many systems the full list of mappings might overwhelm
> the kernel log.

For driver developers a file is easier to use, but in case of an
unusable system one can at least easily read out parts of the
dma-mappings from a crash-dump if it is in the kernel-log. So I think
it makes sense to have both.

Regards,

	Joerg

Re: [PATCH] dma-debug: add dumping facility via debugfs

[LABBE Corentin]

On Wed, Jan 16, 2019 at 06:10:13PM +0000, Robin Murphy wrote:
> On 16/01/2019 13:44, Corentin Labbe wrote:
> > While debugging a DMA mapping leak, I needed to access
> > debug_dma_dump_mappings() but easily from user space.
> > 
> > This patch adds a /sys/kernel/debug/dma-api/dump file which contain all
> > current DMA mapping.
> > 
> > Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> > ---
> >   kernel/dma/debug.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 47 insertions(+)
> > 
> > diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
> > index 23cf5361bcf1..9253382f5729 100644
> > --- a/kernel/dma/debug.c
> > +++ b/kernel/dma/debug.c
> > @@ -144,6 +144,7 @@ static struct dentry *num_free_entries_dent __read_mostly;
> >   static struct dentry *min_free_entries_dent __read_mostly;
> >   static struct dentry *nr_total_entries_dent __read_mostly;
> >   static struct dentry *filter_dent           __read_mostly;
> > +static struct dentry *dump_dent             __read_mostly;
> >   
> >   /* per-driver filter related state */
> >   
> > @@ -840,6 +841,47 @@ static const struct file_operations filter_fops = {
> >   	.llseek = default_llseek,
> >   };
> >   
> > +static int dump_read(struct seq_file *seq, void *v)
> > +{
> > +	int idx;
> > +
> > +	for (idx = 0; idx < HASH_SIZE; idx++) {
> > +		struct hash_bucket *bucket = &dma_entry_hash[idx];
> > +		struct dma_debug_entry *entry;
> > +		unsigned long flags;
> > +
> > +		spin_lock_irqsave(&bucket->lock, flags);
> > +
> > +		list_for_each_entry(entry, &bucket->list, list) {
> > +			seq_printf(seq,
> > +				   "%s %s %s idx %d P=%llx N=%lx D=%llx L=%llx %s %s\n",
> > +				   dev_name(entry->dev),
> > +				   dev_driver_string(entry->dev),
> > +				   type2name[entry->type], idx,
> > +				   phys_addr(entry), entry->pfn,
> > +				   entry->dev_addr, entry->size,
> > +				   dir2name[entry->direction],
> > +				   maperr2str[entry->map_err_type]);
> > +		}
> > +
> > +		spin_unlock_irqrestore(&bucket->lock, flags);
> > +	}
> > +	return 0;
> > +}
> 
> It's a shame that this is pretty much a duplication of 
> debug_dma_dump_mappings(), but there seems no straightforward way to 
> define one in terms of the other :/
> 
> (although given that we'd rather not have that weird external interface 
> anyway, maybe "now you can use debugfs instead" might be justification 
> enough for cleaning it up...)
> 
> > +
> > +static int dump_open(struct inode *inode, struct file *file)
> > +{
> > +	return single_open(file, dump_read, inode->i_private);
> > +}
> > +
> > +static const struct file_operations dump_fops = {
> > +	.owner = THIS_MODULE,
> > +	.open = dump_open,
> > +	.read = seq_read,
> > +	.llseek = seq_lseek,
> > +	.release = single_release,
> > +};
> > +
> 
> DEFINE_SHOW_ATTRIBUTE()?
> 

I will use it

Thanks