When timer_calc_usec() is used with large timeout falues, such as 60s,
the function lacks precision and produces different results than when
using timer_calc(time / 1000) for the same timeout. This patch fixes
the precision issue by falling back to timer_calc(time / 1000) for
usec values over one second.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
src/hw/timer.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/hw/timer.c b/src/hw/timer.c
index 03d22b2..2f0c864 100644
--- a/src/hw/timer.c
+++ b/src/hw/timer.c
@@ -213,6 +213,8 @@ timer_calc(u32 msecs)
u32
timer_calc_usec(u32 usecs)
{
+ if (usecs > 1000000)
+ return timer_calc(usecs / 1000);
return timer_read() + DIV_ROUND_UP(GET_GLOBAL(TimerKHz) * usecs, 1000);
}
--
2.5.5
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
On 11/01/2017 10:40 AM, Stefan Berger wrote:
> When timer_calc_usec() is used with large timeout falues, such as 60s,
obviously a typo ->values.
The problem occurred with the TPM driver where we are given usec values
by the hardware and specs and want to call the timer_calc_usec()
function. That function works well for small values but fails once the
values become larger.
Stefan
> the function lacks precision and produces different results than when
> using timer_calc(time / 1000) for the same timeout. This patch fixes
> the precision issue by falling back to timer_calc(time / 1000) for
> usec values over one second.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
> src/hw/timer.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/src/hw/timer.c b/src/hw/timer.c
> index 03d22b2..2f0c864 100644
> --- a/src/hw/timer.c
> +++ b/src/hw/timer.c
> @@ -213,6 +213,8 @@ timer_calc(u32 msecs)
> u32
> timer_calc_usec(u32 usecs)
> {
> + if (usecs > 1000000)
> + return timer_calc(usecs / 1000);
> return timer_read() + DIV_ROUND_UP(GET_GLOBAL(TimerKHz) * usecs, 1000);
> }
>
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
On Wed, Nov 01, 2017 at 10:40:57AM -0400, Stefan Berger wrote:
> When timer_calc_usec() is used with large timeout falues, such as 60s,
> the function lacks precision and produces different results than when
> using timer_calc(time / 1000) for the same timeout. This patch fixes
> the precision issue by falling back to timer_calc(time / 1000) for
> usec values over one second.
Makes sense. The code would need to use DIV_ROUND_UP though (it's
okay to go slightly longer, but it must not calculate a shorter time).
Also, I think the check should be extended to
nsleep/usleep/ndelay/udelay as well - see the patch below.
Is this needed for the current code, or is this only an issue for the
future TPM CRB patches? I was planning to make a release this week -
I'd prefer to add this to the next release unless it fixes an active
bug.
-Kevin
--- a/src/hw/timer.c
+++ b/src/hw/timer.c
@@ -167,53 +167,60 @@ timer_check(u32 end)
}
static void
-timer_delay(u32 diff)
+timer_delay(u32 end)
{
- u32 start = timer_read();
- u32 end = start + diff;
while (!timer_check(end))
cpu_relax();
}
static void
-timer_sleep(u32 diff)
+timer_sleep(u32 end)
{
- u32 start = timer_read();
- u32 end = start + diff;
while (!timer_check(end))
yield();
}
+// Return the TSC value that is 'msecs' time in the future.
+u32
+timer_calc(u32 msecs)
+{
+ return timer_read() + (GET_GLOBAL(TimerKHz) * msecs);
+}
+u32
+timer_calc_usec(u32 usecs)
+{
+ u32 cur = timer_read(), khz = GET_GLOBAL(TimerKHz);
+ if (usecs > 500000)
+ return cur + DIV_ROUND_UP(usecs, 1000) * khz;
+ return cur + DIV_ROUND_UP(usecs * khz, 1000);
+}
+static u32
+timer_calc_nsec(u32 nsecs)
+{
+ u32 cur = timer_read(), khz = GET_GLOBAL(TimerKHz);
+ if (nsecs > 500000)
+ return cur + DIV_ROUND_UP(nsecs, 1000000) * khz;
+ return cur + DIV_ROUND_UP(nsecs * khz, 1000000);
+}
+
void ndelay(u32 count) {
- timer_delay(DIV_ROUND_UP(count * GET_GLOBAL(TimerKHz), 1000000));
+ timer_delay(timer_calc_nsec(count));
}
void udelay(u32 count) {
- timer_delay(DIV_ROUND_UP(count * GET_GLOBAL(TimerKHz), 1000));
+ timer_delay(timer_calc_usec(count));
}
void mdelay(u32 count) {
- timer_delay(count * GET_GLOBAL(TimerKHz));
+ timer_delay(timer_calc(count));
}
void nsleep(u32 count) {
- timer_sleep(DIV_ROUND_UP(count * GET_GLOBAL(TimerKHz), 1000000));
+ timer_sleep(timer_calc_nsec(count));
}
void usleep(u32 count) {
- timer_sleep(DIV_ROUND_UP(count * GET_GLOBAL(TimerKHz), 1000));
+ timer_sleep(timer_calc_usec(count));
}
void msleep(u32 count) {
- timer_sleep(count * GET_GLOBAL(TimerKHz));
-}
-
-// Return the TSC value that is 'msecs' time in the future.
-u32
-timer_calc(u32 msecs)
-{
- return timer_read() + (GET_GLOBAL(TimerKHz) * msecs);
-}
-u32
-timer_calc_usec(u32 usecs)
-{
- return timer_read() + DIV_ROUND_UP(GET_GLOBAL(TimerKHz) * usecs, 1000);
+ timer_sleep(timer_calc(count));
}
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
On 11/01/2017 11:51 AM, Kevin O'Connor wrote:
> On Wed, Nov 01, 2017 at 10:40:57AM -0400, Stefan Berger wrote:
>> When timer_calc_usec() is used with large timeout falues, such as 60s,
>> the function lacks precision and produces different results than when
>> using timer_calc(time / 1000) for the same timeout. This patch fixes
>> the precision issue by falling back to timer_calc(time / 1000) for
>> usec values over one second.
> Makes sense. The code would need to use DIV_ROUND_UP though (it's
> okay to go slightly longer, but it must not calculate a shorter time).
> Also, I think the check should be extended to
> nsleep/usleep/ndelay/udelay as well - see the patch below.
>
> Is this needed for the current code, or is this only an issue for the
> future TPM CRB patches? I was planning to make a release this week -
I do see wrong calculations of course when using the usec function, but
I am not running into actual timeouts (because the emulated device is
fast). I think it also depends on the values of the timeouts and what
bit patterns the values have and then what bits get lost during the
calculation that it may or may not matter. For the general case I would
say we have an active bug that, depending on timeout values, may cause
unwanted behavior.
With your patch applied the values returned from the usec function is
correct.
Stefan
> I'd prefer to add this to the next release unless it fixes an active
> bug.
>
> -Kevin
>
>
> --- a/src/hw/timer.c
> +++ b/src/hw/timer.c
> @@ -167,53 +167,60 @@ timer_check(u32 end)
> }
>
> static void
> -timer_delay(u32 diff)
> +timer_delay(u32 end)
> {
> - u32 start = timer_read();
> - u32 end = start + diff;
> while (!timer_check(end))
> cpu_relax();
> }
>
> static void
> -timer_sleep(u32 diff)
> +timer_sleep(u32 end)
> {
> - u32 start = timer_read();
> - u32 end = start + diff;
> while (!timer_check(end))
> yield();
> }
>
> +// Return the TSC value that is 'msecs' time in the future.
> +u32
> +timer_calc(u32 msecs)
> +{
> + return timer_read() + (GET_GLOBAL(TimerKHz) * msecs);
> +}
> +u32
> +timer_calc_usec(u32 usecs)
> +{
> + u32 cur = timer_read(), khz = GET_GLOBAL(TimerKHz);
> + if (usecs > 500000)
> + return cur + DIV_ROUND_UP(usecs, 1000) * khz;
> + return cur + DIV_ROUND_UP(usecs * khz, 1000);
> +}
> +static u32
> +timer_calc_nsec(u32 nsecs)
> +{
> + u32 cur = timer_read(), khz = GET_GLOBAL(TimerKHz);
> + if (nsecs > 500000)
> + return cur + DIV_ROUND_UP(nsecs, 1000000) * khz;
> + return cur + DIV_ROUND_UP(nsecs * khz, 1000000);
> +}
> +
> void ndelay(u32 count) {
> - timer_delay(DIV_ROUND_UP(count * GET_GLOBAL(TimerKHz), 1000000));
> + timer_delay(timer_calc_nsec(count));
> }
> void udelay(u32 count) {
> - timer_delay(DIV_ROUND_UP(count * GET_GLOBAL(TimerKHz), 1000));
> + timer_delay(timer_calc_usec(count));
> }
> void mdelay(u32 count) {
> - timer_delay(count * GET_GLOBAL(TimerKHz));
> + timer_delay(timer_calc(count));
> }
>
> void nsleep(u32 count) {
> - timer_sleep(DIV_ROUND_UP(count * GET_GLOBAL(TimerKHz), 1000000));
> + timer_sleep(timer_calc_nsec(count));
> }
> void usleep(u32 count) {
> - timer_sleep(DIV_ROUND_UP(count * GET_GLOBAL(TimerKHz), 1000));
> + timer_sleep(timer_calc_usec(count));
> }
> void msleep(u32 count) {
> - timer_sleep(count * GET_GLOBAL(TimerKHz));
> -}
> -
> -// Return the TSC value that is 'msecs' time in the future.
> -u32
> -timer_calc(u32 msecs)
> -{
> - return timer_read() + (GET_GLOBAL(TimerKHz) * msecs);
> -}
> -u32
> -timer_calc_usec(u32 usecs)
> -{
> - return timer_read() + DIV_ROUND_UP(GET_GLOBAL(TimerKHz) * usecs, 1000);
> + timer_sleep(timer_calc(count));
> }
>
>
>
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
On Wed, Nov 01, 2017 at 12:37:25PM -0400, Stefan Berger wrote: > On 11/01/2017 11:51 AM, Kevin O'Connor wrote: > > On Wed, Nov 01, 2017 at 10:40:57AM -0400, Stefan Berger wrote: > > > When timer_calc_usec() is used with large timeout falues, such as 60s, > > > the function lacks precision and produces different results than when > > > using timer_calc(time / 1000) for the same timeout. This patch fixes > > > the precision issue by falling back to timer_calc(time / 1000) for > > > usec values over one second. > > Makes sense. The code would need to use DIV_ROUND_UP though (it's > > okay to go slightly longer, but it must not calculate a shorter time). > > Also, I think the check should be extended to > > nsleep/usleep/ndelay/udelay as well - see the patch below. > > > > Is this needed for the current code, or is this only an issue for the > > future TPM CRB patches? I was planning to make a release this week - > > I do see wrong calculations of course when using the usec function, but I am > not running into actual timeouts (because the emulated device is fast). I > think it also depends on the values of the timeouts and what bit patterns > the values have and then what bits get lost during the calculation that it > may or may not matter. For the general case I would say we have an active > bug that, depending on timeout values, may cause unwanted behavior. Ah, I missed that. I committed the patch. Unless anyone screams, I'm going to push back the release a few days as well. Thanks, -Kevin _______________________________________________ SeaBIOS mailing list SeaBIOS@seabios.org https://mail.coreboot.org/mailman/listinfo/seabios
© 2016 - 2024 Red Hat, Inc.