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@xxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxx>
>>> + */
>>> +
>>> +#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
>>> [...]
