On Thu, 1 Dec 2022 at 15:42, ~axelheider <axelhei...@git.sr.ht> wrote:
>
> From: Axel Heider <axel.hei...@hensoldt.net>
>
> - fix #1263 for CR writes
> - rework compare time handling
> - The compare timer has to run even if CR.OCIEN is not set,
> as SR.OCIF must be updated.
> - The compare timer fires exactly once when the
> compare value is less than the current value, but the
> reload values is less than the compare value.
> - The compare timer will never fire if the reload value is
> less than the compare value. Disable it in this case.
>
> Signed-off-by: Axel Heider <axel.hei...@hensoldt.net>
Hi; Coverity has just noticed an issue with this patch:
> -/* Must be called from ptimer_transaction_begin/commit block for
> s->timer_cmp */
> -static void imx_epit_reload_compare_timer(IMXEPITState *s)
> +/*
> + * Must be called from a ptimer_transaction_begin/commit block for
> + * s->timer_cmp, but outside of a transaction block of s->timer_reload,
> + * so the proper counter value is read.
> + */
> +static void imx_epit_update_compare_timer(IMXEPITState *s)
> {
> - if ((s->cr & (CR_EN | CR_OCIEN)) == (CR_EN | CR_OCIEN)) {
> - /* if the compare feature is on and timers are running */
> - uint32_t tmp = ptimer_get_count(s->timer_reload);
> - uint64_t next;
> - if (tmp > s->cmp) {
> - /* It'll fire in this round of the timer */
> - next = tmp - s->cmp;
> - } else { /* catch it next time around */
> - next = tmp - s->cmp + ((s->cr & CR_RLD) ? EPIT_TIMER_MAX :
> s->lr);
> + uint64_t counter = 0;
> + bool is_oneshot = false;
Here we declare the is_oneshot variable...
> + /* The compare timer only has to run if the timer peripheral is active
> + * and there is an input clock, Otherwise it can be switched off.
> + */
> + bool is_active = (s->cr & CR_EN) && imx_epit_get_freq(s);
> + if (is_active) {
> + /*
> + * Calculate next timeout for compare timer. Reading the reload
> + * counter returns proper results only if pending transactions
> + * on it are committed here. Otherwise stale values are be read.
> + */
> + counter = ptimer_get_count(s->timer_reload);
> + uint64_t limit = ptimer_get_limit(s->timer_cmp);
> + /*
> + * The compare timer is a periodic timer if the limit is at least
> + * the compare value. Otherwise it may fire at most once in the
> + * current round.
> + */
> + bool is_oneshot = (limit >= s->cmp);
...but here we declare another is_oneshot, which shadows the first
declaration...
> + if (counter >= s->cmp) {
> + /* The compare timer fires in the current round. */
> + counter -= s->cmp;
> + } else if (!is_oneshot) {
> + /*
> + * The compare timer fires after a reload, as it below the
> + * compare value already in this round. Note that the counter
> + * value calculated below can be above the 32-bit limit, which
> + * is legal here because the compare timer is an internal
> + * helper ptimer only.
> + */
> + counter += limit - s->cmp;
> + } else {
> + /*
> + * The compare timer won't fire in this round, and the limit is
> + * set to a value below the compare value. This practically means
> + * it will never fire, so it can be switched off.
> + */
> + is_active = false;
> }
> - ptimer_set_count(s->timer_cmp, next);
> }
> +
> + /*
> + * Set the compare timer and let it run, or stop it. This is agnostic
> + * of CR.OCIEN bit, as this bit affects interrupt generation only. The
> + * compare timer needs to run even if no interrupts are to be generated,
> + * because the SR.OCIF bit must be updated also.
> + * Note that the timer might already be stopped or be running with
> + * counter values. However, finding out when an update is needed and
> + * when not is not trivial. It's much easier applying the setting again,
> + * as this does not harm either and the overhead is negligible.
> + */
> + if (is_active) {
> + ptimer_set_count(s->timer_cmp, counter);
> + ptimer_run(s->timer_cmp, is_oneshot ? 1 : 0);
...so here when the inner variable is no longer in scope, the
value of the outer is_oneshot variable must always be 'false',
because there's never any assignment to it.
> + } else {
> + ptimer_stop(s->timer_cmp);
> + }
> +
> }
What was the intention here? My guess is that there should only
have been one 'is_oneshot', not two.
There's also been this bug report:
https://gitlab.com/qemu-project/qemu/-/issues/1491
which suggests that the condition for setting is_oneshot
should be "(limit <= s->cmp)" rather than ">=".
What do you think ?
thanks
-- PMM
