[RFC PATCH 0/4] fpga: add initial KUnit test suite for the subsystem

[RFC PATCH 0/4] fpga: add initial KUnit test suite for the subsystem

[Marco Pagani]

This patch set introduces a KUnit suite to test the core components
of the FPGA subsystem. More specifically, the suite tests the core
functions of the FPGA manager, FPGA bridge, and FPGA region.

These components are tested using "fake" modules that allow
observing their internals without altering the source code.

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

Marco Pagani (4):
  fpga: add initial KUnit test suite
  fpga: add fake FPGA region
  fpga: add fake FPGA manager
  fpga: add fake FPGA bridge

 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/fake-fpga-bridge.c | 214 +++++++++++++++
 drivers/fpga/tests/fake-fpga-bridge.h |  36 +++
 drivers/fpga/tests/fake-fpga-mgr.c    | 365 ++++++++++++++++++++++++++
 drivers/fpga/tests/fake-fpga-mgr.h    |  42 +++
 drivers/fpga/tests/fake-fpga-region.c | 186 +++++++++++++
 drivers/fpga/tests/fake-fpga-region.h |  37 +++
 drivers/fpga/tests/fpga-tests.c       | 264 +++++++++++++++++++
 12 files changed, 1175 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/fake-fpga-bridge.c
 create mode 100644 drivers/fpga/tests/fake-fpga-bridge.h
 create mode 100644 drivers/fpga/tests/fake-fpga-mgr.c
 create mode 100644 drivers/fpga/tests/fake-fpga-mgr.h
 create mode 100644 drivers/fpga/tests/fake-fpga-region.c
 create mode 100644 drivers/fpga/tests/fake-fpga-region.h
 create mode 100644 drivers/fpga/tests/fpga-tests.c

-- 
2.39.1

[RFC PATCH 1/4] fpga: add initial KUnit test suite

[Marco Pagani]

Introduce an initial KUnit suite to test the core components of the
FPGA subsystem.

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@redhat.com>
---
 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
+obj-$(CONFIG_FPGA_KUNIT_TESTS) += fpga-tests.o
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@redhat.com>
+ */
+
+#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);
+
+	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);
+
+	test_img_info->sgt = &sgt_bit;
+
+	/* Re-program the fake FPGA using the image scatter list */
+	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_sg(&mgr_ctx);
+
+	KUNIT_EXPECT_EQ(test, 2, 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, 2, fake_fpga_bridge_get_cycles_count(&base_bridge_ctx));
+
+	free_sgt_bit(&sgt_bit);
+	fpga_image_info_free(test_img_info);
+	fpga_free_base_sys(&mgr_ctx, &base_bridge_ctx, &base_region_ctx);
+}
+
+static void fpga_pr_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 fake_fpga_bridge pr_bridge_0_ctx;
+	struct fake_fpga_bridge pr_bridge_1_ctx;
+	struct fake_fpga_region pr_region_ctx;
+
+	struct fpga_image_info *test_static_img_info;
+	struct fpga_image_info *test_pr_img_info;
+
+	fpga_build_base_sys(test, &mgr_ctx, &base_bridge_ctx, &base_region_ctx);
+
+	/* Allocate a fake test image using a buffer */
+	test_static_img_info = fpga_image_info_alloc(&mgr_ctx.pdev->dev);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_static_img_info);
+
+	test_static_img_info->buf = fake_bit;
+	test_static_img_info->count = sizeof(fake_bit);
+
+	kunit_info(test, "fake bitstream size: %zu\n", test_static_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_static_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));
+
+	/* The static image contains a reconfigurable sub-region with two soft bridges */
+	ret = fake_fpga_bridge_register(&pr_bridge_0_ctx, test);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = fake_fpga_bridge_register(&pr_bridge_1_ctx, test);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = fake_fpga_region_register(&pr_region_ctx, mgr_ctx.mgr, test);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = fake_fpga_region_add_bridge(&pr_region_ctx, pr_bridge_0_ctx.bridge);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = fake_fpga_region_add_bridge(&pr_region_ctx, pr_bridge_1_ctx.bridge);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	/* Allocate a fake partial test image using a buffer */
+	test_pr_img_info = fpga_image_info_alloc(&mgr_ctx.pdev->dev);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_pr_img_info);
+
+	test_pr_img_info->buf = fake_bit;
+	test_pr_img_info->count = sizeof(fake_bit) / 2;
+	test_pr_img_info->flags = FPGA_MGR_PARTIAL_RECONFIG;
+
+	kunit_info(test, "fake partial bitstream size: %zu\n", test_pr_img_info->count);
+
+	/* Program the reconfigurable sub-region */
+	pr_region_ctx.region->info = test_pr_img_info;
+	ret = fpga_region_program_fpga(pr_region_ctx.region);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	fake_fpga_mgr_check_write_buf(&mgr_ctx);
+
+	KUNIT_EXPECT_EQ(test, 2, fake_fpga_mgr_get_rcfg_count(&mgr_ctx));
+
+	KUNIT_EXPECT_EQ(test, 1, fake_fpga_bridge_get_state(&pr_bridge_0_ctx));
+	KUNIT_EXPECT_EQ(test, 1, fake_fpga_bridge_get_cycles_count(&pr_bridge_0_ctx));
+
+	KUNIT_EXPECT_EQ(test, 1, fake_fpga_bridge_get_state(&pr_bridge_1_ctx));
+	KUNIT_EXPECT_EQ(test, 1, fake_fpga_bridge_get_cycles_count(&pr_bridge_1_ctx));
+
+	/* Check that the base bridge has not been disabled */
+	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_pr_img_info);
+	fpga_image_info_free(test_static_img_info);
+
+	fake_fpga_region_unregister(&pr_region_ctx);
+	fake_fpga_bridge_unregister(&pr_bridge_0_ctx);
+	fake_fpga_bridge_unregister(&pr_bridge_1_ctx);
+
+	fpga_free_base_sys(&mgr_ctx, &base_bridge_ctx, &base_region_ctx);
+}
+
+static struct kunit_case fpga_test_cases[] = {
+	KUNIT_CASE(fpga_base_test),
+	KUNIT_CASE(fpga_pr_test),
+	{},
+};
+
+static struct kunit_suite fpga_test_suite = {
+	.name = "fpga-tests",
+	.suite_init = fpga_suite_init,
+	.test_cases = fpga_test_cases,
+};
+
+kunit_test_suite(fpga_test_suite);
-- 
2.39.1

[RFC PATCH 2/4] fpga: add fake FPGA region

[Marco Pagani]

Add fake FPGA region platform driver with support functions. This
module is part of the KUnit test suite for the FPGA subsystem.

Signed-off-by: Marco Pagani <marpagan@redhat.com>
---
 drivers/fpga/tests/fake-fpga-region.c | 186 ++++++++++++++++++++++++++
 drivers/fpga/tests/fake-fpga-region.h |  37 +++++
 2 files changed, 223 insertions(+)
 create mode 100644 drivers/fpga/tests/fake-fpga-region.c
 create mode 100644 drivers/fpga/tests/fake-fpga-region.h

diff --git a/drivers/fpga/tests/fake-fpga-region.c b/drivers/fpga/tests/fake-fpga-region.c
new file mode 100644
index 000000000000..095397e41837
--- /dev/null
+++ b/drivers/fpga/tests/fake-fpga-region.c
@@ -0,0 +1,186 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for fake FPGA region
+ *
+ * Copyright (C) 2023 Red Hat, Inc. All rights reserved.
+ *
+ * Author: Marco Pagani <marpagan@redhat.com>
+ */
+
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/fpga/fpga-region.h>
+#include <linux/fpga/fpga-bridge.h>
+#include <kunit/test.h>
+
+#include "fake-fpga-region.h"
+
+#define FAKE_FPGA_REGION_DEV_NAME	"fake_fpga_region"
+
+struct fake_region_priv {
+	int id;
+	struct kunit *test;
+};
+
+struct fake_region_data {
+	struct fpga_manager *mgr;
+	struct kunit *test;
+};
+
+/**
+ * fake_fpga_region_register - register a fake FPGA region
+ * @region_ctx: fake FPGA region context data structure.
+ * @test: KUnit test context object.
+ *
+ * Return: 0 if registration succeeded, an error code otherwise.
+ */
+int fake_fpga_region_register(struct fake_fpga_region *region_ctx,
+			      struct fpga_manager *mgr, struct kunit *test)
+{
+	struct fake_region_data pdata;
+	struct fake_region_priv *priv;
+	int ret;
+
+	pdata.mgr = mgr;
+	pdata.test = test;
+
+	region_ctx->pdev = platform_device_alloc(FAKE_FPGA_REGION_DEV_NAME,
+						 PLATFORM_DEVID_AUTO);
+	if (IS_ERR(region_ctx->pdev)) {
+		pr_err("Fake FPGA region device allocation failed\n");
+		return -ENOMEM;
+	}
+
+	platform_device_add_data(region_ctx->pdev, &pdata, sizeof(pdata));
+
+	ret = platform_device_add(region_ctx->pdev);
+	if (ret) {
+		pr_err("Fake FPGA region device add failed\n");
+		platform_device_put(region_ctx->pdev);
+		return ret;
+	}
+
+	region_ctx->region = platform_get_drvdata(region_ctx->pdev);
+
+	if (test) {
+		priv = region_ctx->region->priv;
+		kunit_info(test, "Fake FPGA region %d registered\n", priv->id);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fake_fpga_region_register);
+
+/**
+ * fake_fpga_region_unregister - unregister a fake FPGA region
+ * @region_ctx: fake FPGA region context data structure.
+ */
+void fake_fpga_region_unregister(struct fake_fpga_region *region_ctx)
+{
+	struct fake_region_priv *priv;
+	struct kunit *test;
+	int id;
+
+	priv = region_ctx->region->priv;
+	test = priv->test;
+	id = priv->id;
+
+	if (region_ctx->pdev) {
+		platform_device_unregister(region_ctx->pdev);
+		if (test)
+			kunit_info(test, "Fake FPGA region %d unregistered\n", id);
+	}
+}
+EXPORT_SYMBOL_GPL(fake_fpga_region_unregister);
+
+/**
+ * fake_fpga_region_add_bridge - add a bridge to a fake FPGA region
+ * @region_ctx: fake FPGA region context data structure.
+ * @bridge: FPGA bridge.
+ *
+ * Return: 0 if registration succeeded, an error code otherwise.
+ */
+int fake_fpga_region_add_bridge(struct fake_fpga_region *region_ctx,
+				struct fpga_bridge *bridge)
+{
+	struct fake_region_priv *priv;
+	int ret;
+
+	priv = region_ctx->region->priv;
+
+	ret = fpga_bridge_get_to_list(bridge->dev.parent, NULL,
+				      &region_ctx->region->bridge_list);
+
+	if (priv->test && !ret)
+		kunit_info(priv->test, "Bridge added to fake FPGA region %d\n",
+			   priv->id);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(fake_fpga_region_add_bridge);
+
+static int fake_fpga_region_probe(struct platform_device *pdev)
+{
+	struct device *dev;
+	struct fpga_region *region;
+	struct fpga_manager *mgr;
+	struct fake_region_data *pdata;
+	struct fake_region_priv *priv;
+	static int id_count;
+
+	dev = &pdev->dev;
+	pdata = dev_get_platdata(dev);
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	mgr = fpga_mgr_get(pdata->mgr->dev.parent);
+	if (IS_ERR(mgr))
+		return PTR_ERR(mgr);
+
+	/*
+	 * No get_bridges() method since the bridges list is
+	 * pre-built using fake_fpga_region_add_bridge()
+	 */
+	region = fpga_region_register(dev, mgr, NULL);
+	if (IS_ERR(region)) {
+		fpga_mgr_put(mgr);
+		return PTR_ERR(region);
+	}
+
+	priv->test = pdata->test;
+	priv->id = id_count++;
+	region->priv = priv;
+
+	platform_set_drvdata(pdev, region);
+
+	return 0;
+}
+
+static int fake_fpga_region_remove(struct platform_device *pdev)
+{
+	struct fpga_region *region = platform_get_drvdata(pdev);
+	struct fpga_manager *mgr = region->mgr;
+
+	fpga_mgr_put(mgr);
+	fpga_bridges_put(&region->bridge_list);
+	fpga_region_unregister(region);
+
+	return 0;
+}
+
+static struct platform_driver fake_fpga_region_drv = {
+	.driver = {
+		.name = FAKE_FPGA_REGION_DEV_NAME
+	},
+	.probe = fake_fpga_region_probe,
+	.remove = fake_fpga_region_remove,
+};
+
+module_platform_driver(fake_fpga_region_drv);
+
+MODULE_AUTHOR("Marco Pagani <marpagan@redhat.com>");
+MODULE_DESCRIPTION("Fake FPGA Bridge");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/fpga/tests/fake-fpga-region.h b/drivers/fpga/tests/fake-fpga-region.h
new file mode 100644
index 000000000000..55b2df3f04ba
--- /dev/null
+++ b/drivers/fpga/tests/fake-fpga-region.h
@@ -0,0 +1,37 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Header file for fake FPGA region
+ *
+ * Copyright (C) 2023 Red Hat, Inc. All rights reserved.
+ *
+ * Author: Marco Pagani <marpagan@redhat.com>
+ */
+
+#ifndef __FPGA_FAKE_RGN_H
+#define __FPGA_FAKE_RGN_H
+
+#include <linux/platform_device.h>
+#include <kunit/test.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/fpga/fpga-bridge.h>
+
+/**
+ * struct fake_fpga_region - fake FPGA region context data structure
+ *
+ * @region: FPGA region.
+ * @pdev: platform device of the FPGA region.
+ */
+struct fake_fpga_region {
+	struct fpga_region *region;
+	struct platform_device *pdev;
+};
+
+int fake_fpga_region_register(struct fake_fpga_region *region_ctx,
+			      struct fpga_manager *mgr, struct kunit *test);
+
+int fake_fpga_region_add_bridge(struct fake_fpga_region *region_ctx,
+				struct fpga_bridge *bridge);
+
+void fake_fpga_region_unregister(struct fake_fpga_region *region_ctx);
+
+#endif /* __FPGA_FAKE_RGN_H */
-- 
2.39.1

[RFC PATCH 3/4] fpga: add fake FPGA manager

[Marco Pagani]

Add fake FPGA manager platform driver with support functions.
The driver checks the programming sequence using KUnit expectations.
This module is part of the KUnit test suite for the FPGA subsystem.

Signed-off-by: Marco Pagani <marpagan@redhat.com>
---
 drivers/fpga/tests/fake-fpga-mgr.c | 365 +++++++++++++++++++++++++++++
 drivers/fpga/tests/fake-fpga-mgr.h |  42 ++++
 2 files changed, 407 insertions(+)
 create mode 100644 drivers/fpga/tests/fake-fpga-mgr.c
 create mode 100644 drivers/fpga/tests/fake-fpga-mgr.h

diff --git a/drivers/fpga/tests/fake-fpga-mgr.c b/drivers/fpga/tests/fake-fpga-mgr.c
new file mode 100644
index 000000000000..9daf328353d8
--- /dev/null
+++ b/drivers/fpga/tests/fake-fpga-mgr.c
@@ -0,0 +1,365 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for fake FPGA manager
+ *
+ * Copyright (C) 2023 Red Hat, Inc. All rights reserved.
+ *
+ * Author: Marco Pagani <marpagan@redhat.com>
+ */
+
+#include <linux/types.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <kunit/test.h>
+
+#include "fake-fpga-mgr.h"
+
+#define FAKE_FPGA_MGR_DEV_NAME	"fake_fpga_mgr"
+
+#define FAKE_HEADER_BYTE	0x3f
+#define FAKE_HEADER_SIZE	FPGA_TEST_BIT_BLOCK
+
+struct fake_mgr_priv {
+	int rcfg_count;
+	bool op_parse_header;
+	bool op_write_init;
+	bool op_write;
+	bool op_write_sg;
+	bool op_write_complete;
+	struct kunit *test;
+};
+
+struct fake_mgr_data {
+	struct kunit *test;
+};
+
+static void check_header(struct kunit *test, const u8 *buf);
+
+static enum fpga_mgr_states op_state(struct fpga_manager *mgr)
+{
+	struct fake_mgr_priv *priv;
+
+	priv = mgr->priv;
+
+	if (priv->test)
+		kunit_info(priv->test, "Fake FPGA manager: state\n");
+
+	return FPGA_MGR_STATE_UNKNOWN;
+}
+
+static u64 op_status(struct fpga_manager *mgr)
+{
+	struct fake_mgr_priv *priv;
+
+	priv = mgr->priv;
+
+	if (priv->test)
+		kunit_info(priv->test, "Fake FPGA manager: status\n");
+
+	return 0;
+}
+
+static int op_parse_header(struct fpga_manager *mgr, struct fpga_image_info *info,
+			   const char *buf, size_t count)
+{
+	struct fake_mgr_priv *priv;
+
+	priv = mgr->priv;
+
+	if (priv->test) {
+		kunit_info(priv->test, "Fake FPGA manager: parse_header\n");
+
+		KUNIT_EXPECT_EQ(priv->test, mgr->state,
+				FPGA_MGR_STATE_PARSE_HEADER);
+
+		check_header(priv->test, buf);
+	}
+
+	priv->op_parse_header = true;
+
+	return 0;
+}
+
+static int op_write_init(struct fpga_manager *mgr, struct fpga_image_info *info,
+			 const char *buf, size_t count)
+{
+	struct fake_mgr_priv *priv;
+
+	priv = mgr->priv;
+
+	if (priv->test) {
+		kunit_info(priv->test, "Fake FPGA manager: write_init\n");
+
+		KUNIT_EXPECT_EQ(priv->test, mgr->state,
+				FPGA_MGR_STATE_WRITE_INIT);
+	}
+
+	priv->op_write_init = true;
+
+	return 0;
+}
+
+static int op_write(struct fpga_manager *mgr, const char *buf, size_t count)
+{
+	struct fake_mgr_priv *priv;
+
+	priv = mgr->priv;
+
+	if (priv->test) {
+		kunit_info(priv->test, "Fake FPGA manager: write\n");
+
+		KUNIT_EXPECT_EQ(priv->test, mgr->state,
+				FPGA_MGR_STATE_WRITE);
+	}
+
+	priv->op_write = true;
+
+	return 0;
+}
+
+static int op_write_sg(struct fpga_manager *mgr, struct sg_table *sgt)
+{
+	struct fake_mgr_priv *priv;
+
+	priv = mgr->priv;
+
+	if (priv->test) {
+		kunit_info(priv->test, "Fake FPGA manager: write_sg\n");
+
+		KUNIT_EXPECT_EQ(priv->test, mgr->state,
+				FPGA_MGR_STATE_WRITE);
+	}
+
+	priv->op_write_sg = true;
+
+	return 0;
+}
+
+static int op_write_complete(struct fpga_manager *mgr, struct fpga_image_info *info)
+{
+	struct fake_mgr_priv *priv;
+
+	priv = mgr->priv;
+
+	if (priv->test) {
+		kunit_info(priv->test, "Fake FPGA manager: write_complete\n");
+
+		KUNIT_EXPECT_EQ(priv->test, mgr->state,
+				FPGA_MGR_STATE_WRITE_COMPLETE);
+	}
+
+	priv->op_write_complete = true;
+	priv->rcfg_count++;
+
+	return 0;
+}
+
+static void op_fpga_remove(struct fpga_manager *mgr)
+{
+	struct fake_mgr_priv *priv;
+
+	priv = mgr->priv;
+
+	if (priv->test)
+		kunit_info(priv->test, "Fake FPGA manager: remove\n");
+}
+
+static const struct fpga_manager_ops fake_fpga_mgr_ops = {
+	.initial_header_size = FAKE_HEADER_SIZE,
+	.skip_header = false,
+	.state = op_state,
+	.status = op_status,
+	.parse_header = op_parse_header,
+	.write_init = op_write_init,
+	.write = op_write,
+	.write_sg = op_write_sg,
+	.write_complete = op_write_complete,
+	.fpga_remove = op_fpga_remove,
+};
+
+/**
+ * fake_fpga_mgr_register - register a fake FPGA manager
+ * @mgr_ctx: fake FPGA manager context data structure.
+ * @test: KUnit test context object.
+ *
+ * Return: 0 if registration succeeded, an error code otherwise.
+ */
+int fake_fpga_mgr_register(struct fake_fpga_mgr *mgr_ctx, struct kunit *test)
+{
+	struct fake_mgr_data pdata;
+	int ret;
+
+	pdata.test = test;
+
+	mgr_ctx->pdev = platform_device_alloc(FAKE_FPGA_MGR_DEV_NAME,
+					      PLATFORM_DEVID_AUTO);
+	if (IS_ERR(mgr_ctx->pdev)) {
+		pr_err("Fake FPGA manager device allocation failed\n");
+		return -ENOMEM;
+	}
+
+	platform_device_add_data(mgr_ctx->pdev, &pdata, sizeof(pdata));
+
+	ret = platform_device_add(mgr_ctx->pdev);
+	if (ret) {
+		pr_err("Fake FPGA manager device add failed\n");
+		platform_device_put(mgr_ctx->pdev);
+		return ret;
+	}
+
+	mgr_ctx->mgr = platform_get_drvdata(mgr_ctx->pdev);
+
+	if (test)
+		kunit_info(test, "Fake FPGA manager registered\n");
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fake_fpga_mgr_register);
+
+/**
+ * fake_fpga_mgr_unregister - unregister a fake FPGA manager
+ * @mgr_ctx: fake FPGA manager context data structure.
+ */
+void fake_fpga_mgr_unregister(struct fake_fpga_mgr *mgr_ctx)
+{
+	struct fake_mgr_priv *priv;
+	struct kunit *test;
+
+	priv = mgr_ctx->mgr->priv;
+	test = priv->test;
+
+	if (mgr_ctx->pdev) {
+		platform_device_unregister(mgr_ctx->pdev);
+		if (test)
+			kunit_info(test, "Fake FPGA manager unregistered\n");
+	}
+}
+EXPORT_SYMBOL_GPL(fake_fpga_mgr_unregister);
+
+/**
+ * fake_fpga_mgr_get_rcfg_count - get the number of reconfigurations
+ * @mgr_ctx: fake FPGA manager context data structure.
+ *
+ * Return: number of reconfigurations.
+ */
+int fake_fpga_mgr_get_rcfg_count(const struct fake_fpga_mgr *mgr_ctx)
+{
+	struct fake_mgr_priv *priv;
+
+	priv = mgr_ctx->mgr->priv;
+
+	return priv->rcfg_count;
+}
+EXPORT_SYMBOL_GPL(fake_fpga_mgr_get_rcfg_count);
+
+/**
+ * fake_fpga_mgr_fill_header - fill the bitstream buffer with the test header
+ * @mgr_ctx: fake FPGA manager context data structure.
+ */
+void fake_fpga_mgr_fill_header(u8 *buf)
+{
+	int i;
+
+	for (i = 0; i < FAKE_HEADER_SIZE; i++)
+		buf[i] = FAKE_HEADER_BYTE;
+}
+EXPORT_SYMBOL_GPL(fake_fpga_mgr_fill_header);
+
+static void check_header(struct kunit *test, const u8 *buf)
+{
+	int i;
+
+	for (i = 0; i < FAKE_HEADER_SIZE; i++)
+		KUNIT_EXPECT_EQ(test, buf[i], FAKE_HEADER_BYTE);
+}
+
+static void clear_op_flags(struct fake_mgr_priv *priv)
+{
+	priv->op_parse_header = false;
+	priv->op_write_init = false;
+	priv->op_write = false;
+	priv->op_write_sg = false;
+	priv->op_write_complete = false;
+}
+
+/**
+ * fake_fpga_mgr_check_write_buf - check if programming using a buffer succeeded
+ * @mgr_ctx: fake FPGA manager context data structure.
+ */
+void fake_fpga_mgr_check_write_buf(struct fake_fpga_mgr *mgr_ctx)
+{
+	struct fake_mgr_priv *priv;
+
+	priv = mgr_ctx->mgr->priv;
+
+	if (priv->test) {
+		KUNIT_EXPECT_EQ(priv->test, priv->op_parse_header, true);
+		KUNIT_EXPECT_EQ(priv->test, priv->op_write_init, true);
+		KUNIT_EXPECT_EQ(priv->test, priv->op_write, true);
+		KUNIT_EXPECT_EQ(priv->test, priv->op_write_complete, true);
+	}
+
+	clear_op_flags(priv);
+}
+EXPORT_SYMBOL_GPL(fake_fpga_mgr_check_write_buf);
+
+/**
+ * fake_fpga_mgr_check_write_sg - check if programming using a s.g. table succeeded
+ * @mgr_ctx: fake FPGA manager context data structure.
+ */
+void fake_fpga_mgr_check_write_sg(struct fake_fpga_mgr *mgr_ctx)
+{
+	struct fake_mgr_priv *priv;
+
+	priv = mgr_ctx->mgr->priv;
+
+	if (priv->test) {
+		KUNIT_EXPECT_EQ(priv->test, priv->op_parse_header, true);
+		KUNIT_EXPECT_EQ(priv->test, priv->op_write_init, true);
+		KUNIT_EXPECT_EQ(priv->test, priv->op_write_sg, true);
+		KUNIT_EXPECT_EQ(priv->test, priv->op_write_complete, true);
+	}
+
+	clear_op_flags(priv);
+}
+EXPORT_SYMBOL_GPL(fake_fpga_mgr_check_write_sg);
+
+static int fake_fpga_mgr_probe(struct platform_device *pdev)
+{
+	struct device *dev;
+	struct fake_mgr_priv *priv;
+	struct fake_mgr_data *pdata;
+	struct fpga_manager *mgr;
+
+	dev = &pdev->dev;
+	pdata = dev_get_platdata(dev);
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->test = pdata->test;
+
+	mgr = devm_fpga_mgr_register(dev, "Fake FPGA Manager",
+				     &fake_fpga_mgr_ops, priv);
+	if (IS_ERR(mgr))
+		return PTR_ERR(mgr);
+
+	platform_set_drvdata(pdev, mgr);
+
+	return 0;
+}
+
+static struct platform_driver fake_fpga_mgr_drv = {
+	.driver = {
+		.name = FAKE_FPGA_MGR_DEV_NAME
+	},
+	.probe = fake_fpga_mgr_probe,
+};
+
+module_platform_driver(fake_fpga_mgr_drv);
+
+MODULE_AUTHOR("Marco Pagani <marpagan@redhat.com>");
+MODULE_DESCRIPTION("Fake FPGA Manager");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/fpga/tests/fake-fpga-mgr.h b/drivers/fpga/tests/fake-fpga-mgr.h
new file mode 100644
index 000000000000..5cecb6f646c9
--- /dev/null
+++ b/drivers/fpga/tests/fake-fpga-mgr.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Header file for fake FPGA manager
+ *
+ * Copyright (C) 2023 Red Hat, Inc. All rights reserved.
+ *
+ * Author: Marco Pagani <marpagan@redhat.com>
+ */
+
+#ifndef __FPGA_FAKE_MGR_H
+#define __FPGA_FAKE_MGR_H
+
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/platform_device.h>
+#include <kunit/test.h>
+
+#define FPGA_TEST_BIT_BLOCK	1024
+
+/**
+ * struct fake_fpga_mgr - fake FPGA manager context data structure
+ *
+ * @mgr: FPGA manager.
+ * @pdev: platform device of the FPGA manager.
+ */
+struct fake_fpga_mgr {
+	struct fpga_manager *mgr;
+	struct platform_device *pdev;
+};
+
+int fake_fpga_mgr_register(struct fake_fpga_mgr *mgr_ctx, struct kunit *test);
+
+void fake_fpga_mgr_unregister(struct fake_fpga_mgr *mgr_ctx);
+
+int fake_fpga_mgr_get_rcfg_count(const struct fake_fpga_mgr *mgr_ctx);
+
+void fake_fpga_mgr_fill_header(u8 *buf);
+
+void fake_fpga_mgr_check_write_buf(struct fake_fpga_mgr *mgr_ctx);
+
+void fake_fpga_mgr_check_write_sg(struct fake_fpga_mgr *mgr_ctx);
+
+#endif /* __FPGA_FAKE_MGR_H */
-- 
2.39.1

[RFC PATCH 4/4] fpga: add fake FPGA bridge

[Marco Pagani]

Add fake FPGA bridge driver with support functions. The driver includes
a counter for the number of switching cycles. This module is part of
the KUnit test suite for the FPGA subsystem.

Signed-off-by: Marco Pagani <marpagan@redhat.com>
---
 drivers/fpga/tests/fake-fpga-bridge.c | 214 ++++++++++++++++++++++++++
 drivers/fpga/tests/fake-fpga-bridge.h |  36 +++++
 2 files changed, 250 insertions(+)
 create mode 100644 drivers/fpga/tests/fake-fpga-bridge.c
 create mode 100644 drivers/fpga/tests/fake-fpga-bridge.h

diff --git a/drivers/fpga/tests/fake-fpga-bridge.c b/drivers/fpga/tests/fake-fpga-bridge.c
new file mode 100644
index 000000000000..1f3c8e4fbb6a
--- /dev/null
+++ b/drivers/fpga/tests/fake-fpga-bridge.c
@@ -0,0 +1,214 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for fake FPGA bridge
+ *
+ * Copyright (C) 2023 Red Hat, Inc. All rights reserved.
+ *
+ * Author: Marco Pagani <marpagan@redhat.com>
+ */
+
+#include <linux/types.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/fpga/fpga-bridge.h>
+#include <kunit/test.h>
+
+#include "fake-fpga-bridge.h"
+
+#define FAKE_FPGA_BRIDGE_DEV_NAME	"fake_fpga_bridge"
+
+struct fake_bridge_priv {
+	int id;
+	bool enable;
+	int cycles_count;
+	struct kunit *test;
+};
+
+struct fake_bridge_data {
+	struct kunit *test;
+};
+
+static int op_enable_show(struct fpga_bridge *bridge)
+{
+	struct fake_bridge_priv *priv;
+
+	priv = bridge->priv;
+
+	if (priv->test)
+		kunit_info(priv->test, "Fake FPGA bridge %d: enable_show\n",
+			   priv->id);
+
+	return priv->enable;
+}
+
+static int op_enable_set(struct fpga_bridge *bridge, bool enable)
+{
+	struct fake_bridge_priv *priv;
+
+	priv = bridge->priv;
+
+	if (enable && !priv->enable)
+		priv->cycles_count++;
+
+	priv->enable = enable;
+
+	if (priv->test)
+		kunit_info(priv->test, "Fake FPGA bridge %d: enable_set: %d\n",
+			   priv->id, enable);
+
+	return 0;
+}
+
+static void op_remove(struct fpga_bridge *bridge)
+{
+}
+
+static const struct fpga_bridge_ops fake_fpga_bridge_ops = {
+	.enable_show = op_enable_show,
+	.enable_set = op_enable_set,
+	.fpga_bridge_remove = op_remove,
+};
+
+/**
+ * fake_fpga_bridge_register - register a fake FPGA bridge
+ * @bridge_ctx: fake FPGA bridge context data structure.
+ * @test: KUnit test context object.
+ *
+ * Return: 0 if registration succeeded, an error code otherwise.
+ */
+int fake_fpga_bridge_register(struct fake_fpga_bridge *bridge_ctx,
+			      struct kunit *test)
+{
+	struct fake_bridge_data pdata;
+	struct fake_bridge_priv *priv;
+	int ret;
+
+	pdata.test = test;
+
+	bridge_ctx->pdev = platform_device_alloc(FAKE_FPGA_BRIDGE_DEV_NAME,
+						 PLATFORM_DEVID_AUTO);
+	if (IS_ERR(bridge_ctx->pdev)) {
+		pr_err("Fake FPGA bridge device allocation failed\n");
+		return -ENOMEM;
+	}
+
+	platform_device_add_data(bridge_ctx->pdev, &pdata, sizeof(pdata));
+
+	ret = platform_device_add(bridge_ctx->pdev);
+	if (ret) {
+		pr_err("Fake FPGA bridge device add failed\n");
+		platform_device_put(bridge_ctx->pdev);
+		return ret;
+	}
+
+	bridge_ctx->bridge = platform_get_drvdata(bridge_ctx->pdev);
+
+	if (test) {
+		priv = bridge_ctx->bridge->priv;
+		kunit_info(test, "Fake FPGA bridge %d registered\n", priv->id);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fake_fpga_bridge_register);
+
+/**
+ * fake_fpga_bridge_unregister - unregister a fake FPGA bridge
+ * @bridge_ctx: fake FPGA bridge context data structure.
+ */
+void fake_fpga_bridge_unregister(struct fake_fpga_bridge *bridge_ctx)
+{
+	struct fake_bridge_priv *priv;
+	struct kunit *test;
+	int id;
+
+	priv = bridge_ctx->bridge->priv;
+	test = priv->test;
+	id = priv->id;
+
+	if (bridge_ctx->pdev) {
+		platform_device_unregister(bridge_ctx->pdev);
+		if (test)
+			kunit_info(test, "Fake FPGA bridge %d unregistered\n", id);
+	}
+}
+EXPORT_SYMBOL_GPL(fake_fpga_bridge_unregister);
+
+/**
+ * fake_fpga_bridge_get_state - get state of a fake FPGA bridge
+ * @bridge_ctx: fake FPGA bridge context data structure.
+ *
+ * Return: 1 if the bridge is enabled, 0 if disabled.
+ */
+int fake_fpga_bridge_get_state(const struct fake_fpga_bridge *bridge_ctx)
+{
+	return bridge_ctx->bridge->br_ops->enable_show(bridge_ctx->bridge);
+}
+EXPORT_SYMBOL_GPL(fake_fpga_bridge_get_state);
+
+/**
+ * fake_fpga_bridge_get_cycles_count - get the number of switching cycles
+ * @bridge_ctx: fake FPGA bridge context data structure.
+ *
+ * Return: number of switching cycles.
+ */
+int fake_fpga_bridge_get_cycles_count(const struct fake_fpga_bridge *bridge_ctx)
+{
+	struct fake_bridge_priv *priv;
+
+	priv = bridge_ctx->bridge->priv;
+
+	return priv->cycles_count;
+}
+EXPORT_SYMBOL_GPL(fake_fpga_bridge_get_cycles_count);
+
+static int fake_fpga_bridge_probe(struct platform_device *pdev)
+{
+	struct device *dev;
+	struct fpga_bridge *bridge;
+	struct fake_bridge_data *pdata;
+	struct fake_bridge_priv *priv;
+	static int id_count;
+
+	dev = &pdev->dev;
+	pdata = dev_get_platdata(dev);
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->id = id_count++;
+	priv->test = pdata->test;
+
+	bridge = fpga_bridge_register(dev, "Fake FPGA Bridge",
+				      &fake_fpga_bridge_ops, priv);
+	if (IS_ERR(bridge))
+		return PTR_ERR(bridge);
+
+	platform_set_drvdata(pdev, bridge);
+
+	return 0;
+}
+
+static int fake_fpga_bridge_remove(struct platform_device *pdev)
+{
+	struct fpga_bridge *bridge = platform_get_drvdata(pdev);
+
+	fpga_bridge_unregister(bridge);
+
+	return 0;
+}
+
+static struct platform_driver fake_fpga_bridge_drv = {
+	.driver = {
+		.name = FAKE_FPGA_BRIDGE_DEV_NAME
+	},
+	.probe = fake_fpga_bridge_probe,
+	.remove = fake_fpga_bridge_remove,
+};
+
+module_platform_driver(fake_fpga_bridge_drv);
+
+MODULE_AUTHOR("Marco Pagani <marpagan@redhat.com>");
+MODULE_DESCRIPTION("Fake FPGA Bridge");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/fpga/tests/fake-fpga-bridge.h b/drivers/fpga/tests/fake-fpga-bridge.h
new file mode 100644
index 000000000000..9de62d2f993b
--- /dev/null
+++ b/drivers/fpga/tests/fake-fpga-bridge.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Header file for fake FPGA bridge
+ *
+ * Copyright (C) 2023 Red Hat, Inc. All rights reserved.
+ *
+ * Author: Marco Pagani <marpagan@redhat.com>
+ */
+
+#ifndef __FPGA_FAKE_BRIDGE_H
+#define __FPGA_FAKE_BRIDGE_H
+
+#include <linux/platform_device.h>
+#include <kunit/test.h>
+
+/**
+ * struct fake_fpga_bridge - fake FPGA bridge context data structure
+ *
+ * @bridge: FPGA bridge.
+ * @pdev: platform device of the FPGA bridge.
+ */
+struct fake_fpga_bridge {
+	struct fpga_bridge *bridge;
+	struct platform_device *pdev;
+};
+
+int fake_fpga_bridge_register(struct fake_fpga_bridge *bridge_ctx,
+			      struct kunit *test);
+
+void fake_fpga_bridge_unregister(struct fake_fpga_bridge *bridge_ctx);
+
+int fake_fpga_bridge_get_state(const struct fake_fpga_bridge *bridge_ctx);
+
+int fake_fpga_bridge_get_cycles_count(const struct fake_fpga_bridge *bridge_ctx);
+
+#endif /* __FPGA_FAKE_BRIDGE_H */
-- 
2.39.1

Re: [RFC PATCH 1/4] fpga: add initial KUnit test suite

[Russ Weight]

Hi Marco,

I've just started looking at this, but I have a couple of early comments below

On 2/3/23 09:06, Marco Pagani wrote:
> Introduce an initial KUnit suite to test the core components of the
> FPGA subsystem.
>
> 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@redhat.com>
> ---
>  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.
These lines seem shorter than necessary. You can use up to 75
characters per line.

> +
> +	  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
> +obj-$(CONFIG_FPGA_KUNIT_TESTS) += fpga-tests.o
> 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@redhat.com>
> + */
> +
> +#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);
> +
> +	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);
> +
> +	test_img_info->sgt = &sgt_bit;
> +
> +	/* Re-program the fake FPGA using the image scatter list */
> +	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_sg(&mgr_ctx);
> +
> +	KUNIT_EXPECT_EQ(test, 2, 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, 2, fake_fpga_bridge_get_cycles_count(&base_bridge_ctx));
> +
> +	free_sgt_bit(&sgt_bit);
> +	fpga_image_info_free(test_img_info);
> +	fpga_free_base_sys(&mgr_ctx, &base_bridge_ctx, &base_region_ctx);
> +}
> +
> +static void fpga_pr_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 fake_fpga_bridge pr_bridge_0_ctx;
> +	struct fake_fpga_bridge pr_bridge_1_ctx;
> +	struct fake_fpga_region pr_region_ctx;
> +
> +	struct fpga_image_info *test_static_img_info;
> +	struct fpga_image_info *test_pr_img_info;
> +
> +	fpga_build_base_sys(test, &mgr_ctx, &base_bridge_ctx, &base_region_ctx);
> +
> +	/* Allocate a fake test image using a buffer */
> +	test_static_img_info = fpga_image_info_alloc(&mgr_ctx.pdev->dev);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_static_img_info);
> +
> +	test_static_img_info->buf = fake_bit;
> +	test_static_img_info->count = sizeof(fake_bit);
> +
> +	kunit_info(test, "fake bitstream size: %zu\n", test_static_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_static_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));
> +
> +	/* The static image contains a reconfigurable sub-region with two soft bridges */
> +	ret = fake_fpga_bridge_register(&pr_bridge_0_ctx, test);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	ret = fake_fpga_bridge_register(&pr_bridge_1_ctx, test);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	ret = fake_fpga_region_register(&pr_region_ctx, mgr_ctx.mgr, test);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	ret = fake_fpga_region_add_bridge(&pr_region_ctx, pr_bridge_0_ctx.bridge);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	ret = fake_fpga_region_add_bridge(&pr_region_ctx, pr_bridge_1_ctx.bridge);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	/* Allocate a fake partial test image using a buffer */
> +	test_pr_img_info = fpga_image_info_alloc(&mgr_ctx.pdev->dev);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_pr_img_info);
> +
> +	test_pr_img_info->buf = fake_bit;
> +	test_pr_img_info->count = sizeof(fake_bit) / 2;
> +	test_pr_img_info->flags = FPGA_MGR_PARTIAL_RECONFIG;
> +
> +	kunit_info(test, "fake partial bitstream size: %zu\n", test_pr_img_info->count);
> +
> +	/* Program the reconfigurable sub-region */
> +	pr_region_ctx.region->info = test_pr_img_info;
> +	ret = fpga_region_program_fpga(pr_region_ctx.region);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	fake_fpga_mgr_check_write_buf(&mgr_ctx);
> +
> +	KUNIT_EXPECT_EQ(test, 2, fake_fpga_mgr_get_rcfg_count(&mgr_ctx));
> +
> +	KUNIT_EXPECT_EQ(test, 1, fake_fpga_bridge_get_state(&pr_bridge_0_ctx));
> +	KUNIT_EXPECT_EQ(test, 1, fake_fpga_bridge_get_cycles_count(&pr_bridge_0_ctx));
> +
> +	KUNIT_EXPECT_EQ(test, 1, fake_fpga_bridge_get_state(&pr_bridge_1_ctx));
> +	KUNIT_EXPECT_EQ(test, 1, fake_fpga_bridge_get_cycles_count(&pr_bridge_1_ctx));
> +
> +	/* Check that the base bridge has not been disabled */
> +	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_pr_img_info);
> +	fpga_image_info_free(test_static_img_info);
> +
> +	fake_fpga_region_unregister(&pr_region_ctx);
> +	fake_fpga_bridge_unregister(&pr_bridge_0_ctx);
> +	fake_fpga_bridge_unregister(&pr_bridge_1_ctx);
> +
> +	fpga_free_base_sys(&mgr_ctx, &base_bridge_ctx, &base_region_ctx);
> +}
> +
> +static struct kunit_case fpga_test_cases[] = {
> +	KUNIT_CASE(fpga_base_test),
> +	KUNIT_CASE(fpga_pr_test),
> +	{},
> +};
> +
> +static struct kunit_suite fpga_test_suite = {
> +	.name = "fpga-tests",
> +	.suite_init = fpga_suite_init,
> +	.test_cases = fpga_test_cases,
> +};
> +
> +kunit_test_suite(fpga_test_suite);

When I try to build with these patches, I get this error:
>
> ERROR: modpost: missing MODULE_LICENSE() in drivers/fpga/tests/fpga-tests.o

I was able to fix it by adding this line at the bottom of the file:

> MODULE_LICENSE("GPL");

- Russ

Re: [RFC PATCH 1/4] fpga: add initial KUnit test suite

[Marco Pagani]

On 2023-02-07 02:05, Russ Weight wrote:
> Hi Marco,
> 
> I've just started looking at this, but I have a couple of early comments below
>

Thanks for looking into this.

[...]
 
>> --- /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.
> These lines seem shorter than necessary. You can use up to 75
> characters per line.
> 

I'll reflow the text in the next revision.

[...]

>> +static struct kunit_case fpga_test_cases[] = {
>> +	KUNIT_CASE(fpga_base_test),
>> +	KUNIT_CASE(fpga_pr_test),
>> +	{},
>> +};
>> +
>> +static struct kunit_suite fpga_test_suite = {
>> +	.name = "fpga-tests",
>> +	.suite_init = fpga_suite_init,
>> +	.test_cases = fpga_test_cases,
>> +};
>> +
>> +kunit_test_suite(fpga_test_suite);
> 
> When I try to build with these patches, I get this error:
>>
>> ERROR: modpost: missing MODULE_LICENSE() in drivers/fpga/tests/fpga-tests.o
> 
> I was able to fix it by adding this line at the bottom of the file:
> 
>> MODULE_LICENSE("GPL");
> 
> - Russ
>

Right, I forgot to add module info macros to the test suite module.
I'll add MODULE_LICENSE() in the next revision.

Thanks for the feedback,
Marco

Re: [RFC PATCH 1/4] fpga: add initial KUnit test suite

[Russ Weight]

On 2/3/23 09:06, Marco Pagani wrote:
> Introduce an initial KUnit suite to test the core components of the
> FPGA subsystem.
>
> 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@redhat.com>
> ---
>  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
> +obj-$(CONFIG_FPGA_KUNIT_TESTS) += fpga-tests.o
> 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@redhat.com>
> + */
> +
> +#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];

I take it "bit" in fake_bit and sgt_bit is short for "bitstream". Initially,
I found this confusing as I tend to think of a bit as a single bit. It might
be better to expand that something like "fake_bitstream" or "fake_image".

- Russ
> +
> +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);
> +
> +	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);
> +
> +	test_img_info->sgt = &sgt_bit;
> +
> +	/* Re-program the fake FPGA using the image scatter list */
> +	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_sg(&mgr_ctx);
> +
> +	KUNIT_EXPECT_EQ(test, 2, 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, 2, fake_fpga_bridge_get_cycles_count(&base_bridge_ctx));
> +
> +	free_sgt_bit(&sgt_bit);
> +	fpga_image_info_free(test_img_info);
> +	fpga_free_base_sys(&mgr_ctx, &base_bridge_ctx, &base_region_ctx);
> +}
> +
> +static void fpga_pr_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 fake_fpga_bridge pr_bridge_0_ctx;
> +	struct fake_fpga_bridge pr_bridge_1_ctx;
> +	struct fake_fpga_region pr_region_ctx;
> +
> +	struct fpga_image_info *test_static_img_info;
> +	struct fpga_image_info *test_pr_img_info;
> +
> +	fpga_build_base_sys(test, &mgr_ctx, &base_bridge_ctx, &base_region_ctx);
> +
> +	/* Allocate a fake test image using a buffer */
> +	test_static_img_info = fpga_image_info_alloc(&mgr_ctx.pdev->dev);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_static_img_info);
> +
> +	test_static_img_info->buf = fake_bit;
> +	test_static_img_info->count = sizeof(fake_bit);
> +
> +	kunit_info(test, "fake bitstream size: %zu\n", test_static_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_static_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));
> +
> +	/* The static image contains a reconfigurable sub-region with two soft bridges */
> +	ret = fake_fpga_bridge_register(&pr_bridge_0_ctx, test);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	ret = fake_fpga_bridge_register(&pr_bridge_1_ctx, test);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	ret = fake_fpga_region_register(&pr_region_ctx, mgr_ctx.mgr, test);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	ret = fake_fpga_region_add_bridge(&pr_region_ctx, pr_bridge_0_ctx.bridge);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	ret = fake_fpga_region_add_bridge(&pr_region_ctx, pr_bridge_1_ctx.bridge);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	/* Allocate a fake partial test image using a buffer */
> +	test_pr_img_info = fpga_image_info_alloc(&mgr_ctx.pdev->dev);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_pr_img_info);
> +
> +	test_pr_img_info->buf = fake_bit;
> +	test_pr_img_info->count = sizeof(fake_bit) / 2;
> +	test_pr_img_info->flags = FPGA_MGR_PARTIAL_RECONFIG;
> +
> +	kunit_info(test, "fake partial bitstream size: %zu\n", test_pr_img_info->count);
> +
> +	/* Program the reconfigurable sub-region */
> +	pr_region_ctx.region->info = test_pr_img_info;
> +	ret = fpga_region_program_fpga(pr_region_ctx.region);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	fake_fpga_mgr_check_write_buf(&mgr_ctx);
> +
> +	KUNIT_EXPECT_EQ(test, 2, fake_fpga_mgr_get_rcfg_count(&mgr_ctx));
> +
> +	KUNIT_EXPECT_EQ(test, 1, fake_fpga_bridge_get_state(&pr_bridge_0_ctx));
> +	KUNIT_EXPECT_EQ(test, 1, fake_fpga_bridge_get_cycles_count(&pr_bridge_0_ctx));
> +
> +	KUNIT_EXPECT_EQ(test, 1, fake_fpga_bridge_get_state(&pr_bridge_1_ctx));
> +	KUNIT_EXPECT_EQ(test, 1, fake_fpga_bridge_get_cycles_count(&pr_bridge_1_ctx));
> +
> +	/* Check that the base bridge has not been disabled */
> +	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_pr_img_info);
> +	fpga_image_info_free(test_static_img_info);
> +
> +	fake_fpga_region_unregister(&pr_region_ctx);
> +	fake_fpga_bridge_unregister(&pr_bridge_0_ctx);
> +	fake_fpga_bridge_unregister(&pr_bridge_1_ctx);
> +
> +	fpga_free_base_sys(&mgr_ctx, &base_bridge_ctx, &base_region_ctx);
> +}
> +
> +static struct kunit_case fpga_test_cases[] = {
> +	KUNIT_CASE(fpga_base_test),
> +	KUNIT_CASE(fpga_pr_test),
> +	{},
> +};
> +
> +static struct kunit_suite fpga_test_suite = {
> +	.name = "fpga-tests",
> +	.suite_init = fpga_suite_init,
> +	.test_cases = fpga_test_cases,
> +};
> +
> +kunit_test_suite(fpga_test_suite);

Re: [RFC PATCH 0/4] fpga: add initial KUnit test suite for the subsystem

[Russ Weight]

On 2/3/23 09:06, Marco Pagani wrote:
> This patch set introduces a KUnit suite to test the core components
> of the FPGA subsystem. More specifically, the suite tests the core
> functions of the FPGA manager, FPGA bridge, and FPGA region.
>
> These components are tested using "fake" modules that allow
> observing their internals without altering the source code.
>
> The test suite can be run using
> [user@localhost linux]$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/fpga/tests
When I tried running these tests, I got an error until I created this file:

drivers/fpga/tests/.kunitconfig:
CONFIG_KUNIT=y
CONFIG_FPGA=y
CONFIG_FPGA_REGION=y
CONFIG_FPGA_BRIDGE=y
CONFIG_FPGA_KUNIT_TESTS=y

I think this file needs to be included in your patchset?

- Russ

>
> Marco Pagani (4):
>   fpga: add initial KUnit test suite
>   fpga: add fake FPGA region
>   fpga: add fake FPGA manager
>   fpga: add fake FPGA bridge
>
>  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/fake-fpga-bridge.c | 214 +++++++++++++++
>  drivers/fpga/tests/fake-fpga-bridge.h |  36 +++
>  drivers/fpga/tests/fake-fpga-mgr.c    | 365 ++++++++++++++++++++++++++
>  drivers/fpga/tests/fake-fpga-mgr.h    |  42 +++
>  drivers/fpga/tests/fake-fpga-region.c | 186 +++++++++++++
>  drivers/fpga/tests/fake-fpga-region.h |  37 +++
>  drivers/fpga/tests/fpga-tests.c       | 264 +++++++++++++++++++
>  12 files changed, 1175 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/fake-fpga-bridge.c
>  create mode 100644 drivers/fpga/tests/fake-fpga-bridge.h
>  create mode 100644 drivers/fpga/tests/fake-fpga-mgr.c
>  create mode 100644 drivers/fpga/tests/fake-fpga-mgr.h
>  create mode 100644 drivers/fpga/tests/fake-fpga-region.c
>  create mode 100644 drivers/fpga/tests/fake-fpga-region.h
>  create mode 100644 drivers/fpga/tests/fpga-tests.c
>

Re: [RFC PATCH 0/4] fpga: add initial KUnit test suite for the subsystem

[Marco Pagani]

On 2023-02-14 02:20, Russ Weight wrote:
> 
> 
> On 2/3/23 09:06, Marco Pagani wrote:
>> This patch set introduces a KUnit suite to test the core components
>> of the FPGA subsystem. More specifically, the suite tests the core
>> functions of the FPGA manager, FPGA bridge, and FPGA region.
>>
>> These components are tested using "fake" modules that allow
>> observing their internals without altering the source code.
>>
>> The test suite can be run using
>> [user@localhost linux]$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/fpga/tests
> When I tried running these tests, I got an error until I created this file:
> 
> drivers/fpga/tests/.kunitconfig:
> CONFIG_KUNIT=y
> CONFIG_FPGA=y
> CONFIG_FPGA_REGION=y
> CONFIG_FPGA_BRIDGE=y
> CONFIG_FPGA_KUNIT_TESTS=y
> 
> I think this file needs to be included in your patchset?
> 
> - Russ
>


Patch 1/4 includes a .kunitconfig file with these configs set =y

> 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

To double-check for any patch format errors, I downloaded the patch set
from lore.kernel.org and applied it on a fresh tree with Git (version
2.39.1) using git am. In my case, Git created the .kunitconfig file and
I was able to run the tests.


>>
>> Marco Pagani (4):
>>   fpga: add initial KUnit test suite
>>   fpga: add fake FPGA region
>>   fpga: add fake FPGA manager
>>   fpga: add fake FPGA bridge
>>
>>  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/fake-fpga-bridge.c | 214 +++++++++++++++
>>  drivers/fpga/tests/fake-fpga-bridge.h |  36 +++
>>  drivers/fpga/tests/fake-fpga-mgr.c    | 365 ++++++++++++++++++++++++++
>>  drivers/fpga/tests/fake-fpga-mgr.h    |  42 +++
>>  drivers/fpga/tests/fake-fpga-region.c | 186 +++++++++++++
>>  drivers/fpga/tests/fake-fpga-region.h |  37 +++
>>  drivers/fpga/tests/fpga-tests.c       | 264 +++++++++++++++++++
>>  12 files changed, 1175 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/fake-fpga-bridge.c
>>  create mode 100644 drivers/fpga/tests/fake-fpga-bridge.h
>>  create mode 100644 drivers/fpga/tests/fake-fpga-mgr.c
>>  create mode 100644 drivers/fpga/tests/fake-fpga-mgr.h
>>  create mode 100644 drivers/fpga/tests/fake-fpga-region.c
>>  create mode 100644 drivers/fpga/tests/fake-fpga-region.h
>>  create mode 100644 drivers/fpga/tests/fpga-tests.c
>>
> 

Thanks,
Marco

Re: [RFC PATCH 1/4] fpga: add initial KUnit test suite

[Marco Pagani]

On 2023-02-14 00:37, Russ Weight wrote:
> 
> 
> On 2/3/23 09:06, Marco Pagani wrote:
>> Introduce an initial KUnit suite to test the core components of the
>> FPGA subsystem.
>>
>> 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@redhat.com>
>> ---
>>  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
>> +obj-$(CONFIG_FPGA_KUNIT_TESTS) += fpga-tests.o
>> 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@redhat.com>
>> + */
>> +
>> +#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];
> 
> I take it "bit" in fake_bit and sgt_bit is short for "bitstream". Initially,
> I found this confusing as I tend to think of a bit as a single bit. It might
> be better to expand that something like "fake_bitstream" or "fake_image".
> 
> - Russ


You're right. Using "bit" in the name can be confusing. I'll change it
to "fake_image" or maybe "fake_image_buf" to be consistent with the naming
convention used in the subsystem. I'll also change "test_img_info"
to "fake_img_info" to improve naming consistency.

Thanks,
Marco

Re: [RFC PATCH 0/4] fpga: add initial KUnit test suite for the subsystem

[Russ Weight]

On 2/15/23 03:19, Marco Pagani wrote:
>> When I tried running these tests, I got an error until I created this file:
>>
>> drivers/fpga/tests/.kunitconfig:
>> CONFIG_KUNIT=y
>> CONFIG_FPGA=y
>> CONFIG_FPGA_REGION=y
>> CONFIG_FPGA_BRIDGE=y
>> CONFIG_FPGA_KUNIT_TESTS=y
>>
>> I think this file needs to be included in your patchset?
>>
>> - Russ
>>
> Patch 1/4 includes a .kunitconfig file with these configs set =y
>
>> 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
> To double-check for any patch format errors, I downloaded the patch set
> from lore.kernel.org and applied it on a fresh tree with Git (version
> 2.39.1) using git am. In my case, Git created the .kunitconfig file and
> I was able to run the tests.
>
>
I can see the .kunitconfig file in the emailed patch. I had to resolve
some conflicts when I applied patch #1 - I must have missed this file
when I committed the changes.

Thanks,
- Russ

Re: [RFC PATCH 1/4] fpga: add initial KUnit test suite

[Xu Yilun]

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.

> 
> 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@redhat.com>
> ---
>  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.

> +obj-$(CONFIG_FPGA_KUNIT_TESTS) += fpga-tests.o

Maybe fpga-test.o?

And could they be built in a single module? I haven't find a reason
these fake components been used alone.

> 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@redhat.com>
> + */
> +
> +#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?

> +
> +	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?

> +
> +	test_img_info->sgt = &sgt_bit;
> +
> +	/* Re-program the fake FPGA using the image scatter list */
> +	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_sg(&mgr_ctx);
> +
> +	KUNIT_EXPECT_EQ(test, 2, 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, 2, fake_fpga_bridge_get_cycles_count(&base_bridge_ctx));
> +
> +	free_sgt_bit(&sgt_bit);
> +	fpga_image_info_free(test_img_info);
> +	fpga_free_base_sys(&mgr_ctx, &base_bridge_ctx, &base_region_ctx);
> +}
> +
> +static void fpga_pr_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 fake_fpga_bridge pr_bridge_0_ctx;
> +	struct fake_fpga_bridge pr_bridge_1_ctx;
> +	struct fake_fpga_region pr_region_ctx;
> +
> +	struct fpga_image_info *test_static_img_info;
> +	struct fpga_image_info *test_pr_img_info;
> +
> +	fpga_build_base_sys(test, &mgr_ctx, &base_bridge_ctx, &base_region_ctx);

If we need the base region/bridge/mgr for each case, could we create
global ones in .init(), or .suite_init()?

> +
> +	/* Allocate a fake test image using a buffer */
> +	test_static_img_info = fpga_image_info_alloc(&mgr_ctx.pdev->dev);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_static_img_info);
> +
> +	test_static_img_info->buf = fake_bit;
> +	test_static_img_info->count = sizeof(fake_bit);

Same concern, may remove the test image info initialization from each
test case code.

> +
> +	kunit_info(test, "fake bitstream size: %zu\n", test_static_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_static_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));
> +
> +	/* The static image contains a reconfigurable sub-region with two soft bridges */

Till now I didn't find any difference with fpga_base_test.
And I can't figure out how the "static parent region - sub pr region"
topology is created?

> +	ret = fake_fpga_bridge_register(&pr_bridge_0_ctx, test);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	ret = fake_fpga_bridge_register(&pr_bridge_1_ctx, test);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	ret = fake_fpga_region_register(&pr_region_ctx, mgr_ctx.mgr, test);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	ret = fake_fpga_region_add_bridge(&pr_region_ctx, pr_bridge_0_ctx.bridge);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	ret = fake_fpga_region_add_bridge(&pr_region_ctx, pr_bridge_1_ctx.bridge);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	/* Allocate a fake partial test image using a buffer */
> +	test_pr_img_info = fpga_image_info_alloc(&mgr_ctx.pdev->dev);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_pr_img_info);
> +
> +	test_pr_img_info->buf = fake_bit;
> +	test_pr_img_info->count = sizeof(fake_bit) / 2;
> +	test_pr_img_info->flags = FPGA_MGR_PARTIAL_RECONFIG;
> +
> +	kunit_info(test, "fake partial bitstream size: %zu\n", test_pr_img_info->count);
> +
> +	/* Program the reconfigurable sub-region */
> +	pr_region_ctx.region->info = test_pr_img_info;
> +	ret = fpga_region_program_fpga(pr_region_ctx.region);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	fake_fpga_mgr_check_write_buf(&mgr_ctx);
> +
> +	KUNIT_EXPECT_EQ(test, 2, fake_fpga_mgr_get_rcfg_count(&mgr_ctx));
> +
> +	KUNIT_EXPECT_EQ(test, 1, fake_fpga_bridge_get_state(&pr_bridge_0_ctx));
> +	KUNIT_EXPECT_EQ(test, 1, fake_fpga_bridge_get_cycles_count(&pr_bridge_0_ctx));
> +
> +	KUNIT_EXPECT_EQ(test, 1, fake_fpga_bridge_get_state(&pr_bridge_1_ctx));
> +	KUNIT_EXPECT_EQ(test, 1, fake_fpga_bridge_get_cycles_count(&pr_bridge_1_ctx));
> +
> +	/* Check that the base bridge has not been disabled */
> +	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_pr_img_info);
> +	fpga_image_info_free(test_static_img_info);
> +
> +	fake_fpga_region_unregister(&pr_region_ctx);
> +	fake_fpga_bridge_unregister(&pr_bridge_0_ctx);
> +	fake_fpga_bridge_unregister(&pr_bridge_1_ctx);
> +
> +	fpga_free_base_sys(&mgr_ctx, &base_bridge_ctx, &base_region_ctx);

Same concern, may put them in .exit() or suite_exit()?

> +}
> +
> +static struct kunit_case fpga_test_cases[] = {
> +	KUNIT_CASE(fpga_base_test),
> +	KUNIT_CASE(fpga_pr_test),

I feel there are too many tasks for each test case, and some duplicated
routines.

Could we have a suite for the common routine test in each case, like
region/bridge/mgr (un)register, fpga image alloc ... And another suite
which have these common routines in .init() or .suite_init().

> +	{},
> +};
> +
> +static struct kunit_suite fpga_test_suite = {
> +	.name = "fpga-tests",

I see from style.rst that:
  
  "Names should use underscores, not dashes, to separate words"

and

  "*Do not* include "test" or "kunit" directly in the subsystem name
   unless we are actually testing other tests or the kunit framework
   itself"

So IIUC I assume the name should be "fpga"?

BTW: I do see some existing test cases that are not conform to the style,
even the examples in doc itself.

Thanks,
Yilun

> +	.suite_init = fpga_suite_init,
> +	.test_cases = fpga_test_cases,
> +};
> +
> +kunit_test_suite(fpga_test_suite);
> -- 
> 2.39.1
> 

Re: [RFC PATCH 2/4] fpga: add fake FPGA region

[Xu Yilun]

On 2023-02-03 at 18:06:51 +0100, Marco Pagani wrote:
> Add fake FPGA region platform driver with support functions. This
> module is part of the KUnit test suite for the FPGA subsystem.
> 
> Signed-off-by: Marco Pagani <marpagan@redhat.com>
> ---
>  drivers/fpga/tests/fake-fpga-region.c | 186 ++++++++++++++++++++++++++
>  drivers/fpga/tests/fake-fpga-region.h |  37 +++++
>  2 files changed, 223 insertions(+)
>  create mode 100644 drivers/fpga/tests/fake-fpga-region.c
>  create mode 100644 drivers/fpga/tests/fake-fpga-region.h
> 
> diff --git a/drivers/fpga/tests/fake-fpga-region.c b/drivers/fpga/tests/fake-fpga-region.c
> new file mode 100644
> index 000000000000..095397e41837
> --- /dev/null
> +++ b/drivers/fpga/tests/fake-fpga-region.c
> @@ -0,0 +1,186 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for fake FPGA region
> + *
> + * Copyright (C) 2023 Red Hat, Inc. All rights reserved.
> + *
> + * Author: Marco Pagani <marpagan@redhat.com>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/fpga/fpga-mgr.h>
> +#include <linux/fpga/fpga-region.h>
> +#include <linux/fpga/fpga-bridge.h>
> +#include <kunit/test.h>
> +
> +#include "fake-fpga-region.h"
> +
> +#define FAKE_FPGA_REGION_DEV_NAME	"fake_fpga_region"
> +
> +struct fake_region_priv {
> +	int id;
> +	struct kunit *test;
> +};
> +
> +struct fake_region_data {
> +	struct fpga_manager *mgr;
> +	struct kunit *test;
> +};
> +
> +/**
> + * fake_fpga_region_register - register a fake FPGA region
> + * @region_ctx: fake FPGA region context data structure.
> + * @test: KUnit test context object.
> + *
> + * Return: 0 if registration succeeded, an error code otherwise.
> + */
> +int fake_fpga_region_register(struct fake_fpga_region *region_ctx,
> +			      struct fpga_manager *mgr, struct kunit *test)
> +{
> +	struct fake_region_data pdata;
> +	struct fake_region_priv *priv;
> +	int ret;
> +
> +	pdata.mgr = mgr;
> +	pdata.test = test;
> +
> +	region_ctx->pdev = platform_device_alloc(FAKE_FPGA_REGION_DEV_NAME,
> +						 PLATFORM_DEVID_AUTO);
> +	if (IS_ERR(region_ctx->pdev)) {
> +		pr_err("Fake FPGA region device allocation failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	platform_device_add_data(region_ctx->pdev, &pdata, sizeof(pdata));
> +
> +	ret = platform_device_add(region_ctx->pdev);
> +	if (ret) {
> +		pr_err("Fake FPGA region device add failed\n");
> +		platform_device_put(region_ctx->pdev);
> +		return ret;
> +	}
> +
> +	region_ctx->region = platform_get_drvdata(region_ctx->pdev);
> +
> +	if (test) {
> +		priv = region_ctx->region->priv;
> +		kunit_info(test, "Fake FPGA region %d registered\n", priv->id);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(fake_fpga_region_register);
> +
> +/**
> + * fake_fpga_region_unregister - unregister a fake FPGA region
> + * @region_ctx: fake FPGA region context data structure.
> + */
> +void fake_fpga_region_unregister(struct fake_fpga_region *region_ctx)
> +{
> +	struct fake_region_priv *priv;
> +	struct kunit *test;
> +	int id;
> +
> +	priv = region_ctx->region->priv;
> +	test = priv->test;
> +	id = priv->id;
> +
> +	if (region_ctx->pdev) {
> +		platform_device_unregister(region_ctx->pdev);
> +		if (test)
> +			kunit_info(test, "Fake FPGA region %d unregistered\n", id);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(fake_fpga_region_unregister);
> +
> +/**
> + * fake_fpga_region_add_bridge - add a bridge to a fake FPGA region
> + * @region_ctx: fake FPGA region context data structure.
> + * @bridge: FPGA bridge.
> + *
> + * Return: 0 if registration succeeded, an error code otherwise.
> + */
> +int fake_fpga_region_add_bridge(struct fake_fpga_region *region_ctx,
> +				struct fpga_bridge *bridge)
> +{
> +	struct fake_region_priv *priv;
> +	int ret;
> +
> +	priv = region_ctx->region->priv;
> +
> +	ret = fpga_bridge_get_to_list(bridge->dev.parent, NULL,
> +				      &region_ctx->region->bridge_list);
> +
> +	if (priv->test && !ret)
> +		kunit_info(priv->test, "Bridge added to fake FPGA region %d\n",
> +			   priv->id);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(fake_fpga_region_add_bridge);
> +
> +static int fake_fpga_region_probe(struct platform_device *pdev)
> +{
> +	struct device *dev;
> +	struct fpga_region *region;
> +	struct fpga_manager *mgr;
> +	struct fake_region_data *pdata;
> +	struct fake_region_priv *priv;
> +	static int id_count;
> +
> +	dev = &pdev->dev;
> +	pdata = dev_get_platdata(dev);
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	mgr = fpga_mgr_get(pdata->mgr->dev.parent);
> +	if (IS_ERR(mgr))
> +		return PTR_ERR(mgr);
> +
> +	/*
> +	 * No get_bridges() method since the bridges list is
> +	 * pre-built using fake_fpga_region_add_bridge()
> +	 */

This is not the common use for drivers to associate the region & bridge,
Better to realize the get_bridges() method.

Thanks,
Yilun

> +	region = fpga_region_register(dev, mgr, NULL);
> +	if (IS_ERR(region)) {
> +		fpga_mgr_put(mgr);
> +		return PTR_ERR(region);
> +	}
> +
> +	priv->test = pdata->test;
> +	priv->id = id_count++;
> +	region->priv = priv;
> +
> +	platform_set_drvdata(pdev, region);
> +
> +	return 0;
> +}

Re: [RFC PATCH 1/4] fpga: add initial KUnit test suite

[Marco Pagani]

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@redhat.com>
>> ---
>>  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.

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@redhat.com>
>> + */
>> +
>> +#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.
 
>> +
>> +	test_img_info->sgt = &sgt_bit;
>> +
>> +	/* Re-program the fake FPGA using the image scatter list */
>> +	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_sg(&mgr_ctx);
>> +
>> +	KUNIT_EXPECT_EQ(test, 2, 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, 2, fake_fpga_bridge_get_cycles_count(&base_bridge_ctx));
>> +
>> +	free_sgt_bit(&sgt_bit);
>> +	fpga_image_info_free(test_img_info);
>> +	fpga_free_base_sys(&mgr_ctx, &base_bridge_ctx, &base_region_ctx);
>> +}
>> +
>> +static void fpga_pr_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 fake_fpga_bridge pr_bridge_0_ctx;
>> +	struct fake_fpga_bridge pr_bridge_1_ctx;
>> +	struct fake_fpga_region pr_region_ctx;
>> +
>> +	struct fpga_image_info *test_static_img_info;
>> +	struct fpga_image_info *test_pr_img_info;
>> +
>> +	fpga_build_base_sys(test, &mgr_ctx, &base_bridge_ctx, &base_region_ctx);
> 
> If we need the base region/bridge/mgr for each case, could we create
> global ones in .init(), or .suite_init()?
>

Ok, I'll reduce code duplication in the next version. My only concern is that
I would not want to complicate the test code.

In my intentions, this is just an initial set of tests intended to lay the
foundation for other test suites. At this stage, I'm not sure if other tests
will need or use this kind of setup. So I would like to keep the test code
as simple as possible.

>> +
>> +	/* Allocate a fake test image using a buffer */
>> +	test_static_img_info = fpga_image_info_alloc(&mgr_ctx.pdev->dev);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_static_img_info);
>> +
>> +	test_static_img_info->buf = fake_bit;
>> +	test_static_img_info->count = sizeof(fake_bit);
> 
> Same concern, may remove the test image info initialization from each
> test case code.
> 

Same as above, I'll reduce code duplication in the next version.

>> +
>> +	kunit_info(test, "fake bitstream size: %zu\n", test_static_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_static_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));
>> +
>> +	/* The static image contains a reconfigurable sub-region with two soft bridges */
> 
> Till now I didn't find any difference with fpga_base_test.
> And I can't figure out how the "static parent region - sub pr region"
> topology is created?
> 

You're right, the topology is missing. I'm preparing a new version where regions
are hierarchically organized according to the FPGA Region DT binding documentation.
I.e., the static region is the parent device of the reconfigurable region.

>> +	ret = fake_fpga_bridge_register(&pr_bridge_0_ctx, test);
>> +	KUNIT_ASSERT_EQ(test, ret, 0);
>> +
>> +	ret = fake_fpga_bridge_register(&pr_bridge_1_ctx, test);
>> +	KUNIT_ASSERT_EQ(test, ret, 0);
>> +
>> +	ret = fake_fpga_region_register(&pr_region_ctx, mgr_ctx.mgr, test);
>> +	KUNIT_ASSERT_EQ(test, ret, 0);
>> +
>> +	ret = fake_fpga_region_add_bridge(&pr_region_ctx, pr_bridge_0_ctx.bridge);
>> +	KUNIT_ASSERT_EQ(test, ret, 0);
>> +
>> +	ret = fake_fpga_region_add_bridge(&pr_region_ctx, pr_bridge_1_ctx.bridge);
>> +	KUNIT_ASSERT_EQ(test, ret, 0);
>> +
>> +	/* Allocate a fake partial test image using a buffer */
>> +	test_pr_img_info = fpga_image_info_alloc(&mgr_ctx.pdev->dev);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_pr_img_info);
>> +
>> +	test_pr_img_info->buf = fake_bit;
>> +	test_pr_img_info->count = sizeof(fake_bit) / 2;
>> +	test_pr_img_info->flags = FPGA_MGR_PARTIAL_RECONFIG;
>> +
>> +	kunit_info(test, "fake partial bitstream size: %zu\n", test_pr_img_info->count);
>> +
>> +	/* Program the reconfigurable sub-region */
>> +	pr_region_ctx.region->info = test_pr_img_info;
>> +	ret = fpga_region_program_fpga(pr_region_ctx.region);
>> +	KUNIT_ASSERT_EQ(test, ret, 0);
>> +
>> +	fake_fpga_mgr_check_write_buf(&mgr_ctx);
>> +
>> +	KUNIT_EXPECT_EQ(test, 2, fake_fpga_mgr_get_rcfg_count(&mgr_ctx));
>> +
>> +	KUNIT_EXPECT_EQ(test, 1, fake_fpga_bridge_get_state(&pr_bridge_0_ctx));
>> +	KUNIT_EXPECT_EQ(test, 1, fake_fpga_bridge_get_cycles_count(&pr_bridge_0_ctx));
>> +
>> +	KUNIT_EXPECT_EQ(test, 1, fake_fpga_bridge_get_state(&pr_bridge_1_ctx));
>> +	KUNIT_EXPECT_EQ(test, 1, fake_fpga_bridge_get_cycles_count(&pr_bridge_1_ctx));
>> +
>> +	/* Check that the base bridge has not been disabled */
>> +	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_pr_img_info);
>> +	fpga_image_info_free(test_static_img_info);
>> +
>> +	fake_fpga_region_unregister(&pr_region_ctx);
>> +	fake_fpga_bridge_unregister(&pr_bridge_0_ctx);
>> +	fake_fpga_bridge_unregister(&pr_bridge_1_ctx);
>> +
>> +	fpga_free_base_sys(&mgr_ctx, &base_bridge_ctx, &base_region_ctx);
> 
> Same concern, may put them in .exit() or suite_exit()?

Same as above, I'll reduce code duplication.

> 
>> +}
>> +
>> +static struct kunit_case fpga_test_cases[] = {
>> +	KUNIT_CASE(fpga_base_test),
>> +	KUNIT_CASE(fpga_pr_test),
> 
> I feel there are too many tasks for each test case, and some duplicated
> routines.
> 
> Could we have a suite for the common routine test in each case, like
> region/bridge/mgr (un)register, fpga image alloc ... And another suite
> which have these common routines in .init() or .suite_init().
>

Right, I'll reduce code duplication in the next version.

>> +	{},
>> +};
>> +
>> +static struct kunit_suite fpga_test_suite = {
>> +	.name = "fpga-tests",
> 
> I see from style.rst that:
>   
>   "Names should use underscores, not dashes, to separate words"
> 
> and
> 
>   "*Do not* include "test" or "kunit" directly in the subsystem name
>    unless we are actually testing other tests or the kunit framework
>    itself"
> 
> So IIUC I assume the name should be "fpga"?
> 
> BTW: I do see some existing test cases that are not conform to the style,
> even the examples in doc itself.

Thanks for noticing this. I'll change the name in the next version.

> 
> Thanks,
> Yilun
> 
>> +	.suite_init = fpga_suite_init,
>> +	.test_cases = fpga_test_cases,
>> +};
>> +
>> +kunit_test_suite(fpga_test_suite);
>> -- 
>> 2.39.1
>>
> 

Thanks,
Marco

Re: [RFC PATCH 2/4] fpga: add fake FPGA region

[Marco Pagani]

On 2023-02-18 11:13, Xu Yilun wrote:
> On 2023-02-03 at 18:06:51 +0100, Marco Pagani wrote:
>> Add fake FPGA region platform driver with support functions. This
>> module is part of the KUnit test suite for the FPGA subsystem.
>>
>> Signed-off-by: Marco Pagani <marpagan@redhat.com>
>> ---
>>  drivers/fpga/tests/fake-fpga-region.c | 186 ++++++++++++++++++++++++++
>>  drivers/fpga/tests/fake-fpga-region.h |  37 +++++
>>  2 files changed, 223 insertions(+)
>>  create mode 100644 drivers/fpga/tests/fake-fpga-region.c
>>  create mode 100644 drivers/fpga/tests/fake-fpga-region.h
>>
>> diff --git a/drivers/fpga/tests/fake-fpga-region.c b/drivers/fpga/tests/fake-fpga-region.c
>> new file mode 100644
>> index 000000000000..095397e41837
>> --- /dev/null
>> +++ b/drivers/fpga/tests/fake-fpga-region.c
>> @@ -0,0 +1,186 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Driver for fake FPGA region
>> + *
>> + * Copyright (C) 2023 Red Hat, Inc. All rights reserved.
>> + *
>> + * Author: Marco Pagani <marpagan@redhat.com>
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/fpga/fpga-mgr.h>
>> +#include <linux/fpga/fpga-region.h>
>> +#include <linux/fpga/fpga-bridge.h>
>> +#include <kunit/test.h>
>> +
>> +#include "fake-fpga-region.h"
>> +
>> +#define FAKE_FPGA_REGION_DEV_NAME	"fake_fpga_region"
>> +
>> +struct fake_region_priv {
>> +	int id;
>> +	struct kunit *test;
>> +};
>> +
>> +struct fake_region_data {
>> +	struct fpga_manager *mgr;
>> +	struct kunit *test;
>> +};
>> +
>> +/**
>> + * fake_fpga_region_register - register a fake FPGA region
>> + * @region_ctx: fake FPGA region context data structure.
>> + * @test: KUnit test context object.
>> + *
>> + * Return: 0 if registration succeeded, an error code otherwise.
>> + */
>> +int fake_fpga_region_register(struct fake_fpga_region *region_ctx,
>> +			      struct fpga_manager *mgr, struct kunit *test)
>> +{
>> +	struct fake_region_data pdata;
>> +	struct fake_region_priv *priv;
>> +	int ret;
>> +
>> +	pdata.mgr = mgr;
>> +	pdata.test = test;
>> +
>> +	region_ctx->pdev = platform_device_alloc(FAKE_FPGA_REGION_DEV_NAME,
>> +						 PLATFORM_DEVID_AUTO);
>> +	if (IS_ERR(region_ctx->pdev)) {
>> +		pr_err("Fake FPGA region device allocation failed\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	platform_device_add_data(region_ctx->pdev, &pdata, sizeof(pdata));
>> +
>> +	ret = platform_device_add(region_ctx->pdev);
>> +	if (ret) {
>> +		pr_err("Fake FPGA region device add failed\n");
>> +		platform_device_put(region_ctx->pdev);
>> +		return ret;
>> +	}
>> +
>> +	region_ctx->region = platform_get_drvdata(region_ctx->pdev);
>> +
>> +	if (test) {
>> +		priv = region_ctx->region->priv;
>> +		kunit_info(test, "Fake FPGA region %d registered\n", priv->id);
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(fake_fpga_region_register);
>> +
>> +/**
>> + * fake_fpga_region_unregister - unregister a fake FPGA region
>> + * @region_ctx: fake FPGA region context data structure.
>> + */
>> +void fake_fpga_region_unregister(struct fake_fpga_region *region_ctx)
>> +{
>> +	struct fake_region_priv *priv;
>> +	struct kunit *test;
>> +	int id;
>> +
>> +	priv = region_ctx->region->priv;
>> +	test = priv->test;
>> +	id = priv->id;
>> +
>> +	if (region_ctx->pdev) {
>> +		platform_device_unregister(region_ctx->pdev);
>> +		if (test)
>> +			kunit_info(test, "Fake FPGA region %d unregistered\n", id);
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(fake_fpga_region_unregister);
>> +
>> +/**
>> + * fake_fpga_region_add_bridge - add a bridge to a fake FPGA region
>> + * @region_ctx: fake FPGA region context data structure.
>> + * @bridge: FPGA bridge.
>> + *
>> + * Return: 0 if registration succeeded, an error code otherwise.
>> + */
>> +int fake_fpga_region_add_bridge(struct fake_fpga_region *region_ctx,
>> +				struct fpga_bridge *bridge)
>> +{
>> +	struct fake_region_priv *priv;
>> +	int ret;
>> +
>> +	priv = region_ctx->region->priv;
>> +
>> +	ret = fpga_bridge_get_to_list(bridge->dev.parent, NULL,
>> +				      &region_ctx->region->bridge_list);
>> +
>> +	if (priv->test && !ret)
>> +		kunit_info(priv->test, "Bridge added to fake FPGA region %d\n",
>> +			   priv->id);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(fake_fpga_region_add_bridge);
>> +
>> +static int fake_fpga_region_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev;
>> +	struct fpga_region *region;
>> +	struct fpga_manager *mgr;
>> +	struct fake_region_data *pdata;
>> +	struct fake_region_priv *priv;
>> +	static int id_count;
>> +
>> +	dev = &pdev->dev;
>> +	pdata = dev_get_platdata(dev);
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	mgr = fpga_mgr_get(pdata->mgr->dev.parent);
>> +	if (IS_ERR(mgr))
>> +		return PTR_ERR(mgr);
>> +
>> +	/*
>> +	 * No get_bridges() method since the bridges list is
>> +	 * pre-built using fake_fpga_region_add_bridge()
>> +	 */
> 
> This is not the common use for drivers to associate the region & bridge,
> Better to realize the get_bridges() method.

Initially, I was using a get_bridges() method to create the list of bridges
before each reconfiguration. However, this required having a "duplicated"
list of bridges in the context of the fake region low-level driver.

Since I couldn't find a reason to keep a duplicate list of bridges in the
fake region driver, I decided then to change the approach and build the
list of bridges at device instantiation time.

In my understanding, the approach of creating the list of bridges just
before reconfiguration with a get_bridges() method works best for the
OF case, where the topology is already encoded in the DT. I feel using
this approach on platforms without OF support would increase complexity
and create unnecessary duplication.

> 
> Thanks,
> Yilun
> 
>> +	region = fpga_region_register(dev, mgr, NULL);
>> +	if (IS_ERR(region)) {
>> +		fpga_mgr_put(mgr);
>> +		return PTR_ERR(region);
>> +	}
>> +
>> +	priv->test = pdata->test;
>> +	priv->id = id_count++;
>> +	region->priv = priv;
>> +
>> +	platform_set_drvdata(pdev, region);
>> +
>> +	return 0;
>> +}
> 

Thanks,
Marco

Re: [RFC PATCH 1/4] fpga: add initial KUnit test suite

[Xu Yilun]

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@redhat.com>
> >> ---
> >>  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@redhat.com>
> >> + */
> >> +
> >> +#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

Re: [RFC PATCH 2/4] fpga: add fake FPGA region

[Xu Yilun]

On 2023-02-21 at 15:53:20 +0100, Marco Pagani wrote:
> 
> 
> On 2023-02-18 11:13, Xu Yilun wrote:
> > On 2023-02-03 at 18:06:51 +0100, Marco Pagani wrote:
> >> Add fake FPGA region platform driver with support functions. This
> >> module is part of the KUnit test suite for the FPGA subsystem.
> >>
> >> Signed-off-by: Marco Pagani <marpagan@redhat.com>
> >> ---
> >>  drivers/fpga/tests/fake-fpga-region.c | 186 ++++++++++++++++++++++++++
> >>  drivers/fpga/tests/fake-fpga-region.h |  37 +++++
> >>  2 files changed, 223 insertions(+)
> >>  create mode 100644 drivers/fpga/tests/fake-fpga-region.c
> >>  create mode 100644 drivers/fpga/tests/fake-fpga-region.h
> >>
> >> diff --git a/drivers/fpga/tests/fake-fpga-region.c b/drivers/fpga/tests/fake-fpga-region.c
> >> new file mode 100644
> >> index 000000000000..095397e41837
> >> --- /dev/null
> >> +++ b/drivers/fpga/tests/fake-fpga-region.c
> >> @@ -0,0 +1,186 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Driver for fake FPGA region
> >> + *
> >> + * Copyright (C) 2023 Red Hat, Inc. All rights reserved.
> >> + *
> >> + * Author: Marco Pagani <marpagan@redhat.com>
> >> + */
> >> +
> >> +#include <linux/device.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/fpga/fpga-mgr.h>
> >> +#include <linux/fpga/fpga-region.h>
> >> +#include <linux/fpga/fpga-bridge.h>
> >> +#include <kunit/test.h>
> >> +
> >> +#include "fake-fpga-region.h"
> >> +
> >> +#define FAKE_FPGA_REGION_DEV_NAME	"fake_fpga_region"
> >> +
> >> +struct fake_region_priv {
> >> +	int id;
> >> +	struct kunit *test;
> >> +};
> >> +
> >> +struct fake_region_data {
> >> +	struct fpga_manager *mgr;
> >> +	struct kunit *test;
> >> +};
> >> +
> >> +/**
> >> + * fake_fpga_region_register - register a fake FPGA region
> >> + * @region_ctx: fake FPGA region context data structure.
> >> + * @test: KUnit test context object.
> >> + *
> >> + * Return: 0 if registration succeeded, an error code otherwise.
> >> + */
> >> +int fake_fpga_region_register(struct fake_fpga_region *region_ctx,
> >> +			      struct fpga_manager *mgr, struct kunit *test)
> >> +{
> >> +	struct fake_region_data pdata;
> >> +	struct fake_region_priv *priv;
> >> +	int ret;
> >> +
> >> +	pdata.mgr = mgr;
> >> +	pdata.test = test;
> >> +
> >> +	region_ctx->pdev = platform_device_alloc(FAKE_FPGA_REGION_DEV_NAME,
> >> +						 PLATFORM_DEVID_AUTO);
> >> +	if (IS_ERR(region_ctx->pdev)) {
> >> +		pr_err("Fake FPGA region device allocation failed\n");
> >> +		return -ENOMEM;
> >> +	}
> >> +
> >> +	platform_device_add_data(region_ctx->pdev, &pdata, sizeof(pdata));
> >> +
> >> +	ret = platform_device_add(region_ctx->pdev);
> >> +	if (ret) {
> >> +		pr_err("Fake FPGA region device add failed\n");
> >> +		platform_device_put(region_ctx->pdev);
> >> +		return ret;
> >> +	}
> >> +
> >> +	region_ctx->region = platform_get_drvdata(region_ctx->pdev);
> >> +
> >> +	if (test) {
> >> +		priv = region_ctx->region->priv;
> >> +		kunit_info(test, "Fake FPGA region %d registered\n", priv->id);
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(fake_fpga_region_register);
> >> +
> >> +/**
> >> + * fake_fpga_region_unregister - unregister a fake FPGA region
> >> + * @region_ctx: fake FPGA region context data structure.
> >> + */
> >> +void fake_fpga_region_unregister(struct fake_fpga_region *region_ctx)
> >> +{
> >> +	struct fake_region_priv *priv;
> >> +	struct kunit *test;
> >> +	int id;
> >> +
> >> +	priv = region_ctx->region->priv;
> >> +	test = priv->test;
> >> +	id = priv->id;
> >> +
> >> +	if (region_ctx->pdev) {
> >> +		platform_device_unregister(region_ctx->pdev);
> >> +		if (test)
> >> +			kunit_info(test, "Fake FPGA region %d unregistered\n", id);
> >> +	}
> >> +}
> >> +EXPORT_SYMBOL_GPL(fake_fpga_region_unregister);
> >> +
> >> +/**
> >> + * fake_fpga_region_add_bridge - add a bridge to a fake FPGA region
> >> + * @region_ctx: fake FPGA region context data structure.
> >> + * @bridge: FPGA bridge.
> >> + *
> >> + * Return: 0 if registration succeeded, an error code otherwise.
> >> + */
> >> +int fake_fpga_region_add_bridge(struct fake_fpga_region *region_ctx,
> >> +				struct fpga_bridge *bridge)
> >> +{
> >> +	struct fake_region_priv *priv;
> >> +	int ret;
> >> +
> >> +	priv = region_ctx->region->priv;
> >> +
> >> +	ret = fpga_bridge_get_to_list(bridge->dev.parent, NULL,
> >> +				      &region_ctx->region->bridge_list);
> >> +
> >> +	if (priv->test && !ret)
> >> +		kunit_info(priv->test, "Bridge added to fake FPGA region %d\n",
> >> +			   priv->id);
> >> +
> >> +	return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(fake_fpga_region_add_bridge);
> >> +
> >> +static int fake_fpga_region_probe(struct platform_device *pdev)
> >> +{
> >> +	struct device *dev;
> >> +	struct fpga_region *region;
> >> +	struct fpga_manager *mgr;
> >> +	struct fake_region_data *pdata;
> >> +	struct fake_region_priv *priv;
> >> +	static int id_count;
> >> +
> >> +	dev = &pdev->dev;
> >> +	pdata = dev_get_platdata(dev);
> >> +
> >> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >> +	if (!priv)
> >> +		return -ENOMEM;
> >> +
> >> +	mgr = fpga_mgr_get(pdata->mgr->dev.parent);
> >> +	if (IS_ERR(mgr))
> >> +		return PTR_ERR(mgr);
> >> +
> >> +	/*
> >> +	 * No get_bridges() method since the bridges list is
> >> +	 * pre-built using fake_fpga_region_add_bridge()
> >> +	 */
> > 
> > This is not the common use for drivers to associate the region & bridge,
> > Better to realize the get_bridges() method.
> 
> Initially, I was using a get_bridges() method to create the list of bridges
> before each reconfiguration. However, this required having a "duplicated"
> list of bridges in the context of the fake region low-level driver.
> 
> Since I couldn't find a reason to keep a duplicate list of bridges in the
> fake region driver, I decided then to change the approach and build the
> list of bridges at device instantiation time.
> 
> In my understanding, the approach of creating the list of bridges just
> before reconfiguration with a get_bridges() method works best for the
> OF case, where the topology is already encoded in the DT. I feel using
> this approach on platforms without OF support would increase complexity
> and create unnecessary duplication.

I'm not fully get your point. My understanding is we don't have to
always grab the bridge driver module if we don't reprogram. In many
cases, we just work with the existing bitstream before Linux is started.
So generally I prefer not to have an example that gets all bridges at
initialization unless there is a real need.

Thanks,
Yilun

Re: [RFC PATCH 1/4] fpga: add initial KUnit test suite

[Marco Pagani]

On 2023-02-24 07:14, Xu Yilun wrote:
> 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@redhat.com>
>>>> ---
>>>>  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@redhat.com>
>>>> + */
>>>> +
>>>> +#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?

I looked through the documentation for guidelines on how to handle
initialization errors, but found only the eeprom example where KUNIT_ASSERT
is used to handle errors in eeprom_buffer_test_init(). Existing test suites
seem to use different approaches to handle initialization errors. Some
return an error code, while others use KUnit assertions.

I'm more inclined to follow the example in the documentation and use
KUnit assertions. Does this approach work for you?


After some thought, I'm restructuring the code to test single components
in isolation before testing them together. In this way, I think the test
suite will be more in line with the unit testing methodology.


Thanks,
Marco

Re: [RFC PATCH 2/4] fpga: add fake FPGA region

[Marco Pagani]

On 2023-02-24 08:20, Xu Yilun wrote:
> On 2023-02-21 at 15:53:20 +0100, Marco Pagani wrote:
>>
>>
>> On 2023-02-18 11:13, Xu Yilun wrote:
>>> On 2023-02-03 at 18:06:51 +0100, Marco Pagani wrote:
>>>> Add fake FPGA region platform driver with support functions. This
>>>> module is part of the KUnit test suite for the FPGA subsystem.
>>>>
>>>> Signed-off-by: Marco Pagani <marpagan@redhat.com>
>>>> ---
>>>>  drivers/fpga/tests/fake-fpga-region.c | 186 ++++++++++++++++++++++++++
>>>>  drivers/fpga/tests/fake-fpga-region.h |  37 +++++
>>>>  2 files changed, 223 insertions(+)
>>>>  create mode 100644 drivers/fpga/tests/fake-fpga-region.c
>>>>  create mode 100644 drivers/fpga/tests/fake-fpga-region.h
>>>>
>>>> diff --git a/drivers/fpga/tests/fake-fpga-region.c b/drivers/fpga/tests/fake-fpga-region.c
>>>> new file mode 100644
>>>> index 000000000000..095397e41837
>>>> --- /dev/null
>>>> +++ b/drivers/fpga/tests/fake-fpga-region.c
>>>> @@ -0,0 +1,186 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Driver for fake FPGA region
>>>> + *
>>>> + * Copyright (C) 2023 Red Hat, Inc. All rights reserved.
>>>> + *
>>>> + * Author: Marco Pagani <marpagan@redhat.com>
>>>> + */
>>>> +
>>>> +#include <linux/device.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/fpga/fpga-mgr.h>
>>>> +#include <linux/fpga/fpga-region.h>
>>>> +#include <linux/fpga/fpga-bridge.h>
>>>> +#include <kunit/test.h>
>>>> +
>>>> +#include "fake-fpga-region.h"
>>>> +
>>>> +#define FAKE_FPGA_REGION_DEV_NAME	"fake_fpga_region"
>>>> +
>>>> +struct fake_region_priv {
>>>> +	int id;
>>>> +	struct kunit *test;
>>>> +};
>>>> +
>>>> +struct fake_region_data {
>>>> +	struct fpga_manager *mgr;
>>>> +	struct kunit *test;
>>>> +};
>>>> +
>>>> +/**
>>>> + * fake_fpga_region_register - register a fake FPGA region
>>>> + * @region_ctx: fake FPGA region context data structure.
>>>> + * @test: KUnit test context object.
>>>> + *
>>>> + * Return: 0 if registration succeeded, an error code otherwise.
>>>> + */
>>>> +int fake_fpga_region_register(struct fake_fpga_region *region_ctx,
>>>> +			      struct fpga_manager *mgr, struct kunit *test)
>>>> +{
>>>> +	struct fake_region_data pdata;
>>>> +	struct fake_region_priv *priv;
>>>> +	int ret;
>>>> +
>>>> +	pdata.mgr = mgr;
>>>> +	pdata.test = test;
>>>> +
>>>> +	region_ctx->pdev = platform_device_alloc(FAKE_FPGA_REGION_DEV_NAME,
>>>> +						 PLATFORM_DEVID_AUTO);
>>>> +	if (IS_ERR(region_ctx->pdev)) {
>>>> +		pr_err("Fake FPGA region device allocation failed\n");
>>>> +		return -ENOMEM;
>>>> +	}
>>>> +
>>>> +	platform_device_add_data(region_ctx->pdev, &pdata, sizeof(pdata));
>>>> +
>>>> +	ret = platform_device_add(region_ctx->pdev);
>>>> +	if (ret) {
>>>> +		pr_err("Fake FPGA region device add failed\n");
>>>> +		platform_device_put(region_ctx->pdev);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	region_ctx->region = platform_get_drvdata(region_ctx->pdev);
>>>> +
>>>> +	if (test) {
>>>> +		priv = region_ctx->region->priv;
>>>> +		kunit_info(test, "Fake FPGA region %d registered\n", priv->id);
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(fake_fpga_region_register);
>>>> +
>>>> +/**
>>>> + * fake_fpga_region_unregister - unregister a fake FPGA region
>>>> + * @region_ctx: fake FPGA region context data structure.
>>>> + */
>>>> +void fake_fpga_region_unregister(struct fake_fpga_region *region_ctx)
>>>> +{
>>>> +	struct fake_region_priv *priv;
>>>> +	struct kunit *test;
>>>> +	int id;
>>>> +
>>>> +	priv = region_ctx->region->priv;
>>>> +	test = priv->test;
>>>> +	id = priv->id;
>>>> +
>>>> +	if (region_ctx->pdev) {
>>>> +		platform_device_unregister(region_ctx->pdev);
>>>> +		if (test)
>>>> +			kunit_info(test, "Fake FPGA region %d unregistered\n", id);
>>>> +	}
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(fake_fpga_region_unregister);
>>>> +
>>>> +/**
>>>> + * fake_fpga_region_add_bridge - add a bridge to a fake FPGA region
>>>> + * @region_ctx: fake FPGA region context data structure.
>>>> + * @bridge: FPGA bridge.
>>>> + *
>>>> + * Return: 0 if registration succeeded, an error code otherwise.
>>>> + */
>>>> +int fake_fpga_region_add_bridge(struct fake_fpga_region *region_ctx,
>>>> +				struct fpga_bridge *bridge)
>>>> +{
>>>> +	struct fake_region_priv *priv;
>>>> +	int ret;
>>>> +
>>>> +	priv = region_ctx->region->priv;
>>>> +
>>>> +	ret = fpga_bridge_get_to_list(bridge->dev.parent, NULL,
>>>> +				      &region_ctx->region->bridge_list);
>>>> +
>>>> +	if (priv->test && !ret)
>>>> +		kunit_info(priv->test, "Bridge added to fake FPGA region %d\n",
>>>> +			   priv->id);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(fake_fpga_region_add_bridge);
>>>> +
>>>> +static int fake_fpga_region_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct device *dev;
>>>> +	struct fpga_region *region;
>>>> +	struct fpga_manager *mgr;
>>>> +	struct fake_region_data *pdata;
>>>> +	struct fake_region_priv *priv;
>>>> +	static int id_count;
>>>> +
>>>> +	dev = &pdev->dev;
>>>> +	pdata = dev_get_platdata(dev);
>>>> +
>>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>> +	if (!priv)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	mgr = fpga_mgr_get(pdata->mgr->dev.parent);
>>>> +	if (IS_ERR(mgr))
>>>> +		return PTR_ERR(mgr);
>>>> +
>>>> +	/*
>>>> +	 * No get_bridges() method since the bridges list is
>>>> +	 * pre-built using fake_fpga_region_add_bridge()
>>>> +	 */
>>>
>>> This is not the common use for drivers to associate the region & bridge,
>>> Better to realize the get_bridges() method.
>>
>> Initially, I was using a get_bridges() method to create the list of bridges
>> before each reconfiguration. However, this required having a "duplicated"
>> list of bridges in the context of the fake region low-level driver.
>>
>> Since I couldn't find a reason to keep a duplicate list of bridges in the
>> fake region driver, I decided then to change the approach and build the
>> list of bridges at device instantiation time.
>>
>> In my understanding, the approach of creating the list of bridges just
>> before reconfiguration with a get_bridges() method works best for the
>> OF case, where the topology is already encoded in the DT. I feel using
>> this approach on platforms without OF support would increase complexity
>> and create unnecessary duplication.
> 
> I'm not fully get your point. My understanding is we don't have to
> always grab the bridge driver module if we don't reprogram. In many
> cases, we just work with the existing bitstream before Linux is started.
> So generally I prefer not to have an example that gets all bridges at
> initialization unless there is a real need.

The fake region can be used without bridges to model the scenario where
the FPGA is statically configured by the bootloader.

I was referring to the choice between building the bridge list of the
region (fpga_region->bridge_list) ahead of programming vs. just before
programming.

Currently, fake_fpga_region_add_bridge() attaches the bridge directly
to the bridge_list of the fpga_region struct.

Alternatively, I could change fake_fpga_region_add_bridge() to attach
the bridge to a secondary list in the low-level driver. The secondary
list would then be copied to the fpga_region->bridge_list by a
get_bridges() method just before programming.

However, I feel that using this approach would make test code more
complicated than necessary. Ideally, I would like to keep fake modules
as simple as possible.


Thanks,
Marco

Re: [RFC PATCH 1/4] fpga: add initial KUnit test suite

[Xu Yilun]

> >>>> +	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?
> 
> I looked through the documentation for guidelines on how to handle
> initialization errors, but found only the eeprom example where KUNIT_ASSERT
> is used to handle errors in eeprom_buffer_test_init(). Existing test suites
> seem to use different approaches to handle initialization errors. Some
> return an error code, while others use KUnit assertions.
> 
> I'm more inclined to follow the example in the documentation and use
> KUnit assertions. Does this approach work for you?

It's good to me.

> 
> 
> After some thought, I'm restructuring the code to test single components
> in isolation before testing them together. In this way, I think the test
> suite will be more in line with the unit testing methodology.
> 
> 
> Thanks,
> Marco
> 

Re: [RFC PATCH 2/4] fpga: add fake FPGA region

[Xu Yilun]

On 2023-03-01 at 11:51:44 +0100, Marco Pagani wrote:
> 
> 
> On 2023-02-24 08:20, Xu Yilun wrote:
> > On 2023-02-21 at 15:53:20 +0100, Marco Pagani wrote:
> >>
> >>
> >> On 2023-02-18 11:13, Xu Yilun wrote:
> >>> On 2023-02-03 at 18:06:51 +0100, Marco Pagani wrote:
> >>>> Add fake FPGA region platform driver with support functions. This
> >>>> module is part of the KUnit test suite for the FPGA subsystem.
> >>>>
> >>>> Signed-off-by: Marco Pagani <marpagan@redhat.com>
> >>>> ---
> >>>>  drivers/fpga/tests/fake-fpga-region.c | 186 ++++++++++++++++++++++++++
> >>>>  drivers/fpga/tests/fake-fpga-region.h |  37 +++++
> >>>>  2 files changed, 223 insertions(+)
> >>>>  create mode 100644 drivers/fpga/tests/fake-fpga-region.c
> >>>>  create mode 100644 drivers/fpga/tests/fake-fpga-region.h
> >>>>
> >>>> diff --git a/drivers/fpga/tests/fake-fpga-region.c b/drivers/fpga/tests/fake-fpga-region.c
> >>>> new file mode 100644
> >>>> index 000000000000..095397e41837
> >>>> --- /dev/null
> >>>> +++ b/drivers/fpga/tests/fake-fpga-region.c
> >>>> @@ -0,0 +1,186 @@
> >>>> +// SPDX-License-Identifier: GPL-2.0
> >>>> +/*
> >>>> + * Driver for fake FPGA region
> >>>> + *
> >>>> + * Copyright (C) 2023 Red Hat, Inc. All rights reserved.
> >>>> + *
> >>>> + * Author: Marco Pagani <marpagan@redhat.com>
> >>>> + */
> >>>> +
> >>>> +#include <linux/device.h>
> >>>> +#include <linux/platform_device.h>
> >>>> +#include <linux/fpga/fpga-mgr.h>
> >>>> +#include <linux/fpga/fpga-region.h>
> >>>> +#include <linux/fpga/fpga-bridge.h>
> >>>> +#include <kunit/test.h>
> >>>> +
> >>>> +#include "fake-fpga-region.h"
> >>>> +
> >>>> +#define FAKE_FPGA_REGION_DEV_NAME	"fake_fpga_region"
> >>>> +
> >>>> +struct fake_region_priv {
> >>>> +	int id;
> >>>> +	struct kunit *test;
> >>>> +};
> >>>> +
> >>>> +struct fake_region_data {
> >>>> +	struct fpga_manager *mgr;
> >>>> +	struct kunit *test;
> >>>> +};
> >>>> +
> >>>> +/**
> >>>> + * fake_fpga_region_register - register a fake FPGA region
> >>>> + * @region_ctx: fake FPGA region context data structure.
> >>>> + * @test: KUnit test context object.
> >>>> + *
> >>>> + * Return: 0 if registration succeeded, an error code otherwise.
> >>>> + */
> >>>> +int fake_fpga_region_register(struct fake_fpga_region *region_ctx,
> >>>> +			      struct fpga_manager *mgr, struct kunit *test)
> >>>> +{
> >>>> +	struct fake_region_data pdata;
> >>>> +	struct fake_region_priv *priv;
> >>>> +	int ret;
> >>>> +
> >>>> +	pdata.mgr = mgr;
> >>>> +	pdata.test = test;
> >>>> +
> >>>> +	region_ctx->pdev = platform_device_alloc(FAKE_FPGA_REGION_DEV_NAME,
> >>>> +						 PLATFORM_DEVID_AUTO);
> >>>> +	if (IS_ERR(region_ctx->pdev)) {
> >>>> +		pr_err("Fake FPGA region device allocation failed\n");
> >>>> +		return -ENOMEM;
> >>>> +	}
> >>>> +
> >>>> +	platform_device_add_data(region_ctx->pdev, &pdata, sizeof(pdata));
> >>>> +
> >>>> +	ret = platform_device_add(region_ctx->pdev);
> >>>> +	if (ret) {
> >>>> +		pr_err("Fake FPGA region device add failed\n");
> >>>> +		platform_device_put(region_ctx->pdev);
> >>>> +		return ret;
> >>>> +	}
> >>>> +
> >>>> +	region_ctx->region = platform_get_drvdata(region_ctx->pdev);
> >>>> +
> >>>> +	if (test) {
> >>>> +		priv = region_ctx->region->priv;
> >>>> +		kunit_info(test, "Fake FPGA region %d registered\n", priv->id);
> >>>> +	}
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(fake_fpga_region_register);
> >>>> +
> >>>> +/**
> >>>> + * fake_fpga_region_unregister - unregister a fake FPGA region
> >>>> + * @region_ctx: fake FPGA region context data structure.
> >>>> + */
> >>>> +void fake_fpga_region_unregister(struct fake_fpga_region *region_ctx)
> >>>> +{
> >>>> +	struct fake_region_priv *priv;
> >>>> +	struct kunit *test;
> >>>> +	int id;
> >>>> +
> >>>> +	priv = region_ctx->region->priv;
> >>>> +	test = priv->test;
> >>>> +	id = priv->id;
> >>>> +
> >>>> +	if (region_ctx->pdev) {
> >>>> +		platform_device_unregister(region_ctx->pdev);
> >>>> +		if (test)
> >>>> +			kunit_info(test, "Fake FPGA region %d unregistered\n", id);
> >>>> +	}
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(fake_fpga_region_unregister);
> >>>> +
> >>>> +/**
> >>>> + * fake_fpga_region_add_bridge - add a bridge to a fake FPGA region
> >>>> + * @region_ctx: fake FPGA region context data structure.
> >>>> + * @bridge: FPGA bridge.
> >>>> + *
> >>>> + * Return: 0 if registration succeeded, an error code otherwise.
> >>>> + */
> >>>> +int fake_fpga_region_add_bridge(struct fake_fpga_region *region_ctx,
> >>>> +				struct fpga_bridge *bridge)
> >>>> +{
> >>>> +	struct fake_region_priv *priv;
> >>>> +	int ret;
> >>>> +
> >>>> +	priv = region_ctx->region->priv;
> >>>> +
> >>>> +	ret = fpga_bridge_get_to_list(bridge->dev.parent, NULL,
> >>>> +				      &region_ctx->region->bridge_list);
> >>>> +
> >>>> +	if (priv->test && !ret)
> >>>> +		kunit_info(priv->test, "Bridge added to fake FPGA region %d\n",
> >>>> +			   priv->id);
> >>>> +
> >>>> +	return ret;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(fake_fpga_region_add_bridge);
> >>>> +
> >>>> +static int fake_fpga_region_probe(struct platform_device *pdev)
> >>>> +{
> >>>> +	struct device *dev;
> >>>> +	struct fpga_region *region;
> >>>> +	struct fpga_manager *mgr;
> >>>> +	struct fake_region_data *pdata;
> >>>> +	struct fake_region_priv *priv;
> >>>> +	static int id_count;
> >>>> +
> >>>> +	dev = &pdev->dev;
> >>>> +	pdata = dev_get_platdata(dev);
> >>>> +
> >>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >>>> +	if (!priv)
> >>>> +		return -ENOMEM;
> >>>> +
> >>>> +	mgr = fpga_mgr_get(pdata->mgr->dev.parent);
> >>>> +	if (IS_ERR(mgr))
> >>>> +		return PTR_ERR(mgr);
> >>>> +
> >>>> +	/*
> >>>> +	 * No get_bridges() method since the bridges list is
> >>>> +	 * pre-built using fake_fpga_region_add_bridge()
> >>>> +	 */
> >>>
> >>> This is not the common use for drivers to associate the region & bridge,
> >>> Better to realize the get_bridges() method.
> >>
> >> Initially, I was using a get_bridges() method to create the list of bridges
> >> before each reconfiguration. However, this required having a "duplicated"
> >> list of bridges in the context of the fake region low-level driver.
> >>
> >> Since I couldn't find a reason to keep a duplicate list of bridges in the
> >> fake region driver, I decided then to change the approach and build the
> >> list of bridges at device instantiation time.
> >>
> >> In my understanding, the approach of creating the list of bridges just
> >> before reconfiguration with a get_bridges() method works best for the
> >> OF case, where the topology is already encoded in the DT. I feel using
> >> this approach on platforms without OF support would increase complexity
> >> and create unnecessary duplication.
> > 
> > I'm not fully get your point. My understanding is we don't have to
> > always grab the bridge driver module if we don't reprogram. In many
> > cases, we just work with the existing bitstream before Linux is started.
> > So generally I prefer not to have an example that gets all bridges at
> > initialization unless there is a real need.
> 
> The fake region can be used without bridges to model the scenario where
> the FPGA is statically configured by the bootloader.
> 
> I was referring to the choice between building the bridge list of the
> region (fpga_region->bridge_list) ahead of programming vs. just before
> programming.
> 
> Currently, fake_fpga_region_add_bridge() attaches the bridge directly
> to the bridge_list of the fpga_region struct.
> 
> Alternatively, I could change fake_fpga_region_add_bridge() to attach
> the bridge to a secondary list in the low-level driver. The secondary
> list would then be copied to the fpga_region->bridge_list by a
> get_bridges() method just before programming.

I prefer to attach bridges just before programming in the test, as this
is the logic for fpga region core and all fpga drivers now. It is better
the first few tests covers the normal workflow.

Thanks,
Yilun

> 
> However, I feel that using this approach would make test code more
> complicated than necessary. Ideally, I would like to keep fake modules
> as simple as possible.
> 
> 
> Thanks,
> Marco
>