Message-ID: <20101020215732.12c1a3ac@endymion.delvare>
Date:	Wed, 20 Oct 2010 21:57:32 +0200
From:	Jean Delvare <khali@...ux-fr.org>
To:	Guenter Roeck <guenter.roeck@...csson.com>
Cc:	Joe Perches <joe@...ches.com>,
	Hans de Goede <j.w.r.degoede@....nl>,
	Alistair John Strachan <alistair@...zero.co.uk>,
	Henrik Rydberg <rydberg@...omail.se>,
	"Mark M. Hoffman" <mhoffman@...htlink.com>,
	Luca Tettamanti <kronos.it@...il.com>,
	Fenghua Yu <fenghua.yu@...el.com>,
	Juerg Haefliger <juergh@...il.com>,
	Eric Piel <eric.piel@...mplin-utc.net>,
	Jim Cromie <jim.cromie@...il.com>,
	Roger Lucas <vt8231@...denengine.co.uk>,
	"lm-sensors@...sensors.org" <lm-sensors@...sensors.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drivers/hwmon: Use pr_fmt and pr_<level>

On Wed, 20 Oct 2010 11:35:44 -0700, Guenter Roeck wrote:
> On Wed, Oct 20, 2010 at 02:02:42PM -0400, Joe Perches wrote:
> > On Wed, 2010-10-20 at 10:48 -0700, Guenter Roeck wrote:
> > > The modified define in kernel.org would only apply if pr_fmt isn't already defined,
> > > so that argument is not really correct. The real difference would be that you could
> > > then remove the individual pr_fmt defines from 211 files, and all users of pr_fmt
> > > without module name (ie those hard to read) would be easier to read/identify.
> > 
> > I think that's not a good idea for now,
> > though I agree with the general concept.
> > 
> > This is in kernel.h now.
> > 
> > #ifndef pr_fmt
> > #define pr_fmt(fmt) fmt
> > #endif
> > 
> > A lot of the pr_<level> calls already include
> > some internal prefix.
> > 
> > Try:
> > $ grep -rP --include=*.[ch] -oh "\bpr_\w+\s*\(\s*\"\w+:" * | \
> > 	cut -f2- -d"(" | sort | uniq -c | sort -rn
> > 
> > You're suggesting modifying 100+ files to get the
> > same output used now.
> > 
> Yes, plus another 200+ files to remove the then-unnecessary existing pr_fmt defines.

We're set to change a lot of files anyway, so we might as well do the
right thing now and be done with it.

> > I think doing something like this after some more
> > generally accepted agreement is reached on how best
> > to do it would be better.
> > 
> Sounds like the ~400 definitions of TRUE and FALSE in the code base I am working with here.
> 
> My take is that the time to reach such an agreement is now, to avoid cluttering more code
> with pr_fmt defines. The longer we wait, the more difficult it will get to reach an agreement,
> since more and more subsystems will be affected.
> 
> Personally, I think the pr_fmt cleanup should be more important than replacing printk
> with pr_<level>. But I'll defer to Jean's judgement on this one, and follow his lead.

I totally agree with you, Guenter. My initial reaction (which I did
post!) was exactly the same as yours. Adding a define for pr_fmt to all
drivers in the kernel tree is simply insane. If the default definition
of pr_fmt is not good enough, let's just make it better. Now. The whole
point of pr_*() and dev_*() message printing functions is to have
standard message formats common to all drivers across the tree. Not
providing a sane default value for pr_fmt goes against this goal.

As far as I can see, the only issue with defaulting to 'KBUILD_BASENAME
": " fmt' is that some drivers which include a hard-coded prefix in log
message will now have a redundant prefix. Should be easy enough to spot
and fix, right?

I would even go one step further and claim that making pr_fmt
configurable per driver is pointless. But this can be discussed
separately/later.

-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists