Message ID | 1438676851-10684-1-git-send-email-lvivier@redhat.com |
---|---|
State | New |
Headers | show |
On Tue, Aug 04, 2015 at 10:27:31AM +0200, Laurent Vivier wrote: > --- a/hw/watchdog/wdt_i6300esb.c > +++ b/hw/watchdog/wdt_i6300esb.c > @@ -136,7 +136,7 @@ static void i6300esb_restart_timer(I6300State *d, int stage) > * multiply here can exceed 64-bits, before we divide by 33MHz, so > * we use a higher-precision intermediate result. > */ > - timeout = muldiv64(get_ticks_per_sec(), timeout, 33000000); > + timeout = muldiv64(timeout, get_ticks_per_sec(), 33000000); > > i6300esb_debug("stage %d, timeout %" PRIi64 "\n", d->stage, timeout); Here are the test results for this (v2) patch: Requested timeout Observed timeout 60 58 120 120 250 253 270 274 500 507 520 529 1010 1026 1030 1045 2046 2078 2500 ioctl: WDIOC_SETTIMEOUT: error setting timeout: Invalid argument Rich.
On Tue, Aug 04, 2015 at 10:27:31AM +0200, Laurent Vivier wrote: > We use muldiv64() to compute the time to wait: > > timeout = muldiv64(get_ticks_per_sec(), timeout, 33000000); > > but get_ticks_per_sec() is 10^9 (30 bit value) and timeout > is a 35 bit value. > > Whereas muldiv64 is: > > uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c) > > So we loose 3 bits of timeout. > > Swapping get_ticks_per_sec() and timeout fixes it. > > We can also replace it by a multiplication by 30 ns, > but this changes PCI clock frequency from 33MHz to 33.333333MHz > and we need to do this on all the QEMU PCI devices (later...) > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> Hah. 32-bit second argument. Totally missed that when I put the muldiv64() in there. So I didn't eliminate the overflow, just pushed it out some bits. Thanks for finding this. Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
04.08.2015 11:27, Laurent Vivier wrote: > We use muldiv64() to compute the time to wait: > > timeout = muldiv64(get_ticks_per_sec(), timeout, 33000000); ... Applied to -trivial, thank you! /mjt
On 06/09/2015 12:29, Michael Tokarev wrote: > 04.08.2015 11:27, Laurent Vivier wrote: >> We use muldiv64() to compute the time to wait: >> >> timeout = muldiv64(get_ticks_per_sec(), timeout, 33000000); > ... > > Applied to -trivial, thank you! I think this was superseded by a later patch. Paolo
Le 06/09/2015 16:35, Paolo Bonzini a écrit : > > > On 06/09/2015 12:29, Michael Tokarev wrote: >> 04.08.2015 11:27, Laurent Vivier wrote: >>> We use muldiv64() to compute the time to wait: >>> >>> timeout = muldiv64(get_ticks_per_sec(), timeout, 33000000); >> ... >> >> Applied to -trivial, thank you! > > I think this was superseded by a later patch. Yes, but as the later has not already be taken, I prefer this one taken and I'll update the other. Laurent
diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c index cfa2b1b..3e07d44 100644 --- a/hw/watchdog/wdt_i6300esb.c +++ b/hw/watchdog/wdt_i6300esb.c @@ -136,7 +136,7 @@ static void i6300esb_restart_timer(I6300State *d, int stage) * multiply here can exceed 64-bits, before we divide by 33MHz, so * we use a higher-precision intermediate result. */ - timeout = muldiv64(get_ticks_per_sec(), timeout, 33000000); + timeout = muldiv64(timeout, get_ticks_per_sec(), 33000000); i6300esb_debug("stage %d, timeout %" PRIi64 "\n", d->stage, timeout);
We use muldiv64() to compute the time to wait: timeout = muldiv64(get_ticks_per_sec(), timeout, 33000000); but get_ticks_per_sec() is 10^9 (30 bit value) and timeout is a 35 bit value. Whereas muldiv64 is: uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c) So we loose 3 bits of timeout. Swapping get_ticks_per_sec() and timeout fixes it. We can also replace it by a multiplication by 30 ns, but this changes PCI clock frequency from 33MHz to 33.333333MHz and we need to do this on all the QEMU PCI devices (later...) Signed-off-by: Laurent Vivier <lvivier@redhat.com> --- hw/watchdog/wdt_i6300esb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)