From: Marco Pagani <marpagan@redhat.com>
To: Xu Yilun <yilun.xu@intel.com>
Cc: Moritz Fischer <mdf@kernel.org>, Wu Hao <hao.wu@intel.com>,
Tom Rix <trix@redhat.com>,
linux-kernel@vger.kernel.org, linux-fpga@vger.kernel.org
Subject: Re: [RFC PATCH v5 4/4] fpga: add initial KUnit test suites
Date: Thu, 18 May 2023 14:43:45 +0200 [thread overview]
Message-ID: <f9b7d388-9a8b-0663-ea02-892606950907@redhat.com> (raw)
In-Reply-To: <ZGSmtCkPwi2TMW7K@yilunxu-OptiPlex-7050>
On 2023-05-17 12:04, Xu Yilun wrote:
> On 2023-05-15 at 19:24:07 +0200, Marco Pagani wrote:
>>
>>
>> On 2023-05-13 19:40, Xu Yilun wrote:
>>> On 2023-05-11 at 16:19:22 +0200, Marco Pagani wrote:
>>>> Introduce initial KUnit tests for the FPGA subsystem. Tests are organized
>>>> into three test suites. The first suite tests the FPGA Manager.
>>>> The second suite tests the FPGA Bridge. Finally, the last test suite
>>>> models a complete FPGA platform and tests static and partial reconfiguration.
>>>>
>>>> Signed-off-by: Marco Pagani <marpagan@redhat.com>
>>
>> [...]
>>
>>>> +static void fpga_bridge_test_get_put_list(struct kunit *test)
>>>> +{
>>>> + struct list_head bridge_list;
>>>> + struct fake_fpga_bridge *bridge_0_ctx, *bridge_1_ctx;
>>>> + int ret;
>>>> +
>>>> + bridge_0_ctx = test->priv;
>>>> +
>>>> + /* Register another bridge for this test */
>>>> + bridge_1_ctx = fake_fpga_bridge_register(test, NULL);
>>>> + KUNIT_ASSERT_FALSE(test, IS_ERR(bridge_1_ctx));
>>>
>>> I think bridge_1 could also be initialized in test_init together with
>>> bridge_0
>>
>> I can do it, but it would remain unused in the previous test case.
>>
>>>> +
>>>> + INIT_LIST_HEAD(&bridge_list);
>>>> +
>>>> + /* Get bridge_0 and add it to the list */
>>>> + ret = fpga_bridge_get_to_list(bridge_1_ctx->bridge->dev.parent, NULL,
>>>> + &bridge_list);
>>>> + KUNIT_EXPECT_EQ(test, ret, 0);
>>>> +
>>>> + KUNIT_EXPECT_PTR_EQ(test, bridge_1_ctx->bridge,
>>>> + list_first_entry_or_null(&bridge_list, struct fpga_bridge, node));
>>>
>>> Should operate on bridge_0_ctx?
>>
>> Yes, sorry. Code and comments are reversed. I'll fix it in the next version.
>>
>>>> +
>>>> + /* Get bridge_1 and add it to the list */
>>>> + ret = fpga_bridge_get_to_list(bridge_0_ctx->bridge->dev.parent, NULL,
>>>> + &bridge_list);
>>>> + KUNIT_EXPECT_EQ(test, ret, 0);
>>>> +
>>>> + KUNIT_EXPECT_PTR_EQ(test, bridge_0_ctx->bridge,
>>>> + list_first_entry_or_null(&bridge_list, struct fpga_bridge, node));
>>>
>>> Should operate on bridge_1_ctx?
>>
>> Same.
>>
>>>> +
>>>> + /* Disable an then enable both bridges from the list */
>>>> + KUNIT_EXPECT_TRUE(test, bridge_0_ctx->stats.enable);
>>>
>>> Why expect enable without fpga_bridges_enable()?
>>
>> To check that the bridge is initialized in the correct (enabled) state.
>>
>> [...]
>>
>>>> +static void fpga_test_partial_rcfg(struct kunit *test)
>>>> +{
>>>> + struct fpga_base_ctx *base_ctx;
>>>> + struct fake_fpga_region *sub_region_0_ctx, *sub_region_1_ctx;
>>>> + struct fake_fpga_bridge *sub_bridge_0_ctx, *sub_bridge_1_ctx;
>>>> + struct fpga_image_info *partial_img_info;
>>>> + int ret;
>>>> +
>>>> + base_ctx = test->priv;
>>>> +
>>>> + /*
>>>> + * Add two reconfigurable sub-regions, each controlled by a bridge. The
>>>> + * reconfigurable sub-region are children of their bridges which are,
>>>> + * in turn, children of the base region. For simplicity, the same image
>>>> + * is used to configure reconfigurable regions
>>>> + */
>>>> + sub_bridge_0_ctx = fake_fpga_bridge_register(test,
>>>> + &base_ctx->region_ctx->region->dev);
>>>> + KUNIT_ASSERT_FALSE(test, IS_ERR(sub_bridge_0_ctx));
>>>> +
>>>> + sub_region_0_ctx = fake_fpga_region_register(test, base_ctx->mgr_ctx->mgr,
>>>> + &sub_bridge_0_ctx->bridge->dev);
>>>> + KUNIT_ASSERT_FALSE(test, IS_ERR(sub_region_0_ctx));
>>>> +
>>>> + ret = fake_fpga_region_add_bridge(sub_region_0_ctx, sub_bridge_0_ctx->bridge);
>>>> + KUNIT_ASSERT_EQ(test, ret, 0);
>>>> +
>>>> + sub_bridge_1_ctx = fake_fpga_bridge_register(test,
>>>> + &base_ctx->region_ctx->region->dev);
>>>> + KUNIT_ASSERT_FALSE(test, IS_ERR(sub_bridge_1_ctx));
>>>> +
>>>> + sub_region_1_ctx = fake_fpga_region_register(test, base_ctx->mgr_ctx->mgr,
>>>> + &sub_bridge_1_ctx->bridge->dev);
>>>> + KUNIT_ASSERT_FALSE(test, IS_ERR(sub_region_1_ctx));
>>>> +
>>>> + ret = fake_fpga_region_add_bridge(sub_region_1_ctx, sub_bridge_1_ctx->bridge);
>>>> + KUNIT_ASSERT_EQ(test, ret, 0);
>>>
>>> I'm wondering if we need to construct the topology for partial
>>> reconfiguration test. The FPGA core doesn't actually check the topology.
>>> It is OK to do partial reconfiguration for a region without parents as
>>> long as its associated FPGA manager device has the capability.
>>>
>>> Thanks,
>>> Yilun
>>
>> I agree with you. Creating a hierarchical layout is rather unnecessary.
>>
>
> I assume the following sections have nothing to do with hierarchial
> layout, is it?
>
It was a general summary to put things in perspective and ask your opinion
before moving forward with the next version.
>> Initially, the idea was to test that all components behave as expected
>> in a complete setup, e.g., only the bridge of the specific reconfigurable
>> region gets disabled during programming and then re-enabled.
>>
>> However, after some iterations, I'm starting to think that it would be
>> better to restructure the whole test code into a set of self-contained
>> test modules, one for each core component.
>>
>> In that way, each module would contain the implementation of the fake/mock
>> low-level driver and the related tests. For instance, the manager module
>> would contain the implementation of the fake manager and the test_img_load_buf
>> and test_img_load_sgt test cases. Similarly, the bridge module would contain
>> the fake/mock bridge implementation and the test_toggle and test_get_put_list
>> cases.
>>
>> I think that in this way, the code would be simpler and more adherent to the
>> unit testing methodology. The downside is that making tests that need multiple
>> components would be more cumbersome and possibly lead to code duplication.
>> For instance, testing the region's fpga_region_program_fpga() would require
>> implementing additional local mock/fakes for the manager and bridge.
>
> This way is good to me.
>
Okay, I'll move toward multiple test modules for v6.
>>
>> What do you think?
>>
>> Thanks,
>> Marco
>>
>> [...]
>>
>
Thanks,
Marco
prev parent reply other threads:[~2023-05-18 12:46 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-11 14:19 [RFC PATCH v5 0/4] fpga: add initial KUnit tests for the subsystem Marco Pagani
2023-05-11 14:19 ` [RFC PATCH v5 1/4] fpga: add fake FPGA manager Marco Pagani
2023-05-13 15:44 ` Xu Yilun
2023-05-11 14:19 ` [RFC PATCH v5 2/4] fpga: add fake FPGA bridge Marco Pagani
2023-05-13 15:46 ` Xu Yilun
2023-05-11 14:19 ` [RFC PATCH v5 3/4] fpga: add fake FPGA region Marco Pagani
2023-05-13 16:05 ` Xu Yilun
2023-05-11 14:19 ` [RFC PATCH v5 4/4] fpga: add initial KUnit test suites Marco Pagani
2023-05-13 17:40 ` Xu Yilun
2023-05-15 17:24 ` Marco Pagani
2023-05-17 10:04 ` Xu Yilun
2023-05-18 12:43 ` Marco Pagani [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f9b7d388-9a8b-0663-ea02-892606950907@redhat.com \
--to=marpagan@redhat.com \
--cc=hao.wu@intel.com \
--cc=linux-fpga@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mdf@kernel.org \
--cc=trix@redhat.com \
--cc=yilun.xu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).