(lkml Cc:-ed - this might be of interest to others too)
* Andy Whitcroft <[email protected]> wrote:
> WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> #411: FILE: home/apw/git/linux-2.6/kernel/sched.c:408:
> +EXPORT_SYMBOL_GPL(cpu_clock);
yes, this is a legit warning and i fix it every time i see it. (I cannot
fix this one now because mainline does not have an EXPORT_SYMBOL_GPL for
cpu_clock(), it's added in -mm? But i cannot find it in mm either. I'll
fix it once i find the patch :)
> WARNING: printk() should include KERN_ facility level
> #4838: FILE: home/apw/git/linux-2.6/kernel/sched.c:4835:
> + printk("%-13.13s %c", p->comm,
>
> WARNING: printk() should include KERN_ facility level
> #5622: FILE: home/apw/git/linux-2.6/kernel/sched.c:5619:
> + printk("\n");
>
> WARNING: printk() should include KERN_ facility level
> #5633: FILE: home/apw/git/linux-2.6/kernel/sched.c:5630:
> + printk("\n");
>
> WARNING: printk() should include KERN_ facility level
> #5640: FILE: home/apw/git/linux-2.6/kernel/sched.c:5637:
> + printk(" %s", str);
>
> These are actually only in debug code and so are unimportant, but
> technically they are wrong. This check is a very difficult one in the
> face of these constructs. But in this case I think it has got the
> report right.
this is actually a false positive - as the debug code constructs a
printk output _without_ \n. So the script should check whether there's
any \n in the printk string - if there is none, do not emit a warning.
(if you implement that then i think it can remain a warning and does not
need to move to CHECK.)
> At this point _if_ we took an error we would not have _DEBUG open and so
> a start would be required, otherwise not.
>
> printk(" %s", str);
>
> To be 100% correct I assume it would need to have a
> printk(KERN_DEBUG); after each of the KERN_ERR printk's.
i've fixed this in my tree. But i dont think checkpatch.pl notices this
level of bug - it just detects the missing KERN_ prefix in printk(),
right?
> WARNING: braces {} are not necessary for single statement blocks
> #5706: FILE: home/apw/git/linux-2.6/kernel/sched.c:5703:
> + if (parent->groups == parent->groups->next) {
> + pflags &= ~(SD_LOAD_BALANCE |
> + SD_BALANCE_NEWIDLE |
> + SD_BALANCE_FORK |
> + SD_BALANCE_EXEC |
> + SD_SHARE_CPUPOWER |
> + SD_SHARE_PKG_RESOURCES);
> + }
>
> Ok, this one is "correct" at least for the rules as I have them.
> Perhaps the message should not be emitted for very long blocks?
If a statement is not single-line, then braces _are_ fine. Where does
CodingStyle say that it's not fine?
> WARNING: no space between function name and open parenthesis '('
> #5768: FILE: home/apw/git/linux-2.6/kernel/sched.c:5765:
> +__setup ("isolcpus=", isolated_cpu_setup);
>
> Almost all other instances of __setup do not have a space, so I assume
> this is a reasonable report.
yes. I have just fixed it in my tree.
> WARNING: externs should be avoided in .c files
> #6545: FILE: home/apw/git/linux-2.6/kernel/sched.c:6542:
> + extern char __sched_text_start[], __sched_text_end[];
>
> That one ... dunno. This check is a difficult one. Should it be a
> CHECK?
no, this is a legitimate warning - externs in .c files should move into
the proper .h file. So i'd suggest to keep this a WARNING.
there are a few other valid warnings that checkpatch.pl emits: such as
the kfree one - i fixed that too in sched.c.
(Could you send me the v11-to-be version of checkpatch.pl so that i can
check the remaining issues?)
Ingo
On Mon, 1 Oct 2007 08:44:48 +0200 Ingo Molnar <[email protected]> wrote:
>
> (lkml Cc:-ed - this might be of interest to others too)
>
> * Andy Whitcroft <[email protected]> wrote:
>
> > WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> > #411: FILE: home/apw/git/linux-2.6/kernel/sched.c:408:
> > +EXPORT_SYMBOL_GPL(cpu_clock);
>
> yes, this is a legit warning and i fix it every time i see it. (I cannot
> fix this one now because mainline does not have an EXPORT_SYMBOL_GPL for
> cpu_clock(), it's added in -mm? But i cannot find it in mm either. I'll
> fix it once i find the patch :)
ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.23-rc8/2.6.23-rc8-mm2/broken-out/make-rcutorture-rng-use-temporal-entropy.patch
> > WARNING: printk() should include KERN_ facility level
> > #4838: FILE: home/apw/git/linux-2.6/kernel/sched.c:4835:
> > + printk("%-13.13s %c", p->comm,
> >
> > WARNING: printk() should include KERN_ facility level
> > #5622: FILE: home/apw/git/linux-2.6/kernel/sched.c:5619:
> > + printk("\n");
> >
> > WARNING: printk() should include KERN_ facility level
> > #5633: FILE: home/apw/git/linux-2.6/kernel/sched.c:5630:
> > + printk("\n");
> >
> > WARNING: printk() should include KERN_ facility level
> > #5640: FILE: home/apw/git/linux-2.6/kernel/sched.c:5637:
> > + printk(" %s", str);
> >
> > These are actually only in debug code and so are unimportant, but
> > technically they are wrong. This check is a very difficult one in the
> > face of these constructs. But in this case I think it has got the
> > report right.
>
> this is actually a false positive - as the debug code constructs a
> printk output _without_ \n. So the script should check whether there's
> any \n in the printk string - if there is none, do not emit a warning.
> (if you implement that then i think it can remain a warning and does not
> need to move to CHECK.)
Yeah, it does that sometimes. I don't think it's fixable within the scope
of checkpatch. It needs to check whether some preceding printk which might
not even be in the patch has a \n:
printk(KERN_ERR "foo");
<100 lines of whatever>
+ printk("bar\n");
we're screwed...
> > At this point _if_ we took an error we would not have _DEBUG open and so
> > a start would be required, otherwise not.
> >
> > printk(" %s", str);
> >
> > To be 100% correct I assume it would need to have a
> > printk(KERN_DEBUG); after each of the KERN_ERR printk's.
>
> i've fixed this in my tree. But i dont think checkpatch.pl notices this
> level of bug - it just detects the missing KERN_ prefix in printk(),
> right?
>
> > WARNING: braces {} are not necessary for single statement blocks
> > #5706: FILE: home/apw/git/linux-2.6/kernel/sched.c:5703:
> > + if (parent->groups == parent->groups->next) {
> > + pflags &= ~(SD_LOAD_BALANCE |
> > + SD_BALANCE_NEWIDLE |
> > + SD_BALANCE_FORK |
> > + SD_BALANCE_EXEC |
> > + SD_SHARE_CPUPOWER |
> > + SD_SHARE_PKG_RESOURCES);
> > + }
> >
> > Ok, this one is "correct" at least for the rules as I have them.
> > Perhaps the message should not be emitted for very long blocks?
>
> If a statement is not single-line, then braces _are_ fine. Where does
> CodingStyle say that it's not fine?
I'd disagree with checkpatch there. Again, it might be hard to fix. Wanna
rename it to checkpatch-and-suggest-stuff-to-think-about.pl?
> > WARNING: no space between function name and open parenthesis '('
> > #5768: FILE: home/apw/git/linux-2.6/kernel/sched.c:5765:
> > +__setup ("isolcpus=", isolated_cpu_setup);
> >
> > Almost all other instances of __setup do not have a space, so I assume
> > this is a reasonable report.
>
> yes. I have just fixed it in my tree.
>
> > WARNING: externs should be avoided in .c files
> > #6545: FILE: home/apw/git/linux-2.6/kernel/sched.c:6542:
> > + extern char __sched_text_start[], __sched_text_end[];
> >
> > That one ... dunno. This check is a difficult one. Should it be a
> > CHECK?
>
> no, this is a legitimate warning - externs in .c files should move into
> the proper .h file. So i'd suggest to keep this a WARNING.
Yes. When the symbol is defined in .S or vmlinux.lds we have
traditionally declared it in .c.
But why? It _is_ a global symbol. We might as well declare it in .h.
That's consistent, and prevents duplicated declarations from popping up
later on.
Andrew Morton wrote:
>> this is actually a false positive - as the debug code constructs a
>> printk output _without_ \n. So the script should check whether there's
>> any \n in the printk string - if there is none, do not emit a warning.
>> (if you implement that then i think it can remain a warning and does not
>> need to move to CHECK.)
>>
>
> Yeah, it does that sometimes. I don't think it's fixable within the scope
> of checkpatch. It needs to check whether some preceding printk which might
> not even be in the patch has a \n:
>
> printk(KERN_ERR "foo");
> <100 lines of whatever>
> + printk("bar\n");
>
> we're screwed...
>
>
Isn't that broken on SMP (or with preemption) anyway?
--
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
On Mon, 01 Oct 2007 09:48:25 +0200 Avi Kivity <[email protected]> wrote:
> Andrew Morton wrote:
> >> this is actually a false positive - as the debug code constructs a
> >> printk output _without_ \n. So the script should check whether there's
> >> any \n in the printk string - if there is none, do not emit a warning.
> >> (if you implement that then i think it can remain a warning and does not
> >> need to move to CHECK.)
> >>
> >
> > Yeah, it does that sometimes. I don't think it's fixable within the scope
> > of checkpatch. It needs to check whether some preceding printk which might
> > not even be in the patch has a \n:
> >
> > printk(KERN_ERR "foo");
> > <100 lines of whatever>
> > + printk("bar\n");
> >
> > we're screwed...
> >
> >
>
> Isn't that broken on SMP (or with preemption) anyway?
Yep. Or with interrupts...
> >
> > > WARNING: externs should be avoided in .c files
> > > #6545: FILE: home/apw/git/linux-2.6/kernel/sched.c:6542:
> > > + extern char __sched_text_start[], __sched_text_end[];
> > >
> > > That one ... dunno. This check is a difficult one. Should it be a
> > > CHECK?
> >
> > no, this is a legitimate warning - externs in .c files should move into
> > the proper .h file. So i'd suggest to keep this a WARNING.
>
> Yes. When the symbol is defined in .S or vmlinux.lds we have
> traditionally declared it in .c.
>
> But why? It _is_ a global symbol. We might as well declare it in .h.
> That's consistent, and prevents duplicated declarations from popping up
> later on.
It would be nice to collect all linker symbol externs in one place..
asm-$(ARCH)/???.h
We could even auto-generate it but thats not worth it I think.
Sam
* Andrew Morton <[email protected]> wrote:
> > > WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> > > #411: FILE: home/apw/git/linux-2.6/kernel/sched.c:408:
> > > +EXPORT_SYMBOL_GPL(cpu_clock);
> >
> > yes, this is a legit warning and i fix it every time i see it. (I
> > cannot fix this one now because mainline does not have an
> > EXPORT_SYMBOL_GPL for cpu_clock(), it's added in -mm? But i cannot
> > find it in mm either. I'll fix it once i find the patch :)
>
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.23-rc8/2.6.23-rc8-mm2/broken-out/make-rcutorture-rng-use-temporal-entropy.patch
ok - i have picked up the sched.c bit for sched-devel.git. (see below)
Ingo
---------------->
Subject: sched: export cpu_clock()
From: "Paul E. McKenney" <[email protected]>
export cpu_clock() - the preferred API instead of sched_clock().
Signed-off-by: Paul E. McKenney <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
kernel/sched.c | 1 +
1 file changed, 1 insertion(+)
Index: linux/kernel/sched.c
===================================================================
--- linux.orig/kernel/sched.c
+++ linux/kernel/sched.c
@@ -469,6 +469,7 @@ unsigned long long cpu_clock(int cpu)
return now;
}
+EXPORT_SYMBOL_GPL(cpu_clock);
#ifndef prepare_arch_switch
# define prepare_arch_switch(next) do { } while (0)
* Andrew Morton <[email protected]> wrote:
> On Mon, 01 Oct 2007 09:48:25 +0200 Avi Kivity <[email protected]> wrote:
>
> > Andrew Morton wrote:
> > >> this is actually a false positive - as the debug code constructs a
> > >> printk output _without_ \n. So the script should check whether there's
> > >> any \n in the printk string - if there is none, do not emit a warning.
> > >> (if you implement that then i think it can remain a warning and does not
> > >> need to move to CHECK.)
> > >>
> > >
> > > Yeah, it does that sometimes. I don't think it's fixable within the scope
> > > of checkpatch. It needs to check whether some preceding printk which might
> > > not even be in the patch has a \n:
> > >
> > > printk(KERN_ERR "foo");
> > > <100 lines of whatever>
> > > + printk("bar\n");
> > >
> > > we're screwed...
> > >
> > >
> >
> > Isn't that broken on SMP (or with preemption) anyway?
>
> Yep. Or with interrupts...
not if it's a boot-time only debug check before SMP bringup. (as it is
in sched.c) We could make this intention explicit via a simple
raw_printk() wrapper to printk, which could be used without KERN_.
Ingo
* Andrew Morton <[email protected]> wrote:
> > > WARNING: braces {} are not necessary for single statement blocks
> > > #5706: FILE: home/apw/git/linux-2.6/kernel/sched.c:5703:
> > > + if (parent->groups == parent->groups->next) {
> > > + pflags &= ~(SD_LOAD_BALANCE |
> > > + SD_BALANCE_NEWIDLE |
> > > + SD_BALANCE_FORK |
> > > + SD_BALANCE_EXEC |
> > > + SD_SHARE_CPUPOWER |
> > > + SD_SHARE_PKG_RESOURCES);
> > > + }
> > >
> > > Ok, this one is "correct" at least for the rules as I have them.
> > > Perhaps the message should not be emitted for very long blocks?
> >
> > If a statement is not single-line, then braces _are_ fine. Where does
> > CodingStyle say that it's not fine?
>
> I'd disagree with checkpatch there. Again, it might be hard to fix.
> Wanna rename it to checkpatch-and-suggest-stuff-to-think-about.pl?
i think checkpatch.pl is quite close to its most useful purpose and
role: that of a reliable tool that i can use in an automated fashion
too, with only very rare false positive - and not the current 50%-80%
false positives rate. (And i'm fighting hard for Andy to realize the
true scope and purpose of this script ;-)
Ingo
On Mon, Oct 01, 2007 at 12:30:07AM -0700, Andrew Morton wrote:
> On Mon, 1 Oct 2007 08:44:48 +0200 Ingo Molnar <[email protected]> wrote:
>
> >
> > (lkml Cc:-ed - this might be of interest to others too)
> >
> > * Andy Whitcroft <[email protected]> wrote:
> >
> > > WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> > > #411: FILE: home/apw/git/linux-2.6/kernel/sched.c:408:
> > > +EXPORT_SYMBOL_GPL(cpu_clock);
> >
> > yes, this is a legit warning and i fix it every time i see it. (I cannot
> > fix this one now because mainline does not have an EXPORT_SYMBOL_GPL for
> > cpu_clock(), it's added in -mm? But i cannot find it in mm either. I'll
> > fix it once i find the patch :)
>
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.23-rc8/2.6.23-rc8-mm2/broken-out/make-rcutorture-rng-use-temporal-entropy.patch
>
> > > WARNING: printk() should include KERN_ facility level
> > > #4838: FILE: home/apw/git/linux-2.6/kernel/sched.c:4835:
> > > + printk("%-13.13s %c", p->comm,
> > >
> > > WARNING: printk() should include KERN_ facility level
> > > #5622: FILE: home/apw/git/linux-2.6/kernel/sched.c:5619:
> > > + printk("\n");
> > >
> > > WARNING: printk() should include KERN_ facility level
> > > #5633: FILE: home/apw/git/linux-2.6/kernel/sched.c:5630:
> > > + printk("\n");
> > >
> > > WARNING: printk() should include KERN_ facility level
> > > #5640: FILE: home/apw/git/linux-2.6/kernel/sched.c:5637:
> > > + printk(" %s", str);
> > >
> > > These are actually only in debug code and so are unimportant, but
> > > technically they are wrong. This check is a very difficult one in the
> > > face of these constructs. But in this case I think it has got the
> > > report right.
> >
> > this is actually a false positive - as the debug code constructs a
> > printk output _without_ \n. So the script should check whether there's
> > any \n in the printk string - if there is none, do not emit a warning.
> > (if you implement that then i think it can remain a warning and does not
> > need to move to CHECK.)
>
> Yeah, it does that sometimes. I don't think it's fixable within the scope
> of checkpatch. It needs to check whether some preceding printk which might
> not even be in the patch has a \n:
>
> printk(KERN_ERR "foo");
> <100 lines of whatever>
> + printk("bar\n");
>
> we're screwed...
Actually checkpatch does take that into account. If the printk has no
precceding printk ending in a newline, within the scope of the diff
then the warning is suppressed.
However, in this mm/sched.c case there are paths through the code which
do violate the "every line starts with KERN_" mantra. Now checkpatch is
actually not clever enough to spot that these are conditional, but in
this case there is an error. Its not important as it is debug. But I
contend there is an erorr. When an error is thrown we close the
unfinished line, emit a line at KERN_ERR and then continue to emit the
previous line without switching to KERN_DEBUG again.
> > > At this point _if_ we took an error we would not have _DEBUG open and so
> > > a start would be required, otherwise not.
> > >
> > > printk(" %s", str);
> > >
> > > To be 100% correct I assume it would need to have a
> > > printk(KERN_DEBUG); after each of the KERN_ERR printk's.
> >
> > i've fixed this in my tree. But i dont think checkpatch.pl notices this
> > level of bug - it just detects the missing KERN_ prefix in printk(),
> > right?
> >
> > > WARNING: braces {} are not necessary for single statement blocks
> > > #5706: FILE: home/apw/git/linux-2.6/kernel/sched.c:5703:
> > > + if (parent->groups == parent->groups->next) {
> > > + pflags &= ~(SD_LOAD_BALANCE |
> > > + SD_BALANCE_NEWIDLE |
> > > + SD_BALANCE_FORK |
> > > + SD_BALANCE_EXEC |
> > > + SD_SHARE_CPUPOWER |
> > > + SD_SHARE_PKG_RESOURCES);
> > > + }
> > >
> > > Ok, this one is "correct" at least for the rules as I have them.
> > > Perhaps the message should not be emitted for very long blocks?
> >
> > If a statement is not single-line, then braces _are_ fine. Where does
> > CodingStyle say that it's not fine?
>
> I'd disagree with checkpatch there. Again, it might be hard to fix. Wanna
> rename it to checkpatch-and-suggest-stuff-to-think-about.pl?
Checkpatch is capable of making the determination as at the time we
emit the warning we have all of the block in question. We already allow
a number of other exceptions. Just I think what I think the "rule" is
and its actuallity are not the same.
Are we saying the rule is "braces are not required for single _line_
blocks?
> > > WARNING: no space between function name and open parenthesis '('
> > > #5768: FILE: home/apw/git/linux-2.6/kernel/sched.c:5765:
> > > +__setup ("isolcpus=", isolated_cpu_setup);
> > >
> > > Almost all other instances of __setup do not have a space, so I assume
> > > this is a reasonable report.
> >
> > yes. I have just fixed it in my tree.
> >
> > > WARNING: externs should be avoided in .c files
> > > #6545: FILE: home/apw/git/linux-2.6/kernel/sched.c:6542:
> > > + extern char __sched_text_start[], __sched_text_end[];
> > >
> > > That one ... dunno. This check is a difficult one. Should it be a
> > > CHECK?
> >
> > no, this is a legitimate warning - externs in .c files should move into
> > the proper .h file. So i'd suggest to keep this a WARNING.
>
> Yes. When the symbol is defined in .S or vmlinux.lds we have
> traditionally declared it in .c.
>
> But why? It _is_ a global symbol. We might as well declare it in .h.
> That's consistent, and prevents duplicated declarations from popping up
> later on.
-apw
On Mon, Oct 01, 2007 at 12:30:07AM -0700, Andrew Morton wrote:
> On Mon, 1 Oct 2007 08:44:48 +0200 Ingo Molnar <[email protected]> wrote:
>
> >
> > (lkml Cc:-ed - this might be of interest to others too)
> >
> > * Andy Whitcroft <[email protected]> wrote:
> >
> > > WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> > > #411: FILE: home/apw/git/linux-2.6/kernel/sched.c:408:
> > > +EXPORT_SYMBOL_GPL(cpu_clock);
> >
> > yes, this is a legit warning and i fix it every time i see it. (I cannot
> > fix this one now because mainline does not have an EXPORT_SYMBOL_GPL for
> > cpu_clock(), it's added in -mm? But i cannot find it in mm either. I'll
> > fix it once i find the patch :)
>
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.23-rc8/2.6.23-rc8-mm2/broken-out/make-rcutorture-rng-use-temporal-entropy.patch
>
> > > WARNING: printk() should include KERN_ facility level
> > > #4838: FILE: home/apw/git/linux-2.6/kernel/sched.c:4835:
> > > + printk("%-13.13s %c", p->comm,
> > >
> > > WARNING: printk() should include KERN_ facility level
> > > #5622: FILE: home/apw/git/linux-2.6/kernel/sched.c:5619:
> > > + printk("\n");
> > >
> > > WARNING: printk() should include KERN_ facility level
> > > #5633: FILE: home/apw/git/linux-2.6/kernel/sched.c:5630:
> > > + printk("\n");
> > >
> > > WARNING: printk() should include KERN_ facility level
> > > #5640: FILE: home/apw/git/linux-2.6/kernel/sched.c:5637:
> > > + printk(" %s", str);
> > >
> > > These are actually only in debug code and so are unimportant, but
> > > technically they are wrong. This check is a very difficult one in the
> > > face of these constructs. But in this case I think it has got the
> > > report right.
> >
> > this is actually a false positive - as the debug code constructs a
> > printk output _without_ \n. So the script should check whether there's
> > any \n in the printk string - if there is none, do not emit a warning.
> > (if you implement that then i think it can remain a warning and does not
> > need to move to CHECK.)
>
> Yeah, it does that sometimes. I don't think it's fixable within the scope
> of checkpatch. It needs to check whether some preceding printk which might
> not even be in the patch has a \n:
>
> printk(KERN_ERR "foo");
> <100 lines of whatever>
> + printk("bar\n");
>
> we're screwed...
Well, I think that we could do something like this :
#define KERN_CONT ""
...
printk(KERN_ERR "foo");
<100 lines of whatever>
printk(KERN_CONT "bar\n");
It would indicate the author's *intent* which is to continue a line which
has already been started. It would both permit us to remove false positives
from automated scripts, and to read the code more easily. And this is not a
big constaint for the author, given that such constructs are quite rare.
Regards,
Willy
* Willy Tarreau <[email protected]> wrote:
> Well, I think that we could do something like this :
>
> #define KERN_CONT ""
> ...
> printk(KERN_ERR "foo");
> <100 lines of whatever>
> printk(KERN_CONT "bar\n");
>
> It would indicate the author's *intent* which is to continue a line
> which has already been started. It would both permit us to remove
> false positives from automated scripts, and to read the code more
> easily. And this is not a big constaint for the author, given that
> such constructs are quite rare.
ah, this is even nicer than the raw_printk() thing i suggested, and it
also nicely documents the intention of the author. Patch attached below.
And i'd like to stress the principle that is followed here: in this
particular case the checkpatch.pl warning is very useful, but still
there are false positives. Fortunately they are so rare that it's worth
annotating those few exceptions in the source. Note that the goal is
still to be able to achieve 100% warning-free source code. _That_ should
be the driving principle behind checkpatch.pl warnings.
Ingo
------------------>
Subject: printk: add KERN_CONT annotation
From: Ingo Molnar <[email protected]>
printk: add the KERN_CONT annotation (which is empty string but via
which checkpatch.pl can notice that the lacking KERN_ level is fine).
This useful for multiple calls of hand-crafted printk output done by
early debug code or similar.
Signed-off-by: Ingo Molnar <[email protected]>
---
include/linux/kernel.h | 7 +++++++
kernel/sched.c | 22 +++++++++++-----------
2 files changed, 18 insertions(+), 11 deletions(-)
Index: linux/include/linux/kernel.h
===================================================================
--- linux.orig/include/linux/kernel.h
+++ linux/include/linux/kernel.h
@@ -61,6 +61,13 @@ extern const char linux_proc_banner[];
#define KERN_INFO "<6>" /* informational */
#define KERN_DEBUG "<7>" /* debug-level messages */
+/*
+ * Annotation for a "continued" line of log printout (only done after a
+ * line that had no enclosing \n). Only to be used by core/arch code
+ * during early bootup (a continued line is not SMP-safe otherwise).
+ */
+#define KERN_CONT ""
+
extern int console_printk[];
#define console_loglevel (console_printk[0])
Index: linux/kernel/sched.c
===================================================================
--- linux.orig/kernel/sched.c
+++ linux/kernel/sched.c
@@ -4770,18 +4770,18 @@ static void show_task(struct task_struct
unsigned state;
state = p->state ? __ffs(p->state) + 1 : 0;
- printk("%-13.13s %c", p->comm,
+ printk(KERN_INFO "%-13.13s %c", p->comm,
state < sizeof(stat_nam) - 1 ? stat_nam[state] : '?');
#if BITS_PER_LONG == 32
if (state == TASK_RUNNING)
- printk(" running ");
+ printk(KERN_CONT " running ");
else
- printk(" %08lx ", thread_saved_pc(p));
+ printk(KERN_CONT " %08lx ", thread_saved_pc(p));
#else
if (state == TASK_RUNNING)
- printk(" running task ");
+ printk(KERN_CONT " running task ");
else
- printk(" %016lx ", thread_saved_pc(p));
+ printk(KERN_CONT " %016lx ", thread_saved_pc(p));
#endif
#ifdef CONFIG_DEBUG_STACK_USAGE
{
@@ -4791,7 +4791,7 @@ static void show_task(struct task_struct
free = (unsigned long)n - (unsigned long)end_of_stack(p);
}
#endif
- printk("%5lu %5d %6d\n", free, p->pid, p->parent->pid);
+ printk(KERN_CONT "%5lu %5d %6d\n", free, p->pid, p->parent->pid);
if (state != TASK_RUNNING)
show_stack(p, NULL);
@@ -5527,20 +5527,20 @@ static void sched_domain_debug(struct sc
}
if (!group->__cpu_power) {
- printk("\n");
+ printk(KERN_CONT "\n");
printk(KERN_ERR "ERROR: domain->cpu_power not "
"set\n");
break;
}
if (!cpus_weight(group->cpumask)) {
- printk("\n");
+ printk(KERN_CONT "\n");
printk(KERN_ERR "ERROR: empty group\n");
break;
}
if (cpus_intersects(groupmask, group->cpumask)) {
- printk("\n");
+ printk(KERN_CONT "\n");
printk(KERN_ERR "ERROR: repeated CPUs\n");
break;
}
@@ -5548,11 +5548,11 @@ static void sched_domain_debug(struct sc
cpus_or(groupmask, groupmask, group->cpumask);
cpumask_scnprintf(str, NR_CPUS, group->cpumask);
- printk(" %s", str);
+ printk(KERN_CONT " %s", str);
group = group->next;
} while (group != sd->groups);
- printk("\n");
+ printk(KERN_CONT "\n");
if (!cpus_equal(sd->span, groupmask))
printk(KERN_ERR "ERROR: groups don't span "
On Tue, 2 October 2007 07:18:52 +0200, Ingo Molnar wrote:
>
> ah, this is even nicer than the raw_printk() thing i suggested, and it
> also nicely documents the intention of the author. Patch attached below.
KERN_CONT was brought up in the linux-tiny discussion. Not sure if you
want to get involved in that, but there may be value in adding one
variant of KERN_CONT per debug level:
http://lkml.org/lkml/2007/9/30/151
> And i'd like to stress the principle that is followed here: in this
> particular case the checkpatch.pl warning is very useful, but still
> there are false positives. Fortunately they are so rare that it's worth
> annotating those few exceptions in the source. Note that the goal is
> still to be able to achieve 100% warning-free source code. _That_ should
> be the driving principle behind checkpatch.pl warnings.
Thank you for working on this. I had nearly given up on checkpatch
before.
Jörn
--
When people work hard for you for a pat on the back, you've got
to give them that pat.
-- Robert Heinlein
On Tue, 2007-10-02 at 07:18 +0200, Ingo Molnar wrote:
> +#define KERN_CONT ""
This doesn't work with printk(char** array[index]) continuations
or with strings with embedded KERN_ prefixes.
printk(array[index])
printk(version)
On Oct 2 2007 08:41, Joe Perches wrote:
>On Tue, 2007-10-02 at 07:18 +0200, Ingo Molnar wrote:
>> +#define KERN_CONT ""
>
>This doesn't work with printk(char** array[index]) continuations
>or with strings with embedded KERN_ prefixes.
Huh?
...Ah. Yeah, pasting a string literal with a variable won't work.
But then again, you should not use printk(var) directly, but always
use printk("%s", var)
On Tue, 2007-10-02 at 17:45 +0200, Jan Engelhardt wrote:
> always use printk("%s", var)
You have to use indirect arguments to log something?
Don't you think that's a stupid rule?
Look at the version strings that are used as __devinitdata.
On Oct 2 2007 09:03, Joe Perches wrote:
>On Tue, 2007-10-02 at 17:45 +0200, Jan Engelhardt wrote:
>> always use printk("%s", var)
>
>You have to use indirect arguments to log something?
No, you do not have to.
>Don't you think that's a stupid rule?
Not at all. var may contain format specifiers, which poses a
certain security issue into people's hands. This is already
important in userspace, so is probably even more in the kernel,
even though user-supplied strings are less common.
On Tue, 2 Oct 2007 07:18:52 +0200
Ingo Molnar <[email protected]> wrote:
>
> printk: add the KERN_CONT annotation (which is empty string but via
> which checkpatch.pl can notice that the lacking KERN_ level is fine).
> This useful for multiple calls of hand-crafted printk output done by
> early debug code or similar.
>
I like it. Sort of self-documenting notation.
>
> Index: linux/include/linux/kernel.h
> ===================================================================
> --- linux.orig/include/linux/kernel.h
> +++ linux/include/linux/kernel.h
> @@ -61,6 +61,13 @@ extern const char linux_proc_banner[];
> #define KERN_INFO "<6>" /* informational */
> #define KERN_DEBUG "<7>" /* debug-level messages */
>
> +/*
> + * Annotation for a "continued" line of log printout (only done after a
> + * line that had no enclosing \n). Only to be used by core/arch code
> + * during early bootup (a continued line is not SMP-safe otherwise).
> + */
> +#define KERN_CONT ""
> +
> extern int console_printk[];
>
> #define console_loglevel (console_printk[0])
> Index: linux/kernel/sched.c
I get rejects from the sched.c hunk and that's your stuff anwyay, so I
dropped that bit.
* Andrew Morton <[email protected]> wrote:
> On Tue, 2 Oct 2007 07:18:52 +0200
> Ingo Molnar <[email protected]> wrote:
>
> >
> > printk: add the KERN_CONT annotation (which is empty string but via
> > which checkpatch.pl can notice that the lacking KERN_ level is fine).
> > This useful for multiple calls of hand-crafted printk output done by
> > early debug code or similar.
> >
>
> I like it. Sort of self-documenting notation.
cool. Please push into v2.6.24.
> > #define console_loglevel (console_printk[0])
> > Index: linux/kernel/sched.c
>
> I get rejects from the sched.c hunk and that's your stuff anwyay, so I
> dropped that bit.
yeah, i'll fix sched.c up once it goes upstream.
Ingo