| Message ID | 1317977417-17874-1-git-send-email-vjaquez@igalia.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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.
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.
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.
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.
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.
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; } }
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(-)