kernel/module: fix warning when !CONFIG_DYNAMIC_PRINTK_DEBUG

PATCH In review Alexander Beregalov 4 participants 6 messages Aug 20, 2008 – Sep 11, 2008 Module support

lore

Share & tools

lore

Alexander Beregalov

author

2008-08-20 14:13 UTC #0 lore raw

PATCH 1/1 kernel/module: fix warning when !CONFIG_DYNAMIC_PRINTK_DEBUG

From: Alexander Beregalov <[email protected]>

kernel/module.c: In function 'load_module':
kernel/module.c:1839: warning: unused variable 'value' kernel/module.c:1838: warning: unused variable 'iter'

Signed-off-by Alexander Beregalov <[email protected]>

---

 kernel/module.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

kernel/module.c +2 -0

index 1721320..5a1258e 100644
@@ -1835,8 +1835,10 @@ static struct module *load_module(void __user *umod,
 	unsigned int markersindex;
 	unsigned int markersstringsindex;
 	unsigned int verboseindex;
+#ifdef CONFIG_DYNAMIC_PRINTK_DEBUG
 	struct mod_debug *iter;
 	unsigned long value;
+#endif
 	unsigned int tracepointsindex;
 	unsigned int tracepointsstringsindex;
 	struct module *mod;

Greg KH

2008-08-20 14:27 UTC #1 lore raw

Quoted 18 lines · Alexander Beregalov

From: Alexander Beregalov <[email protected]>
kernel/module.c: In function 'load_module':
kernel/module.c:1839: warning: unused variable 'value' kernel/module.c:1838: warning: unused variable 'iter'
Signed-off-by Alexander Beregalov <[email protected]>
---
 kernel/module.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
kernel/module.c +0 -0
index 1721320..5a1258e 100644

kernel/module.c static struct module *load_module(void __user *umod, · line 1841

Quoted 7 lines

kernel/module.c +2 -0

@@ -1835,8 +1835,10 @@ static struct module *load_module(void __user *umod,
 	unsigned int markersindex;
 	unsigned int markersstringsindex;
 	unsigned int verboseindex;
+#ifdef CONFIG_DYNAMIC_PRINTK_DEBUG
 	struct mod_debug *iter;
 	unsigned long value;
+#endif

Hm, that's not very nice, we try to keep #ifdefs out of the .c file where ever possible, especially within a single function. I'll split the section out of this function that has the #ifdef which should fix this problem.

thanks,

greg k-h

Greg KH

2008-08-20 15:16 UTC #2 lore raw

Quoted 23 lines · Greg KH

Quoted 22 lines · Alexander Beregalov
From: Alexander Beregalov <[email protected]>
kernel/module.c: In function 'load_module':
kernel/module.c:1839: warning: unused variable 'value' kernel/module.c:1838: warning: unused variable 'iter'
Signed-off-by Alexander Beregalov <[email protected]>
---
 kernel/module.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
kernel/module.c +1 -0
index 1721320..5a1258e 100644
@@ -1835,8 +1835,10 @@ static struct module *load_module(void __user *umod,
 	unsigned int markersindex;
 	unsigned int markersstringsindex;
 	unsigned int verboseindex;
+#ifdef CONFIG_DYNAMIC_PRINTK_DEBUG

Quoted 8 lines

kernel/module.c static struct module *load_module(void __user *umod, · line 1841
Quoted 3 lines
kernel/module.c +1 -0
 	struct mod_debug *iter;
 	unsigned long value;
+#endif
Hm, that's not very nice, we try to keep #ifdefs out of the .c file where ever possible, especially within a single function. I'll split the section out of this function that has the #ifdef which should fix this problem.

I'll merge in this patch below, unless someone objects.

thanks,

greg k-h

---
 kernel/module.c |   54 ++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 20 deletions(-)

kernel/module.c +34 ~~-20~~

@@ -1778,12 +1778,44 @@ static void add_kallsyms(struct module *
 static inline void add_kallsyms(struct module *mod,
 				Elf_Shdr *sechdrs,
 				unsigned int symindex,
-				unsigned int strindex,
 				const char *secstrings)
 {
 }
 #endif /* CONFIG_KALLSYMS */
 
+#ifdef CONFIG_DYNAMIC_PRINTK_DEBUG
+static void dynamic_printk_setup(struct module *mod,
+				 Elf_Shdr *sechdrs,
+				 unsigned int verboseindex,
+				 const char *secstrings)
+{
+	struct mod_debug *iter;
+	unsigned long value;
+
+	mod->start_verbose = (void *)sechdrs[verboseindex].sh_addr;
+	mod->num_verbose = sechdrs[verboseindex].sh_size /
+				sizeof(*mod->start_verbose);
+
+	for (value = (unsigned long)mod->start_verbose;
+		value < (unsigned long)mod->start_verbose +
+		(unsigned long)(mod->num_verbose * sizeof(struct mod_debug));
+		value += sizeof(struct mod_debug)) {
+			iter = (struct mod_debug *)value;
+			register_dynamic_debug_module(iter->modname,
+				iter->type,
+				iter->logical_modname,
+				iter->flag_names, iter->hash, iter->hash2);
+	}
+}
+#else
+static inline void dynamic_printk_setup(struct module *mod,
+					Elf_Shdr *sechdrs,
+					unsigned int symindex,
+					const char *secstrings)
+{
+}
+#endif /* CONFIG_DYNAMIC_PRINTK_DEBUG */
+
 static void *module_alloc_update_bounds(unsigned long size)
 {
 	void *ret = module_alloc(size);

@@ -1833,8 +1865,6 @@ static struct module *load_module(void _
 	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 */

@@ -2149,11 +2179,6 @@ static struct module *load_module(void _
 	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);

@@ -2177,18 +2202,7 @@ static struct module *load_module(void _
 		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 +
-		(unsigned long)(mod->num_verbose * sizeof(struct mod_debug));
-		value += sizeof(struct mod_debug)) {
-			iter = (struct mod_debug *)value;
-			register_dynamic_debug_module(iter->modname,
-				iter->type,
-				iter->logical_modname,
-				iter->flag_names, iter->hash, iter->hash2);
-	}
-#endif
+	dynamic_printk_setup(mod, sechdrs, verboseindex, secstrings);
 	err = module_finalize(hdr, sechdrs, mod);
 	if (err < 0)
 		goto cleanup;

Jason Baron

2008-08-21 20:36 UTC #3 lore raw

Quoted 89 lines · Greg KH

I'll merge in this patch below, unless someone objects.

thanks,

greg k-h

---
 kernel/module.c |   54 ++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 20 deletions(-)

kernel/module.c +33 ~~-14~~

@@ -1778,12 +1778,44 @@ static void add_kallsyms(struct module *
 static inline void add_kallsyms(struct module *mod,
 				Elf_Shdr *sechdrs,
 				unsigned int symindex,
-				unsigned int strindex,
 				const char *secstrings)
 {
 }
 #endif /* CONFIG_KALLSYMS */
 
+#ifdef CONFIG_DYNAMIC_PRINTK_DEBUG
+static void dynamic_printk_setup(struct module *mod,
+				 Elf_Shdr *sechdrs,
+				 unsigned int verboseindex,
+				 const char *secstrings)
+{
+	struct mod_debug *iter;
+	unsigned long value;
+
+	mod->start_verbose = (void *)sechdrs[verboseindex].sh_addr;
+	mod->num_verbose = sechdrs[verboseindex].sh_size /
+				sizeof(*mod->start_verbose);
+
+	for (value = (unsigned long)mod->start_verbose;
+		value < (unsigned long)mod->start_verbose +
+		(unsigned long)(mod->num_verbose * sizeof(struct mod_debug));
+		value += sizeof(struct mod_debug)) {
+			iter = (struct mod_debug *)value;
+			register_dynamic_debug_module(iter->modname,
+				iter->type,
+				iter->logical_modname,
+				iter->flag_names, iter->hash, iter->hash2);
+	}
+}
+#else
+static inline void dynamic_printk_setup(struct module *mod,
+					Elf_Shdr *sechdrs,
+					unsigned int symindex,
+					const char *secstrings)
+{
+}
+#endif /* CONFIG_DYNAMIC_PRINTK_DEBUG */
+
 static void *module_alloc_update_bounds(unsigned long size)
 {
 	void *ret = module_alloc(size);

@@ -1833,8 +1865,6 @@ static struct module *load_module(void _
 	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 */

@@ -2149,11 +2179,6 @@ static struct module *load_module(void _
 	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);

@@ -2177,18 +2202,7 @@ static struct module *load_module(void _
 		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 +
-		(unsigned long)(mod->num_verbose * sizeof(struct mod_debug));
-		value += sizeof(struct mod_debug)) {
-			iter = (struct mod_debug *)value;

kernel/module.c static struct module *load_module(void _ · line 2208

Quoted 10 lines

kernel/module.c +1 -6

-			register_dynamic_debug_module(iter->modname,
-				iter->type,
-				iter->logical_modname,
-				iter->flag_names, iter->hash, iter->hash2);
-	}
-#endif
+	dynamic_printk_setup(mod, sechdrs, verboseindex, secstrings);
 	err = module_finalize(hdr, sechdrs, mod);
 	if (err < 0)
 		goto cleanup;

looks much cleaner. thanks.

we can simplify this module code a bit more...how about the following on top of what you did?

thanks,

-Jason

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

---

 include/linux/module.h |    5 -----
 kernel/module.c        |   40 +++++++++++++++++-----------------------
 2 files changed, 17 insertions(+), 28 deletions(-)

include/linux/module.h +0 -5

index 3336bd3..5d2970c 100644
@@ -350,11 +350,6 @@ struct module
 	/* Reference counts */
 	struct module_ref ref[NR_CPUS];
 #endif
-
-#ifdef CONFIG_DYNAMIC_PRINTK_DEBUG
-	struct mod_debug *start_verbose;
-	unsigned int num_verbose;
-#endif
 };
 #ifndef MODULE_ARCH_INIT
 #define MODULE_ARCH_INIT {}

kernel/module.c +17 ~~-23~~

index b227ca4..dc5fa65 100644
@@ -1787,34 +1787,28 @@ static inline void add_kallsyms(struct module *mod,
 #endif /* CONFIG_KALLSYMS */
 
 #ifdef CONFIG_DYNAMIC_PRINTK_DEBUG
-static void dynamic_printk_setup(struct module *mod,
-				 Elf_Shdr *sechdrs,
-				 unsigned int verboseindex,
-				 const char *secstrings)
+static void dynamic_printk_setup(Elf_Shdr *sechdrs, unsigned int verboseindex)
 {
-	struct mod_debug *iter;
-	unsigned long value;
+	struct mod_debug *debug_info;
+	unsigned long pos, end;
+	unsigned int num_verbose;
 
-	mod->start_verbose = (void *)sechdrs[verboseindex].sh_addr;
-	mod->num_verbose = sechdrs[verboseindex].sh_size /
-				sizeof(*mod->start_verbose);
+	pos = sechdrs[verboseindex].sh_addr;
+	num_verbose = sechdrs[verboseindex].sh_size /
+				sizeof(struct mod_debug);
+	end = pos + (num_verbose * sizeof(struct mod_debug));
 
-	for (value = (unsigned long)mod->start_verbose;
-		value < (unsigned long)mod->start_verbose +
-		(unsigned long)(mod->num_verbose * sizeof(struct mod_debug));
-		value += sizeof(struct mod_debug)) {
-			iter = (struct mod_debug *)value;
-			register_dynamic_debug_module(iter->modname,
-				iter->type,
-				iter->logical_modname,
-				iter->flag_names, iter->hash, iter->hash2);
+	for (; pos < end; pos += sizeof(struct mod_debug)) {
+		debug_info = (struct mod_debug *)pos;
+		register_dynamic_debug_module(debug_info->modname,
+			debug_info->type, debug_info->logical_modname,
+			debug_info->flag_names, debug_info->hash, 
+			debug_info->hash2);
 	}
 }
 #else
-static inline void dynamic_printk_setup(struct module *mod,
-					Elf_Shdr *sechdrs,
-					unsigned int symindex,
-					const char *secstrings)
+static inline void dynamic_printk_setup(Elf_Shdr *sechdrs, 
+					unsigned int verboseindex)
 {
 }
 #endif /* CONFIG_DYNAMIC_PRINTK_DEBUG */

@@ -2221,7 +2215,7 @@ static struct module *load_module(void __user *umod,
 			mod->tracepoints + mod->num_tracepoints);
 #endif
 	}
-	dynamic_printk_setup(mod, sechdrs, verboseindex, secstrings);
+	dynamic_printk_setup(sechdrs, verboseindex);
 	err = module_finalize(hdr, sechdrs, mod);
 	if (err < 0)
 		goto cleanup;

Jason Baron

2008-08-21 20:45 UTC #4 lore raw

Quoted 9 lines · Jason Baron

looks much cleaner. thanks.
we can simplify this module code a bit more...how about the following on top of what you did?
thanks,
-Jason

or better yet, without trailing whitespace...

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

---

 include/linux/module.h |    5 -----
 kernel/module.c        |   40 +++++++++++++++++-----------------------
 2 files changed, 17 insertions(+), 28 deletions(-)

include/linux/module.h +0 -5

index 3336bd3..5d2970c 100644
@@ -350,11 +350,6 @@ struct module
 	/* Reference counts */
 	struct module_ref ref[NR_CPUS];
 #endif
-
-#ifdef CONFIG_DYNAMIC_PRINTK_DEBUG
-	struct mod_debug *start_verbose;
-	unsigned int num_verbose;
-#endif
 };
 #ifndef MODULE_ARCH_INIT
 #define MODULE_ARCH_INIT {}

kernel/module.c +17 ~~-23~~

index b227ca4..2f69b81 100644
@@ -1787,34 +1787,28 @@ static inline void add_kallsyms(struct module *mod,
 #endif /* CONFIG_KALLSYMS */
 
 #ifdef CONFIG_DYNAMIC_PRINTK_DEBUG
-static void dynamic_printk_setup(struct module *mod,
-				 Elf_Shdr *sechdrs,
-				 unsigned int verboseindex,
-				 const char *secstrings)
+static void dynamic_printk_setup(Elf_Shdr *sechdrs, unsigned int verboseindex)
 {
-	struct mod_debug *iter;
-	unsigned long value;
+	struct mod_debug *debug_info;
+	unsigned long pos, end;
+	unsigned int num_verbose;
 
-	mod->start_verbose = (void *)sechdrs[verboseindex].sh_addr;
-	mod->num_verbose = sechdrs[verboseindex].sh_size /
-				sizeof(*mod->start_verbose);
+	pos = sechdrs[verboseindex].sh_addr;
+	num_verbose = sechdrs[verboseindex].sh_size /
+				sizeof(struct mod_debug);
+	end = pos + (num_verbose * sizeof(struct mod_debug));
 
-	for (value = (unsigned long)mod->start_verbose;
-		value < (unsigned long)mod->start_verbose +
-		(unsigned long)(mod->num_verbose * sizeof(struct mod_debug));
-		value += sizeof(struct mod_debug)) {
-			iter = (struct mod_debug *)value;
-			register_dynamic_debug_module(iter->modname,
-				iter->type,
-				iter->logical_modname,
-				iter->flag_names, iter->hash, iter->hash2);
+	for (; pos < end; pos += sizeof(struct mod_debug)) {
+		debug_info = (struct mod_debug *)pos;
+		register_dynamic_debug_module(debug_info->modname,
+			debug_info->type, debug_info->logical_modname,
+			debug_info->flag_names, debug_info->hash,
+			debug_info->hash2);
 	}
 }
 #else
-static inline void dynamic_printk_setup(struct module *mod,
-					Elf_Shdr *sechdrs,
-					unsigned int symindex,
-					const char *secstrings)
+static inline void dynamic_printk_setup(Elf_Shdr *sechdrs,
+					unsigned int verboseindex)
 {
 }
 #endif /* CONFIG_DYNAMIC_PRINTK_DEBUG */

@@ -2221,7 +2215,7 @@ static struct module *load_module(void __user *umod,
 			mod->tracepoints + mod->num_tracepoints);
 #endif
 	}
-	dynamic_printk_setup(mod, sechdrs, verboseindex, secstrings);
+	dynamic_printk_setup(sechdrs, verboseindex);
 	err = module_finalize(hdr, sechdrs, mod);
 	if (err < 0)
 		goto cleanup;

Greg KH

2008-09-11 10:13 UTC #5 lore raw

Quoted 6 lines · Jason Baron

Quoted 5 lines · Jason Baron
looks much cleaner. thanks.
we can simplify this module code a bit more...how about the following on top of what you did?

Quoted 6 lines

Quoted 4 lines
thanks,
-Jason
or better yet, without trailing whitespace...

Looks good, thanks, I've merged it in.

greg k-h