[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL 07/15] hw/timer: RX62N 8-Bit timer (TMR)
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PULL 07/15] hw/timer: RX62N 8-Bit timer (TMR) |
Date: |
Mon, 29 Jun 2020 11:58:56 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 |
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?
>
> thanks
> -- PMM
>
- [PULL 01/15] MAINTAINERS: Cover sh_intc files in the R2D/Shix machine sections, (continued)
- [PULL 01/15] MAINTAINERS: Cover sh_intc files in the R2D/Shix machine sections, Philippe Mathieu-Daudé, 2020/06/21
- [PULL 02/15] MAINTAINERS: Add an entry for common Renesas peripherals, Philippe Mathieu-Daudé, 2020/06/21
- [PULL 03/15] hw/sh4: Use MemoryRegion typedef, Philippe Mathieu-Daudé, 2020/06/21
- [PULL 05/15] hw/timer/sh_timer: Remove unused 'qemu/timer.h' include, Philippe Mathieu-Daudé, 2020/06/21
- [PULL 04/15] hw/sh4: Extract timer definitions to 'hw/timer/tmu012.h', Philippe Mathieu-Daudé, 2020/06/21
- [PULL 06/15] hw/intc: RX62N interrupt controller (ICUa), Philippe Mathieu-Daudé, 2020/06/21
- [PULL 07/15] hw/timer: RX62N 8-Bit timer (TMR), Philippe Mathieu-Daudé, 2020/06/21
- [PULL 09/15] hw/char: RX62N serial communication interface (SCI), Philippe Mathieu-Daudé, 2020/06/21
- [PULL 08/15] hw/timer: RX62N compare match timer (CMT), Philippe Mathieu-Daudé, 2020/06/21
- [PULL 11/15] hw/rx: Honor -accel qtest, Philippe Mathieu-Daudé, 2020/06/21
- [PULL 10/15] hw/rx: RX62N microcontroller (MCU), Philippe Mathieu-Daudé, 2020/06/21
- [PULL 13/15] hw/rx: Add RX GDB simulator, Philippe Mathieu-Daudé, 2020/06/21
- [PULL 14/15] BootLinuxConsoleTest: Test the RX GDB simulator, Philippe Mathieu-Daudé, 2020/06/21
- [PULL 15/15] docs: Document the RX target, Philippe Mathieu-Daudé, 2020/06/21
- [PULL 12/15] hw/rx: Register R5F562N7 and R5F562N8 MCUs, Philippe Mathieu-Daudé, 2020/06/21
- Re: [PULL 00/15] Renesas hardware patches for 2020-06-21, Peter Maydell, 2020/06/22