Received-SPF: pass (google.com: domain of linux-kernel+bounces-130112-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 3 Apr 2024 16:41:45 +0100
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Ira Weiny <ira.weiny@intel.com>
CC: Dave Jiang <dave.jiang@intel.com>, Davidlohr Bueso <dave@stgolabs.net>,
	Alison Schofield <alison.schofield@intel.com>, Vishal Verma
	<vishal.l.verma@intel.com>, Dan Williams <dan.j.williams@intel.com>,
	<linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>, Kwangjin Ko
	<kwangjin.ko@sk.com>
Subject: Re: [PATCH] cxl/test: Enhance event testing
Message-ID: <20240403164145.00007bcd@Huawei.com>
In-Reply-To: <20240401-enhance-event-test-v1-1-6669a524ed38@intel.com>
References: <20240401-enhance-event-test-v1-1-6669a524ed38@intel.com>
Organization: Huawei Technologies Research and Development (UK) Ltd.
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit

On Mon, 01 Apr 2024 22:31:58 -0700
Ira Weiny <ira.weiny@intel.com> wrote:

> An issue was found in the processing of event logs when the output
> buffer length was not reset.[1]
> 
> This bug was not caught with cxl-test for 2 reasons.  First, the test
> harness mbox_send command [mock_get_event()] does not set the output
> size based on the amount of data returned like the hardware command
> does.  Second, the simplistic event log testing always returned the same
> number of elements per-get command.
> 
> Enhance the simulation of the event log mailbox to better match the bug
> found with real hardware to cover potential regressions.
> 
> NOTE: These changes will cause cxl-events.sh in ndctl to fail without
> the fix from Kwangjin.  However, no changes to the user space test was
> required.  Therefore ndctl itself will be compatible with old or new
> kernels once both patches land in the new kernel.

Good to state how many events are read out (22 I think) as that allows
reader of this patch to see that it will cycle all the way around, so we
will get the 4 to 1 reduction between two reads ensuring things work
correctly when smaller number of events are returned.

1, 2, 3, 4, 1, 2, 3, 4, 1, 1 
I think.
> 
> [1] Link: https://lore.kernel.org/all/20240401091057.1044-1-kwangjin.ko@sk.com/
> 
> Cc: Kwangjin Ko <kwangjin.ko@sk.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


> ---
>  tools/testing/cxl/test/mem.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index 35ee41e435ab..6584443144de 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -127,7 +127,7 @@ static struct {
>  #define CXL_TEST_EVENT_CNT_MAX 15
>  
>  /* Set a number of events to return at a time for simulation.  */
> -#define CXL_TEST_EVENT_CNT 3
> +#define CXL_TEST_EVENT_RET_MAX 4
>  
>  struct mock_event_log {
>  	u16 clear_idx;
> @@ -222,6 +222,12 @@ static void mes_add_event(struct mock_event_store *mes,
>  	log->nr_events++;
>  }
>  
> +/*
> + * Vary the number of events returned to simulate events occuring while the
> + * logs are being read.
> + */
> +static int ret_limit = 0;
> +
>  static int mock_get_event(struct device *dev, struct cxl_mbox_cmd *cmd)
>  {
>  	struct cxl_get_event_payload *pl;
> @@ -233,14 +239,18 @@ static int mock_get_event(struct device *dev, struct cxl_mbox_cmd *cmd)
>  	if (cmd->size_in != sizeof(log_type))
>  		return -EINVAL;
>  
> -	if (cmd->size_out < struct_size(pl, records, CXL_TEST_EVENT_CNT))
> +	ret_limit = (ret_limit + 1) % CXL_TEST_EVENT_RET_MAX;
> +	if (!ret_limit)
> +		ret_limit = 1;
> +
> +	if (cmd->size_out < struct_size(pl, records, ret_limit))
>  		return -EINVAL;
>  
>  	log_type = *((u8 *)cmd->payload_in);
>  	if (log_type >= CXL_EVENT_TYPE_MAX)
>  		return -EINVAL;
>  
> -	memset(cmd->payload_out, 0, cmd->size_out);
> +	memset(cmd->payload_out, 0, struct_size(pl, records, 0));
>  
>  	log = event_find_log(dev, log_type);
>  	if (!log || event_log_empty(log))
> @@ -248,7 +258,7 @@ static int mock_get_event(struct device *dev, struct cxl_mbox_cmd *cmd)
>  
>  	pl = cmd->payload_out;
>  
> -	for (i = 0; i < CXL_TEST_EVENT_CNT && !event_log_empty(log); i++) {
> +	for (i = 0; i < ret_limit && !event_log_empty(log); i++) {
>  		memcpy(&pl->records[i], event_get_current(log),
>  		       sizeof(pl->records[i]));
>  		pl->records[i].event.generic.hdr.handle =
> @@ -256,6 +266,7 @@ static int mock_get_event(struct device *dev, struct cxl_mbox_cmd *cmd)
>  		log->cur_idx++;
>  	}
>  
> +	cmd->size_out = struct_size(pl, records, i);
>  	pl->record_count = cpu_to_le16(i);
>  	if (!event_log_empty(log))
>  		pl->flags |= CXL_GET_EVENT_FLAG_MORE_RECORDS;
> 
> ---
> base-commit: 8d025e2092e29bfd13e56c78e22af25fac83c8ec
> change-id: 20240401-enhance-event-test-cebeff5189cd
> 
> Best regards,