[PATCH 0/4] dma-debug: driver filter

[PATCH 0/4] dma-debug: driver filter

[Joerg Roedel]

Hi,

this small patchset adds a new feature to dma-api debugging: a driver
filter. A driver developer can write the name of his driver to
dma-api/driver_filter and the kernel will then only print errors
produced by this driver. This is useful if other drivers in the system
also have bugs. In this case the driver developer can limit the bug
output to messages interesting for him.

Joerg

diffstat:

 Documentation/DMA-API.txt |   11 +++
 lib/dma-debug.c           |  157 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 167 insertions(+), 1 deletions(-)

shortlog:

Joerg Roedel (4):
      dma-debug: add variables and checks for driver filter
      dma-debug: add debugfs file for driver filter
      dma-debug: add dma_debug_driver kernel command line
      dma-debug: add documentation for the driver filter

[PATCH 1/4] dma-debug: add variables and checks for driver filter

[Joerg Roedel]

This patch adds the state variables for the driver filter and a function
to check if the filter is enabled and matches to the current device. The
check is built into the err_printk function.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 lib/dma-debug.c |   49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 48 insertions(+), 1 deletions(-)

diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index cdd205d..e953270 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -99,6 +99,15 @@ static struct dentry *show_num_errors_dent  __read_mostly;
 static struct dentry *num_free_entries_dent __read_mostly;
 static struct dentry *min_free_entries_dent __read_mostly;
 
+/* per-driver filter related state */
+
+#define NAME_MAX_LEN	64
+
+static char                  current_driver_name[NAME_MAX_LEN] __read_mostly;
+static struct device_driver *current_driver                    __read_mostly;
+
+static DEFINE_RWLOCK(driver_name_lock);
+
 static const char *type2name[4] = { "single", "page",
 				    "scather-gather", "coherent" };
 
@@ -128,9 +137,47 @@ static inline void dump_entry_trace(struct dma_debug_entry *entry)
 #endif
 }
 
+static inline bool driver_filter(struct device *dev)
+{
+	/* driver filter off */
+	if (likely(!current_driver_name[0]))
+		return true;
+
+	/* driver filter on and initialized */
+	if (current_driver && dev->driver == current_driver)
+		return true;
+
+	/* driver filter on but not yet initialized */
+	if (!current_driver && current_driver_name[0]) {
+		struct device_driver *drv = get_driver(dev->driver);
+		unsigned long flags;
+		bool ret = false;
+
+		if (!drv)
+			return false;
+
+		/* lock to protect against change of current_driver_name */
+		read_lock_irqsave(&driver_name_lock, flags);
+
+		if (drv->name &&
+		    strncmp(current_driver_name, drv->name, 63) == 0) {
+			current_driver = drv;
+			ret = true;
+		}
+
+		read_unlock_irqrestore(&driver_name_lock, flags);
+		put_driver(drv);
+
+		return ret;
+	}
+
+	return false;
+}
+
 #define err_printk(dev, entry, format, arg...) do {		\
 		error_count += 1;				\
-		if (show_all_errors || show_num_errors > 0) {	\
+		if (driver_filter(dev) &&			\
+		    (show_all_errors || show_num_errors > 0)) {	\
 			WARN(1, "%s %s: " format,		\
 			     dev_driver_string(dev),		\
 			     dev_name(dev) , ## arg);		\
-- 
1.6.3.1

Re: [PATCH 1/4] dma-debug: add variables and checks for driver filter

[Andrew Morton]

On Thu, 28 May 2009 17:19:28 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> This patch adds the state variables for the driver filter and a function
> to check if the filter is enabled and matches to the current device. The
> check is built into the err_printk function.
> 

The path by which lib/dma-debug.c patches get into mainline seems to
be largely random.  Is it a dwmw2 tree?  An Ingo tree?  A Joerg tree? -mm?


> @@ -128,9 +137,47 @@ static inline void dump_entry_trace(struct dma_debug_entry *entry)
>  #endif
>  }
>  
> +static inline bool driver_filter(struct device *dev)
> +{
> +	/* driver filter off */
> +	if (likely(!current_driver_name[0]))
> +		return true;
> +
> +	/* driver filter on and initialized */
> +	if (current_driver && dev->driver == current_driver)
> +		return true;
> +
> +	/* driver filter on but not yet initialized */
> +	if (!current_driver && current_driver_name[0]) {
> +		struct device_driver *drv = get_driver(dev->driver);
> +		unsigned long flags;
> +		bool ret = false;
> +
> +		if (!drv)
> +			return false;
> +
> +		/* lock to protect against change of current_driver_name */
> +		read_lock_irqsave(&driver_name_lock, flags);
> +
> +		if (drv->name &&
> +		    strncmp(current_driver_name, drv->name, 63) == 0) {
> +			current_driver = drv;
> +			ret = true;
> +		}
> +
> +		read_unlock_irqrestore(&driver_name_lock, flags);
> +		put_driver(drv);
> +
> +		return ret;
> +	}
> +
> +	return false;
> +}
> +
>  #define err_printk(dev, entry, format, arg...) do {		\
>  		error_count += 1;				\
> -		if (show_all_errors || show_num_errors > 0) {	\
> +		if (driver_filter(dev) &&			\
> +		    (show_all_errors || show_num_errors > 0)) {	\
>  			WARN(1, "%s %s: " format,		\
>  			     dev_driver_string(dev),		\
>  			     dev_name(dev) , ## arg);		\

The patch attempts to inline the very large driver_filter() into every
err_printk() callsite.  That's not at all desirable!

Re: [PATCH 1/4] dma-debug: add variables and checks for driver filter

[Joerg Roedel]

On Mon, Jun 01, 2009 at 02:55:02PM -0700, Andrew Morton wrote:
> On Thu, 28 May 2009 17:19:28 +0200
> Joerg Roedel <joerg.roedel@amd.com> wrote:
> 
> > This patch adds the state variables for the driver filter and a function
> > to check if the filter is enabled and matches to the current device. The
> > check is built into the err_printk function.
> > 
> 
> The path by which lib/dma-debug.c patches get into mainline seems to
> be largely random.  Is it a dwmw2 tree?  An Ingo tree?  A Joerg tree? -mm?

When I addresses your comment I will repost and send it to Ingo later
on. He will send it upstream. At least thats the current plan :-)

Joerg

-- 
           | Advanced Micro Devices GmbH
 Operating | Karl-Hammerschmidt-Str. 34, 85609 Dornach bei München
 System    | 
 Research  | Geschäftsführer: Thomas M. McCoy, Giuliano Meroni
 Center    | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
           | Registergericht München, HRB Nr. 43632

[PATCH 2/4] dma-debug: add debugfs file for driver filter

[Joerg Roedel]

This patch adds the dma-api/driver_filter file to debugfs. The root user
can write a driver name into this file to see only dma-api errors for
that particular driver in the kernel log. Writing an empty string to
that file disables the driver filter.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 lib/dma-debug.c |   91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 90 insertions(+), 1 deletions(-)

diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index e953270..65043c7 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -23,9 +23,11 @@
 #include <linux/dma-debug.h>
 #include <linux/spinlock.h>
 #include <linux/debugfs.h>
+#include <linux/uaccess.h>
 #include <linux/device.h>
 #include <linux/types.h>
 #include <linux/sched.h>
+#include <linux/ctype.h>
 #include <linux/list.h>
 #include <linux/slab.h>
 
@@ -98,6 +100,7 @@ static struct dentry *show_all_errors_dent  __read_mostly;
 static struct dentry *show_num_errors_dent  __read_mostly;
 static struct dentry *num_free_entries_dent __read_mostly;
 static struct dentry *min_free_entries_dent __read_mostly;
+static struct dentry *filter_dent           __read_mostly;
 
 /* per-driver filter related state */
 
@@ -160,7 +163,8 @@ static inline bool driver_filter(struct device *dev)
 		read_lock_irqsave(&driver_name_lock, flags);
 
 		if (drv->name &&
-		    strncmp(current_driver_name, drv->name, 63) == 0) {
+		    strncmp(current_driver_name, drv->name,
+			    NAME_MAX_LEN-1) == 0) {
 			current_driver = drv;
 			ret = true;
 		}
@@ -454,6 +458,86 @@ out_err:
 	return -ENOMEM;
 }
 
+static ssize_t filter_read(struct file *file, char __user *user_buf,
+			   size_t count, loff_t *ppos)
+{
+	unsigned long flags;
+	char buf[NAME_MAX_LEN + 1];
+	int len;
+
+	if (!current_driver_name[0])
+		return 0;
+
+	/*
+	 * We can't copy to userspace directly because current_driver_name can
+	 * only be read under the driver_name_lock with irqs disabled. So
+	 * create a temporary copy first.
+	 */
+	read_lock_irqsave(&driver_name_lock, flags);
+	strncpy(buf, current_driver_name, NAME_MAX_LEN - 1);
+	read_unlock_irqrestore(&driver_name_lock, flags);
+
+	/* make it look nice :-) */
+	len = strnlen(buf, NAME_MAX_LEN - 1);
+	buf[len]     = '\n';
+	buf[len + 1] = 0;
+
+	return simple_read_from_buffer(user_buf, count, ppos, buf, len + 1);
+}
+
+static ssize_t filter_write(struct file *file, const char __user *userbuf,
+			    size_t count, loff_t *ppos)
+{
+	unsigned long flags;
+	char buf[NAME_MAX_LEN];
+	size_t len = NAME_MAX_LEN - 1;
+	int i;
+
+	/*
+	 * We can't copy from userspace directly. Access to
+	 * current_driver_name is protected with a write_lock with irqs
+	 * disabled. Since copy_from_user can fault and may sleep we
+	 * need to copy to temporary buffer first
+	 */
+	len = min(count, len);
+	if (copy_from_user(buf, userbuf, len))
+		return -EFAULT;
+
+	buf[NAME_MAX_LEN - 1] = 0;
+
+	write_lock_irqsave(&driver_name_lock, flags);
+	if (!isalnum(buf[0])) {
+		if (current_driver_name[0])
+			printk(KERN_INFO "DMA-API: switching off dma-debug "
+					 "driver filter\n");
+		current_driver_name[0] = 0;
+		current_driver = NULL;
+		goto out_unlock;
+	}
+
+	for (i = 0; i < NAME_MAX_LEN; ++i) {
+		current_driver_name[i] = buf[i];
+		if (isspace(buf[i]) || buf[i] == ' ' ||
+		    buf[i] == '\t' || buf[i] == 0)
+			break;
+	}
+	current_driver_name[i] = 0;
+	current_driver = NULL;
+
+	printk(KERN_INFO "DMA-API: enable driver filter for driver [%s]\n",
+	       current_driver_name);
+
+out_unlock:
+	write_unlock_irqrestore(&driver_name_lock, flags);
+
+	return count;
+}
+
+const struct file_operations filter_fops = {
+	.read  = filter_read,
+	.write = filter_write,
+};
+
 static int dma_debug_fs_init(void)
 {
 	dma_debug_dent = debugfs_create_dir("dma-api", NULL);
@@ -497,6 +581,11 @@ static int dma_debug_fs_init(void)
 	if (!min_free_entries_dent)
 		goto out_err;
 
+	filter_dent = debugfs_create_file("driver_filter", 0644,
+					  dma_debug_dent, NULL, &filter_fops);
+	if (!filter_dent)
+		goto out_err;
+
 	return 0;
 
 out_err:
-- 
1.6.3.1

Re: [PATCH 2/4] dma-debug: add debugfs file for driver filter

[Andrew Morton]

On Thu, 28 May 2009 17:19:29 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> This patch adds the dma-api/driver_filter file to debugfs. The root user
> can write a driver name into this file to see only dma-api errors for
> that particular driver in the kernel log. Writing an empty string to
> that file disables the driver filter.
> 
>
> ...
>
> +static ssize_t filter_read(struct file *file, char __user *user_buf,
> +			   size_t count, loff_t *ppos)
> +{
> +	unsigned long flags;
> +	char buf[NAME_MAX_LEN + 1];
> +	int len;
> +
> +	if (!current_driver_name[0])
> +		return 0;
> +
> +	/*
> +	 * We can't copy to userspace directly because current_driver_name can
> +	 * only be read under the driver_name_lock with irqs disabled. So
> +	 * create a temporary copy first.
> +	 */
> +	read_lock_irqsave(&driver_name_lock, flags);
> +	strncpy(buf, current_driver_name, NAME_MAX_LEN - 1);

If you use strlcpy() here,

> +	read_unlock_irqrestore(&driver_name_lock, flags);
> +
> +	/* make it look nice :-) */
> +	len = strnlen(buf, NAME_MAX_LEN - 1);
> +	buf[len]     = '\n';
> +	buf[len + 1] = 0;

the above can all be replaced with a simple strcat().  I think.

Perhaps the whole lot can be replaced with a single scnprintf(), if
it's known that the input string has appropriate length.

> +	return simple_read_from_buffer(user_buf, count, ppos, buf, len + 1);
> +}
> +
> +static ssize_t filter_write(struct file *file, const char __user *userbuf,
> +			    size_t count, loff_t *ppos)
> +{
> +	unsigned long flags;
> +	char buf[NAME_MAX_LEN];
> +	size_t len = NAME_MAX_LEN - 1;
> +	int i;
> +
> +	/*
> +	 * We can't copy from userspace directly. Access to
> +	 * current_driver_name is protected with a write_lock with irqs
> +	 * disabled. Since copy_from_user can fault and may sleep we
> +	 * need to copy to temporary buffer first
> +	 */
> +	len = min(count, len);
> +	if (copy_from_user(buf, userbuf, len))
> +		return -EFAULT;
> +
> +	buf[NAME_MAX_LEN - 1] = 0;

Might be able to use strncpy_from_user() here.

> +	write_lock_irqsave(&driver_name_lock, flags);
> +	if (!isalnum(buf[0])) {
> +		if (current_driver_name[0])
> +			printk(KERN_INFO "DMA-API: switching off dma-debug "
> +					 "driver filter\n");
> +		current_driver_name[0] = 0;
> +		current_driver = NULL;
> +		goto out_unlock;
> +	}
> +
> +	for (i = 0; i < NAME_MAX_LEN; ++i) {
> +		current_driver_name[i] = buf[i];
> +		if (isspace(buf[i]) || buf[i] == ' ' ||
> +		    buf[i] == '\t' || buf[i] == 0)
> +			break;
> +	}
> +	current_driver_name[i] = 0;
> +	current_driver = NULL;
> +
> +	printk(KERN_INFO "DMA-API: enable driver filter for driver [%s]\n",
> +	       current_driver_name);
> +
> +out_unlock:
> +	write_unlock_irqrestore(&driver_name_lock, flags);
> +
> +	return count;
> +}

It's unobvious what user interface this code is attempting to implement.
The change to DMA-API.txt in the fourth patch doesn't illuminate the
issue either. Some code comments explaining wtf this is doing and why
would help, please.

I'd have thought that the test for isspace() makes the tests for ' '
and '\t' redundant?


> +const struct file_operations filter_fops = {
> +	.read  = filter_read,
> +	.write = filter_write,
> +};
> +
>  static int dma_debug_fs_init(void)
>  {
>  	dma_debug_dent = debugfs_create_dir("dma-api", NULL);
> @@ -497,6 +581,11 @@ static int dma_debug_fs_init(void)
>  	if (!min_free_entries_dent)
>  		goto out_err;
>  
> +	filter_dent = debugfs_create_file("driver_filter", 0644,
> +					  dma_debug_dent, NULL, &filter_fops);
> +	if (!filter_dent)
> +		goto out_err;
> +
>  	return 0;
>  
>  out_err:

Re: [PATCH 2/4] dma-debug: add debugfs file for driver filter

[Joerg Roedel]

On Mon, Jun 01, 2009 at 03:04:18PM -0700, Andrew Morton wrote:
> On Thu, 28 May 2009 17:19:29 +0200
> Joerg Roedel <joerg.roedel@amd.com> wrote:
> 
> > This patch adds the dma-api/driver_filter file to debugfs. The root user
> > can write a driver name into this file to see only dma-api errors for
> > that particular driver in the kernel log. Writing an empty string to
> > that file disables the driver filter.
> > 
> >
> > ...
> >
> > +static ssize_t filter_read(struct file *file, char __user *user_buf,
> > +			   size_t count, loff_t *ppos)
> > +{
> > +	unsigned long flags;
> > +	char buf[NAME_MAX_LEN + 1];
> > +	int len;
> > +
> > +	if (!current_driver_name[0])
> > +		return 0;
> > +
> > +	/*
> > +	 * We can't copy to userspace directly because current_driver_name can
> > +	 * only be read under the driver_name_lock with irqs disabled. So
> > +	 * create a temporary copy first.
> > +	 */
> > +	read_lock_irqsave(&driver_name_lock, flags);
> > +	strncpy(buf, current_driver_name, NAME_MAX_LEN - 1);
> 
> If you use strlcpy() here,
> 
> > +	read_unlock_irqrestore(&driver_name_lock, flags);
> > +
> > +	/* make it look nice :-) */
> > +	len = strnlen(buf, NAME_MAX_LEN - 1);
> > +	buf[len]     = '\n';
> > +	buf[len + 1] = 0;
> 
> the above can all be replaced with a simple strcat().  I think.

Right, thanks.

> Perhaps the whole lot can be replaced with a single scnprintf(), if
> it's known that the input string has appropriate length.

I will check that. Thanks.

> > +	return simple_read_from_buffer(user_buf, count, ppos, buf, len + 1);
> > +}
> > +
> > +static ssize_t filter_write(struct file *file, const char __user *userbuf,
> > +			    size_t count, loff_t *ppos)
> > +{
> > +	unsigned long flags;
> > +	char buf[NAME_MAX_LEN];
> > +	size_t len = NAME_MAX_LEN - 1;
> > +	int i;
> > +
> > +	/*
> > +	 * We can't copy from userspace directly. Access to
> > +	 * current_driver_name is protected with a write_lock with irqs
> > +	 * disabled. Since copy_from_user can fault and may sleep we
> > +	 * need to copy to temporary buffer first
> > +	 */
> > +	len = min(count, len);
> > +	if (copy_from_user(buf, userbuf, len))
> > +		return -EFAULT;
> > +
> > +	buf[NAME_MAX_LEN - 1] = 0;
> 
> Might be able to use strncpy_from_user() here.
> 
> > +	write_lock_irqsave(&driver_name_lock, flags);
> > +	if (!isalnum(buf[0])) {
> > +		if (current_driver_name[0])
> > +			printk(KERN_INFO "DMA-API: switching off dma-debug "
> > +					 "driver filter\n");
> > +		current_driver_name[0] = 0;
> > +		current_driver = NULL;
> > +		goto out_unlock;
> > +	}
> > +
> > +	for (i = 0; i < NAME_MAX_LEN; ++i) {
> > +		current_driver_name[i] = buf[i];
> > +		if (isspace(buf[i]) || buf[i] == ' ' ||
> > +		    buf[i] == '\t' || buf[i] == 0)
> > +			break;
> > +	}
> > +	current_driver_name[i] = 0;
> > +	current_driver = NULL;
> > +
> > +	printk(KERN_INFO "DMA-API: enable driver filter for driver [%s]\n",
> > +	       current_driver_name);
> > +
> > +out_unlock:
> > +	write_unlock_irqrestore(&driver_name_lock, flags);
> > +
> > +	return count;
> > +}
> 
> It's unobvious what user interface this code is attempting to implement.
> The change to DMA-API.txt in the fourth patch doesn't illuminate the
> issue either. Some code comments explaining wtf this is doing and why
> would help, please.

It implements a file to which the name of a driver can be written. If
done, dma-debug will only print errors from that particular driver. I
will state that also in a comment.

> I'd have thought that the test for isspace() makes the tests for ' '
> and '\t' redundant?

Checking again, this is true for '\t' but not for ' '. The later one
would be checked by isblank() which is not implemented in the kernel.


Joerg

-- 
           | Advanced Micro Devices GmbH
 Operating | Karl-Hammerschmidt-Str. 34, 85609 Dornach bei München
 System    | 
 Research  | Geschäftsführer: Thomas M. McCoy, Giuliano Meroni
 Center    | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
           | Registergericht München, HRB Nr. 43632

Re: [PATCH 2/4] dma-debug: add debugfs file for driver filter

[Joerg Roedel]

On Mon, Jun 01, 2009 at 03:04:18PM -0700, Andrew Morton wrote:
> On Thu, 28 May 2009 17:19:29 +0200
> Joerg Roedel <joerg.roedel@amd.com> wrote:
> 
> > +	/*
> > +	 * We can't copy from userspace directly. Access to
> > +	 * current_driver_name is protected with a write_lock with irqs
> > +	 * disabled. Since copy_from_user can fault and may sleep we
> > +	 * need to copy to temporary buffer first
> > +	 */
> > +	len = min(count, len);
> > +	if (copy_from_user(buf, userbuf, len))
> > +		return -EFAULT;
> > +
> > +	buf[NAME_MAX_LEN - 1] = 0;
> 
> Might be able to use strncpy_from_user() here.
>

Hmm, no, not in this place because I can't be sure that userspace passes
a NULL-terminated string. So setting a character in the resulting string
to \0 is always required. This eliminates the benefit of
strncpy_from_user() here.

Joerg

-- 
           | Advanced Micro Devices GmbH
 Operating | Karl-Hammerschmidt-Str. 34, 85609 Dornach bei München
 System    | 
 Research  | Geschäftsführer: Thomas M. McCoy, Giuliano Meroni
 Center    | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
           | Registergericht München, HRB Nr. 43632

[PATCH 3/4] dma-debug: add dma_debug_driver kernel command line

[Joerg Roedel]

This patch add the dma_debug_driver= boot parameter to enable the driver
filter for early boot.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 lib/dma-debug.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index 65043c7..7b51fa4 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -1098,3 +1098,22 @@ void debug_dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg,
 }
 EXPORT_SYMBOL(debug_dma_sync_sg_for_device);
 
+static int __init parse_dma_debug_driver(char *str)
+{
+	int i;
+
+	for (i = 0; i < NAME_MAX_LEN - 1; ++i, ++str) {
+		current_driver_name[i] = *str;
+		if (*str == ' ' || *str == '\t' || isspace(*str) || *str == 0)
+			break;
+	}
+	current_driver_name[i] = 0;
+
+	if (current_driver_name[0])
+		printk(KERN_INFO "DMA-API: enable driver filter for "
+				 "driver [%s]\n", current_driver_name);
+
+
+	return 1;
+}
+__setup("dma_debug_driver=", parse_dma_debug_driver);
-- 
1.6.3.1

Re: [PATCH 3/4] dma-debug: add dma_debug_driver kernel command line

[Andrew Morton]

On Thu, 28 May 2009 17:19:30 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> This patch add the dma_debug_driver= boot parameter to enable the driver
> filter for early boot.
> 

Please always update Documentation/kernel-parameters.txt when adding
new boot options.

> +static int __init parse_dma_debug_driver(char *str)
> +{
> +	int i;
> +
> +	for (i = 0; i < NAME_MAX_LEN - 1; ++i, ++str) {
> +		current_driver_name[i] = *str;
> +		if (*str == ' ' || *str == '\t' || isspace(*str) || *str == 0)

The comparisons with ' ' and '\t' are unneeded, surely?

> +			break;
> +	}
> +	current_driver_name[i] = 0;
> +
> +	if (current_driver_name[0])
> +		printk(KERN_INFO "DMA-API: enable driver filter for "
> +				 "driver [%s]\n", current_driver_name);
> +
> +
> +	return 1;
> +}
> +__setup("dma_debug_driver=", parse_dma_debug_driver);

The whole thing seems a bit overdone - take a look at some other __setup
functions, see what they're doing.  None of them bother looking for
spaces in the input - higher level code took care of that.

It's conventional for the name of a __setup function to end in "_setup".

Re: [PATCH 3/4] dma-debug: add dma_debug_driver kernel command line

[Joerg Roedel]

On Mon, Jun 01, 2009 at 03:08:13PM -0700, Andrew Morton wrote:
> On Thu, 28 May 2009 17:19:30 +0200
> Joerg Roedel <joerg.roedel@amd.com> wrote:
> 
> > This patch add the dma_debug_driver= boot parameter to enable the driver
> > filter for early boot.
> > 
> 
> Please always update Documentation/kernel-parameters.txt when adding
> new boot options.
> 
> > +static int __init parse_dma_debug_driver(char *str)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < NAME_MAX_LEN - 1; ++i, ++str) {
> > +		current_driver_name[i] = *str;
> > +		if (*str == ' ' || *str == '\t' || isspace(*str) || *str == 0)
> 
> The comparisons with ' ' and '\t' are unneeded, surely?
> 
> > +			break;
> > +	}
> > +	current_driver_name[i] = 0;
> > +
> > +	if (current_driver_name[0])
> > +		printk(KERN_INFO "DMA-API: enable driver filter for "
> > +				 "driver [%s]\n", current_driver_name);
> > +
> > +
> > +	return 1;
> > +}
> > +__setup("dma_debug_driver=", parse_dma_debug_driver);
> 
> The whole thing seems a bit overdone - take a look at some other __setup
> functions, see what they're doing.  None of them bother looking for
> spaces in the input - higher level code took care of that.
> 
> It's conventional for the name of a __setup function to end in "_setup".

Ok, thanks. Will check again here.

Joerg

-- 
           | Advanced Micro Devices GmbH
 Operating | Karl-Hammerschmidt-Str. 34, 85609 Dornach bei München
 System    | 
 Research  | Geschäftsführer: Thomas M. McCoy, Giuliano Meroni
 Center    | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
           | Registergericht München, HRB Nr. 43632

[PATCH 4/4] dma-debug: add documentation for the driver filter

[Joerg Roedel]

This patch adds the driver filter feature to the dma-debug
documentation.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 Documentation/DMA-API.txt |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/Documentation/DMA-API.txt b/Documentation/DMA-API.txt
index d9aa43d..eb6f9d5 100644
--- a/Documentation/DMA-API.txt
+++ b/Documentation/DMA-API.txt
@@ -704,12 +704,23 @@ this directory the following files can currently be found:
 				The current number of free dma_debug_entries
 				in the allocator.
 
+	dma-api/driver-filter
+				You can write a name of driver into this file
+				to limit the debug output to requests from that
+				particular driver. Write an empty string to
+				that file to disable the filter again.
+
 If you have this code compiled into your kernel it will be enabled by default.
 If you want to boot without the bookkeeping anyway you can provide
 'dma_debug=off' as a boot parameter. This will disable DMA-API debugging.
 Notice that you can not enable it again at runtime. You have to reboot to do
 so.
 
+If you want to see debug messages only for a special device driver you can
+specify the dma_debug_driver=<drivername> parameter. This will enable the
+driver filter at boot time. The debug code will only print errors for that
+driver afterwards. This filter can be disabled or changed later using debugfs.
+
 When the code disables itself at runtime this is most likely because it ran
 out of dma_debug_entries. These entries are preallocated at boot. The number
 of preallocated entries is defined per architecture. If it is too low for you
-- 
1.6.3.1

[PATCH 0/4] dma-debug: driver filter v2

[Joerg Roedel]

Hi,

this small patchset adds a new feature to dma-api debugging: a driver
filter. A driver developer can write the name of his driver to
dma-api/driver_filter and the kernel will then only print errors
produced by this driver. This is useful if other drivers in the system
also have bugs. In this case the driver developer can limit the bug
output to messages interesting for him.

	Joerg

Changes since v1:

	- addressed Andrews comments (used string functions were
	  appropriate and reduced inlining, added some more
	  comments)

diffstat:

 Documentation/DMA-API.txt           |   12 +++
 Documentation/kernel-parameters.txt |    7 ++
 lib/dma-debug.c                     |  167 ++++++++++++++++++++++++++++++++++-
 3 files changed, 185 insertions(+), 1 deletions(-)

shortlog:

Joerg Roedel (4):
      dma-debug: add variables and checks for driver filter
      dma-debug: add debugfs file for driver filter
      dma-debug: add dma_debug_driver kernel command line
      dma-debug: add documentation for the driver filter

[PATCH 1/4] dma-debug: add variables and checks for driver filter

[Joerg Roedel]

This patch adds the state variables for the driver filter and a function
to check if the filter is enabled and matches to the current device. The
check is built into the err_printk function.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 lib/dma-debug.c |   49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 48 insertions(+), 1 deletions(-)

diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index cdd205d..c01f647 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -99,6 +99,15 @@ static struct dentry *show_num_errors_dent  __read_mostly;
 static struct dentry *num_free_entries_dent __read_mostly;
 static struct dentry *min_free_entries_dent __read_mostly;
 
+/* per-driver filter related state */
+
+#define NAME_MAX_LEN	64
+
+static char                  current_driver_name[NAME_MAX_LEN] __read_mostly;
+static struct device_driver *current_driver                    __read_mostly;
+
+static DEFINE_RWLOCK(driver_name_lock);
+
 static const char *type2name[4] = { "single", "page",
 				    "scather-gather", "coherent" };
 
@@ -128,9 +137,47 @@ static inline void dump_entry_trace(struct dma_debug_entry *entry)
 #endif
 }
 
+static bool driver_filter(struct device *dev)
+{
+	/* driver filter off */
+	if (likely(!current_driver_name[0]))
+		return true;
+
+	/* driver filter on and initialized */
+	if (current_driver && dev->driver == current_driver)
+		return true;
+
+	/* driver filter on but not yet initialized */
+	if (!current_driver && current_driver_name[0]) {
+		struct device_driver *drv = get_driver(dev->driver);
+		unsigned long flags;
+		bool ret = false;
+
+		if (!drv)
+			return false;
+
+		/* lock to protect against change of current_driver_name */
+		read_lock_irqsave(&driver_name_lock, flags);
+
+		if (drv->name &&
+		    strncmp(current_driver_name, drv->name, 63) == 0) {
+			current_driver = drv;
+			ret = true;
+		}
+
+		read_unlock_irqrestore(&driver_name_lock, flags);
+		put_driver(drv);
+
+		return ret;
+	}
+
+	return false;
+}
+
 #define err_printk(dev, entry, format, arg...) do {		\
 		error_count += 1;				\
-		if (show_all_errors || show_num_errors > 0) {	\
+		if (driver_filter(dev) &&			\
+		    (show_all_errors || show_num_errors > 0)) {	\
 			WARN(1, "%s %s: " format,		\
 			     dev_driver_string(dev),		\
 			     dev_name(dev) , ## arg);		\
-- 
1.6.3.1