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;
Date:   Thu, 4 May 2023 20:25:32 +0800
From:   Xu Yilun <yilun.xu@intel.com>
To:     Marco Pagani <marpagan@redhat.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
Message-ID: <ZFOkPNOmbjsalTmn@yilunxu-OptiPlex-7050>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <594789b2-eb5d-11fc-9c47-310bdb258f7c@redhat.com>
Precedence: bulk

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

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