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: <a50022a6-59e1-6b53-2c5b-c6eb44277876@redhat.com>
Date:   Mon, 15 May 2023 19:24:07 +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>
Content-Language: en-US
From:   Marco Pagani <marpagan@redhat.com>
In-Reply-To: <ZF/LpdlyBu2Z1uQE@yilunxu-OptiPlex-7050>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


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.

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.

What do you think?

Thanks,
Marco

[...]