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

[Dave Jiang <dave.jiang@intel.com>]

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