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

[Stafford Horne]

On Wed, Nov 04, 2020 at 10:37:17AM +0000, Peter Maydell wrote:
> 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.

If that's the case, then definitely remove it.  Sorry, I was just going off the
patch and didn't look into the code.

> > 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.)

Yeah, that is want I would want to do.  As I couldn't remember if there was a
mask variable available I just came up with the shift alternative.  Sorry, I was
in a bit of a hurry.

As for the patch.

Acked-by: Stafford Horne <shorne@gmail.com>