[PATCH 0/11] dynamic_debug: allow multiple pending queries on boot-line

[PATCH 0/11] dynamic_debug: allow multiple pending queries on boot-line

[Jim Cromie]

This patchset extends dynamic-debug facility to allow 
use of pr_debug() within a loadable module's module_init() 
function.  Query/rules can be given on the boot-line,
and are saved to a pending list if they cannot be applied
immediately.  Later, when the module is being loaded, the
pending list is scanned, and matching rules are applied.
Thus pr_debug() calls in the module's initialization function
are active when it is invoked.

Other features:
- dynamic_debug.verbose=1 at boot-time or while running
- ddebug_query can specify multiple queries, separated by ';'
- warn on multiple uses of a single match-spec in single query
- tolerate bad queries in ddebug_query w/o taking down facility

Possible additions (not done now)

- <dbgfs>/dynamic_debug/pending
  to show pending query/rules

- scan existing pending list before adding new entry
  currently same rule can be appended multiple times

- persistent queries
  when module is unloaded, applied rules are deleted


[PATCH 01/11] dynamic_debug: allow changing of dynamic_debug verbosity any time
[PATCH 02/11] dynamic_debug: trim source-path prefix from dynamic_debug/control
[PATCH 03/11] dynamic_debug: process multiple commands on a line
[PATCH 04/11] dynamic_debug: warn when >1 of each type of match-spec is given
[PATCH 05/11] dynamic_debug: use pr_info instead of printk(KERN_INFO ..
[PATCH 06/11] dynamic_debug: KERN_ERR should not depend upon verbosity
[PATCH 07/11] dynamic_debug: dont kill entire facility on error parsing ddebug_query
[PATCH 08/11] dynamic_debug: return int from ddebug_change
[PATCH 09/11] dynamic_debug: add_to_pending() saves non-matching queries for later.
[PATCH 10/11] dynamic_debug: call apply_pending_queries from ddebug_add_module
[PATCH 11/11] dynamic_debug: document use of pendinq queries at boot-time

[PATCH 01/11] dynamic_debug: allow changing of dynamic_debug verbosity any time

[Jim Cromie]

allow changing dynamic_debug verbosity
at boot-time, with: dynamic_debug.verbose=1
or at runtime with:
root@voyage:~# echo 1 > /sys/module/dynamic_debug/parameters/verbose

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 75ca78f..a3b08d5 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -58,6 +58,7 @@ struct ddebug_iter {
 static DEFINE_MUTEX(ddebug_lock);
 static LIST_HEAD(ddebug_tables);
 static int verbose = 0;
+module_param(verbose, int, 0744);
 
 /* Return the last part of a pathname */
 static inline const char *basename(const char *path)
-- 
1.7.4.1

[PATCH 02/11] dynamic_debug: trim source-path prefix from dynamic_debug/control

[Jim Cromie]

Skip past unchanging absolute source-path prefix to print a path
thats relative to kernel source's root-dir.  This makes the file
easier to read without a wide screen.  For example:

kernel/freezer.c:128 [freezer]cancel_freezing - "  clean up: %s\012"

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index a3b08d5..eb08a2f 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -601,6 +601,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
 	struct ddebug_iter *iter = m->private;
 	struct _ddebug *dp = p;
 	char flagsbuf[8];
+	const int offset = strlen(__FILE__) - strlen("lib/dynamic_debug.c");
 
 	if (verbose)
 		printk(KERN_INFO "%s: called m=%p p=%p\n",
@@ -613,7 +614,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
 	}
 
 	seq_printf(m, "%s:%u [%s]%s %s \"",
-		   dp->filename, dp->lineno,
+		   dp->filename + offset, dp->lineno,
 		   iter->table->mod_name, dp->function,
 		   ddebug_describe_flags(dp, flagsbuf, sizeof(flagsbuf)));
 	seq_escape(m, dp->format, "\t\r\n\"");
-- 
1.7.4.1

[PATCH 03/11] dynamic_debug: process multiple commands on a line

[Jim Cromie]

Process multiple commands per line, separated by ';'.  All commands are
processed, independent of errors, allowing individual commands to fail,
for example when a module is not installed.  Last error code is returned.
With this, extensive command sets can be given on the boot-line.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 Documentation/dynamic-debug-howto.txt |   14 ++++++++++-
 lib/dynamic_debug.c                   |   39 +++++++++++++++++++++++++++++++-
 2 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/Documentation/dynamic-debug-howto.txt b/Documentation/dynamic-debug-howto.txt
index f959909..d0faf98 100644
--- a/Documentation/dynamic-debug-howto.txt
+++ b/Documentation/dynamic-debug-howto.txt
@@ -92,8 +92,18 @@ nullarbor:~ # echo -c 'file svcsock.c\nline 1603 +p' >
 nullarbor:~ # echo -n 'file svcsock.c line 1603 +p' >
 				<debugfs>/dynamic_debug/control
 
-Commands are bounded by a write() system call.  If you want to do
-multiple commands you need to do a separate "echo" for each, like:
+Commands are bounded by a write() system call.  Subject to this limit
+(or 1024 for boot-line parameter) you can send multiple commands,
+separated by ';'
+
+foo:~ # echo "module nsc_gpio +p ; module pc8736x_gpio +p ; " \
+ "module scx200_gpio +p " > /dbg/dynamic_debug/control
+
+Multiple commands are processed independently, this allows you to send
+commands which may fail, for example if a module is not present.  The
+last failing command returns its error.
+
+Or you can do an "echo" for each, like:
 
 nullarbor:~ # echo 'file svcsock.c line 1603 +p' > /proc/dprintk ;\
 > echo 'file svcsock.c line 1563 +p' > /proc/dprintk
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index eb08a2f..0e567ad 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -428,6 +428,41 @@ static int ddebug_exec_query(char *query_string)
 	return 0;
 }
 
+/* handle multiple queries, continue on error, return last error */
+static int ddebug_exec_queries(char *query)
+{
+	char *split = query;
+	int i, errs = 0, exitcode = 0, rc;
+
+	if (verbose)
+		/* clean up for logging */
+		for (; (split = strpbrk(split, "\t\n")); split++)
+			*split = ' ';
+
+	for (i = 0; query; query = split, i++) {
+
+		split = strchr(query, ';');
+		if (split)
+			*split++ = '\0';
+
+		if (verbose)
+			printk(KERN_INFO "%s: query %d: \"%s\"",
+				__func__, i, query);
+
+		rc = ddebug_exec_query(query);
+		if (rc) {
+			errs++;
+			exitcode = rc;
+		}
+	}
+	if (verbose)
+		printk(KERN_INFO
+			"%s: processed %d queries, with %d errs",
+			__func__, i, errs);
+
+	return exitcode;
+}
+
 int __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...)
 {
 	va_list args;
@@ -492,7 +527,7 @@ static ssize_t ddebug_proc_write(struct file *file, const char __user *ubuf,
 		printk(KERN_INFO "%s: read %d bytes from userspace\n",
 			__func__, (int)len);
 
-	ret = ddebug_exec_query(tmpbuf);
+	ret = ddebug_exec_queries(tmpbuf);
 	if (ret)
 		return ret;
 
@@ -804,7 +839,7 @@ static int __init dynamic_debug_init(void)
 
 	/* ddebug_query boot param got passed -> set it up */
 	if (ddebug_setup_string[0] != '\0') {
-		ret = ddebug_exec_query(ddebug_setup_string);
+		ret = ddebug_exec_queries(ddebug_setup_string);
 		if (ret)
 			pr_warning("Invalid ddebug boot param %s",
 				   ddebug_setup_string);
-- 
1.7.4.1

[PATCH 04/11] dynamic_debug: warn when >1 of each type of match-spec is given

[Jim Cromie]

Issue warnings when any match-spec is given multiple times in a rule.
Code honoredi last one, but was silent about it.  Docs imply only one is
allowed, by saying match-specs are ANDed together, given that module M
cannot match both A and B.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |   19 +++++++++++++++----
 1 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 0e567ad..2505232 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -284,6 +284,14 @@ static char *unescape(char *str)
 	return str;
 }
 
+static inline void check_set(const char **dest, char *src, char *name)
+{
+	if (*dest)
+		pr_warning("match-spec:%s val:%s overridden by %s",
+			name, *dest, src);
+	*dest = src;
+}
+
 /*
  * Parse words[] as a ddebug query specification, which is a series
  * of (keyword, value) pairs chosen from these possibilities:
@@ -295,6 +303,9 @@ static char *unescape(char *str)
  * format <escaped-string-to-find-in-format>
  * line <lineno>
  * line <first-lineno>-<last-lineno> // where either may be empty
+ *
+ * only 1 pair of each type is expected, subsequent ones elicit a
+ * warning, and override the setting.
  */
 static int ddebug_parse_query(char *words[], int nwords,
 			       struct ddebug_query *query)
@@ -308,13 +319,13 @@ static int ddebug_parse_query(char *words[], int nwords,
 
 	for (i = 0 ; i < nwords ; i += 2) {
 		if (!strcmp(words[i], "func"))
-			query->function = words[i+1];
+			check_set(&query->function, words[i+1], "func");
 		else if (!strcmp(words[i], "file"))
-			query->filename = words[i+1];
+			check_set(&query->filename, words[i+1], "file");
 		else if (!strcmp(words[i], "module"))
-			query->module = words[i+1];
+			check_set(&query->module, words[i+1], "module");
 		else if (!strcmp(words[i], "format"))
-			query->format = unescape(words[i+1]);
+			check_set(&query->format, words[i+1], "format");
 		else if (!strcmp(words[i], "line")) {
 			char *first = words[i+1];
 			char *last = strchr(first, '-');
-- 
1.7.4.1

[PATCH 05/11] dynamic_debug: use pr_info instead of printk(KERN_INFO ..

[Jim Cromie]

pr_info was already in use here, update most others.
Use pr_fmt to uniformly add __func__ to all calls.
A few printks were left alone, where line was left un-terminated,
and subsequent printks appended to it.  These need separate attention.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |   55 +++++++++++++++++++++++---------------------------
 1 files changed, 25 insertions(+), 30 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 2505232..17bace8 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -31,6 +31,9 @@
 #include <linux/hardirq.h>
 #include <linux/sched.h>
 
+#undef pr_fmt
+#define pr_fmt(fmt) "%s: " fmt, __func__
+
 extern struct _ddebug __start___verbose[];
 extern struct _ddebug __stop___verbose[];
 
@@ -160,8 +163,7 @@ static void ddebug_change(const struct ddebug_query *query,
 			else
 				dp->enabled = 0;
 			if (verbose)
-				printk(KERN_INFO
-					"ddebug: changed %s:%d [%s]%s %s\n",
+				pr_info("changed %s:%d [%s]%s %s\n",
 					dp->filename, dp->lineno,
 					dt->mod_name, dp->function,
 					ddebug_describe_flags(dp, flagbuf,
@@ -171,7 +173,7 @@ static void ddebug_change(const struct ddebug_query *query,
 	mutex_unlock(&ddebug_lock);
 
 	if (!nfound && verbose)
-		printk(KERN_INFO "ddebug: no matches for query\n");
+		pr_info("no matches for query\n");
 }
 
 /*
@@ -349,9 +351,9 @@ static int ddebug_parse_query(char *words[], int nwords,
 	}
 
 	if (verbose)
-		printk(KERN_INFO "%s: q->function=\"%s\" q->filename=\"%s\" "
+		pr_info("q->function=\"%s\" q->filename=\"%s\" "
 		       "q->module=\"%s\" q->format=\"%s\" q->lineno=%u-%u\n",
-			__func__, query->function, query->filename,
+			query->function, query->filename,
 			query->module, query->format, query->first_lineno,
 			query->last_lineno);
 
@@ -380,7 +382,7 @@ static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
 		return -EINVAL;
 	}
 	if (verbose)
-		printk(KERN_INFO "%s: op='%c'\n", __func__, op);
+		pr_info("op='%c'\n", op);
 
 	for ( ; *str ; ++str) {
 		for (i = ARRAY_SIZE(opt_array) - 1; i >= 0; i--) {
@@ -395,7 +397,7 @@ static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
 	if (flags == 0)
 		return -EINVAL;
 	if (verbose)
-		printk(KERN_INFO "%s: flags=0x%x\n", __func__, flags);
+		pr_info("flags=0x%x\n", flags);
 
 	/* calculate final *flagsp, *maskp according to mask and op */
 	switch (op) {
@@ -413,8 +415,8 @@ static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
 		break;
 	}
 	if (verbose)
-		printk(KERN_INFO "%s: *flagsp=0x%x *maskp=0x%x\n",
-			__func__, *flagsp, *maskp);
+		pr_info("*flagsp=0x%x *maskp=0x%x\n", *flagsp, *maskp);
+
 	return 0;
 }
 
@@ -457,8 +459,7 @@ static int ddebug_exec_queries(char *query)
 			*split++ = '\0';
 
 		if (verbose)
-			printk(KERN_INFO "%s: query %d: \"%s\"",
-				__func__, i, query);
+			pr_info("query %d: \"%s\"\n", i, query);
 
 		rc = ddebug_exec_query(query);
 		if (rc) {
@@ -467,9 +468,7 @@ static int ddebug_exec_queries(char *query)
 		}
 	}
 	if (verbose)
-		printk(KERN_INFO
-			"%s: processed %d queries, with %d errs",
-			__func__, i, errs);
+		pr_info("processed %d queries, with %d errs", i, errs);
 
 	return exitcode;
 }
@@ -507,7 +506,7 @@ static __initdata char ddebug_setup_string[1024];
 static __init int ddebug_setup_query(char *str)
 {
 	if (strlen(str) >= 1024) {
-		pr_warning("ddebug boot param string too large\n");
+		pr_warning("boot param string too large\n");
 		return 0;
 	}
 	strcpy(ddebug_setup_string, str);
@@ -535,8 +534,7 @@ static ssize_t ddebug_proc_write(struct file *file, const char __user *ubuf,
 		return -EFAULT;
 	tmpbuf[len] = '\0';
 	if (verbose)
-		printk(KERN_INFO "%s: read %d bytes from userspace\n",
-			__func__, (int)len);
+		pr_info("read %d bytes from userspace\n", (int)len);
 
 	ret = ddebug_exec_queries(tmpbuf);
 	if (ret)
@@ -599,8 +597,8 @@ static void *ddebug_proc_start(struct seq_file *m, loff_t *pos)
 	int n = *pos;
 
 	if (verbose)
-		printk(KERN_INFO "%s: called m=%p *pos=%lld\n",
-			__func__, m, (unsigned long long)*pos);
+		pr_info("called m=%p *pos=%lld\n",
+			m, (unsigned long long)*pos);
 
 	mutex_lock(&ddebug_lock);
 
@@ -625,8 +623,8 @@ static void *ddebug_proc_next(struct seq_file *m, void *p, loff_t *pos)
 	struct _ddebug *dp;
 
 	if (verbose)
-		printk(KERN_INFO "%s: called m=%p p=%p *pos=%lld\n",
-			__func__, m, p, (unsigned long long)*pos);
+		pr_info("called m=%p p=%p *pos=%lld\n",
+			m, p, (unsigned long long)*pos);
 
 	if (p == SEQ_START_TOKEN)
 		dp = ddebug_iter_first(iter);
@@ -650,8 +648,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
 	const int offset = strlen(__FILE__) - strlen("lib/dynamic_debug.c");
 
 	if (verbose)
-		printk(KERN_INFO "%s: called m=%p p=%p\n",
-			__func__, m, p);
+		pr_info("called m=%p p=%p\n", m, p);
 
 	if (p == SEQ_START_TOKEN) {
 		seq_puts(m,
@@ -676,8 +673,8 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
 static void ddebug_proc_stop(struct seq_file *m, void *p)
 {
 	if (verbose)
-		printk(KERN_INFO "%s: called m=%p p=%p\n",
-			__func__, m, p);
+		pr_info("called m=%p p=%p\n", m, p);
+			
 	mutex_unlock(&ddebug_lock);
 }
 
@@ -700,7 +697,7 @@ static int ddebug_proc_open(struct inode *inode, struct file *file)
 	int err;
 
 	if (verbose)
-		printk(KERN_INFO "%s: called\n", __func__);
+		pr_info("called\n");
 
 	iter = kzalloc(sizeof(*iter), GFP_KERNEL);
 	if (iter == NULL)
@@ -752,8 +749,7 @@ int ddebug_add_module(struct _ddebug *tab, unsigned int n,
 	mutex_unlock(&ddebug_lock);
 
 	if (verbose)
-		printk(KERN_INFO "%u debug prints in module %s\n",
-				 n, dt->mod_name);
+		pr_info("%u debug prints in module %s\n", n, dt->mod_name);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(ddebug_add_module);
@@ -775,8 +771,7 @@ int ddebug_remove_module(const char *mod_name)
 	int ret = -ENOENT;
 
 	if (verbose)
-		printk(KERN_INFO "%s: removing module \"%s\"\n",
-				__func__, mod_name);
+		pr_info("removing module \"%s\"\n", mod_name);
 
 	mutex_lock(&ddebug_lock);
 	list_for_each_entry_safe(dt, nextdt, &ddebug_tables, link) {
-- 
1.7.4.1

[PATCH 06/11] dynamic_debug: KERN_ERR should not depend upon verbosity

[Jim Cromie]

issue keyword/parsing errors even w/o verbose set;
uncover otherwize mysterious non-functionality.
Also convert to pr_err(), which supplies __func__ via pr_fmt.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 17bace8..7cf5fa7 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -343,9 +343,7 @@ static int ddebug_parse_query(char *words[], int nwords,
 				query->last_lineno = query->first_lineno;
 			}
 		} else {
-			if (verbose)
-				printk(KERN_ERR "%s: unknown keyword \"%s\"\n",
-					__func__, words[i]);
+			pr_err("unknown keyword \"%s\"\n", words[i]);
 			return -EINVAL;
 		}
 	}
-- 
1.7.4.1

[PATCH 07/11] dynamic_debug: dont kill entire facility on error parsing ddebug_query

[Jim Cromie]

Currently, a parsing error on ddebug_query results in effective loss
of the facility; all tables are removed, and the init-call returns error.
This is way too severe, especially when an innocent quoting error can
be the cause:

Kernel command line: root=LABEL=ROOT_FS ... ddebug_query='file char_dev.c +p'
yields:

ddebug_exec_queries: query 0: "'file"
query before parse <'file>
ddebug_exec_queries: processed 1 queries, with 1 errs
Invalid ddebug boot param 'file

Fix this by zeroing return-code for parse error before returning.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 7cf5fa7..97f6a9b 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -844,10 +844,11 @@ static int __init dynamic_debug_init(void)
 	/* ddebug_query boot param got passed -> set it up */
 	if (ddebug_setup_string[0] != '\0') {
 		ret = ddebug_exec_queries(ddebug_setup_string);
-		if (ret)
+		if (ret) {
+			ret = 0; /* dont kill entire facility */
 			pr_warning("Invalid ddebug boot param %s",
 				   ddebug_setup_string);
-		else
+		} else
 			pr_info("ddebug initialized with string %s",
 				ddebug_setup_string);
 	}
-- 
1.7.4.1

[PATCH 08/11] dynamic_debug: return int from ddebug_change

[Jim Cromie]

Alter ddebug_change to return number of matches found for query/rule.
This lets caller know whether rule applied, and potentially what
to do next.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 97f6a9b..9b8b1e8 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -102,8 +102,8 @@ static char *ddebug_describe_flags(struct _ddebug *dp, char *buf,
  * the user which ddebug's were changed, or whether none
  * were matched.
  */
-static void ddebug_change(const struct ddebug_query *query,
-			   unsigned int flags, unsigned int mask)
+static int ddebug_change(const struct ddebug_query *query,
+			unsigned int flags, unsigned int mask)
 {
 	int i;
 	struct ddebug_table *dt;
@@ -172,8 +172,7 @@ static void ddebug_change(const struct ddebug_query *query,
 	}
 	mutex_unlock(&ddebug_lock);
 
-	if (!nfound && verbose)
-		pr_info("no matches for query\n");
+	return nfound;
 }
 
 /*
@@ -425,6 +424,7 @@ static int ddebug_exec_query(char *query_string)
 #define MAXWORDS 9
 	int nwords;
 	char *words[MAXWORDS];
+	int nfound;
 
 	nwords = ddebug_tokenize(query_string, words, MAXWORDS);
 	if (nwords <= 0)
@@ -435,7 +435,8 @@ static int ddebug_exec_query(char *query_string)
 		return -EINVAL;
 
 	/* actually go and implement the change */
-	ddebug_change(&query, flags, mask);
+	nfound = ddebug_change(&query, flags, mask);
+
 	return 0;
 }
 
-- 
1.7.4.1

[PATCH 09/11] dynamic_debug: add_to_pending() saves non-matching queries for later.

[Jim Cromie]

When a query/rule doesnt match, call add_to_pending(new function)
to copy the query off the stack, into a (new) struct pending_query,
and add to pending_queries (new) list.

Patch adds: /sys/module/dynamic_debug/parameters/
  verbose - rw, added previously, here for completeness
  pending_ct - ro, shows current count of pending queries
  pending_max - rw, set max pending, further pending queries are rejected

With this and previous patches, queries added on the boot-line which
do not match (because module is not built-in, and thus not present yet)
are added to pending_queries.

IE, with these boot-line additions:
 dynamic_debug.verbose=1 ddebug_query="module scx200_hrt +p"

Kernel will log the following:

ddebug_exec_queries: query 0: "module scx200_hrt +p"
ddebug_tokenize: split into words: "module" "scx200_hrt" "+p"
ddebug_parse_query: parsed q->function="(null)" q->filename="(null)" q->module="scx200_hrt" q->format="(null)" q->lineno=0-0
ddebug_parse_flags: op='+'
ddebug_parse_flags: flags=0x1
ddebug_parse_flags: *flagsp=0x1 *maskp=0xffffffff
ddebug_exec_query: nfound 0 on q->function="(null)" q->filename="(null)" q->module="scx200_hrt" q->format="(null)" q->lineno=0-0
ddebug_add_to_pending: add to pending: q->function="(null)" q->filename="(null)" q->module="scx200_hrt" q->format="(null)" q->lineno=0-0
ddebug_add_to_pending: ddebug: query saved as pending 1

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |   78 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 72 insertions(+), 6 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 9b8b1e8..37d0275 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -53,6 +53,16 @@ struct ddebug_query {
 	unsigned int first_lineno, last_lineno;
 };
 
+struct pending_query {
+	struct list_head link;
+	struct ddebug_query query;
+	char filename[100];
+	char module[30];
+	char function[50];
+	char format[200];
+	unsigned int flags, mask;
+};
+
 struct ddebug_iter {
 	struct ddebug_table *table;
 	unsigned int idx;
@@ -61,7 +71,13 @@ struct ddebug_iter {
 static DEFINE_MUTEX(ddebug_lock);
 static LIST_HEAD(ddebug_tables);
 static int verbose = 0;
-module_param(verbose, int, 0744);
+module_param(verbose, int, 0644);
+
+/* legal but inapplicable queries, save and test against new modules */
+static LIST_HEAD(pending_queries);
+static int pending_ct, pending_max = 100;
+module_param(pending_ct, int, 0444);
+module_param(pending_max, int, 0644);
 
 /* Return the last part of a pathname */
 static inline const char *basename(const char *path)
@@ -96,6 +112,56 @@ static char *ddebug_describe_flags(struct _ddebug *dp, char *buf,
 	return buf;
 }
 
+static char prbuf_query[300];
+
+static char *show_ddebug_query(const struct ddebug_query *q)
+{
+        sprintf(prbuf_query,
+		"q->function=\"%s\" q->filename=\"%s\" "
+		"q->module=\"%s\" q->format=\"%s\" q->lineno=%u-%u",
+		q->function, q->filename, q->module, q->format,
+		q->first_lineno, q->last_lineno);
+
+	return prbuf_query;
+}
+
+/* copy query off stack, save flags & mask, and store in pending-list */
+static void add_to_pending(const struct ddebug_query *query,
+			   unsigned int flags, unsigned int mask)
+{
+	struct pending_query *pq;
+
+	if (pending_ct >= pending_max) {
+		pr_warn("ddebug: no matches for query, pending is full\n");
+		return;
+	}
+	if (verbose)
+		pr_info("add to pending: %s\n", show_ddebug_query(query));
+
+	pending_ct++;
+	pq = kzalloc(sizeof(struct pending_query), GFP_KERNEL);
+
+	/* copy non-null match-specs into allocd mem, update pointers */
+	if (query->module)
+		pq->query.module = strcpy(pq->module, query->module);
+	if (query->function)
+		pq->query.function = strcpy(pq->function, query->function);
+	if (query->filename)
+		pq->query.filename = strcpy(pq->filename, query->filename);
+	if (query->format)
+		pq->query.format = strcpy(pq->format, query->format);
+
+	pq->flags = flags;
+	pq->mask = mask;
+
+	mutex_lock(&ddebug_lock);
+	list_add(&pq->link, &pending_queries);
+	mutex_unlock(&ddebug_lock);
+
+	if (verbose)
+		pr_info("ddebug: query saved as pending %d\n", pending_ct);
+}
+
 /*
  * Search the tables for _ddebug's which match the given
  * `query' and apply the `flags' and `mask' to them.  Tells
@@ -348,11 +414,7 @@ static int ddebug_parse_query(char *words[], int nwords,
 	}
 
 	if (verbose)
-		pr_info("q->function=\"%s\" q->filename=\"%s\" "
-		       "q->module=\"%s\" q->format=\"%s\" q->lineno=%u-%u\n",
-			query->function, query->filename,
-			query->module, query->format, query->first_lineno,
-			query->last_lineno);
+		pr_info("parsed %s\n", show_ddebug_query(query));
 
 	return 0;
 }
@@ -437,6 +499,10 @@ static int ddebug_exec_query(char *query_string)
 	/* actually go and implement the change */
 	nfound = ddebug_change(&query, flags, mask);
 
+	pr_info("nfound %d on %s\n", nfound, show_ddebug_query(&query));
+	if (!nfound)
+		ddebug_add_to_pending(&query, flags, mask);
+
 	return 0;
 }
 
-- 
1.7.4.1

[PATCH 10/11] dynamic_debug: call apply_pending_queries from ddebug_add_module

[Jim Cromie]

When a module is loaded, ddebug_add_module calls apply_pending_queries
to scan pending_queries list and call ddebug_change to apply them.
Matching rules are removed from pending_queries.

With this change, the loaded module's pr_debugs are enabled when
its module_init is invoked, which is not possible otherwize.

With verbose=1, kernel logs look like:

apply_pending_queries: check: pc8736x_gpio <-> q->function="(null)" q->filename="(null)" q->module="pc8736x_gpio" q->format="(null)" q->lineno=0-0 q->flags=0x1 q->mask=0xffffffff

ddebug_change: changed /home/jimc/projects/lx/linux-2.6/drivers/char/pc8736x_gpio.c:342 [pc8736x_gpio]pc8736x_gpio_cleanup p
ddebug_change: changed /home/jimc/projects/lx/linux-2.6/drivers/char/pc8736x_gpio.c:319 [pc8736x_gpio]pc8736x_gpio_init p
...
platform pc8736x_gpio.0: NatSemi pc8736x GPIO Driver Initializing
platform pc8736x_gpio.0: GPIO ioport 6600 reserved

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 37d0275..252888a 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -126,8 +126,8 @@ static char *show_ddebug_query(const struct ddebug_query *q)
 }
 
 /* copy query off stack, save flags & mask, and store in pending-list */
-static void add_to_pending(const struct ddebug_query *query,
-			   unsigned int flags, unsigned int mask)
+static void ddebug_add_to_pending(struct ddebug_query *query,
+				unsigned int flags, unsigned int mask)
 {
 	struct pending_query *pq;
 
@@ -351,6 +351,20 @@ static char *unescape(char *str)
 	return str;
 }
 
+static char *show_pending_query(struct pending_query *pq)
+{
+	struct ddebug_query *dq = &pq->query;
+        sprintf(prbuf_query,
+		"q->function=\"%s\" q->filename=\"%s\" "
+		"q->module=\"%s\" q->format=\"%s\" q->lineno=%u-%u "
+		"q->flags=0x%x q->mask=0x%x\n",
+		dq->function, dq->filename, dq->module, dq->format,
+		dq->first_lineno, dq->last_lineno,
+		pq->flags, pq->mask);
+
+	return prbuf_query;
+}
+
 static inline void check_set(const char **dest, char *src, char *name)
 {
 	if (*dest)
@@ -786,6 +800,35 @@ static const struct file_operations ddebug_proc_fops = {
 	.write = ddebug_proc_write
 };
 
+/* apply matching queries in pending-queries list */
+static void apply_pending_queries(struct ddebug_table *dt)
+{
+	struct pending_query *pq, *pqnext;
+	int nfound;
+
+	if (verbose)
+		pr_info("pending_ct: %d\n", pending_ct);
+
+	list_for_each_entry_safe(pq, pqnext, &pending_queries, link) {
+		
+		if (verbose)
+			pr_info("check: %s <-> %s\n",
+				dt->mod_name, show_pending_query(pq));
+
+		nfound = ddebug_change(&pq->query, pq->flags, pq->mask);
+
+		if (nfound) {
+			mutex_lock(&ddebug_lock);
+			list_del(&pq->link);
+			mutex_unlock(&ddebug_lock);
+			kfree(pq);
+			pending_ct--;
+		}
+		else if (verbose)
+			pr_info("no-match: %s\n", show_pending_query(pq));
+	}
+
+}
 /*
  * Allocate a new ddebug_table for the given module
  * and add it to the global list.
@@ -815,6 +858,9 @@ int ddebug_add_module(struct _ddebug *tab, unsigned int n,
 
 	if (verbose)
 		pr_info("%u debug prints in module %s\n", n, dt->mod_name);
+
+	apply_pending_queries(dt);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(ddebug_add_module);
-- 
1.7.4.1

[PATCH 11/11] dynamic_debug: document use of pendinq queries at boot-time

[Jim Cromie]

Add paragraph that pendinq-queries are checked when modules are loaded,
so debug statements can be used in module_init functions.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 Documentation/dynamic-debug-howto.txt |   20 ++++++++++++++------
 1 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/Documentation/dynamic-debug-howto.txt b/Documentation/dynamic-debug-howto.txt
index d0faf98..49b75b81 100644
--- a/Documentation/dynamic-debug-howto.txt
+++ b/Documentation/dynamic-debug-howto.txt
@@ -120,11 +120,12 @@ specifications, followed by a flags change specification.
 
 command ::= match-spec* flags-spec
 
-The match-spec's are used to choose a subset of the known dprintk()
+The match-specs are used to choose a subset of the known dprintk()
 callsites to which to apply the flags-spec.  Think of them as a query
-with implicit ANDs between each pair.  Note that an empty list of
-match-specs is possible, but is not very useful because it will not
-match any debug statement callsites.
+with implicit ANDs between each pair.  This means that multiple specs
+of a given type are nonsense; a module cannot match A and B
+simultaneously.  Note that an empty list of match-specs is legal but
+pointless, it will not match any debug statement callsites.
 
 A match specification comprises a keyword, which controls the attribute
 of the callsite to be compared, and a value to compare against.  Possible
@@ -242,13 +243,20 @@ QUERY follows the syntax described above, but must not exceed 1023
 characters. The enablement of debug messages is done as an arch_initcall.
 Thus you can enable debug messages in all code processed after this
 arch_initcall via this boot parameter.
-On an x86 system for example ACPI enablement is a subsys_initcall and
-ddebug_query="file ec.c +p"
+
+On an x86 system for example ACPI enablement is a subsys_initcall, so
+  ddebug_query="file ec.c +p"
 will show early Embedded Controller transactions during ACPI setup if
 your machine (typically a laptop) has an Embedded Controller.
 PCI (or other devices) initialization also is a hot candidate for using
 this boot parameter for debugging purposes.
 
+You can also give queries for modules which are not yet loaded, but
+will be, via udev or /etc/modules.  These queries are saved to a
+pending-queries list, and are applied when the module is loaded, and
+before the module's init function is invoked.  Note that modules which
+have no debug messages do not trigger this, so queries for them will
+remain on the pending-list until reboot.
 
 Examples
 ========
-- 
1.7.4.1

Re: [PATCH 02/11] dynamic_debug: trim source-path prefix from dynamic_debug/control

[Bart Van Assche]

On Tue, Jun 28, 2011 at 9:09 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
>
> Skip past unchanging absolute source-path prefix to print a path
> thats relative to kernel source's root-dir.  This makes the file
> easier to read without a wide screen.  For example:
>
> kernel/freezer.c:128 [freezer]cancel_freezing - "  clean up: %s\012"
>
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
>  lib/dynamic_debug.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index a3b08d5..eb08a2f 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -601,6 +601,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
>        struct ddebug_iter *iter = m->private;
>        struct _ddebug *dp = p;
>        char flagsbuf[8];
> +       const int offset = strlen(__FILE__) - strlen("lib/dynamic_debug.c");
>
>        if (verbose)
>                printk(KERN_INFO "%s: called m=%p p=%p\n",
> @@ -613,7 +614,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
>        }
>
>        seq_printf(m, "%s:%u [%s]%s %s \"",
> -                  dp->filename, dp->lineno,
> +                  dp->filename + offset, dp->lineno,
>                   iter->table->mod_name, dp->function,
>                   ddebug_describe_flags(dp, flagsbuf, sizeof(flagsbuf)));
>        seq_escape(m, dp->format, "\t\r\n\"");

The above will break the output generated for out-of-tree kernel
modules that use the dynamic debug facility.

Bart.

Re: [PATCH 04/11] dynamic_debug: warn when >1 of each type of match-spec is given

[Bart Van Assche]

On Tue, Jun 28, 2011 at 9:09 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
> -                       query->format = unescape(words[i+1]);
> +                       check_set(&query->format, words[i+1], "format");

Was it your intention to remove the call to "unescape" ?

Bart.

Re: [PATCH 08/11] dynamic_debug: return int from ddebug_change

[Bart Van Assche]

On Tue, Jun 28, 2011 at 9:09 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
> @@ -425,6 +424,7 @@ static int ddebug_exec_query(char *query_string)
>  #define MAXWORDS 9
>        int nwords;
>        char *words[MAXWORDS];
> +       int nfound;
>
>        nwords = ddebug_tokenize(query_string, words, MAXWORDS);
>        if (nwords <= 0)
> @@ -435,7 +435,8 @@ static int ddebug_exec_query(char *query_string)
>                return -EINVAL;
>
>        /* actually go and implement the change */
> -       ddebug_change(&query, flags, mask);
> +       nfound = ddebug_change(&query, flags, mask);
> +
>        return 0;

Do these changes actually do anything, or did I miss something ?

Bart.

Re: [PATCH 09/11] dynamic_debug: add_to_pending() saves non-matching queries for later.

[Jason Baron]

Hi Jim,

A number of ppl have expressed interest to me in this functionality...

I'm wondering if we should call out this usage explicitly with a new
flag like '+a'. Meaning 'add' as a pending query if it doesn't match
anything existing.

In this way, we can have error when things don't match in the normal case.
Otherwise, somebody might think they've enabled something when they've in fact
added a pending query.

Otherwise, it looks pretty good.

thanks,

-Jason


On Tue, Jun 28, 2011 at 01:09:50AM -0600, Jim Cromie wrote:
> When a query/rule doesnt match, call add_to_pending(new function)
> to copy the query off the stack, into a (new) struct pending_query,
> and add to pending_queries (new) list.
> 
> Patch adds: /sys/module/dynamic_debug/parameters/
>   verbose - rw, added previously, here for completeness
>   pending_ct - ro, shows current count of pending queries
>   pending_max - rw, set max pending, further pending queries are rejected
> 
> With this and previous patches, queries added on the boot-line which
> do not match (because module is not built-in, and thus not present yet)
> are added to pending_queries.
> 
> IE, with these boot-line additions:
>  dynamic_debug.verbose=1 ddebug_query="module scx200_hrt +p"
> 
> Kernel will log the following:
> 
> ddebug_exec_queries: query 0: "module scx200_hrt +p"
> ddebug_tokenize: split into words: "module" "scx200_hrt" "+p"
> ddebug_parse_query: parsed q->function="(null)" q->filename="(null)" q->module="scx200_hrt" q->format="(null)" q->lineno=0-0
> ddebug_parse_flags: op='+'
> ddebug_parse_flags: flags=0x1
> ddebug_parse_flags: *flagsp=0x1 *maskp=0xffffffff
> ddebug_exec_query: nfound 0 on q->function="(null)" q->filename="(null)" q->module="scx200_hrt" q->format="(null)" q->lineno=0-0
> ddebug_add_to_pending: add to pending: q->function="(null)" q->filename="(null)" q->module="scx200_hrt" q->format="(null)" q->lineno=0-0
> ddebug_add_to_pending: ddebug: query saved as pending 1
> 
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
>  lib/dynamic_debug.c |   78 +++++++++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 72 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 9b8b1e8..37d0275 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -53,6 +53,16 @@ struct ddebug_query {
>  	unsigned int first_lineno, last_lineno;
>  };
>  
> +struct pending_query {
> +	struct list_head link;
> +	struct ddebug_query query;
> +	char filename[100];
> +	char module[30];
> +	char function[50];
> +	char format[200];
> +	unsigned int flags, mask;
> +};
> +
>  struct ddebug_iter {
>  	struct ddebug_table *table;
>  	unsigned int idx;
> @@ -61,7 +71,13 @@ struct ddebug_iter {
>  static DEFINE_MUTEX(ddebug_lock);
>  static LIST_HEAD(ddebug_tables);
>  static int verbose = 0;
> -module_param(verbose, int, 0744);
> +module_param(verbose, int, 0644);
> +
> +/* legal but inapplicable queries, save and test against new modules */
> +static LIST_HEAD(pending_queries);
> +static int pending_ct, pending_max = 100;
> +module_param(pending_ct, int, 0444);
> +module_param(pending_max, int, 0644);
>  
>  /* Return the last part of a pathname */
>  static inline const char *basename(const char *path)
> @@ -96,6 +112,56 @@ static char *ddebug_describe_flags(struct _ddebug *dp, char *buf,
>  	return buf;
>  }
>  
> +static char prbuf_query[300];
> +
> +static char *show_ddebug_query(const struct ddebug_query *q)
> +{
> +        sprintf(prbuf_query,
> +		"q->function=\"%s\" q->filename=\"%s\" "
> +		"q->module=\"%s\" q->format=\"%s\" q->lineno=%u-%u",
> +		q->function, q->filename, q->module, q->format,
> +		q->first_lineno, q->last_lineno);
> +
> +	return prbuf_query;
> +}
> +
> +/* copy query off stack, save flags & mask, and store in pending-list */
> +static void add_to_pending(const struct ddebug_query *query,
> +			   unsigned int flags, unsigned int mask)
> +{
> +	struct pending_query *pq;
> +
> +	if (pending_ct >= pending_max) {
> +		pr_warn("ddebug: no matches for query, pending is full\n");
> +		return;
> +	}
> +	if (verbose)
> +		pr_info("add to pending: %s\n", show_ddebug_query(query));
> +
> +	pending_ct++;
> +	pq = kzalloc(sizeof(struct pending_query), GFP_KERNEL);
> +
> +	/* copy non-null match-specs into allocd mem, update pointers */
> +	if (query->module)
> +		pq->query.module = strcpy(pq->module, query->module);
> +	if (query->function)
> +		pq->query.function = strcpy(pq->function, query->function);
> +	if (query->filename)
> +		pq->query.filename = strcpy(pq->filename, query->filename);
> +	if (query->format)
> +		pq->query.format = strcpy(pq->format, query->format);
> +
> +	pq->flags = flags;
> +	pq->mask = mask;
> +
> +	mutex_lock(&ddebug_lock);
> +	list_add(&pq->link, &pending_queries);
> +	mutex_unlock(&ddebug_lock);
> +
> +	if (verbose)
> +		pr_info("ddebug: query saved as pending %d\n", pending_ct);
> +}
> +
>  /*
>   * Search the tables for _ddebug's which match the given
>   * `query' and apply the `flags' and `mask' to them.  Tells
> @@ -348,11 +414,7 @@ static int ddebug_parse_query(char *words[], int nwords,
>  	}
>  
>  	if (verbose)
> -		pr_info("q->function=\"%s\" q->filename=\"%s\" "
> -		       "q->module=\"%s\" q->format=\"%s\" q->lineno=%u-%u\n",
> -			query->function, query->filename,
> -			query->module, query->format, query->first_lineno,
> -			query->last_lineno);
> +		pr_info("parsed %s\n", show_ddebug_query(query));
>  
>  	return 0;
>  }
> @@ -437,6 +499,10 @@ static int ddebug_exec_query(char *query_string)
>  	/* actually go and implement the change */
>  	nfound = ddebug_change(&query, flags, mask);
>  
> +	pr_info("nfound %d on %s\n", nfound, show_ddebug_query(&query));
> +	if (!nfound)
> +		ddebug_add_to_pending(&query, flags, mask);
> +
>  	return 0;
>  }
>  
> -- 
> 1.7.4.1
> 

Re: [PATCH 08/11] dynamic_debug: return int from ddebug_change

[Jason Baron]

On Tue, Jun 28, 2011 at 12:24:56PM +0200, Bart Van Assche wrote:
> On Tue, Jun 28, 2011 at 9:09 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
> > @@ -425,6 +424,7 @@ static int ddebug_exec_query(char *query_string)
> >  #define MAXWORDS 9
> >        int nwords;
> >        char *words[MAXWORDS];
> > +       int nfound;
> >
> >        nwords = ddebug_tokenize(query_string, words, MAXWORDS);
> >        if (nwords <= 0)
> > @@ -435,7 +435,8 @@ static int ddebug_exec_query(char *query_string)
> >                return -EINVAL;
> >
> >        /* actually go and implement the change */
> > -       ddebug_change(&query, flags, mask);
> > +       nfound = ddebug_change(&query, flags, mask);
> > +
> >        return 0;
> 
> Do these changes actually do anything, or did I miss something ?
> 
> Bart.

its used in a subsequent patch to decide whether or not to call
add_to_pending.

thanks,

-Jason

Re: [PATCH 04/11] dynamic_debug: warn when >1 of each type of match-spec is given

[Jim Cromie]

On Tue, Jun 28, 2011 at 4:17 AM, Bart Van Assche <bvanassche@acm.org> wrote:
> On Tue, Jun 28, 2011 at 9:09 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
>> -                       query->format = unescape(words[i+1]);
>> +                       check_set(&query->format, words[i+1], "format");
>
> Was it your intention to remove the call to "unescape" ?
>
> Bart.
>

No, that was accidental. thanks for pointing it out.

Re: [PATCH 08/11] dynamic_debug: return int from ddebug_change

[Bart Van Assche]

On Tue, Jun 28, 2011 at 4:54 PM, Jason Baron <jbaron@redhat.com> wrote:
> On Tue, Jun 28, 2011 at 12:24:56PM +0200, Bart Van Assche wrote:
> > On Tue, Jun 28, 2011 at 9:09 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
> > > @@ -425,6 +424,7 @@ static int ddebug_exec_query(char *query_string)
> > >  #define MAXWORDS 9
> > >        int nwords;
> > >        char *words[MAXWORDS];
> > > +       int nfound;
> > >
> > >        nwords = ddebug_tokenize(query_string, words, MAXWORDS);
> > >        if (nwords <= 0)
> > > @@ -435,7 +435,8 @@ static int ddebug_exec_query(char *query_string)
> > >                return -EINVAL;
> > >
> > >        /* actually go and implement the change */
> > > -       ddebug_change(&query, flags, mask);
> > > +       nfound = ddebug_change(&query, flags, mask);
> > > +
> > >        return 0;
> >
> > Do these changes actually do anything, or did I miss something ?
>
> its used in a subsequent patch to decide whether or not to call
> add_to_pending.

As far as I can see your comment applies to the function
ddebug_change() while my comment applies to the function
ddebug_exec_query(). If you have a close look at the above changes you
will see that these do nothing more than adding a dead assignment.

Bart.

Re: [PATCH 08/11] dynamic_debug: return int from ddebug_change

[Jim Cromie]

On Tue, Jun 28, 2011 at 11:48 AM, Bart Van Assche <bvanassche@acm.org> wrote:
> On Tue, Jun 28, 2011 at 4:54 PM, Jason Baron <jbaron@redhat.com> wrote:
>> On Tue, Jun 28, 2011 at 12:24:56PM +0200, Bart Van Assche wrote:
>> > On Tue, Jun 28, 2011 at 9:09 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
>> > > @@ -425,6 +424,7 @@ static int ddebug_exec_query(char *query_string)
>> > >  #define MAXWORDS 9
>> > >        int nwords;
>> > >        char *words[MAXWORDS];
>> > > +       int nfound;
>> > >
>> > >        nwords = ddebug_tokenize(query_string, words, MAXWORDS);
>> > >        if (nwords <= 0)
>> > > @@ -435,7 +435,8 @@ static int ddebug_exec_query(char *query_string)
>> > >                return -EINVAL;
>> > >
>> > >        /* actually go and implement the change */
>> > > -       ddebug_change(&query, flags, mask);
>> > > +       nfound = ddebug_change(&query, flags, mask);
>> > > +
>> > >        return 0;
>> >
>> > Do these changes actually do anything, or did I miss something ?
>>
>> its used in a subsequent patch to decide whether or not to call
>> add_to_pending.
>
> As far as I can see your comment applies to the function
> ddebug_change() while my comment applies to the function
> ddebug_exec_query(). If you have a close look at the above changes you
> will see that these do nothing more than adding a dead assignment.
>
> Bart.
>

its dead in 8/11, but used in 9/11

 	nfound = ddebug_change(&query, flags, mask);

+	pr_info("nfound %d on %s\n", nfound, show_ddebug_query(&query));
+	if (!nfound)
+		ddebug_add_to_pending(&query, flags, mask);
+
 	return 0;

I can merge 8 & 9 if it matters,
but ISTM that the important part is that the patchset is bisectable.

- Things compile at each patch (with a couple warnings,
including unescape, which I fat-fingered and will fix)
$ for i in `seq 1 10`; do (cd - && git checkout HEAD~1 && git status);
make; done

- and functionality doesnt regress
AFAIK - I may have botched something while rebasing bits and pieces.

Re: [PATCH 02/11] dynamic_debug: trim source-path prefix from dynamic_debug/control

[Jim Cromie]

On Tue, Jun 28, 2011 at 4:08 AM, Bart Van Assche <bvanassche@acm.org> wrote:
> On Tue, Jun 28, 2011 at 9:09 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
>>
>> Skip past unchanging absolute source-path prefix to print a path
>> thats relative to kernel source's root-dir.  This makes the file
>> easier to read without a wide screen.  For example:
>>
>> kernel/freezer.c:128 [freezer]cancel_freezing - "  clean up: %s\012"


>
> The above will break the output generated for out-of-tree kernel
> modules that use the dynamic debug facility.
>
> Bart.
>

ack, yes.

I'll add a check to see prefix matches before skipping it.
this will reduce efficiency slightly more, but its not a hot path.

thanks.

Re: [PATCH 09/11] dynamic_debug: add_to_pending() saves non-matching queries for later.

[Jim Cromie]

On Tue, Jun 28, 2011 at 1:09 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
> When a query/rule doesnt match, call add_to_pending(new function)
> to copy the query off the stack, into a (new) struct pending_query,
> and add to pending_queries (new) list.
>
> Patch adds: /sys/module/dynamic_debug/parameters/
>  verbose - rw, added previously, here for completeness
>  pending_ct - ro, shows current count of pending queries
>  pending_max - rw, set max pending, further pending queries are rejected
>
> With this and previous patches, queries added on the boot-line which
> do not match (because module is not built-in, and thus not present yet)
> are added to pending_queries.
>
> IE, with these boot-line additions:
>  dynamic_debug.verbose=1 ddebug_query="module scx200_hrt +p"
>
> Kernel will log the following:
>
> ddebug_exec_queries: query 0: "module scx200_hrt +p"
> ddebug_tokenize: split into words: "module" "scx200_hrt" "+p"
> ddebug_parse_query: parsed q->function="(null)" q->filename="(null)" q->module="scx200_hrt" q->format="(null)" q->lineno=0-0
> ddebug_parse_flags: op='+'
> ddebug_parse_flags: flags=0x1
> ddebug_parse_flags: *flagsp=0x1 *maskp=0xffffffff
> ddebug_exec_query: nfound 0 on q->function="(null)" q->filename="(null)" q->module="scx200_hrt" q->format="(null)" q->lineno=0-0
> ddebug_add_to_pending: add to pending: q->function="(null)" q->filename="(null)" q->module="scx200_hrt" q->format="(null)" q->lineno=0-0
> ddebug_add_to_pending: ddebug: query saved as pending 1
>
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
>  lib/dynamic_debug.c |   78 +++++++++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 72 insertions(+), 6 deletions(-)
>
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 9b8b1e8..37d0275 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -53,6 +53,16 @@ struct ddebug_query {
>        unsigned int first_lineno, last_lineno;
>  };
>
> +struct pending_query {
> +       struct list_head link;
> +       struct ddebug_query query;
> +       char filename[100];
> +       char module[30];
> +       char function[50];
> +       char format[200];
> +       unsigned int flags, mask;
> +};
> +
>  struct ddebug_iter {
>        struct ddebug_table *table;
>        unsigned int idx;
> @@ -61,7 +71,13 @@ struct ddebug_iter {
>  static DEFINE_MUTEX(ddebug_lock);
>  static LIST_HEAD(ddebug_tables);
>  static int verbose = 0;
> -module_param(verbose, int, 0744);
> +module_param(verbose, int, 0644);
> +
> +/* legal but inapplicable queries, save and test against new modules */
> +static LIST_HEAD(pending_queries);
> +static int pending_ct, pending_max = 100;
> +module_param(pending_ct, int, 0444);
> +module_param(pending_max, int, 0644);
>
>  /* Return the last part of a pathname */
>  static inline const char *basename(const char *path)
> @@ -96,6 +112,56 @@ static char *ddebug_describe_flags(struct _ddebug *dp, char *buf,
>        return buf;
>  }
>
> +static char prbuf_query[300];
> +
> +static char *show_ddebug_query(const struct ddebug_query *q)
> +{


this part (the static fixed-length var)
gives me some heartburn.

I almost went with macros using pr_info to replace the
 2 show_*_query fns, but a <debugfs/dynamic_debug/pending file
might want to use show_pending_query

advice ?

Re: [PATCH 01/11] dynamic_debug: allow changing of dynamic_debug verbosity any time

[Bart Van Assche]

On Tue, Jun 28, 2011 at 9:09 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
> allow changing dynamic_debug verbosity
> at boot-time, with: dynamic_debug.verbose=1
> or at runtime with:
> root@voyage:~# echo 1 > /sys/module/dynamic_debug/parameters/verbose
>
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
>  lib/dynamic_debug.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 75ca78f..a3b08d5 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -58,6 +58,7 @@ struct ddebug_iter {
>  static DEFINE_MUTEX(ddebug_lock);
>  static LIST_HEAD(ddebug_tables);
>  static int verbose = 0;
> +module_param(verbose, int, 0744);

Why 0744 and not 0644 ? Why to set the 'executable' bit ?

Bart.

Re: [PATCH 03/11] dynamic_debug: process multiple commands on a line

[Bart Van Assche]

On Tue, Jun 28, 2011 at 9:09 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
> Process multiple commands per line, separated by ';'.  All commands are
> processed, independent of errors, allowing individual commands to fail,
> for example when a module is not installed.  Last error code is returned.
> With this, extensive command sets can be given on the boot-line.
>
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
>  Documentation/dynamic-debug-howto.txt |   14 ++++++++++-
>  lib/dynamic_debug.c                   |   39 +++++++++++++++++++++++++++++++-
>  2 files changed, 49 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/dynamic-debug-howto.txt b/Documentation/dynamic-debug-howto.txt
> index f959909..d0faf98 100644
> --- a/Documentation/dynamic-debug-howto.txt
> +++ b/Documentation/dynamic-debug-howto.txt
> @@ -92,8 +92,18 @@ nullarbor:~ # echo -c 'file svcsock.c\nline 1603 +p' >
>  nullarbor:~ # echo -n 'file svcsock.c line 1603 +p' >
>                                <debugfs>/dynamic_debug/control
>
> -Commands are bounded by a write() system call.  If you want to do
> -multiple commands you need to do a separate "echo" for each, like:
> +Commands are bounded by a write() system call.  Subject to this limit
> +(or 1024 for boot-line parameter) you can send multiple commands,
> +separated by ';'
> +
> +foo:~ # echo "module nsc_gpio +p ; module pc8736x_gpio +p ; " \
> + "module scx200_gpio +p " > /dbg/dynamic_debug/control
> +
> +Multiple commands are processed independently, this allows you to send
> +commands which may fail, for example if a module is not present.  The
> +last failing command returns its error.
> +
> +Or you can do an "echo" for each, like:
>
>  nullarbor:~ # echo 'file svcsock.c line 1603 +p' > /proc/dprintk ;\
>  > echo 'file svcsock.c line 1563 +p' > /proc/dprintk
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index eb08a2f..0e567ad 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -428,6 +428,41 @@ static int ddebug_exec_query(char *query_string)
>        return 0;
>  }
>
> +/* handle multiple queries, continue on error, return last error */
> +static int ddebug_exec_queries(char *query)
> +{
> +       char *split = query;
> +       int i, errs = 0, exitcode = 0, rc;
> +
> +       if (verbose)
> +               /* clean up for logging */
> +               for (; (split = strpbrk(split, "\t\n")); split++)
> +                       *split = ' ';

The above will join multiple lines into a single line and hence will
cause a shell statement like the one below to be interpret as a single
query:

printf "module nsc_gpio +p\n module pc8736x_gpio +p\n" > /proc/dprintk

Are you sure that's how things should work ?

> +       for (i = 0; query; query = split, i++) {
> +

No blank line past "for" please.

> +               split = strchr(query, ';');
> +               if (split)
> +                       *split++ = '\0';
> +
> +               if (verbose)
> +                       printk(KERN_INFO "%s: query %d: \"%s\"",
> +                               __func__, i, query);
> +
> +               rc = ddebug_exec_query(query);
> +               if (rc) {
> +                       errs++;
> +                       exitcode = rc;
> +               }
> +       }
> +       if (verbose)
> +               printk(KERN_INFO
> +                       "%s: processed %d queries, with %d errs",
> +                       __func__, i, errs);
> +
> +       return exitcode;
> +}
> +
>  int __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...)
>  {
>        va_list args;
> @@ -492,7 +527,7 @@ static ssize_t ddebug_proc_write(struct file *file, const char __user *ubuf,
>                printk(KERN_INFO "%s: read %d bytes from userspace\n",
>                        __func__, (int)len);
>
> -       ret = ddebug_exec_query(tmpbuf);
> +       ret = ddebug_exec_queries(tmpbuf);
>        if (ret)
>                return ret;
>
> @@ -804,7 +839,7 @@ static int __init dynamic_debug_init(void)
>
>        /* ddebug_query boot param got passed -> set it up */
>        if (ddebug_setup_string[0] != '\0') {
> -               ret = ddebug_exec_query(ddebug_setup_string);
> +               ret = ddebug_exec_queries(ddebug_setup_string);
>                if (ret)
>                        pr_warning("Invalid ddebug boot param %s",
>                                   ddebug_setup_string);
> --
> 1.7.4.1
>
>

Re: [PATCH 01/11] dynamic_debug: allow changing of dynamic_debug verbosity any time

[Greg KH]

On Wed, Jun 29, 2011 at 12:41:18PM +0200, Bart Van Assche wrote:
> On Tue, Jun 28, 2011 at 9:09 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
> > allow changing dynamic_debug verbosity
> > at boot-time, with: dynamic_debug.verbose=1
> > or at runtime with:
> > root@voyage:~# echo 1 > /sys/module/dynamic_debug/parameters/verbose
> >
> > Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> > ---
> >  lib/dynamic_debug.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index 75ca78f..a3b08d5 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -58,6 +58,7 @@ struct ddebug_iter {
> >  static DEFINE_MUTEX(ddebug_lock);
> >  static LIST_HEAD(ddebug_tables);
> >  static int verbose = 0;
> > +module_param(verbose, int, 0744);
> 
> Why 0744 and not 0644 ? Why to set the 'executable' bit ?

Yeah, that's wrong, Jim, care to fix this?

Re: [PATCH 01/11] dynamic_debug: allow changing of dynamic_debug verbosity any time

[Jim Cromie]

On Fri, Jul 1, 2011 at 3:57 PM, Greg KH <greg@kroah.com> wrote:
> On Wed, Jun 29, 2011 at 12:41:18PM +0200, Bart Van Assche wrote:
>> On Tue, Jun 28, 2011 at 9:09 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
>> > allow changing dynamic_debug verbosity
>> > at boot-time, with: dynamic_debug.verbose=1
>> > or at runtime with:
>> > root@voyage:~# echo 1 > /sys/module/dynamic_debug/parameters/verbose
>> >
>> > Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
>> > ---
>> >  lib/dynamic_debug.c |    1 +
>> >  1 files changed, 1 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
>> > index 75ca78f..a3b08d5 100644
>> > --- a/lib/dynamic_debug.c
>> > +++ b/lib/dynamic_debug.c
>> > @@ -58,6 +58,7 @@ struct ddebug_iter {
>> >  static DEFINE_MUTEX(ddebug_lock);
>> >  static LIST_HEAD(ddebug_tables);
>> >  static int verbose = 0;
>> > +module_param(verbose, int, 0744);
>>
>> Why 0744 and not 0644 ? Why to set the 'executable' bit ?
>
> Yeah, that's wrong, Jim, care to fix this?
>
>

yes, sorry for not ackg.
Id had it right on the pending_ct and pending_max,
this one was a d'oh

second set forthcoming, with some of the refinements
to pending queries that Jason wanted.

Re: [PATCH 0/11] dynamic_debug: allow multiple pending queries on boot-line

[Bart Van Assche]

On Tue, Jun 28, 2011 at 9:09 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
> This patchset extends dynamic-debug facility to allow
> use of pr_debug() within a loadable module's module_init()
> function.

Can you please clean up checkpatch complaints before resubmitting ?
This is what checkpatch reports on the current patch set:

ERROR: code indent should use tabs where possible
#125: FILE: lib/dynamic_debug.c:119:
+        sprintf(prbuf_query,$

WARNING: please, no spaces at the start of a line
#125: FILE: lib/dynamic_debug.c:119:
+        sprintf(prbuf_query,$

ERROR: code indent should use tabs where possible
#211: FILE: lib/dynamic_debug.c:357:
+        sprintf(prbuf_query,$

WARNING: please, no spaces at the start of a line
#211: FILE: lib/dynamic_debug.c:357:
+        sprintf(prbuf_query,$

ERROR: trailing whitespace
#442: FILE: lib/dynamic_debug.c:756:
+^I^I^I$

ERROR: trailing whitespace
#469: FILE: lib/dynamic_debug.c:813:
+^I^I$

ERROR: else should follow close brace '}'
#483: FILE: lib/dynamic_debug.c:827:
+               }
+               else if (verbose)

Thanks,

Bart.

Re: [PATCH 09/11] dynamic_debug: add_to_pending() saves non-matching queries for later.

[Bart Van Assche]

On Tue, Jun 28, 2011 at 9:17 PM, Jim Cromie <jim.cromie@gmail.com> wrote:
> On Tue, Jun 28, 2011 at 1:09 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
>> +static char prbuf_query[300];
>> +
>> +static char *show_ddebug_query(const struct ddebug_query *q)
>> +{
>
>
> this part (the static fixed-length var)
> gives me some heartburn.
>
> I almost went with macros using pr_info to replace the
>  2 show_*_query fns, but a <debugfs/dynamic_debug/pending file
> might want to use show_pending_query
>
> advice ?

How about using kasprintf() inside show_ddebug_query() and let the
show_ddebug_query() free the buffer that has been allocated by
kasprintf() ?

Bart.

Re: [PATCH 09/11] dynamic_debug: add_to_pending() saves non-matching queries for later.

[Bart Van Assche]

On Tue, Jun 28, 2011 at 4:48 PM, Jason Baron <jbaron@redhat.com> wrote:
> A number of ppl have expressed interest to me in this functionality...
>
> I'm wondering if we should call out this usage explicitly with a new
> flag like '+a'. Meaning 'add' as a pending query if it doesn't match
> anything existing.
>
> In this way, we can have error when things don't match in the normal case.
> Otherwise, somebody might think they've enabled something when they've in fact
> added a pending query.

That suggestion makes sense to me - I think it's better that the user
makes it explicit whether or not a query should be added to the
pending list instead of the ddebug code trying to find out which list
a query should be added to.

Bart.

Re: [PATCH 09/11] dynamic_debug: add_to_pending() saves non-matching queries for later.

[Bart Van Assche]

On Tue, Jun 28, 2011 at 9:09 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
>  pending_max - rw, set max pending, further pending queries are rejected

It's not clear to me why this kernel module parameter has been
introduced ? I'm afraid that this parameter will frustrate users. My
suggestion is to leave it out entirely.

Bart.

Re: [PATCH 10/11] dynamic_debug: call apply_pending_queries from ddebug_add_module

[Bart Van Assche]

On Tue, Jun 28, 2011 at 9:09 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
> +/* apply matching queries in pending-queries list */
> +static void apply_pending_queries(struct ddebug_table *dt)
> +{
> +       struct pending_query *pq, *pqnext;
> +       int nfound;
> +
> +       if (verbose)
> +               pr_info("pending_ct: %d\n", pending_ct);
> +
> +       list_for_each_entry_safe(pq, pqnext, &pending_queries, link) {
> +
> +               if (verbose)
> +                       pr_info("check: %s <-> %s\n",
> +                               dt->mod_name, show_pending_query(pq));
> +
> +               nfound = ddebug_change(&pq->query, pq->flags, pq->mask);
> +
> +               if (nfound) {
> +                       mutex_lock(&ddebug_lock);
> +                       list_del(&pq->link);
> +                       mutex_unlock(&ddebug_lock);
> +                       kfree(pq);
> +                       pending_ct--;
> +               }
> +               else if (verbose)
> +                       pr_info("no-match: %s\n", show_pending_query(pq));
> +       }

The above code doesn't look thread-safe to me. List walking, list
deletion and pending_ct manipulation all should be protected by the
ddebug_lock mutex instead of only list deletion.

Also, why to remove pending entries once a match has been found ? The
resulting behavior will be that when unloading and reloading a kernel
module repeatedly that only the first time a kernel module is loaded
the matching dynamic debug statements in the loaded module will be
enabled.

Bart.

Re: [PATCH 03/11] dynamic_debug: process multiple commands on a line

[Jim Cromie]

On Wed, Jun 29, 2011 at 4:50 AM, Bart Van Assche <bvanassche@acm.org> wrote:
> On Tue, Jun 28, 2011 at 9:09 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
>> Process multiple commands per line, separated by ';'.  All commands are
>> processed, independent of errors, allowing individual commands to fail,
>> for example when a module is not installed.  Last error code is returned.
>> With this, extensive command sets can be given on the boot-line.
>> +/* handle multiple queries, continue on error, return last error */
>> +static int ddebug_exec_queries(char *query)
>> +{
>> +       char *split = query;
>> +       int i, errs = 0, exitcode = 0, rc;
>> +
>> +       if (verbose)
>> +               /* clean up for logging */
>> +               for (; (split = strpbrk(split, "\t\n")); split++)
>> +                       *split = ' ';
>
> The above will join multiple lines into a single line and hence will
> cause a shell statement like the one below to be interpret as a single
> query:
>

oof.
I got impression from Documentation/dynamic-debug-howto.txt
that multiple commands were not supported.  Hence this change
in patch 3:

-Commands are bounded by a write() system call.  If you want to do
-multiple commands you need to do a separate "echo" for each, like:
+Commands are bounded by a write() system call.  Subject to this limit
+(or 1024 for boot-line parameter) you can send multiple commands,
+separated by ';'

Clearly by your example it is possible.
I'll remove those lines, they were for pretty debug messages anyway.

FWIW, using \n to separate multiple commands doesnt work
on kernel bootline (in mainline); the following kills the ddebug facility
(see 0007-dynamic_debug-dont-kill-entire-facility-on-error-par.patch)

root@voyage:~# more /proc/cmdline
root=LABEL=ROOT_FS console=ttyS0,115200n8 all_generic_ide \
ide_core.nodma=0.0 \
ddebug_query="module init_32 +p\n module suspend +p\n module freezer +p"

>
>> +       for (i = 0; query; query = split, i++) {
>> +
>
> No blank line past "for" please.

ack. will fix.

(no subject)

[Jim Cromie]

This patchset extends dynamic-debug facility to allow 
use of pr_debug() within a loadable module's module_init() 
function.  Query/rules can be given on the boot-line,
and are saved to a pending list if they cannot be applied
immediately.  Later, when the module is being loaded, the
pending list is scanned, and matching rules are applied.
Thus pr_debug() calls in the module's initialization function
are active when it is invoked.


Changes since rev1:

- rebased on top of Jasons & Joes patchset
- fixed accidental unescape removal, noted by Bart
- trim src-path patch checks for matching prefix before trimming
  should now work for out-of-tree modules.
- undid verbose newline-strip in exec-queries - Bart
- verbose param 644, not 744 - Bart, Greg
- whitespace - Bart
- added 'a' flag - Jason, Bart
- drop pending_max - Bart

New Stuff, Bigger items:

- lock around all list-work, pending-ct

In response to Bart's locking-bug observation, I hoisted locks up to
callers, so theyd protect all list, pending-ct manipulation.  This
means longer hold-times, but less locking/unlocking.  Left as separate
patch for now, partly cuz having pr_info's under lock gave me some
heartburn.  That said, lockdep didnt complain.

- 'a' pending query modification, removal

Ive extended flags spec to have <match-flags>* <op> <new-flags>*
IE, matching /[pmflta]*[+-=][pmflta]*/

match-flags (optional) allows a query/rule to be more selective, which
increases the utility of otherwise unconstrained rules.  So the
following query matches all call-sites that are already enabled,
adding the TID flag.

$> echo " p+t " > <dbgfs>/dynamic_debug/control

Further, it allows modification or deletion of currently pending
queries:

$> echo "module foo +ap" > <debugfs>/dynamic_debug/control
$> echo "module foo +apt" > <debugfs>/dynamic_debug/control
$> echo "module foo ap=" > <debugfs>/dynamic_debug/control
    
1st command adds a pending query on module foo
2nd command modifies it by adding a TID flag
3rd command deletes the pending query by setting flags to 0

Note that 2,3 have exact match on the query-spec, the match-flags in 3
specify flags required to match against the pending query; the 't'
flag added in 2 is not required, but allowed.

TODO:

1. when pending-queries is not empty, theres a hang during shutdown.
If I delete all pending queries, shutdown works cleanly.  Ive added
code to clear the pending-list into ddebug_remove_all_tables(), but
its apparently not executed.  Please advise how to invoke
ddebug_remove_all_tables() at shutdown, and whether to do it
separately.

2. $> cat <dbfs>/dynamic_debug/pending
This would display currently pending queries, simplifying their deletion.

3. Decide whether to keep pending on list after they apply.
Given that user added 'a' flag, and can now delete them, its
reasonable to leave them on pending list.  Bart's point, TBD.

Notes:

1. There is a subtle asymmetry in the 'a' flag, example 1 doesnt care
that 'p' is still enabled for the query, once 'a' is turned off, we'd
remove it from the pending list, and same holds for any unconstrained
flags, like t.  However ISTM this is what the user would/should expect.

2. echo " +p " > /dbg/dynamic_debug/control 

The above felt a little radical, but it isnt really; it works on
mainline.  Therefore one part of the Doc is slightly misleading (last
sentence):

 The match-spec's are used to choose a subset of the known dprintk()
 callsites to which to apply the flags-spec.  Think of them as a query
 with implicit ANDs between each pair.  Note that an empty list of
 match-specs is possible, but is not very useful because it will not
 match any debug statement callsites.

3. this runs 2 separate writes.

printf "module nsc_gpio +p\n module pc8736x_gpio +p\npc8736x_gpio +p\n" \
       > /dbg/dynamic_debug/control

This form of command is seen by kernel as 2 separate writes, so the
form is not usable in boot-line in mainline; it breaks the debug
facility (see patch 6).  Patch 3 allows multiple commands, separated
by ';', and does work on boot-line.

thanks
Jim Cromie

[PATCH 01/21] dynamic_debug: add pending flag 'a' to make pending queries explicit
[PATCH 02/21] dynamic_debug: allow changing of dynamic_debug verbosity any time
[PATCH 03/21] dynamic_debug: process multiple commands on a line
[PATCH 04/21] dynamic_debug: warn when >1 of each type of match-spec is given
[PATCH 05/21] dynamic_debug: pr_err() call should not depend upon verbosity
[PATCH 06/21] dynamic_debug: dont kill entire facility on error parsing ddebug_query
[PATCH 07/21] dynamic_debug: return int from ddebug_change
[PATCH 08/21] dynamic_debug: factor show_ddebug_query out of ddebug_parse_query
[PATCH 09/21] dynamic_debug: save_pending() saves non-matching queries for later.
[PATCH 10/21] dynamic_debug: call apply_pending_queries from ddebug_add_module
[PATCH 11/21] dynamic_debug: refactor query_matches_callsite out of ddebug_change
[PATCH 12/21] dynamic_debug: document use of pending queries at boot-time
[PATCH 13/21] dynamic_debug: require 'a' flag to explicitly mark pending queries
[PATCH 14/21] dynamic_debug: hoist locking in ddebug_change to callers
[PATCH 15/21] dynamic_debug: describe_flags with '=[ptmfl]*'
[PATCH 16/21] dynamic_debug: define several levels of verbosity
[PATCH 17/21] dynamic_debug: non-optimization - remove != NULL
[PATCH 18/21] dynamic_debug: trim source-path prefix from dynamic_debug/control
[PATCH 19/21] dynamic_debug: add flags filtering to flags spec handling
[PATCH 20/21] dynamic_debug: clear pending_queries list in remove_all_tables
[PATCH 21/21] dynamic_debug: delete pending queries

[PATCH 01/21] dynamic_debug: add pending flag 'a' to make pending queries explicit

[Jim Cromie]

this just adds flag to header, so it can be rebased to the front of
the patchset, so that rebuilds within the set are faster.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 include/linux/dynamic_debug.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index fdd3c0e..079a9e9 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -26,6 +26,7 @@ struct _ddebug {
 #define _DPRINTK_FLAGS_INCL_FUNCNAME	(1<<2)
 #define _DPRINTK_FLAGS_INCL_LINENO	(1<<3)
 #define _DPRINTK_FLAGS_INCL_TID		(1<<4)
+#define _DPRINTK_FLAGS_APPEND   (1<<5)  /* add query to pending list */
 #define _DPRINTK_FLAGS_DEFAULT 0
 	unsigned int flags:8;
 	char enabled;
-- 
1.7.4.1

[PATCH 02/21] dynamic_debug: allow changing of dynamic_debug verbosity any time

[Jim Cromie]

allow changing dynamic_debug verbosity
at boot-time, with: dynamic_debug.verbose=1
or at runtime with:
root@voyage:~# echo 1 > /sys/module/dynamic_debug/parameters/verbose

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index fbb4547..1597d5a 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -61,6 +61,7 @@ struct ddebug_iter {
 static DEFINE_MUTEX(ddebug_lock);
 static LIST_HEAD(ddebug_tables);
 static int verbose = 0;
+module_param(verbose, int, 0644);
 
 /* Return the last part of a pathname */
 static inline const char *basename(const char *path)
-- 
1.7.4.1

[PATCH 03/21] dynamic_debug: process multiple commands on a line

[Jim Cromie]

Process multiple commands per line, separated by ';'.  All commands are
processed, independent of errors, allowing individual commands to fail,
for example when a module is not installed.  Last error code is returned.
With this, extensive command sets can be given on the boot-line.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |   30 ++++++++++++++++++++++++++++--
 1 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 1597d5a..ae72402 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -423,6 +423,32 @@ static int ddebug_exec_query(char *query_string)
 	return 0;
 }
 
+/* handle multiple queries, continue on error, return last error */
+static int ddebug_exec_queries(char *query)
+{
+	char *split;
+	int i, errs = 0, exitcode = 0, rc;
+
+	for (i = 0; query; query = split, i++) {
+		split = strchr(query, ';');
+		if (split)
+			*split++ = '\0';
+
+		if (verbose)
+			pr_info("query %d: \"%s\"\n", i, query);
+
+		rc = ddebug_exec_query(query);
+		if (rc) {
+			errs++;
+			exitcode = rc;
+		}
+	}
+	if (verbose)
+		pr_info("processed %d queries, with %d errs\n", i, errs);
+
+	return exitcode;
+}
+
 static int dynamic_emit_prefix(const struct _ddebug *descriptor)
 {
 	char tid[sizeof(int) + sizeof(int)/2 + 4];
@@ -557,7 +583,7 @@ static ssize_t ddebug_proc_write(struct file *file, const char __user *ubuf,
 	if (verbose)
 		pr_info("read %d bytes from userspace\n", (int)len);
 
-	ret = ddebug_exec_query(tmpbuf);
+	ret = ddebug_exec_queries(tmpbuf);
 	if (ret)
 		return ret;
 
@@ -862,7 +888,7 @@ static int __init dynamic_debug_init(void)
 
 	/* ddebug_query boot param got passed -> set it up */
 	if (ddebug_setup_string[0] != '\0') {
-		ret = ddebug_exec_query(ddebug_setup_string);
+		ret = ddebug_exec_queries(ddebug_setup_string);
 		if (ret)
 			pr_warn("Invalid ddebug boot param %s",
 				ddebug_setup_string);
-- 
1.7.4.1

[PATCH 04/21] dynamic_debug: warn when >1 of each type of match-spec is given

[Jim Cromie]

Issue warning when a match-spec is given multiple times in a rule.
Previous code kept last one, but was silent about it.  Docs imply only
one is allowed by saying match-specs are ANDed together, given that
module M cannot match both A and B.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index ae72402..0575113 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -281,6 +281,14 @@ static char *unescape(char *str)
 	return str;
 }
 
+static inline void check_set(const char **dest, char *src, char *name)
+{
+	if (*dest)
+		pr_warn("match-spec:%s val:%s overridden by %s",
+			name, *dest, src);
+	*dest = src;
+}
+
 /*
  * Parse words[] as a ddebug query specification, which is a series
  * of (keyword, value) pairs chosen from these possibilities:
@@ -292,6 +300,9 @@ static char *unescape(char *str)
  * format <escaped-string-to-find-in-format>
  * line <lineno>
  * line <first-lineno>-<last-lineno> // where either may be empty
+ *
+ * only 1 pair of each type is expected, subsequent ones elicit a
+ * warning, and override the setting.
  */
 static int ddebug_parse_query(char *words[], int nwords,
 			       struct ddebug_query *query)
@@ -305,13 +316,14 @@ static int ddebug_parse_query(char *words[], int nwords,
 
 	for (i = 0 ; i < nwords ; i += 2) {
 		if (!strcmp(words[i], "func"))
-			query->function = words[i+1];
+			check_set(&query->function, words[i+1], "func");
 		else if (!strcmp(words[i], "file"))
-			query->filename = words[i+1];
+			check_set(&query->filename, words[i+1], "file");
 		else if (!strcmp(words[i], "module"))
-			query->module = words[i+1];
+			check_set(&query->module, words[i+1], "module");
 		else if (!strcmp(words[i], "format"))
-			query->format = unescape(words[i+1]);
+			check_set(&query->format, unescape(words[i+1]),
+				"format");
 		else if (!strcmp(words[i], "line")) {
 			char *first = words[i+1];
 			char *last = strchr(first, '-');
-- 
1.7.4.1

[PATCH 05/21] dynamic_debug: pr_err() call should not depend upon verbosity

[Jim Cromie]

issue keyword/parsing errors even w/o verbose set;
uncover otherwize mysterious non-functionality.
Also convert to pr_err(), which supplies __func__ via pr_fmt.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 0575113..fc31707 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -339,8 +339,7 @@ static int ddebug_parse_query(char *words[], int nwords,
 				query->last_lineno = query->first_lineno;
 			}
 		} else {
-			if (verbose)
-				pr_err("unknown keyword \"%s\"\n", words[i]);
+			pr_err("unknown keyword \"%s\"\n", words[i]);
 			return -EINVAL;
 		}
 	}
-- 
1.7.4.1

[PATCH 06/21] dynamic_debug: dont kill entire facility on error parsing ddebug_query

[Jim Cromie]

Currently, a parsing error on ddebug_query results in effective loss
of the facility; all tables are removed, and the init-call returns error.
This is way too severe, especially when an innocent quoting error can
be the cause:

Kernel command line: ... ddebug_query='file char_dev.c +p'
yields:

ddebug_exec_queries: query 0: "'file"
query before parse <'file>
ddebug_exec_queries: processed 1 queries, with 1 errs
Invalid ddebug boot param 'file

Fix this by zeroing return-code for query parse errors before
returning from dynamic_debug_init().

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index fc31707..bbee0aa 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -900,10 +900,11 @@ static int __init dynamic_debug_init(void)
 	/* ddebug_query boot param got passed -> set it up */
 	if (ddebug_setup_string[0] != '\0') {
 		ret = ddebug_exec_queries(ddebug_setup_string);
-		if (ret)
+		if (ret) {
+			ret = 0; /* make this non-fatal */
 			pr_warn("Invalid ddebug boot param %s",
 				ddebug_setup_string);
-		else
+		} else
 			pr_info("ddebug initialized with string %s",
 				ddebug_setup_string);
 	}
-- 
1.7.4.1

[PATCH 07/21] dynamic_debug: return int from ddebug_change

[Jim Cromie]

Alter ddebug_change to return number of matches found for query/rule.
This lets caller know whether rule applied, and potentially what
to do next.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index bbee0aa..de2a679 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -102,8 +102,8 @@ static char *ddebug_describe_flags(struct _ddebug *dp, char *buf,
  * the user which ddebug's were changed, or whether none
  * were matched.
  */
-static void ddebug_change(const struct ddebug_query *query,
-			   unsigned int flags, unsigned int mask)
+static int ddebug_change(const struct ddebug_query *query,
+			unsigned int flags, unsigned int mask)
 {
 	int i;
 	struct ddebug_table *dt;
@@ -167,8 +167,7 @@ static void ddebug_change(const struct ddebug_query *query,
 	}
 	mutex_unlock(&ddebug_lock);
 
-	if (!nfound && verbose)
-		pr_info("no matches for query\n");
+	return nfound;
 }
 
 /*
@@ -420,6 +419,7 @@ static int ddebug_exec_query(char *query_string)
 #define MAXWORDS 9
 	int nwords;
 	char *words[MAXWORDS];
+	int nfound;
 
 	nwords = ddebug_tokenize(query_string, words, MAXWORDS);
 	if (nwords <= 0)
@@ -430,7 +430,7 @@ static int ddebug_exec_query(char *query_string)
 		return -EINVAL;
 
 	/* actually go and implement the change */
-	ddebug_change(&query, flags, mask);
+	nfound = ddebug_change(&query, flags, mask);
 	return 0;
 }
 
-- 
1.7.4.1

[PATCH 08/21] dynamic_debug: factor show_ddebug_query out of ddebug_parse_query

[Jim Cromie]

Will reuse for show_pending_query too.  Alloc and free
print buffer space inside ddebug_exec_queries, instead of
a permanent static allocation.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |   24 +++++++++++++++++++-----
 1 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index de2a679..81268e2 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -96,6 +96,19 @@ static char *ddebug_describe_flags(struct _ddebug *dp, char *buf,
 	return buf;
 }
 
+static char *prbuf_query;
+
+static char *show_ddebug_query(const struct ddebug_query *q)
+{
+	sprintf(prbuf_query,
+		"q->function=\"%s\" q->filename=\"%s\" "
+		"q->module=\"%s\" q->format=\"%s\" q->lineno=%u-%u",
+		q->function, q->filename, q->module, q->format,
+		q->first_lineno, q->last_lineno);
+
+	return prbuf_query;
+}
+
 /*
  * Search the tables for _ddebug's which match the given
  * `query' and apply the `flags' and `mask' to them.  Tells
@@ -344,11 +357,7 @@ static int ddebug_parse_query(char *words[], int nwords,
 	}
 
 	if (verbose)
-		pr_info("q->function=\"%s\" q->filename=\"%s\" "
-			"q->module=\"%s\" q->format=\"%s\" q->lineno=%u-%u\n",
-			query->function, query->filename,
-			query->module, query->format, query->first_lineno,
-			query->last_lineno);
+		pr_info("parsed %s\n", show_ddebug_query(query));
 
 	return 0;
 }
@@ -440,6 +449,10 @@ static int ddebug_exec_queries(char *query)
 	char *split;
 	int i, errs = 0, exitcode = 0, rc;
 
+	prbuf_query = kmalloc(1024, GFP_KERNEL);
+	if (prbuf_query == NULL)
+		return -ENOMEM;
+
 	for (i = 0; query; query = split, i++) {
 		split = strchr(query, ';');
 		if (split)
@@ -454,6 +467,7 @@ static int ddebug_exec_queries(char *query)
 			exitcode = rc;
 		}
 	}
+	kfree(prbuf_query);
 	if (verbose)
 		pr_info("processed %d queries, with %d errs\n", i, errs);
 
-- 
1.7.4.1

[PATCH 09/21] dynamic_debug: save_pending() saves non-matching queries for later.

[Jim Cromie]

When a query/rule doesnt match, call save_pending(new function)
to copy the query off the stack, into a (new) struct pending_query,
and add to pending_queries (new) list.

Patch adds: /sys/module/dynamic_debug/parameters/
  verbose - rw, added previously, here for completeness
  pending_ct - ro, shows current count of pending queries

With this and previous patches, queries added on the boot-line which
do not match (because module is not built-in, and thus not present yet)
are added to pending_queries.

IE, with these boot-line additions:
 dynamic_debug.verbose=1 ddebug_query="module scx200_hrt +p"

Kernel will log the following:

ddebug_exec_queries: query 0: "module scx200_hrt +p"
ddebug_tokenize: split into words: "module" "scx200_hrt" "+p"
ddebug_parse_query: parsed q->function="(null)" q->filename="(null)" \
	q->module="scx200_hrt" q->format="(null)" q->lineno=0-0
ddebug_parse_flags: op='+'
ddebug_parse_flags: flags=0x1
ddebug_parse_flags: *flagsp=0x1 *maskp=0xffffffff
ddebug_exec_query: nfound 0 on q->function="(null)" q->filename="(null)" \
	q->module="scx200_hrt" q->format="(null)" q->lineno=0-0
ddebug_save_pending: add to pending: q->function="(null)" q->filename="(null)"\
	q->module="scx200_hrt" q->format="(null)" q->lineno=0-0
ddebug_save_pending: ddebug: query saved as pending 1

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |   56 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 81268e2..b049ef2 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -53,6 +53,16 @@ struct ddebug_query {
 	unsigned int first_lineno, last_lineno;
 };
 
+struct pending_query {
+	struct list_head link;
+	struct ddebug_query query;
+	char filename[100];
+	char module[30];
+	char function[50];
+	char format[200];
+	unsigned int flags, mask;
+};
+
 struct ddebug_iter {
 	struct ddebug_table *table;
 	unsigned int idx;
@@ -63,6 +73,11 @@ static LIST_HEAD(ddebug_tables);
 static int verbose = 0;
 module_param(verbose, int, 0644);
 
+/* legal but inapplicable queries, save and test against new modules */
+static LIST_HEAD(pending_queries);
+static int pending_ct;
+module_param(pending_ct, int, 0444);
+
 /* Return the last part of a pathname */
 static inline const char *basename(const char *path)
 {
@@ -421,6 +436,42 @@ static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
 	return 0;
 }
 
+/* copy query off stack, save flags & mask, and store in pending-list */
+static int ddebug_save_pending(struct ddebug_query *query,
+				unsigned int flags, unsigned int mask)
+{
+	struct pending_query *pq;
+
+	if (verbose)
+		pr_info("add to pending: %s\n", show_ddebug_query(query));
+
+	pending_ct++;
+	pq = kzalloc(sizeof(struct pending_query), GFP_KERNEL);
+	if (pq == NULL)
+		return -ENOMEM;
+
+	/* copy non-null match-specs into allocd mem, update pointers */
+	if (query->module)
+		pq->query.module = strcpy(pq->module, query->module);
+	if (query->function)
+		pq->query.function = strcpy(pq->function, query->function);
+	if (query->filename)
+		pq->query.filename = strcpy(pq->filename, query->filename);
+	if (query->format)
+		pq->query.format = strcpy(pq->format, query->format);
+
+	pq->flags = flags;
+	pq->mask = mask;
+
+	mutex_lock(&ddebug_lock);
+	list_add(&pq->link, &pending_queries);
+	mutex_unlock(&ddebug_lock);
+
+	if (verbose)
+		pr_info("query saved as pending %d\n", pending_ct);
+	return 0;
+}
+
 static int ddebug_exec_query(char *query_string)
 {
 	unsigned int flags = 0, mask = 0;
@@ -440,6 +491,11 @@ static int ddebug_exec_query(char *query_string)
 
 	/* actually go and implement the change */
 	nfound = ddebug_change(&query, flags, mask);
+
+	pr_info("nfound %d on %s\n", nfound, show_ddebug_query(&query));
+	if (!nfound)
+		return ddebug_save_pending(&query, flags, mask);
+
 	return 0;
 }
 
-- 
1.7.4.1

[PATCH 10/21] dynamic_debug: call apply_pending_queries from ddebug_add_module

[Jim Cromie]

When a module is loaded, ddebug_add_module calls apply_pending_queries
to scan pending_queries list and call ddebug_change to apply them.
Matching rules are removed from pending_queries.

With this change, the loaded module's pr_debugs are enabled when
its module_init is invoked, which is not possible otherwize.

With verbose=1, kernel logs look like:

  apply_pending_queries: check: pc8736x_gpio <-> q->function="(null)" q->filename="(null)" \
	q->module="pc8736x_gpio" q->format="(null)" q->lineno=0-0 q->flags=0x1 q->mask=0xffffffff
  ...
  ddebug_change: changed .../drivers/char/pc8736x_gpio.c:342 [pc8736x_gpio]pc8736x_gpio_cleanup p
  ddebug_change: changed .../drivers/char/pc8736x_gpio.c:319 [pc8736x_gpio]pc8736x_gpio_init p
  ...
  platform pc8736x_gpio.0: NatSemi pc8736x GPIO Driver Initializing
  platform pc8736x_gpio.0: GPIO ioport 6600 reserved

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |   41 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index b049ef2..cc8e157 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -124,6 +124,16 @@ static char *show_ddebug_query(const struct ddebug_query *q)
 	return prbuf_query;
 }
 
+static char *show_pending_query(struct pending_query *pq)
+{
+	struct ddebug_query *dq = &pq->query;
+	char *tmp = show_ddebug_query(dq);
+
+	sprintf(tmp + strlen(tmp), " q->flags=0x%x q->mask=0x%x",
+		pq->flags, pq->mask);
+	return prbuf_query;
+}
+
 /*
  * Search the tables for _ddebug's which match the given
  * `query' and apply the `flags' and `mask' to them.  Tells
@@ -846,6 +856,34 @@ static const struct file_operations ddebug_proc_fops = {
 	.write = ddebug_proc_write
 };
 
+/* apply matching queries in pending-queries list */
+static void apply_pending_queries(struct ddebug_table *dt)
+{
+	struct pending_query *pq, *pqnext;
+	int nfound;
+
+	if (verbose)
+		pr_info("pending_ct: %d\n", pending_ct);
+
+	list_for_each_entry_safe(pq, pqnext, &pending_queries, link) {
+
+		if (verbose)
+			pr_info("check: %s <-> %s\n",
+				dt->mod_name, show_pending_query(pq));
+
+		nfound = ddebug_change(&pq->query, pq->flags, pq->mask);
+
+		if (nfound) {
+			mutex_lock(&ddebug_lock);
+			list_del(&pq->link);
+			mutex_unlock(&ddebug_lock);
+			kfree(pq);
+			pending_ct--;
+		} else if (verbose)
+			pr_info("no-match: %s\n", show_pending_query(pq));
+	}
+
+}
 /*
  * Allocate a new ddebug_table for the given module
  * and add it to the global list.
@@ -874,6 +912,9 @@ int ddebug_add_module(struct _ddebug *tab, unsigned int n,
 
 	if (verbose)
 		pr_info("%u debug prints in module %s\n", n, dt->mod_name);
+
+	apply_pending_queries(dt);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(ddebug_add_module);
-- 
1.7.4.1

[PATCH 11/21] dynamic_debug: refactor query_matches_callsite out of ddebug_change

[Jim Cromie]

this makes no behavioral change, its just a code-move.
Originally done cuz it looked momentarily to be related to
query-vs-query check needed soon.

If this function is reused, note that it excludes check of the module;
that is done once for the table, this code refactors the test inside
the loop over the module's ddebug callsites.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |   52 +++++++++++++++++++++++++++++---------------------
 1 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index cc8e157..bee1376 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -134,6 +134,35 @@ static char *show_pending_query(struct pending_query *pq)
 	return prbuf_query;
 }
 
+static int query_matches_callsite(struct _ddebug *dp,
+				const struct ddebug_query *query)
+{
+	/* match against the source filename */
+	if (query->filename != NULL &&
+		strcmp(query->filename, dp->filename) &&
+		strcmp(query->filename, basename(dp->filename)))
+		return 0;
+
+	/* match against the function */
+	if (query->function != NULL &&
+		strcmp(query->function, dp->function))
+		return 0;
+
+	/* match against the format */
+	if (query->format != NULL &&
+		strstr(dp->format, query->format) == NULL)
+		return 0;
+
+	/* match against the line number range */
+	if (query->first_lineno && dp->lineno < query->first_lineno)
+		return 0;
+
+	if (query->last_lineno && dp->lineno > query->last_lineno)
+		return 0;
+
+	return 1;
+}
+
 /*
  * Search the tables for _ddebug's which match the given
  * `query' and apply the `flags' and `mask' to them.  Tells
@@ -161,28 +190,7 @@ static int ddebug_change(const struct ddebug_query *query,
 		for (i = 0 ; i < dt->num_ddebugs ; i++) {
 			struct _ddebug *dp = &dt->ddebugs[i];
 
-			/* match against the source filename */
-			if (query->filename != NULL &&
-			    strcmp(query->filename, dp->filename) &&
-			    strcmp(query->filename, basename(dp->filename)))
-				continue;
-
-			/* match against the function */
-			if (query->function != NULL &&
-			    strcmp(query->function, dp->function))
-				continue;
-
-			/* match against the format */
-			if (query->format != NULL &&
-			    strstr(dp->format, query->format) == NULL)
-				continue;
-
-			/* match against the line number range */
-			if (query->first_lineno &&
-			    dp->lineno < query->first_lineno)
-				continue;
-			if (query->last_lineno &&
-			    dp->lineno > query->last_lineno)
+			if (!query_matches_callsite(dp, query))
 				continue;
 
 			nfound++;
-- 
1.7.4.1

[PATCH 12/21] dynamic_debug: document use of pending queries at boot-time

[Jim Cromie]

Add paragraph that pending-queries are checked when modules are loaded,
so debug statements can be used in module_init functions.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 Documentation/dynamic-debug-howto.txt |   34 +++++++++++++++++++++++++-------
 1 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/Documentation/dynamic-debug-howto.txt b/Documentation/dynamic-debug-howto.txt
index f959909..49b75b81 100644
--- a/Documentation/dynamic-debug-howto.txt
+++ b/Documentation/dynamic-debug-howto.txt
@@ -92,8 +92,18 @@ nullarbor:~ # echo -c 'file svcsock.c\nline 1603 +p' >
 nullarbor:~ # echo -n 'file svcsock.c line 1603 +p' >
 				<debugfs>/dynamic_debug/control
 
-Commands are bounded by a write() system call.  If you want to do
-multiple commands you need to do a separate "echo" for each, like:
+Commands are bounded by a write() system call.  Subject to this limit
+(or 1024 for boot-line parameter) you can send multiple commands,
+separated by ';'
+
+foo:~ # echo "module nsc_gpio +p ; module pc8736x_gpio +p ; " \
+ "module scx200_gpio +p " > /dbg/dynamic_debug/control
+
+Multiple commands are processed independently, this allows you to send
+commands which may fail, for example if a module is not present.  The
+last failing command returns its error.
+
+Or you can do an "echo" for each, like:
 
 nullarbor:~ # echo 'file svcsock.c line 1603 +p' > /proc/dprintk ;\
 > echo 'file svcsock.c line 1563 +p' > /proc/dprintk
@@ -110,11 +120,12 @@ specifications, followed by a flags change specification.
 
 command ::= match-spec* flags-spec
 
-The match-spec's are used to choose a subset of the known dprintk()
+The match-specs are used to choose a subset of the known dprintk()
 callsites to which to apply the flags-spec.  Think of them as a query
-with implicit ANDs between each pair.  Note that an empty list of
-match-specs is possible, but is not very useful because it will not
-match any debug statement callsites.
+with implicit ANDs between each pair.  This means that multiple specs
+of a given type are nonsense; a module cannot match A and B
+simultaneously.  Note that an empty list of match-specs is legal but
+pointless, it will not match any debug statement callsites.
 
 A match specification comprises a keyword, which controls the attribute
 of the callsite to be compared, and a value to compare against.  Possible
@@ -232,13 +243,20 @@ QUERY follows the syntax described above, but must not exceed 1023
 characters. The enablement of debug messages is done as an arch_initcall.
 Thus you can enable debug messages in all code processed after this
 arch_initcall via this boot parameter.
-On an x86 system for example ACPI enablement is a subsys_initcall and
-ddebug_query="file ec.c +p"
+
+On an x86 system for example ACPI enablement is a subsys_initcall, so
+  ddebug_query="file ec.c +p"
 will show early Embedded Controller transactions during ACPI setup if
 your machine (typically a laptop) has an Embedded Controller.
 PCI (or other devices) initialization also is a hot candidate for using
 this boot parameter for debugging purposes.
 
+You can also give queries for modules which are not yet loaded, but
+will be, via udev or /etc/modules.  These queries are saved to a
+pending-queries list, and are applied when the module is loaded, and
+before the module's init function is invoked.  Note that modules which
+have no debug messages do not trigger this, so queries for them will
+remain on the pending-list until reboot.
 
 Examples
 ========
-- 
1.7.4.1

[PATCH 13/21] dynamic_debug: require 'a' flag to explicitly mark pending queries

[Jim Cromie]

Require that user mark pending queries explicitly, otherwise warn
that they do not apply.  Note that pending rules are still applied
by default, and are only added to pending-list if they do not apply,
and bear the flag.

Also test that query hasnt already been added to pending-list,
if it has (according to queries_match), just update the found query.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |   52 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index bee1376..0faac83 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -91,6 +91,7 @@ static struct { unsigned flag:8; char opt_char; } opt_array[] = {
 	{ _DPRINTK_FLAGS_INCL_FUNCNAME, 'f' },
 	{ _DPRINTK_FLAGS_INCL_LINENO, 'l' },
 	{ _DPRINTK_FLAGS_INCL_TID, 't' },
+	{ _DPRINTK_FLAGS_APPEND, 'a' },
 };
 
 /* format a string into buf[] which describes the _ddebug's flags */
@@ -454,12 +455,47 @@ static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
 	return 0;
 }
 
-/* copy query off stack, save flags & mask, and store in pending-list */
+static int queries_match(struct ddebug_query *q1, struct ddebug_query *q2)
+{
+	if (!q1->module ^ !q2->module ||
+		!q1->filename ^ !q2->filename ||
+		!q1->function ^ !q2->function ||
+		!q1->format ^ !q2->format)
+		return 0;  /* a match-spec set/unset state differs */
+
+	if (q1->last_lineno != q2->last_lineno ||
+		q1->first_lineno != q2->first_lineno)
+		return 0;
+
+	if ((q1->module && strcmp(q1->module, q2->module)) ||
+		(q1->filename && strcmp(q1->filename, q2->filename)) ||
+		(q1->function && strcmp(q1->function, q2->function)) ||
+		(q1->format && strcmp(q1->format, q2->format)))
+		return 0;
+	return 1;
+}
+
+/* copy query off stack, save flags & mask, and store in pending-list,
+   after checking that query isnt already there
+ */
 static int ddebug_save_pending(struct ddebug_query *query,
 				unsigned int flags, unsigned int mask)
 {
-	struct pending_query *pq;
+	struct pending_query *pq, *pqnext;
 
+	list_for_each_entry_safe(pq, pqnext, &pending_queries, link) {
+		if (queries_match(query, &pq->query)) {
+			/* query already in list, update flags */
+			if (pq->flags != flags)
+				pq->flags = flags;
+			if (pq->mask != mask)
+				pq->mask = mask;
+			if (verbose)
+				pr_info("already pending, updated it: %s\n",
+					show_pending_query(pq));
+			return 0;
+		}
+	}
 	if (verbose)
 		pr_info("add to pending: %s\n", show_ddebug_query(query));
 
@@ -511,9 +547,13 @@ static int ddebug_exec_query(char *query_string)
 	nfound = ddebug_change(&query, flags, mask);
 
 	pr_info("nfound %d on %s\n", nfound, show_ddebug_query(&query));
-	if (!nfound)
-		return ddebug_save_pending(&query, flags, mask);
-
+	if (!nfound) {
+		if (flags & _DPRINTK_FLAGS_APPEND)
+			return ddebug_save_pending(&query, flags, mask);
+		else
+			pr_warn("no match on: %s\n",
+				show_ddebug_query(&query));
+	}
 	return 0;
 }
 
-- 
1.7.4.1

[PATCH 14/21] dynamic_debug: hoist locking in ddebug_change to callers

[Jim Cromie]

Hoist locking out of ddebug_change(), into ddebug_exec_query(),
and fix return from inside the critical section.
Remove locking in ddebug_save_pending(), since its also
protected by ddebug_exec_query().

ddebug_add_module() also calls ddebug_change() via apply_pending_queries(),
move that call inside the critical section protecting list_add_tail()

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |   37 ++++++++++++++-----------------------
 1 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 0faac83..d115f52 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -168,7 +168,7 @@ static int query_matches_callsite(struct _ddebug *dp,
  * Search the tables for _ddebug's which match the given
  * `query' and apply the `flags' and `mask' to them.  Tells
  * the user which ddebug's were changed, or whether none
- * were matched.
+ * were matched.  Called with ddebug_lock held.
  */
 static int ddebug_change(const struct ddebug_query *query,
 			unsigned int flags, unsigned int mask)
@@ -180,7 +180,6 @@ static int ddebug_change(const struct ddebug_query *query,
 	char flagbuf[8];
 
 	/* search for matching ddebugs */
-	mutex_lock(&ddebug_lock);
 	list_for_each_entry(dt, &ddebug_tables, link) {
 
 		/* match against the module name */
@@ -212,8 +211,6 @@ static int ddebug_change(const struct ddebug_query *query,
 							sizeof(flagbuf)));
 		}
 	}
-	mutex_unlock(&ddebug_lock);
-
 	return nfound;
 }
 
@@ -475,8 +472,8 @@ static int queries_match(struct ddebug_query *q1, struct ddebug_query *q2)
 	return 1;
 }
 
-/* copy query off stack, save flags & mask, and store in pending-list,
-   after checking that query isnt already there
+/* copy query off stack, save flags & mask, and store or update in
+   pending-list.  Called with ddebug_lock held.
  */
 static int ddebug_save_pending(struct ddebug_query *query,
 				unsigned int flags, unsigned int mask)
@@ -499,7 +496,6 @@ static int ddebug_save_pending(struct ddebug_query *query,
 	if (verbose)
 		pr_info("add to pending: %s\n", show_ddebug_query(query));
 
-	pending_ct++;
 	pq = kzalloc(sizeof(struct pending_query), GFP_KERNEL);
 	if (pq == NULL)
 		return -ENOMEM;
@@ -517,9 +513,8 @@ static int ddebug_save_pending(struct ddebug_query *query,
 	pq->flags = flags;
 	pq->mask = mask;
 
-	mutex_lock(&ddebug_lock);
 	list_add(&pq->link, &pending_queries);
-	mutex_unlock(&ddebug_lock);
+	pending_ct++;
 
 	if (verbose)
 		pr_info("query saved as pending %d\n", pending_ct);
@@ -533,7 +528,7 @@ static int ddebug_exec_query(char *query_string)
 #define MAXWORDS 9
 	int nwords;
 	char *words[MAXWORDS];
-	int nfound;
+	int nfound, rc = 0;
 
 	nwords = ddebug_tokenize(query_string, words, MAXWORDS);
 	if (nwords <= 0)
@@ -544,17 +539,18 @@ static int ddebug_exec_query(char *query_string)
 		return -EINVAL;
 
 	/* actually go and implement the change */
+	mutex_lock(&ddebug_lock);
 	nfound = ddebug_change(&query, flags, mask);
-
-	pr_info("nfound %d on %s\n", nfound, show_ddebug_query(&query));
 	if (!nfound) {
 		if (flags & _DPRINTK_FLAGS_APPEND)
-			return ddebug_save_pending(&query, flags, mask);
+			rc = ddebug_save_pending(&query, flags, mask);
 		else
 			pr_warn("no match on: %s\n",
 				show_ddebug_query(&query));
 	}
-	return 0;
+	mutex_unlock(&ddebug_lock);
+	pr_info("nfound %d on %s\n", nfound, show_ddebug_query(&query));
+	return rc;
 }
 
 /* handle multiple queries, continue on error, return last error */
@@ -904,7 +900,7 @@ static const struct file_operations ddebug_proc_fops = {
 	.write = ddebug_proc_write
 };
 
-/* apply matching queries in pending-queries list */
+/* apply matching queries in pending-queries list. Called with lock held */
 static void apply_pending_queries(struct ddebug_table *dt)
 {
 	struct pending_query *pq, *pqnext;
@@ -914,7 +910,6 @@ static void apply_pending_queries(struct ddebug_table *dt)
 		pr_info("pending_ct: %d\n", pending_ct);
 
 	list_for_each_entry_safe(pq, pqnext, &pending_queries, link) {
-
 		if (verbose)
 			pr_info("check: %s <-> %s\n",
 				dt->mod_name, show_pending_query(pq));
@@ -922,15 +917,12 @@ static void apply_pending_queries(struct ddebug_table *dt)
 		nfound = ddebug_change(&pq->query, pq->flags, pq->mask);
 
 		if (nfound) {
-			mutex_lock(&ddebug_lock);
 			list_del(&pq->link);
-			mutex_unlock(&ddebug_lock);
 			kfree(pq);
 			pending_ct--;
 		} else if (verbose)
 			pr_info("no-match: %s\n", show_pending_query(pq));
 	}
-
 }
 /*
  * Allocate a new ddebug_table for the given module
@@ -954,14 +946,13 @@ int ddebug_add_module(struct _ddebug *tab, unsigned int n,
 	dt->num_ddebugs = n;
 	dt->ddebugs = tab;
 
-	mutex_lock(&ddebug_lock);
-	list_add_tail(&dt->link, &ddebug_tables);
-	mutex_unlock(&ddebug_lock);
-
 	if (verbose)
 		pr_info("%u debug prints in module %s\n", n, dt->mod_name);
 
+	mutex_lock(&ddebug_lock);
+	list_add_tail(&dt->link, &ddebug_tables);
 	apply_pending_queries(dt);
+	mutex_unlock(&ddebug_lock);
 
 	return 0;
 }
-- 
1.7.4.1

[PATCH 15/21] dynamic_debug: describe_flags with '=[ptmfl]*'

[Jim Cromie]

Describe flags in text using =<flag-chars>.  This has better mnemonics
than the current dash-unless-flags, it says "Flags are = <flag-chars>",
and =<flags> is more selective in a grep; '=' will not show up in module
or filenames, nor in line-ranges.

This isnt yet optimal; "grep ' = ' control" reports on a couple dozen
format-strings, this could be avoided by altering describe_flags()
to emit '-' or '_' for empty flags, such that the grep for empty
flags would be '=_'.  TBD.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index d115f52..31ee0fd 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -102,11 +102,10 @@ static char *ddebug_describe_flags(struct _ddebug *dp, char *buf,
 	int i;
 
 	BUG_ON(maxlen < 4);
+	*p++ = '=';
 	for (i = 0; i < ARRAY_SIZE(opt_array); ++i)
 		if (dp->flags & opt_array[i].flag)
 			*p++ = opt_array[i].opt_char;
-	if (p == buf)
-		*p++ = '-';
 	*p = '\0';
 
 	return buf;
-- 
1.7.4.1

[PATCH 16/21] dynamic_debug: define several levels of verbosity

[Jim Cromie]

define verbose levels 10, 11 to turn on noisy pr_info()s selectively
10: ddebug_proc_(start|stop) - once each per page of output
11: ddebug_proc_(show|next) - once each for every call-site
1-9: are unspecified, may be used for other pr_info()s

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 31ee0fd..8dde999 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -70,7 +70,11 @@ struct ddebug_iter {
 
 static DEFINE_MUTEX(ddebug_lock);
 static LIST_HEAD(ddebug_tables);
-static int verbose = 0;
+
+#define VERB_BASIC 1
+#define VERB_PROC 10
+#define VERB_PROC_SHOW 11
+static int verbose;
 module_param(verbose, int, 0644);
 
 /* legal but inapplicable queries, save and test against new modules */
@@ -777,7 +781,7 @@ static void *ddebug_proc_start(struct seq_file *m, loff_t *pos)
 	struct _ddebug *dp;
 	int n = *pos;
 
-	if (verbose)
+	if (verbose >= VERB_PROC)
 		pr_info("called m=%p *pos=%lld\n", m, (unsigned long long)*pos);
 
 	mutex_lock(&ddebug_lock);
@@ -802,7 +806,7 @@ static void *ddebug_proc_next(struct seq_file *m, void *p, loff_t *pos)
 	struct ddebug_iter *iter = m->private;
 	struct _ddebug *dp;
 
-	if (verbose)
+	if (verbose >= VERB_PROC_SHOW)
 		pr_info("called m=%p p=%p *pos=%lld\n",
 			m, p, (unsigned long long)*pos);
 
@@ -826,7 +830,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
 	struct _ddebug *dp = p;
 	char flagsbuf[8];
 
-	if (verbose)
+	if (verbose >= VERB_PROC_SHOW)
 		pr_info("called m=%p p=%p\n", m, p);
 
 	if (p == SEQ_START_TOKEN) {
@@ -851,7 +855,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
  */
 static void ddebug_proc_stop(struct seq_file *m, void *p)
 {
-	if (verbose)
+	if (verbose >= VERB_PROC)
 		pr_info("called m=%p p=%p\n", m, p);
 	mutex_unlock(&ddebug_lock);
 }
-- 
1.7.4.1

[PATCH 17/21] dynamic_debug: non-optimization - remove != NULL

[Jim Cromie]

Reduce != NULL relational test to its 0/1 equivalent.
No code difference, may not be worth including.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 8dde999..1763cc2 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -142,18 +142,18 @@ static int query_matches_callsite(struct _ddebug *dp,
 				const struct ddebug_query *query)
 {
 	/* match against the source filename */
-	if (query->filename != NULL &&
+	if (query->filename &&
 		strcmp(query->filename, dp->filename) &&
 		strcmp(query->filename, basename(dp->filename)))
 		return 0;
 
 	/* match against the function */
-	if (query->function != NULL &&
+	if (query->function &&
 		strcmp(query->function, dp->function))
 		return 0;
 
 	/* match against the format */
-	if (query->format != NULL &&
+	if (query->format &&
 		strstr(dp->format, query->format) == NULL)
 		return 0;
 
-- 
1.7.4.1

[PATCH 18/21] dynamic_debug: trim source-path prefix from dynamic_debug/control

[Jim Cromie]

Check for a common source-path prefix, and skip past it, to print a
path thats relative to kernel source's root-dir.  For in-tree modules,
this makes the control file easier to read without a wide screen.

For example:
  kernel/freezer.c:128 [freezer]cancel_freezing - "  clean up: %s\012"

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 1763cc2..44ccc02 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -829,6 +829,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
 	struct ddebug_iter *iter = m->private;
 	struct _ddebug *dp = p;
 	char flagsbuf[8];
+	int skip = strlen(__FILE__) - strlen("lib/dynamic_debug.c");
 
 	if (verbose >= VERB_PROC_SHOW)
 		pr_info("called m=%p p=%p\n", m, p);
@@ -839,8 +840,11 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
 		return 0;
 	}
 
+	if (strncmp(dp->filename, __FILE__, skip))
+		skip = 0; /* prefix mismatch, don't skip */
+
 	seq_printf(m, "%s:%u [%s]%s %s \"",
-		   dp->filename, dp->lineno,
+		   dp->filename + skip, dp->lineno,
 		   iter->table->mod_name, dp->function,
 		   ddebug_describe_flags(dp, flagsbuf, sizeof(flagsbuf)));
 	seq_escape(m, dp->format, "\t\r\n\"");
-- 
1.7.4.1

[PATCH 19/21] dynamic_debug: add flags filtering to flags spec handling

[Jim Cromie]

Change definition of flags spec to [ptlmfa]*[-+=][ptlmfa]+
The flags preceding the op are optional filter-flags, if provided,
they add an additional constraint to call-site matching done
by ddebug_change; call-sites which do not have all specified flags
are skipped (additional flags are allowed).

This allows query/rules like "p+t" to turn on TID logging for all
currently enabled call-sites, while leaving the others alone.
This will also allow filtering on pending rules (with 'a' flag),
which will support removal of pending rules.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |   81 +++++++++++++++++++++++++++++++-------------------
 1 files changed, 50 insertions(+), 31 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 44ccc02..9514071 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -174,7 +174,8 @@ static int query_matches_callsite(struct _ddebug *dp,
  * were matched.  Called with ddebug_lock held.
  */
 static int ddebug_change(const struct ddebug_query *query,
-			unsigned int flags, unsigned int mask)
+			unsigned int flags, unsigned int mask,
+			unsigned int filter)
 {
 	int i;
 	struct ddebug_table *dt;
@@ -196,6 +197,9 @@ static int ddebug_change(const struct ddebug_query *query,
 			if (!query_matches_callsite(dp, query))
 				continue;
 
+			if (filter && (dp->flags & filter) != filter)
+				continue;
+
 			nfound++;
 
 			newflags = (dp->flags & mask) | flags;
@@ -396,29 +400,9 @@ static int ddebug_parse_query(char *words[], int nwords,
 	return 0;
 }
 
-/*
- * Parse `str' as a flags specification, format [-+=][p]+.
- * Sets up *maskp and *flagsp to be used when changing the
- * flags fields of matched _ddebug's.  Returns 0 on success
- * or <0 on error.
- */
-static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
-			       unsigned int *maskp)
+static int ddebug_parse_flag_settings(const char *str)
 {
-	unsigned flags = 0;
-	int op = '=', i;
-
-	switch (*str) {
-	case '+':
-	case '-':
-	case '=':
-		op = *str++;
-		break;
-	default:
-		return -EINVAL;
-	}
-	if (verbose)
-		pr_info("op='%c'\n", op);
+	int i, flags = 0;
 
 	for ( ; *str ; ++str) {
 		for (i = ARRAY_SIZE(opt_array) - 1; i >= 0; i--) {
@@ -427,13 +411,49 @@ static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
 				break;
 			}
 		}
-		if (i < 0)
+		if (i < 0 && *str) {
+			pr_err("unknown flag: %c\n", *str);
 			return -EINVAL;
+		}
 	}
-	if (flags == 0)
+	return flags;
+}
+
+/*
+ * Parse `str' as a flags filter and specification, with format
+ * [ptlmfa]*[-+=][ptlmfa]+.  Sets up *maskp, *flagsp and *filterp to
+ * be used when changing the flags fields of matched _ddebug's.
+ * Returns 0 on success or <0 on error.
+ */
+static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
+			unsigned int *maskp, unsigned int *filterp)
+{
+	int flags, filter;
+	int op = '=';
+	char *s = strpbrk(str, "+-=");
+
+	if (!s) {
+		pr_err("flags spec missing op, expecting [+-=]\n");
 		return -EINVAL;
+	} else {
+		op = *s;
+		*s++ = '\0';
+	}
+	filter = ddebug_parse_flag_settings(str);
+	if (filter < 0) {
+		pr_err("flags_filter=0x%x\n", filter);
+		return -EINVAL;
+	}
+	*filterp = filter;
+
+	flags = ddebug_parse_flag_settings(s);
+	if (flags <= 0) {
+		pr_err("flags=0x%x\n", flags);
+		return -EINVAL;
+	}
 	if (verbose)
-		pr_info("flags=0x%x\n", flags);
+		pr_info(" flags_filter=0x%x\n op='%c'\n flags=0x%x\n",
+			filter, op, flags);
 
 	/* calculate final *flagsp, *maskp according to mask and op */
 	switch (op) {
@@ -526,7 +546,7 @@ static int ddebug_save_pending(struct ddebug_query *query,
 
 static int ddebug_exec_query(char *query_string)
 {
-	unsigned int flags = 0, mask = 0;
+	unsigned int flags = 0, mask = 0, filter = 0;
 	struct ddebug_query query;
 #define MAXWORDS 9
 	int nwords;
@@ -538,12 +558,12 @@ static int ddebug_exec_query(char *query_string)
 		return -EINVAL;
 	if (ddebug_parse_query(words, nwords-1, &query))
 		return -EINVAL;
-	if (ddebug_parse_flags(words[nwords-1], &flags, &mask))
+	if (ddebug_parse_flags(words[nwords-1], &flags, &mask, &filter))
 		return -EINVAL;
 
 	/* actually go and implement the change */
 	mutex_lock(&ddebug_lock);
-	nfound = ddebug_change(&query, flags, mask);
+	nfound = ddebug_change(&query, flags, mask, filter);
 	if (!nfound) {
 		if (flags & _DPRINTK_FLAGS_APPEND)
 			rc = ddebug_save_pending(&query, flags, mask);
@@ -921,8 +941,7 @@ static void apply_pending_queries(struct ddebug_table *dt)
 			pr_info("check: %s <-> %s\n",
 				dt->mod_name, show_pending_query(pq));
 
-		nfound = ddebug_change(&pq->query, pq->flags, pq->mask);
-
+		nfound = ddebug_change(&pq->query, pq->flags, pq->mask, 0);
 		if (nfound) {
 			list_del(&pq->link);
 			kfree(pq);
-- 
1.7.4.1

[PATCH 20/21] dynamic_debug: clear pending_queries list in remove_all_tables

[Jim Cromie]

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 9514071..f0c2b39 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -1017,6 +1017,8 @@ EXPORT_SYMBOL_GPL(ddebug_remove_module);
 
 static void ddebug_remove_all_tables(void)
 {
+	struct pending_query *pq, *pqnext;
+
 	mutex_lock(&ddebug_lock);
 	while (!list_empty(&ddebug_tables)) {
 		struct ddebug_table *dt = list_entry(ddebug_tables.next,
@@ -1024,6 +1026,13 @@ static void ddebug_remove_all_tables(void)
 						      link);
 		ddebug_table_free(dt);
 	}
+	list_for_each_entry_safe(pq, pqnext, &pending_queries, link) {
+		if (verbose > 1)
+			pr_info("delete pending: %s\n",
+				show_pending_query(pq));
+		list_del_init(&pq->link);
+		kfree(pq);
+	}
 	mutex_unlock(&ddebug_lock);
 }
 
-- 
1.7.4.1

[PATCH 21/21] dynamic_debug: delete pending queries

[Jim Cromie]

delete pending queries by re-issuing it with disabled flags

with:
$> echo "module foo +ap" > <debugfs>/dynamic_debug/control
$> echo "module foo +apt" > <debugfs>/dynamic_debug/control
$> echo "module foo ap=" > <debugfs>/dynamic_debug/control

1st command adds a pending query on foo
2nd command modifies it by adding a TID flag
3rd command deletes it by setting flags to 0

Note that the pending query matches 'ap' as required by the flags-filter,
but additionally has 't'.  An exact match on the query-spec is required,
"module foo line 10-20" would not match, and wouldnt be removed.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index f0c2b39..2fd207d 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -447,7 +447,7 @@ static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
 	*filterp = filter;
 
 	flags = ddebug_parse_flag_settings(s);
-	if (flags <= 0) {
+	if (flags < 0) {
 		pr_err("flags=0x%x\n", flags);
 		return -EINVAL;
 	}
@@ -505,7 +505,17 @@ static int ddebug_save_pending(struct ddebug_query *query,
 
 	list_for_each_entry_safe(pq, pqnext, &pending_queries, link) {
 		if (queries_match(query, &pq->query)) {
-			/* query already in list, update flags */
+			/* query already in list */
+			if (!flags) {
+				/* zeroed flags, remove query */
+				if (verbose)
+					pr_info("delete pending: %s\n",
+						show_pending_query(pq));
+				list_del_init(&pq->link);
+				kfree(pq);
+				pending_ct--;
+				return 0;
+			}
 			if (pq->flags != flags)
 				pq->flags = flags;
 			if (pq->mask != mask)
@@ -565,7 +575,8 @@ static int ddebug_exec_query(char *query_string)
 	mutex_lock(&ddebug_lock);
 	nfound = ddebug_change(&query, flags, mask, filter);
 	if (!nfound) {
-		if (flags & _DPRINTK_FLAGS_APPEND)
+		if (flags & _DPRINTK_FLAGS_APPEND ||
+			filter & _DPRINTK_FLAGS_APPEND)
 			rc = ddebug_save_pending(&query, flags, mask);
 		else
 			pr_warn("no match on: %s\n",
-- 
1.7.4.1

Re: [PATCH 21/21] dynamic_debug: delete pending queries

[Bart Van Assche]

On Mon, Jul 11, 2011 at 9:46 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
> delete pending queries by re-issuing it with disabled flags
>
> with:
> $> echo "module foo +ap" > <debugfs>/dynamic_debug/control
> $> echo "module foo +apt" > <debugfs>/dynamic_debug/control
> $> echo "module foo ap=" > <debugfs>/dynamic_debug/control
>
> 1st command adds a pending query on foo
> 2nd command modifies it by adding a TID flag
> 3rd command deletes it by setting flags to 0
>
> Note that the pending query matches 'ap' as required by the flags-filter,
> but additionally has 't'.  An exact match on the query-spec is required,
> "module foo line 10-20" would not match, and wouldnt be removed.

Maybe I've missed something, but do we need both 'a' and 'p' ? Would
'a' alone be sufficient ?

Bart.

Re: [PATCH 21/21] dynamic_debug: delete pending queries

[Jim Cromie]

On Mon, Jul 11, 2011 at 4:49 AM, Bart Van Assche <bvanassche@acm.org> wrote:
> On Mon, Jul 11, 2011 at 9:46 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
>> delete pending queries by re-issuing it with disabled flags
>>
>> with:
>> $> echo "module foo +ap" > <debugfs>/dynamic_debug/control
>> $> echo "module foo +apt" > <debugfs>/dynamic_debug/control
>> $> echo "module foo ap=" > <debugfs>/dynamic_debug/control
>>
>> 1st command adds a pending query on foo
>> 2nd command modifies it by adding a TID flag
>> 3rd command deletes it by setting flags to 0
>>
>> Note that the pending query matches 'ap' as required by the flags-filter,
>> but additionally has 't'.  An exact match on the query-spec is required,
>> "module foo line 10-20" would not match, and wouldnt be removed.
>
> Maybe I've missed something, but do we need both 'a' and 'p' ? Would
> 'a' alone be sufficient ?

p is for printing, not appending.
unfortunate mnemonics.
maybe 'k' for keep, instead of a for pending is better.

>
> Bart.
>

Re: [PATCH 09/21] dynamic_debug: save_pending() saves non-matching queries for later.

[Bart Van Assche]

On Mon, Jul 11, 2011 at 9:46 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
>
> When a query/rule doesnt match, call save_pending(new function)
> to copy the query off the stack, into a (new) struct pending_query,
> and add to pending_queries (new) list.
>
> Patch adds: /sys/module/dynamic_debug/parameters/
>  verbose - rw, added previously, here for completeness
>  pending_ct - ro, shows current count of pending queries
>
> With this and previous patches, queries added on the boot-line which
> do not match (because module is not built-in, and thus not present yet)
> are added to pending_queries.
>
> IE, with these boot-line additions:
>  dynamic_debug.verbose=1 ddebug_query="module scx200_hrt +p"
>
> Kernel will log the following:
>
> ddebug_exec_queries: query 0: "module scx200_hrt +p"
> ddebug_tokenize: split into words: "module" "scx200_hrt" "+p"
> ddebug_parse_query: parsed q->function="(null)" q->filename="(null)" \
>        q->module="scx200_hrt" q->format="(null)" q->lineno=0-0
> ddebug_parse_flags: op='+'
> ddebug_parse_flags: flags=0x1
> ddebug_parse_flags: *flagsp=0x1 *maskp=0xffffffff
> ddebug_exec_query: nfound 0 on q->function="(null)" q->filename="(null)" \
>        q->module="scx200_hrt" q->format="(null)" q->lineno=0-0
> ddebug_save_pending: add to pending: q->function="(null)" q->filename="(null)"\
>        q->module="scx200_hrt" q->format="(null)" q->lineno=0-0
> ddebug_save_pending: ddebug: query saved as pending 1
>
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
>  lib/dynamic_debug.c |   56 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 56 insertions(+), 0 deletions(-)
>
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 81268e2..b049ef2 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -53,6 +53,16 @@ struct ddebug_query {
>        unsigned int first_lineno, last_lineno;
>  };
>
> +struct pending_query {
> +       struct list_head link;
> +       struct ddebug_query query;
> +       char filename[100];
> +       char module[30];
> +       char function[50];
> +       char format[200];
> +       unsigned int flags, mask;
> +};
> +
>  struct ddebug_iter {
>        struct ddebug_table *table;
>        unsigned int idx;
> @@ -63,6 +73,11 @@ static LIST_HEAD(ddebug_tables);
>  static int verbose = 0;
>  module_param(verbose, int, 0644);
>
> +/* legal but inapplicable queries, save and test against new modules */
> +static LIST_HEAD(pending_queries);
> +static int pending_ct;
> +module_param(pending_ct, int, 0444);
> +
>  /* Return the last part of a pathname */
>  static inline const char *basename(const char *path)
>  {
> @@ -421,6 +436,42 @@ static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
>        return 0;
>  }
>
> +/* copy query off stack, save flags & mask, and store in pending-list */
> +static int ddebug_save_pending(struct ddebug_query *query,
> +                               unsigned int flags, unsigned int mask)
> +{
> +       struct pending_query *pq;
> +
> +       if (verbose)
> +               pr_info("add to pending: %s\n", show_ddebug_query(query));
> +
> +       pending_ct++;
> +       pq = kzalloc(sizeof(struct pending_query), GFP_KERNEL);
> +       if (pq == NULL)
> +               return -ENOMEM;
> +
> +       /* copy non-null match-specs into allocd mem, update pointers */
> +       if (query->module)
> +               pq->query.module = strcpy(pq->module, query->module);
> +       if (query->function)
> +               pq->query.function = strcpy(pq->function, query->function);
> +       if (query->filename)
> +               pq->query.filename = strcpy(pq->filename, query->filename);
> +       if (query->format)
> +               pq->query.format = strcpy(pq->format, query->format);

I see here several calls of strcpy() without prior size check.
Shouldn't these be changed into strlcpy() ?

Bart.

Re: [PATCH 08/21] dynamic_debug: factor show_ddebug_query out of ddebug_parse_query

[Bart Van Assche]

On Mon, Jul 11, 2011 at 9:46 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
> Will reuse for show_pending_query too.  Alloc and free
> print buffer space inside ddebug_exec_queries, instead of
> a permanent static allocation.
>
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
>  lib/dynamic_debug.c |   24 +++++++++++++++++++-----
>  1 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index de2a679..81268e2 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -96,6 +96,19 @@ static char *ddebug_describe_flags(struct _ddebug *dp, char *buf,
>        return buf;
>  }
>
> +static char *prbuf_query;
> +
> +static char *show_ddebug_query(const struct ddebug_query *q)
> +{
> +       sprintf(prbuf_query,
> +               "q->function=\"%s\" q->filename=\"%s\" "
> +               "q->module=\"%s\" q->format=\"%s\" q->lineno=%u-%u",
> +               q->function, q->filename, q->module, q->format,
> +               q->first_lineno, q->last_lineno);
> +
> +       return prbuf_query;
> +}
> +
>  /*
>  * Search the tables for _ddebug's which match the given
>  * `query' and apply the `flags' and `mask' to them.  Tells
> @@ -344,11 +357,7 @@ static int ddebug_parse_query(char *words[], int nwords,
>        }
>
>        if (verbose)
> -               pr_info("q->function=\"%s\" q->filename=\"%s\" "
> -                       "q->module=\"%s\" q->format=\"%s\" q->lineno=%u-%u\n",
> -                       query->function, query->filename,
> -                       query->module, query->format, query->first_lineno,
> -                       query->last_lineno);
> +               pr_info("parsed %s\n", show_ddebug_query(query));
>
>        return 0;
>  }
> @@ -440,6 +449,10 @@ static int ddebug_exec_queries(char *query)
>        char *split;
>        int i, errs = 0, exitcode = 0, rc;
>
> +       prbuf_query = kmalloc(1024, GFP_KERNEL);
> +       if (prbuf_query == NULL)
> +               return -ENOMEM;
> +
>        for (i = 0; query; query = split, i++) {
>                split = strchr(query, ';');
>                if (split)
> @@ -454,6 +467,7 @@ static int ddebug_exec_queries(char *query)
>                        exitcode = rc;
>                }
>        }
> +       kfree(prbuf_query);
>        if (verbose)
>                pr_info("processed %d queries, with %d errs\n", i, errs);

Why to invoke kmalloc() with a magic constant as the first argument
while the above code can be simplified by replacing the sprintf() call
(that is missing an output buffer size check) by a kasprintf() call ?
And why does the static variable 'prbuf_query' exist at all ?

Bart.

Re: [PATCH 07/21] dynamic_debug: return int from ddebug_change

[Bart Van Assche]

On Mon, Jul 11, 2011 at 9:46 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
> Alter ddebug_change to return number of matches found for query/rule.
> This lets caller know whether rule applied, and potentially what
> to do next.
>
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
>  lib/dynamic_debug.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index bbee0aa..de2a679 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -102,8 +102,8 @@ static char *ddebug_describe_flags(struct _ddebug *dp, char *buf,
>  * the user which ddebug's were changed, or whether none
>  * were matched.
>  */
> -static void ddebug_change(const struct ddebug_query *query,
> -                          unsigned int flags, unsigned int mask)
> +static int ddebug_change(const struct ddebug_query *query,
> +                       unsigned int flags, unsigned int mask)
>  {
>        int i;
>        struct ddebug_table *dt;
> @@ -167,8 +167,7 @@ static void ddebug_change(const struct ddebug_query *query,
>        }
>        mutex_unlock(&ddebug_lock);
>
> -       if (!nfound && verbose)
> -               pr_info("no matches for query\n");
> +       return nfound;
>  }
>
>  /*
> @@ -420,6 +419,7 @@ static int ddebug_exec_query(char *query_string)
>  #define MAXWORDS 9
>        int nwords;
>        char *words[MAXWORDS];
> +       int nfound;
>
>        nwords = ddebug_tokenize(query_string, words, MAXWORDS);
>        if (nwords <= 0)
> @@ -430,7 +430,7 @@ static int ddebug_exec_query(char *query_string)
>                return -EINVAL;
>
>        /* actually go and implement the change */
> -       ddebug_change(&query, flags, mask);
> +       nfound = ddebug_change(&query, flags, mask);
>        return 0;
>  }

I still see the introduction of a dead assignment in the second half
of the above patch. After the first series had been posted you had
proposed yourself to merge this patch with the subsequent patch.
Apparently that hasn't been done yet ?

Bart.

Re: [PATCH 21/21] dynamic_debug: delete pending queries

[Joe Perches]

On Mon, 2011-07-11 at 01:46 -0600, Jim Cromie wrote:
> delete pending queries by re-issuing it with disabled flags
[]
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
[]
> @@ -505,7 +505,17 @@ static int ddebug_save_pending(struct ddebug_query *query,
>  
>  	list_for_each_entry_safe(pq, pqnext, &pending_queries, link) {
>  		if (queries_match(query, &pq->query)) {
> -			/* query already in list, update flags */
> +			/* query already in list */
> +			if (!flags) {
> +				/* zeroed flags, remove query */
> +				if (verbose)
> +					pr_info("delete pending: %s\n",
> +						show_pending_query(pq));

I think these should be pr_debug.
I know you're only using the current style.

Jason, any reason these can not be converted?

Re: [PATCH 03/21] dynamic_debug: process multiple commands on a line

[Bart Van Assche]

On Mon, Jul 11, 2011 at 9:46 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
> Process multiple commands per line, separated by ';'.  All commands are
> processed, independent of errors, allowing individual commands to fail,
> for example when a module is not installed.  Last error code is returned.
> With this, extensive command sets can be given on the boot-line.
>
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
>  lib/dynamic_debug.c |   30 ++++++++++++++++++++++++++++--
>  1 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 1597d5a..ae72402 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -423,6 +423,32 @@ static int ddebug_exec_query(char *query_string)
>        return 0;
>  }
>
> +/* handle multiple queries, continue on error, return last error */
> +static int ddebug_exec_queries(char *query)
> +{
> +       char *split;
> +       int i, errs = 0, exitcode = 0, rc;
> +
> +       for (i = 0; query; query = split, i++) {
> +               split = strchr(query, ';');
> +               if (split)
> +                       *split++ = '\0';
> +
> +               if (verbose)
> +                       pr_info("query %d: \"%s\"\n", i, query);
> +
> +               rc = ddebug_exec_query(query);
> +               if (rc) {
> +                       errs++;
> +                       exitcode = rc;
> +               }
> +       }
> +       if (verbose)
> +               pr_info("processed %d queries, with %d errs\n", i, errs);
> +
> +       return exitcode;
> +}
> +

Does the above mean that semicolon is considered a valid separator but
newline not ?

Bart.

Re: [PATCH 09/21] dynamic_debug: save_pending() saves non-matching queries for later.

[Bart Van Assche]

On Mon, Jul 11, 2011 at 9:46 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
> +/* copy query off stack, save flags & mask, and store in pending-list */
> +static int ddebug_save_pending(struct ddebug_query *query,
> +                               unsigned int flags, unsigned int mask)
> +{
> +       struct pending_query *pq;
> +
> +       if (verbose)
> +               pr_info("add to pending: %s\n", show_ddebug_query(query));
> +
> +       pending_ct++;
> +       pq = kzalloc(sizeof(struct pending_query), GFP_KERNEL);
> +       if (pq == NULL)
> +               return -ENOMEM;
> +
> +       /* copy non-null match-specs into allocd mem, update pointers */
> +       if (query->module)
> +               pq->query.module = strcpy(pq->module, query->module);
> +       if (query->function)
> +               pq->query.function = strcpy(pq->function, query->function);
> +       if (query->filename)
> +               pq->query.filename = strcpy(pq->filename, query->filename);
> +       if (query->format)
> +               pq->query.format = strcpy(pq->format, query->format);
> +
> +       pq->flags = flags;
> +       pq->mask = mask;
> +
> +       mutex_lock(&ddebug_lock);
> +       list_add(&pq->link, &pending_queries);
> +       mutex_unlock(&ddebug_lock);
> +
> +       if (verbose)
> +               pr_info("query saved as pending %d\n", pending_ct);
> +       return 0;
> +}

As I wrote before, manipulating pending_ct without protecting it by
any kind of locking is racy.

Bart.

Re: [PATCH 13/21] dynamic_debug: require 'a' flag to explicitly mark pending queries

[Bart Van Assche]

On Mon, Jul 11, 2011 at 9:46 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
> +static int queries_match(struct ddebug_query *q1, struct ddebug_query *q2)
> +{
> +       if (!q1->module ^ !q2->module ||
> +               !q1->filename ^ !q2->filename ||
> +               !q1->function ^ !q2->function ||
> +               !q1->format ^ !q2->format)
> +               return 0;  /* a match-spec set/unset state differs */
> +
> +       if (q1->last_lineno != q2->last_lineno ||
> +               q1->first_lineno != q2->first_lineno)
> +               return 0;
> +
> +       if ((q1->module && strcmp(q1->module, q2->module)) ||
> +               (q1->filename && strcmp(q1->filename, q2->filename)) ||
> +               (q1->function && strcmp(q1->function, q2->function)) ||
> +               (q1->format && strcmp(q1->format, q2->format)))
> +               return 0;
> +       return 1;
> +}

Is there a reason why the return type of this function is "int" and
not "bool" ? Also, in the Linux kernel you can use the symbolic
constants "true" and "false" instead of "1" and "0".

Bart.

Re: [PATCH 21/21] dynamic_debug: delete pending queries

[Jason Baron]

On Mon, Jul 11, 2011 at 05:25:35PM -0700, Joe Perches wrote:
> On Mon, 2011-07-11 at 01:46 -0600, Jim Cromie wrote:
> > delete pending queries by re-issuing it with disabled flags
> []
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> []
> > @@ -505,7 +505,17 @@ static int ddebug_save_pending(struct ddebug_query *query,
> >  
> >  	list_for_each_entry_safe(pq, pqnext, &pending_queries, link) {
> >  		if (queries_match(query, &pq->query)) {
> > -			/* query already in list, update flags */
> > +			/* query already in list */
> > +			if (!flags) {
> > +				/* zeroed flags, remove query */
> > +				if (verbose)
> > +					pr_info("delete pending: %s\n",
> > +						show_pending_query(pq));
> 
> I think these should be pr_debug.
> I know you're only using the current style.
> 
> Jason, any reason these can not be converted?
> 

it should be ok, although we have to be careful not to use them in the
printing path, since that will cause a recursion.

Also, if there is an issue with the dynamic debug code, it makes it more
of a pain to debug :)

-Jason

Re: [PATCH 03/21] dynamic_debug: process multiple commands on a line

[Jim Cromie]

On Mon, Jul 11, 2011 at 11:54 PM, Bart Van Assche <bvanassche@acm.org> wrote:
> On Mon, Jul 11, 2011 at 9:46 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
>> Process multiple commands per line, separated by ';'.  All commands are
>> processed, independent of errors, allowing individual commands to fail,
>> for example when a module is not installed.  Last error code is returned.
>> With this, extensive command sets can be given on the boot-line.


> Does the above mean that semicolon is considered a valid separator but
> newline not ?
>
> Bart.
>


no, your printf example still works.
Note that it actually issues 2 writes to the control-file.

root@voyage:~# echo 1 > /sys/module/dynamic_debug/parameters/verbose
trol voyage:~# printf "module foo +ap\n module bar +ap" >
/dbg/dynamic_debug/cont
dynamic_debug:ddebug_proc_open: called
dynamic_debug:ddebug_proc_write: read 15 bytes from userspace
dynamic_debug:ddebug_exec_queries: query 0: "module foo +ap
"
dynamic_debug:ddebug_tokenize: split into words: "module" "foo" "+ap"
dynamic_debug:ddebug_parse_query: parsed func="" file="" module="foo"
format="" lineno=0-0
dynamic_debug:ddebug_parse_flags: flags_filter=0x0 op='+' flags=0x21
dynamic_debug:ddebug_parse_flags: *flagsp=0x21 *maskp=0xffffffff
dynamic_debug:ddebug_save_pending: add to pending: func="" file=""
module="foo" format="" lineno=0-0
dynamic_debug:ddebug_save_pending: query saved as pending 1
dynamic_debug:ddebug_exec_query: nfound 0 on func="" file=""
module="foo" format="" lineno=0-0
dynamic_debug:ddebug_exec_queries: processed 1 queries, with 0 errs
dynamic_debug:ddebug_proc_write: read 15 bytes from userspace
dynamic_debug:ddebug_exec_queries: query 0: " module bar +ap"
dynamic_debug:ddebug_tokenize: split into words: "module" "bar" "+ap"
dynamic_debug:ddebug_parse_query: parsed func="" file="" module="bar"
format="" lineno=0-0
dynamic_debug:ddebug_parse_flags: flags_filter=0x0 op='+' flags=0x21
dynamic_debug:ddebug_parse_flags: *flagsp=0x21 *maskp=0xffffffff
dynamic_debug:ddebug_save_pending: add to pending: func="" file=""
module="bar" format="" lineno=0-0
dynamic_debug:ddebug_save_pending: query saved as pending 2
dynamic_debug:ddebug_exec_query: nfound 0 on func="" file=""
module="bar" format="" lineno=0-0
root@voyage:~#

FWIW, the \n approach doesnt help on the boot-line;
on mainline, the following will break the facility -
/dbg/dynamic_debug will not get created.

Kernel command line: ...  ddebug_query="module pc8736x_gpio +p \n
module nsc_gpio +p "

Re: [PATCH 03/21] dynamic_debug: process multiple commands on a line

[Bart Van Assche]

On Wed, Jul 13, 2011 at 9:25 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
> On Mon, Jul 11, 2011 at 11:54 PM, Bart Van Assche <bvanassche@acm.org> wrote:
> > On Mon, Jul 11, 2011 at 9:46 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
> >> Process multiple commands per line, separated by ';'.  All commands are
> >> processed, independent of errors, allowing individual commands to fail,
> >> for example when a module is not installed.  Last error code is returned.
> >> With this, extensive command sets can be given on the boot-line.
> >
> > Does the above mean that semicolon is considered a valid separator but
> > newline not ?
>
> no, your printf example still works.
> Note that it actually issues 2 writes to the control-file.
>
> root@voyage:~# echo 1 > /sys/module/dynamic_debug/parameters/verbose
> trol voyage:~# printf "module foo +ap\n module bar +ap" >
> /dbg/dynamic_debug/cont
>
> FWIW, the \n approach doesnt help on the boot-line;
> on mainline, the following will break the facility -
> /dbg/dynamic_debug will not get created.
>
> Kernel command line: ...  ddebug_query="module pc8736x_gpio +p \n
> module nsc_gpio +p "

Just as a note, I do not expect that anyone will pass a string
containing an embedded newline via the kernel command line but I do
expect that people will use cat or printf to send multiple queries at
once to /sys/kernel/debug/dynamic_debug/control.

Bart.

Re: [PATCH 08/21] dynamic_debug: factor show_ddebug_query out of ddebug_parse_query

[Jim Cromie]

On Mon, Jul 11, 2011 at 12:36 PM, Bart Van Assche <bvanassche@acm.org> wrote:
> On Mon, Jul 11, 2011 at 9:46 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
>> Will reuse for show_pending_query too.  Alloc and free
>> print buffer space inside ddebug_exec_queries, instead of
>> a permanent static allocation.
>>
>> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
>> ---
>>  lib/dynamic_debug.c |   24 +++++++++++++++++++-----
>>  1 files changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
>> index de2a679..81268e2 100644
>> --- a/lib/dynamic_debug.c
>> +++ b/lib/dynamic_debug.c
>> @@ -96,6 +96,19 @@ static char *ddebug_describe_flags(struct _ddebug *dp, char *buf,
>>        return buf;
>>  }
>>
>> +static char *prbuf_query;
>> +
>> +static char *show_ddebug_query(const struct ddebug_query *q)
>> +{
>> +       sprintf(prbuf_query,
>> +               "q->function=\"%s\" q->filename=\"%s\" "
>> +               "q->module=\"%s\" q->format=\"%s\" q->lineno=%u-%u",
>> +               q->function, q->filename, q->module, q->format,
>> +               q->first_lineno, q->last_lineno);
>> +
>> +       return prbuf_query;
>> +}
>> +
>>  /*
>>  * Search the tables for _ddebug's which match the given
>>  * `query' and apply the `flags' and `mask' to them.  Tells
>> @@ -344,11 +357,7 @@ static int ddebug_parse_query(char *words[], int nwords,
>>        }
>>
>>        if (verbose)
>> -               pr_info("q->function=\"%s\" q->filename=\"%s\" "
>> -                       "q->module=\"%s\" q->format=\"%s\" q->lineno=%u-%u\n",
>> -                       query->function, query->filename,
>> -                       query->module, query->format, query->first_lineno,
>> -                       query->last_lineno);
>> +               pr_info("parsed %s\n", show_ddebug_query(query));
>>
>>        return 0;
>>  }
>> @@ -440,6 +449,10 @@ static int ddebug_exec_queries(char *query)
>>        char *split;
>>        int i, errs = 0, exitcode = 0, rc;
>>
>> +       prbuf_query = kmalloc(1024, GFP_KERNEL);
>> +       if (prbuf_query == NULL)
>> +               return -ENOMEM;
>> +
>>        for (i = 0; query; query = split, i++) {
>>                split = strchr(query, ';');
>>                if (split)
>> @@ -454,6 +467,7 @@ static int ddebug_exec_queries(char *query)
>>                        exitcode = rc;
>>                }
>>        }
>> +       kfree(prbuf_query);
>>        if (verbose)
>>                pr_info("processed %d queries, with %d errs\n", i, errs);
>
> Why to invoke kmalloc() with a magic constant as the first argument
> while the above code can be simplified by replacing the sprintf() call
> (that is missing an output buffer size check) by a kasprintf() call ?
> And why does the static variable 'prbuf_query' exist at all ?
>
> Bart.
>

using kasprintf would entail;
- a local char* var,
- a call to the string-producer (show_ddebug_query above)
- a kfree(var)
all in the caller context.
Using a preallocated string buffer avoids this,
and is slightly more efficient wrt kallocs.

Re: [PATCH 21/21] dynamic_debug: delete pending queries

[Jim Cromie]

On Tue, Jul 12, 2011 at 2:50 PM, Jason Baron <jbaron@redhat.com> wrote:
> On Mon, Jul 11, 2011 at 05:25:35PM -0700, Joe Perches wrote:
>> On Mon, 2011-07-11 at 01:46 -0600, Jim Cromie wrote:
>> > delete pending queries by re-issuing it with disabled flags
>> []
>> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
>> []
>> > @@ -505,7 +505,17 @@ static int ddebug_save_pending(struct ddebug_query *query,
>> >
>> >     list_for_each_entry_safe(pq, pqnext, &pending_queries, link) {
>> >             if (queries_match(query, &pq->query)) {
>> > -                   /* query already in list, update flags */
>> > +                   /* query already in list */
>> > +                   if (!flags) {
>> > +                           /* zeroed flags, remove query */
>> > +                           if (verbose)
>> > +                                   pr_info("delete pending: %s\n",
>> > +                                           show_pending_query(pq));
>>
>> I think these should be pr_debug.
>> I know you're only using the current style.
>>
>> Jason, any reason these can not be converted?
>>
>
> it should be ok, although we have to be careful not to use them in the
> printing path, since that will cause a recursion.
>
> Also, if there is an issue with the dynamic debug code, it makes it more
> of a pain to debug :)
>
> -Jason
>


I replaced almost all pr_info with pr_debug,
works nicely.

One pr_info remains, cuz code also uses pr_cont(),
which has no corresponding fn

	if (verbose) {
		int i;
		pr_info("split into words: %d ", i);
		for (i = 0 ; i < nwords ; i++)
			pr_cont(" \"%s\"", words[i]);
		pr_cont("\n");
	}

Would you consider adding it ?
If so, I can drop verbose var completely.

Re: [PATCH 21/21] dynamic_debug: delete pending queries

[Jim Cromie]

On Mon, Jul 11, 2011 at 6:25 PM, Joe Perches <joe@perches.com> wrote:
> On Mon, 2011-07-11 at 01:46 -0600, Jim Cromie wrote:
>> delete pending queries by re-issuing it with disabled flags
> []
>> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> []
>> @@ -505,7 +505,17 @@ static int ddebug_save_pending(struct ddebug_query *query,
>>
>>       list_for_each_entry_safe(pq, pqnext, &pending_queries, link) {
>>               if (queries_match(query, &pq->query)) {
>> -                     /* query already in list, update flags */
>> +                     /* query already in list */
>> +                     if (!flags) {
>> +                             /* zeroed flags, remove query */
>> +                             if (verbose)
>> +                                     pr_info("delete pending: %s\n",
>> +                                             show_pending_query(pq));
>
> I think these should be pr_debug.
> I know you're only using the current style.
>
> Jason, any reason these can not be converted?
>
>

hi Joe,

one other corner case (in addition to pr_cont, which I probly should
have addressed to you)

+#define pr_fmt(fmt) KBUILD_MODNAME ":%s: " fmt, __func__

makes pr_debug unconditionally print stuff that can be
doubled up if flags are also used.

Ive done s/pr_info/pr_debug/ locally, when combined with:
  ddebug_query="module dynamic_debug +pfmlt "
lines are too long, and are redundant

This is pretty trivial, and can be avoided by
"dont add those flags then", but I could imagine it
being a minor annoyance wherever both pr_debug
and pr_info|warn|err|etc are used together.

Perhaps pr_debug and friends should ignore pr_fmt
under CONFIG_DYNAMIC_DEBUG ?

Re: [PATCH 21/21] dynamic_debug: delete pending queries

[Joe Perches]

On Wed, 2011-07-20 at 11:43 -0600, Jim Cromie wrote:
> On Mon, Jul 11, 2011 at 6:25 PM, Joe Perches <joe@perches.com> wrote:
> > On Mon, 2011-07-11 at 01:46 -0600, Jim Cromie wrote:
> >> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > []
> >> @@ -505,7 +505,17 @@ static int ddebug_save_pending(struct ddebug_query *query,
[]
> >> +                             if (verbose)
> >> +                                     pr_info("delete pending: %s\n",
> >> +                                             show_pending_query(pq));
> > I think these should be pr_debug.
> > I know you're only using the current style.
> > Jason, any reason these can not be converted?
> one other corner case (in addition to pr_cont, which I probly should
> have addressed to you)
> +#define pr_fmt(fmt) KBUILD_MODNAME ":%s: " fmt, __func__
> makes pr_debug unconditionally print stuff that can be
> doubled up if flags are also used.
> Ive done s/pr_info/pr_debug/ locally, when combined with:
>   ddebug_query="module dynamic_debug +pfmlt "
> lines are too long, and are redundant
> This is pretty trivial, and can be avoided by
> "dont add those flags then", but I could imagine it
> being a minor annoyance wherever both pr_debug
> and pr_info|warn|err|etc are used together.
> Perhaps pr_debug and friends should ignore pr_fmt
> under CONFIG_DYNAMIC_DEBUG ?

Better I think would be to remove __func__ from those
dynamic debug pr_fmt defines.

Actually, I think it's better to never use __func__,
but that's an argument for a different day.

I think the only reason Jason did it was to always
have __func__ printed.  If those become pr_debug
then anyone that really wants to can use +pf.

cheers, Joe

Re: [PATCH 08/21] dynamic_debug: factor show_ddebug_query out of ddebug_parse_query

[Bart Van Assche]

On Sat, Jul 16, 2011 at 11:32 PM, Jim Cromie <jim.cromie@gmail.com> wrote:
> using kasprintf would entail;
> - a local char* var,
> - a call to the string-producer (show_ddebug_query above)
> - a kfree(var)
> all in the caller context.
> Using a preallocated string buffer avoids this,
> and is slightly more efficient wrt kallocs.

I'm still missing an explanation why the static variable "prbuf_query"
is necessary. Wouldn't eliminating that static variable make the code
more elegant ?

Bart.

Re: [PATCH 08/21] dynamic_debug: factor show_ddebug_query out of ddebug_parse_query

[Jim Cromie]

On Mon, Jul 25, 2011 at 5:10 AM, Bart Van Assche <bvanassche@acm.org> wrote:
> On Sat, Jul 16, 2011 at 11:32 PM, Jim Cromie <jim.cromie@gmail.com> wrote:
>> using kasprintf would entail;
>> - a local char* var,
>> - a call to the string-producer (show_ddebug_query above)
>> - a kfree(var)
>> all in the caller context.
>> Using a preallocated string buffer avoids this,
>> and is slightly more efficient wrt kallocs.
>
> I'm still missing an explanation why the static variable "prbuf_query"
> is necessary. Wouldn't eliminating that static variable make the code
> more elegant ?
>
> Bart.
>

After further consideration, Ive done it as you suggest.
Its a few more lines of code, but less complex -
all kfrees follow use immediately, no buffer mgmt at a distance.