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 <[email protected]>
Signed-off-by: Jim Cromie <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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.
Hi Jim,
On Thu, 3 May 2012 11:57:39 -0600 Jim Cromie <[email protected]> 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 <[email protected]>
> ---
> 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 [email protected]
http://www.canb.auug.org.au/~sfr/
Hi Stephen,
On Fri, May 4, 2012 at 10:36 PM, Stephen Rothwell <[email protected]> wrote:
> Hi Jim,
>
> On Thu, ?3 May 2012 11:57:39 -0600 Jim Cromie <[email protected]> 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 <[email protected]>
>> ---
>> ?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 ? ? ? ? ? ? ? ? ? [email protected]
> http://www.canb.auug.org.au/~sfr/
thanks for taking that look.
Jim
BTW, how were you carrying that merge conflict resolution ?
git rerere ?
Hi Jim,
On Fri, 4 May 2012 23:52:24 -0600 Jim Cromie <[email protected]> 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 [email protected]
http://www.canb.auug.org.au/~sfr/