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: <6f3ba18c-8724-f4ae-8267-719cc6f45c69@redhat.com>
Date:   Thu, 4 May 2023 22:05:43 +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 v4 1/4] fpga: add fake FPGA manager
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: <20230417122308.131453-1-marpagan@redhat.com>
 <20230417122308.131453-2-marpagan@redhat.com>
 <ZEGE+UqTyKGTyv9h@yilunxu-OptiPlex-7050>
 <cef792a1-42c1-b262-b07e-529006fdcb6d@redhat.com>
 <594789b2-eb5d-11fc-9c47-310bdb258f7c@redhat.com>
 <ZFOkPNOmbjsalTmn@yilunxu-OptiPlex-7050>
Content-Language: en-US
From:   Marco Pagani <marpagan@redhat.com>
In-Reply-To: <ZFOkPNOmbjsalTmn@yilunxu-OptiPlex-7050>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


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
>>
>>
>>>>> [...]
>>
>