Re: [PATCH] target/openrisc: Remove dead code attempting to check "is timer disabled"

[Peter Maydell]

On Wed, 4 Nov 2020 at 07:10, Stafford Horne <shorne@gmail.com> wrote:
>
> On Tue, Nov 03, 2020 at 11:46:54AM +0000, Peter Maydell wrote:
> > In the mtspr helper we attempt to check for "is the timer disabled"
> > with "if (env->ttmr & TIMER_NONE)".  This is wrong because TIMER_NONE
> > is zero and the condition is always false (Coverity complains about
> > the dead code.)
> >
> > The correct check would be to test whether the TTMR_M field in the
> > register is equal to TIMER_NONE instead.  However, the
> > cpu_openrisc_timer_update() function checks whether the timer is
> > enabled (it looks at cpu->env.is_counting, which is set to 0 via
> > cpu_openrisc_count_stop() when the TTMR_M field is set to
> > TIMER_NONE), so there's no need to check for "timer disabled" in the
> > target/openrisc code.  Instead, simply remove the dead code.
>
> Thanks for the patch!
>
> I think the check is needed, but it's coded wrong.  The ttmr bits 31,30
> are the timer mode.  If they are equal to zero (TIMER_NONE) then it means
> the timer is disabled and we can skip the timer update.

My analysis is in the commit message -- the timer_update() function
will just happily do nothing if the timer is disabled. It seemed
cleaner to me to let the timer function just do the right thing
rather than having both layers of the code try to figure out if
there's no action to take.

> The code should be something like ((env->ttmr >> 30) == TIMER_NONE). But I
> haven't tested it.

TIMER_NONE and the other TIMER_* fields are defined as (n << 30),
and the mask TTMR_M is (3 << 30), so "(env->ttmr & TTMR_M) == TIMER_NONE"
would be the condition to check if we wanted to do it here. (That
matches how the code earlier in the function does it.)

thanks
-- PMM