Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f9b7d388-9a8b-0663-ea02-892606950907@redhat.com>
Date:   Thu, 18 May 2023 14:43:45 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [RFC PATCH v5 4/4] fpga: add initial KUnit test suites
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
References: <20230511141922.437328-1-marpagan@redhat.com>
 <20230511141922.437328-5-marpagan@redhat.com>
 <ZF/LpdlyBu2Z1uQE@yilunxu-OptiPlex-7050>
 <a50022a6-59e1-6b53-2c5b-c6eb44277876@redhat.com>
 <ZGSmtCkPwi2TMW7K@yilunxu-OptiPlex-7050>
Content-Language: en-US
From:   Marco Pagani <marpagan@redhat.com>
In-Reply-To: <ZGSmtCkPwi2TMW7K@yilunxu-OptiPlex-7050>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


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