Re: [PATCH qemu.git v3 8/8] hw/timer/imx_epit: fix compare timer handling

From: Peter Maydell <peter.maydell@linaro.org>
To: "~axelheider" <axelheider@gmx.de>
Cc: qemu-devel@nongnu.org, qemu-arm@nongnu.org
Subject: Re: [PATCH qemu.git v3 8/8] hw/timer/imx_epit: fix compare timer handling
Date: Tue, 4 Apr 2023 16:44:45 +0100	[thread overview]
Message-ID: <CAFEAcA9JLOhxa3LeiFJ8YqinuSV33N4Vbj2m4ZnoOxry0=YGGA@mail.gmail.com> (raw)
In-Reply-To: <166990932074.29941.8709118178538288040-8@git.sr.ht>

On Thu, 1 Dec 2022 at 15:42, ~axelheider <axelheider@git.sr.ht> wrote:
>
> From: Axel Heider <axel.heider@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.heider@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