[PATCH] omap: dmtimer: convert printk to pr_*

[PATCH] omap: dmtimer: convert printk to pr_*

[Víctor Manuel Jáquez Leal]

Convert all the printk(<level>) messages in the driver to pr_<level>().

Signed-off-by: V?ctor Manuel J?quez Leal <vjaquez@igalia.com>
---
 arch/arm/plat-omap/dmtimer.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index ee9f6eb..34384b0 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -322,7 +322,7 @@ static void omap_dm_timer_wait_for_reset(struct omap_dm_timer *timer)
 	while (!(omap_dm_timer_read_reg(timer, OMAP_TIMER_SYS_STAT_REG) & 1)) {
 		c++;
 		if (c > 100000) {
-			printk(KERN_ERR "Timer failed to reset\n");
+			pr_err("Timer failed to reset\n");
 			return;
 		}
 	}
@@ -397,8 +397,7 @@ struct omap_dm_timer *omap_dm_timer_request_specific(int id)
 	spin_lock_irqsave(&dm_timer_lock, flags);
 	if (id <= 0 || id > dm_timer_count || dm_timers[id-1].reserved) {
 		spin_unlock_irqrestore(&dm_timer_lock, flags);
-		printk("BUG: warning at %s:%d/%s(): unable to get timer %d\n",
-		       __FILE__, __LINE__, __func__, id);
+		pr_warning("unable to get timer %d\n", id);
 		dump_stack();
 		return NULL;
 	}
-- 
1.7.6.3

[PATCH] omap: dmtimer: convert printk to pr_*

[Joe Perches]

On Thu, 2011-10-06 at 12:48 +0200, V?ctor Manuel J?quez Leal wrote:
> Convert all the printk(<level>) messages in the driver to pr_<level>().
[]
> diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
[]
> @@ -397,8 +397,7 @@ struct omap_dm_timer *omap_dm_timer_request_specific(int id)
>  	spin_lock_irqsave(&dm_timer_lock, flags);
>  	if (id <= 0 || id > dm_timer_count || dm_timers[id-1].reserved) {
>  		spin_unlock_irqrestore(&dm_timer_lock, flags);
> -		printk("BUG: warning at %s:%d/%s(): unable to get timer %d\n",
> -		       __FILE__, __LINE__, __func__, id);
> +		pr_warning("unable to get timer %d\n", id);

For new conversions, please use pr_warn not pr_warning.
It's shorter and similar to other dev_warn/netdev_warn
uses.

>  		dump_stack();

To emit a message then dump stack, please use
	WARN(1, fmt, ...)

[PATCH] omap: dmtimer: convert printk to pr_*

[Víctor M. Jáquez L.]

On Thu, Oct 06, 2011 at 08:19:39AM -0700, Joe Perches wrote:
> On Thu, 2011-10-06 at 12:48 +0200, V?ctor Manuel J?quez Leal wrote:
> > Convert all the printk(<level>) messages in the driver to pr_<level>().
> []
> > diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
> []
> > @@ -397,8 +397,7 @@ struct omap_dm_timer *omap_dm_timer_request_specific(int id)
> >  	spin_lock_irqsave(&dm_timer_lock, flags);
> >  	if (id <= 0 || id > dm_timer_count || dm_timers[id-1].reserved) {
> >  		spin_unlock_irqrestore(&dm_timer_lock, flags);
> > -		printk("BUG: warning at %s:%d/%s(): unable to get timer %d\n",
> > -		       __FILE__, __LINE__, __func__, id);
> > +		pr_warning("unable to get timer %d\n", id);
> 
> For new conversions, please use pr_warn not pr_warning.
> It's shorter and similar to other dev_warn/netdev_warn
> uses.
> 
> >  		dump_stack();
> 
> To emit a message then dump stack, please use
> 	WARN(1, fmt, ...)
> 

Thanks a lot!

I'll do that and then resend.

vmjl

[PATCH v2] omap: dmtimer: convert printk to pr_err / WARN

[Víctor Manuel Jáquez Leal]

Convert a printk(KERN_ERR) message in the driver to pr_err(), and
use WARN() instead of a custom log message with a stack dump.

Signed-off-by: V?ctor Manuel J?quez Leal <vjaquez@igalia.com>
---
 arch/arm/plat-omap/dmtimer.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index ee9f6eb..dead0f3 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -322,7 +322,7 @@ static void omap_dm_timer_wait_for_reset(struct omap_dm_timer *timer)
 	while (!(omap_dm_timer_read_reg(timer, OMAP_TIMER_SYS_STAT_REG) & 1)) {
 		c++;
 		if (c > 100000) {
-			printk(KERN_ERR "Timer failed to reset\n");
+			pr_err("Timer failed to reset\n");
 			return;
 		}
 	}
@@ -397,9 +397,7 @@ struct omap_dm_timer *omap_dm_timer_request_specific(int id)
 	spin_lock_irqsave(&dm_timer_lock, flags);
 	if (id <= 0 || id > dm_timer_count || dm_timers[id-1].reserved) {
 		spin_unlock_irqrestore(&dm_timer_lock, flags);
-		printk("BUG: warning at %s:%d/%s(): unable to get timer %d\n",
-		       __FILE__, __LINE__, __func__, id);
-		dump_stack();
+		WARN(1, "unable to get timer %d\n", id);
 		return NULL;
 	}
 
-- 
1.7.6.3

[PATCH v2] omap: dmtimer: convert printk to pr_err / WARN

[Tony Lindgren]

* V?ctor Manuel J?quez Leal <vjaquez@igalia.com> [111006 09:56]:
> Convert a printk(KERN_ERR) message in the driver to pr_err(), and
> use WARN() instead of a custom log message with a stack dump.
> 
> Signed-off-by: V?ctor Manuel J?quez Leal <vjaquez@igalia.com>

Looks like this won't apply with the changes we have queued up,
care to refresh this against linux-omap master branch (or dmtimer
branch)?

Regards,

Tony

> ---
>  arch/arm/plat-omap/dmtimer.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
> index ee9f6eb..dead0f3 100644
> --- a/arch/arm/plat-omap/dmtimer.c
> +++ b/arch/arm/plat-omap/dmtimer.c
> @@ -322,7 +322,7 @@ static void omap_dm_timer_wait_for_reset(struct omap_dm_timer *timer)
>  	while (!(omap_dm_timer_read_reg(timer, OMAP_TIMER_SYS_STAT_REG) & 1)) {
>  		c++;
>  		if (c > 100000) {
> -			printk(KERN_ERR "Timer failed to reset\n");
> +			pr_err("Timer failed to reset\n");
>  			return;
>  		}
>  	}
> @@ -397,9 +397,7 @@ struct omap_dm_timer *omap_dm_timer_request_specific(int id)
>  	spin_lock_irqsave(&dm_timer_lock, flags);
>  	if (id <= 0 || id > dm_timer_count || dm_timers[id-1].reserved) {
>  		spin_unlock_irqrestore(&dm_timer_lock, flags);
> -		printk("BUG: warning at %s:%d/%s(): unable to get timer %d\n",
> -		       __FILE__, __LINE__, __func__, id);
> -		dump_stack();
> +		WARN(1, "unable to get timer %d\n", id);
>  		return NULL;
>  	}
>  
> -- 
> 1.7.6.3
> 

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

[Víctor Manuel Jáquez Leal]

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

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;
 		}
 	}
-- 
1.7.6.3

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

[Russell King - ARM Linux]

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.

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

[Víctor M. Jáquez L.]

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

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

[Joe Perches]

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.

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

[Russell King - ARM Linux]

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.

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

[Joe Perches]

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.

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

[Russell King - ARM Linux]

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.

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

[Joe Perches]

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.