Re: [PATCH v2 02/21] ptimer: Provide new transaction-based API

[Richard Henderson <richard.henderson@linaro.org>]

On 10/8/19 1:17 PM, Peter Maydell wrote:
> Provide the new transaction-based API. If a ptimer is created
> using ptimer_init() rather than ptimer_init_with_bh(), then
> instead of providing a QEMUBH, it provides a pointer to the
> callback function directly, and has opted into the transaction
> API. All calls to functions which modify ptimer state:
>  - ptimer_set_period()
>  - ptimer_set_freq()
>  - ptimer_set_limit()
>  - ptimer_set_count()
>  - ptimer_run()
>  - ptimer_stop()
> must be between matched calls to ptimer_transaction_begin()
> and ptimer_transaction_commit(). When ptimer_transaction_commit()
> is called it will evaluate the state of the timer after all the
> changes in the transaction, and call the callback if necessary.
> 
> In the old API the individual update functions generally would
> call ptimer_trigger() immediately, which would schedule the QEMUBH.
> In the new API the update functions will instead defer the
> "set s->next_event and call ptimer_reload()" work to
> ptimer_transaction_commit().
> 
> Because ptimer_trigger() can now immediately call into the
> device code which may then call other ptimer functions that
> update ptimer_state fields, we must be more careful in
> ptimer_reload() not to cache fields from ptimer_state across
> the ptimer_trigger() call. (This was harmless with the QEMUBH
> mechanism as the BH would not be invoked until much later.)
> 
> We use assertions to check that:
>  * the functions modifying ptimer state are not called outside
>    a transaction block
>  * ptimer_transaction_begin() and _commit() calls are paired
>  * the transaction API is not used with a QEMUBH ptimer
> 
> There is some slight repetition of code:
>  * most of the set functions have similar looking "if s->bh
>    call ptimer_reload, otherwise set s->need_reload" code
>  * ptimer_init() and ptimer_init_with_bh() have similar code
> We deliberately don't try to avoid this repetition, because
> it will all be deleted when the QEMUBH version of the API
> is removed.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~