dynamic debug v2 - infrastructure

include/linux/device.h +0 -0

index 1a06026..fb03dbc 100644
@@ -592,7 +592,11 @@ extern const char *dev_driver_string(struct device *dev);
 #define dev_info(dev, format, arg...)		\
 	dev_printk(KERN_INFO , dev , format , ## arg)

Return-Path: <[email protected]>

Quoted 7 lines · Jason Baron

include/linux/device.h +0 -0

index 1a06026..fb03dbc 100644
@@ -592,7 +592,11 @@ extern const char *dev_driver_string(struct device *dev);
 #define dev_info(dev, format, arg...)		\
 	dev_printk(KERN_INFO , dev , format , ## arg)

include/linux/device.h extern const char *dev_driver_string(struct device *dev); · line 602

Quoted 10 lines

include/linux/device.h +5 -1

 
-#ifdef DEBUG
+#if defined(CONFIG_DYNAMIC_PRINTK_DEBUG)
+#define dev_dbg(dev, format, ...) do { \
+	dynamic_dev_dbg(dev, format, ##__VA_ARGS__); \
+	} while (0)
+#elif defined(DEBUG)
 #define dev_dbg(dev, format, arg...)		\
 	dev_printk(KERN_DEBUG , dev , format , ## arg)
 #else

In looking at your follow-on patches, where you add this to subsystems, it seems that you have to add a lot of #include <linux/dynamic_printk.h> lines.

And in looking at this modification to device.h, I think lots of the kernel should break, but you are getting it "for free" by including the .h file within kernel.h.

Why not also include it here in device.h?

Also, with this change, code that is already using dev_dbg() today is instantly converted over to this logic now, right?

Quoted 28 lines · Greg KH

Return-Path: <[email protected]>

Quoted 7 lines · Jason Baron

include/linux/device.h +0 -0

index 1a06026..fb03dbc 100644
@@ -592,7 +592,11 @@ extern const char *dev_driver_string(struct device *dev);
 #define dev_info(dev, format, arg...)		\
 	dev_printk(KERN_INFO , dev , format , ## arg)

include/linux/device.h extern const char *dev_driver_string(struct device *dev); · line 602

Quoted 10 lines

include/linux/device.h +5 -1

 
-#ifdef DEBUG
+#if defined(CONFIG_DYNAMIC_PRINTK_DEBUG)
+#define dev_dbg(dev, format, ...) do { \
+	dynamic_dev_dbg(dev, format, ##__VA_ARGS__); \
+	} while (0)
+#elif defined(DEBUG)
 #define dev_dbg(dev, format, arg...)		\
 	dev_printk(KERN_DEBUG , dev , format , ## arg)
 #else

In looking at your follow-on patches, where you add this to subsystems, it seems that you have to add a lot of #include <linux/dynamic_printk.h> lines.

And in looking at this modification to device.h, I think lots of the kernel should break, but you are getting it "for free" by including the .h file within kernel.h.

Quoted 2 lines

Why not also include it here in device.h?

I don't believe the follow-on patches include "<linux/dynamic_printk.h>". However, some of them do include a subsystem specific #include. For example, the cpufreq subsystem include, <linux/dynamic_debug_cpufreq.h>:

+++ b/include/linux/dynamic_debug_cpufreq.h

(quoted diff) +8 -0

@@ -0,0 +1,8 @@
+#define DYNAMIC_DEBUG_NUM_FLAGS "3"
+#define DYNAMIC_DEBUG_FLAG_NAMES "CPUFREQ_DEBUG_CORE,CPUFREQ_DEBUG_DRIVER,CPUFREQ_DEBUG_GOVERNOR"
+#define DYNAMIC_DEBUG_TYPE "2"
+#define DYNAMIC_DEBUG_MODNAME "cpufreq_shared"
+
+#ifdef CONFIG_CPU_FREQ_DEBUG
+#define DEBUG 1
+#endif

This header file conveys that cpufreq subsystem, wants non-default options-it has 3 debugging flags that can be set, and all modules in the subsystem are tied together using the module name: "cpufreq_shared".

I don't have an explicit #include of kernel.h in device.h b/c it already picked up.

Quoted 3 lines

Also, with this change, code that is already using dev_dbg() today is instantly converted over to this logic now, right?

that is correct. any callers of dev_dbg() don't have to do anything. its really only the more complex debugging, where there are flags or levels that need to make adjustments to work with the new infrastructure.

Quoted 3 lines

that is correct. any callers of dev_dbg() don't have to do anything. its really only the more complex debugging, where there are flags or levels that need to make adjustments to work with the new infrastructure.

For this reason alone, I see no reason why your patch should not be merged today. You don't need the other subsystems at this point in time in my opinion, it's benifit is huge already.

Hi,

Quoted 7 lines · Greg KH

Quoted 3 lines

that is correct. any callers of dev_dbg() don't have to do anything. its really only the more complex debugging, where there are flags or levels that need to make adjustments to work with the new infrastructure.

For this reason alone, I see no reason why your patch should not be merged today. You don't need the other subsystems at this point in time in my opinion, it's benifit is huge already.

not to object to this statement, but:

what about the user-visible interface? currently, it's based around one big debugfs file. What about doing

	<debugfs>/dynamic_printk/<module_name>/{enabled[,level][,flag][,modules]}

instead, or even

	<sysfs>/module/<module_name>/debug/{enabled[,level][,flag]}

Quoted 1 line

	<sysfs>/module/<module_name>/debug/{enabled[,level][,flag]}

I like this as that is what a number of current modules do (usb-serial drivers), but you have to be careful about the module parameter namespace to not get collisions here with existing "debug" files.

So for now, I recommend staying in debugfs, it makes more sense.

Quoted 29 lines · Jason Baron

Quoted 28 lines · Greg KH

Return-Path: <[email protected]>

Quoted 7 lines · Jason Baron

include/linux/device.h +0 -0

index 1a06026..fb03dbc 100644
@@ -592,7 +592,11 @@ extern const char *dev_driver_string(struct device *dev);
 #define dev_info(dev, format, arg...)		\
 	dev_printk(KERN_INFO , dev , format , ## arg)

include/linux/device.h extern const char *dev_driver_string(struct device *dev); · line 602

Quoted 10 lines

include/linux/device.h +5 -1

 
-#ifdef DEBUG
+#if defined(CONFIG_DYNAMIC_PRINTK_DEBUG)
+#define dev_dbg(dev, format, ...) do { \
+	dynamic_dev_dbg(dev, format, ##__VA_ARGS__); \
+	} while (0)
+#elif defined(DEBUG)
 #define dev_dbg(dev, format, arg...)		\
 	dev_printk(KERN_DEBUG , dev , format , ## arg)
 #else

In looking at your follow-on patches, where you add this to subsystems, it seems that you have to add a lot of #include <linux/dynamic_printk.h> lines.

And in looking at this modification to device.h, I think lots of the kernel should break, but you are getting it "for free" by including the .h file within kernel.h.

Quoted 6 lines

Quoted 2 lines

Why not also include it here in device.h?

I don't believe the follow-on patches include "<linux/dynamic_printk.h>". However, some of them do include a subsystem specific #include. For example, the cpufreq subsystem include, <linux/dynamic_debug_cpufreq.h>:

Ah, you are correct, sorry about that, I misread.

Quoted 18 lines

+++ b/include/linux/dynamic_debug_cpufreq.h

(quoted diff) +8 -0

@@ -0,0 +1,8 @@
+#define DYNAMIC_DEBUG_NUM_FLAGS "3"
+#define DYNAMIC_DEBUG_FLAG_NAMES "CPUFREQ_DEBUG_CORE,CPUFREQ_DEBUG_DRIVER,CPUFREQ_DEBUG_GOVERNOR"
+#define DYNAMIC_DEBUG_TYPE "2"
+#define DYNAMIC_DEBUG_MODNAME "cpufreq_shared"
+
+#ifdef CONFIG_CPU_FREQ_DEBUG
+#define DEBUG 1
+#endif

This header file conveys that cpufreq subsystem, wants non-default options-it has 3 debugging flags that can be set, and all modules in the subsystem are tied together using the module name: "cpufreq_shared".

I don't have an explicit #include of kernel.h in device.h b/c it already picked up.

Quoted 7 lines

Quoted 3 lines

Also, with this change, code that is already using dev_dbg() today is instantly converted over to this logic now, right?

that is correct. any callers of dev_dbg() don't have to do anything. its really only the more complex debugging, where there are flags or levels that need to make adjustments to work with the new infrastructure.

For this reason alone, I see no reason why your patch should not be merged today. You don't need the other subsystems at this point in time in my opinion, it's benifit is huge already.

Not to say that you shouldn't also go after these subsystems in your overall scheme here, but please, don't feel it should hold your code up at this point in time :)

Because of that, do you want me to try to merge the basic infrastructure and dev_dbg() support through the driver-core tree? This code has been in the past -mm and linux-next releases, right?

Quoted 8 lines

Quoted 4 lines

Also, with this change, code that is already using dev_dbg() today is instantly converted over to this logic now, right?

that is correct. any callers of dev_dbg() don't have to do anything. its really only the more complex debugging, where there are flags or levels that need to make adjustments to work with the new infrastructure.

For this reason alone, I see no reason why your patch should not be merged today. You don't need the other subsystems at this point in time in my opinion, it's benifit is huge already.

Not to say that you shouldn't also go after these subsystems in your overall scheme here, but please, don't feel it should hold your code up at this point in time :)

Because of that, do you want me to try to merge the basic infrastructure and dev_dbg() support through the driver-core tree? This code has been in the past -mm and linux-next releases, right?

ok, here's a patch that implements just the basic core functionality that ties into pr_debug() and dev_dbg(). That is, any users of those two functions can be dynamically enabled/disabled at runtime if CONFIG_DYNAMIC_PRINTK_DEBUG is set at compile time. We can add the more complex flag/level debugging later as you suggest...

Few notes...there is still one control file: <debugfs>/dynamic_printk/modules We can split this up now or later, but I kind of like being able to see all the controls in one file. Also, i've used a djb2 hash function in the code, which i'm not sure is under the correct license/copyright, so i just wanted to point that out as well. see: scripts/basic/hash.c. If its an issue, i can find another hash function.

Quoted 6 lines

Few notes...there is still one control file: <debugfs>/dynamic_printk/modules We can split this up now or later, but I kind of like being able to see all the controls in one file. Also, i've used a djb2 hash function in the code, which i'm not sure is under the correct license/copyright, so i just wanted to point that out as well. see: scripts/basic/hash.c. If its an issue, i can find another hash function.

Why use that hash function? Actually, it looks like you are using 2 different ones, right? Why?

include/linux/dynamic_printk.h line 4

Quoted 7 lines

include/linux/dynamic_printk.h new file +4 -0

@@ -0,0 +1,94 @@
+#ifndef _DYNAMIC_PRINTK_H
+#define _DYNAMIC_PRINTK_H
+
+#ifdef __KERNEL__

Shouldn't be needed.

include/linux/dynamic_printk.h line 12

Quoted 7 lines

include/linux/dynamic_printk.h new file +7 -0

+#include <linux/string.h>
+#include <linux/hash.h>
+
+#define DYNAMIC_DEBUG_HASH_BITS 6
+#define DEBUG_HASH_TABLE_SIZE (1 << DYNAMIC_DEBUG_HASH_BITS)
+
+#define TYPE_BOOLEAN 1

What's this for?

include/linux/dynamic_printk.h line 20

Quoted 7 lines

include/linux/dynamic_printk.h new file +7 -0

+#define DYNAMIC_ENABLED_ALL 0
+#define DYNAMIC_ENABLED_NONE 1
+#define DYNAMIC_ENABLED_SOME 2
+
+extern int dynamic_enabled;
+extern long long dynamic_printk_enabled;
+extern long long dynamic_printk_enabled2;

What's up with the "2" versions?

Quoted 2 lines

What's up with the "2" versions?

"2" here is the second bitmask as explained previously.

Few notes...there is still one control file: <debugfs>/dynamic_printk/modules We can split this up now or later, but I kind of like being able to see all the controls in one file. Also, i've used a djb2 hash function in the code, which i'm not sure is under the correct license/copyright, so i just wanted to point that out as well. see: scripts/basic/hash.c. If its an issue, i can find another hash function.

---

 Documentation/kernel-parameters.txt |    5 
 include/asm-generic/vmlinux.lds.h   |   10 +
 include/linux/device.h              |    6 -
 include/linux/dynamic_printk.h      |   94 ++++++++
 include/linux/kernel.h              |    7 +
 include/linux/module.h              |    4 
 kernel/module.c                     |   22 ++
 lib/Kconfig.debug                   |   56 +++++
 lib/Makefile                        |    2 
 lib/dynamic_printk.c                |  409 +++++++++++++++++++++++++++++++++++
 net/netfilter/nf_conntrack_pptp.c   |    2 
 scripts/Makefile.lib                |   11 +
 scripts/basic/Makefile              |    2 
 scripts/basic/hash.c                |   64 +++++
 14 files changed, 688 insertions(+), 6 deletions(-)
 create mode 100644 include/linux/dynamic_printk.h
 create mode 100644 lib/dynamic_printk.c
 create mode 100644 scripts/basic/hash.c

lib/Kconfig.debug +0 -0

index 623ef24..6664783 100644
@@ -632,6 +632,62 @@ config FIREWIRE_OHCI_REMOTE_DMA
 
 	  If unsure, say N.
 

lib/Kconfig.debug +9 -0

+
+	  Enable a module:
+
+	  	$echo "set enabled=1 <module_name>" > dynamic_printk/modules
+
+	  Disable a module:
+
+	  	$echo "set enabled=0 <module_name>" > dynamic_printk/modules
+

lib/dynamic_printk.c new file +16 -0

+
+struct debug_name {
+	struct hlist_node hlist;
+	struct hlist_node hlist2;
+	int hash1;
+	int hash2;
+	char *name;
+	int enable;
+	int type;
+};
+
+static int nr_entries;
+static int num_enabled;
+int dynamic_enabled = DYNAMIC_ENABLED_NONE;
+static struct hlist_head module_table[DEBUG_HASH_TABLE_SIZE] =
+	{ [0 ... DEBUG_HASH_TABLE_SIZE-1] = HLIST_HEAD_INIT };

lib/dynamic_printk.c new file +8 -0

+static struct debug_name *find_debug_module(char *module_name)
+{
+	int i;
+	struct hlist_head *head;
+	struct hlist_node *node;
+	struct debug_name *element;
+
+	element = NULL;

lib/dynamic_printk.c new file +4 -0

+static struct debug_name *find_debug_module_hash(char *module_name, int hash)
+{
+	struct hlist_head *head;
+	struct hlist_node *node;

lib/dynamic_printk.c new file +51 -0

+int unregister_dynamic_debug_module(char *mod_name)
+{
+	struct debug_name *element;
+	int ret = 0;
+
+	down(&debug_list_mutex);
+	element = find_debug_module(mod_name);
+	if (!element) {
+		ret = -EINVAL;
+		goto out;
+	}
+	hlist_del_rcu(&element->hlist);
+	hlist_del_rcu(&element->hlist2);
+	synchronize_rcu();
+	if (element->name)
+		kfree(element->name);
+	if (element->enable)
+		num_enabled--;
+	kfree(element);
+	nr_entries--;
+out:
+	up(&debug_list_mutex);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(unregister_dynamic_debug_module);
+
+int register_dynamic_debug_module(char *mod_name, int type, char *share_name,
+					char *flags, int hash, int hash2)
+{
+	struct debug_name *elem;
+	int ret = 0;
+
+	down(&debug_list_mutex);
+	elem = find_debug_module(mod_name);
+	if (!elem) {
+		if (__add_debug_module(mod_name, hash, hash2))
+			goto out;
+		elem = find_debug_module(mod_name);
+		if (dynamic_enabled == DYNAMIC_ENABLED_ALL &&
+				!strcmp(mod_name, share_name)) {
+			elem->enable = true;
+			num_enabled++;
+		}
+	}
+	elem->type |= type;
+out:
+	up(&debug_list_mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(register_dynamic_debug_module);
+

scripts/basic/hash.c new file +53 -0

new file mode 100644
index 0000000..3299ad7
@@ -0,0 +1,64 @@
+/*
+ * Copyright (C) 2008 Red Hat, Inc., Jason Baron <[email protected]>
+ *
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#define DYNAMIC_DEBUG_HASH_BITS 6
+
+static const char *program;
+
+static void usage(void)
+{
+	printf("Usage: %s <djb2|r5> <modname>\n", program);
+	exit(1);
+}
+
+/* djb2 hashing algorithm by Dan Bernstein. From:
+ * http://www.cse.yorku.ca/~oz/hash.html
+ */
+
+unsigned int djb2_hash(char *str)
+{
+	unsigned long hash = 5381;
+	int c;
+
+	c = *str;
+	while (c) {
+		hash = ((hash << 5) + hash) + c;
+		c = *++str;
+	}
+	return (unsigned int)(hash & ((1 << DYNAMIC_DEBUG_HASH_BITS) - 1));
+}
+
+unsigned int r5_hash(char *str)
+{
+	unsigned long hash = 0;
+	int c;
+
+	c = *str;
+	while (c) {
+		hash = (hash + (c << 4) + (c >> 4)) * 11;
+		c = *++str;
+	}
+	return (unsigned int)(hash & ((1 << DYNAMIC_DEBUG_HASH_BITS) - 1));
+}
+
+int main(int argc, char *argv[])
+{
+	program = argv[0];
+

Quoted 6 lines · On Fri, 8 Aug 2008 17:51:53 -0400 Jason Baron wrote:

Few notes...there is still one control file: <debugfs>/dynamic_printk/modules We can split this up now or later, but I kind of like being able to see all the controls in one file. Also, i've used a djb2 hash function in the code, which i'm not sure is under the correct license/copyright, so i just wanted to point that out as well. see: scripts/basic/hash.c. If its an issue, i can find another hash function.

What license does it have? I don't see one.

Quoted 9 lines · Randy Dunlap

Quoted 6 lines · On Fri, 8 Aug 2008 17:51:53 -0400 Jason Baron wrote:

Few notes...there is still one control file: <debugfs>/dynamic_printk/modules We can split this up now or later, but I kind of like being able to see all the controls in one file. Also, i've used a djb2 hash function in the code, which i'm not sure is under the correct license/copyright, so i just wanted to point that out as well. see: scripts/basic/hash.c. If its an issue, i can find another hash function.

Quoted 2 lines

What license does it have? I don't see one.

hmmm...a search for djb2 finds a number of users, for example, i found this one which is covered by GPL2, http://www.telegraphics.com.au/svn/dpa/trunk/symtab.c
is that sufficient to say its ok?

Quoted 13 lines · Jason Baron

Quoted 11 lines · Randy Dunlap

Quoted 6 lines · On Fri, 8 Aug 2008 17:51:53 -0400 Jason Baron wrote:

Few notes...there is still one control file: <debugfs>/dynamic_printk/modules We can split this up now or later, but I kind of like being able to see all the controls in one file. Also, i've used a djb2 hash function in the code, which i'm not sure is under the correct license/copyright, so i just wanted to point that out as well. see: scripts/basic/hash.c. If its an issue, i can find another hash function.

What license does it have? I don't see one.

Quoted 3 lines

hmmm...a search for djb2 finds a number of users, for example, i found this one which is covered by GPL2, http://www.telegraphics.com.au/svn/dpa/trunk/symtab.c
is that sufficient to say its ok?

That's a tough call. I'd run that through the license board of your employer if you have any questions about this, as it is your name with their domain that is going to have the "signed-off-by:" pointing back to.

Quoted 14 lines · Greg KH

Quoted 13 lines · Jason Baron

Quoted 11 lines · Randy Dunlap

Quoted 6 lines · On Fri, 8 Aug 2008 17:51:53 -0400 Jason Baron wrote:

Few notes...there is still one control file: <debugfs>/dynamic_printk/modules We can split this up now or later, but I kind of like being able to see all the controls in one file. Also, i've used a djb2 hash function in the code, which i'm not sure is under the correct license/copyright, so i just wanted to point that out as well. see: scripts/basic/hash.c. If its an issue, i can find another hash function.

What license does it have? I don't see one.

Quoted 9 lines

Quoted 3 lines

hmmm...a search for djb2 finds a number of users, for example, i found this one which is covered by GPL2, http://www.telegraphics.com.au/svn/dpa/trunk/symtab.c
is that sufficient to say its ok?

That's a tough call. I'd run that through the license board of your employer if you have any questions about this, as it is your name with their domain that is going to have the "signed-off-by:" pointing back to.

We're ok with the patch as is...i'm including an updated version below, which contains some documentation improvements...as you mentioned before, perhaps you can include this patch through the driver tree. It should apply with minimal munging, but please point me at a tree, if there are any issues.

thanks,

-Jason

Signed-off-by Jason Baron <[email protected]>

So close, can I have a good changelog comment with the patch so people know what it is when they look in the logs?

Care to resend it with that?

Quoted 5 lines · Greg KH

So close, can I have a good changelog comment with the patch so people know what it is when they look in the logs?

Care to resend it with that?

Base infrastructure to enable per-module debug messages.

Quoted 8 lines · Jason Baron

Quoted 5 lines · Greg KH

So close, can I have a good changelog comment with the patch so people know what it is when they look in the logs?

Care to resend it with that?

Base infrastructure to enable per-module debug messages.

Ah, so close...

With this patch, I get the following build error:

CC [M] drivers/usb/gadget/u_ether.o
drivers/usb/gadget/u_ether.c: In function ___gether_setup___:
drivers/usb/gadget/u_ether.c:787: error: ___KBUILD_MODNAME___ undeclared (first use in this function) drivers/usb/gadget/u_ether.c:787: error: (Each undeclared identifier is reported only once drivers/usb/gadget/u_ether.c:787: error: for each function it appears in.) drivers/usb/gadget/u_ether.c:787: error: unknown field ___Usage___ specified in initializer drivers/usb/gadget/u_ether.c:787: error: expected expression before ___.___ token drivers/usb/gadget/u_ether.c:787: error: initializer element is not constant drivers/usb/gadget/u_ether.c:787: error: (near initialization for ___descriptor.hash___) drivers/usb/gadget/u_ether.c:787: error: ___Usage___ undeclared (first use in this function) drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary << (have ___long long int___ and ___char *___)

drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___:___ token
drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary & (have ___long long int___ and ___char *___) drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary << (have ___long long int___ and ___char *___) drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___:___ token
drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary & (have ___long long int___ and ___char *___) drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___:___ token
drivers/usb/gadget/u_ether.c:787: warning: passing argument 4 of _____dynamic_dbg_enabled_helper___ makes integer from pointer without a cast drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___KBUILD_MODNAME___

drivers/usb/gadget/u_ether.c:787: warning: too few arguments for format

Quoted 25 lines · On Tue, 12 Aug 2008 18:08:04 -0700 Greg KH <[email protected]> wrote:

Quoted 8 lines · Jason Baron

Quoted 5 lines · Greg KH

So close, can I have a good changelog comment with the patch so people know what it is when they look in the logs?

Care to resend it with that?

Base infrastructure to enable per-module debug messages.

Ah, so close...

With this patch, I get the following build error:

CC [M] drivers/usb/gadget/u_ether.o
drivers/usb/gadget/u_ether.c: In function ___gether_setup___:
drivers/usb/gadget/u_ether.c:787: error: ___KBUILD_MODNAME___ undeclared (first use in this function) drivers/usb/gadget/u_ether.c:787: error: (Each undeclared identifier is reported only once drivers/usb/gadget/u_ether.c:787: error: for each function it appears in.) drivers/usb/gadget/u_ether.c:787: error: unknown field ___Usage___ specified in initializer drivers/usb/gadget/u_ether.c:787: error: expected expression before ___.___ token drivers/usb/gadget/u_ether.c:787: error: initializer element is not constant drivers/usb/gadget/u_ether.c:787: error: (near initialization for ___descriptor.hash___) drivers/usb/gadget/u_ether.c:787: error: ___Usage___ undeclared (first use in this function) drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary << (have ___long long int___ and ___char *___)

Quoted 8 lines

drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___:___ token
drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary & (have ___long long int___ and ___char *___) drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary << (have ___long long int___ and ___char *___) drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___:___ token
drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary & (have ___long long int___ and ___char *___) drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___:___ token
drivers/usb/gadget/u_ether.c:787: warning: passing argument 4 of _____dynamic_dbg_enabled_helper___ makes integer from pointer without a cast drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___KBUILD_MODNAME___

(lol at gcc crap)

Quoted 1 line

drivers/usb/gadget/u_ether.c:787: warning: too few arguments for format

argh, not that again. We had this with a patch from Jesse a while back, which I fixed thusly:

Quoted 37 lines · Andrew Morton

Quoted 25 lines · On Tue, 12 Aug 2008 18:08:04 -0700 Greg KH <[email protected]> wrote:

Quoted 8 lines · Jason Baron

Quoted 5 lines · Greg KH

So close, can I have a good changelog comment with the patch so people know what it is when they look in the logs?

Care to resend it with that?

Base infrastructure to enable per-module debug messages.

Ah, so close...

With this patch, I get the following build error:

CC [M] drivers/usb/gadget/u_ether.o
drivers/usb/gadget/u_ether.c: In function ___gether_setup___:
drivers/usb/gadget/u_ether.c:787: error: ___KBUILD_MODNAME___ undeclared (first use in this function) drivers/usb/gadget/u_ether.c:787: error: (Each undeclared identifier is reported only once drivers/usb/gadget/u_ether.c:787: error: for each function it appears in.) drivers/usb/gadget/u_ether.c:787: error: unknown field ___Usage___ specified in initializer drivers/usb/gadget/u_ether.c:787: error: expected expression before ___.___ token drivers/usb/gadget/u_ether.c:787: error: initializer element is not constant drivers/usb/gadget/u_ether.c:787: error: (near initialization for ___descriptor.hash___) drivers/usb/gadget/u_ether.c:787: error: ___Usage___ undeclared (first use in this function) drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary << (have ___long long int___ and ___char *___)

Quoted 8 lines

drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___:___ token
drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary & (have ___long long int___ and ___char *___) drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary << (have ___long long int___ and ___char *___) drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___:___ token
drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary & (have ___long long int___ and ___char *___) drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___:___ token
drivers/usb/gadget/u_ether.c:787: warning: passing argument 4 of _____dynamic_dbg_enabled_helper___ makes integer from pointer without a cast drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___KBUILD_MODNAME___

Quoted 6 lines

(lol at gcc crap)

Quoted 1 line

drivers/usb/gadget/u_ether.c:787: warning: too few arguments for format

argh, not that again. We had this with a patch from Jesse a while back, which I fixed thusly:

<patch moving KBUILD_MODNAME from a inline function to a macro snipped>

Yeah, I remember that too, but in this case, KBUILD_MODNAME is being used in a macro, not an inline function, the exact opposite.

Sam, any ideas here? I'll bounce the original patch at you if you missed it last time.

Quoted 45 lines · Greg KH

Quoted 37 lines · Andrew Morton

Quoted 25 lines · On Tue, 12 Aug 2008 18:08:04 -0700 Greg KH <[email protected]> wrote:

Quoted 8 lines · Jason Baron

Quoted 5 lines · Greg KH

So close, can I have a good changelog comment with the patch so people know what it is when they look in the logs?

Care to resend it with that?

Base infrastructure to enable per-module debug messages.

Ah, so close...

With this patch, I get the following build error:

CC [M] drivers/usb/gadget/u_ether.o
drivers/usb/gadget/u_ether.c: In function ___gether_setup___:
drivers/usb/gadget/u_ether.c:787: error: ___KBUILD_MODNAME___ undeclared (first use in this function) drivers/usb/gadget/u_ether.c:787: error: (Each undeclared identifier is reported only once drivers/usb/gadget/u_ether.c:787: error: for each function it appears in.) drivers/usb/gadget/u_ether.c:787: error: unknown field ___Usage___ specified in initializer drivers/usb/gadget/u_ether.c:787: error: expected expression before ___.___ token drivers/usb/gadget/u_ether.c:787: error: initializer element is not constant drivers/usb/gadget/u_ether.c:787: error: (near initialization for ___descriptor.hash___) drivers/usb/gadget/u_ether.c:787: error: ___Usage___ undeclared (first use in this function) drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary << (have ___long long int___ and ___char *___)

Quoted 8 lines

drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___:___ token
drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary & (have ___long long int___ and ___char *___) drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary << (have ___long long int___ and ___char *___) drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___:___ token
drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary & (have ___long long int___ and ___char *___) drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___:___ token
drivers/usb/gadget/u_ether.c:787: warning: passing argument 4 of _____dynamic_dbg_enabled_helper___ makes integer from pointer without a cast drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___KBUILD_MODNAME___

Quoted 6 lines

(lol at gcc crap)

Quoted 1 line

drivers/usb/gadget/u_ether.c:787: warning: too few arguments for format

argh, not that again. We had this with a patch from Jesse a while back, which I fixed thusly:

Quoted 7 lines

<patch moving KBUILD_MODNAME from a inline function to a macro snipped>

Yeah, I remember that too, but in this case, KBUILD_MODNAME is being used in a macro, not an inline function, the exact opposite.

Sam, any ideas here? I'll bounce the original patch at you if you missed it last time.

I have not looked into the particular error but what happens is that u_ether.o is used by more than one module so kbuild does not know what value to assign the KBUILD_MODNAME and therefore decide not to assign KBUILD_MODNAME at all.

And because any usage of KBUILD_MODNAME is anyway flawed in u_ether.c as we do not know for what module it is being used it makes sense for kbuild to do so.

The above error happens because u_ether.c uses something that require KBUILD_MODNAME to be define.
So the correct fix is to avid usage of this macro in the source.

Note: The old error Andrew mentions was caused by alpha indirectly pulling in a inline function that used KBUILD_MODNAME - something that did not happen on the other architectures.

Sam

Quoted 50 lines · Greg KH

Quoted 37 lines · Andrew Morton

Quoted 25 lines · On Tue, 12 Aug 2008 18:08:04 -0700 Greg KH <[email protected]> wrote:

Quoted 8 lines · Jason Baron

Quoted 5 lines · Greg KH

So close, can I have a good changelog comment with the patch so people know what it is when they look in the logs?

Care to resend it with that?

Base infrastructure to enable per-module debug messages.

Ah, so close...

With this patch, I get the following build error:

CC [M] drivers/usb/gadget/u_ether.o
drivers/usb/gadget/u_ether.c: In function ___gether_setup___:
drivers/usb/gadget/u_ether.c:787: error: ___KBUILD_MODNAME___ undeclared (first use in this function) drivers/usb/gadget/u_ether.c:787: error: (Each undeclared identifier is reported only once drivers/usb/gadget/u_ether.c:787: error: for each function it appears in.) drivers/usb/gadget/u_ether.c:787: error: unknown field ___Usage___ specified in initializer drivers/usb/gadget/u_ether.c:787: error: expected expression before ___.___ token drivers/usb/gadget/u_ether.c:787: error: initializer element is not constant drivers/usb/gadget/u_ether.c:787: error: (near initialization for ___descriptor.hash___) drivers/usb/gadget/u_ether.c:787: error: ___Usage___ undeclared (first use in this function) drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary << (have ___long long int___ and ___char *___)

Quoted 8 lines

drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___:___ token
drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary & (have ___long long int___ and ___char *___) drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary << (have ___long long int___ and ___char *___) drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___:___ token
drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary & (have ___long long int___ and ___char *___) drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___:___ token
drivers/usb/gadget/u_ether.c:787: warning: passing argument 4 of _____dynamic_dbg_enabled_helper___ makes integer from pointer without a cast drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___KBUILD_MODNAME___

Quoted 6 lines

(lol at gcc crap)

Quoted 1 line

drivers/usb/gadget/u_ether.c:787: warning: too few arguments for format

argh, not that again. We had this with a patch from Jesse a while back, which I fixed thusly:

<patch moving KBUILD_MODNAME from a inline function to a macro snipped>

Yeah, I remember that too, but in this case, KBUILD_MODNAME is being used in a macro, not an inline function, the exact opposite.

Quoted 2 lines

Sam, any ideas here? I'll bounce the original patch at you if you missed it last time.

I have not looked into the particular error but what happens is that u_ether.o is used by more than one module so kbuild does not know what value to assign the KBUILD_MODNAME and therefore decide not to assign KBUILD_MODNAME at all.

Quoted 51 lines · Sam Ravnborg

Quoted 50 lines · Greg KH

Quoted 37 lines · Andrew Morton

Quoted 25 lines · On Tue, 12 Aug 2008 18:08:04 -0700 Greg KH <[email protected]> wrote:

Quoted 8 lines · Jason Baron

Quoted 5 lines · Greg KH

So close, can I have a good changelog comment with the patch so people know what it is when they look in the logs?

Care to resend it with that?

Base infrastructure to enable per-module debug messages.

Ah, so close...

With this patch, I get the following build error:

CC [M] drivers/usb/gadget/u_ether.o
drivers/usb/gadget/u_ether.c: In function ___gether_setup___:
drivers/usb/gadget/u_ether.c:787: error: ___KBUILD_MODNAME___ undeclared (first use in this function) drivers/usb/gadget/u_ether.c:787: error: (Each undeclared identifier is reported only once drivers/usb/gadget/u_ether.c:787: error: for each function it appears in.) drivers/usb/gadget/u_ether.c:787: error: unknown field ___Usage___ specified in initializer drivers/usb/gadget/u_ether.c:787: error: expected expression before ___.___ token drivers/usb/gadget/u_ether.c:787: error: initializer element is not constant drivers/usb/gadget/u_ether.c:787: error: (near initialization for ___descriptor.hash___) drivers/usb/gadget/u_ether.c:787: error: ___Usage___ undeclared (first use in this function) drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary << (have ___long long int___ and ___char *___)

Quoted 8 lines

drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___:___ token
drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary & (have ___long long int___ and ___char *___) drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary << (have ___long long int___ and ___char *___) drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___:___ token
drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary & (have ___long long int___ and ___char *___) drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___:___ token
drivers/usb/gadget/u_ether.c:787: warning: passing argument 4 of _____dynamic_dbg_enabled_helper___ makes integer from pointer without a cast drivers/usb/gadget/u_ether.c:787: error: expected ___)___ before ___KBUILD_MODNAME___

Quoted 6 lines

(lol at gcc crap)

Quoted 1 line

drivers/usb/gadget/u_ether.c:787: warning: too few arguments for format

argh, not that again. We had this with a patch from Jesse a while back, which I fixed thusly:

<patch moving KBUILD_MODNAME from a inline function to a macro snipped>

Yeah, I remember that too, but in this case, KBUILD_MODNAME is being used in a macro, not an inline function, the exact opposite.

Quoted 7 lines

Quoted 2 lines

Sam, any ideas here? I'll bounce the original patch at you if you missed it last time.

I have not looked into the particular error but what happens is that u_ether.o is used by more than one module so kbuild does not know what value to assign the KBUILD_MODNAME and therefore decide not to assign KBUILD_MODNAME at all.

Ick.

Hm, how do I fix this? Build u_ether.c into two different .o files, depending on which module it is being included into?

Or do:

	#include "u_ether.c"

in the source code that was linking it in?

Actually, as ugly as that last one is, it probably would solve this issue, and be the same end-result, right?

Quoted 8 lines · Jason Baron

Quoted 5 lines · Greg KH

So close, can I have a good changelog comment with the patch so people know what it is when they look in the logs?

Care to resend it with that?

Base infrastructure to enable per-module debug messages.

Ah, so close...

With this patch, I get the following build error:

CC [M] drivers/usb/gadget/u_ether.o
drivers/usb/gadget/u_ether.c: In function ‘gether_setup’:
drivers/usb/gadget/u_ether.c:787: error: ‘KBUILD_MODNAME’ undeclared (first use in this function) drivers/usb/gadget/u_ether.c:787: error: (Each undeclared identifier is reported only once drivers/usb/gadget/u_ether.c:787: error: for each function it appears in.)
drivers/usb/gadget/u_ether.c:787: error: unknown field ‘Usage’ specified in initializer drivers/usb/gadget/u_ether.c:787: error: expected expression before ‘.’ token
drivers/usb/gadget/u_ether.c:787: error: initializer element is not constant
drivers/usb/gadget/u_ether.c:787: error: (near initialization for ‘descriptor.hash’) drivers/usb/gadget/u_ether.c:787: error: ‘Usage’ undeclared (first use in this function) drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary << (have ‘long long int’ and ‘char *’) drivers/usb/gadget/u_ether.c:787: error: expected ‘)’ before ‘:’ token
drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary & (have ‘long long int’ and ‘char *’) drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary << (have ‘long long int’ and ‘char *’) drivers/usb/gadget/u_ether.c:787: error: expected ‘)’ before ‘:’ token
drivers/usb/gadget/u_ether.c:787: error: invalid operands to binary & (have ‘long long int’ and ‘char *’) drivers/usb/gadget/u_ether.c:787: error: expected ‘)’ before ‘:’ token
drivers/usb/gadget/u_ether.c:787: warning: passing argument 4 of ‘__dynamic_dbg_enabled_helper’ makes integer from pointer without a cast drivers/usb/gadget/u_ether.c:787: error: expected ‘)’ before ‘KBUILD_MODNAME’
drivers/usb/gadget/u_ether.c:787: warning: too few arguments for format
make[1]: *** [drivers/usb/gadget/u_ether.o] Error 1
make: *** [_module_drivers/usb/gadget] Error 2

Did you try a simple 'make allmodconfig'?

Also, I cleaned it up a bit to make it pass checkpatch.pl and sparse, doesn't anyone run these things anymore...

I've attached my fixed up version below.

Any ideas?

Quoted 5 lines · Greg KH

So close, can I have a good changelog comment with the patch so people know what it is when they look in the logs?

Care to resend it with that?

Base infrastructure to enable per-module debug messages.

Also, would it make sense to add a patch like the one below to this code? It might be nice to know when each module's debugging value is manually changed by the user for the log files.

If you like it, I can merge it in the main patch.

thanks,

greg k-h

---
 lib/dynamic_printk.c |    6 ++++++
 1 file changed, 6 insertions(+)

lib/dynamic_printk.c +3 -0

@@ -288,6 +288,9 @@ static ssize_t pr_debug_write(struct fil
 					num_enabled++;
 					dynamic_enabled = DYNAMIC_ENABLED_SOME;
 					err = 0;
+					printk(KERN_DEBUG
+					       "debugging enabled for module %s",
+					       elem->name);
 				} else if (!value && (elem->enable == 1)) {
 					elem->enable = 0;
 					num_enabled--;

include/linux/module.h +12 -0

@@ -1717,6 +1718,9 @@ static struct module *load_module(void __user *umod,
 	unsigned int unusedgplcrcindex;
 	unsigned int markersindex;
 	unsigned int markersstringsindex;
+	unsigned int verboseindex;
+	struct mod_debug *iter;
+	unsigned long value;
 	struct module *mod;
 	long err = 0;
 	void *percpu = NULL, *ptr = NULL; /* Stops spurious gcc warning */

@@ -1993,6 +1997,7 @@ static struct module *load_module(void __user *umod,
 	markersindex = find_sec(hdr, sechdrs, secstrings, "__markers");
  	markersstringsindex = find_sec(hdr, sechdrs, secstrings,
 					"__markers_strings");
+	verboseindex = find_sec(hdr, sechdrs, secstrings, "__verbose");
 
 	/* Now do relocations. */
 	for (i = 1; i < hdr->e_shnum; i++) {

@@ -2020,6 +2025,11 @@ static struct module *load_module(void __user *umod,
 	mod->num_markers =
 		sechdrs[markersindex].sh_size / sizeof(*mod->markers);
 #endif
+#ifdef CONFIG_DYNAMIC_PRINTK_DEBUG
+	mod->start_verbose = (void *)sechdrs[verboseindex].sh_addr;
+	mod->num_verbose = sechdrs[verboseindex].sh_size /
+				sizeof(*mod->start_verbose);
+#endif
 
         /* Find duplicate symbols */
 	err = verify_export_symbols(mod);

@@ -2043,6 +2053,19 @@ static struct module *load_module(void __user *umod,
 		marker_update_probe_range(mod->markers,
 			mod->markers + mod->num_markers);
 #endif
+#ifdef CONFIG_DYNAMIC_PRINTK_DEBUG
+	for (value = (unsigned long)mod->start_verbose;
+		value < (unsigned long)mod->start_verbose +