Messages in this thread | | | Date | Mon, 30 Mar 2009 17:23:57 -0400 | From | Paul Gortmaker <> | Subject | Re: [Patch] Fix the possibility of insane return value of hpet_calibrate() against SMI. (take 2) |
| |
[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 <y-goto@jp.fujitsu.com> 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 <y-goto@jp.fujitsu.com> > > > > > > --- > > 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 <paul.gortmaker@windriver.com>
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). > > >
| |