On 2023-02-21 at 12:10:48 +0100, Marco Pagani wrote:
>
>
> On 2023-02-18 10:59, Xu Yilun wrote:
> > On 2023-02-03 at 18:06:50 +0100, Marco Pagani wrote:
> >> Introduce an initial KUnit suite to test the core components of the
> >> FPGA subsystem.
> >
> > I'm not familiar with kunit, and I spend some time to read the
> > Documentation/dev-tools/kunit/, sorry for late response.
>
> Thank you for reviewing.
>
> >
> >>
> >> The test suite consists of two test cases. The first test case checks
> >> the programming of a static image on a fake FPGA with a single hardware
> >> bridge. The FPGA is first programmed using a test image stored in a
> >> buffer, and then with the same image linked to a single-entry
> >> scatter-gather list.
> >>
> >> The second test case models dynamic partial reconfiguration. The FPGA
> >> is first configured with a static image that implements a
> >> reconfigurable design containing a sub-region controlled by two soft
> >> bridges. Then, the reconfigurable sub-region is reconfigured using
> >> a fake partial bitstream image. After the reconfiguration, the test
> >> checks that the soft bridges have been correctly activated.
> >>
> >> Signed-off-by: Marco Pagani <marpagan@xxxxxxxxxx>
> >> ---
> >> drivers/fpga/Kconfig | 2 +
> >> drivers/fpga/Makefile | 3 +
> >> drivers/fpga/tests/.kunitconfig | 5 +
> >> drivers/fpga/tests/Kconfig | 15 ++
> >> drivers/fpga/tests/Makefile | 6 +
> >> drivers/fpga/tests/fpga-tests.c | 264 ++++++++++++++++++++++++++++++++
> >> 6 files changed, 295 insertions(+)
> >> create mode 100644 drivers/fpga/tests/.kunitconfig
> >> create mode 100644 drivers/fpga/tests/Kconfig
> >> create mode 100644 drivers/fpga/tests/Makefile
> >> create mode 100644 drivers/fpga/tests/fpga-tests.c
> >>
> >> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> >> index 0a00763b9f28..2f689ac4ba3a 100644
> >> --- a/drivers/fpga/Kconfig
> >> +++ b/drivers/fpga/Kconfig
> >> @@ -276,4 +276,6 @@ config FPGA_MGR_LATTICE_SYSCONFIG_SPI
> >> FPGA manager driver support for Lattice FPGAs programming over slave
> >> SPI sysCONFIG interface.
> >>
> >> +source "drivers/fpga/tests/Kconfig"
> >> +
> >> endif # FPGA
> >> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> >> index 72e554b4d2f7..352a2612623e 100644
> >> --- a/drivers/fpga/Makefile
> >> +++ b/drivers/fpga/Makefile
> >> @@ -55,3 +55,6 @@ obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000) += dfl-n3000-nios.o
> >>
> >> # Drivers for FPGAs which implement DFL
> >> obj-$(CONFIG_FPGA_DFL_PCI) += dfl-pci.o
> >> +
> >> +# KUnit tests
> >> +obj-$(CONFIG_FPGA_KUNIT_TESTS) += tests/
> >> diff --git a/drivers/fpga/tests/.kunitconfig b/drivers/fpga/tests/.kunitconfig
> >> new file mode 100644
> >> index 000000000000..a1c2a2974c39
> >> --- /dev/null
> >> +++ b/drivers/fpga/tests/.kunitconfig
> >> @@ -0,0 +1,5 @@
> >> +CONFIG_KUNIT=y
> >> +CONFIG_FPGA=y
> >> +CONFIG_FPGA_REGION=y
> >> +CONFIG_FPGA_BRIDGE=y
> >> +CONFIG_FPGA_KUNIT_TESTS=y
> >> diff --git a/drivers/fpga/tests/Kconfig b/drivers/fpga/tests/Kconfig
> >> new file mode 100644
> >> index 000000000000..5198e605b38d
> >> --- /dev/null
> >> +++ b/drivers/fpga/tests/Kconfig
> >> @@ -0,0 +1,15 @@
> >> +config FPGA_KUNIT_TESTS
> >> + tristate "FPGA KUnit tests" if !KUNIT_ALL_TESTS
> >> + depends on FPGA && FPGA_REGION && FPGA_BRIDGE && KUNIT
> >> + default KUNIT_ALL_TESTS
> >> + help
> >> + Builds unit tests for the FPGA subsystem. This option
> >> + is not useful for distributions or general kernels,
> >> + but only for kernel developers working on the FPGA
> >> + subsystem and its associated drivers.
> >> +
> >> + For more information on KUnit and unit tests in general,
> >> + please refer to the KUnit documentation in
> >> + Documentation/dev-tools/kunit/.
> >> +
> >> + If in doubt, say "N".
> >> diff --git a/drivers/fpga/tests/Makefile b/drivers/fpga/tests/Makefile
> >> new file mode 100644
> >> index 000000000000..74346ae62457
> >> --- /dev/null
> >> +++ b/drivers/fpga/tests/Makefile
> >> @@ -0,0 +1,6 @@
> >> +# SPDX-License-Identifier: GPL-2.0
> >> +
> >> +obj-$(CONFIG_FPGA_KUNIT_TESTS) += fake-fpga-mgr.o
> >> +obj-$(CONFIG_FPGA_KUNIT_TESTS) += fake-fpga-region.o
> >> +obj-$(CONFIG_FPGA_KUNIT_TESTS) += fake-fpga-bridge.o
> >
> > It is better the patches for fake components come first, otherwise may
> > break the compilation. Also not friendly for review.
>
> Sorry. I'll change the order in the next version.
>
> >
> >> +obj-$(CONFIG_FPGA_KUNIT_TESTS) += fpga-tests.o
> >
> > Maybe fpga-test.o?
>
> I'll change the name in the next version.
>
> >
> > And could they be built in a single module? I haven't find a reason
> > these fake components been used alone.
> >
>
> My feeling is that they could also come in handy to do some general
> development or testing on the subsystem. For instance, I used the fake
> FPGA manager in isolation to experiment with the OF region.
That's fine.
>
> Initially, the fake manager also had an of_device_id device matching
> struct. However, I later removed it because it was not used for the
> test setup, and I was not sure if adding an OF device matching struct
> was acceptable for a test driver.
>
> >> diff --git a/drivers/fpga/tests/fpga-tests.c b/drivers/fpga/tests/fpga-tests.c
> >> new file mode 100644
> >> index 000000000000..33f04079b32f
> >> --- /dev/null
> >> +++ b/drivers/fpga/tests/fpga-tests.c
> >> @@ -0,0 +1,264 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Test suite for the FPGA subsystem
> >> + *
> >> + * Copyright (C) 2023 Red Hat, Inc. All rights reserved.
> >> + *
> >> + * Author: Marco Pagani <marpagan@xxxxxxxxxx>
> >> + */
> >> +
> >> +#include <kunit/test.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/scatterlist.h>
> >> +
> >> +#include <linux/fpga/fpga-mgr.h>
> >> +#include <linux/fpga/fpga-region.h>
> >> +#include <linux/fpga/fpga-bridge.h>
> >> +
> >> +#include "fake-fpga-region.h"
> >> +#include "fake-fpga-bridge.h"
> >> +#include "fake-fpga-mgr.h"
> >> +
> >> +#define FAKE_BIT_BLOCKS 16
> >> +#define FAKE_BIT_SIZE (FPGA_TEST_BIT_BLOCK * FAKE_BIT_BLOCKS)
> >> +
> >> +static u8 fake_bit[FAKE_BIT_SIZE];
> >> +
> >> +static int init_sgt_bit(struct sg_table *sgt, void *bit, size_t len)
> >> +{
> >> + int ret;
> >> +
> >> + ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + sg_init_one(sgt->sgl, bit, len);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static void free_sgt_bit(struct sg_table *sgt)
> >> +{
> >> + if (sgt)
> >> + sg_free_table(sgt);
> >> +}
> >> +
> >> +static void fpga_build_base_sys(struct kunit *test, struct fake_fpga_mgr *mgr_ctx,
> >> + struct fake_fpga_bridge *bridge_ctx,
> >> + struct fake_fpga_region *region_ctx)
> >> +{
> >> + int ret;
> >> +
> >> + ret = fake_fpga_mgr_register(mgr_ctx, test);
> >> + KUNIT_ASSERT_EQ(test, ret, 0);
> >> +
> >> + ret = fake_fpga_bridge_register(bridge_ctx, test);
> >> + KUNIT_ASSERT_EQ(test, ret, 0);
> >> +
> >> + ret = fake_fpga_region_register(region_ctx, mgr_ctx->mgr, test);
> >> + KUNIT_ASSERT_EQ(test, ret, 0);
> >> +
> >> + ret = fake_fpga_region_add_bridge(region_ctx, bridge_ctx->bridge);
> >> + KUNIT_ASSERT_EQ(test, ret, 0);
> >> +}
> >> +
> >> +static void fpga_free_base_sys(struct fake_fpga_mgr *mgr_ctx,
> >> + struct fake_fpga_bridge *bridge_ctx,
> >> + struct fake_fpga_region *region_ctx)
> >> +{
> >> + if (region_ctx)
> >> + fake_fpga_region_unregister(region_ctx);
> >> +
> >> + if (bridge_ctx)
> >> + fake_fpga_bridge_unregister(bridge_ctx);
> >> +
> >> + if (region_ctx)
> >> + fake_fpga_mgr_unregister(mgr_ctx);
> >> +}
> >> +
> >> +static int fpga_suite_init(struct kunit_suite *suite)
> >> +{
> >> + fake_fpga_mgr_fill_header(fake_bit);
> >
> > Do we need to run it before every case? Or just run once for all cases?
> >
>
> Just once for all cases. So I'm calling it from the suite_init function.
>
> For the next version, I'm thinking of allocating the image buffer using
> kunit_kzalloc() instead of using a global static array.
>
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void fpga_base_test(struct kunit *test)
> >> +{
> >> + int ret;
> >> +
> >> + struct fake_fpga_mgr mgr_ctx;
> >> + struct fake_fpga_bridge base_bridge_ctx;
> >> + struct fake_fpga_region base_region_ctx;
> >> +
> >> + struct fpga_image_info *test_img_info;
> >> +
> >> + struct sg_table sgt_bit;
> >> +
> >> + fpga_build_base_sys(test, &mgr_ctx, &base_bridge_ctx, &base_region_ctx);
> >> +
> >> + /* Allocate a fake test image using a buffer */
> >> + test_img_info = fpga_image_info_alloc(&mgr_ctx.pdev->dev);
> >> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_img_info);
> >> +
> >> + test_img_info->buf = fake_bit;
> >> + test_img_info->count = sizeof(fake_bit);
> >> +
> >> + kunit_info(test, "fake bitstream size: %zu\n", test_img_info->count);
> >> +
> >> + KUNIT_EXPECT_EQ(test, 0, fake_fpga_mgr_get_rcfg_count(&mgr_ctx));
> >> +
> >> + KUNIT_EXPECT_EQ(test, 0, fake_fpga_bridge_get_state(&base_bridge_ctx));
> >> + KUNIT_EXPECT_EQ(test, 0, fake_fpga_bridge_get_cycles_count(&base_bridge_ctx));
> >> +
> >> + /* Program the fake FPGA using the image buffer */
> >> + base_region_ctx.region->info = test_img_info;
> >> + ret = fpga_region_program_fpga(base_region_ctx.region);
> >> + KUNIT_ASSERT_EQ(test, ret, 0);
> >> +
> >> + fake_fpga_mgr_check_write_buf(&mgr_ctx);
> >> +
> >> + KUNIT_EXPECT_EQ(test, 1, fake_fpga_mgr_get_rcfg_count(&mgr_ctx));
> >> +
> >> + KUNIT_EXPECT_EQ(test, 1, fake_fpga_bridge_get_state(&base_bridge_ctx));
> >> + KUNIT_EXPECT_EQ(test, 1, fake_fpga_bridge_get_cycles_count(&base_bridge_ctx));
> >> +
> >> + fpga_image_info_free(test_img_info);
> >> +
> >> + /* Allocate another fake test image using a scatter list */
> >> + test_img_info = fpga_image_info_alloc(&mgr_ctx.pdev->dev);
> >> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_img_info);
> >> +
> >> + ret = init_sgt_bit(&sgt_bit, fake_bit, FAKE_BIT_SIZE);
> >> + KUNIT_ASSERT_EQ(test, ret, 0);
> >
> > This is not fpga function, do we need the ASSERT?
> >
>
> You're right. I'll change it to EXPECT.
Mm.. I think we may move the sgt initialization in .suite_init, and just
return ERROR for failure. Does it help to quickly find out this is an
ENV error, not a test case failure?
Thanks,
Yilun
