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:   Wed, 12 Jul 2023 10:00:16 +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: [PATCH v8 1/4] fpga: add an initial KUnit suite for the FPGA
 Manager
Message-ID: <ZK4JMDZfdVyFkN92@yilunxu-OptiPlex-7050>
References: <20230630152507.30219-1-marpagan@redhat.com>
 <20230630152507.30219-2-marpagan@redhat.com>
 <ZKuMosEu242BrY0K@yilunxu-OptiPlex-7050>
 <1d03a695-d603-a4d5-b726-4b0ff6bc33f8@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1d03a695-d603-a4d5-b726-4b0ff6bc33f8@redhat.com>
Precedence: bulk

On 2023-07-11 at 16:02:44 +0200, Marco Pagani wrote:
> 
> 
> On 2023-07-10 06:44, Xu Yilun wrote:
> > On 2023-06-30 at 17:25:04 +0200, Marco Pagani wrote:
> >> The suite tests the basic behaviors of the FPGA Manager including
> >> programming using a single contiguous buffer and a scatter gather table.
> >>
> >> Signed-off-by: Marco Pagani <marpagan@redhat.com>
> >> ---
> >>  drivers/fpga/tests/fpga-mgr-test.c | 311 +++++++++++++++++++++++++++++
> >>  1 file changed, 311 insertions(+)
> >>  create mode 100644 drivers/fpga/tests/fpga-mgr-test.c
> >>
> >> diff --git a/drivers/fpga/tests/fpga-mgr-test.c b/drivers/fpga/tests/fpga-mgr-test.c
> >> new file mode 100644
> >> index 000000000000..6fd2e235f195
> >> --- /dev/null
> >> +++ b/drivers/fpga/tests/fpga-mgr-test.c
> >> @@ -0,0 +1,311 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * KUnit test for the FPGA Manager
> >> + *
> >> + * Copyright (C) 2023 Red Hat, Inc.
> >> + *
> >> + * Author: Marco Pagani <marpagan@redhat.com>
> >> + */
> >> +
> 
> [...]
> 
> >> +static int op_write(struct fpga_manager *mgr, const char *buf, size_t count)
> >> +{
> >> +	struct mgr_stats *stats = mgr->priv;
> >> +	size_t i;
> >> +
> >> +	/* Check the image */
> >> +	stats->image_match = true;
> >> +	for (i = 0; i < count; i++)
> >> +		if (buf[i] != IMAGE_FILL)
> >> +			stats->image_match = false;
> >> +
> >> +	stats->op_write_state = mgr->state;
> >> +	stats->op_write_seq = stats->seq_num++;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int op_write_sg(struct fpga_manager *mgr, struct sg_table *sgt)
> >> +{
> >> +	struct mgr_stats *stats = mgr->priv;
> >> +	struct sg_mapping_iter miter;
> >> +	char *img;
> >> +	size_t i;
> >> +
> >> +	/*
> >> +	 * Check the image, but first skip the header since write_sg will get
> >> +	 * the whole image in sg_table.
> >> +	 */
> >> +	stats->image_match = true;
> >> +	sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG);
> >> +
> >> +	if (!sg_miter_skip(&miter, HEADER_SIZE))
> >> +		stats->image_match = false;
> > 
> > If this fails, should we continue?
> 
> Would it be okay to set the image_match flag to false and then
> return 0 if sg_miter_skip() fails?

That's good to me.

> 
> I think returning an error code to the FPGA manager would not
> be beneficial in this case since if an op fails, it is a failure
> of the FPGA manager itself, not the low-level driver that tests
> the FPGA manager.

So the idea is we never return error code in any fake driver ops?
That's OK. Please add some comments in code for it and the reason.

Thanks,
Yilun

> 
> 
> > 
> >> +
> >> +	while (sg_miter_next(&miter)) {
> >> +		img = miter.addr;
> >> +		for (i = 0; i < miter.length; i++) {
> >> +			if (img[i] != IMAGE_FILL)
> >> +				stats->image_match = false;
> >> +		}
> >> +	}
> >> +
> >> +	sg_miter_stop(&miter);
> >> +
> >> +	stats->op_write_sg_state = mgr->state;
> >> +	stats->op_write_sg_seq = stats->seq_num++;
> >> +
> >> +	return 0;
> >> +}
> > 
>