Re: [PATCH 1/3] fix debug message of CPU clock speed

[Ingo Molnar]

* Yasuaki Ishimatsu <isimatu.yasuaki@xxxxxxxxxxxxxx> wrote:

> LOCAL APIC is corrected by PM-Timer, when SMI occurred while LOCAL APIC 
> is calibrated. In this case, LOCAL APIC debug message(Boot with 
> apic=debug) is displayed correctly, however, CPU clock speed debug 
> message is displayed wrongly .

your fix looks good, but there are a few minor style details that need to 
be addressed:

> When SMI occured on my machine, which has 1.6GHz CPU, CPU clock speed is 
> displayed 3622.0205 MHz as follow.
> 
> 	CPU0: Intel(R) Xeon(R) CPU            5110  @ 1.60GHz stepping 06
> 	Using local APIC timer interrupts.
> 	calibrating APIC timer ...
> 	... lapic delta = 3773130
> 	... PM timer delta = 812434
> 	APIC calibration not consistent with PM Timer: 226ms instead of 100ms
> 	APIC delta adjusted to PM-Timer: 1662420 (3773130)
> 	..... delta 1662420
> 	..... mult: 71411249
> 	..... calibration result: 265987
> 	..... CPU clock speed is 3622.0205 MHz.  =====>  here
> 	..... host bus clock speed is 265.0987 MHz.
> 
> This patch fixes to displaying CPU clock speed correctly as follow.
> 
> 	CPU0: Intel(R) Xeon(R) CPU            5110  @ 1.60GHz stepping 06
> 	Using local APIC timer interrupts.
> 	calibrating APIC timer ...
> 	... lapic delta = 3773131
> 	... PM timer delta = 812434
> 	APIC calibration not consistent with PM Timer: 226ms instead of 100ms
> 	APIC delta adjusted to PM-Timer: 1662420 (3773131)
> 	TSC delta adjusted to PM-Timer: 159592409 (362220564)
> 	..... delta 1662420
> 	..... mult: 71411249
> 	..... calibration result: 265987
> 	..... CPU clock speed is 1595.0924 MHz.
> 	..... host bus clock speed is 265.0987 MHz.
> 
> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@xxxxxxxxxxxxxx>
> 
> ---
>  arch/x86/kernel/apic.c |   23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> Index: linux-2.6.29-rc2/arch/x86/kernel/apic.c
> ===================================================================
> --- linux-2.6.29-rc2.orig/arch/x86/kernel/apic.c	2009-01-27 11:04:40.000000000 +0900
> +++ linux-2.6.29-rc2/arch/x86/kernel/apic.c	2009-01-27 12:29:03.000000000 +0900
> @@ -535,7 +535,8 @@ static void __init lapic_cal_handler(str
>  	}
>  }
> 
> -static int __init calibrate_by_pmtimer(long deltapm, long *delta)
> +static int __init calibrate_by_pmtimer(long deltapm, long *delta,
> +						long *deltatsc)

The cleaner prototype line-width break is:

  + static int __init
  + calibrate_by_pmtimer(long deltapm, long *delta, long *deltatsc)


this function:

> @@ -569,6 +570,15 @@ static int __init calibrate_by_pmtimer(l
>  		pr_info("APIC delta adjusted to PM-Timer: "
>  			"%lu (%ld)\n", (unsigned long)res, *delta);
>  		*delta = (long)res;
> +
> +		if (cpu_has_tsc) {
> +			res = (((u64)(*deltatsc)) * pm_100ms);
> +			do_div(res, deltapm);
> +			apic_printk(APIC_VERBOSE, "TSC delta adjusted to "
> +					"PM-Timer: %lu (%ld)\n",
> +					(unsigned long)res, *deltatsc);
> +			*deltatsc = (long)res;
> +		}

has too deep nesting in the form of:

	if (A) {
		...
	} else {
		...
		if (B) {
			...
		}
	}

	return 0;


Could you please restructure it to a cleaner control flow, in the form of:

	if (A) {
		...
		return 0;
	}
	...
	if (B) {
		...
  	}

	return 0;

?

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/