[PATCH] MIPS: Don't overflow cevt-r4k.c calculations at high clock rates.

[PATCH] MIPS: Don't overflow cevt-r4k.c calculations at high clock rates.

[David Daney]

The 'mult' element of struct clock_event_device must never be wider
than 32-bits.  If it were, it would get truncated when used by
clockevent_delta2ns() when this calls do_div().

We meet the requirement by ensuring that the relationship:

 (mips_hpt_frequency >> (32 - shift)) < NSEC_PER_SEC

Always holds.

Signed-off-by: David Daney <ddaney@caviumnetworks.com>
CC: Thomas Gleixner <tglx@linutronix.de>
---
 arch/mips/kernel/cevt-r4k.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/arch/mips/kernel/cevt-r4k.c b/arch/mips/kernel/cevt-r4k.c
index 0b2450c..4495158 100644
--- a/arch/mips/kernel/cevt-r4k.c
+++ b/arch/mips/kernel/cevt-r4k.c
@@ -163,10 +163,11 @@ int c0_compare_int_usable(void)
 
 int __cpuinit r4k_clockevent_init(void)
 {
-	uint64_t mips_freq = mips_hpt_frequency;
+	uint64_t scaled_freq = mips_hpt_frequency;
 	unsigned int cpu = smp_processor_id();
 	struct clock_event_device *cd;
 	unsigned int irq;
+	int shift;
 
 	if (!cpu_has_counter || !mips_hpt_frequency)
 		return -ENXIO;
@@ -189,8 +190,17 @@ int __cpuinit r4k_clockevent_init(void)
 	cd->features		= CLOCK_EVT_FEAT_ONESHOT;
 
 	/* Calculate the min / max delta */
-	cd->mult	= div_sc((unsigned long) mips_freq, NSEC_PER_SEC, 32);
-	cd->shift		= 32;
+	shift = 32;
+	while (scaled_freq >= NSEC_PER_SEC && shift) {
+		scaled_freq = scaled_freq >> 1;
+		shift--;
+	}
+	BUG_ON(shift == 0);
+
+	cd->mult		= div_sc((unsigned long) mips_hpt_frequency,
+					 NSEC_PER_SEC, shift);
+	cd->shift		= shift;
+
 	cd->max_delta_ns	= clockevent_delta2ns(0x7fffffff, cd);
 	cd->min_delta_ns	= clockevent_delta2ns(0x300, cd);
 
-- 
1.6.6.1

Re: [PATCH] MIPS: Don't overflow cevt-r4k.c calculations at high clock rates.

[Kevin D. Kissell]

Easy for me to say, of course, but once this patch is accepted, the same 
thing needs to be done to smtc_clockevent_init() in cevt-smtc.c.

          Regards,

          Kevin K.


David Daney wrote:
> The 'mult' element of struct clock_event_device must never be wider
> than 32-bits.  If it were, it would get truncated when used by
> clockevent_delta2ns() when this calls do_div().
>
> We meet the requirement by ensuring that the relationship:
>
>  (mips_hpt_frequency >> (32 - shift)) < NSEC_PER_SEC
>
> Always holds.
>
> Signed-off-by: David Daney <ddaney@caviumnetworks.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/mips/kernel/cevt-r4k.c |   16 +++++++++++++---
>  1 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/arch/mips/kernel/cevt-r4k.c b/arch/mips/kernel/cevt-r4k.c
> index 0b2450c..4495158 100644
> --- a/arch/mips/kernel/cevt-r4k.c
> +++ b/arch/mips/kernel/cevt-r4k.c
> @@ -163,10 +163,11 @@ int c0_compare_int_usable(void)
>  
>  int __cpuinit r4k_clockevent_init(void)
>  {
> -	uint64_t mips_freq = mips_hpt_frequency;
> +	uint64_t scaled_freq = mips_hpt_frequency;
>  	unsigned int cpu = smp_processor_id();
>  	struct clock_event_device *cd;
>  	unsigned int irq;
> +	int shift;
>  
>  	if (!cpu_has_counter || !mips_hpt_frequency)
>  		return -ENXIO;
> @@ -189,8 +190,17 @@ int __cpuinit r4k_clockevent_init(void)
>  	cd->features		= CLOCK_EVT_FEAT_ONESHOT;
>  
>  	/* Calculate the min / max delta */
> -	cd->mult	= div_sc((unsigned long) mips_freq, NSEC_PER_SEC, 32);
> -	cd->shift		= 32;
> +	shift = 32;
> +	while (scaled_freq >= NSEC_PER_SEC && shift) {
> +		scaled_freq = scaled_freq >> 1;
> +		shift--;
> +	}
> +	BUG_ON(shift == 0);
> +
> +	cd->mult		= div_sc((unsigned long) mips_hpt_frequency,
> +					 NSEC_PER_SEC, shift);
> +	cd->shift		= shift;
> +
>  	cd->max_delta_ns	= clockevent_delta2ns(0x7fffffff, cd);
>  	cd->min_delta_ns	= clockevent_delta2ns(0x300, cd);
>  
>   

Re: [PATCH] MIPS: Don't overflow cevt-r4k.c calculations at high clock rates.

[Ralf Baechle]

On Tue, May 18, 2010 at 03:48:17PM -0700, Kevin D. Kissell wrote:

> Easy for me to say, of course, but once this patch is accepted, the
> same thing needs to be done to smtc_clockevent_init() in
> cevt-smtc.c.

Thanks for the reminder, Kevin.  I'll take care of it.

That said, testing might be a bit harder - there is not enough liquid
helium on the world to get my CoreFPGA3 to run at > 1GHz.

  Ralf

Re: [PATCH] MIPS: Don't overflow cevt-r4k.c calculations at high clock rates.

[Wu Zhangjin]

Hi,

Seems this is similar to the overflow problem in the high resolution
sched_clock(), If not ensure the 'mult' is in 32bit, we must use the
128bit calculation in the common function clocksource_cyc2ns() defined
in include/linux/clocksource.h.

Regards,
	Wu Zhangjin

On Tue, 2010-05-18 at 15:11 -0700, David Daney wrote:
> The 'mult' element of struct clock_event_device must never be wider
> than 32-bits.  If it were, it would get truncated when used by
> clockevent_delta2ns() when this calls do_div().
> 
> We meet the requirement by ensuring that the relationship:
> 
>  (mips_hpt_frequency >> (32 - shift)) < NSEC_PER_SEC
> 
> Always holds.
> 
> Signed-off-by: David Daney <ddaney@caviumnetworks.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/mips/kernel/cevt-r4k.c |   16 +++++++++++++---
>  1 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/mips/kernel/cevt-r4k.c b/arch/mips/kernel/cevt-r4k.c
> index 0b2450c..4495158 100644
> --- a/arch/mips/kernel/cevt-r4k.c
> +++ b/arch/mips/kernel/cevt-r4k.c
> @@ -163,10 +163,11 @@ int c0_compare_int_usable(void)
>  
>  int __cpuinit r4k_clockevent_init(void)
>  {
> -	uint64_t mips_freq = mips_hpt_frequency;
> +	uint64_t scaled_freq = mips_hpt_frequency;
>  	unsigned int cpu = smp_processor_id();
>  	struct clock_event_device *cd;
>  	unsigned int irq;
> +	int shift;
>  
>  	if (!cpu_has_counter || !mips_hpt_frequency)
>  		return -ENXIO;
> @@ -189,8 +190,17 @@ int __cpuinit r4k_clockevent_init(void)
>  	cd->features		= CLOCK_EVT_FEAT_ONESHOT;
>  
>  	/* Calculate the min / max delta */
> -	cd->mult	= div_sc((unsigned long) mips_freq, NSEC_PER_SEC, 32);
> -	cd->shift		= 32;
> +	shift = 32;
> +	while (scaled_freq >= NSEC_PER_SEC && shift) {
> +		scaled_freq = scaled_freq >> 1;
> +		shift--;
> +	}
> +	BUG_ON(shift == 0);
> +
> +	cd->mult		= div_sc((unsigned long) mips_hpt_frequency,
> +					 NSEC_PER_SEC, shift);
> +	cd->shift		= shift;
> +
>  	cd->max_delta_ns	= clockevent_delta2ns(0x7fffffff, cd);
>  	cd->min_delta_ns	= clockevent_delta2ns(0x300, cd);
>  

Re: [PATCH] MIPS: Don't overflow cevt-r4k.c calculations at high clock rates.

[Sergei Shtylyov]

Hello.

David Daney wrote:

> The 'mult' element of struct clock_event_device must never be wider
> than 32-bits.  If it were, it would get truncated when used by
> clockevent_delta2ns() when this calls do_div().

> We meet the requirement by ensuring that the relationship:

>  (mips_hpt_frequency >> (32 - shift)) < NSEC_PER_SEC

> Always holds.

> Signed-off-by: David Daney <ddaney@caviumnetworks.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
[...]
> diff --git a/arch/mips/kernel/cevt-r4k.c b/arch/mips/kernel/cevt-r4k.c
> index 0b2450c..4495158 100644
> --- a/arch/mips/kernel/cevt-r4k.c
> +++ b/arch/mips/kernel/cevt-r4k.c
[...]
> @@ -189,8 +190,17 @@ int __cpuinit r4k_clockevent_init(void)
>  	cd->features		= CLOCK_EVT_FEAT_ONESHOT;
>  
>  	/* Calculate the min / max delta */
> -	cd->mult	= div_sc((unsigned long) mips_freq, NSEC_PER_SEC, 32);
> -	cd->shift		= 32;
> +	shift = 32;
> +	while (scaled_freq >= NSEC_PER_SEC && shift) {
> +		scaled_freq = scaled_freq >> 1;

    Why not:

		scaled_freq >>= 1;

    It's C after all. :-)

WBR, Sergei

Re: [PATCH] MIPS: Don't overflow cevt-r4k.c calculations at high clock rates.

[Thomas Gleixner]

On Tue, 18 May 2010, David Daney wrote:

> The 'mult' element of struct clock_event_device must never be wider
> than 32-bits.  If it were, it would get truncated when used by
> clockevent_delta2ns() when this calls do_div().
> 
> We meet the requirement by ensuring that the relationship:
> 
>  (mips_hpt_frequency >> (32 - shift)) < NSEC_PER_SEC
> 
> Always holds.

clocks_calc_mult_shift() is your friend :)

Thanks,

	tglx

Re: [PATCH] MIPS: Don't overflow cevt-r4k.c calculations at high clock rates.

[Thomas Gleixner]

On Wed, 19 May 2010, Thomas Gleixner wrote:

> On Tue, 18 May 2010, David Daney wrote:
> 
> > The 'mult' element of struct clock_event_device must never be wider
> > than 32-bits.  If it were, it would get truncated when used by
> > clockevent_delta2ns() when this calls do_div().
> > 
> > We meet the requirement by ensuring that the relationship:
> > 
> >  (mips_hpt_frequency >> (32 - shift)) < NSEC_PER_SEC
> > 
> > Always holds.
> 
> clocks_calc_mult_shift() is your friend :)

There are wrapper functions for clock sources and clock events as well:

clocksource_calc_mult_shift()
clockevents_calc_mult_shift()

Thanks,

	tglx