Re: [PATCH v3] omap: dmtimer: convert printk to pr_err
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 10) {
- 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.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] omap: dmtimer: convert printk to pr_err
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 10) {
- 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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] omap: dmtimer: convert printk to pr_err
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 10) {
- 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.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] omap: dmtimer: convert printk to pr_err
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. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] omap: dmtimer: convert printk to pr_err
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_levels 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. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] omap: dmtimer: convert printk to pr_err
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_levels 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. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] omap: dmtimer: convert printk to pr_err
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. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
