On Tue, 30 May 2023 at 22:23, Vikram Garhwal <vikram.garh...@amd.com> wrote: > > The QTests perform three tests on the Xilinx VERSAL CANFD controller: > Tests the CANFD controllers in loopback. > Tests the CANFD controllers in normal mode with CAN frame. > Tests the CANFD controllers in normal mode with CANFD frame. > > Signed-off-by: Vikram Garhwal <vikram.garh...@amd.com> > Acked-by: Thomas Huth <th...@redhat.com> > Reviewed-by: Francisco Iglesias <francisco.igles...@amd.com> > Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
Hi; Coverity has spotted some issues with this test code; could you investigate and send followup patches, please ? > +static void match_rx_tx_data(const uint32_t *buf_tx, const uint32_t *buf_rx, > + bool is_canfd_frame) > +{ > + uint16_t size = 0; > + uint8_t len = CAN_FRAME_SIZE; > + > + if (is_canfd_frame) { > + len = CANFD_FRAME_SIZE; > + } Here len is either 4 (if !is_canfd_frame) or 18 (if is_canfd_frame)... > + > + while (size < len) { ...and we loop with size always less than len... > + if (R_RX0_ID_OFFSET + 4 * size == R_RX0_DLC_OFFSET) { > + g_assert_cmpint((buf_rx[size] & DLC_FD_BIT_MASK), ==, > + (buf_tx[size] & DLC_FD_BIT_MASK)); > + } else { > + if (!is_canfd_frame && size == 4) { ...so here this condition can never be true: if !is_canfd_frame then we know size is less than 4. What was the intention here ? (CID 1512900) > + break; > + } > + > + g_assert_cmpint(buf_rx[size], ==, buf_tx[size]); > + } > + > + size++; > + } > +} > +/* > + * Xilinx CANFD supports both CAN and CANFD frames. This test will be > + * transferring CAN frame i.e. 8 bytes of data from CANFD0 and CANFD1 through > + * canbus. CANFD0 initiate the data transfer to can-bus, CANFD1 receives the > + * data. Test compares the can frame data sent from CANFD0 and received on > + * CANFD1. > + */ > +static void test_can_data_transfer(void) > +{ > + uint32_t buf_tx[CAN_FRAME_SIZE] = { 0x5a5bb9a4, 0x80000000, > + 0x12345678, 0x87654321 }; > + uint32_t buf_rx[CAN_FRAME_SIZE] = { 0x00, 0x00, 0x00, 0x00 }; The buf_rx[] array here is only 4 bytes long... > + uint32_t status = 0; > + > + generate_random_data(buf_tx, false); > + > + QTestState *qts = qtest_init("-machine xlnx-versal-virt" > + " -object can-bus,id=canbus" > + " -machine canbus0=canbus" > + " -machine canbus1=canbus" > + ); > + > + configure_canfd(qts, MSR_NORMAL_MODE); > + > + /* Check if CANFD0 and CANFD1 are in Normal mode. */ > + status = qtest_readl(qts, CANFD0_BASE_ADDR + R_SR_OFFSET); > + status = status & STATUS_REG_MASK; > + g_assert_cmpint(status, ==, STATUS_NORMAL_MODE); > + > + status = qtest_readl(qts, CANFD1_BASE_ADDR + R_SR_OFFSET); > + status = status & STATUS_REG_MASK; > + g_assert_cmpint(status, ==, STATUS_NORMAL_MODE); > + > + write_data(qts, CANFD0_BASE_ADDR, buf_tx, false); > + > + send_data(qts, CANFD0_BASE_ADDR); > + read_data(qts, CANFD1_BASE_ADDR, buf_rx); ...but read_data() will write up to 17 bytes of data to the buffer, if the incoming data from the device claims it to be a canfd frame. The device shouldn't really do that, but the point of a test is that the device might not be functioning correctly, so we should size buf_rx[] large enough to fit whatever read_data() writes to it. (CID 1512899) > + match_rx_tx_data(buf_tx, buf_rx, false); > + > + qtest_quit(qts); > +} thanks -- PMM