[NDCTL PATCH v4 0/3] ndctl: Add support and test for CXL Features support

[NDCTL PATCH v4 0/3] ndctl: Add support and test for CXL Features support

[Dave Jiang]

v3:
- Update test to use opcode instead of command id.

v2:
- Drop features device enumeration
- Add discovery of char device under memdev

The series provides support of libcxl enumerating FWCTL character device
under the cxl_memdev device. It discovers the char device major
and minor numbers for the CXL features device in order to allow issuing
of ioctls to the device. 

A unit test is added to locate cxl_memdev exported by the cxl_test
kernel module and issue all the supported ioctls to the associated
FWCTL char device to verify that all the ioctl paths are working as expected.

Kernel series: https://lore.kernel.org/linux-cxl/20250218225721.2682235-1-dave.jiang@intel.com/T/#t

Dave Jiang (3):
      cxl: Add cxl_bus_get_by_provider()
      cxl: Enumerate major/minor of FWCTL char device
      cxl/test: Add test for cxl features device

 cxl/lib/libcxl.c     |  85 ++++++++++++++++
 cxl/lib/libcxl.sym   |   7 ++
 cxl/lib/private.h    |   1 +
 cxl/libcxl.h         |   4 +
 test/cxl-features.sh |  31 ++++++
 test/fwctl.c         | 383 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 test/meson.build     |  45 +++++++++
 7 files changed, 556 insertions(+)

[NDCTL PATCH v4 1/3] cxl: Add cxl_bus_get_by_provider()

[Dave Jiang]

Add helper function cxl_bus_get_by_provider() in order to support unit
test that will utilize the API call.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 cxl/lib/libcxl.c   | 11 +++++++++++
 cxl/lib/libcxl.sym |  5 +++++
 cxl/libcxl.h       |  2 ++
 3 files changed, 18 insertions(+)

diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index 63aa4ef3acdc..bab7343e8a4a 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -3358,6 +3358,17 @@ CXL_EXPORT struct cxl_ctx *cxl_bus_get_ctx(struct cxl_bus *bus)
 	return cxl_port_get_ctx(&bus->port);
 }
 
+CXL_EXPORT struct cxl_bus *cxl_bus_get_by_provider(struct cxl_ctx *ctx,
+						   const char *provider)
+{
+	struct cxl_bus *bus;
+
+	cxl_bus_foreach(ctx, bus)
+		if (strcmp(provider, cxl_bus_get_provider(bus)) == 0)
+			return bus;
+	return NULL;
+}
+
 CXL_EXPORT void cxl_cmd_unref(struct cxl_cmd *cmd)
 {
 	if (!cmd)
diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
index 0c155a40ad47..1fc33cc6e1a4 100644
--- a/cxl/lib/libcxl.sym
+++ b/cxl/lib/libcxl.sym
@@ -288,3 +288,8 @@ global:
 	cxl_memdev_trigger_poison_list;
 	cxl_region_trigger_poison_list;
 } LIBCXL_7;
+
+LIBCXL_9 {
+global:
+	cxl_bus_get_by_provider;
+} LIBECXL_8;
diff --git a/cxl/libcxl.h b/cxl/libcxl.h
index 0a5fd0e13cc2..3b309968a808 100644
--- a/cxl/libcxl.h
+++ b/cxl/libcxl.h
@@ -122,6 +122,8 @@ int cxl_bus_get_id(struct cxl_bus *bus);
 struct cxl_port *cxl_bus_get_port(struct cxl_bus *bus);
 struct cxl_ctx *cxl_bus_get_ctx(struct cxl_bus *bus);
 int cxl_bus_disable_invalidate(struct cxl_bus *bus);
+struct cxl_bus *cxl_bus_get_by_provider(struct cxl_ctx *ctx,
+					const char *provider);
 
 #define cxl_bus_foreach(ctx, bus)                                              \
 	for (bus = cxl_bus_get_first(ctx); bus != NULL;                        \
-- 
2.48.1

Re: [NDCTL PATCH v4 1/3] cxl: Add cxl_bus_get_by_provider()

[Alison Schofield]

On Tue, Feb 18, 2025 at 03:59:54PM -0700, Dave Jiang wrote:
> Add helper function cxl_bus_get_by_provider() in order to support unit
> test that will utilize the API call.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  cxl/lib/libcxl.c   | 11 +++++++++++
>  cxl/lib/libcxl.sym |  5 +++++
>  cxl/libcxl.h       |  2 ++
>  3 files changed, 18 insertions(+)

Please describe the new library interface in 
"Documentation/cxl/lib/libcxl.txt"

Add To: nvdimm@lists.linux.dev

snip

[NDCTL PATCH v4 2/3] cxl: Enumerate major/minor of FWCTL char device

[Dave Jiang]

Add major/minor discovery for the FWCTL character device that is associated
with supprting CXL Features under the cxl_memdev. Add libcxl API functions
to retrieve the major and minor of the FWCTL character device.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 cxl/lib/libcxl.c   | 74 ++++++++++++++++++++++++++++++++++++++++++++++
 cxl/lib/libcxl.sym |  2 ++
 cxl/lib/private.h  |  1 +
 cxl/libcxl.h       |  2 ++
 4 files changed, 79 insertions(+)

diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index bab7343e8a4a..566870acb30a 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -1253,6 +1253,66 @@ static int add_cxl_memdev_fwl(struct cxl_memdev *memdev,
 	return -ENOMEM;
 }
 
+static const char fwctl_prefix[] = "fwctl";
+static int get_feature_chardev(const char *base, char *chardev_path)
+{
+	char *path = calloc(1, strlen(base) + 100);
+	struct dirent *entry;
+	int rc = 0;
+	DIR *dir;
+
+	if (!path)
+		return -ENOMEM;
+
+	sprintf(path, "%s/fwctl/", base);
+	dir = opendir(path);
+	if (!dir) {
+		rc = -errno;
+		goto err;
+	}
+
+	while ((entry = readdir(dir)) != NULL)
+		if (strncmp(entry->d_name, fwctl_prefix, strlen(fwctl_prefix)) == 0)
+			break;
+
+	if (!entry) {
+		rc = -ENOENT;
+		goto read_err;
+	}
+
+	sprintf(chardev_path, "/dev/fwctl/%s", entry->d_name);
+
+read_err:
+	closedir(dir);
+err:
+	free(path);
+	return rc;
+}
+
+static int memdev_get_fwctl_chardev(struct cxl_memdev *memdev,
+				    const char *cxlmem_base)
+{
+	char *path = calloc(1, strlen(cxlmem_base) + 100);
+	struct stat st;
+	int rc;
+
+	rc = get_feature_chardev(cxlmem_base, path);
+	if (rc)
+		goto out;
+
+	if (stat(path, &st) < 0) {
+		rc = -errno;
+		goto out;
+	}
+
+	memdev->fwctl_major = major(st.st_rdev);
+	memdev->fwctl_minor = minor(st.st_rdev);
+
+out:
+	free(path);
+	return rc;
+}
+
 static void *add_cxl_memdev(void *parent, int id, const char *cxlmem_base)
 {
 	const char *devname = devpath_to_devname(cxlmem_base);
@@ -1279,6 +1339,10 @@ static void *add_cxl_memdev(void *parent, int id, const char *cxlmem_base)
 	memdev->major = major(st.st_rdev);
 	memdev->minor = minor(st.st_rdev);
 
+	/* If this fails, no Features support. */
+	if (memdev_get_fwctl_chardev(memdev, cxlmem_base))
+		dbg(ctx, "%s: no Features support.\n", devname);
+
 	sprintf(path, "%s/pmem/size", cxlmem_base);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		memdev->pmem_size = strtoull(buf, NULL, 0);
@@ -1515,6 +1579,16 @@ CXL_EXPORT int cxl_memdev_get_minor(struct cxl_memdev *memdev)
 	return memdev->minor;
 }
 
+CXL_EXPORT int cxl_memdev_get_fwctl_major(struct cxl_memdev *memdev)
+{
+	return memdev->fwctl_major;
+}
+
+CXL_EXPORT int cxl_memdev_get_fwctl_minor(struct cxl_memdev *memdev)
+{
+	return memdev->fwctl_minor;
+}
+
 CXL_EXPORT unsigned long long cxl_memdev_get_pmem_size(struct cxl_memdev *memdev)
 {
 	return memdev->pmem_size;
diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
index 1fc33cc6e1a4..b2b51a72673c 100644
--- a/cxl/lib/libcxl.sym
+++ b/cxl/lib/libcxl.sym
@@ -292,4 +292,6 @@ global:
 LIBCXL_9 {
 global:
 	cxl_bus_get_by_provider;
+	cxl_memdev_get_fwctl_major;
+	cxl_memdev_get_fwctl_minor;
 } LIBECXL_8;
diff --git a/cxl/lib/private.h b/cxl/lib/private.h
index b6cd910e9335..676bf1573487 100644
--- a/cxl/lib/private.h
+++ b/cxl/lib/private.h
@@ -37,6 +37,7 @@ enum cxl_fwl_loading {
 struct cxl_endpoint;
 struct cxl_memdev {
 	int id, major, minor;
+	int fwctl_major, fwctl_minor;
 	int numa_node;
 	void *dev_buf;
 	size_t buf_len;
diff --git a/cxl/libcxl.h b/cxl/libcxl.h
index 3b309968a808..26aa906740af 100644
--- a/cxl/libcxl.h
+++ b/cxl/libcxl.h
@@ -69,6 +69,8 @@ const char *cxl_memdev_get_host(struct cxl_memdev *memdev);
 struct cxl_bus *cxl_memdev_get_bus(struct cxl_memdev *memdev);
 int cxl_memdev_get_major(struct cxl_memdev *memdev);
 int cxl_memdev_get_minor(struct cxl_memdev *memdev);
+int cxl_memdev_get_fwctl_major(struct cxl_memdev *memdev);
+int cxl_memdev_get_fwctl_minor(struct cxl_memdev *memdev);
 struct cxl_ctx *cxl_memdev_get_ctx(struct cxl_memdev *memdev);
 unsigned long long cxl_memdev_get_pmem_size(struct cxl_memdev *memdev);
 unsigned long long cxl_memdev_get_ram_size(struct cxl_memdev *memdev);
-- 
2.48.1

Re: [NDCTL PATCH v4 2/3] cxl: Enumerate major/minor of FWCTL char device

[Alison Schofield]

On Tue, Feb 18, 2025 at 03:59:55PM -0700, Dave Jiang wrote:
> Add major/minor discovery for the FWCTL character device that is associated
> with supprting CXL Features under the cxl_memdev. Add libcxl API functions
> to retrieve the major and minor of the FWCTL character device.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  cxl/lib/libcxl.c   | 74 ++++++++++++++++++++++++++++++++++++++++++++++
>  cxl/lib/libcxl.sym |  2 ++
>  cxl/lib/private.h  |  1 +
>  cxl/libcxl.h       |  2 ++
>  4 files changed, 79 insertions(+)

Please describe the new library interface in 
"Documentation/cxl/lib/libcxl.txt"

Add To: nvdimm@lists.linux.dev

> 
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index bab7343e8a4a..566870acb30a 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -1253,6 +1253,66 @@ static int add_cxl_memdev_fwl(struct cxl_memdev *memdev,
>  	return -ENOMEM;
>  }
>  


Can we define like this:
#define CXL_PATH_MAX 512

and do this:
char path[CXL_PATH_MAX];

in next 2 funcs?

> +static const char fwctl_prefix[] = "fwctl";
> +static int get_feature_chardev(const char *base, char *chardev_path)
> +{
> +	char *path = calloc(1, strlen(base) + 100);
> +	struct dirent *entry;
> +	int rc = 0;
> +	DIR *dir;
> +
> +	if (!path)
> +		return -ENOMEM;
> +
> +	sprintf(path, "%s/fwctl/", base);
> +	dir = opendir(path);
> +	if (!dir) {
> +		rc = -errno;
> +		goto err;
> +	}
> +
> +	while ((entry = readdir(dir)) != NULL)
> +		if (strncmp(entry->d_name, fwctl_prefix, strlen(fwctl_prefix)) == 0)
> +			break;

Can we exit this while loop w entry not NULL, yet not a match?
Maybe a found flag or store a match separately.

> +
> +	if (!entry) {
> +		rc = -ENOENT;
> +		goto read_err;
> +	}
> +
> +	sprintf(chardev_path, "/dev/fwctl/%s", entry->d_name);
> +
> +read_err:
> +	closedir(dir);
> +err:
> +	free(path);
> +	return rc;
> +}
> +
> +static int memdev_get_fwctl_chardev(struct cxl_memdev *memdev,
> +				    const char *cxlmem_base)
> +{
> +	char *path = calloc(1, strlen(cxlmem_base) + 100);
> +	struct stat st;
> +	int rc;
> +

Do we need to init memdev->fwctl... to something to avoid garbage?


> +	rc = get_feature_chardev(cxlmem_base, path);
> +	if (rc)
> +		goto out;
> +
> +	if (stat(path, &st) < 0) {
> +		rc = -errno;
> +		goto out;
> +	}
> +
> +	memdev->fwctl_major = major(st.st_rdev);
> +	memdev->fwctl_minor = minor(st.st_rdev);
> +
> +out:
> +	free(path);
> +	return rc;
> +}
> +
>  static void *add_cxl_memdev(void *parent, int id, const char *cxlmem_base)
>  {
>  	const char *devname = devpath_to_devname(cxlmem_base);
> @@ -1279,6 +1339,10 @@ static void *add_cxl_memdev(void *parent, int id, const char *cxlmem_base)
>  	memdev->major = major(st.st_rdev);
>  	memdev->minor = minor(st.st_rdev);
>  
> +	/* If this fails, no Features support. */
> +	if (memdev_get_fwctl_chardev(memdev, cxlmem_base))
> +		dbg(ctx, "%s: no Features support.\n", devname);
> +
>  	sprintf(path, "%s/pmem/size", cxlmem_base);
>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>  		memdev->pmem_size = strtoull(buf, NULL, 0);
> @@ -1515,6 +1579,16 @@ CXL_EXPORT int cxl_memdev_get_minor(struct cxl_memdev *memdev)
>  	return memdev->minor;
>  }
>  
> +CXL_EXPORT int cxl_memdev_get_fwctl_major(struct cxl_memdev *memdev)
> +{
> +	return memdev->fwctl_major;
> +}
> +
> +CXL_EXPORT int cxl_memdev_get_fwctl_minor(struct cxl_memdev *memdev)
> +{
> +	return memdev->fwctl_minor;
> +}
> +
>  CXL_EXPORT unsigned long long cxl_memdev_get_pmem_size(struct cxl_memdev *memdev)
>  {
>  	return memdev->pmem_size;
> diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
> index 1fc33cc6e1a4..b2b51a72673c 100644
> --- a/cxl/lib/libcxl.sym
> +++ b/cxl/lib/libcxl.sym
> @@ -292,4 +292,6 @@ global:
>  LIBCXL_9 {
>  global:
>  	cxl_bus_get_by_provider;
> +	cxl_memdev_get_fwctl_major;
> +	cxl_memdev_get_fwctl_minor;
>  } LIBECXL_8;
> diff --git a/cxl/lib/private.h b/cxl/lib/private.h
> index b6cd910e9335..676bf1573487 100644
> --- a/cxl/lib/private.h
> +++ b/cxl/lib/private.h
> @@ -37,6 +37,7 @@ enum cxl_fwl_loading {
>  struct cxl_endpoint;
>  struct cxl_memdev {
>  	int id, major, minor;
> +	int fwctl_major, fwctl_minor;
>  	int numa_node;
>  	void *dev_buf;
>  	size_t buf_len;
> diff --git a/cxl/libcxl.h b/cxl/libcxl.h
> index 3b309968a808..26aa906740af 100644
> --- a/cxl/libcxl.h
> +++ b/cxl/libcxl.h
> @@ -69,6 +69,8 @@ const char *cxl_memdev_get_host(struct cxl_memdev *memdev);
>  struct cxl_bus *cxl_memdev_get_bus(struct cxl_memdev *memdev);
>  int cxl_memdev_get_major(struct cxl_memdev *memdev);
>  int cxl_memdev_get_minor(struct cxl_memdev *memdev);
> +int cxl_memdev_get_fwctl_major(struct cxl_memdev *memdev);
> +int cxl_memdev_get_fwctl_minor(struct cxl_memdev *memdev);
>  struct cxl_ctx *cxl_memdev_get_ctx(struct cxl_memdev *memdev);
>  unsigned long long cxl_memdev_get_pmem_size(struct cxl_memdev *memdev);
>  unsigned long long cxl_memdev_get_ram_size(struct cxl_memdev *memdev);
> -- 
> 2.48.1
> 

Re: [NDCTL PATCH v4 2/3] cxl: Enumerate major/minor of FWCTL char device

[Dave Jiang]

On 4/9/25 12:56 PM, Alison Schofield wrote:
> On Tue, Feb 18, 2025 at 03:59:55PM -0700, Dave Jiang wrote:
>> Add major/minor discovery for the FWCTL character device that is associated
>> with supprting CXL Features under the cxl_memdev. Add libcxl API functions
>> to retrieve the major and minor of the FWCTL character device.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>  cxl/lib/libcxl.c   | 74 ++++++++++++++++++++++++++++++++++++++++++++++
>>  cxl/lib/libcxl.sym |  2 ++
>>  cxl/lib/private.h  |  1 +
>>  cxl/libcxl.h       |  2 ++
>>  4 files changed, 79 insertions(+)
> 
> Please describe the new library interface in 
> "Documentation/cxl/lib/libcxl.txt"
> 
> Add To: nvdimm@lists.linux.dev
> 
>>
>> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
>> index bab7343e8a4a..566870acb30a 100644
>> --- a/cxl/lib/libcxl.c
>> +++ b/cxl/lib/libcxl.c
>> @@ -1253,6 +1253,66 @@ static int add_cxl_memdev_fwl(struct cxl_memdev *memdev,
>>  	return -ENOMEM;
>>  }
>>  
> 
> 
> Can we define like this:
> #define CXL_PATH_MAX 512
> 
> and do this:
> char path[CXL_PATH_MAX];
> 
> in next 2 funcs?
> 
>> +static const char fwctl_prefix[] = "fwctl";
>> +static int get_feature_chardev(const char *base, char *chardev_path)
>> +{
>> +	char *path = calloc(1, strlen(base) + 100);
>> +	struct dirent *entry;
>> +	int rc = 0;
>> +	DIR *dir;
>> +
>> +	if (!path)
>> +		return -ENOMEM;
>> +
>> +	sprintf(path, "%s/fwctl/", base);
>> +	dir = opendir(path);
>> +	if (!dir) {
>> +		rc = -errno;
>> +		goto err;
>> +	}
>> +
>> +	while ((entry = readdir(dir)) != NULL)
>> +		if (strncmp(entry->d_name, fwctl_prefix, strlen(fwctl_prefix)) == 0)
>> +			break;
> 
> Can we exit this while loop w entry not NULL, yet not a match?
> Maybe a found flag or store a match separately.
> 
>> +
>> +	if (!entry) {
>> +		rc = -ENOENT;
>> +		goto read_err;
>> +	}
>> +
>> +	sprintf(chardev_path, "/dev/fwctl/%s", entry->d_name);
>> +
>> +read_err:
>> +	closedir(dir);
>> +err:
>> +	free(path);
>> +	return rc;
>> +}
>> +
>> +static int memdev_get_fwctl_chardev(struct cxl_memdev *memdev,
>> +				    const char *cxlmem_base)
>> +{
>> +	char *path = calloc(1, strlen(cxlmem_base) + 100);
>> +	struct stat st;
>> +	int rc;
>> +
> 
> Do we need to init memdev->fwctl... to something to avoid garbage?

I think 0.0 is probably sufficient as incorrect chardev for user. We don't init memdev->major/minor either.

DJ

 
> 
> 
>> +	rc = get_feature_chardev(cxlmem_base, path);
>> +	if (rc)
>> +		goto out;
>> +
>> +	if (stat(path, &st) < 0) {
>> +		rc = -errno;
>> +		goto out;
>> +	}
>> +
>> +	memdev->fwctl_major = major(st.st_rdev);
>> +	memdev->fwctl_minor = minor(st.st_rdev);
>> +
>> +out:
>> +	free(path);
>> +	return rc;
>> +}
>> +
>>  static void *add_cxl_memdev(void *parent, int id, const char *cxlmem_base)
>>  {
>>  	const char *devname = devpath_to_devname(cxlmem_base);
>> @@ -1279,6 +1339,10 @@ static void *add_cxl_memdev(void *parent, int id, const char *cxlmem_base)
>>  	memdev->major = major(st.st_rdev);
>>  	memdev->minor = minor(st.st_rdev);
>>  
>> +	/* If this fails, no Features support. */
>> +	if (memdev_get_fwctl_chardev(memdev, cxlmem_base))
>> +		dbg(ctx, "%s: no Features support.\n", devname);
>> +
>>  	sprintf(path, "%s/pmem/size", cxlmem_base);
>>  	if (sysfs_read_attr(ctx, path, buf) == 0)
>>  		memdev->pmem_size = strtoull(buf, NULL, 0);
>> @@ -1515,6 +1579,16 @@ CXL_EXPORT int cxl_memdev_get_minor(struct cxl_memdev *memdev)
>>  	return memdev->minor;
>>  }
>>  
>> +CXL_EXPORT int cxl_memdev_get_fwctl_major(struct cxl_memdev *memdev)
>> +{
>> +	return memdev->fwctl_major;
>> +}
>> +
>> +CXL_EXPORT int cxl_memdev_get_fwctl_minor(struct cxl_memdev *memdev)
>> +{
>> +	return memdev->fwctl_minor;
>> +}
>> +
>>  CXL_EXPORT unsigned long long cxl_memdev_get_pmem_size(struct cxl_memdev *memdev)
>>  {
>>  	return memdev->pmem_size;
>> diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
>> index 1fc33cc6e1a4..b2b51a72673c 100644
>> --- a/cxl/lib/libcxl.sym
>> +++ b/cxl/lib/libcxl.sym
>> @@ -292,4 +292,6 @@ global:
>>  LIBCXL_9 {
>>  global:
>>  	cxl_bus_get_by_provider;
>> +	cxl_memdev_get_fwctl_major;
>> +	cxl_memdev_get_fwctl_minor;
>>  } LIBECXL_8;
>> diff --git a/cxl/lib/private.h b/cxl/lib/private.h
>> index b6cd910e9335..676bf1573487 100644
>> --- a/cxl/lib/private.h
>> +++ b/cxl/lib/private.h
>> @@ -37,6 +37,7 @@ enum cxl_fwl_loading {
>>  struct cxl_endpoint;
>>  struct cxl_memdev {
>>  	int id, major, minor;
>> +	int fwctl_major, fwctl_minor;
>>  	int numa_node;
>>  	void *dev_buf;
>>  	size_t buf_len;
>> diff --git a/cxl/libcxl.h b/cxl/libcxl.h
>> index 3b309968a808..26aa906740af 100644
>> --- a/cxl/libcxl.h
>> +++ b/cxl/libcxl.h
>> @@ -69,6 +69,8 @@ const char *cxl_memdev_get_host(struct cxl_memdev *memdev);
>>  struct cxl_bus *cxl_memdev_get_bus(struct cxl_memdev *memdev);
>>  int cxl_memdev_get_major(struct cxl_memdev *memdev);
>>  int cxl_memdev_get_minor(struct cxl_memdev *memdev);
>> +int cxl_memdev_get_fwctl_major(struct cxl_memdev *memdev);
>> +int cxl_memdev_get_fwctl_minor(struct cxl_memdev *memdev);
>>  struct cxl_ctx *cxl_memdev_get_ctx(struct cxl_memdev *memdev);
>>  unsigned long long cxl_memdev_get_pmem_size(struct cxl_memdev *memdev);
>>  unsigned long long cxl_memdev_get_ram_size(struct cxl_memdev *memdev);
>> -- 
>> 2.48.1
>>

Re: [NDCTL PATCH v4 2/3] cxl: Enumerate major/minor of FWCTL char device

[Dan Williams]

Dave Jiang wrote:
> Add major/minor discovery for the FWCTL character device that is associated
> with supprting CXL Features under the cxl_memdev. Add libcxl API functions
> to retrieve the major and minor of the FWCTL character device.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
[..]
> diff --git a/cxl/lib/private.h b/cxl/lib/private.h
> index b6cd910e9335..676bf1573487 100644
> --- a/cxl/lib/private.h
> +++ b/cxl/lib/private.h
> @@ -37,6 +37,7 @@ enum cxl_fwl_loading {
>  struct cxl_endpoint;
>  struct cxl_memdev {
>  	int id, major, minor;
> +	int fwctl_major, fwctl_minor;

Unlike daxctl_dev_get_{major,minor}() which need a valid daxctl_dev,
there is no guarantee that a memdev has a fwctl capability. So I would
prefer that the helper functions are:

int cxl_fwctl_get_major(const struct cxl_fwctl *)
int cxl_fwctl_get_minor(const struct cxl_fwctl *)
struct cxl_fwctl *cxl_memdev_get_fwctl(struct cxl_memdev *)

...where obtaining that 'struct cxl_fwctl' gives you a chance to return
NULL and prevent non-sensical cxl_memdev_get_fwctl_major() against a
memdev that does not have an active fwctl portal.

It also lets you build other cxl_fwctl helper functionality around that
object.

[NDCTL PATCH v4 3/3] cxl/test: Add test for cxl features device

[Dave Jiang]

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.

Signed-off-by: Dave Jiang <dave.jiang@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
new file mode 100755
index 000000000000..8e44a5bcc3e5
--- /dev/null
+++ b/test/cxl-features.sh
@@ -0,0 +1,31 @@
+#!/bin/bash -Ex
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2025 Intel Corporation. All rights reserved.
+
+rc=77
+# 237 is -ENODEV
+ERR_NODEV=237
+
+. $(dirname $0)/common
+FWCTL="$TEST_PATH"/fwctl
+
+trap 'err $LINENO' ERR
+
+modprobe cxl_test
+
+test -x "$FWCTL" || do_skip "no fwctl"
+# disable trap
+trap - $(compgen -A signal)
+"$FWCTL"
+rc=$?
+
+echo "error: $rc"
+if [ "$rc" -eq "$ERR_NODEV" ]; then
+	do_skip "no fwctl char dev"
+elif [ "$rc" -ne 0 ]; then
+	echo "fail: $LINENO" && exit 1
+fi
+
+trap 'err $LINENO' ERR
+
+_cxl_cleanup
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>
+
+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;
+}
+
+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);
+	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;
+}
+
+static int cxl_fwctl_rpc_set_test_feature(int fd, struct test_feature *feat_ctx)
+{
+	struct cxl_mbox_set_feat_in *feat_in;
+	struct fwctl_rpc_cxl_out *out;
+	struct fwctl_rpc_cxl *in;
+	struct fwctl_rpc rpc = {0};
+	size_t in_size, out_size;
+	uint32_t val;
+	void *data;
+	int rc;
+
+	in_size = sizeof(*in) + sizeof(*feat_in) + sizeof(val);
+	rc = posix_memalign((void **)&in, 16, in_size);
+	if (rc)
+		return -ENOMEM;
+	memset(in, 0, in_size);
+	feat_in = &in->set_feat_in;
+
+	out_size = sizeof(*out) + sizeof(val);
+	rc = posix_memalign((void **)&out, 16, out_size);
+	if (rc)
+		goto free_in;
+
+	uuid_copy(feat_in->uuid, feat_ctx->uuid);
+	data = feat_in->feat_data;
+	val = DEFAULT_TEST_DATA2;
+	*(uint32_t *)data = htole32(val);
+	feat_in->flags = CXL_SET_FEAT_FLAG_FULL_DATA_TRANSFER;
+
+	in->opcode = CXL_MBOX_OPCODE_SET_FEATURE;
+	in->op_size = sizeof(*feat_in) + sizeof(val);
+
+	rpc.size = sizeof(rpc);
+	rpc.scope = FWCTL_RPC_DEBUG_WRITE_FULL;
+	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;
+
+	rc = cxl_fwctl_rpc_get_test_feature(fd, feat_ctx, DEFAULT_TEST_DATA2);
+	if (rc) {
+		fprintf(stderr, "Failed ioctl to get feature verify: %d\n", rc);
+		goto free_all;
+	}
+
+free_all:
+	free(out);
+free_in:
+	free(in);
+	return rc;
+}
+
+static int cxl_fwctl_rpc_get_supported_features(int fd, struct test_feature *feat_ctx)
+{
+	struct cxl_mbox_get_sup_feats_out *feat_out;
+	struct cxl_mbox_get_sup_feats_in *feat_in;
+	struct fwctl_rpc_cxl_out *out;
+	struct fwctl_rpc_cxl *in;
+	struct cxl_feat_entry *entry;
+	struct fwctl_rpc rpc = {0};
+	size_t out_size, in_size;
+	int feats, rc;
+
+	in_size = sizeof(*in) + sizeof(*feat_in);
+	rc = posix_memalign((void **)&in, 16, in_size);
+	if (rc)
+		return rc;
+	memset(in, 0, in_size);
+	/* First query, to get number of features w/o per feature data */
+	in->opcode = CXL_MBOX_OPCODE_GET_SUPPORTED_FEATURES;
+	in->op_size = sizeof(*feat_in);
+
+	out_size = sizeof(*out) + sizeof(*feat_out);
+	rc = posix_memalign((void **)&out, 16, out_size);
+	if (rc)
+		goto free_in;
+
+	/* No need to fill in feat_in first go as we are passing in all 0's */
+
+	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;
+
+	feat_out = &out->get_sup_feats_out;
+	feats = le16toh(feat_out->supported_feats);
+	if (feats != MAX_TEST_FEATURES) {
+		fprintf(stderr, "Test device has greater than %d test features.\n",
+			MAX_TEST_FEATURES);
+		rc = -ENXIO;
+		goto free_all;
+	}
+
+	free(in);
+	free(out);
+
+	/* Going second round to retrieve each feature details */
+	in_size += feats * sizeof(*entry);
+	rc = posix_memalign((void **)&in, 16, in_size);
+	if (rc)
+		return rc;
+	memset(in, 0, in_size);
+	in->opcode = CXL_MBOX_OPCODE_GET_SUPPORTED_FEATURES;
+	in->op_size = sizeof(*feat_in);
+
+	out_size += feats * sizeof(*entry);
+	rpc.out_len = out_size;
+	rc = posix_memalign((void **)&out, 16, out_size);
+	if (rc)
+		goto free_in;
+	memset(out, 0, out_size);
+
+	feat_in = &in->get_sup_feats_in;
+
+	rpc.out = (uint64_t)(uint64_t *)out;
+	rpc.out_len = out_size;
+	rpc.in = (uint64_t)(uint64_t *)in;
+	rpc.in_len = in_size;
+	feat_in->count = htole32(feats * sizeof(*entry));
+
+	rc = send_command(fd, &rpc, out);
+	if (rc)
+		goto free_all;
+
+	feat_out = &out->get_sup_feats_out;
+	feats = le16toh(feat_out->supported_feats);
+	if (feats != MAX_TEST_FEATURES) {
+		fprintf(stderr, "Test device has greater than %u test features.\n",
+			MAX_TEST_FEATURES);
+		rc = -ENXIO;
+		goto free_all;
+	}
+
+	if (le16toh(feat_out->num_entries) != MAX_TEST_FEATURES) {
+		fprintf(stderr, "Test device did not return expected entries. %u\n",
+			le16toh(feat_out->num_entries));
+		rc = -ENXIO;
+		goto free_all;
+	}
+
+	entry = &feat_out->ents[0];
+	if (uuid_compare(test_uuid, entry->uuid) != 0) {
+		fprintf(stderr, "Test device did not export expected test feature.\n");
+		rc = -ENXIO;
+		goto free_all;
+	}
+
+	if (le16toh(entry->get_feat_size) != GET_FEAT_SIZE ||
+	    le16toh(entry->set_feat_size) != SET_FEAT_SIZE) {
+		fprintf(stderr, "Test device feature in/out size incorrect.\n");
+		rc = -ENXIO;
+		goto free_all;
+	}
+
+	if (le16toh(entry->effects) != EFFECTS_MASK) {
+		fprintf(stderr, "Test device set effects incorrect\n");
+		rc = -ENXIO;
+		goto free_all;
+	}
+
+	uuid_copy(feat_ctx->uuid, entry->uuid);
+	feat_ctx->get_size = le16toh(entry->get_feat_size);
+	feat_ctx->set_size = le16toh(entry->set_feat_size);
+
+free_all:
+	free(out);
+free_in:
+	free(in);
+	return rc;
+}
+
+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;
+	}
+
+	rc = cxl_fwctl_rpc_get_supported_features(fd, &feat_ctx);
+	if (rc) {
+		fprintf(stderr, "Failed ioctl to get supported features: %d\n", rc);
+		goto out;
+	}
+
+	rc = cxl_fwctl_rpc_get_test_feature(fd, &feat_ctx, DEFAULT_TEST_DATA);
+	if (rc) {
+		fprintf(stderr, "Failed ioctl to get feature: %d\n", rc);
+		goto out;
+	}
+
+	rc = cxl_fwctl_rpc_set_test_feature(fd, &feat_ctx);
+	if (rc) {
+		fprintf(stderr, "Failed ioctl to set feature: %d\n", rc);
+		goto out;
+	}
+
+out:
+	close(fd);
+	return rc;
+}
+
+static int test_fwctl(struct cxl_ctx *ctx, struct cxl_bus *bus)
+{
+	struct cxl_memdev *memdev;
+
+	cxl_memdev_foreach(ctx, memdev) {
+		if (cxl_memdev_get_bus(memdev) != bus)
+			continue;
+		return test_fwctl_features(memdev);
+	}
+
+	return 0;
+}
+
+int main(int argc, char *argv[])
+{
+	struct cxl_ctx *ctx;
+	struct cxl_bus *bus;
+	int rc;
+
+	rc = cxl_new(&ctx);
+	if (rc < 0)
+		return rc;
+
+	cxl_set_log_priority(ctx, LOG_DEBUG);
+
+	bus = cxl_bus_get_by_provider(ctx, provider);
+	if (!bus) {
+		fprintf(stderr, "%s: unable to find bus (%s)\n",
+			argv[0], provider);
+		rc = -EINVAL;
+		goto out;
+	}
+
+	rc = test_fwctl(ctx, bus);
+
+out:
+	cxl_unref(ctx);
+	return rc;
+}
diff --git a/test/meson.build b/test/meson.build
index d871e28e17ce..476f4ba6c97c 100644
--- a/test/meson.build
+++ b/test/meson.build
@@ -17,6 +17,13 @@ ndctl_deps = libndctl_deps + [
   versiondep,
 ]
 
+libcxl_deps = [
+  cxl_dep,
+  ndctl_dep,
+  uuid,
+  kmod,
+]
+
 libndctl = executable('libndctl', testcore + [ 'libndctl.c'],
   dependencies : libndctl_deps,
   include_directories : root_inc,
@@ -130,6 +137,33 @@ revoke_devmem = executable('revoke_devmem', testcore + [
   include_directories : root_inc,
 )
 
+fs = import('fs')
+
+feature_hdrs = [
+  '/usr/include/cxl/features.h',
+  '/usr/include/fwctl/cxl.h',
+  '/usr/include/fwctl/fwctl.h',
+]
+
+feat_hdrs_exist = true
+foreach file : feature_hdrs
+  if not fs.exists(file)
+    feat_hdrs_exist = false
+    break
+  endif
+endforeach
+
+if feat_hdrs_exist
+    fwctl = executable('fwctl', [
+        'fwctl.c',
+    ],
+    include_directories : root_inc,
+    dependencies : libcxl_deps,
+    )
+else
+    fwctl = []
+endif
+
 mmap = executable('mmap', 'mmap.c',)
 
 create = find_program('create.sh')
@@ -162,6 +196,10 @@ cxl_destroy_region = find_program('cxl-destroy-region.sh')
 cxl_qos_class = find_program('cxl-qos-class.sh')
 cxl_poison = find_program('cxl-poison.sh')
 
+if feat_hdrs_exist
+  cxl_features = find_program('cxl-features.sh')
+endif
+
 tests = [
   [ 'libndctl',               libndctl,		  'ndctl' ],
   [ 'dsm-fail',               dsm_fail,	      	  'ndctl' ],
@@ -196,6 +234,12 @@ tests = [
   [ 'cxl-poison.sh',          cxl_poison,         'cxl'   ],
 ]
 
+if feat_hdrs_exist
+  tests += [
+    [ 'cxl-features.sh',        cxl_features,       'cxl'   ],
+  ]
+endif
+
 if get_option('destructive').enabled()
   sub_section = find_program('sub-section.sh')
   dax_ext4 = find_program('dax-ext4.sh')
@@ -249,6 +293,7 @@ foreach t : tests
       daxdev_errors,
       dax_dev,
       mmap,
+      fwctl,
     ],
     suite: t[2],
     timeout : 600,
-- 
2.48.1

Re: [NDCTL PATCH v4 3/3] cxl/test: Add test for cxl features device

[Alison Schofield]

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.jiang@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 

Re: [NDCTL PATCH v4 3/3] cxl/test: Add test for cxl features device

[Dave Jiang]

On 4/9/25 1:40 PM, Alison Schofield wrote:
> 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 ???

I don't have strong opinions on this. I can change it to feature-control.c. 

DJ

> 
>> Signed-off-by: Dave Jiang <dave.jiang@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 

Re: [NDCTL PATCH v4 3/3] cxl/test: Add test for cxl features device

[Dave Jiang]

On 4/9/25 1:40 PM, Alison Schofield wrote:
> 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.jiang@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?

Yes. Needed for using BIT() for EFFECTS_MASK. 

DJ
> 
>> +
>> +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