checkpatch and kernel/sched.c
A message that is not in the archive (sent off-list or not yet received): [email protected]
(lkml Cc:-ed - this might be of interest to others too)
Quoted 3 lines · Andy Whitcroft
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 :)
Quoted 16 lines
WARNING:
printk()should includeKERN_facility level #4838: FILE:home/apw/git/linux-2.6/kernel/sched.c:4835:(quoted diff) +1
-0+ printk("%-13.13s %c", p->comm,WARNING:
printk()should includeKERN_facility level #5622: FILE:home/apw/git/linux-2.6/kernel/sched.c:5619:(quoted diff) +1
-0+ printk("\n");WARNING:
printk()should includeKERN_facility level #5633: FILE:home/apw/git/linux-2.6/kernel/sched.c:5630:(quoted diff) +1
-0+ printk("\n");WARNING:
printk()should includeKERN_facility level #5640: FILE:home/apw/git/linux-2.6/kernel/sched.c:5637:(quoted diff) +1
-0+ printk(" %s", str);
Quoted 4 lines
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.)
Quoted 7 lines
At this point _if_ we took an error we would not have
_DEBUGopen 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 theKERN_ERRprintk'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?
Quoted 11 lines
WARNING: braces {} are not necessary for single statement blocks #5706: FILE:
home/apw/git/linux-2.6/kernel/sched.c:5703:(quoted diff) +8
-0+ 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); + }
Quoted 2 lines
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?
Quoted 6 lines
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
__setupdo not have a space, so I assume this is a reasonable report.
yes. I have just fixed it in my tree.
Quoted 6 lines
WARNING: externs should be avoided in .c files #6545: FILE:
home/apw/git/linux-2.6/kernel/sched.c:6542:(quoted diff) +1
-0+ 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
Quoted 5 lines · 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:
Quoted 8 lines
Quoted 3 lines
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_GPLforcpu_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
Quoted 16 lines
Quoted 16 lines
WARNING:
printk()should includeKERN_facility level #4838: FILE:home/apw/git/linux-2.6/kernel/sched.c:4835:(quoted diff) +1
-0+ printk("%-13.13s %c", p->comm,WARNING:
printk()should includeKERN_facility level #5622: FILE:home/apw/git/linux-2.6/kernel/sched.c:5619:(quoted diff) +1
-0+ printk("\n");WARNING:
printk()should includeKERN_facility level #5633: FILE:home/apw/git/linux-2.6/kernel/sched.c:5630:(quoted diff) +1
-0+ printk("\n");WARNING:
printk()should includeKERN_facility level #5640: FILE:home/apw/git/linux-2.6/kernel/sched.c:5637:(quoted diff) +1
-0+ printk(" %s", str);
Quoted 10 lines
Quoted 4 lines
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...
Quoted 23 lines
Quoted 7 lines
At this point _if_ we took an error we would not have
_DEBUGopen 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 theKERN_ERRprintk'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 inprintk(), right?Quoted 11 lines
WARNING: braces {} are not necessary for single statement blocks #5706: FILE:
home/apw/git/linux-2.6/kernel/sched.c:5703:(quoted diff) +8
-0+ 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); + }
Quoted 5 lines
Quoted 2 lines
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?
Quoted 13 lines
Quoted 6 lines
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
__setupdo not have a space, so I assume this is a reasonable report.yes. I have just fixed it in my tree.
Quoted 4 lines
WARNING: externs should be avoided in .c files #6545: FILE:
home/apw/git/linux-2.6/kernel/sched.c:6542:(quoted diff) +1
-0+ extern char __sched_text_start[], __sched_text_end[];
Quoted 5 lines
Quoted 2 lines
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.
Quoted 15 lines · Andrew Morton
Quoted 6 lines
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>(quoted diff) +1
-0+ printk("bar\n");
Quoted 3 lines
we're screwed...
Isn't that broken on SMP (or with preemption) anyway?
Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
Quoted 16 lines · On Mon, 01 Oct 2007 09:48:25 +0200 Avi Kivity <[email protected]> wrote:
Quoted 15 lines · Andrew Morton
Quoted 6 lines
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>(quoted diff) +1
-0+ printk("bar\n");
Quoted 5 lines
Quoted 3 lines
we're screwed...
Isn't that broken on SMP (or with preemption) anyway?
Yep. Or with interrupts...
Quoted 18 lines · Andrew Morton
Quoted 16 lines · On Mon, 01 Oct 2007 09:48:25 +0200 Avi Kivity <[email protected]> wrote:
Quoted 15 lines · Andrew Morton
Quoted 6 lines
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>(quoted diff) +1
-0+ printk("bar\n");
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
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
Quoted 11 lines
Quoted 10 lines
Quoted 7 lines
WARNING: externs should be avoided in .c files #6545: FILE:
home/apw/git/linux-2.6/kernel/sched.c:6542:(quoted diff) +1
-0+ 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.
Quoted 6 lines
Yes. When the symbol is defined in .S or
vmlinux.ldswe 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
Quoted 10 lines · Andrew Morton
Quoted 8 lines
Quoted 3 lines
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_GPLforcpu_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().
--- kernel/sched.c | 1 + 1 file changed, 1 insertion(+)
kernel/sched.c +1 -0
@@ -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)
Quoted 11 lines · Andrew Morton
Quoted 11 lines
Quoted 11 lines
WARNING: braces {} are not necessary for single statement blocks #5706: FILE:
home/apw/git/linux-2.6/kernel/sched.c:5703:(quoted diff) +8
-0+ 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); + }
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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
Quoted 53 lines · Andrew Morton
Quoted 5 lines · 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:
Quoted 8 lines
Quoted 3 lines
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_GPLforcpu_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
Quoted 16 lines
Quoted 16 lines
WARNING:
printk()should includeKERN_facility level #4838: FILE:home/apw/git/linux-2.6/kernel/sched.c:4835:(quoted diff) +1
-0+ printk("%-13.13s %c", p->comm,WARNING:
printk()should includeKERN_facility level #5622: FILE:home/apw/git/linux-2.6/kernel/sched.c:5619:(quoted diff) +1
-0+ printk("\n");WARNING:
printk()should includeKERN_facility level #5633: FILE:home/apw/git/linux-2.6/kernel/sched.c:5630:(quoted diff) +1
-0+ printk("\n");WARNING:
printk()should includeKERN_facility level #5640: FILE:home/apw/git/linux-2.6/kernel/sched.c:5637:(quoted diff) +1
-0+ printk(" %s", str);Quoted 10 lines
Quoted 4 lines
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>(quoted diff) +1
-0+ printk("bar\n");
Quoted 1 line
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.
Quoted 23 lines
Quoted 23 lines
Quoted 7 lines
At this point _if_ we took an error we would not have
_DEBUGopen 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 theKERN_ERRprintk'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 inprintk(), right?Quoted 11 lines
WARNING: braces {} are not necessary for single statement blocks #5706: FILE:
home/apw/git/linux-2.6/kernel/sched.c:5703:(quoted diff) +8
-0+ 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); + }
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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?
Quoted 19 lines
Quoted 13 lines
Quoted 6 lines
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
__setupdo not have a space, so I assume this is a reasonable report.yes. I have just fixed it in my tree.
Quoted 4 lines
WARNING: externs should be avoided in .c files #6545: FILE:
home/apw/git/linux-2.6/kernel/sched.c:6542:(quoted diff) +1
-0+ extern char __sched_text_start[], __sched_text_end[];Quoted 5 lines
Quoted 2 lines
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.
Quoted 6 lines
Yes. When the symbol is defined in .S or
vmlinux.ldswe 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
Quoted 53 lines · Andrew Morton
Quoted 5 lines · 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:
Quoted 8 lines
Quoted 3 lines
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_GPLforcpu_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
Quoted 16 lines
Quoted 16 lines
WARNING:
printk()should includeKERN_facility level #4838: FILE:home/apw/git/linux-2.6/kernel/sched.c:4835:(quoted diff) +1
-0+ printk("%-13.13s %c", p->comm,WARNING:
printk()should includeKERN_facility level #5622: FILE:home/apw/git/linux-2.6/kernel/sched.c:5619:(quoted diff) +1
-0+ printk("\n");WARNING:
printk()should includeKERN_facility level #5633: FILE:home/apw/git/linux-2.6/kernel/sched.c:5630:(quoted diff) +1
-0+ printk("\n");WARNING:
printk()should includeKERN_facility level #5640: FILE:home/apw/git/linux-2.6/kernel/sched.c:5637:(quoted diff) +1
-0+ printk(" %s", str);Quoted 10 lines
Quoted 4 lines
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>(quoted diff) +1
-0+ printk("bar\n");
Quoted 1 line
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
PATCH 1/1 printk: add KERN_CONT annotation
Quoted 8 lines · Willy Tarreau
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");
Quoted 5 lines
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.
--- include/linux/kernel.h | 7 +++++++ kernel/sched.c | 22 +++++++++++----------- 2 files changed, 18 insertions(+), 11 deletions(-)
include/linux/kernel.h +7 -0
@@ -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])
kernel/sched.c +11 -11
@@ -4770,18 +4770,18 @@ static void show_task(struct task_structunsigned 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 sccpus_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 "
Re: [patch] printk: add KERN_CONT annotation
Quoted 3 lines · Ingo Molnar
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
Quoted 6 lines
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
Signature · 3 lines
When people work hard for you for a pat on the back, you've got to give them that pat. -- Robert Heinlein
Re: [patch] printk: add KERN_CONT annotation
In reply to Ingo MolnarQuoted 1 line · Ingo Molnar
+#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)
Re: [patch] printk: add KERN_CONT annotation
Quoted 5 lines · Joe Perches
Quoted 1 line · Ingo Molnar
+#define
KERN_CONT""This doesn't work with
printk(char** array[index])continuations or with strings with embeddedKERN_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)
Re: [patch] printk: add KERN_CONT annotation
Quoted 1 line · Jan Engelhardt
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.
Re: [patch] printk: add KERN_CONT annotation
Quoted 4 lines · Joe Perches
Quoted 1 line · Jan Engelhardt
always use
printk("%s", var)You have to use indirect arguments to log something?
No, you do not have to.
Quoted 1 line
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.
Re: [patch] printk: add KERN_CONT annotation
In reply to Ingo MolnarQuoted 6 lines · On Tue, 2 Oct 2007 07:18:52 +0200 Ingo Molnar <[email protected]> wrote:
printk: add the
KERN_CONTannotation (which is empty string but via which checkpatch.pl can notice that the lackingKERN_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.
Quoted 19 lines
Index:
linux/include/linux/kernel.h
===================================================================include/linux/kernel.h +7
-0@@ -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])
Quoted 1 line
Index:
linux/kernel/sched.c
I get rejects from the sched.c hunk and that's your stuff anwyay, so I dropped that bit.
Re: [patch] printk: add KERN_CONT annotation
Quoted 11 lines · Andrew Morton
Quoted 6 lines · On Tue, 2 Oct 2007 07:18:52 +0200 Ingo Molnar <[email protected]> wrote:
printk: add the
KERN_CONTannotation (which is empty string but via which checkpatch.pl can notice that the lackingKERN_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.
Quoted 5 lines
Quoted 2 lines
#define
console_loglevel(console_printk[0])
Index:linux/kernel/sched.cI get rejects from the
sched.chunk and that's your stuff anwyay, so I dropped that bit.
yeah, i'll fix sched.c up once it goes upstream.
Ingo