Re: [PATCH] mcheck/therm_throt: Replace printk(KERN_XX...) with pr_xx(...)

Re: [PATCH] mcheck/therm_throt: Replace printk(KERN_XX...) with pr_xx(...)

[Ingo Molnar]

(changed the subject to be lkml compatible. Mail quoted below.)

* Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Chen Yucong <slaoub@gmail.com> wrote:
> 
> > This patch also has replaced one more printk_once() with pr_info_once().
> > 
> > Signed-off-by: Chen Yucong <slaoub@gmail.com>
> > ---
> >  arch/x86/kernel/cpu/mcheck/therm_throt.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> > index 2c5aaf8..6cc5eed 100644
> > --- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
> > +++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> > @@ -190,7 +190,7 @@ static int therm_throt_process(bool new_event, int event, int level)
> >  	/* if we just entered the thermal event */
> >  	if (new_event) {
> >  		if (event == THERMAL_THROTTLING_EVENT)
> > -			printk(KERN_CRIT "CPU%d: %s temperature above threshold, cpu clock throttled (total events = %lu)\n",
> > +			pr_crit("CPU%d: %s temperature above threshold, cpu clock throttled (total events = %lu)\n",
> >  				this_cpu,
> >  				level == CORE_LEVEL ? "Core" : "Package",
> >  				state->count);
> > @@ -198,7 +198,7 @@ static int therm_throt_process(bool new_event, int event, int level)
> >  	}
> >  	if (old_event) {
> >  		if (event == THERMAL_THROTTLING_EVENT)
> > -			printk(KERN_INFO "CPU%d: %s temperature/speed normal\n",
> > +			pr_info("CPU%d: %s temperature/speed normal\n",
> >  				this_cpu,
> >  				level == CORE_LEVEL ? "Core" : "Package");
> >  		return 1;
> > @@ -417,8 +417,7 @@ static void intel_thermal_interrupt(void)
> >  
> >  static void unexpected_thermal_interrupt(void)
> >  {
> > -	printk(KERN_ERR "CPU%d: Unexpected LVT thermal interrupt!\n",
> > -			smp_processor_id());
> > +	pr_err("CPU%d: Unexpected LVT thermal interrupt!\n", smp_processor_id());
> >  }
> >  
> >  static void (*smp_thermal_vector)(void) = unexpected_thermal_interrupt;
> > @@ -499,7 +498,7 @@ void intel_init_thermal(struct cpuinfo_x86 *c)
> >  
> >  	if ((l & MSR_IA32_MISC_ENABLE_TM1) && (h & APIC_DM_SMI)) {
> >  		if (system_state == SYSTEM_BOOTING)
> > -			printk(KERN_DEBUG "CPU%d: Thermal monitoring handled by SMI\n", cpu);
> > +			pr_debug("CPU%d: Thermal monitoring handled by SMI\n", cpu);
> >  		return;
> >  	}
> >  
> > @@ -557,8 +556,8 @@ void intel_init_thermal(struct cpuinfo_x86 *c)
> >  	l = apic_read(APIC_LVTTHMR);
> >  	apic_write(APIC_LVTTHMR, l & ~APIC_LVT_MASKED);
> >  
> > -	printk_once(KERN_INFO "CPU0: Thermal monitoring enabled (%s)\n",
> > -		       tm2 ? "TM2" : "TM1");
> > +	pr_info_once("CPU0: Thermal monitoring enabled (%s)\n",
> > +		      tm2 ? "TM2" : "TM1");
> >  
> >  	/* enable thermal throttle processing */
> >  	atomic_set(&therm_throt_en, 1);
> 
> So there are 37,000+ printk() calls in the kernel and 900+ ones in arch/x86/ 
> alone! Plus even after this patch there's 20 more printk()s left in 
> arch/x86/kernel/cpu/mcheck/...
> 
> We don't want to create a churn of 10,000+ commits to convert them to pr_*() 
> facilities...
> 
> So we don't apply such 'conversion' patches unless it's done for a whole subsystem 
> and done as part of a larger work with good reason, or if it's done as part of 
> completely new facilities/drivers - and is done consistently.
> 
> Even then it's dubious to convert: people keep re-adding printk()s (which is a 
> perfectly fine facility), which generates never ending churn ...
> 
> Thanks,
> 
> 	Ingo

Re: [PATCH] mcheck/therm_throt: Replace printk(KERN_XX...) with pr_xx(...)

[Borislav Petkov]

On Mon, Feb 01, 2016 at 09:24:12AM +0100, Ingo Molnar wrote:
> > So there are 37,000+ printk() calls in the kernel and 900+ ones in arch/x86/ 
> > alone! Plus even after this patch there's 20 more printk()s left in 
> > arch/x86/kernel/cpu/mcheck/...
> > 
> > We don't want to create a churn of 10,000+ commits to convert them to pr_*() 
> > facilities...
> > 
> > So we don't apply such 'conversion' patches unless it's done for a whole subsystem 
> > and done as part of a larger work with good reason, or if it's done as part of 
> > completely new facilities/drivers - and is done consistently.
> > 
> > Even then it's dubious to convert: people keep re-adding printk()s (which is a 
> > perfectly fine facility), which generates never ending churn ...

Amen to that!

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.