On Thu, 17 Oct 2019 at 16:22, Philippe Mathieu-Daudé <[email protected]> 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é <[email protected]> Thanks; I'll add the NULL-check cleanup in v2. Coverity will probably complain otherwise. thanks -- PMM
