* [PATCH 1/3] params.c: fix Smack complaint about parse_args @ 2012-05-03 17:57 Jim Cromie 2012-05-03 17:57 ` [PATCH 2/3] params: replace printk(KERN_<LVL>...) with pr_<lvl>(...) Jim Cromie 2012-05-03 17:57 ` [PATCH 3/3] dynamic_debug: remove unneeded includes Jim Cromie 0 siblings, 2 replies; 7+ messages in thread From: Jim Cromie @ 2012-05-03 17:57 UTC (permalink / raw) To: gregkh, linux-kernel; +Cc: jbaron, Jim Cromie, Dan Carpenter 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/3] params: replace printk(KERN_<LVL>...) with pr_<lvl>(...) 2012-05-03 17:57 [PATCH 1/3] params.c: fix Smack complaint about parse_args Jim Cromie @ 2012-05-03 17:57 ` Jim Cromie 2012-05-03 18:50 ` Joe Perches 2012-05-03 17:57 ` [PATCH 3/3] dynamic_debug: remove unneeded includes Jim Cromie 1 sibling, 1 reply; 7+ messages in thread From: Jim Cromie @ 2012-05-03 17:57 UTC (permalink / raw) To: gregkh, linux-kernel; +Cc: jbaron, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] params: replace printk(KERN_<LVL>...) with pr_<lvl>(...) 2012-05-03 17:57 ` [PATCH 2/3] params: replace printk(KERN_<LVL>...) with pr_<lvl>(...) Jim Cromie @ 2012-05-03 18:50 ` Joe Perches 0 siblings, 0 replies; 7+ messages in thread From: Joe Perches @ 2012-05-03 18:50 UTC (permalink / raw) To: Jim Cromie; +Cc: gregkh, linux-kernel, jbaron 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 3/3] dynamic_debug: remove unneeded includes 2012-05-03 17:57 [PATCH 1/3] params.c: fix Smack complaint about parse_args Jim Cromie 2012-05-03 17:57 ` [PATCH 2/3] params: replace printk(KERN_<LVL>...) with pr_<lvl>(...) Jim Cromie @ 2012-05-03 17:57 ` Jim Cromie 2012-05-05 4:36 ` Stephen Rothwell 1 sibling, 1 reply; 7+ messages in thread From: Jim Cromie @ 2012-05-03 17:57 UTC (permalink / raw) To: gregkh, linux-kernel; +Cc: jbaron, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3] dynamic_debug: remove unneeded includes 2012-05-03 17:57 ` [PATCH 3/3] dynamic_debug: remove unneeded includes Jim Cromie @ 2012-05-05 4:36 ` Stephen Rothwell 2012-05-05 5:52 ` Jim Cromie 0 siblings, 1 reply; 7+ messages in thread From: Stephen Rothwell @ 2012-05-05 4:36 UTC (permalink / raw) To: Jim Cromie; +Cc: gregkh, linux-kernel, jbaron [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3] dynamic_debug: remove unneeded includes 2012-05-05 4:36 ` Stephen Rothwell @ 2012-05-05 5:52 ` Jim Cromie 2012-05-05 6:08 ` Stephen Rothwell 0 siblings, 1 reply; 7+ messages in thread From: Jim Cromie @ 2012-05-05 5:52 UTC (permalink / raw) To: Stephen Rothwell; +Cc: gregkh, linux-kernel, jbaron 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 ? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3] dynamic_debug: remove unneeded includes 2012-05-05 5:52 ` Jim Cromie @ 2012-05-05 6:08 ` Stephen Rothwell 0 siblings, 0 replies; 7+ messages in thread From: Stephen Rothwell @ 2012-05-05 6:08 UTC (permalink / raw) To: Jim Cromie; +Cc: gregkh, linux-kernel, jbaron [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-05-05 6:08 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-05-03 17:57 [PATCH 1/3] params.c: fix Smack complaint about parse_args Jim Cromie 2012-05-03 17:57 ` [PATCH 2/3] params: replace printk(KERN_<LVL>...) with pr_<lvl>(...) Jim Cromie 2012-05-03 18:50 ` Joe Perches 2012-05-03 17:57 ` [PATCH 3/3] dynamic_debug: remove unneeded includes Jim Cromie 2012-05-05 4:36 ` Stephen Rothwell 2012-05-05 5:52 ` Jim Cromie 2012-05-05 6:08 ` Stephen Rothwell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox