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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 10/17/19 5:00 PM, Peter Maydell wrote:
> On Thu, 17 Oct 2019 at 15:54, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> Hi Peter,
>>
>> On 10/17/19 3:23 PM, Peter Maydell wrote:
>>> Switch the slavio_timer code away from bottom-half based ptimers to
>>> the new transaction-based ptimer API.  This just requires adding
>>> begin/commit calls around the various places that modify the ptimer
>>> state, and using the new ptimer_init() function to create the timer.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>>    hw/timer/slavio_timer.c | 20 ++++++++++++++++----
>>>    1 file changed, 16 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/timer/slavio_timer.c b/hw/timer/slavio_timer.c
>>> index 692d213897d..0e2efe6fe89 100644
>>> --- a/hw/timer/slavio_timer.c
>>> +++ b/hw/timer/slavio_timer.c
>>> @@ -30,7 +30,6 @@
>>>    #include "hw/sysbus.h"
>>>    #include "migration/vmstate.h"
>>>    #include "trace.h"
>>> -#include "qemu/main-loop.h"
>>>    #include "qemu/module.h"
>>>
>>>    /*
>>> @@ -213,6 +212,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
>>>        saddr = addr >> 2;
>>>        switch (saddr) {
>>>        case TIMER_LIMIT:
>>> +        ptimer_transaction_begin(t->timer);
>>>            if (slavio_timer_is_user(tc)) {
>>>                uint64_t count;
>>
>>
>> This part is odd since there is a check on t->timer != NULL later, and
>> ptimer_transaction_commit() can't take NULL.
> 
> Hmm, I hadn't noticed that. I think the bug is the check
> for NULL, though, beacuse the slavio_timer_init() function
> always initializes all the timer pointers, so there's
> no situation where the pointer can be non-NULL as far
> as I can see. So I think I'd rather fix this by removing
> the NULL pointer check...

Yes, you are correct.

>>> @@ -255,13 +258,16 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
>>>        case TIMER_COUNTER_NORST:
>>>            // set limit without resetting counter
>>>            t->limit = val & TIMER_MAX_COUNT32;
>>> +        ptimer_transaction_begin(t->timer);
>>>            if (t->limit == 0) { /* free-run */
>>>                ptimer_set_limit(t->timer, LIMIT_TO_PERIODS(TIMER_MAX_COUNT32), 0);
>>>            } else {
>>>                ptimer_set_limit(t->timer, LIMIT_TO_PERIODS(t->limit), 0);
>>>            }
>>> +        ptimer_transaction_commit(t->timer);
>>>            break;
>>>        case TIMER_STATUS:
>>> +        ptimer_transaction_begin(t->timer);
>>>            if (slavio_timer_is_user(tc)) {
>>
>> I'd move the begin() here.
> 
> This would be awkward because then it won't neatly nest with
> the commit call unless you add an extra if() for the
> commit or otherwise rearrange/duplicate code...
> 
>>>                // start/stop user counter
>>>                if (val & 1) {
>>> @@ -273,6 +279,7 @@ static void slavio_timer_mem_writel(void *opaque, hwaddr addr,
>>>                }
>>>            }
>>>            t->run = val & 1;
>>> +        ptimer_transaction_commit(t->timer);
> 
> ...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...

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

Thanks,

Phil.