[PATCH 1/3] params.c: fix Smack complaint about parse_args

[PATCH 1/3] params.c: fix Smack complaint about parse_args

[Jim Cromie]

In commit 9fb48c744: "params: add 3rd arg to option handler callback
signature", the if-guard added to the pr_debug was overzealous; no
callers pass NULL, and existing code above and below the guard assumes
as much.  Change the if-guard to match, and silence the Smack
complaint.

CC: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 kernel/params.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/params.c b/kernel/params.c
index b60e2c7..be78c90 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -190,7 +190,7 @@ int parse_args(const char *doing,
 	/* Chew leading spaces */
 	args = skip_spaces(args);
 
-	if (args && *args)
+	if (*args)
 		pr_debug("doing %s, parsing ARGS: '%s'\n", doing, args);
 
 	while (*args) {
-- 
1.7.8.1

[PATCH 2/3] params: replace printk(KERN_<LVL>...) with pr_<lvl>(...)

[Jim Cromie]

I left 1 printk which uses __FILE__, __LINE__ explicitly, which should
not be subject to generic preferences expressed via pr_fmt().

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 kernel/params.c |   33 ++++++++++++---------------------
 1 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/kernel/params.c b/kernel/params.c
index be78c90..08414ba 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -201,25 +201,21 @@ int parse_args(const char *doing,
 		irq_was_disabled = irqs_disabled();
 		ret = parse_one(param, val, doing, params, num,
 				min_level, max_level, unknown);
-		if (irq_was_disabled && !irqs_disabled()) {
-			printk(KERN_WARNING "parse_args(): option '%s' enabled "
-					"irq's!\n", param);
-		}
+		if (irq_was_disabled && !irqs_disabled())
+			pr_warn("option '%s' enabled irq's!\n", param);
+
 		switch (ret) {
 		case -ENOENT:
-			printk(KERN_ERR "%s: Unknown parameter `%s'\n",
-			       doing, param);
+			pr_err("%s: Unknown parameter `%s'\n", doing, param);
 			return ret;
 		case -ENOSPC:
-			printk(KERN_ERR
-			       "%s: `%s' too large for parameter `%s'\n",
+			pr_err("%s: `%s' too large for parameter `%s'\n",
 			       doing, val ?: "", param);
 			return ret;
 		case 0:
 			break;
 		default:
-			printk(KERN_ERR
-			       "%s: `%s' invalid for parameter `%s'\n",
+			pr_err("%s: `%s' invalid for parameter `%s'\n",
 			       doing, val ?: "", param);
 			return ret;
 		}
@@ -266,8 +262,7 @@ STANDARD_PARAM_DEF(ulong, unsigned long, "%lu", unsigned long, strict_strtoul);
 int param_set_charp(const char *val, const struct kernel_param *kp)
 {
 	if (strlen(val) > 1024) {
-		printk(KERN_ERR "%s: string parameter too long\n",
-		       kp->name);
+		pr_err("%s: string parameter too long\n", kp->name);
 		return -ENOSPC;
 	}
 
@@ -403,8 +398,7 @@ static int param_array(const char *name,
 		int len;
 
 		if (*num == max) {
-			printk(KERN_ERR "%s: can only take %i arguments\n",
-			       name, max);
+			pr_err("%s: can only take %i arguments\n", name, max);
 			return -EINVAL;
 		}
 		len = strcspn(val, ",");
@@ -423,8 +417,7 @@ static int param_array(const char *name,
 	} while (save == ',');
 
 	if (*num < min) {
-		printk(KERN_ERR "%s: needs at least %i arguments\n",
-		       name, min);
+		pr_err("%s: needs at least %i arguments\n", name, min);
 		return -EINVAL;
 	}
 	return 0;
@@ -483,7 +476,7 @@ int param_set_copystring(const char *val, const struct kernel_param *kp)
 	const struct kparam_string *kps = kp->str;
 
 	if (strlen(val)+1 > kps->maxlen) {
-		printk(KERN_ERR "%s: string doesn't fit in %u chars.\n",
+		pr_err("%s: string doesn't fit in %u chars.\n",
 		       kp->name, kps->maxlen-1);
 		return -ENOSPC;
 	}
@@ -753,11 +746,9 @@ static struct module_kobject * __init locate_module_kobject(const char *name)
 #endif
 		if (err) {
 			kobject_put(&mk->kobj);
-			printk(KERN_ERR
-				"Module '%s' failed add to sysfs, error number %d\n",
+			pr_err("Module '%s' failed add to sysfs, error: %d\n"
+				"The system will be unstable now.\n",
 				name, err);
-			printk(KERN_ERR
-				"The system will be unstable now.\n");
 			return NULL;
 		}
 
-- 
1.7.8.1

[PATCH 3/3] dynamic_debug: remove unneeded includes

[Jim Cromie]

These arent currently needed, so drop them.  Some will probably get
re-added when static-branches are added, but include loops prevent
that at present.

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

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 7ca29a0..fc5d270 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -14,24 +14,14 @@
 
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/moduleparam.h>
-#include <linux/kallsyms.h>
-#include <linux/types.h>
 #include <linux/mutex.h>
-#include <linux/proc_fs.h>
 #include <linux/seq_file.h>
-#include <linux/list.h>
-#include <linux/sysctl.h>
 #include <linux/ctype.h>
-#include <linux/string.h>
-#include <linux/uaccess.h>
 #include <linux/dynamic_debug.h>
 #include <linux/debugfs.h>
 #include <linux/slab.h>
-#include <linux/jump_label.h>
 #include <linux/hardirq.h>
 #include <linux/sched.h>
-#include <linux/device.h>
 #include <linux/netdevice.h>
 
 extern struct _ddebug __start___verbose[];
-- 
1.7.8.1

Re: [PATCH 2/3] params: replace printk(KERN_<LVL>...) with pr_<lvl>(...)

[Joe Perches]

On Thu, 2012-05-03 at 11:57 -0600, Jim Cromie wrote:
> I left 1 printk which uses __FILE__, __LINE__ explicitly, which should
> not be subject to generic preferences expressed via pr_fmt().
> 
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
>  kernel/params.c |   33 ++++++++++++---------------------
>  1 files changed, 12 insertions(+), 21 deletions(-)
> 
> diff --git a/kernel/params.c b/kernel/params.c
> index be78c90..08414ba 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -201,25 +201,21 @@ int parse_args(const char *doing,
>  		irq_was_disabled = irqs_disabled();
>  		ret = parse_one(param, val, doing, params, num,
>  				min_level, max_level, unknown);
> -		if (irq_was_disabled && !irqs_disabled()) {
> -			printk(KERN_WARNING "parse_args(): option '%s' enabled "
> -					"irq's!\n", param);
> -		}
> +		if (irq_was_disabled && !irqs_disabled())
> +			pr_warn("option '%s' enabled irq's!\n", param);
> +

The other parse_args pr_<level> uses have
	'"%s: ...", doing, ...'.
Maybe this one should too.

>  		switch (ret) {
>  		case -ENOENT:
> -			printk(KERN_ERR "%s: Unknown parameter `%s'\n",
> -			       doing, param);
> +			pr_err("%s: Unknown parameter `%s'\n", doing, param);
>  			return ret;
[]
> @@ -753,11 +746,9 @@ static struct module_kobject * __init locate_module_kobject(const char *name)
>  #endif
>  		if (err) {
>  			kobject_put(&mk->kobj);
> -			printk(KERN_ERR
> -				"Module '%s' failed add to sysfs, error number %d\n",
> +			pr_err("Module '%s' failed add to sysfs, error: %d\n"
> +				"The system will be unstable now.\n",
>  				name, err);
> -			printk(KERN_ERR
> -				"The system will be unstable now.\n");
>  			return NULL;
>  		}
>  

It'd be nice to align the arguments and perhaps a
single line output may be better.

Maybe something like:

			pr_err("Adding module '%s' to sysfs failed (%d), the system is unstable\n",
			       name, err);

though perhaps this should be pr_crit
if it's really unstable.

[PATCH 2/3] params: replace printk(KERN_<LVL>...) with pr_<lvl>(...)

[Jim Cromie]

I left 1 printk which uses __FILE__, __LINE__ explicitly, which should
not be subject to generic preferences expressed via pr_fmt().

+ tweaks suggested by Joe Perches:
- add doing to irq-enabled warning, like others.  It wont happen often..
- change sysfs failure crit, not just err, make it 1 line in logs.

cc: Joe Perches <joe@perches.com>
Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 kernel/params.c |   34 +++++++++++++---------------------
 1 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/kernel/params.c b/kernel/params.c
index be78c90..2332e0b 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -201,25 +201,22 @@ int parse_args(const char *doing,
 		irq_was_disabled = irqs_disabled();
 		ret = parse_one(param, val, doing, params, num,
 				min_level, max_level, unknown);
-		if (irq_was_disabled && !irqs_disabled()) {
-			printk(KERN_WARNING "parse_args(): option '%s' enabled "
-					"irq's!\n", param);
-		}
+		if (irq_was_disabled && !irqs_disabled())
+			pr_warn("%s: option '%s' enabled irq's!\n",
+				doing, param);
+
 		switch (ret) {
 		case -ENOENT:
-			printk(KERN_ERR "%s: Unknown parameter `%s'\n",
-			       doing, param);
+			pr_err("%s: Unknown parameter `%s'\n", doing, param);
 			return ret;
 		case -ENOSPC:
-			printk(KERN_ERR
-			       "%s: `%s' too large for parameter `%s'\n",
+			pr_err("%s: `%s' too large for parameter `%s'\n",
 			       doing, val ?: "", param);
 			return ret;
 		case 0:
 			break;
 		default:
-			printk(KERN_ERR
-			       "%s: `%s' invalid for parameter `%s'\n",
+			pr_err("%s: `%s' invalid for parameter `%s'\n",
 			       doing, val ?: "", param);
 			return ret;
 		}
@@ -266,8 +263,7 @@ STANDARD_PARAM_DEF(ulong, unsigned long, "%lu", unsigned long, strict_strtoul);
 int param_set_charp(const char *val, const struct kernel_param *kp)
 {
 	if (strlen(val) > 1024) {
-		printk(KERN_ERR "%s: string parameter too long\n",
-		       kp->name);
+		pr_err("%s: string parameter too long\n", kp->name);
 		return -ENOSPC;
 	}
 
@@ -403,8 +399,7 @@ static int param_array(const char *name,
 		int len;
 
 		if (*num == max) {
-			printk(KERN_ERR "%s: can only take %i arguments\n",
-			       name, max);
+			pr_err("%s: can only take %i arguments\n", name, max);
 			return -EINVAL;
 		}
 		len = strcspn(val, ",");
@@ -423,8 +418,7 @@ static int param_array(const char *name,
 	} while (save == ',');
 
 	if (*num < min) {
-		printk(KERN_ERR "%s: needs at least %i arguments\n",
-		       name, min);
+		pr_err("%s: needs at least %i arguments\n", name, min);
 		return -EINVAL;
 	}
 	return 0;
@@ -483,7 +477,7 @@ int param_set_copystring(const char *val, const struct kernel_param *kp)
 	const struct kparam_string *kps = kp->str;
 
 	if (strlen(val)+1 > kps->maxlen) {
-		printk(KERN_ERR "%s: string doesn't fit in %u chars.\n",
+		pr_err("%s: string doesn't fit in %u chars.\n",
 		       kp->name, kps->maxlen-1);
 		return -ENOSPC;
 	}
@@ -753,11 +747,9 @@ static struct module_kobject * __init locate_module_kobject(const char *name)
 #endif
 		if (err) {
 			kobject_put(&mk->kobj);
-			printk(KERN_ERR
-				"Module '%s' failed add to sysfs, error number %d\n",
+			pr_crit("Adding module '%s' to sysfs failed (%d), "
+				"the system may be unstable.\n",
 				name, err);
-			printk(KERN_ERR
-				"The system will be unstable now.\n");
 			return NULL;
 		}
 
-- 
1.7.8.1

Re: [PATCH 2/3] params: replace printk(KERN_<LVL>...) with pr_<lvl>(...)

[Joe Perches]

On Thu, 2012-05-03 at 13:12 -0600, Jim Cromie wrote:
> I left 1 printk which uses __FILE__, __LINE__ explicitly, which should
> not be subject to generic preferences expressed via pr_fmt().
[]
> diff --git a/kernel/params.c b/kernel/params.c
[]
> @@ -753,11 +747,9 @@ static struct module_kobject * __init locate_module_kobject(const char *name)
>  #endif
>  		if (err) {
>  			kobject_put(&mk->kobj);
> -			printk(KERN_ERR
> -				"Module '%s' failed add to sysfs, error number %d\n",
> +			pr_crit("Adding module '%s' to sysfs failed (%d), "
> +				"the system may be unstable.\n",
>  				name, err);
> -			printk(KERN_ERR
> -				"The system will be unstable now.\n");
>  			return NULL;
>  		}
>  

It'd be nicer to coalesce the formats and not worry about 80 columns.
Coalesced formats are easier to find via grep.

			pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable\n",

[PATCH 2/3] params: replace printk(KERN_<LVL>...) with pr_<lvl>(...)

[Jim Cromie]

I left 1 printk which uses __FILE__, __LINE__ explicitly, which should
not be subject to generic preferences expressed via pr_fmt().

+ tweaks suggested by Joe Perches:
- add doing to irq-enabled warning, like others.  It wont happen often..
- change sysfs failure crit, not just err, make it 1 line in logs.
- coalese 2 format fragments into 1 >80 char line

cc: Joe Perches <joe@perches.com>
Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 kernel/params.c |   33 ++++++++++++---------------------
 1 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/kernel/params.c b/kernel/params.c
index be78c90..ed35345 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -201,25 +201,22 @@ int parse_args(const char *doing,
 		irq_was_disabled = irqs_disabled();
 		ret = parse_one(param, val, doing, params, num,
 				min_level, max_level, unknown);
-		if (irq_was_disabled && !irqs_disabled()) {
-			printk(KERN_WARNING "parse_args(): option '%s' enabled "
-					"irq's!\n", param);
-		}
+		if (irq_was_disabled && !irqs_disabled())
+			pr_warn("%s: option '%s' enabled irq's!\n",
+				doing, param);
+
 		switch (ret) {
 		case -ENOENT:
-			printk(KERN_ERR "%s: Unknown parameter `%s'\n",
-			       doing, param);
+			pr_err("%s: Unknown parameter `%s'\n", doing, param);
 			return ret;
 		case -ENOSPC:
-			printk(KERN_ERR
-			       "%s: `%s' too large for parameter `%s'\n",
+			pr_err("%s: `%s' too large for parameter `%s'\n",
 			       doing, val ?: "", param);
 			return ret;
 		case 0:
 			break;
 		default:
-			printk(KERN_ERR
-			       "%s: `%s' invalid for parameter `%s'\n",
+			pr_err("%s: `%s' invalid for parameter `%s'\n",
 			       doing, val ?: "", param);
 			return ret;
 		}
@@ -266,8 +263,7 @@ STANDARD_PARAM_DEF(ulong, unsigned long, "%lu", unsigned long, strict_strtoul);
 int param_set_charp(const char *val, const struct kernel_param *kp)
 {
 	if (strlen(val) > 1024) {
-		printk(KERN_ERR "%s: string parameter too long\n",
-		       kp->name);
+		pr_err("%s: string parameter too long\n", kp->name);
 		return -ENOSPC;
 	}
 
@@ -403,8 +399,7 @@ static int param_array(const char *name,
 		int len;
 
 		if (*num == max) {
-			printk(KERN_ERR "%s: can only take %i arguments\n",
-			       name, max);
+			pr_err("%s: can only take %i arguments\n", name, max);
 			return -EINVAL;
 		}
 		len = strcspn(val, ",");
@@ -423,8 +418,7 @@ static int param_array(const char *name,
 	} while (save == ',');
 
 	if (*num < min) {
-		printk(KERN_ERR "%s: needs at least %i arguments\n",
-		       name, min);
+		pr_err("%s: needs at least %i arguments\n", name, min);
 		return -EINVAL;
 	}
 	return 0;
@@ -483,7 +477,7 @@ int param_set_copystring(const char *val, const struct kernel_param *kp)
 	const struct kparam_string *kps = kp->str;
 
 	if (strlen(val)+1 > kps->maxlen) {
-		printk(KERN_ERR "%s: string doesn't fit in %u chars.\n",
+		pr_err("%s: string doesn't fit in %u chars.\n",
 		       kp->name, kps->maxlen-1);
 		return -ENOSPC;
 	}
@@ -753,11 +747,8 @@ static struct module_kobject * __init locate_module_kobject(const char *name)
 #endif
 		if (err) {
 			kobject_put(&mk->kobj);
-			printk(KERN_ERR
-				"Module '%s' failed add to sysfs, error number %d\n",
+			pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n",
 				name, err);
-			printk(KERN_ERR
-				"The system will be unstable now.\n");
 			return NULL;
 		}
 
-- 
1.7.8.1

Re: [PATCH 3/3] dynamic_debug: remove unneeded includes

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1732 bytes --]

Hi Jim,

On Thu,  3 May 2012 11:57:39 -0600 Jim Cromie <jim.cromie@gmail.com> wrote:
>
> These arent currently needed, so drop them.  Some will probably get
> re-added when static-branches are added, but include loops prevent
> that at present.

How sure are you that these are not needed?  Has this been done by
inspection, or by compile testing?  How widely have you compile tested?

I took a brief look ...

> 
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
>  lib/dynamic_debug.c |   10 ----------
>  1 files changed, 0 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 7ca29a0..fc5d270 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -14,24 +14,14 @@
>  
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> -#include <linux/moduleparam.h>

There is a use of module_param() in this file ...

> -#include <linux/kallsyms.h>
> -#include <linux/types.h>
>  #include <linux/mutex.h>
> -#include <linux/proc_fs.h>
>  #include <linux/seq_file.h>
> -#include <linux/list.h>

List_for_each_entry is used ...

> -#include <linux/sysctl.h>
>  #include <linux/ctype.h>
> -#include <linux/string.h>

strlen() is used ...

> -#include <linux/uaccess.h>

copy_from_user() is used ...

>  #include <linux/dynamic_debug.h>
>  #include <linux/debugfs.h>
>  #include <linux/slab.h>
> -#include <linux/jump_label.h>
>  #include <linux/hardirq.h>
>  #include <linux/sched.h>
> -#include <linux/device.h>

struct device is referenced ...

I am a bit more paranoid about these things than most.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/3] dynamic_debug: remove unneeded includes

[Jim Cromie]

Hi Stephen,

On Fri, May 4, 2012 at 10:36 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> Hi Jim,
>
> On Thu,  3 May 2012 11:57:39 -0600 Jim Cromie <jim.cromie@gmail.com> wrote:
>>
>> These arent currently needed, so drop them.  Some will probably get
>> re-added when static-branches are added, but include loops prevent
>> that at present.
>
> How sure are you that these are not needed?  Has this been done by
> inspection, or by compile testing?  How widely have you compile tested?
>

compiled on Core, and boot tested on i586
also recently compiled on x86_64

IIRC (its been a while) I just commented them all out,
and uncommented the ones needed to compile ok.

That doesnt mean theyre not being included indirectly,
so youre right to be paranoid.

> I took a brief look ...
>
>>
>> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
>> ---
>>  lib/dynamic_debug.c |   10 ----------
>>  1 files changed, 0 insertions(+), 10 deletions(-)
>>
>> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
>> index 7ca29a0..fc5d270 100644
>> --- a/lib/dynamic_debug.c
>> +++ b/lib/dynamic_debug.c
>> @@ -14,24 +14,14 @@
>>
>>  #include <linux/kernel.h>
>>  #include <linux/module.h>
>> -#include <linux/moduleparam.h>
>
> There is a use of module_param() in this file ...
>
>> -#include <linux/kallsyms.h>
>> -#include <linux/types.h>
>>  #include <linux/mutex.h>
>> -#include <linux/proc_fs.h>
>>  #include <linux/seq_file.h>
>> -#include <linux/list.h>
>
> List_for_each_entry is used ...
>
>> -#include <linux/sysctl.h>
>>  #include <linux/ctype.h>
>> -#include <linux/string.h>
>
> strlen() is used ...
>
>> -#include <linux/uaccess.h>
>
> copy_from_user() is used ...


>
>>  #include <linux/dynamic_debug.h>
>>  #include <linux/debugfs.h>
>>  #include <linux/slab.h>
>> -#include <linux/jump_label.h>
>>  #include <linux/hardirq.h>
>>  #include <linux/sched.h>
>> -#include <linux/device.h>
>
> struct device is referenced ...
>
> I am a bit more paranoid about these things than most.

I guess I better make lib/dynamic_debug.i
inspect the file, and possibly revert parts of this patch.

> --
> Cheers,
> Stephen Rothwell                    sfr@canb.auug.org.au
> http://www.canb.auug.org.au/~sfr/


thanks for taking that look.
Jim


BTW, how were you carrying that merge conflict resolution ?
git rerere ?

Re: [PATCH 3/3] dynamic_debug: remove unneeded includes

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 784 bytes --]

Hi Jim,

On Fri, 4 May 2012 23:52:24 -0600 Jim Cromie <jim.cromie@gmail.com> wrote:
>
> compiled on Core, and boot tested on i586
> also recently compiled on x86_64

Yeah, the problem being that we have 27 architectures. a large number of
platforms and en enormous number of CONFIG variables and all of these
things can affect whet gets implicitly included.

> I guess I better make lib/dynamic_debug.i
> inspect the file, and possibly revert parts of this patch.

I am not sure that would help much (see above).  Also note Rule 1 from
Documentation/SubmitChecklist :-)

> BTW, how were you carrying that merge conflict resolution ?
> git rerere ?

Yep.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]