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 v4 1/4] fpga: add fake FPGA manager
Date: Thu, 4 May 2023 22:05:43 +0200 [thread overview]
Message-ID: <6f3ba18c-8724-f4ae-8267-719cc6f45c69@redhat.com> (raw)
In-Reply-To: <ZFOkPNOmbjsalTmn@yilunxu-OptiPlex-7050>
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
>>
>>
>>>>> [...]
>>
>
next prev parent reply other threads:[~2023-05-04 20:34 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-17 12:23 [RFC PATCH v4 0/4] fpga: add initial KUnit tests for the subsystem Marco Pagani
2023-04-17 12:23 ` [RFC PATCH v4 1/4] fpga: add fake FPGA manager Marco Pagani
2023-04-20 18:31 ` Xu Yilun
2023-04-26 15:44 ` Marco Pagani
2023-05-03 16:53 ` Marco Pagani
2023-05-04 12:25 ` Xu Yilun
2023-05-04 20:05 ` Marco Pagani [this message]
2023-04-17 12:23 ` [RFC PATCH v4 2/4] fpga: add fake FPGA bridge Marco Pagani
2023-04-17 12:23 ` [RFC PATCH v4 3/4] fpga: add fake FPGA region Marco Pagani
2023-04-20 18:38 ` Xu Yilun
2023-04-26 16:21 ` Marco Pagani
2023-04-17 12:23 ` [RFC PATCH v4 4/4] fpga: add initial KUnit test suites Marco Pagani
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=6f3ba18c-8724-f4ae-8267-719cc6f45c69@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).