On Wed, Nov 04, 2020 at 10:37:17AM +0000, Peter Maydell wrote: > On Wed, 4 Nov 2020 at 07:10, Stafford Horne <sho...@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 <sho...@gmail.com>