Re: [PULL 07/15] hw/timer: RX62N 8-Bit timer (TMR)

[Yoshinori Sato <ysato@users.sourceforge.jp>]

On Mon, 29 Jun 2020 18:58:56 +0900,
Philippe Mathieu-Daudé wrote:
> 
> Hi Yoshinori,
> 
> On 6/25/20 11:25 AM, Peter Maydell wrote:
> > On Sun, 21 Jun 2020 at 13:54, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>
> >> From: Yoshinori Sato <ysato@users.sourceforge.jp>
> >>
> >> renesas_tmr: 8bit timer modules.
> > 
> > Hi; the recent Coverity run reports a potential bug in this
> > code: (CID 1429976)
> > 
> > 
> >> +static uint16_t read_tcnt(RTMRState *tmr, unsigned size, int ch)
> >> +{
> >> +    int64_t delta, now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> >> +    int elapsed, ovf = 0;
> >> +    uint16_t tcnt[2];
> > 
> > Here we declare tcnt[] but do not initialize its contents...
> > 
> >> +    uint32_t ret;
> >> +
> >> +    delta = (now - tmr->tick) * NANOSECONDS_PER_SECOND / tmr->input_freq;
> >> +    if (delta > 0) {
> >> +        tmr->tick = now;
> >> +
> >> +        if (FIELD_EX8(tmr->tccr[1], TCCR, CSS) == INTERNAL) {
> >> +            /* timer1 count update */
> >> +            elapsed = elapsed_time(tmr, 1, delta);
> >> +            if (elapsed >= 0x100) {
> >> +                ovf = elapsed >> 8;
> >> +            }
> >> +            tcnt[1] = tmr->tcnt[1] + (elapsed & 0xff);
> >> +        }
> >> +        switch (FIELD_EX8(tmr->tccr[0], TCCR, CSS)) {
> >> +        case INTERNAL:
> >> +            elapsed = elapsed_time(tmr, 0, delta);
> >> +            tcnt[0] = tmr->tcnt[0] + elapsed;
> >> +            break;
> >> +        case CASCADING:
> >> +            if (ovf > 0) {
> >> +                tcnt[0] = tmr->tcnt[0] + ovf;
> >> +            }
> >> +            break;
> >> +        }
> > 
> > ...but not all cases here set both tcnt[0] and tcnt[1]
> > (for instance in the "case CASCADING:" if ovf <=0 we
> > won't set either of them)...
> > 
> >> +    } else {
> >> +        tcnt[0] = tmr->tcnt[0];
> >> +        tcnt[1] = tmr->tcnt[1];
> >> +    }
> >> +    if (size == 1) {
> >> +        return tcnt[ch];
> >> +    } else {
> >> +        ret = 0;
> >> +        ret = deposit32(ret, 0, 8, tcnt[1]);
> >> +        ret = deposit32(ret, 8, 8, tcnt[0]);
> >> +        return ret;
> > 
> > ...and so here we will end up returning uninitialized
> > data. Presumably the spec says what value is actually
> > supposed to be returned in these cases?
> > 
> > PS: the "else" branch with the deposit32() calls could be
> > rewritten more simply as
> >   return lduw_be_p(tcnt);
> > 
> >> +static uint64_t tmr_read(void *opaque, hwaddr addr, unsigned size)
> >> +{
> > 
> > In this function Coverity reports a missing "break" (CID 1429977):
> > 
> >> +    case A_TCORA:
> >> +        if (size == 1) {
> >> +            return tmr->tcora[ch];
> >> +        } else if (ch == 0) {
> >> +            return concat_reg(tmr->tcora);
> >> +        }
> > 
> > Here execution can fall through but there is no 'break' or '/* fallthrough */'.
> > 
> >> +    case A_TCORB:
> >> +        if (size == 1) {
> >> +            return tmr->tcorb[ch];
> >> +        } else {
> >> +            return concat_reg(tmr->tcorb);
> >> +        }
> > 
> > Is it correct that the A_TCORA and A_TCORB code is different?
> > It looks odd, so if this is intentional then a comment describing
> > why it is so might be helpful to readers.
> 
> Can you address Peter's comments please?

This register can 8bit and 16bit access.
8bit case return separate single TCORA or TCORB.
16bit case return merged two channel's TCORA or TCORB.
high byte: channel 0 register.
low byte: channel 1 register

> > 
> > thanks
> > -- PMM
> > 
> 

-- 
Yosinori Sato