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); }
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);
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)); }
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)); }
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
-
Kevin O'Connor -
Stefan Berger