Re: [PATCH 2/2] hw/timer/slavio_timer.c: Switch to transaction-based ptimer API

[Peter Maydell]

On Thu, 17 Oct 2019 at 16:22, Philippe Mathieu-Daudé <address@hidden> wrote:
> On 10/17/19 5:00 PM, Peter Maydell wrote:
> > ...because the commit should come after we have finished
> > updating the timer state (t->run in this case), because
> > the trigger callback slavio_timer_irq() otherwise sees
> > inconsistent half-updated state if commit() calls it.
>
> Oh, slavio_timer_irq() calls slavio_timer_get_out() which accesses the
> ptimer... OK I missed that.
>
> Whew we need to be extra cautious with this API...

Yes. If the callback function is a trivial "just update
the interrupt register bit and set an irq line" one, like
about half the ptimer users, then it doesn't matter too
much where the commit call goes, but for those users who
do more complicated work in the timer callback it gets
a little trickier (and I didn't realise this wrinkle until
about halfway through doing the API conversion work).
It doesn't much matter where the begin call goes (an odd
asymmetry), but the commit call is effectively a "voluntarily
yield control to the callback function" and so its placement
can be important.

> If possible I'd rather see the patch removing the NULL check previous to
> this one, anyway:
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

Thanks; I'll add the NULL-check cleanup in v2. Coverity will
probably complain otherwise.

thanks
-- PMM