(lkml Cc:-ed - this might be of interest to others too)


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 

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 

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(), 

If a statement is not single-line, then braces _are_ fine. Where does 


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
-

[ message continues ]

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");


I'd disagree with checkpatch there.  Again, it might be hard to fix.  Wanna

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.

-

[ message continues ]

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.

-

[ message continues ]

Yep.  Or with interrupts...
-

[ message continues ]

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
-

[ message continues ]

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
-

[ message continues ]

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" <paulmck@linux.vnet.ibm.com>

export cpu_clock() - the preferred API instead of sched_clock().

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 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)
-

[ message continues ]

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
-

[ message continues ]

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

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_

-apw
-

[ message continues ]

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

-

[ message continues ]

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 <mingo@elte.hu>

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 <mingo@elte.hu>
---
 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 ...
[ message continues ]

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:

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
-

[ message continues ]

This doesn't work with printk(char** array[index]) continuations
or with strings with embedded KERN_ prefixes.

printk(array[index])
printk(version)


-

[ message continues ]

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)
-

[ message continues ]

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.


-

[ message continues ]

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.
-

[ message continues ]

On Tue, 2 Oct 2007 07:18:52 +0200


I get rejects from the sched.c hunk and that's your stuff anwyay, so I
dropped that bit.

-

[ message continues ]

yeah, i'll fix sched.c up once it goes upstream.

	Ingo
-

[ message continues ]