2010-02-18 22:30:16

by Alok Kataria

[permalink] [raw]
Subject: Re: Bug in HPET RTC emulation.

[Copying LKML]

<ping>

Not sure if you guys got a chance to look at this patch, the fix is
pretty trivial too.

Thanks,
Alok

On Tue, 2010-02-09 at 17:26 -0800, Alok Kataria wrote:
> Hi Venki/ Vojtech,
>
> We think there exists a bug in the HPET code that emulates the RTC.
>
> In the normal case, when the RTC frequency is set, the rtc driver tells
> the hpet code about it here:
>
> int hpet_set_periodic_freq(unsigned long freq)
> {
> uint64_t clc;
>
> if (!is_hpet_enabled())
> return 0;
>
> if (freq <= DEFAULT_RTC_INT_FREQ)
> hpet_pie_limit = DEFAULT_RTC_INT_FREQ / freq;
> else {
> clc = (uint64_t) hpet_clockevent.mult * NSEC_PER_SEC;
> do_div(clc, freq);
> clc >>= hpet_clockevent.shift;
> hpet_pie_delta = (unsigned long) clc;
> }
> return 1;
> }
>
> If freq is set to 64Hz (DEFAULT_RTC_INT_FREQ) or lower, then
> hpet_pie_limit (a static) is set to non-zero.
> Then, on every one-shot HPET interrupt, hpet_rtc_timer_reinit is called
> to compute the next timeout. Well, that function has this logic:
>
> if (!(hpet_rtc_flags & RTC_PIE) || hpet_pie_limit)
> delta = hpet_default_delta;
> else
> delta = hpet_pie_delta;
>
> Since hpet_pie_limit is not 0, hpet_default_delta is used. That
> corresponds to 64Hz.
>
> Now, if you set a different rtc frequency, you'll take the else path
> through hpet_set_periodic_freq, but unfortunately no one resets
> hpet_pie_limit back to 0.
>
> Boom....now you are stuck with 64Hz HPET interrupts forever.
>
> The patch below just resets the hpet_pie_limit value when requested freq
> is greater than DEFAULT_RTC_INT_FREQ, which we think fixes this problem.
>
> Thanks,
> Alok
>
> Index: linux-2.6/arch/x86/kernel/hpet.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/hpet.c 2010-01-14 13:47:15.000000000 -0800
> +++ linux-2.6/arch/x86/kernel/hpet.c 2010-02-09 17:20:50.000000000 -0800
> @@ -1135,6 +1135,7 @@ int hpet_set_periodic_freq(unsigned long
> do_div(clc, freq);
> clc >>= hpet_clockevent.shift;
> hpet_pie_delta = clc;
> + hpet_pie_limit = 0;
> }
> return 1;
> }
>


2010-02-22 22:26:48

by Andrew Morton

[permalink] [raw]
Subject: Re: Bug in HPET RTC emulation.

On Thu, 18 Feb 2010 14:30:08 -0800 Alok Kataria <[email protected]> wrote:

> [Copying LKML]

Please do that all the time!

> <ping>
>
> Not sure if you guys got a chance to look at this patch, the fix is
> pretty trivial too.
>

Please resend everything, including a Signed-off-by: as per
Documentation/SubmittingPatches.

Thanks.

2010-02-22 23:20:55

by Alok Kataria

[permalink] [raw]
Subject: Re: Bug in HPET RTC emulation.


On Mon, 2010-02-22 at 14:24 -0800, Andrew Morton wrote:
> On Thu, 18 Feb 2010 14:30:08 -0800 Alok Kataria <[email protected]> wrote:
>
> > [Copying LKML]
>
> Please do that all the time!
>
> > <ping>
> >
> > Not sure if you guys got a chance to look at this patch, the fix is
> > pretty trivial too.
> >
>
> Please resend everything, including a Signed-off-by: as per
> Documentation/SubmittingPatches.

Hi Andrew,

Here it is.

--

We think there exists a bug in the HPET code that emulates the RTC.

In the normal case, when the RTC frequency is set, the rtc driver tells
the hpet code about it here:

int hpet_set_periodic_freq(unsigned long freq)
{
uint64_t clc;

if (!is_hpet_enabled())
return 0;

if (freq <= DEFAULT_RTC_INT_FREQ)
hpet_pie_limit = DEFAULT_RTC_INT_FREQ / freq;
else {
clc = (uint64_t) hpet_clockevent.mult * NSEC_PER_SEC;
do_div(clc, freq);
clc >>= hpet_clockevent.shift;
hpet_pie_delta = (unsigned long) clc;
}
return 1;
}

If freq is set to 64Hz (DEFAULT_RTC_INT_FREQ) or lower, then
hpet_pie_limit (a static) is set to non-zero.
Then, on every one-shot HPET interrupt, hpet_rtc_timer_reinit is called
to compute the next timeout. Well, that function has this logic:

if (!(hpet_rtc_flags & RTC_PIE) || hpet_pie_limit)
delta = hpet_default_delta;
else
delta = hpet_pie_delta;

Since hpet_pie_limit is not 0, hpet_default_delta is used. That
corresponds to 64Hz.

Now, if you set a different rtc frequency, you'll take the else path
through hpet_set_periodic_freq, but unfortunately no one resets
hpet_pie_limit back to 0.

Boom....now you are stuck with 64Hz RTC interrupts forever.

The patch below just resets the hpet_pie_limit value when requested freq
is greater than DEFAULT_RTC_INT_FREQ, which we think fixes this problem.

Signed-off-by: Alok N Kataria <[email protected]>
Signed-off-by: Daniel Hecht <[email protected]>

---

arch/x86/kernel/hpet.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)


diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index ad80a1c..a79a358 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -1143,6 +1143,7 @@ int hpet_set_periodic_freq(unsigned long freq)
do_div(clc, freq);
clc >>= hpet_clockevent.shift;
hpet_pie_delta = clc;
+ hpet_pie_limit = 0;
}
return 1;
}