Re: [PATCH 0/8] replacing/fixing printk with pr_debug/pr_info in arch/i386 - intro

Re: [PATCH 0/8] replacing/fixing printk with pr_debug/pr_info in arch/i386 - intro

[Ingo Molnar]

* Daniele Pizzoni <auouo@tin.it> wrote:

> Hello, I'm going to post a series of small janitorial patches focused on
> 1) replacing DPRINTK-style macros with pr_debug from kernel.h
> 2) replacing printk(KERN_INFO ...) with pr_info(...)
> 3) fixing _obvious_ inconsistencies of printk levels as:
> 
> printk(KERN_INFO "Start... ");
> ...
> printk("Ok!\n");

1) be careful, there is no inconsistency here. It's a printk that doesnt
end in a "\n" in the first line.

2) i dont like the pr_print name at all. What's wrong with Dprintk or
dprintk? Just define them in kernel.h, this will also make your patch
much smaller.

	Ingo

[PATCH 0/8] replacing/fixing printk with pr_debug/pr_info in arch/i386 - intro

[Daniele Pizzoni]

Hello, I'm going to post a series of small janitorial patches focused on
1) replacing DPRINTK-style macros with pr_debug from kernel.h
2) replacing printk(KERN_INFO ...) with pr_info(...)
3) fixing _obvious_ inconsistencies of printk levels as:

printk(KERN_INFO "Start... ");
...
printk("Ok!\n");

The patches fix only obvious inconsistencies: seems that there has been
no policy about how a subsystem should print boot/log messages
(subsistem prefix, etc.) nor about the difference between printk(...)
and printk(KERN_WARNING ...), etc. So I did not touch those issues.

The patches regard (mainly) files in arch/i386/kernel.
The patches are based upon 2.6.9-rc4.
The patches have been compile and boot tested.
One file one patch.

Any comment is very welcome.

CC'ed to Ingo Molnar, hoping that's appropriate.

Bye
Daniele Pizzoni <auouo@tin.it>

Re: [PATCH 0/8] replacing/fixing printk with pr_debug/pr_info in arch/i386 - intro

[Geert Uytterhoeven]

On Sun, 17 Oct 2004, Ingo Molnar wrote:
> * Daniele Pizzoni <auouo@tin.it> wrote:
> 
> > Hello, I'm going to post a series of small janitorial patches focused on
> > 1) replacing DPRINTK-style macros with pr_debug from kernel.h
> > 2) replacing printk(KERN_INFO ...) with pr_info(...)
> > 3) fixing _obvious_ inconsistencies of printk levels as:
> > 
> > printk(KERN_INFO "Start... ");
> > ...
> > printk("Ok!\n");
> 
> 1) be careful, there is no inconsistency here. It's a printk that doesnt
> end in a "\n" in the first line.

Indeed.

Iff you ever want to replace the above, make sure to do it like this:

    printk_info("Start... ");
    ...
    printkc_info("Ok!\n");

(with `printkc_info()' being a continuation of `printk_info()'. And do the same
for all other KERN_* variations. This would add real value, since the next step
is to make the printk{,c}_*() definitions conditionally empty for embedded
systems and/or systems with few memory.

Gr{oetje,eeting}s,

						Geert

P.S. The naming conventions above are purely hypothetical. I suggest we first
     have a few Holy Wars(tm) about them before settling :-)
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH 0/8] replacing/fixing printk with pr_debug/pr_info in arch/i386 - intro

[Joe Perches]

On Sun, 2004-10-17 at 20:10 +0200, Geert Uytterhoeven wrote:
> Iff you ever want to replace the above, make sure to do it like this:
>     printk_info("Start... ");
>     ...
>     printkc_info("Ok!\n");
> 
> (with `printkc_info()' being a continuation of `printk_info()'. And do the same
> for all other KERN_* variations. This would add real value, since the next step
> is to make the printk{,c}_*() definitions conditionally empty for embedded
> systems and/or systems with few memory.
> 
> Gr{oetje,eeting}s,

I suggest this is the incorrect usage.
Start then Continue then End.
I suppose one could use Continue/End with an implied start
or simply End (ie:  printk)

cheers,  joe

Re: [PATCH 0/8] replacing/fixing printk with pr_debug/pr_info in arch/i386 - intro

[Geert Uytterhoeven]

On Sun, 17 Oct 2004, Joe Perches wrote:
> On Sun, 2004-10-17 at 20:10 +0200, Geert Uytterhoeven wrote:
> > Iff you ever want to replace the above, make sure to do it like this:
> >     printk_info("Start... ");
> >     ...
> >     printkc_info("Ok!\n");
> > 
> > (with `printkc_info()' being a continuation of `printk_info()'. And do the same
> > for all other KERN_* variations. This would add real value, since the next step
> > is to make the printk{,c}_*() definitions conditionally empty for embedded
> > systems and/or systems with few memory.
> > 
> > Gr{oetje,eeting}s,
> 
> I suggest this is the incorrect usage.
> Start then Continue then End.
> I suppose one could use Continue/End with an implied start
> or simply End (ie:  printk)

Why?

The only things you need to know are
  - what is the printk() level? (so you can conditionally keep it out of the
    binary)
  - which printk() call contains the first part of the line? (so it can prepend
    the correct KERN_* definition)

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH 0/8] replacing/fixing printk with pr_debug/pr_info in arch/i386 - intro

[Andi Kleen]

Ingo Molnar <mingo@elte.hu> writes:
>
> 2) i dont like the pr_print name at all. What's wrong with Dprintk or
> dprintk? Just define them in kernel.h, this will also make your patch
> much smaller.

I prefer to declare them in subsystem specific files. This way you
can easily enable/disable debugging for only specific files, 
not for everything.

-Andi

Re: [PATCH 0/8] replacing/fixing printk with pr_debug/pr_info in arch/i386 - intro

[Daniele Pizzoni]

On dom, 2004-10-17 at 18:19, Ingo Molnar wrote: 
> [...]
> 
> 1) be careful, there is no inconsistency here. It's a printk that doesnt
> end in a "\n" in the first line.

You're right, my fault and a big one!

Anyway I'm going to ask some questions.

DEFAULT_MESSAGE_LOGLEVEL in printk.c is the loglevel printks' without
loglevel print to. What's its use? Why should I change the loglevel of
some randomly chosen printks around the kernel only? When should I use
the defaulting printks' in my code? Isn't confusing the fact that the
call

printk("Hello!\n");

behaves differently ("log using the default loglevel" or "continue
logging from before") depending on what's happened before?
Is this a feature, a bug or am a newbie? :)

> 2) i dont like the pr_print name at all. What's wrong with Dprintk or
> dprintk? Just define them in kernel.h, this will also make your patch
> much smaller.

There's nothing wrong with Dprintk or dprintk. I simply found a request
to do so on the janitors TODO list. I found out that in kernel.h there
was really a pr_debug macro and I used it.

The rationale is that in the kernel there are lots of custom dprintk,
Dprintk, DPRINTK, etc that we need a bit of housekeeping, I think.
Anyway I didn't like pr_info either (why not a pr_notice...?) but I used
it: it was in kernel.h I assumed it was for good.

I need a bit of advice now: should I forget about printks' levels,
consistency and focus on other issues or with a bit of work these
patches may became worth of?

Bye and thanks for your replies.

-- 
Daniele Pizzoni <auouo@tin.it>

Re: [PATCH 0/8] replacing/fixing printk with pr_debug/pr_info in arch/i386 - intro

[Ingo Molnar]

* Daniele Pizzoni <auouo@tin.it> wrote:

> On dom, 2004-10-17 at 18:19, Ingo Molnar wrote: 
> > [...]
> > 
> > 1) be careful, there is no inconsistency here. It's a printk that doesnt
> > end in a "\n" in the first line.
> 
> You're right, my fault and a big one!
> 
> Anyway I'm going to ask some questions.

> There's nothing wrong with Dprintk or dprintk. I simply found a
> request to do so on the janitors TODO list. I found out that in
> kernel.h there was really a pr_debug macro and I used it.

ok. 

> The rationale is that in the kernel there are lots of custom dprintk,
> Dprintk, DPRINTK, etc that we need a bit of housekeeping, I think.
> Anyway I didn't like pr_info either (why not a pr_notice...?) but I
> used it: it was in kernel.h I assumed it was for good.

ok - pr_debug() is ok i think for the APIC code. It pairs well with the
other variants: pr_notice(), etc.

> I need a bit of advice now: should I forget about printks' levels,
> consistency and focus on other issues or with a bit of work these
> patches may became worth of?

i'd suggest to first do the Dprintk -> pr_debug replacement patch with
as few output changes as possible. (output changes are unavoidable when
converting a \n-less printout.) Then do any format cleanups in a
separate patch.

(some of your other comments about 'spurious' whitespaces need a
double-check too, sometimes they are done for formatting reasons. So
always take a look at the log output before changing it.)

	Ingo

Re: [PATCH 0/8] replacing/fixing printk with pr_debug/pr_info in arch/i386 - intro

[Daniele Pizzoni]

On lun, 2004-10-18 at 12:36, Ingo Molnar wrote:
> 
> ok - pr_debug() is ok i think for the APIC code. It pairs well with the
> other variants: pr_notice(), etc.
> [...] 
> i'd suggest to first do the Dprintk -> pr_debug replacement patch with
> as few output changes as possible. (output changes are unavoidable when
> converting a \n-less printout.) Then do any format cleanups in a
> separate patch.

Look, there is no pr_notice at all. The whole thing is not very clear I
think, and the pr_ macros have some problem.

This is the kernel.h part regarding the two and only pr_debug and
pr_info:

#ifdef DEBUG
#define pr_debug(fmt,arg...) \
printk(KERN_DEBUG fmt,##arg)
#else
#define pr_debug(fmt,arg...) \
do { } while (0)
#endif

#define pr_info(fmt,arg...) \
printk(KERN_INFO fmt,##arg)

I think that pr_debug does make sense, because it depends on DEBUG and
is a useful macro uniforming all the DPRINTK usage.

pr_info is alone, there are no other pr_*. I used it in the patches but
now I think I was wrong. You cannot "continue" a pr_info as you do with
printk because it always inserts the loglevel tag. I think it's better
not use it or find a better solution. In fact it is not used too much in
the kernel.

I'll try some strict dprintk -> pr_debug replacements.

Bye
Daniele