On 7/7/20 5:02 PM, 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é <f4...@amsat.org>
>>> wrote:
>>>>
>>>> From: Yoshinori Sato <ys...@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, can you send a patch to fix the potential bug?
>
>>>
>>> thanks
>>> -- PMM
>>>
>>
>
