Hello.
I think there is a possibility that HPET driver will return
insane value due to a SMI interruption (or switching guests by hypervisor).
I found it by reviewing, and I would like to fix it.
Current HPET driver calibrates the adjustment value
by calculation the elapse time in CPU busy loop.
However this way is too dangerous against SMI interruption.
Here is the calibration code in hpet_calibrate()
701 static unsigned long hpet_calibrate(struct hpets *hpetp)
:
:
728 do {
729 m = read_counter(&hpet->hpet_mc);
730 write_counter(t + m + hpetp->hp_delta, &timer->hpet_compare);
731 } while (i++, (m - start) < count);
732
733 local_irq_restore(flags);
734
735 return (m - start) / i;
If SMI interruption occurs between 728 to 731, then return value will be
bigger value than correct one. (SMI is not able to be controlled by OS.)
This patch is a simple solution to fix it.
hpet_calibrate() is called 5 times, and one of them is expected as
correct value.
Thanks.
---
hpet_calibrate() has a possibility of miss-calibration due to SMI.
If SMI interrupts in the while loop of calibration, then return value
will be big. This changes it tries 5 times and get minimum value as
correct value.
Signed-off-by: Yasunori Goto <[email protected]>
---
Index: hpet_test/drivers/char/hpet.c
===================================================================
--- hpet_test.orig/drivers/char/hpet.c 2008-12-04 16:24:02.000000000 +0900
+++ hpet_test/drivers/char/hpet.c 2008-12-04 16:34:59.000000000 +0900
@@ -713,7 +713,7 @@
*/
#define TICK_CALIBRATE (1000UL)
-static unsigned long hpet_calibrate(struct hpets *hpetp)
+static unsigned long __hpet_calibrate(struct hpets *hpetp)
{
struct hpet_timer __iomem *timer = NULL;
unsigned long t, m, count, i, flags, start;
@@ -750,6 +750,17 @@
return (m - start) / i;
}
+static unsigned long hpet_calibrate(struct hpets *hpetp)
+{
+ unsigned long ret = ~0UL, i;
+
+ /* Try 5 times to remove impact of SMI.*/
+ for (i = 0; i < 5; i++)
+ ret = min(ret, __hpet_calibrate(hpetp));
+
+ return ret;
+}
+
int hpet_alloc(struct hpet_data *hdp)
{
u64 cap, mcfg;
--
Yasunori Goto
Yasunori Goto wrote:
> hpet_calibrate() has a possibility of miss-calibration due to SMI.
> If SMI interrupts in the while loop of calibration, then return value
> will be big. This changes it tries 5 times and get minimum value as
> correct value.
>
> Signed-off-by: Yasunori Goto <[email protected]>
Acked-by: Rik van Riel <[email protected]>
--
All rights reversed.
On Fri, Mar 13, 2009 at 1:00 AM, Yasunori Goto <[email protected]> wrote:
> Hello.
>
> I think there is a possibility that HPET driver will return
> insane value due to a SMI interruption (or switching guests by hypervisor).
> I found it by reviewing, and I would like to fix it.
>
> Current HPET driver calibrates the adjustment value
> by calculation the elapse time in CPU busy loop.
> However this way is too dangerous against SMI interruption.
>
> Here is the calibration code in hpet_calibrate()
>
> ?701 static unsigned long hpet_calibrate(struct hpets *hpetp)
> ? ? ? ? ? ? :
> ? ? ? ? ? ? :
> ?728 ? ? ? ? do {
> ?729 ? ? ? ? ? ? ? ? m = read_counter(&hpet->hpet_mc);
> ?730 ? ? ? ? ? ? ? ? write_counter(t + m + hpetp->hp_delta, &timer->hpet_compare);
> ?731 ? ? ? ? } while (i++, (m - start) < count);
> ?732
> ?733 ? ? ? ? local_irq_restore(flags);
> ?734
> ?735 ? ? ? ? return (m - start) / i;
>
> If SMI interruption occurs between 728 to 731, then return value will be
> bigger value than correct one. (SMI is not able to be controlled by OS.)
>
>
> This patch is a simple solution to fix it.
> hpet_calibrate() is called 5 times, and one of them is expected as
> correct value.
I've no sense of feel for how long each calibration run would take.
Would doing it 5 times show up as a significant increase in the boot
time for those that care about boot time being as quick as possible?
Paul.
>
> Thanks.
>
>
> ---
>
> hpet_calibrate() has a possibility of miss-calibration due to SMI.
> If SMI interrupts in the while loop of calibration, then return value
> will be big. This changes it tries 5 times and get minimum value as
> correct value.
>
> Signed-off-by: Yasunori Goto <[email protected]>
>
> ---
>
> Index: hpet_test/drivers/char/hpet.c
> ===================================================================
> --- hpet_test.orig/drivers/char/hpet.c ?2008-12-04 16:24:02.000000000 +0900
> +++ hpet_test/drivers/char/hpet.c ? ? ? 2008-12-04 16:34:59.000000000 +0900
> @@ -713,7 +713,7 @@
> ?*/
> ?#define ? ? ? ?TICK_CALIBRATE ?(1000UL)
>
> -static unsigned long hpet_calibrate(struct hpets *hpetp)
> +static unsigned long __hpet_calibrate(struct hpets *hpetp)
> ?{
> ? ? ? ?struct hpet_timer __iomem *timer = NULL;
> ? ? ? ?unsigned long t, m, count, i, flags, start;
> @@ -750,6 +750,17 @@
> ? ? ? ?return (m - start) / i;
> ?}
>
> +static unsigned long hpet_calibrate(struct hpets *hpetp)
> +{
> + ? ? ? unsigned long ret = ~0UL, i;
> +
> + ? ? ? /* Try 5 times to remove impact of SMI.*/
> + ? ? ? for (i = 0; i < 5; i++)
> + ? ? ? ? ? ? ? ret = min(ret, __hpet_calibrate(hpetp));
> +
> + ? ? ? return ret;
> +}
> +
> ?int hpet_alloc(struct hpet_data *hdp)
> ?{
> ? ? ? ?u64 cap, mcfg;
>
> --
> Yasunori Goto
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>
> On Fri, Mar 13, 2009 at 1:00 AM, Yasunori Goto <[email protected]> wrote:
> > Hello.
> >
> > I think there is a possibility that HPET driver will return
> > insane value due to a SMI interruption (or switching guests by hypervisor).
> > I found it by reviewing, and I would like to fix it.
> >
> > Current HPET driver calibrates the adjustment value
> > by calculation the elapse time in CPU busy loop.
> > However this way is too dangerous against SMI interruption.
> >
> > Here is the calibration code in hpet_calibrate()
> >
> > ?701 static unsigned long hpet_calibrate(struct hpets *hpetp)
> > ? ? ? ? ? ? :
> > ? ? ? ? ? ? :
> > ?728 ? ? ? ? do {
> > ?729 ? ? ? ? ? ? ? ? m = read_counter(&hpet->hpet_mc);
> > ?730 ? ? ? ? ? ? ? ? write_counter(t + m + hpetp->hp_delta, &timer->hpet_compare);
> > ?731 ? ? ? ? } while (i++, (m - start) < count);
> > ?732
> > ?733 ? ? ? ? local_irq_restore(flags);
> > ?734
> > ?735 ? ? ? ? return (m - start) / i;
> >
> > If SMI interruption occurs between 728 to 731, then return value will be
> > bigger value than correct one. (SMI is not able to be controlled by OS.)
> >
> >
> > This patch is a simple solution to fix it.
> > hpet_calibrate() is called 5 times, and one of them is expected as
> > correct value.
>
> I've no sense of feel for how long each calibration run would take.
> Would doing it 5 times show up as a significant increase in the boot
> time for those that care about boot time being as quick as possible?
Hmm. The loop times is trade off against reliable value....
Though SMI is rare interruption, I don't know how frequent
hypervisor's switch is.
Each calibration of this has 1 milli second.
Do you think 5 msec is too long?
If yes, how is 3 msec? Is it still too long?
Thanks.
--
Yasunori Goto
Yasunori Goto wrote:
> > I've no sense of feel for how long each calibration run would take.
> > Would doing it 5 times show up as a significant increase in the boot
> > time for those that care about boot time being as quick as possible?
>
> Hmm. The loop times is trade off against reliable value....
> Though SMI is rare interruption, I don't know how frequent
> hypervisor's switch is.
Could you, just for testing, run the calibration five thousand times or
so instead of five times, and count how often you get insane values?
(And how much delay does such an SMI add?)
> Each calibration of this has 1 milli second.
> Do you think 5 msec is too long?
This shouldn't matter when booting. Anyway, I think it's possible to
increase TICK_CALIBRATE without losing too much accuracy.
Best regards,
Clemens
On Mon, Mar 16, 2009 at 11:34:55AM +0900, Yasunori Goto wrote:
> > I've no sense of feel for how long each calibration run would take.
> > Would doing it 5 times show up as a significant increase in the boot
> > time for those that care about boot time being as quick as possible?
>
> Hmm. The loop times is trade off against reliable value....
> Though SMI is rare interruption, I don't know how frequent
> hypervisor's switch is.
>
> Each calibration of this has 1 milli second.
> Do you think 5 msec is too long?
> If yes, how is 3 msec? Is it still too long?
I would suggest to instead repeat until the total cycle count stops
decreasing: That way we can avoid a SMI and yet in the best case only
run the calibration 2 times.
--
Vojtech Pavlik
Director SuSE Labs
Sorry for late response.
> Yasunori Goto wrote:
> > > I've no sense of feel for how long each calibration run would take.
> > > Would doing it 5 times show up as a significant increase in the boot
> > > time for those that care about boot time being as quick as possible?
> >
> > Hmm. The loop times is trade off against reliable value....
> > Though SMI is rare interruption, I don't know how frequent
> > hypervisor's switch is.
>
> Could you, just for testing, run the calibration five thousand times or
> so instead of five times, and count how often you get insane values?
> (And how much delay does such an SMI add?)
I tried 50000 times. But insane value was nothing.
I think SMI is very rare.
> > Each calibration of this has 1 milli second.
> > Do you think 5 msec is too long?
>
> This shouldn't matter when booting. Anyway, I think it's possible to
> increase TICK_CALIBRATE without losing too much accuracy.
Hmm. If the person who is trying to reduce boot time for fastboot dislikes this impact,
then I'll try Vojtech-san's way.
Thanks.
--
Yasunori Goto
Yasunori Goto wrote:
> Sorry for late response.
>
>
>> Yasunori Goto wrote:
>>
>>>> I've no sense of feel for how long each calibration run would take.
>>>> Would doing it 5 times show up as a significant increase in the boot
>>>> time for those that care about boot time being as quick as possible?
>>>>
>>> Hmm. The loop times is trade off against reliable value....
>>> Though SMI is rare interruption, I don't know how frequent
>>> hypervisor's switch is.
>>>
>> Could you, just for testing, run the calibration five thousand times or
>> so instead of five times, and count how often you get insane values?
>> (And how much delay does such an SMI add?)
>>
>
> I tried 50000 times. But insane value was nothing.
> I think SMI is very rare.
>
>
>>> Each calibration of this has 1 milli second.
>>> Do you think 5 msec is too long?
>>>
>> This shouldn't matter when booting. Anyway, I think it's possible to
>> increase TICK_CALIBRATE without losing too much accuracy.
>>
>
> Hmm. If the person who is trying to reduce boot time for fastboot dislikes this impact,
> then I'll try Vojtech-san's way.
>
That probably makes sense -- if it is extremely rare, and if you get two
values that are the same (within some small delta) then you probably
are 99.99% confident that you have the right data.
Paul.
>
> Thanks.
>
> >>> Each calibration of this has 1 milli second.
> >>> Do you think 5 msec is too long?
> >>>
> >> This shouldn't matter when booting. Anyway, I think it's possible to
> >> increase TICK_CALIBRATE without losing too much accuracy.
> >>
> >
> > Hmm. If the person who is trying to reduce boot time for fastboot dislikes this impact,
> > then I'll try Vojtech-san's way.
> >
>
> That probably makes sense -- if it is extremely rare, and if you get two
> values that are the same (within some small delta) then you probably
> are 99.99% confident that you have the right data.
Ok. I'll make it.
Thanks for your comment.
--
Yasunori Goto
>
> > >>> Each calibration of this has 1 milli second.
> > >>> Do you think 5 msec is too long?
> > >>>
> > >> This shouldn't matter when booting. Anyway, I think it's possible to
> > >> increase TICK_CALIBRATE without losing too much accuracy.
> > >>
> > >
> > > Hmm. If the person who is trying to reduce boot time for fastboot dislikes this impact,
> > > then I'll try Vojtech-san's way.
> > >
> >
> > That probably makes sense -- if it is extremely rare, and if you get two
> > values that are the same (within some small delta) then you probably
> > are 99.99% confident that you have the right data.
>
> Ok. I'll make it.
>
> Thanks for your comment.
Here is updated version.
--------
hpet_calibrate() has a possibility of miss-calibration due to SMI.
If SMI interrupts in the while loop of calibration, then return value
will be big. This change calibrates until stabilizing by the return
value with a small value.
Signed-off-by: Yasunori Goto <[email protected]>
---
drivers/char/hpet.c | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
Index: hpet_test/drivers/char/hpet.c
===================================================================
--- hpet_test.orig/drivers/char/hpet.c 2009-03-12 15:47:45.000000000 +0900
+++ hpet_test/drivers/char/hpet.c 2009-03-18 11:12:42.000000000 +0900
@@ -713,7 +713,7 @@
*/
#define TICK_CALIBRATE (1000UL)
-static unsigned long hpet_calibrate(struct hpets *hpetp)
+static unsigned long __hpet_calibrate(struct hpets *hpetp)
{
struct hpet_timer __iomem *timer = NULL;
unsigned long t, m, count, i, flags, start;
@@ -750,6 +750,25 @@
return (m - start) / i;
}
+static unsigned long hpet_calibrate(struct hpets *hpetp)
+{
+ unsigned long ret = ~0UL, tmp;
+
+ /*
+ * Try to calibrate until return value becomes stable small value.
+ * If SMI interruption occurs in calibration loop, the return value
+ * will be big. This avoids its impact.
+ */
+ do {
+ tmp = __hpet_calibrate(hpetp);
+ if (ret <= tmp)
+ break;
+ ret = tmp;
+ } while (1);
+
+ return ret;
+}
+
int hpet_alloc(struct hpet_data *hdp)
{
u64 cap, mcfg;
--
Yasunori Goto
Yasunori Goto wrote:
> hpet_calibrate() has a possibility of miss-calibration due to SMI.
> If SMI interrupts in the while loop of calibration, then return value
> will be big. This change calibrates until stabilizing by the return
> value with a small value.
>
> Signed-off-by: Yasunori Goto <[email protected]>
Acked-by: Clemens Ladisch <[email protected]>
On Wed, Mar 18, 2009 at 08:44:30AM +0100, Clemens Ladisch wrote:
> Yasunori Goto wrote:
> > hpet_calibrate() has a possibility of miss-calibration due to SMI.
> > If SMI interrupts in the while loop of calibration, then return value
> > will be big. This change calibrates until stabilizing by the return
> > value with a small value.
> >
> > Signed-off-by: Yasunori Goto <[email protected]>
>
> Acked-by: Clemens Ladisch <[email protected]>
Acked-by: Vojtech Pavlik <[email protected]>
--
Vojtech Pavlik
Director SuSE Labs
Yasunori Goto wrote:
>>>>>> Each calibration of this has 1 milli second.
>>>>>> Do you think 5 msec is too long?
>>>>>>
>>>>>>
>>>>> This shouldn't matter when booting. Anyway, I think it's possible to
>>>>> increase TICK_CALIBRATE without losing too much accuracy.
>>>>>
>>>>>
>>>> Hmm. If the person who is trying to reduce boot time for fastboot dislikes this impact,
>>>> then I'll try Vojtech-san's way.
>>>>
>>>>
>>> That probably makes sense -- if it is extremely rare, and if you get two
>>> values that are the same (within some small delta) then you probably
>>> are 99.99% confident that you have the right data.
>>>
>> Ok. I'll make it.
>>
>> Thanks for your comment.
>>
>
> Here is updated version.
>
> --------
>
> hpet_calibrate() has a possibility of miss-calibration due to SMI.
> If SMI interrupts in the while loop of calibration, then return value
> will be big. This change calibrates until stabilizing by the return
> value with a small value.
>
>
> Signed-off-by: Yasunori Goto <[email protected]>
>
>
> ---
> drivers/char/hpet.c | 21 ++++++++++++++++++++-
> 1 file changed, 20 insertions(+), 1 deletion(-)
>
> Index: hpet_test/drivers/char/hpet.c
> ===================================================================
> --- hpet_test.orig/drivers/char/hpet.c 2009-03-12 15:47:45.000000000 +0900
> +++ hpet_test/drivers/char/hpet.c 2009-03-18 11:12:42.000000000 +0900
> @@ -713,7 +713,7 @@
> */
> #define TICK_CALIBRATE (1000UL)
>
> -static unsigned long hpet_calibrate(struct hpets *hpetp)
> +static unsigned long __hpet_calibrate(struct hpets *hpetp)
> {
> struct hpet_timer __iomem *timer = NULL;
> unsigned long t, m, count, i, flags, start;
> @@ -750,6 +750,25 @@
> return (m - start) / i;
> }
>
> +static unsigned long hpet_calibrate(struct hpets *hpetp)
> +{
> + unsigned long ret = ~0UL, tmp;
> +
> + /*
> + * Try to calibrate until return value becomes stable small value.
> + * If SMI interruption occurs in calibration loop, the return value
> + * will be big. This avoids its impact.
> + */
> + do {
> + tmp = __hpet_calibrate(hpetp);
> + if (ret <= tmp)
> + break;
> + ret = tmp;
> + } while (1);
>
For what it is worth, if a situation arose where continued calls to
hpet_calibrate() represented a monotonically decreasing function,
(perhaps some insane power management?) you could be stuck
in this function for an unbounded amount of time. I don't expect
that should ever happen, but I figured I'd mention it.
Paul.
> +
> + return ret;
> +}
> +
> int hpet_alloc(struct hpet_data *hdp)
> {
> u64 cap, mcfg;
>
>
Paul Gortmaker wrote:
> Yasunori Goto wrote:
> > + /*
> > + * Try to calibrate until return value becomes stable small value.
> > + * If SMI interruption occurs in calibration loop, the return value
> > + * will be big. This avoids its impact.
> > + */
> > + do {
> > + tmp = __hpet_calibrate(hpetp);
> > + if (ret <= tmp)
> > + break;
> > + ret = tmp;
> > + } while (1);
>
> For what it is worth, if a situation arose where continued calls to
> hpet_calibrate() represented a monotonically decreasing function,
> (perhaps some insane power management?) you could be stuck
> in this function for an unbounded amount of time.
On my machine, the number of iterations of the loop in __hpet_calibarate
(the value of i) is about 400, and since HPET reads/writes go to the
southbridge, it cannot get much higher than about 1000 even on faster
machines.
The value of (m - start) is approximately constant, as long as the loop
runs for about 1 ms, so there cannot be too many distinct return values.
Consequently, the only way for perverse SMIs to produce more
monotonically decreasing calibration values is to introduce delays that
make the loop run longer than 1 ms, i.e., to eat most of the CPU time
for at least several seconds (or longer if you want unbounded time).
Even in the real world ;-), no laptop manufacturer is that insane.
Best regards,
Clemens
Clemens Ladisch wrote:
> Paul Gortmaker wrote:
>
>> Yasunori Goto wrote:
>>
>>> + /*
>>> + * Try to calibrate until return value becomes stable small value.
>>> + * If SMI interruption occurs in calibration loop, the return value
>>> + * will be big. This avoids its impact.
>>> + */
>>> + do {
>>> + tmp = __hpet_calibrate(hpetp);
>>> + if (ret <= tmp)
>>> + break;
>>> + ret = tmp;
>>> + } while (1);
>>>
>> For what it is worth, if a situation arose where continued calls to
>> hpet_calibrate() represented a monotonically decreasing function,
>> (perhaps some insane power management?) you could be stuck
>> in this function for an unbounded amount of time.
>>
>
> On my machine, the number of iterations of the loop in __hpet_calibarate
> (the value of i) is about 400, and since HPET reads/writes go to the
> southbridge, it cannot get much higher than about 1000 even on faster
> machines.
>
> The value of (m - start) is approximately constant, as long as the loop
> runs for about 1 ms, so there cannot be too many distinct return values.
>
> Consequently, the only way for perverse SMIs to produce more
> monotonically decreasing calibration values is to introduce delays that
> make the loop run longer than 1 ms, i.e., to eat most of the CPU time
> for at least several seconds (or longer if you want unbounded time).
> Even in the real world ;-), no laptop manufacturer is that insane.
>
Right - as I said, I didn't expect that it could ever happen, and your
analysis
above seems to help reinforce that, so we probably should be OK.
Thanks,
Paul.
>
> Best regards,
> Clemens
>
On Wed, 18 Mar 2009 11:47:04 +0900
Yasunori Goto <[email protected]> wrote:
> hpet_calibrate() has a possibility of miss-calibration due to SMI.
> If SMI interrupts in the while loop of calibration, then return value
> will be big. This change calibrates until stabilizing by the return
> value with a small value.
>
>
> Signed-off-by: Yasunori Goto <[email protected]>
>
>
> ---
> drivers/char/hpet.c | 21 ++++++++++++++++++++-
> 1 file changed, 20 insertions(+), 1 deletion(-)
>
> Index: hpet_test/drivers/char/hpet.c
> ===================================================================
> --- hpet_test.orig/drivers/char/hpet.c 2009-03-12 15:47:45.000000000 +0900
> +++ hpet_test/drivers/char/hpet.c 2009-03-18 11:12:42.000000000 +0900
> @@ -713,7 +713,7 @@
> */
> #define TICK_CALIBRATE (1000UL)
>
> -static unsigned long hpet_calibrate(struct hpets *hpetp)
> +static unsigned long __hpet_calibrate(struct hpets *hpetp)
> {
> struct hpet_timer __iomem *timer = NULL;
> unsigned long t, m, count, i, flags, start;
> @@ -750,6 +750,25 @@
> return (m - start) / i;
> }
>
> +static unsigned long hpet_calibrate(struct hpets *hpetp)
> +{
> + unsigned long ret = ~0UL, tmp;
> +
> + /*
> + * Try to calibrate until return value becomes stable small value.
> + * If SMI interruption occurs in calibration loop, the return value
> + * will be big. This avoids its impact.
> + */
> + do {
> + tmp = __hpet_calibrate(hpetp);
> + if (ret <= tmp)
> + break;
> + ret = tmp;
> + } while (1);
> +
> + return ret;
> +}
Call me paranoid, but I'd like to see a maximum retry count here and an
error message-and-continue if it is exceeded. To prevent mysterious
boot-time lockups from misbehaving hpets, perhaps?
Also, style nit - I find
for ( ; ; ) {
...
}
to be more readable than
do {
...
} while (1);
and I believe the former is more common.
And
unsigned long ret = -1;
has the same effect as
unsigned long ret = ~0UL;
but is more maintainable - it doesn't subtly break if someone changes
the type of `ret'. (This is a bit of an ugly C trick).
[Re: [Patch] Fix the possibility of insane return value of hpet_calibrate() against SMI. (take 2)] On 30/03/2009 (Mon 14:06) Andrew Morton wrote:
> On Wed, 18 Mar 2009 11:47:04 +0900
> Yasunori Goto <[email protected]> wrote:
>
> > hpet_calibrate() has a possibility of miss-calibration due to SMI.
> > If SMI interrupts in the while loop of calibration, then return value
> > will be big. This change calibrates until stabilizing by the return
> > value with a small value.
> >
> >
> > Signed-off-by: Yasunori Goto <[email protected]>
> >
> >
> > ---
> > drivers/char/hpet.c | 21 ++++++++++++++++++++-
> > 1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > Index: hpet_test/drivers/char/hpet.c
> > ===================================================================
> > --- hpet_test.orig/drivers/char/hpet.c 2009-03-12 15:47:45.000000000 +0900
> > +++ hpet_test/drivers/char/hpet.c 2009-03-18 11:12:42.000000000 +0900
> > @@ -713,7 +713,7 @@
> > */
> > #define TICK_CALIBRATE (1000UL)
> >
> > -static unsigned long hpet_calibrate(struct hpets *hpetp)
> > +static unsigned long __hpet_calibrate(struct hpets *hpetp)
> > {
> > struct hpet_timer __iomem *timer = NULL;
> > unsigned long t, m, count, i, flags, start;
> > @@ -750,6 +750,25 @@
> > return (m - start) / i;
> > }
> >
> > +static unsigned long hpet_calibrate(struct hpets *hpetp)
> > +{
> > + unsigned long ret = ~0UL, tmp;
> > +
> > + /*
> > + * Try to calibrate until return value becomes stable small value.
> > + * If SMI interruption occurs in calibration loop, the return value
> > + * will be big. This avoids its impact.
> > + */
> > + do {
> > + tmp = __hpet_calibrate(hpetp);
> > + if (ret <= tmp)
> > + break;
> > + ret = tmp;
> > + } while (1);
> > +
> > + return ret;
> > +}
>
> Call me paranoid, but I'd like to see a maximum retry count here and an
> error message-and-continue if it is exceeded. To prevent mysterious
> boot-time lockups from misbehaving hpets, perhaps?
I'd mentioned that here:
http://lkml.org/lkml/2009/3/18/180
but the general consensus was that it was impossible and I was just
being paranoid. :-)
Acked-by: Paul Gortmaker <[email protected]>
Paul.
>
> Also, style nit - I find
>
> for ( ; ; ) {
> ...
> }
>
> to be more readable than
>
> do {
> ...
> } while (1);
>
> and I believe the former is more common.
>
>
> And
>
> unsigned long ret = -1;
>
> has the same effect as
>
> unsigned long ret = ~0UL;
>
> but is more maintainable - it doesn't subtly break if someone changes
> the type of `ret'. (This is a bit of an ugly C trick).
>
>
>