[TRIVIAL] i6300esb: fix timer overflow

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(-)

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