Patchwork [v3] omap: dmtimer: convert printk to pr_err

login
register
mail settings
Submitter Víctor Manuel Jáquez Leal
Date Oct. 7, 2011, 8:50 a.m.
Message ID <1317977417-17874-1-git-send-email-vjaquez@igalia.com>
Download mbox | patch
Permalink /patch/118274/
State New
Headers show

Comments

Víctor Manuel Jáquez Leal - Oct. 7, 2011, 8:50 a.m.
Convert a printk(KERN_ERR) message in the driver to pr_err().

v2:
* Replaced dump_stack() with WARN()

v3:
* Rebased against omap/master

Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>
---
 arch/arm/plat-omap/dmtimer.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Russell King - ARM Linux - Oct. 7, 2011, 9:22 a.m.
On Fri, Oct 07, 2011 at 10:50:16AM +0200, Víctor Manuel Jáquez Leal wrote:
> Convert a printk(KERN_ERR) message in the driver to pr_err().
...
> @@ -111,7 +111,7 @@ static void omap_dm_timer_wait_for_reset(struct omap_dm_timer *timer)
>  	while (!(__raw_readl(timer->sys_stat) & 1)) {
>  		c++;
>  		if (c > 100000) {
> -			printk(KERN_ERR "Timer failed to reset\n");
> +			pr_err("Timer failed to reset\n");

What is the reason behind this change?  It looks like it's to use the
latest and greatest function.

If so, please don't make these changes - we have on many occasions been
blamed for size of diffstat, churn, needless change, and this patch is
exactly that.

By all means fix printk's without KERN_ constants, possibly converting
them to pr_*, but don't go around replacing printk(KERN_* with pr_*(
without ensuring that there's a real benefit to the change.
Víctor Manuel Jáquez Leal - Oct. 7, 2011, 10:46 a.m.
On Fri, Oct 07, 2011 at 10:22:43AM +0100, Russell King - ARM Linux wrote:
> On Fri, Oct 07, 2011 at 10:50:16AM +0200, Víctor Manuel Jáquez Leal wrote:
> > Convert a printk(KERN_ERR) message in the driver to pr_err().
> ...
> > @@ -111,7 +111,7 @@ static void omap_dm_timer_wait_for_reset(struct omap_dm_timer *timer)
> >  	while (!(__raw_readl(timer->sys_stat) & 1)) {
> >  		c++;
> >  		if (c > 100000) {
> > -			printk(KERN_ERR "Timer failed to reset\n");
> > +			pr_err("Timer failed to reset\n");
> 
> What is the reason behind this change?  It looks like it's to use the
> latest and greatest function.
> 
> If so, please don't make these changes - we have on many occasions been
> blamed for size of diffstat, churn, needless change, and this patch is
> exactly that.
> 
> By all means fix printk's without KERN_ constants, possibly converting
> them to pr_*, but don't go around replacing printk(KERN_* with pr_*(
> without ensuring that there's a real benefit to the change.
> 

Thanks a lot Russell, and sorry for the noise. I'm still learning how to
collaborate in the kernel.

vmjl
Joe Perches - Oct. 7, 2011, 5:40 p.m.
On Fri, 2011-10-07 at 10:22 +0100, Russell King - ARM Linux wrote:
> On Fri, Oct 07, 2011 at 10:50:16AM +0200, Víctor Manuel Jáquez Leal wrote:
> > Convert a printk(KERN_ERR) message in the driver to pr_err().
> ...
> > @@ -111,7 +111,7 @@ static void omap_dm_timer_wait_for_reset(struct omap_dm_timer *timer)
> >  	while (!(__raw_readl(timer->sys_stat) & 1)) {
> >  		c++;
> >  		if (c > 100000) {
> > -			printk(KERN_ERR "Timer failed to reset\n");
> > +			pr_err("Timer failed to reset\n");
> 
> What is the reason behind this change?  It looks like it's to use the
> latest and greatest function.

Hi Russell

I'm not promoting this patch, just commenting.

At some point in the next couple of years, I want
to convert all of, or as many as possible of, the
remaining printk uses to pr_<level>.

This would allow finer grained control over the
prefixing of KBUILD_MODNAME and __func__, and
could possibly make the kernel image smaller.

Today, arch/arm has ~3:1 ratio of printk to pr_<level>.
grep shows 1427 printks, 468 pr_<level>, 405 pr_debug's.

> If so, please don't make these changes - we have on many occasions been
> blamed for size of diffstat, churn, needless change, and this patch is
> exactly that.

True.

These trivial changes could wait until arch/arm settles
down a bit more.

> By all means fix printk's without KERN_ constants,

There's still more than 250 of those in arch/arm.
Even more with the uses of secondary things like:
#define PRINTK(x...) (foo && printk(x))

> possibly converting
> them to pr_*, but don't go around replacing printk(KERN_* with pr_*(
> without ensuring that there's a real benefit to the change.

Style consistency patches do need to be governed by
acceptable churn rate.
Russell King - ARM Linux - Oct. 7, 2011, 7:18 p.m.
On Fri, Oct 07, 2011 at 10:40:39AM -0700, Joe Perches wrote:
> At some point in the next couple of years, I want
> to convert all of, or as many as possible of, the
> remaining printk uses to pr_<level>.

If the idea is also to get rid of printk() too (which IMHO would be a
good thing as it kills off the constant need to continually patch for
missing KERN_ prefixes) then that's a good reason (provided Linus
accepts the idea.)  At that point having such patches as part of a
progressive series of patches also makes sense.

Doing it piecemeal when we've already had frequent complaints from
Linus about useless churn with no apparant reasoning behind it doesn't
help relieve us of those accusations.
Joe Perches - Oct. 7, 2011, 7:48 p.m.
On Fri, 2011-10-07 at 20:18 +0100, Russell King - ARM Linux wrote:
> On Fri, Oct 07, 2011 at 10:40:39AM -0700, Joe Perches wrote:
> > At some point in the next couple of years, I want
> > to convert all of, or as many as possible of, the
> > remaining printk uses to pr_<level>.
> If the idea is also to get rid of printk() too (which IMHO would be a
> good thing as it kills off the constant need to continually patch for
> missing KERN_ prefixes) then that's a good reason (provided Linus
> accepts the idea.)

I don't accept that idea yet.

There are about 50K printks vs 20K pr_<level>s
in kernel source.

I think 50K lines is _way_ too many to convert
in a couple of years.

I think it needs to be done subsystem by subsystem,
arch by arch, as maintainers accept.

And there's no hurry.

I have a script that automates most all of the
conversions, argument alignments, etc.
Russell King - ARM Linux - Oct. 7, 2011, 7:57 p.m.
On Fri, Oct 07, 2011 at 12:48:21PM -0700, Joe Perches wrote:
> On Fri, 2011-10-07 at 20:18 +0100, Russell King - ARM Linux wrote:
> > On Fri, Oct 07, 2011 at 10:40:39AM -0700, Joe Perches wrote:
> > > At some point in the next couple of years, I want
> > > to convert all of, or as many as possible of, the
> > > remaining printk uses to pr_<level>.
> > If the idea is also to get rid of printk() too (which IMHO would be a
> > good thing as it kills off the constant need to continually patch for
> > missing KERN_ prefixes) then that's a good reason (provided Linus
> > accepts the idea.)
> 
> I don't accept that idea yet.
> 
> There are about 50K printks vs 20K pr_<level>s
> in kernel source.
> 
> I think 50K lines is _way_ too many to convert
> in a couple of years.
> 
> I think it needs to be done subsystem by subsystem,
> arch by arch, as maintainers accept.

Agreed - but doing one instance here, maybe another instance somewhere
else, and come the merge window having several of these patches
scattered around with no real coherent "this is what we're doing, and
its all done for this bit of the tree" kind of story is not the way to
do it.

It would be good to get core code done, or a sub-arch, and then say
"we're not accepting any patch which re-introduces the problem"...
It's a little late in the cycle for that now though.
Joe Perches - Oct. 7, 2011, 8:02 p.m.
On Fri, 2011-10-07 at 20:57 +0100, Russell King - ARM Linux wrote:
> On Fri, Oct 07, 2011 at 12:48:21PM -0700, Joe Perches wrote:
> > I think it needs to be done subsystem by subsystem,
> > arch by arch, as maintainers accept.
> Agreed - but doing one instance here, maybe another instance somewhere
> else, and come the merge window having several of these patches
> scattered around with no real coherent "this is what we're doing, and
> its all done for this bit of the tree" kind of story is not the way to
> do it.
> It would be good to get core code done, or a sub-arch, and then say
> "we're not accepting any patch which re-introduces the problem"...
> It's a little late in the cycle for that now though.

Well, if you want it done for arch/arm, just let me
know when it would be convenient for you and I'll
do them all as a single patch.

Patch

diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index de7896f..5492ae1 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -111,7 +111,7 @@  static void omap_dm_timer_wait_for_reset(struct omap_dm_timer *timer)
 	while (!(__raw_readl(timer->sys_stat) & 1)) {
 		c++;
 		if (c > 100000) {
-			printk(KERN_ERR "Timer failed to reset\n");
+			pr_err("Timer failed to reset\n");
 			return;
 		}
 	}