Message ID | 20230417122308.131453-2-marpagan@redhat.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | fpga: add initial KUnit tests for the subsystem | expand |
On 2023-04-17 at 14:23:05 +0200, Marco Pagani wrote: > 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 tests for the FPGA subsystem. > > Signed-off-by: Marco Pagani <marpagan@redhat.com> > --- > drivers/fpga/tests/fake-fpga-mgr.c | 386 +++++++++++++++++++++++++++++ > drivers/fpga/tests/fake-fpga-mgr.h | 43 ++++ > 2 files changed, 429 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..636df637b291 > --- /dev/null > +++ b/drivers/fpga/tests/fake-fpga-mgr.c > @@ -0,0 +1,386 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Driver for the fake FPGA manager > + * > + * Copyright (C) 2023 Red Hat, Inc. > + * > + * 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_IMG_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: pointer to a new fake FPGA manager on success, an ERR_PTR() > + * encoded error code on failure. > + */ > +struct fake_fpga_mgr * > +fake_fpga_mgr_register(struct kunit *test, struct device *parent) > +{ > + struct fake_fpga_mgr *mgr_ctx; > + struct fake_mgr_data pdata; > + int ret; > + > + mgr_ctx = kzalloc(sizeof(*mgr_ctx), GFP_KERNEL); > + if (!mgr_ctx) { > + ret = -ENOMEM; > + goto err_mem; > + } > + > + mgr_ctx->pdev = platform_device_alloc(FAKE_FPGA_MGR_DEV_NAME, > + PLATFORM_DEVID_AUTO); > + if (!mgr_ctx->pdev) { > + pr_err("Fake FPGA manager device allocation failed\n"); > + ret = -ENOMEM; > + goto err_mem; > + } > + > + pdata.test = test; > + platform_device_add_data(mgr_ctx->pdev, &pdata, sizeof(pdata)); > + > + mgr_ctx->pdev->dev.parent = parent; > + ret = platform_device_add(mgr_ctx->pdev); > + if (ret) { > + pr_err("Fake FPGA manager device add failed\n"); > + goto err_pdev; > + } > + > + mgr_ctx->mgr = platform_get_drvdata(mgr_ctx->pdev); > + > + if (test) > + kunit_info(test, "Fake FPGA manager registered\n"); > + > + return mgr_ctx; > + > +err_pdev: > + platform_device_put(mgr_ctx->pdev); > + kfree(mgr_ctx); > +err_mem: > + return ERR_PTR(ret); > +} > +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; > + > + if (!mgr_ctx) > + return; > + > + 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"); > + } > + > + kfree(mgr_ctx); > +} > +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 an image buffer with the test header. > + * @buf: image buffer. > + */ > +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_sgt() - check if programming using a s.g. table succeeded. > + * @mgr_ctx: fake FPGA manager context data structure. > + */ > +void fake_fpga_mgr_check_write_sgt(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_sgt); I'm wondering, if we could move all these exported functions out of fake_fpga driver module. And make this driver module serves FPGA mgr framework only, just like other fpga drivers do. I assume the main requirement is to check the statistics produced by the fake fpga driver. Directly accessing mgr->priv outside the driver could be unwanted. To solve this, could we create a shared buffer for the statistics and pass to fake drivers by platform data. I hope move all the tester's actions in fpga-test.c, so that people could easily see from code what a user need to do to enable fpga reprogramming and what are expected in one file. The fake drivers could be kept as simple, they only move the process forward and produce statistics. Thanks, Yilun > + > +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; > + > + if (pdata) > + 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..453672b5df72 > --- /dev/null > +++ b/drivers/fpga/tests/fake-fpga-mgr.h > @@ -0,0 +1,43 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Header file for the fake FPGA manager > + * > + * Copyright (C) 2023 Red Hat, Inc. > + * > + * 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_IMG_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; > +}; > + > +struct fake_fpga_mgr * > +fake_fpga_mgr_register(struct kunit *test, struct device *parent); > + > +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_sgt(struct fake_fpga_mgr *mgr_ctx); > + > +#endif /* __FPGA_FAKE_MGR_H */ > -- > 2.39.2 >
On 2023-04-20 20:31, Xu Yilun wrote: > On 2023-04-17 at 14:23:05 +0200, Marco Pagani wrote: >> 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 tests for the FPGA subsystem. >> >> Signed-off-by: Marco Pagani <marpagan@redhat.com> >> --- >> drivers/fpga/tests/fake-fpga-mgr.c | 386 +++++++++++++++++++++++++++++ >> drivers/fpga/tests/fake-fpga-mgr.h | 43 ++++ >> 2 files changed, 429 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..636df637b291 >> --- /dev/null >> +++ b/drivers/fpga/tests/fake-fpga-mgr.c >> @@ -0,0 +1,386 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Driver for the fake FPGA manager >> + * >> + * Copyright (C) 2023 Red Hat, Inc. >> + * >> + * 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_IMG_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: pointer to a new fake FPGA manager on success, an ERR_PTR() >> + * encoded error code on failure. >> + */ >> +struct fake_fpga_mgr * >> +fake_fpga_mgr_register(struct kunit *test, struct device *parent) >> +{ >> + struct fake_fpga_mgr *mgr_ctx; >> + struct fake_mgr_data pdata; >> + int ret; >> + >> + mgr_ctx = kzalloc(sizeof(*mgr_ctx), GFP_KERNEL); >> + if (!mgr_ctx) { >> + ret = -ENOMEM; >> + goto err_mem; >> + } >> + >> + mgr_ctx->pdev = platform_device_alloc(FAKE_FPGA_MGR_DEV_NAME, >> + PLATFORM_DEVID_AUTO); >> + if (!mgr_ctx->pdev) { >> + pr_err("Fake FPGA manager device allocation failed\n"); >> + ret = -ENOMEM; >> + goto err_mem; >> + } >> + >> + pdata.test = test; >> + platform_device_add_data(mgr_ctx->pdev, &pdata, sizeof(pdata)); >> + >> + mgr_ctx->pdev->dev.parent = parent; >> + ret = platform_device_add(mgr_ctx->pdev); >> + if (ret) { >> + pr_err("Fake FPGA manager device add failed\n"); >> + goto err_pdev; >> + } >> + >> + mgr_ctx->mgr = platform_get_drvdata(mgr_ctx->pdev); >> + >> + if (test) >> + kunit_info(test, "Fake FPGA manager registered\n"); >> + >> + return mgr_ctx; >> + >> +err_pdev: >> + platform_device_put(mgr_ctx->pdev); >> + kfree(mgr_ctx); >> +err_mem: >> + return ERR_PTR(ret); >> +} >> +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; >> + >> + if (!mgr_ctx) >> + return; >> + >> + 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"); >> + } >> + >> + kfree(mgr_ctx); >> +} >> +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 an image buffer with the test header. >> + * @buf: image buffer. >> + */ >> +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_sgt() - check if programming using a s.g. table succeeded. >> + * @mgr_ctx: fake FPGA manager context data structure. >> + */ >> +void fake_fpga_mgr_check_write_sgt(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_sgt); > > I'm wondering, if we could move all these exported functions out of > fake_fpga driver module. And make this driver module serves FPGA > mgr framework only, just like other fpga drivers do. > > I assume the main requirement is to check the statistics produced > by the fake fpga driver. Directly accessing mgr->priv outside the > driver could be unwanted. To solve this, could we create a shared > buffer for the statistics and pass to fake drivers by platform data. > > I hope move all the tester's actions in fpga-test.c, so that people > could easily see from code what a user need to do to enable fpga > reprogramming and what are expected in one file. The fake drivers could > be kept as simple, they only move the process forward and produce > statistics. > > Thanks, > Yilun > I agree with you. Initially, I wanted to keep all KUnit test assertions and expectations contained in fpga-test. However, I could not find a simple way to test that the FPGA manager performs the correct state transitions during programming. So I ended up putting KUnit assertions in the methods of the low-level fake driver as a first solution. I like your suggestion of using a shared buffer to have a cleaner implementation. My only concern is that it would make the code more complex. I will work on this for V5. Thanks, Marco >> + >> +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; >> + >> + if (pdata) >> + 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..453672b5df72 >> --- /dev/null >> +++ b/drivers/fpga/tests/fake-fpga-mgr.h >> @@ -0,0 +1,43 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* >> + * Header file for the fake FPGA manager >> + * >> + * Copyright (C) 2023 Red Hat, Inc. >> + * >> + * 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_IMG_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; >> +}; >> + >> +struct fake_fpga_mgr * >> +fake_fpga_mgr_register(struct kunit *test, struct device *parent); >> + >> +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_sgt(struct fake_fpga_mgr *mgr_ctx); >> + >> +#endif /* __FPGA_FAKE_MGR_H */ >> -- >> 2.39.2 >> >
On 2023-04-26 17:44, Marco Pagani wrote: > > > On 2023-04-20 20:31, Xu Yilun wrote: >> On 2023-04-17 at 14:23:05 +0200, Marco Pagani wrote: >>> 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 tests for the FPGA subsystem. >>> >>> Signed-off-by: Marco Pagani <marpagan@redhat.com> >>> --- >>> drivers/fpga/tests/fake-fpga-mgr.c | 386 +++++++++++++++++++++++++++++ >>> drivers/fpga/tests/fake-fpga-mgr.h | 43 ++++ >>> 2 files changed, 429 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..636df637b291 >>> --- /dev/null >>> +++ b/drivers/fpga/tests/fake-fpga-mgr.c >>> @@ -0,0 +1,386 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * Driver for the fake FPGA manager >>> + * >>> + * Copyright (C) 2023 Red Hat, Inc. >>> + * >>> + * 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_IMG_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: pointer to a new fake FPGA manager on success, an ERR_PTR() >>> + * encoded error code on failure. >>> + */ >>> +struct fake_fpga_mgr * >>> +fake_fpga_mgr_register(struct kunit *test, struct device *parent) >>> +{ >>> + struct fake_fpga_mgr *mgr_ctx; >>> + struct fake_mgr_data pdata; >>> + int ret; >>> + >>> + mgr_ctx = kzalloc(sizeof(*mgr_ctx), GFP_KERNEL); >>> + if (!mgr_ctx) { >>> + ret = -ENOMEM; >>> + goto err_mem; >>> + } >>> + >>> + mgr_ctx->pdev = platform_device_alloc(FAKE_FPGA_MGR_DEV_NAME, >>> + PLATFORM_DEVID_AUTO); >>> + if (!mgr_ctx->pdev) { >>> + pr_err("Fake FPGA manager device allocation failed\n"); >>> + ret = -ENOMEM; >>> + goto err_mem; >>> + } >>> + >>> + pdata.test = test; >>> + platform_device_add_data(mgr_ctx->pdev, &pdata, sizeof(pdata)); >>> + >>> + mgr_ctx->pdev->dev.parent = parent; >>> + ret = platform_device_add(mgr_ctx->pdev); >>> + if (ret) { >>> + pr_err("Fake FPGA manager device add failed\n"); >>> + goto err_pdev; >>> + } >>> + >>> + mgr_ctx->mgr = platform_get_drvdata(mgr_ctx->pdev); >>> + >>> + if (test) >>> + kunit_info(test, "Fake FPGA manager registered\n"); >>> + >>> + return mgr_ctx; >>> + >>> +err_pdev: >>> + platform_device_put(mgr_ctx->pdev); >>> + kfree(mgr_ctx); >>> +err_mem: >>> + return ERR_PTR(ret); >>> +} >>> +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; >>> + >>> + if (!mgr_ctx) >>> + return; >>> + >>> + 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"); >>> + } >>> + >>> + kfree(mgr_ctx); >>> +} >>> +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 an image buffer with the test header. >>> + * @buf: image buffer. >>> + */ >>> +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_sgt() - check if programming using a s.g. table succeeded. >>> + * @mgr_ctx: fake FPGA manager context data structure. >>> + */ >>> +void fake_fpga_mgr_check_write_sgt(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_sgt); >> >> I'm wondering, if we could move all these exported functions out of >> fake_fpga driver module. And make this driver module serves FPGA >> mgr framework only, just like other fpga drivers do. >> >> I assume the main requirement is to check the statistics produced >> by the fake fpga driver. Directly accessing mgr->priv outside the >> driver could be unwanted. To solve this, could we create a shared >> buffer for the statistics and pass to fake drivers by platform data. >> >> I hope move all the tester's actions in fpga-test.c, so that people >> could easily see from code what a user need to do to enable fpga >> reprogramming and what are expected in one file. The fake drivers could >> be kept as simple, they only move the process forward and produce >> statistics. >> >> Thanks, >> Yilun >> > > I agree with you. Initially, I wanted to keep all KUnit test assertions > and expectations contained in fpga-test. However, I could not find a simple > way to test that the FPGA manager performs the correct state transitions > during programming. So I ended up putting KUnit assertions in the methods > of the low-level fake driver as a first solution. > > I like your suggestion of using a shared buffer to have a cleaner > implementation. My only concern is that it would make the code more complex. > I will work on this for V5. > I experimented with a couple of alternatives to move all tests inside fpga-test and remove the external functions. Unfortunately, each alternative comes with its drawbacks. Using a shared buffer (e.g., kfifo) to implement an events buffer between fake mgr/bridge and the fpga-test overcomplicates the code (i.e., defining message structs, enums for the operations, locks, etc.). Moving fake modules' (mgr, bridge, region) implementations inside fpga-test makes fpga-test monolithic and harder to understand and maintain. Accessing modules' private data directly from fpga-test breaks encapsulation. Overall, it seems to me that using external functions to get the state of fake modules is the least-worst alternative. What are your thoughts and preferences? Thanks, Marco >>> [...]
On 2023-05-03 at 18:53:02 +0200, Marco Pagani wrote: > On 2023-04-26 17:44, Marco Pagani wrote: > > > > > > On 2023-04-20 20:31, Xu Yilun wrote: > >> On 2023-04-17 at 14:23:05 +0200, Marco Pagani wrote: > >>> 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 tests for the FPGA subsystem. > >>> > >>> Signed-off-by: Marco Pagani <marpagan@redhat.com> [...] > >>> +EXPORT_SYMBOL_GPL(fake_fpga_mgr_check_write_sgt); > >> > >> I'm wondering, if we could move all these exported functions out of > >> fake_fpga driver module. And make this driver module serves FPGA > >> mgr framework only, just like other fpga drivers do. > >> > >> I assume the main requirement is to check the statistics produced > >> by the fake fpga driver. Directly accessing mgr->priv outside the > >> driver could be unwanted. To solve this, could we create a shared > >> buffer for the statistics and pass to fake drivers by platform data. > >> > >> I hope move all the tester's actions in fpga-test.c, so that people > >> could easily see from code what a user need to do to enable fpga > >> reprogramming and what are expected in one file. The fake drivers could > >> be kept as simple, they only move the process forward and produce > >> statistics. > >> > >> Thanks, > >> Yilun > >> > > > > I agree with you. Initially, I wanted to keep all KUnit test assertions > > and expectations contained in fpga-test. However, I could not find a simple > > way to test that the FPGA manager performs the correct state transitions > > during programming. So I ended up putting KUnit assertions in the methods > > of the low-level fake driver as a first solution. > > > > I like your suggestion of using a shared buffer to have a cleaner > > implementation. My only concern is that it would make the code more complex. > > I will work on this for V5. > > > > I experimented with a couple of alternatives to move all tests inside > fpga-test and remove the external functions. Unfortunately, each alternative > comes with its drawbacks. > > Using a shared buffer (e.g., kfifo) to implement an events buffer between > fake mgr/bridge and the fpga-test overcomplicates the code (i.e., defining > message structs, enums for the operations, locks, etc.). Oh, I actually didn't expect a message based mechanism for statistics reading, which is overcomplicated for a test. Maybe just pass a structured data buffer via platform_data, so that both fpga-test & fake drivers could recognize and access it directly. fpga-test could directly check the updated statistics after reprograming and assert them. Is that OK for you? Thanks, Yilun > > Moving fake modules' (mgr, bridge, region) implementations inside fpga-test > makes fpga-test monolithic and harder to understand and maintain. > > Accessing modules' private data directly from fpga-test breaks encapsulation. > > Overall, it seems to me that using external functions to get the state of fake > modules is the least-worst alternative. What are your thoughts and preferences? > > Thanks, > Marco > > > >>> [...] >
On 2023-05-04 14:25, Xu Yilun wrote: > On 2023-05-03 at 18:53:02 +0200, Marco Pagani wrote: >> On 2023-04-26 17:44, Marco Pagani wrote: >>> >>> >>> On 2023-04-20 20:31, Xu Yilun wrote: >>>> On 2023-04-17 at 14:23:05 +0200, Marco Pagani wrote: >>>>> 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 tests for the FPGA subsystem. >>>>> >>>>> Signed-off-by: Marco Pagani <marpagan@redhat.com> > > [...] > >>>>> +EXPORT_SYMBOL_GPL(fake_fpga_mgr_check_write_sgt); >>>> >>>> I'm wondering, if we could move all these exported functions out of >>>> fake_fpga driver module. And make this driver module serves FPGA >>>> mgr framework only, just like other fpga drivers do. >>>> >>>> I assume the main requirement is to check the statistics produced >>>> by the fake fpga driver. Directly accessing mgr->priv outside the >>>> driver could be unwanted. To solve this, could we create a shared >>>> buffer for the statistics and pass to fake drivers by platform data. >>>> >>>> I hope move all the tester's actions in fpga-test.c, so that people >>>> could easily see from code what a user need to do to enable fpga >>>> reprogramming and what are expected in one file. The fake drivers could >>>> be kept as simple, they only move the process forward and produce >>>> statistics. >>>> >>>> Thanks, >>>> Yilun >>>> >>> >>> I agree with you. Initially, I wanted to keep all KUnit test assertions >>> and expectations contained in fpga-test. However, I could not find a simple >>> way to test that the FPGA manager performs the correct state transitions >>> during programming. So I ended up putting KUnit assertions in the methods >>> of the low-level fake driver as a first solution. >>> >>> I like your suggestion of using a shared buffer to have a cleaner >>> implementation. My only concern is that it would make the code more complex. >>> I will work on this for V5. >>> >> >> I experimented with a couple of alternatives to move all tests inside >> fpga-test and remove the external functions. Unfortunately, each alternative >> comes with its drawbacks. >> >> Using a shared buffer (e.g., kfifo) to implement an events buffer between >> fake mgr/bridge and the fpga-test overcomplicates the code (i.e., defining >> message structs, enums for the operations, locks, etc.). > > Oh, I actually didn't expect a message based mechanism for statistics > reading, which is overcomplicated for a test. > > Maybe just pass a structured data buffer via platform_data, so that both > fpga-test & fake drivers could recognize and access it directly. fpga-test > could directly check the updated statistics after reprograming and assert > them. Is that OK for you? > > Thanks, > Yilun In principle, yes. I was just concerned that testing all properties with a plain buffer wouldn't be feasible. With the current implementation, fake-fpga-mgr tests two distinct sets of properties: (i) all required ops are called during programming, and (ii) the fpga mgr is in the correct state when each op is called. I was convinced that testing (ii) would have required some sort of events queue to trace the calls. However, on second thought, I might simply use fpga_mgr_states enums to test (ii) using a plain buffer. Moreover, by adding sequence numbers, we could also test an additional property (iii) ops are called in the right order. I'll follow the shared buffer approach for V5. Thanks, Marco > >> >> Moving fake modules' (mgr, bridge, region) implementations inside fpga-test >> makes fpga-test monolithic and harder to understand and maintain. >> >> Accessing modules' private data directly from fpga-test breaks encapsulation. >> >> Overall, it seems to me that using external functions to get the state of fake >> modules is the least-worst alternative. What are your thoughts and preferences? >> >> Thanks, >> Marco >> >> >>>>> [...] >> >
diff --git a/drivers/fpga/tests/fake-fpga-mgr.c b/drivers/fpga/tests/fake-fpga-mgr.c new file mode 100644 index 000000000000..636df637b291 --- /dev/null +++ b/drivers/fpga/tests/fake-fpga-mgr.c @@ -0,0 +1,386 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Driver for the fake FPGA manager + * + * Copyright (C) 2023 Red Hat, Inc. + * + * 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_IMG_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: pointer to a new fake FPGA manager on success, an ERR_PTR() + * encoded error code on failure. + */ +struct fake_fpga_mgr * +fake_fpga_mgr_register(struct kunit *test, struct device *parent) +{ + struct fake_fpga_mgr *mgr_ctx; + struct fake_mgr_data pdata; + int ret; + + mgr_ctx = kzalloc(sizeof(*mgr_ctx), GFP_KERNEL); + if (!mgr_ctx) { + ret = -ENOMEM; + goto err_mem; + } + + mgr_ctx->pdev = platform_device_alloc(FAKE_FPGA_MGR_DEV_NAME, + PLATFORM_DEVID_AUTO); + if (!mgr_ctx->pdev) { + pr_err("Fake FPGA manager device allocation failed\n"); + ret = -ENOMEM; + goto err_mem; + } + + pdata.test = test; + platform_device_add_data(mgr_ctx->pdev, &pdata, sizeof(pdata)); + + mgr_ctx->pdev->dev.parent = parent; + ret = platform_device_add(mgr_ctx->pdev); + if (ret) { + pr_err("Fake FPGA manager device add failed\n"); + goto err_pdev; + } + + mgr_ctx->mgr = platform_get_drvdata(mgr_ctx->pdev); + + if (test) + kunit_info(test, "Fake FPGA manager registered\n"); + + return mgr_ctx; + +err_pdev: + platform_device_put(mgr_ctx->pdev); + kfree(mgr_ctx); +err_mem: + return ERR_PTR(ret); +} +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; + + if (!mgr_ctx) + return; + + 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"); + } + + kfree(mgr_ctx); +} +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 an image buffer with the test header. + * @buf: image buffer. + */ +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_sgt() - check if programming using a s.g. table succeeded. + * @mgr_ctx: fake FPGA manager context data structure. + */ +void fake_fpga_mgr_check_write_sgt(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_sgt); + +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; + + if (pdata) + 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..453672b5df72 --- /dev/null +++ b/drivers/fpga/tests/fake-fpga-mgr.h @@ -0,0 +1,43 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Header file for the fake FPGA manager + * + * Copyright (C) 2023 Red Hat, Inc. + * + * 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_IMG_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; +}; + +struct fake_fpga_mgr * +fake_fpga_mgr_register(struct kunit *test, struct device *parent); + +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_sgt(struct fake_fpga_mgr *mgr_ctx); + +#endif /* __FPGA_FAKE_MGR_H */
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 tests for the FPGA subsystem. Signed-off-by: Marco Pagani <marpagan@redhat.com> --- drivers/fpga/tests/fake-fpga-mgr.c | 386 +++++++++++++++++++++++++++++ drivers/fpga/tests/fake-fpga-mgr.h | 43 ++++ 2 files changed, 429 insertions(+) create mode 100644 drivers/fpga/tests/fake-fpga-mgr.c create mode 100644 drivers/fpga/tests/fake-fpga-mgr.h