On Thu, 14 Mar 2019 at 08:43, Cédric Le Goater <[email protected]> wrote: > > From: Christian Svensson <[email protected]> > > If the host decrements the counter register that results in a negative > delta. This is then passed to muldiv64 which only handles unsigned > numbers resulting in bogus results. > > This fix ensures the data being operated on is signed before it is > ultimately casted to the final unsigned value. > > Test case: kexec a kernel using aspeed_timer and it will freeze on the > second bootup when the kernel initializes the timer. With this patch > that no longer happens and the timer appears to run OK. > > Signed-off-by: Christian Svensson <[email protected]> > [clg: - checkpatch fixes ] > Signed-off-by: Cédric Le Goater <[email protected]> > --- > hw/timer/aspeed_timer.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c > index 9988b8fbbf17..0b16eac8970c 100644 > --- a/hw/timer/aspeed_timer.c > +++ b/hw/timer/aspeed_timer.c > @@ -275,7 +275,8 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState > *s, int timer, int reg, > int64_t delta = (int64_t) value - (int64_t) calculate_ticks(t, > now); > uint32_t rate = calculate_rate(t); > > - t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate); > + t->start = (int64_t)t->start + > + ((__int128_t)delta * NANOSECONDS_PER_SECOND / rate); > aspeed_timer_mod(t);
You can't guarantee that __int128_t exists. If you must use it then you need an alternate code path for the ifndef CONFIG_INT128 case (which is going to be complicated enough that this should really be abstracted away into a signed version of muldiv64().) But overall I'm a little sceptical that the aspeed timer is really a special case that needs a signed version of this when no other timer in the system does... thanks -- PMM
