From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 49997ECAAA1 for ; Tue, 30 Aug 2022 09:14:03 +0000 (UTC) Received: from localhost ([::1]:47286 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oSxJq-0004gZ-3A for qemu-devel@archiver.kernel.org; Tue, 30 Aug 2022 05:14:02 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:51696) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oSxHn-0003S8-5i; Tue, 30 Aug 2022 05:11:55 -0400 Received: from mail-pj1-x1033.google.com ([2607:f8b0:4864:20::1033]:40746) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1oSxHj-0001Xt-H1; Tue, 30 Aug 2022 05:11:54 -0400 Received: by mail-pj1-x1033.google.com with SMTP id t11-20020a17090a510b00b001fac77e9d1fso17252605pjh.5; Tue, 30 Aug 2022 02:11:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc; bh=RV3ABKK0Mwvfu2F96sk7XvRaaGOtohlWvfGgvLEd2kU=; b=FSoorEwaldSSaffBbrh1GxhZNUSTGwZ4Cv2e5d6mLlqos8yvk35ZcEdXamj/EglvBd MHzxJyZFFRp9LALyi7s3syt3wZswu7+tI2RE3Se3YGxm5jHwzOxdj24ollLkGS/a99ne H0AVzNgHdHqEmXc1oZRDKsJs2f7XvWql8uMhkx7IhnXIJVQWES/wGIv9jk7M+wC6jZxS GbNJTK6H9+sKMZuR4Exj1SZLF9gozfCrkBGqn1gLkvbTv9Oa9eEl11IRXwamYs/HCivS HEJmhlsLgxOYvc5JczzcD1BKPX8qaskW2zfVhFAJP5Ir/AjbdlL6LQSXZwJtLosFCB5Y wfmQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc; bh=RV3ABKK0Mwvfu2F96sk7XvRaaGOtohlWvfGgvLEd2kU=; b=DgBIVh/y9T4vIHUjVdhi9EcjBm5UnIUva3e2Tda2+35PWfqHaLZKRLCB7dqjmKL6qL fjTVm9CFkM4OiFOJUQ/rNrN5+VoFLG5EJaczhHaVg9h77YfmUJ0H+1cuQsrKa6L97doK u6Ff9idqTxgPYcc7ET2PuHOroYpXcVCb8P5VVcoQmpfi5SAgnpfb9SKmfc96Ybvq93f4 c7o5atw6RuE9QWxKAJjbvE6N8rIztNH2XtAzORgbFCjnZzdRNKfmXR+pBSlmrtDNsdLa 97ga25estBMUbLE1QXsHE7letUqs0djtwMEX9iIQMKaD8OZ9sUriThkh9lZ2TSL0u+b1 zcNA== X-Gm-Message-State: ACgBeo2GE1G55DQJNMkf+2+68ql1m6eesL8kuXpoEW5fZ5FeJYVeCCbM BrZbCBKmuZhnOJ7OJD/FtLWC/rmL/Ict35uVBwA= X-Google-Smtp-Source: AA6agR7a2IHbalJoY6nmxKNrtejB1UfnGDeWPxGM/AmE1VgnfYrex8imNbOxp8kJhVjxtj8AlPdsH4Vlng/VdfZjiyg= X-Received: by 2002:a17:90a:474b:b0:1fd:62c3:62ef with SMTP id y11-20020a17090a474b00b001fd62c362efmr21173821pjg.8.1661850709564; Tue, 30 Aug 2022 02:11:49 -0700 (PDT) MIME-Version: 1.0 References: <20220823061201.132342-1-wilfred.mallawa@opensource.wdc.com> <20220823061201.132342-3-wilfred.mallawa@opensource.wdc.com> In-Reply-To: <20220823061201.132342-3-wilfred.mallawa@opensource.wdc.com> From: Alistair Francis Date: Tue, 30 Aug 2022 11:11:22 +0200 Message-ID: Subject: Re: [PATCH v4 2/4] hw/ssi: ibex_spi: fixup coverity issue To: Wilfred Mallawa Cc: Alistair Francis , "open list:RISC-V" , "qemu-devel@nongnu.org Developers" , Wilfred Mallawa , Andrew Jones Content-Type: text/plain; charset="UTF-8" Received-SPF: pass client-ip=2607:f8b0:4864:20::1033; envelope-from=alistair23@gmail.com; helo=mail-pj1-x1033.google.com X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_ENVFROM_END_DIGIT=0.25, FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" On Tue, Aug 23, 2022 at 8:13 AM Wilfred Mallawa wrote: > > From: Wilfred Mallawa > > This patch addresses the coverity issues specified in [1], > as suggested, `FIELD_DP32()`/`FIELD_EX32()` macros have been > implemented to clean up the code. > > [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg887713.html > > Fixes: Coverity CID 1488107 > > Signed-off-by: Wilfred Mallawa > Reviewed-by: Alistair Francis I didn't review the previous version, please don't add tags unless they are explicitly stated > Reviewed-by: Andrew Jones For this patch though: Reviewed-by: Alistair Francis Alistair > --- > hw/ssi/ibex_spi_host.c | 132 +++++++++++++++++++++-------------------- > 1 file changed, 68 insertions(+), 64 deletions(-) > > diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c > index 601041d719..d52b193a1a 100644 > --- a/hw/ssi/ibex_spi_host.c > +++ b/hw/ssi/ibex_spi_host.c > @@ -108,18 +108,22 @@ static inline uint8_t div4_round_up(uint8_t dividend) > > static void ibex_spi_rxfifo_reset(IbexSPIHostState *s) > { > + uint32_t data = s->regs[IBEX_SPI_HOST_STATUS]; > /* Empty the RX FIFO and assert RXEMPTY */ > fifo8_reset(&s->rx_fifo); > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXFULL_MASK; > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_RXEMPTY_MASK; > + data = FIELD_DP32(data, STATUS, RXFULL, 0); > + data = FIELD_DP32(data, STATUS, RXEMPTY, 1); > + s->regs[IBEX_SPI_HOST_STATUS] = data; > } > > static void ibex_spi_txfifo_reset(IbexSPIHostState *s) > { > + uint32_t data = s->regs[IBEX_SPI_HOST_STATUS]; > /* Empty the TX FIFO and assert TXEMPTY */ > fifo8_reset(&s->tx_fifo); > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXFULL_MASK; > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_TXEMPTY_MASK; > + data = FIELD_DP32(data, STATUS, TXFULL, 0); > + data = FIELD_DP32(data, STATUS, TXEMPTY, 1); > + s->regs[IBEX_SPI_HOST_STATUS] = data; > } > > static void ibex_spi_host_reset(DeviceState *dev) > @@ -162,37 +166,38 @@ static void ibex_spi_host_reset(DeviceState *dev) > */ > static void ibex_spi_host_irq(IbexSPIHostState *s) > { > - bool error_en = s->regs[IBEX_SPI_HOST_INTR_ENABLE] > - & R_INTR_ENABLE_ERROR_MASK; > - bool event_en = s->regs[IBEX_SPI_HOST_INTR_ENABLE] > - & R_INTR_ENABLE_SPI_EVENT_MASK; > - bool err_pending = s->regs[IBEX_SPI_HOST_INTR_STATE] > - & R_INTR_STATE_ERROR_MASK; > - bool status_pending = s->regs[IBEX_SPI_HOST_INTR_STATE] > - & R_INTR_STATE_SPI_EVENT_MASK; > + uint32_t intr_test_reg = s->regs[IBEX_SPI_HOST_INTR_TEST]; > + uint32_t intr_en_reg = s->regs[IBEX_SPI_HOST_INTR_ENABLE]; > + uint32_t intr_state_reg = s->regs[IBEX_SPI_HOST_INTR_STATE]; > + > + uint32_t err_en_reg = s->regs[IBEX_SPI_HOST_ERROR_ENABLE]; > + uint32_t event_en_reg = s->regs[IBEX_SPI_HOST_EVENT_ENABLE]; > + uint32_t err_status_reg = s->regs[IBEX_SPI_HOST_ERROR_STATUS]; > + uint32_t status_reg = s->regs[IBEX_SPI_HOST_STATUS]; > + > + > + bool error_en = FIELD_EX32(intr_en_reg, INTR_ENABLE, ERROR); > + bool event_en = FIELD_EX32(intr_en_reg, INTR_ENABLE, SPI_EVENT); > + bool err_pending = FIELD_EX32(intr_state_reg, INTR_STATE, ERROR); > + bool status_pending = FIELD_EX32(intr_state_reg, INTR_STATE, SPI_EVENT); > + > int err_irq = 0, event_irq = 0; > > /* Error IRQ enabled and Error IRQ Cleared */ > if (error_en && !err_pending) { > /* Event enabled, Interrupt Test Error */ > - if (s->regs[IBEX_SPI_HOST_INTR_TEST] & R_INTR_TEST_ERROR_MASK) { > + if (FIELD_EX32(intr_test_reg, INTR_TEST, ERROR)) { > err_irq = 1; > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE] > - & R_ERROR_ENABLE_CMDBUSY_MASK) && > - s->regs[IBEX_SPI_HOST_ERROR_STATUS] > - & R_ERROR_STATUS_CMDBUSY_MASK) { > + } else if (FIELD_EX32(err_en_reg, ERROR_ENABLE, CMDBUSY) && > + FIELD_EX32(err_status_reg, ERROR_STATUS, CMDBUSY)) { > /* Wrote to COMMAND when not READY */ > err_irq = 1; > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE] > - & R_ERROR_ENABLE_CMDINVAL_MASK) && > - s->regs[IBEX_SPI_HOST_ERROR_STATUS] > - & R_ERROR_STATUS_CMDINVAL_MASK) { > + } else if (FIELD_EX32(err_en_reg, ERROR_ENABLE, CMDINVAL) && > + FIELD_EX32(err_status_reg, ERROR_STATUS, CMDINVAL)) { > /* Invalid command segment */ > err_irq = 1; > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE] > - & R_ERROR_ENABLE_CSIDINVAL_MASK) && > - s->regs[IBEX_SPI_HOST_ERROR_STATUS] > - & R_ERROR_STATUS_CSIDINVAL_MASK) { > + } else if (FIELD_EX32(err_en_reg, ERROR_ENABLE, CSIDINVAL) && > + FIELD_EX32(err_status_reg, ERROR_STATUS, CSIDINVAL)) { > /* Invalid value for CSID */ > err_irq = 1; > } > @@ -204,22 +209,19 @@ static void ibex_spi_host_irq(IbexSPIHostState *s) > > /* Event IRQ Enabled and Event IRQ Cleared */ > if (event_en && !status_pending) { > - if (s->regs[IBEX_SPI_HOST_INTR_TEST] & R_INTR_TEST_SPI_EVENT_MASK) { > + if (FIELD_EX32(intr_test_reg, INTR_STATE, SPI_EVENT)) { > /* Event enabled, Interrupt Test Event */ > event_irq = 1; > - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE] > - & R_EVENT_ENABLE_READY_MASK) && > - (s->regs[IBEX_SPI_HOST_STATUS] & R_STATUS_READY_MASK)) { > + } else if (FIELD_EX32(event_en_reg, EVENT_ENABLE, READY) && > + FIELD_EX32(status_reg, STATUS, READY)) { > /* SPI Host ready for next command */ > event_irq = 1; > - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE] > - & R_EVENT_ENABLE_TXEMPTY_MASK) && > - (s->regs[IBEX_SPI_HOST_STATUS] & R_STATUS_TXEMPTY_MASK)) { > + } else if (FIELD_EX32(event_en_reg, EVENT_ENABLE, TXEMPTY) && > + FIELD_EX32(status_reg, STATUS, TXEMPTY)) { > /* SPI TXEMPTY, TXFIFO drained */ > event_irq = 1; > - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE] > - & R_EVENT_ENABLE_RXFULL_MASK) && > - (s->regs[IBEX_SPI_HOST_STATUS] & R_STATUS_RXFULL_MASK)) { > + } else if (FIELD_EX32(event_en_reg, EVENT_ENABLE, RXFULL) && > + FIELD_EX32(status_reg, STATUS, RXFULL)) { > /* SPI RXFULL, RXFIFO full */ > event_irq = 1; > } > @@ -232,10 +234,11 @@ static void ibex_spi_host_irq(IbexSPIHostState *s) > > static void ibex_spi_host_transfer(IbexSPIHostState *s) > { > - uint32_t rx, tx; > + uint32_t rx, tx, data; > /* Get num of one byte transfers */ > - uint8_t segment_len = ((s->regs[IBEX_SPI_HOST_COMMAND] & R_COMMAND_LEN_MASK) > - >> R_COMMAND_LEN_SHIFT); > + uint8_t segment_len = FIELD_EX32(s->regs[IBEX_SPI_HOST_COMMAND], > + COMMAND, LEN); > + > while (segment_len > 0) { > if (fifo8_is_empty(&s->tx_fifo)) { > /* Assert Stall */ > @@ -262,22 +265,21 @@ static void ibex_spi_host_transfer(IbexSPIHostState *s) > --segment_len; > } > > + data = s->regs[IBEX_SPI_HOST_STATUS]; > /* Assert Ready */ > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_READY_MASK; > + data = FIELD_DP32(data, STATUS, READY, 1); > /* Set RXQD */ > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXQD_MASK; > - s->regs[IBEX_SPI_HOST_STATUS] |= (R_STATUS_RXQD_MASK > - & div4_round_up(segment_len)); > + data = FIELD_DP32(data, STATUS, RXQD, div4_round_up(segment_len)); > /* Set TXQD */ > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXQD_MASK; > - s->regs[IBEX_SPI_HOST_STATUS] |= (fifo8_num_used(&s->tx_fifo) / 4) > - & R_STATUS_TXQD_MASK; > + data = FIELD_DP32(data, STATUS, TXQD, fifo8_num_used(&s->tx_fifo) / 4); > /* Clear TXFULL */ > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXFULL_MASK; > - /* Assert TXEMPTY and drop remaining bytes that exceed segment_len */ > - ibex_spi_txfifo_reset(s); > + data = FIELD_DP32(data, STATUS, TXFULL, 0); > /* Reset RXEMPTY */ > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXEMPTY_MASK; > + data = FIELD_DP32(data, STATUS, RXEMPTY, 0); > + /* Update register status */ > + s->regs[IBEX_SPI_HOST_STATUS] = data; > + /* Drop remaining bytes that exceed segment_len */ > + ibex_spi_txfifo_reset(s); > > ibex_spi_host_irq(s); > } > @@ -340,7 +342,7 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, > { > IbexSPIHostState *s = opaque; > uint32_t val32 = val64; > - uint32_t shift_mask = 0xff; > + uint32_t shift_mask = 0xff, data = 0, status = 0; > uint8_t txqd_len; > > trace_ibex_spi_host_write(addr, size, val64); > @@ -397,22 +399,24 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, > s->regs[addr] = val32; > > /* STALL, IP not enabled */ > - if (!(s->regs[IBEX_SPI_HOST_CONTROL] & R_CONTROL_SPIEN_MASK)) { > + if (!(FIELD_EX32(s->regs[IBEX_SPI_HOST_CONTROL], > + CONTROL, SPIEN))) { > return; > } > > /* SPI not ready, IRQ Error */ > - if (!(s->regs[IBEX_SPI_HOST_STATUS] & R_STATUS_READY_MASK)) { > + if (!(FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], > + STATUS, READY))) { > s->regs[IBEX_SPI_HOST_ERROR_STATUS] |= R_ERROR_STATUS_CMDBUSY_MASK; > ibex_spi_host_irq(s); > return; > } > + > /* Assert Not Ready */ > s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_READY_MASK; > > - if (((val32 & R_COMMAND_DIRECTION_MASK) >> R_COMMAND_DIRECTION_SHIFT) > - != BIDIRECTIONAL_TRANSFER) { > - qemu_log_mask(LOG_UNIMP, > + if (FIELD_EX32(val32, COMMAND, DIRECTION) != BIDIRECTIONAL_TRANSFER) { > + qemu_log_mask(LOG_UNIMP, > "%s: Rx Only/Tx Only are not supported\n", __func__); > } > > @@ -452,8 +456,8 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, > return; > } > /* Byte ordering is set by the IP */ > - if ((s->regs[IBEX_SPI_HOST_STATUS] & > - R_STATUS_BYTEORDER_MASK) == 0) { > + status = s->regs[IBEX_SPI_HOST_STATUS]; > + if (FIELD_EX32(status, STATUS, BYTEORDER) == 0) { > /* LE: LSB transmitted first (default for ibex processor) */ > shift_mask = 0xff << (i * 8); > } else { > @@ -464,18 +468,18 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr, > > fifo8_push(&s->tx_fifo, (val32 & shift_mask) >> (i * 8)); > } > - > + status = s->regs[IBEX_SPI_HOST_STATUS]; > /* Reset TXEMPTY */ > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXEMPTY_MASK; > + status = FIELD_DP32(status, STATUS, TXEMPTY, 0); > /* Update TXQD */ > - txqd_len = (s->regs[IBEX_SPI_HOST_STATUS] & > - R_STATUS_TXQD_MASK) >> R_STATUS_TXQD_SHIFT; > + txqd_len = FIELD_EX32(status, STATUS, TXQD); > /* Partial bytes (size < 4) are padded, in words. */ > txqd_len += 1; > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXQD_MASK; > - s->regs[IBEX_SPI_HOST_STATUS] |= txqd_len; > + status = FIELD_DP32(status, STATUS, TXQD, txqd_len); > /* Assert Ready */ > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_READY_MASK; > + status = FIELD_DP32(status, STATUS, READY, 1); > + /* Update register status */ > + s->regs[IBEX_SPI_HOST_STATUS] = status; > break; > case IBEX_SPI_HOST_ERROR_ENABLE: > s->regs[addr] = val32; > -- > 2.37.2 > >