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

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

2020-07-08 00:06 に Thomas Huth さんは書きました:
> On 07/07/2020 17.02, Yoshinori Sato wrote:
>> 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
> 
> So could you please provide a patch that either adds the missing
> "break;" statement between the cases here, or adds a "/* fallthrough 
> */"
> comment between the cases?
> 
>  Thanks,
>   Thomas

OK.
This part will be cleaned up more.

Thanks.