On Tue, Feb 18, 2025 at 03:59:56PM -0700, Dave Jiang wrote:
> Add a unit test to verify the features ioctl commands. Test support added
> for locating a features device, retrieve and verify the supported features
> commands, retrieve specific feature command data, retrieve test feature
> data, and write and verify test feature data.
>
Let's revisit the naming -
If the script is cxl-feature.sh then would the C program make sense as
feature-control.c or ???
> Signed-off-by: Dave Jiang <dave.ji...@intel.com>
> ---
> v4:
> - Adjust for kernel changes of input/out data structures
> - Setup test script to error out if not -ENODEV
> - Remove kernel 6.15 check
> ---
> test/cxl-features.sh | 31 ++++
> test/fwctl.c | 383 +++++++++++++++++++++++++++++++++++++++++++
> test/meson.build | 45 +++++
> 3 files changed, 459 insertions(+)
> create mode 100755 test/cxl-features.sh
> create mode 100644 test/fwctl.c
>
> diff --git a/test/cxl-features.sh b/test/cxl-features.sh
snip
> diff --git a/test/fwctl.c b/test/fwctl.c
> new file mode 100644
> index 000000000000..ca39e30f6dca
> --- /dev/null
> +++ b/test/fwctl.c
> @@ -0,0 +1,383 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2024-2025 Intel Corporation. All rights reserved.
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <endian.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <syslog.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <sys/ioctl.h>
> +#include <cxl/libcxl.h>
> +#include <cxl/features.h>
> +#include <fwctl/fwctl.h>
> +#include <fwctl/cxl.h>
> +#include <linux/uuid.h>
> +#include <uuid/uuid.h>
> +#include <util/bitmap.h>
Not clear bitmap.h is needed?
> +
> +static const char provider[] = "cxl_test";
> +
> +UUID_DEFINE(test_uuid,
> + 0xff, 0xff, 0xff, 0xff,
> + 0xff, 0xff,
> + 0xff, 0xff,
> + 0xff, 0xff,
> + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff
> +);
> +
> +#define CXL_MBOX_OPCODE_GET_SUPPORTED_FEATURES 0x0500
> +#define CXL_MBOX_OPCODE_GET_FEATURE 0x0501
> +#define CXL_MBOX_OPCODE_SET_FEATURE 0x0502
> +
> +#define GET_FEAT_SIZE 4
> +#define SET_FEAT_SIZE 4
> +#define EFFECTS_MASK (BIT(0) | BIT(9))
> +
> +#define MAX_TEST_FEATURES 1
> +#define DEFAULT_TEST_DATA 0xdeadbeef
> +#define DEFAULT_TEST_DATA2 0xabcdabcd
> +
> +struct test_feature {
> + uuid_t uuid;
> + size_t get_size;
> + size_t set_size;
> +};
> +
> +static int send_command(int fd, struct fwctl_rpc *rpc, struct
> fwctl_rpc_cxl_out *out)
> +{
> + if (ioctl(fd, FWCTL_RPC, rpc) == -1) {
> + fprintf(stderr, "RPC ioctl error: %s\n", strerror(errno));
> + return -errno;
> + }
> +
> + if (out->retval) {
> + fprintf(stderr, "operation returned failure: %d\n",
> out->retval);
> + return -ENXIO;
> + }
> +
> + return 0;
> +}
Above the send_command() is factored out and reused. How about doing similar
with
the ioctl setup - ie a setup_and_send_command() that setups up the ioctl and
calls
send_command(). That removes redundancy in *get_feature, *set_feature,
*get_supported.
> +
> +static int cxl_fwctl_rpc_get_test_feature(int fd, struct test_feature
> *feat_ctx,
> + const uint32_t expected_data)
> +{
> + struct cxl_mbox_get_feat_in *feat_in;
> + struct fwctl_rpc_cxl_out *out;
> + struct fwctl_rpc rpc = {0};
> + struct fwctl_rpc_cxl *in;
> + size_t out_size, in_size;
> + uint32_t val;
> + void *data;
> + int rc;
> +
> + in_size = sizeof(*in) + sizeof(*feat_in);
> + rc = posix_memalign((void **)&in, 16, in_size);
> + if (rc)
> + return -ENOMEM;
> + memset(in, 0, in_size);
How about de-duplicating the repeated posix_memalign() + memset() pattern into
one helper func like alloc_aligned_memory() - including the memset on success.
> + feat_in = &in->get_feat_in;
> +
> + uuid_copy(feat_in->uuid, feat_ctx->uuid);
> + feat_in->count = feat_ctx->get_size;
> +
> + out_size = sizeof(*out) + feat_ctx->get_size;
> + rc = posix_memalign((void **)&out, 16, out_size);
> + if (rc)
> + goto free_in;
> + memset(out, 0, out_size);
> +
> + in->opcode = CXL_MBOX_OPCODE_GET_FEATURE;
> + in->op_size = sizeof(*feat_in);
> +
> + rpc.size = sizeof(rpc);
> + rpc.scope = FWCTL_RPC_CONFIGURATION;
> + rpc.in_len = in_size;
> + rpc.out_len = out_size;
> + rpc.in = (uint64_t)(uint64_t *)in;
> + rpc.out = (uint64_t)(uint64_t *)out;
> +
> + rc = send_command(fd, &rpc, out);
> + if (rc)
> + goto free_all;
> +
> + data = out->payload;
> + val = le32toh(*(__le32 *)data);
> + if (memcmp(&val, &expected_data, sizeof(val)) != 0) {
> + rc = -ENXIO;
> + goto free_all;
> + }
> +
> +free_all:
> + free(out);
> +free_in:
> + free(in);
> + return rc;
> +}
> +
snip
> +static int test_fwctl_features(struct cxl_memdev *memdev)
> +{
> + struct test_feature feat_ctx;
> + unsigned int major, minor;
> + int fd, rc;
> + char path[256];
> +
> + major = cxl_memdev_get_fwctl_major(memdev);
> + minor = cxl_memdev_get_fwctl_minor(memdev);
> +
> + if (!major && !minor)
> + return -ENODEV;
> +
> + sprintf(path, "/dev/char/%d:%d", major, minor);
> +
> + fd = open(path, O_RDONLY, 0644);
> + if (!fd) {
> + fprintf(stderr, "Failed to open: %d\n", -errno);
> + return -errno;
> + }
Needs to be "if (fd < 0)" as open() returns -1 on failure.
snip
