[RFC PATCH v6 0/4] fpga: add initial KUnit tests for the subsystem

[RFC PATCH v6 0/4] fpga: add initial KUnit tests for the subsystem

[Marco Pagani]

This patch set introduces initial KUnit test suites for the core components
of the FPGA subsystem.

Tests can be run using:
[user@localhost linux]$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/fpga/tests

v6:
- Restructured the code into self-contained test modules
- Added tests for the basic behaviors of the components
- Improved programming tests for the FPGA Manager
- Fixed code/comments mismatch in the list of Bridges test case

v5:
- Removed most of the exported functions using shared buffers for stats
- Moved all KUnit expectations/assertions to the main test module
- Removed standalone use case to simplify the code
- Removed instances counters from fake components (using device.id instead)
- Set header size in the .parse_header op
- Improved bridge get_put_list test case

v4:
- Fix build error

v3:
- Calling fpga_bridges_put() between reconfigurations
- Functions for registering fake modules allocate and return context structs

v2:
- Restructured code into multiple suites to test components in isolation
- Reduced code duplication using init and exit methods
- Using a get_bridges() method to build the list of bridges just before programming
- Regions and Bridges are organized topologically
- Changed bitstream/bit to images
- Allocate images dynamically
- Renamed fpga-tests to fpga-test
- Simplified Kconfig
- Add license info to the fpga-test module

Marco Pagani (4):
  fpga: add an initial KUnit suite for the FPGA Manager
  fpga: add an initial KUnit suite for the FPGA Bridge
  fpga: add an initial KUnit suite for the FPGA Region
  fpga: add configuration for the KUnit test suites.

 drivers/fpga/Kconfig                  |   2 +
 drivers/fpga/Makefile                 |   3 +
 drivers/fpga/tests/.kunitconfig       |   5 +
 drivers/fpga/tests/Kconfig            |  11 +
 drivers/fpga/tests/Makefile           |   5 +
 drivers/fpga/tests/fpga-bridge-test.c | 164 +++++++++++++++
 drivers/fpga/tests/fpga-mgr-test.c    | 289 ++++++++++++++++++++++++++
 drivers/fpga/tests/fpga-region-test.c | 186 +++++++++++++++++
 8 files changed, 665 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-bridge-test.c
 create mode 100644 drivers/fpga/tests/fpga-mgr-test.c
 create mode 100644 drivers/fpga/tests/fpga-region-test.c


base-commit: 7877cb91f1081754a1487c144d85dc0d2e2e7fc4
-- 
2.40.1

[RFC PATCH v6 1/4] fpga: add an initial KUnit suite for the FPGA Manager

[Marco Pagani]

The suite tests the basic behaviors of the FPGA Manager, and the
programming using a single contiguous buffer and scatter gather table.

Signed-off-by: Marco Pagani <marpagan@redhat.com>
---
 drivers/fpga/tests/fpga-mgr-test.c | 289 +++++++++++++++++++++++++++++
 1 file changed, 289 insertions(+)
 create mode 100644 drivers/fpga/tests/fpga-mgr-test.c

diff --git a/drivers/fpga/tests/fpga-mgr-test.c b/drivers/fpga/tests/fpga-mgr-test.c
new file mode 100644
index 000000000000..c771ae4dd05e
--- /dev/null
+++ b/drivers/fpga/tests/fpga-mgr-test.c
@@ -0,0 +1,289 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit test for the FPGA Manager
+ *
+ * Copyright (C) 2023 Red Hat, Inc.
+ *
+ * Author: Marco Pagani <marpagan@redhat.com>
+ */
+
+#include <linux/types.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/scatterlist.h>
+#include <kunit/test.h>
+#include <linux/fpga/fpga-mgr.h>
+
+#define HEADER_MAGIC		'H'
+#define IMAGE_MAGIC		'P'
+#define IMAGE_BLOCK		1024
+
+#define HEADER_SIZE		IMAGE_BLOCK
+#define IMAGE_SIZE		(IMAGE_BLOCK * 4)
+
+struct mgr_stats {
+	bool header_match;
+	bool image_match;
+	u32 seq_num;
+	u32 op_parse_header_seq;
+	u32 op_write_init_seq;
+	u32 op_write_seq;
+	u32 op_write_sg_seq;
+	u32 op_write_complete_seq;
+	enum fpga_mgr_states op_parse_header_state;
+	enum fpga_mgr_states op_write_init_state;
+	enum fpga_mgr_states op_write_state;
+	enum fpga_mgr_states op_write_sg_state;
+	enum fpga_mgr_states op_write_complete_state;
+};
+
+struct mgr_ctx {
+	struct fpga_image_info *img_info;
+	struct fpga_manager *mgr;
+	struct platform_device *pdev;
+	struct mgr_stats stats;
+};
+
+static char *init_test_buffer(struct kunit *test, size_t count)
+{
+	char *buf;
+	size_t i;
+
+	buf = kunit_kzalloc(test, count, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, buf);
+
+	for (i = 0; i < count; i++)
+		buf[i] = i < HEADER_SIZE ? HEADER_MAGIC : IMAGE_MAGIC;
+
+	return buf;
+}
+
+static int op_parse_header(struct fpga_manager *mgr, struct fpga_image_info *info,
+			   const char *buf, size_t count)
+{
+	struct mgr_stats *stats = mgr->priv;
+	size_t i;
+
+	/* Set header_size and data_size for later */
+	info->header_size = HEADER_SIZE;
+	info->data_size = info->count - HEADER_SIZE;
+
+	stats->header_match = true;
+
+	/* Check header */
+	for (i = 0; i < info->header_size; i++)
+		if (buf[i] != HEADER_MAGIC)
+			stats->header_match = false;
+
+	stats->op_parse_header_state = mgr->state;
+	stats->op_parse_header_seq = stats->seq_num++;
+
+	return 0;
+}
+
+static int op_write_init(struct fpga_manager *mgr, struct fpga_image_info *info,
+			 const char *buf, size_t count)
+{
+	struct mgr_stats *stats = mgr->priv;
+
+	stats->op_write_init_state = mgr->state;
+	stats->op_write_init_seq = stats->seq_num++;
+
+	return 0;
+}
+
+static int op_write(struct fpga_manager *mgr, const char *buf, size_t count)
+{
+	struct mgr_stats *stats = mgr->priv;
+	size_t i;
+
+	/* Check image */
+	stats->image_match = true;
+	for (i = 0; i < count; i++)
+		if (buf[i] != IMAGE_MAGIC)
+			stats->image_match = false;
+
+	stats->op_write_state = mgr->state;
+	stats->op_write_seq = stats->seq_num++;
+
+	return 0;
+}
+
+static int op_write_sg(struct fpga_manager *mgr, struct sg_table *sgt)
+{
+	struct mgr_stats *stats = mgr->priv;
+	struct scatterlist *sg;
+	char *img;
+	unsigned int si, i, j = 0;
+
+	stats->image_match = true;
+
+	/* Check image (write_sg will still get whole image in sg_table) */
+	for_each_sgtable_sg(sgt, sg, si) {
+		img = sg_virt(sg);
+		for (i = 0; i < sg->length; i++) {
+			if (i + j > HEADER_SIZE && img[i] != IMAGE_MAGIC)
+				stats->image_match = false;
+		}
+		j += i;
+	}
+
+	stats->op_write_sg_state = mgr->state;
+	stats->op_write_sg_seq = stats->seq_num++;
+
+	return 0;
+}
+
+static int op_write_complete(struct fpga_manager *mgr, struct fpga_image_info *info)
+{
+	struct mgr_stats *stats = mgr->priv;
+
+	stats->op_write_complete_state = mgr->state;
+	stats->op_write_complete_seq = stats->seq_num++;
+
+	return 0;
+}
+
+static const struct fpga_manager_ops fake_mgr_ops = {
+	.skip_header = true,
+	.parse_header = op_parse_header,
+	.write_init = op_write_init,
+	.write = op_write,
+	.write_sg = op_write_sg,
+	.write_complete = op_write_complete,
+};
+
+static void fpga_mgr_test_get(struct kunit *test)
+{
+	struct mgr_ctx *ctx = test->priv;
+	struct fpga_manager *mgr;
+
+	mgr = fpga_mgr_get(&ctx->pdev->dev);
+	KUNIT_EXPECT_PTR_EQ(test, mgr, ctx->mgr);
+
+	fpga_mgr_put(ctx->mgr);
+}
+
+static void fpga_mgr_test_lock(struct kunit *test)
+{
+	struct mgr_ctx *ctx = test->priv;
+	int ret;
+
+	ret = fpga_mgr_lock(ctx->mgr);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	ret = fpga_mgr_lock(ctx->mgr);
+	KUNIT_EXPECT_EQ(test, ret, -EBUSY);
+
+	fpga_mgr_unlock(ctx->mgr);
+}
+
+static void fpga_mgr_test_img_load_buf(struct kunit *test)
+{
+	struct mgr_ctx *ctx = test->priv;
+	char *img_buf;
+	int ret;
+
+	img_buf = init_test_buffer(test, IMAGE_SIZE);
+
+	ctx->img_info->count = IMAGE_SIZE;
+	ctx->img_info->buf = img_buf;
+
+	ret = fpga_mgr_load(ctx->mgr, ctx->img_info);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_TRUE(test, ctx->stats.header_match);
+	KUNIT_EXPECT_TRUE(test, ctx->stats.image_match);
+
+	KUNIT_EXPECT_EQ(test, ctx->stats.op_parse_header_state, FPGA_MGR_STATE_PARSE_HEADER);
+	KUNIT_EXPECT_EQ(test, ctx->stats.op_write_init_state, FPGA_MGR_STATE_WRITE_INIT);
+	KUNIT_EXPECT_EQ(test, ctx->stats.op_write_state, FPGA_MGR_STATE_WRITE);
+	KUNIT_EXPECT_EQ(test, ctx->stats.op_write_complete_state, FPGA_MGR_STATE_WRITE_COMPLETE);
+
+	KUNIT_EXPECT_EQ(test, ctx->stats.op_write_init_seq, ctx->stats.op_parse_header_seq + 1);
+	KUNIT_EXPECT_EQ(test, ctx->stats.op_write_seq, ctx->stats.op_parse_header_seq + 2);
+	KUNIT_EXPECT_EQ(test, ctx->stats.op_write_complete_seq, ctx->stats.op_parse_header_seq + 3);
+}
+
+static void fpga_mgr_test_img_load_sgt(struct kunit *test)
+{
+	struct mgr_ctx *ctx = test->priv;
+	struct sg_table *sgt;
+	char *img_buf;
+	int ret;
+
+	img_buf = init_test_buffer(test, IMAGE_SIZE);
+
+	sgt = kunit_kzalloc(test, sizeof(*sgt), GFP_KERNEL);
+	ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+	sg_init_one(sgt->sgl, img_buf, IMAGE_SIZE);
+
+	ctx->img_info->sgt = sgt;
+
+	ret = fpga_mgr_load(ctx->mgr, ctx->img_info);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_TRUE(test, ctx->stats.header_match);
+	KUNIT_EXPECT_TRUE(test, ctx->stats.image_match);
+
+	KUNIT_EXPECT_EQ(test, ctx->stats.op_parse_header_state, FPGA_MGR_STATE_PARSE_HEADER);
+	KUNIT_EXPECT_EQ(test, ctx->stats.op_write_init_state, FPGA_MGR_STATE_WRITE_INIT);
+	KUNIT_EXPECT_EQ(test, ctx->stats.op_write_sg_state, FPGA_MGR_STATE_WRITE);
+	KUNIT_EXPECT_EQ(test, ctx->stats.op_write_complete_state, FPGA_MGR_STATE_WRITE_COMPLETE);
+
+	KUNIT_EXPECT_EQ(test, ctx->stats.op_write_init_seq, ctx->stats.op_parse_header_seq + 1);
+	KUNIT_EXPECT_EQ(test, ctx->stats.op_write_sg_seq, ctx->stats.op_parse_header_seq + 2);
+	KUNIT_EXPECT_EQ(test, ctx->stats.op_write_complete_seq, ctx->stats.op_parse_header_seq + 3);
+
+	sg_free_table(ctx->img_info->sgt);
+}
+
+static int fpga_mgr_test_init(struct kunit *test)
+{
+	struct mgr_ctx *ctx;
+
+	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
+
+	ctx->pdev = platform_device_register_simple("mgr_pdev", PLATFORM_DEVID_AUTO, NULL, 0);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->pdev);
+
+	ctx->mgr = devm_fpga_mgr_register(&ctx->pdev->dev, "Fake FPGA Manager", &fake_mgr_ops,
+					  &ctx->stats);
+	KUNIT_ASSERT_FALSE(test, IS_ERR_OR_NULL(ctx->mgr));
+
+	ctx->img_info = fpga_image_info_alloc(&ctx->pdev->dev);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->img_info);
+
+	test->priv = ctx;
+
+	return 0;
+}
+
+static void fpga_mgr_test_exit(struct kunit *test)
+{
+	struct mgr_ctx *ctx = test->priv;
+
+	fpga_image_info_free(ctx->img_info);
+	platform_device_unregister(ctx->pdev);
+}
+
+static struct kunit_case fpga_mgr_test_cases[] = {
+	KUNIT_CASE(fpga_mgr_test_get),
+	KUNIT_CASE(fpga_mgr_test_lock),
+	KUNIT_CASE(fpga_mgr_test_img_load_buf),
+	KUNIT_CASE(fpga_mgr_test_img_load_sgt),
+	{}
+};
+
+static struct kunit_suite fpga_mgr_suite = {
+	.name = "fpga_mgr",
+	.init = fpga_mgr_test_init,
+	.exit = fpga_mgr_test_exit,
+	.test_cases = fpga_mgr_test_cases,
+};
+
+kunit_test_suite(fpga_mgr_suite);
+
+MODULE_LICENSE("GPL");
-- 
2.40.1

[RFC PATCH v6 2/4] fpga: add an initial KUnit suite for the FPGA Bridge

[Marco Pagani]

The suite tests the basic behaviors of the FPGA Bridge and the
functions that operate on a list of Bridges.

Signed-off-by: Marco Pagani <marpagan@redhat.com>
---
 drivers/fpga/tests/fpga-bridge-test.c | 164 ++++++++++++++++++++++++++
 1 file changed, 164 insertions(+)
 create mode 100644 drivers/fpga/tests/fpga-bridge-test.c

diff --git a/drivers/fpga/tests/fpga-bridge-test.c b/drivers/fpga/tests/fpga-bridge-test.c
new file mode 100644
index 000000000000..52c82f76a92c
--- /dev/null
+++ b/drivers/fpga/tests/fpga-bridge-test.c
@@ -0,0 +1,164 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit test for the FPGA Bridge
+ *
+ * Copyright (C) 2023 Red Hat, Inc.
+ *
+ * Author: Marco Pagani <marpagan@redhat.com>
+ */
+
+#include <linux/types.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <kunit/test.h>
+#include <linux/fpga/fpga-bridge.h>
+
+struct bridge_stats {
+	bool enable;
+};
+
+struct bridge_ctx {
+	struct fpga_bridge *bridge;
+	struct platform_device *pdev;
+	struct bridge_stats stats;
+};
+
+static int op_enable_set(struct fpga_bridge *bridge, bool enable)
+{
+	struct bridge_stats *stats = bridge->priv;
+
+	stats->enable = enable;
+
+	return 0;
+}
+
+static const struct fpga_bridge_ops fake_bridge_ops = {
+	.enable_set = op_enable_set
+};
+
+static struct bridge_ctx *register_test_bridge(struct kunit *test)
+{
+	struct bridge_ctx *ctx;
+
+	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
+
+	ctx->pdev = platform_device_register_simple("bridge_pdev", PLATFORM_DEVID_AUTO, NULL, 0);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->pdev);
+
+	ctx->bridge = fpga_bridge_register(&ctx->pdev->dev, "Fake FPGA Bridge", &fake_bridge_ops,
+					   &ctx->stats);
+	KUNIT_ASSERT_FALSE(test, IS_ERR_OR_NULL(ctx->bridge));
+
+	return ctx;
+}
+
+static void unregister_test_bridge(struct bridge_ctx *ctx)
+{
+	fpga_bridge_unregister(ctx->bridge);
+	platform_device_unregister(ctx->pdev);
+}
+
+static void fpga_bridge_test_get(struct kunit *test)
+{
+	struct bridge_ctx *ctx = test->priv;
+	struct fpga_bridge *bridge;
+
+	bridge = fpga_bridge_get(&ctx->pdev->dev, NULL);
+	KUNIT_EXPECT_PTR_EQ(test, bridge, ctx->bridge);
+
+	bridge = fpga_bridge_get(&ctx->pdev->dev, NULL);
+	KUNIT_EXPECT_EQ(test, PTR_ERR(bridge), -EBUSY);
+
+	fpga_bridge_put(ctx->bridge);
+}
+
+static void fpga_bridge_test_toggle(struct kunit *test)
+{
+	struct bridge_ctx *ctx = test->priv;
+	int ret;
+
+	ret = fpga_bridge_disable(ctx->bridge);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+	KUNIT_EXPECT_FALSE(test, ctx->stats.enable);
+
+	ret = fpga_bridge_enable(ctx->bridge);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+	KUNIT_EXPECT_TRUE(test, ctx->stats.enable);
+}
+
+static void fpga_bridge_test_get_put_list(struct kunit *test)
+{
+	struct list_head bridge_list;
+	struct bridge_ctx *ctx_0, *ctx_1;
+	int ret;
+
+	ctx_0 = test->priv;
+	ctx_1 = register_test_bridge(test);
+
+	INIT_LIST_HEAD(&bridge_list);
+
+	/* Get bridge 0 and add it to the list */
+	ret = fpga_bridge_get_to_list(&ctx_0->pdev->dev, NULL, &bridge_list);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_PTR_EQ(test, ctx_0->bridge,
+			    list_first_entry_or_null(&bridge_list, struct fpga_bridge, node));
+
+	/* Get bridge 1 and add it to the list */
+	ret = fpga_bridge_get_to_list(&ctx_1->pdev->dev, NULL, &bridge_list);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_PTR_EQ(test, ctx_1->bridge,
+			    list_first_entry_or_null(&bridge_list, struct fpga_bridge, node));
+
+	/* Disable an then enable both bridges from the list */
+	ret = fpga_bridges_disable(&bridge_list);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_FALSE(test, ctx_0->stats.enable);
+	KUNIT_EXPECT_FALSE(test, ctx_1->stats.enable);
+
+	ret = fpga_bridges_enable(&bridge_list);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_TRUE(test, ctx_0->stats.enable);
+	KUNIT_EXPECT_TRUE(test, ctx_1->stats.enable);
+
+	/* Put and remove both bridges from the list */
+	fpga_bridges_put(&bridge_list);
+
+	KUNIT_EXPECT_TRUE(test, list_empty(&bridge_list));
+
+	unregister_test_bridge(ctx_1);
+}
+
+static int fpga_bridge_test_init(struct kunit *test)
+{
+	test->priv = register_test_bridge(test);
+
+	return 0;
+}
+
+static void fpga_bridge_test_exit(struct kunit *test)
+{
+	unregister_test_bridge(test->priv);
+}
+
+static struct kunit_case fpga_bridge_test_cases[] = {
+	KUNIT_CASE(fpga_bridge_test_get),
+	KUNIT_CASE(fpga_bridge_test_toggle),
+	KUNIT_CASE(fpga_bridge_test_get_put_list),
+	{}
+};
+
+static struct kunit_suite fpga_bridge_suite = {
+	.name = "fpga_bridge",
+	.init = fpga_bridge_test_init,
+	.exit = fpga_bridge_test_exit,
+	.test_cases = fpga_bridge_test_cases,
+};
+
+kunit_test_suite(fpga_bridge_suite);
+
+MODULE_LICENSE("GPL");
-- 
2.40.1

[RFC PATCH v6 3/4] fpga: add an initial KUnit suite for the FPGA Region

[Marco Pagani]

The suite tests the programming of an FPGA Region with a Bridge
and the function for finding a particular Region.

Signed-off-by: Marco Pagani <marpagan@redhat.com>
---
 drivers/fpga/tests/fpga-region-test.c | 186 ++++++++++++++++++++++++++
 1 file changed, 186 insertions(+)
 create mode 100644 drivers/fpga/tests/fpga-region-test.c

diff --git a/drivers/fpga/tests/fpga-region-test.c b/drivers/fpga/tests/fpga-region-test.c
new file mode 100644
index 000000000000..81b271088240
--- /dev/null
+++ b/drivers/fpga/tests/fpga-region-test.c
@@ -0,0 +1,186 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit test for the FPGA Region
+ *
+ * Copyright (C) 2023 Red Hat, Inc.
+ *
+ * Author: Marco Pagani <marpagan@redhat.com>
+ */
+
+#include <linux/types.h>
+#include <linux/module.h>
+#include <kunit/test.h>
+#include <linux/platform_device.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/fpga/fpga-bridge.h>
+#include <linux/fpga/fpga-region.h>
+
+struct mgr_stats {
+	u32 write_count;
+};
+
+struct bridge_stats {
+	u32 enable_count;
+};
+
+struct test_ctx {
+	struct fpga_manager *mgr;
+	struct platform_device *mgr_pdev;
+	struct fpga_bridge *bridge;
+	struct platform_device *bridge_pdev;
+	struct fpga_region *region;
+	struct platform_device *region_pdev;
+	struct bridge_stats bridge_stats;
+	struct mgr_stats mgr_stats;
+};
+
+static int op_write(struct fpga_manager *mgr, const char *buf, size_t count)
+{
+	struct mgr_stats *stats = mgr->priv;
+
+	stats->write_count++;
+
+	return 0;
+}
+
+static const struct fpga_manager_ops fake_mgr_ops = {
+	.write = op_write,
+};
+
+static int op_enable_set(struct fpga_bridge *bridge, bool enable)
+{
+	struct bridge_stats *stats = bridge->priv;
+
+	if (enable)
+		stats->enable_count++;
+
+	return 0;
+}
+
+static const struct fpga_bridge_ops fake_bridge_ops = {
+	.enable_set = op_enable_set
+};
+
+static int fake_region_get_bridges(struct fpga_region *region)
+{
+	struct fpga_bridge *bridge = region->priv;
+
+	return fpga_bridge_get_to_list(bridge->dev.parent, region->info, &region->bridge_list);
+}
+
+static int fake_region_match(struct device *dev, const void *data)
+{
+	return dev->parent == data;
+}
+
+static void fpga_region_test_class_find(struct kunit *test)
+{
+	struct test_ctx *ctx = test->priv;
+	struct fpga_region *region;
+
+	region = fpga_region_class_find(NULL, &ctx->region_pdev->dev, fake_region_match);
+	KUNIT_EXPECT_PTR_EQ(test, region, ctx->region);
+}
+
+static void fpga_region_test_program_fpga(struct kunit *test)
+{
+	struct test_ctx *ctx = test->priv;
+	struct fpga_image_info *img_info;
+	char img_buf[4];
+	int ret;
+
+	img_info = fpga_image_info_alloc(&ctx->mgr_pdev->dev);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, img_info);
+
+	img_info->buf = img_buf;
+	img_info->count = sizeof(img_buf);
+
+	ctx->region->info = img_info;
+	ret = fpga_region_program_fpga(ctx->region);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_EQ(test, 1, ctx->mgr_stats.write_count);
+	KUNIT_EXPECT_EQ(test, 1, ctx->bridge_stats.enable_count);
+
+	fpga_bridges_put(&ctx->region->bridge_list);
+
+	ret = fpga_region_program_fpga(ctx->region);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_EQ(test, 2, ctx->mgr_stats.write_count);
+	KUNIT_EXPECT_EQ(test, 2, ctx->bridge_stats.enable_count);
+
+	fpga_bridges_put(&ctx->region->bridge_list);
+
+	fpga_image_info_free(img_info);
+}
+
+static int fpga_region_test_init(struct kunit *test)
+{
+	struct test_ctx *ctx;
+	struct fpga_region_info region_info = { 0 };
+
+	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
+
+	ctx->mgr_pdev = platform_device_register_simple("mgr_pdev", PLATFORM_DEVID_AUTO, NULL, 0);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->mgr_pdev);
+
+	ctx->mgr = devm_fpga_mgr_register(&ctx->mgr_pdev->dev, "Fake FPGA Manager", &fake_mgr_ops,
+					  &ctx->mgr_stats);
+	KUNIT_ASSERT_FALSE(test, IS_ERR_OR_NULL(ctx->mgr));
+
+	ctx->bridge_pdev = platform_device_register_simple("bridge_pdev", PLATFORM_DEVID_AUTO,
+							   NULL, 0);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->bridge_pdev);
+
+	ctx->bridge = fpga_bridge_register(&ctx->bridge_pdev->dev, "Fake FPGA Bridge",
+					   &fake_bridge_ops, &ctx->bridge_stats);
+	KUNIT_ASSERT_FALSE(test, IS_ERR_OR_NULL(ctx->bridge));
+
+	ctx->region_pdev = platform_device_register_simple("region_pdev", PLATFORM_DEVID_AUTO,
+							   NULL, 0);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->region_pdev);
+
+	region_info.mgr = ctx->mgr;
+	region_info.priv = ctx->bridge;
+	region_info.get_bridges = fake_region_get_bridges;
+
+	ctx->region = fpga_region_register_full(&ctx->region_pdev->dev, &region_info);
+	KUNIT_ASSERT_FALSE(test, IS_ERR_OR_NULL(ctx->region));
+
+	test->priv = ctx;
+
+	return 0;
+}
+
+static void fpga_region_test_exit(struct kunit *test)
+{
+	struct test_ctx *ctx = test->priv;
+
+	fpga_region_unregister(ctx->region);
+	platform_device_unregister(ctx->region_pdev);
+
+	fpga_bridge_unregister(ctx->bridge);
+	platform_device_unregister(ctx->bridge_pdev);
+
+	platform_device_unregister(ctx->mgr_pdev);
+}
+
+static struct kunit_case fpga_region_test_cases[] = {
+	KUNIT_CASE(fpga_region_test_class_find),
+	KUNIT_CASE(fpga_region_test_program_fpga),
+
+	{}
+};
+
+static struct kunit_suite fpga_region_suite = {
+	.name = "fpga_mgr",
+	.init = fpga_region_test_init,
+	.exit = fpga_region_test_exit,
+	.test_cases = fpga_region_test_cases,
+};
+
+kunit_test_suite(fpga_region_suite);
+
+MODULE_LICENSE("GPL");
-- 
2.40.1

[RFC PATCH v6 4/4] fpga: add configuration for the KUnit test suites.

[Marco Pagani]

Signed-off-by: Marco Pagani <marpagan@redhat.com>
---
 drivers/fpga/Kconfig            |  2 ++
 drivers/fpga/Makefile           |  3 +++
 drivers/fpga/tests/.kunitconfig |  5 +++++
 drivers/fpga/tests/Kconfig      | 11 +++++++++++
 drivers/fpga/tests/Makefile     |  5 +++++
 5 files changed, 26 insertions(+)
 create mode 100644 drivers/fpga/tests/.kunitconfig
 create mode 100644 drivers/fpga/tests/Kconfig
 create mode 100644 drivers/fpga/tests/Makefile

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..e4a64815f16d
--- /dev/null
+++ b/drivers/fpga/tests/Kconfig
@@ -0,0 +1,11 @@
+config FPGA_KUNIT_TESTS
+	tristate "KUnit test for the FPGA subsystem" if !KUNIT_ALL_TESTS
+	depends on FPGA && FPGA_REGION && FPGA_BRIDGE && KUNIT=y
+	default KUNIT_ALL_TESTS
+        help
+          This builds unit tests for the FPGA subsystem
+
+          For more information on KUnit and unit tests in general,
+          please refer to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+          If unsure, say N.
diff --git a/drivers/fpga/tests/Makefile b/drivers/fpga/tests/Makefile
new file mode 100644
index 000000000000..faa5fa230ab0
--- /dev/null
+++ b/drivers/fpga/tests/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_FPGA_KUNIT_TESTS) += fpga-mgr-test.o
+obj-$(CONFIG_FPGA_KUNIT_TESTS) += fpga-bridge-test.o
+obj-$(CONFIG_FPGA_KUNIT_TESTS) += fpga-region-test.o
-- 
2.40.1

Re: [RFC PATCH v6 3/4] fpga: add an initial KUnit suite for the FPGA Region

[Xu Yilun]

On 2023-05-31 at 11:54:04 +0200, Marco Pagani wrote:
> The suite tests the programming of an FPGA Region with a Bridge
> and the function for finding a particular Region.
> 
> Signed-off-by: Marco Pagani <marpagan@redhat.com>
> ---
>  drivers/fpga/tests/fpga-region-test.c | 186 ++++++++++++++++++++++++++
>  1 file changed, 186 insertions(+)
>  create mode 100644 drivers/fpga/tests/fpga-region-test.c
> 
> diff --git a/drivers/fpga/tests/fpga-region-test.c b/drivers/fpga/tests/fpga-region-test.c
> new file mode 100644
> index 000000000000..81b271088240
> --- /dev/null
> +++ b/drivers/fpga/tests/fpga-region-test.c
> @@ -0,0 +1,186 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit test for the FPGA Region
> + *
> + * Copyright (C) 2023 Red Hat, Inc.
> + *
> + * Author: Marco Pagani <marpagan@redhat.com>
> + */
> +
> +#include <linux/types.h>
> +#include <linux/module.h>
> +#include <kunit/test.h>
> +#include <linux/platform_device.h>
> +#include <linux/fpga/fpga-mgr.h>
> +#include <linux/fpga/fpga-bridge.h>
> +#include <linux/fpga/fpga-region.h>
> +
> +struct mgr_stats {
> +	u32 write_count;
> +};
> +
> +struct bridge_stats {
> +	u32 enable_count;
> +};
> +
> +struct test_ctx {
> +	struct fpga_manager *mgr;
> +	struct platform_device *mgr_pdev;
> +	struct fpga_bridge *bridge;
> +	struct platform_device *bridge_pdev;
> +	struct fpga_region *region;
> +	struct platform_device *region_pdev;
> +	struct bridge_stats bridge_stats;
> +	struct mgr_stats mgr_stats;
> +};
> +
> +static int op_write(struct fpga_manager *mgr, const char *buf, size_t count)
> +{
> +	struct mgr_stats *stats = mgr->priv;
> +
> +	stats->write_count++;
> +
> +	return 0;
> +}
> +
> +static const struct fpga_manager_ops fake_mgr_ops = {
> +	.write = op_write,
> +};

Maybe better just put all tests in one module, and have unified
fake_mgr_ops/mgr_stats/fake_bridge_ops/bridge_stats across all tests.

In previous thread, I said I'm good to the self-contained test module
but I didn't actually follow the idea. Sorry for that.

The concern is why in this region test, the write_count and only the
write_count is taken care of.

Although fpga_mgr_load() test covers all mgr_ops, but does that
means these ops are still good for more complex case like
fpga_region_program_fpga()? And there is no guarantee
fpga_region_program_fpga() would always call mgr_ops the same way
as fpga_mgr_load() in future.

Similar for fpga_bridge. Maybe a complete setup for fpga_region is
still necessary.

BTW: I like the way that fake drivers are removed. Looks much straight
forward.

Thanks,
Yilun

> +
> +static int op_enable_set(struct fpga_bridge *bridge, bool enable)
> +{
> +	struct bridge_stats *stats = bridge->priv;
> +
> +	if (enable)
> +		stats->enable_count++;
> +
> +	return 0;
> +}
> +
> +static const struct fpga_bridge_ops fake_bridge_ops = {
> +	.enable_set = op_enable_set
> +};
> +
> +static int fake_region_get_bridges(struct fpga_region *region)
> +{
> +	struct fpga_bridge *bridge = region->priv;
> +
> +	return fpga_bridge_get_to_list(bridge->dev.parent, region->info, &region->bridge_list);
> +}
> +
> +static int fake_region_match(struct device *dev, const void *data)
> +{
> +	return dev->parent == data;
> +}
> +
> +static void fpga_region_test_class_find(struct kunit *test)
> +{
> +	struct test_ctx *ctx = test->priv;
> +	struct fpga_region *region;
> +
> +	region = fpga_region_class_find(NULL, &ctx->region_pdev->dev, fake_region_match);
> +	KUNIT_EXPECT_PTR_EQ(test, region, ctx->region);
> +}
> +
> +static void fpga_region_test_program_fpga(struct kunit *test)
> +{
> +	struct test_ctx *ctx = test->priv;
> +	struct fpga_image_info *img_info;
> +	char img_buf[4];
> +	int ret;
> +
> +	img_info = fpga_image_info_alloc(&ctx->mgr_pdev->dev);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, img_info);
> +
> +	img_info->buf = img_buf;
> +	img_info->count = sizeof(img_buf);
> +
> +	ctx->region->info = img_info;
> +	ret = fpga_region_program_fpga(ctx->region);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	KUNIT_EXPECT_EQ(test, 1, ctx->mgr_stats.write_count);
> +	KUNIT_EXPECT_EQ(test, 1, ctx->bridge_stats.enable_count);
> +
> +	fpga_bridges_put(&ctx->region->bridge_list);
> +
> +	ret = fpga_region_program_fpga(ctx->region);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	KUNIT_EXPECT_EQ(test, 2, ctx->mgr_stats.write_count);
> +	KUNIT_EXPECT_EQ(test, 2, ctx->bridge_stats.enable_count);
> +
> +	fpga_bridges_put(&ctx->region->bridge_list);
> +
> +	fpga_image_info_free(img_info);
> +}
> +
> +static int fpga_region_test_init(struct kunit *test)
> +{
> +	struct test_ctx *ctx;
> +	struct fpga_region_info region_info = { 0 };
> +
> +	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> +
> +	ctx->mgr_pdev = platform_device_register_simple("mgr_pdev", PLATFORM_DEVID_AUTO, NULL, 0);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->mgr_pdev);
> +
> +	ctx->mgr = devm_fpga_mgr_register(&ctx->mgr_pdev->dev, "Fake FPGA Manager", &fake_mgr_ops,
> +					  &ctx->mgr_stats);
> +	KUNIT_ASSERT_FALSE(test, IS_ERR_OR_NULL(ctx->mgr));
> +
> +	ctx->bridge_pdev = platform_device_register_simple("bridge_pdev", PLATFORM_DEVID_AUTO,
> +							   NULL, 0);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->bridge_pdev);
> +
> +	ctx->bridge = fpga_bridge_register(&ctx->bridge_pdev->dev, "Fake FPGA Bridge",
> +					   &fake_bridge_ops, &ctx->bridge_stats);
> +	KUNIT_ASSERT_FALSE(test, IS_ERR_OR_NULL(ctx->bridge));
> +
> +	ctx->region_pdev = platform_device_register_simple("region_pdev", PLATFORM_DEVID_AUTO,
> +							   NULL, 0);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->region_pdev);
> +
> +	region_info.mgr = ctx->mgr;
> +	region_info.priv = ctx->bridge;
> +	region_info.get_bridges = fake_region_get_bridges;
> +
> +	ctx->region = fpga_region_register_full(&ctx->region_pdev->dev, &region_info);
> +	KUNIT_ASSERT_FALSE(test, IS_ERR_OR_NULL(ctx->region));
> +
> +	test->priv = ctx;
> +
> +	return 0;
> +}
> +
> +static void fpga_region_test_exit(struct kunit *test)
> +{
> +	struct test_ctx *ctx = test->priv;
> +
> +	fpga_region_unregister(ctx->region);
> +	platform_device_unregister(ctx->region_pdev);
> +
> +	fpga_bridge_unregister(ctx->bridge);
> +	platform_device_unregister(ctx->bridge_pdev);
> +
> +	platform_device_unregister(ctx->mgr_pdev);
> +}
> +
> +static struct kunit_case fpga_region_test_cases[] = {
> +	KUNIT_CASE(fpga_region_test_class_find),
> +	KUNIT_CASE(fpga_region_test_program_fpga),
> +
> +	{}
> +};
> +
> +static struct kunit_suite fpga_region_suite = {
> +	.name = "fpga_mgr",
> +	.init = fpga_region_test_init,
> +	.exit = fpga_region_test_exit,
> +	.test_cases = fpga_region_test_cases,
> +};
> +
> +kunit_test_suite(fpga_region_suite);
> +
> +MODULE_LICENSE("GPL");
> -- 
> 2.40.1
> 

Re: [RFC PATCH v6 3/4] fpga: add an initial KUnit suite for the FPGA Region

[Marco Pagani]

On 2023-06-03 21:11, Xu Yilun wrote:
> On 2023-05-31 at 11:54:04 +0200, Marco Pagani wrote:
>> The suite tests the programming of an FPGA Region with a Bridge
>> and the function for finding a particular Region.
>>
>> Signed-off-by: Marco Pagani <marpagan@redhat.com>
>> ---
>>  drivers/fpga/tests/fpga-region-test.c | 186 ++++++++++++++++++++++++++
>>  1 file changed, 186 insertions(+)
>>  create mode 100644 drivers/fpga/tests/fpga-region-test.c

[...]

 
> Maybe better just put all tests in one module, and have unified
> fake_mgr_ops/mgr_stats/fake_bridge_ops/bridge_stats across all tests.
> 
> In previous thread, I said I'm good to the self-contained test module
> but I didn't actually follow the idea. Sorry for that.
> 
> The concern is why in this region test, the write_count and only the
> write_count is taken care of.
> 
> Although fpga_mgr_load() test covers all mgr_ops, but does that
> means these ops are still good for more complex case like
> fpga_region_program_fpga()? And there is no guarantee
> fpga_region_program_fpga() would always call mgr_ops the same way
> as fpga_mgr_load() in future.
> 
> Similar for fpga_bridge. Maybe a complete setup for fpga_region is
> still necessary.

I think that putting all tests in a single module (like in previous
versions) goes against the principles of unit testing, making the
code more similar to an integration test.

Unit tests should be focused on a single behavior. The programming
test case included in the Region's suite should test only the behavior
of the Region itself. Specifically, that fpga_region_program_fpga() calls
get_bridges(), to get and control bridges, and then the Manager for the
actual programming.

The programming sequence itself is outside the responsibilities of the
Region, and its correctness is already ensured by the Manager suite.
Similarly, the correctness of the Bridge's methods used by the Region
for getting and controlling multiple bridges is already ensured by the
Bridge test suite.

For this reason, the Manager and Bridge fakes used in the Region suite
implement only the minimal set of operations necessary to ensure the
correctness of the Region's behavior. If I used a "full" Manager (and
tested all mgr_ops), then the test case would have become an integration
test rather than a unit test for the Region.
> BTW: I like the way that fake drivers are removed. Looks much straight
> forward.

I appreciate that.
 
> Thanks,
> Yilun
>

Thanks,
Marco

[...]

Re: [RFC PATCH v6 3/4] fpga: add an initial KUnit suite for the FPGA Region

[Xu Yilun]

On 2023-06-05 at 18:58:56 +0200, Marco Pagani wrote:
> 
> 
> On 2023-06-03 21:11, Xu Yilun wrote:
> > On 2023-05-31 at 11:54:04 +0200, Marco Pagani wrote:
> >> The suite tests the programming of an FPGA Region with a Bridge
> >> and the function for finding a particular Region.
> >>
> >> Signed-off-by: Marco Pagani <marpagan@redhat.com>
> >> ---
> >>  drivers/fpga/tests/fpga-region-test.c | 186 ++++++++++++++++++++++++++
> >>  1 file changed, 186 insertions(+)
> >>  create mode 100644 drivers/fpga/tests/fpga-region-test.c
> 
> [...]
> 
>  
> > Maybe better just put all tests in one module, and have unified
> > fake_mgr_ops/mgr_stats/fake_bridge_ops/bridge_stats across all tests.
> > 
> > In previous thread, I said I'm good to the self-contained test module
> > but I didn't actually follow the idea. Sorry for that.
> > 
> > The concern is why in this region test, the write_count and only the
> > write_count is taken care of.
> > 
> > Although fpga_mgr_load() test covers all mgr_ops, but does that
> > means these ops are still good for more complex case like
> > fpga_region_program_fpga()? And there is no guarantee
> > fpga_region_program_fpga() would always call mgr_ops the same way
> > as fpga_mgr_load() in future.
> > 
> > Similar for fpga_bridge. Maybe a complete setup for fpga_region is
> > still necessary.
> 
> I think that putting all tests in a single module (like in previous
> versions) goes against the principles of unit testing, making the
> code more similar to an integration test.
> 
> Unit tests should be focused on a single behavior. The programming
> test case included in the Region's suite should test only the behavior
> of the Region itself. Specifically, that fpga_region_program_fpga() calls
> get_bridges(), to get and control bridges, and then the Manager for the
> actual programming.
> 
> The programming sequence itself is outside the responsibilities of the
> Region, and its correctness is already ensured by the Manager suite.
> Similarly, the correctness of the Bridge's methods used by the Region
> for getting and controlling multiple bridges is already ensured by the
> Bridge test suite.
> 
> For this reason, the Manager and Bridge fakes used in the Region suite
> implement only the minimal set of operations necessary to ensure the
> correctness of the Region's behavior. If I used a "full" Manager (and
> tested all mgr_ops), then the test case would have become an integration
> test rather than a unit test for the Region.

I agree with you about a unit test should focus on a single behavior. But
I have concerns that each test suite uses different definitions of the
same structure, mgr/bridge stats, mgr/bridge ops, mgr/bridge ctx. Even
if we have full definitions for these structures to acommodate all
tests, it doesn't break the principle of unit test, just ignore the fields
and skip checks that you don't care. E.g. only checks mgr.write_count &
bridge.enable_count for region test.

And a single module simplifies the implementation.

struct mgr_stats {
	...
};

struct mgr_ctx {
	struct fpga_image_info *img_info;
	struct fpga_manager *mgr;
	struct platform_device *pdev;
	struct mgr_stats stats;
};

struct bridge_stats {
	...
};

struct bridge_ctx {
	struct fpga_bridge *bridge;
	struct platform_device *pdev;
	struct bridge_stats stats;
};

struct region_ctx {
	struct mgr_ctx mgr_ctx;
	struct bridge_ctx bridge_ctx;

	struct fpga_region *region;
	struct platform_device *region_pdev;
};

How do you think?

Thanks,
Yilun

> > BTW: I like the way that fake drivers are removed. Looks much straight
> > forward.
> 
> I appreciate that.
>  
> > Thanks,
> > Yilun
> >
> 
> Thanks,
> Marco
> 
> [...]
> 

Re: [RFC PATCH v6 3/4] fpga: add an initial KUnit suite for the FPGA Region

[Marco Pagani]

On 2023-06-06 13:00, Xu Yilun wrote:
> On 2023-06-05 at 18:58:56 +0200, Marco Pagani wrote:
>>
>>
>> On 2023-06-03 21:11, Xu Yilun wrote:
>>> On 2023-05-31 at 11:54:04 +0200, Marco Pagani wrote:
>>>> The suite tests the programming of an FPGA Region with a Bridge
>>>> and the function for finding a particular Region.
>>>>
>>>> Signed-off-by: Marco Pagani <marpagan@redhat.com>
>>>> ---
>>>>  drivers/fpga/tests/fpga-region-test.c | 186 ++++++++++++++++++++++++++
>>>>  1 file changed, 186 insertions(+)
>>>>  create mode 100644 drivers/fpga/tests/fpga-region-test.c
>>
>> [...]
>>
>>  
>>> Maybe better just put all tests in one module, and have unified
>>> fake_mgr_ops/mgr_stats/fake_bridge_ops/bridge_stats across all tests.
>>>
>>> In previous thread, I said I'm good to the self-contained test module
>>> but I didn't actually follow the idea. Sorry for that.
>>>
>>> The concern is why in this region test, the write_count and only the
>>> write_count is taken care of.
>>>
>>> Although fpga_mgr_load() test covers all mgr_ops, but does that
>>> means these ops are still good for more complex case like
>>> fpga_region_program_fpga()? And there is no guarantee
>>> fpga_region_program_fpga() would always call mgr_ops the same way
>>> as fpga_mgr_load() in future.
>>>
>>> Similar for fpga_bridge. Maybe a complete setup for fpga_region is
>>> still necessary.
>>
>> I think that putting all tests in a single module (like in previous
>> versions) goes against the principles of unit testing, making the
>> code more similar to an integration test.
>>
>> Unit tests should be focused on a single behavior. The programming
>> test case included in the Region's suite should test only the behavior
>> of the Region itself. Specifically, that fpga_region_program_fpga() calls
>> get_bridges(), to get and control bridges, and then the Manager for the
>> actual programming.
>>
>> The programming sequence itself is outside the responsibilities of the
>> Region, and its correctness is already ensured by the Manager suite.
>> Similarly, the correctness of the Bridge's methods used by the Region
>> for getting and controlling multiple bridges is already ensured by the
>> Bridge test suite.
>>
>> For this reason, the Manager and Bridge fakes used in the Region suite
>> implement only the minimal set of operations necessary to ensure the
>> correctness of the Region's behavior. If I used a "full" Manager (and
>> tested all mgr_ops), then the test case would have become an integration
>> test rather than a unit test for the Region.
> 
> I agree with you about a unit test should focus on a single behavior. But
> I have concerns that each test suite uses different definitions of the
> same structure, mgr/bridge stats, mgr/bridge ops, mgr/bridge ctx. Even
> if we have full definitions for these structures to acommodate all
> tests, it doesn't break the principle of unit test, just ignore the fields
> and skip checks that you don't care. E.g. only checks mgr.write_count &
> bridge.enable_count for region test.
> 
> And a single module simplifies the implementation.
> 
> struct mgr_stats {
> 	...
> };
> 
> struct mgr_ctx {
> 	struct fpga_image_info *img_info;
> 	struct fpga_manager *mgr;
> 	struct platform_device *pdev;
> 	struct mgr_stats stats;
> };
> 
> struct bridge_stats {
> 	...
> };
> 
> struct bridge_ctx {
> 	struct fpga_bridge *bridge;
> 	struct platform_device *pdev;
> 	struct bridge_stats stats;
> };
> 
> struct region_ctx {
> 	struct mgr_ctx mgr_ctx;
> 	struct bridge_ctx bridge_ctx;
> 
> 	struct fpga_region *region;
> 	struct platform_device *region_pdev;
> };
> 
> How do you think?
> 
> Thanks,
> Yilun
>

My concern with unified fakes having the same ops, stats, and context structs
is that they would couple the test suites together. I think it's better to
have multiple fakes, each with the single responsibility of providing minimal
support for the component under test. Otherwise, we would end up having
overcomplicated fakes that implement the union (in the set theory sense of
the term) of all behaviors tested by all suites. By using these fakes, some
test cases might implicitly exercise behaviors that are outside their scope
(e.g., the Region programming test case calling all Manager ops). I feel
this would go against the principle of limiting the amount of code under test
to a single unit. 
Thanks,
Marco

>>> BTW: I like the way that fake drivers are removed. Looks much straight
>>> forward.
>>
>> I appreciate that.
>>  
>>> Thanks,
>>> Yilun
>>>
>>
>> Thanks,
>> Marco
>>
>> [...]
>>
> 

Re: [RFC PATCH v6 3/4] fpga: add an initial KUnit suite for the FPGA Region

[Marco Pagani]

On 2023-06-09 13:09, Xu Yilun wrote:
> On 2023-06-07 at 17:57:22 +0200, Marco Pagani wrote:
>>
>>
>> On 2023-06-06 13:00, Xu Yilun wrote:
>>> On 2023-06-05 at 18:58:56 +0200, Marco Pagani wrote:
>>>>
>>>>
>>>> On 2023-06-03 21:11, Xu Yilun wrote:
>>>>> On 2023-05-31 at 11:54:04 +0200, Marco Pagani wrote:
>>>>>> The suite tests the programming of an FPGA Region with a Bridge
>>>>>> and the function for finding a particular Region.
>>>>>>
>>>>>> Signed-off-by: Marco Pagani <marpagan@redhat.com>
>>>>>> ---
>>>>>>  drivers/fpga/tests/fpga-region-test.c | 186 ++++++++++++++++++++++++++
>>>>>>  1 file changed, 186 insertions(+)
>>>>>>  create mode 100644 drivers/fpga/tests/fpga-region-test.c
>>>>
>>>> [...]
>>>>
>>>>  
>>>>> Maybe better just put all tests in one module, and have unified
>>>>> fake_mgr_ops/mgr_stats/fake_bridge_ops/bridge_stats across all tests.
>>>>>
>>>>> In previous thread, I said I'm good to the self-contained test module
>>>>> but I didn't actually follow the idea. Sorry for that.
>>>>>
>>>>> The concern is why in this region test, the write_count and only the
>>>>> write_count is taken care of.
>>>>>
>>>>> Although fpga_mgr_load() test covers all mgr_ops, but does that
>>>>> means these ops are still good for more complex case like
>>>>> fpga_region_program_fpga()? And there is no guarantee
>>>>> fpga_region_program_fpga() would always call mgr_ops the same way
>>>>> as fpga_mgr_load() in future.
>>>>>
>>>>> Similar for fpga_bridge. Maybe a complete setup for fpga_region is
>>>>> still necessary.
>>>>
>>>> I think that putting all tests in a single module (like in previous
>>>> versions) goes against the principles of unit testing, making the
>>>> code more similar to an integration test.
>>>>
>>>> Unit tests should be focused on a single behavior. The programming
>>>> test case included in the Region's suite should test only the behavior
>>>> of the Region itself. Specifically, that fpga_region_program_fpga() calls
>>>> get_bridges(), to get and control bridges, and then the Manager for the
>>>> actual programming.
>>>>
>>>> The programming sequence itself is outside the responsibilities of the
>>>> Region, and its correctness is already ensured by the Manager suite.
>>>> Similarly, the correctness of the Bridge's methods used by the Region
>>>> for getting and controlling multiple bridges is already ensured by the
>>>> Bridge test suite.
>>>>
>>>> For this reason, the Manager and Bridge fakes used in the Region suite
>>>> implement only the minimal set of operations necessary to ensure the
>>>> correctness of the Region's behavior. If I used a "full" Manager (and
>>>> tested all mgr_ops), then the test case would have become an integration
>>>> test rather than a unit test for the Region.
>>>
>>> I agree with you about a unit test should focus on a single behavior. But
>>> I have concerns that each test suite uses different definitions of the
>>> same structure, mgr/bridge stats, mgr/bridge ops, mgr/bridge ctx. Even
>>> if we have full definitions for these structures to acommodate all
>>> tests, it doesn't break the principle of unit test, just ignore the fields
>>> and skip checks that you don't care. E.g. only checks mgr.write_count &
>>> bridge.enable_count for region test.
>>>
>>> And a single module simplifies the implementation.
>>>
>>> struct mgr_stats {
>>> 	...
>>> };
>>>
>>> struct mgr_ctx {
>>> 	struct fpga_image_info *img_info;
>>> 	struct fpga_manager *mgr;
>>> 	struct platform_device *pdev;
>>> 	struct mgr_stats stats;
>>> };
>>>
>>> struct bridge_stats {
>>> 	...
>>> };
>>>
>>> struct bridge_ctx {
>>> 	struct fpga_bridge *bridge;
>>> 	struct platform_device *pdev;
>>> 	struct bridge_stats stats;
>>> };
>>>
>>> struct region_ctx {
>>> 	struct mgr_ctx mgr_ctx;
>>> 	struct bridge_ctx bridge_ctx;
>>>
>>> 	struct fpga_region *region;
>>> 	struct platform_device *region_pdev;
>>> };
>>>
>>> How do you think?
>>>
>>> Thanks,
>>> Yilun
>>>
>>
>> My concern with unified fakes having the same ops, stats, and context structs
>> is that they would couple the test suites together. I think it's better to
>> have multiple fakes, each with the single responsibility of providing minimal
>> support for the component under test. Otherwise, we would end up having
>> overcomplicated fakes that implement the union (in the set theory sense of
>> the term) of all behaviors tested by all suites. By using these fakes, some
>> test cases might implicitly exercise behaviors that are outside their scope
>> (e.g., the Region programming test case calling all Manager ops). I feel
>> this would go against the principle of limiting the amount of code under test
>> to a single unit. 
> 
> OK. On second thought, it is good to me.
> 
> I think now the high level opens are all addressed. Will you keep on
> improving the patchset or make it stable for upstream? If the later, you
> may drop the RFC prefix.

I plan to stabilize the patchset for the upstream.

Thanks,
Marco

> 
> Thanks,
> Yilun
> 
>> Thanks,
>> Marco
>>
>>>>> BTW: I like the way that fake drivers are removed. Looks much straight
>>>>> forward.
>>>>
>>>> I appreciate that.
>>>>  
>>>>> Thanks,
>>>>> Yilun
>>>>>
>>>>
>>>> Thanks,
>>>> Marco
>>>>
>>>> [...]
>>>>
>>>
>>
> 

Re: [RFC PATCH v6 3/4] fpga: add an initial KUnit suite for the FPGA Region

[Marco Pagani]

On 2023-06-09 13:22, Xu Yilun wrote:
> On 2023-05-31 at 11:54:04 +0200, Marco Pagani wrote:
>> The suite tests the programming of an FPGA Region with a Bridge
>> and the function for finding a particular Region.
>>
>> Signed-off-by: Marco Pagani <marpagan@redhat.com>
>> ---
>>  drivers/fpga/tests/fpga-region-test.c | 186 ++++++++++++++++++++++++++
>>  1 file changed, 186 insertions(+)
>>  create mode 100644 drivers/fpga/tests/fpga-region-test.c
>>
>> diff --git a/drivers/fpga/tests/fpga-region-test.c b/drivers/fpga/tests/fpga-region-test.c
>> new file mode 100644
>> index 000000000000..81b271088240
>> --- /dev/null
>> +++ b/drivers/fpga/tests/fpga-region-test.c
>> @@ -0,0 +1,186 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * KUnit test for the FPGA Region
>> + *
>> + * Copyright (C) 2023 Red Hat, Inc.
>> + *
>> + * Author: Marco Pagani <marpagan@redhat.com>
>> + */
>> +
>> +#include <linux/types.h>
>> +#include <linux/module.h>
>> +#include <kunit/test.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/fpga/fpga-mgr.h>
>> +#include <linux/fpga/fpga-bridge.h>
>> +#include <linux/fpga/fpga-region.h>
>> +
>> +struct mgr_stats {
>> +	u32 write_count;
>> +};
>> +
>> +struct bridge_stats {
>> +	u32 enable_count;
>> +};
>> +
>> +struct test_ctx {
>> +	struct fpga_manager *mgr;
>> +	struct platform_device *mgr_pdev;
>> +	struct fpga_bridge *bridge;
>> +	struct platform_device *bridge_pdev;
>> +	struct fpga_region *region;
>> +	struct platform_device *region_pdev;
>> +	struct bridge_stats bridge_stats;
>> +	struct mgr_stats mgr_stats;
>> +};
>> +
>> +static int op_write(struct fpga_manager *mgr, const char *buf, size_t count)
>> +{
>> +	struct mgr_stats *stats = mgr->priv;
>> +
>> +	stats->write_count++;
> 
> Could you add some comments to explain why only implement the write op
> to help region test? and why not choose write_complete or other callback?

Sure, I'll do that in the next version.

Thanks,
Marco


> Thanks,
> Yilun
>

[...]

Re: [RFC PATCH v6 3/4] fpga: add an initial KUnit suite for the FPGA Region

[Xu Yilun]

On 2023-06-07 at 17:57:22 +0200, Marco Pagani wrote:
> 
> 
> On 2023-06-06 13:00, Xu Yilun wrote:
> > On 2023-06-05 at 18:58:56 +0200, Marco Pagani wrote:
> >>
> >>
> >> On 2023-06-03 21:11, Xu Yilun wrote:
> >>> On 2023-05-31 at 11:54:04 +0200, Marco Pagani wrote:
> >>>> The suite tests the programming of an FPGA Region with a Bridge
> >>>> and the function for finding a particular Region.
> >>>>
> >>>> Signed-off-by: Marco Pagani <marpagan@redhat.com>
> >>>> ---
> >>>>  drivers/fpga/tests/fpga-region-test.c | 186 ++++++++++++++++++++++++++
> >>>>  1 file changed, 186 insertions(+)
> >>>>  create mode 100644 drivers/fpga/tests/fpga-region-test.c
> >>
> >> [...]
> >>
> >>  
> >>> Maybe better just put all tests in one module, and have unified
> >>> fake_mgr_ops/mgr_stats/fake_bridge_ops/bridge_stats across all tests.
> >>>
> >>> In previous thread, I said I'm good to the self-contained test module
> >>> but I didn't actually follow the idea. Sorry for that.
> >>>
> >>> The concern is why in this region test, the write_count and only the
> >>> write_count is taken care of.
> >>>
> >>> Although fpga_mgr_load() test covers all mgr_ops, but does that
> >>> means these ops are still good for more complex case like
> >>> fpga_region_program_fpga()? And there is no guarantee
> >>> fpga_region_program_fpga() would always call mgr_ops the same way
> >>> as fpga_mgr_load() in future.
> >>>
> >>> Similar for fpga_bridge. Maybe a complete setup for fpga_region is
> >>> still necessary.
> >>
> >> I think that putting all tests in a single module (like in previous
> >> versions) goes against the principles of unit testing, making the
> >> code more similar to an integration test.
> >>
> >> Unit tests should be focused on a single behavior. The programming
> >> test case included in the Region's suite should test only the behavior
> >> of the Region itself. Specifically, that fpga_region_program_fpga() calls
> >> get_bridges(), to get and control bridges, and then the Manager for the
> >> actual programming.
> >>
> >> The programming sequence itself is outside the responsibilities of the
> >> Region, and its correctness is already ensured by the Manager suite.
> >> Similarly, the correctness of the Bridge's methods used by the Region
> >> for getting and controlling multiple bridges is already ensured by the
> >> Bridge test suite.
> >>
> >> For this reason, the Manager and Bridge fakes used in the Region suite
> >> implement only the minimal set of operations necessary to ensure the
> >> correctness of the Region's behavior. If I used a "full" Manager (and
> >> tested all mgr_ops), then the test case would have become an integration
> >> test rather than a unit test for the Region.
> > 
> > I agree with you about a unit test should focus on a single behavior. But
> > I have concerns that each test suite uses different definitions of the
> > same structure, mgr/bridge stats, mgr/bridge ops, mgr/bridge ctx. Even
> > if we have full definitions for these structures to acommodate all
> > tests, it doesn't break the principle of unit test, just ignore the fields
> > and skip checks that you don't care. E.g. only checks mgr.write_count &
> > bridge.enable_count for region test.
> > 
> > And a single module simplifies the implementation.
> > 
> > struct mgr_stats {
> > 	...
> > };
> > 
> > struct mgr_ctx {
> > 	struct fpga_image_info *img_info;
> > 	struct fpga_manager *mgr;
> > 	struct platform_device *pdev;
> > 	struct mgr_stats stats;
> > };
> > 
> > struct bridge_stats {
> > 	...
> > };
> > 
> > struct bridge_ctx {
> > 	struct fpga_bridge *bridge;
> > 	struct platform_device *pdev;
> > 	struct bridge_stats stats;
> > };
> > 
> > struct region_ctx {
> > 	struct mgr_ctx mgr_ctx;
> > 	struct bridge_ctx bridge_ctx;
> > 
> > 	struct fpga_region *region;
> > 	struct platform_device *region_pdev;
> > };
> > 
> > How do you think?
> > 
> > Thanks,
> > Yilun
> >
> 
> My concern with unified fakes having the same ops, stats, and context structs
> is that they would couple the test suites together. I think it's better to
> have multiple fakes, each with the single responsibility of providing minimal
> support for the component under test. Otherwise, we would end up having
> overcomplicated fakes that implement the union (in the set theory sense of
> the term) of all behaviors tested by all suites. By using these fakes, some
> test cases might implicitly exercise behaviors that are outside their scope
> (e.g., the Region programming test case calling all Manager ops). I feel
> this would go against the principle of limiting the amount of code under test
> to a single unit. 

OK. On second thought, it is good to me.

I think now the high level opens are all addressed. Will you keep on
improving the patchset or make it stable for upstream? If the later, you
may drop the RFC prefix.

Thanks,
Yilun

> Thanks,
> Marco
> 
> >>> BTW: I like the way that fake drivers are removed. Looks much straight
> >>> forward.
> >>
> >> I appreciate that.
> >>  
> >>> Thanks,
> >>> Yilun
> >>>
> >>
> >> Thanks,
> >> Marco
> >>
> >> [...]
> >>
> > 
> 

Re: [RFC PATCH v6 3/4] fpga: add an initial KUnit suite for the FPGA Region

[Xu Yilun]

On 2023-05-31 at 11:54:04 +0200, Marco Pagani wrote:
> The suite tests the programming of an FPGA Region with a Bridge
> and the function for finding a particular Region.
> 
> Signed-off-by: Marco Pagani <marpagan@redhat.com>
> ---
>  drivers/fpga/tests/fpga-region-test.c | 186 ++++++++++++++++++++++++++
>  1 file changed, 186 insertions(+)
>  create mode 100644 drivers/fpga/tests/fpga-region-test.c
> 
> diff --git a/drivers/fpga/tests/fpga-region-test.c b/drivers/fpga/tests/fpga-region-test.c
> new file mode 100644
> index 000000000000..81b271088240
> --- /dev/null
> +++ b/drivers/fpga/tests/fpga-region-test.c
> @@ -0,0 +1,186 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit test for the FPGA Region
> + *
> + * Copyright (C) 2023 Red Hat, Inc.
> + *
> + * Author: Marco Pagani <marpagan@redhat.com>
> + */
> +
> +#include <linux/types.h>
> +#include <linux/module.h>
> +#include <kunit/test.h>
> +#include <linux/platform_device.h>
> +#include <linux/fpga/fpga-mgr.h>
> +#include <linux/fpga/fpga-bridge.h>
> +#include <linux/fpga/fpga-region.h>
> +
> +struct mgr_stats {
> +	u32 write_count;
> +};
> +
> +struct bridge_stats {
> +	u32 enable_count;
> +};
> +
> +struct test_ctx {
> +	struct fpga_manager *mgr;
> +	struct platform_device *mgr_pdev;
> +	struct fpga_bridge *bridge;
> +	struct platform_device *bridge_pdev;
> +	struct fpga_region *region;
> +	struct platform_device *region_pdev;
> +	struct bridge_stats bridge_stats;
> +	struct mgr_stats mgr_stats;
> +};
> +
> +static int op_write(struct fpga_manager *mgr, const char *buf, size_t count)
> +{
> +	struct mgr_stats *stats = mgr->priv;
> +
> +	stats->write_count++;

Could you add some comments to explain why only implement the write op
to help region test? and why not choose write_complete or other callback?

Thanks,
Yilun

> +
> +	return 0;
> +}
> +
> +static const struct fpga_manager_ops fake_mgr_ops = {
> +	.write = op_write,
> +};
> +
> +static int op_enable_set(struct fpga_bridge *bridge, bool enable)
> +{
> +	struct bridge_stats *stats = bridge->priv;
> +
> +	if (enable)
> +		stats->enable_count++;
> +
> +	return 0;
> +}
> +
> +static const struct fpga_bridge_ops fake_bridge_ops = {
> +	.enable_set = op_enable_set
> +};
> +
> +static int fake_region_get_bridges(struct fpga_region *region)
> +{
> +	struct fpga_bridge *bridge = region->priv;
> +
> +	return fpga_bridge_get_to_list(bridge->dev.parent, region->info, &region->bridge_list);
> +}
> +
> +static int fake_region_match(struct device *dev, const void *data)
> +{
> +	return dev->parent == data;
> +}
> +
> +static void fpga_region_test_class_find(struct kunit *test)
> +{
> +	struct test_ctx *ctx = test->priv;
> +	struct fpga_region *region;
> +
> +	region = fpga_region_class_find(NULL, &ctx->region_pdev->dev, fake_region_match);
> +	KUNIT_EXPECT_PTR_EQ(test, region, ctx->region);
> +}
> +
> +static void fpga_region_test_program_fpga(struct kunit *test)
> +{
> +	struct test_ctx *ctx = test->priv;
> +	struct fpga_image_info *img_info;
> +	char img_buf[4];
> +	int ret;
> +
> +	img_info = fpga_image_info_alloc(&ctx->mgr_pdev->dev);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, img_info);
> +
> +	img_info->buf = img_buf;
> +	img_info->count = sizeof(img_buf);
> +
> +	ctx->region->info = img_info;
> +	ret = fpga_region_program_fpga(ctx->region);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	KUNIT_EXPECT_EQ(test, 1, ctx->mgr_stats.write_count);
> +	KUNIT_EXPECT_EQ(test, 1, ctx->bridge_stats.enable_count);
> +
> +	fpga_bridges_put(&ctx->region->bridge_list);
> +
> +	ret = fpga_region_program_fpga(ctx->region);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	KUNIT_EXPECT_EQ(test, 2, ctx->mgr_stats.write_count);
> +	KUNIT_EXPECT_EQ(test, 2, ctx->bridge_stats.enable_count);
> +
> +	fpga_bridges_put(&ctx->region->bridge_list);
> +
> +	fpga_image_info_free(img_info);
> +}
> +
> +static int fpga_region_test_init(struct kunit *test)
> +{
> +	struct test_ctx *ctx;
> +	struct fpga_region_info region_info = { 0 };
> +
> +	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> +
> +	ctx->mgr_pdev = platform_device_register_simple("mgr_pdev", PLATFORM_DEVID_AUTO, NULL, 0);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->mgr_pdev);
> +
> +	ctx->mgr = devm_fpga_mgr_register(&ctx->mgr_pdev->dev, "Fake FPGA Manager", &fake_mgr_ops,
> +					  &ctx->mgr_stats);
> +	KUNIT_ASSERT_FALSE(test, IS_ERR_OR_NULL(ctx->mgr));
> +
> +	ctx->bridge_pdev = platform_device_register_simple("bridge_pdev", PLATFORM_DEVID_AUTO,
> +							   NULL, 0);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->bridge_pdev);
> +
> +	ctx->bridge = fpga_bridge_register(&ctx->bridge_pdev->dev, "Fake FPGA Bridge",
> +					   &fake_bridge_ops, &ctx->bridge_stats);
> +	KUNIT_ASSERT_FALSE(test, IS_ERR_OR_NULL(ctx->bridge));
> +
> +	ctx->region_pdev = platform_device_register_simple("region_pdev", PLATFORM_DEVID_AUTO,
> +							   NULL, 0);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->region_pdev);
> +
> +	region_info.mgr = ctx->mgr;
> +	region_info.priv = ctx->bridge;
> +	region_info.get_bridges = fake_region_get_bridges;
> +
> +	ctx->region = fpga_region_register_full(&ctx->region_pdev->dev, &region_info);
> +	KUNIT_ASSERT_FALSE(test, IS_ERR_OR_NULL(ctx->region));
> +
> +	test->priv = ctx;
> +
> +	return 0;
> +}
> +
> +static void fpga_region_test_exit(struct kunit *test)
> +{
> +	struct test_ctx *ctx = test->priv;
> +
> +	fpga_region_unregister(ctx->region);
> +	platform_device_unregister(ctx->region_pdev);
> +
> +	fpga_bridge_unregister(ctx->bridge);
> +	platform_device_unregister(ctx->bridge_pdev);
> +
> +	platform_device_unregister(ctx->mgr_pdev);
> +}
> +
> +static struct kunit_case fpga_region_test_cases[] = {
> +	KUNIT_CASE(fpga_region_test_class_find),
> +	KUNIT_CASE(fpga_region_test_program_fpga),
> +
> +	{}
> +};
> +
> +static struct kunit_suite fpga_region_suite = {
> +	.name = "fpga_mgr",
> +	.init = fpga_region_test_init,
> +	.exit = fpga_region_test_exit,
> +	.test_cases = fpga_region_test_cases,
> +};
> +
> +kunit_test_suite(fpga_region_suite);
> +
> +MODULE_LICENSE("GPL");
> -- 
> 2.40.1
>