On Fri, 23 May 2025 09:46:37 -0700
Dave Jiang <dave.ji...@intel.com> wrote:
> Add major/minor discovery for the FWCTL character device that is associated
> with supprting CXL Features under 'struct cxl_fwctl'. A 'struct cxl_fwctl'
> may be installed under cxl_memdev if CXL Features are supported and FWCTL
> is enabled. Add libcxl API functions to retrieve the major and minor of the
> FWCTL character device.
>
> Acked-by: Dan Williams <dan.j.willi...@intel.com>
> Signed-off-by: Dave Jiang <dave.ji...@intel.com>
Hi Dave,
I don't normally look at ndctl stuff (not enough time) but I had a few
mins so trivial comments inline.
Jonathan
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index bab7343e8a4a..af28f976bdc8 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -1253,6 +1254,67 @@ static int add_cxl_memdev_fwl(struct cxl_memdev
> *memdev,
> return -ENOMEM;
> }
>
> +#ifdef ENABLE_FWCTL
> +static const char fwctl_prefix[] = "fwctl";
> +static int get_feature_chardev(struct cxl_memdev *memdev, const char *base,
> + char *chardev_path)
> +{
> + char *path = memdev->dev_buf;
> + struct dirent *entry;
> + bool found = false;
> + int rc = 0;
> + DIR *dir;
> +
> + sprintf(path, "%s/fwctl/", base);
> + dir = opendir(path);
> + if (!dir)
> + return -errno;
> +
> + while ((entry = readdir(dir)) != NULL) {
> + if (strncmp(entry->d_name, fwctl_prefix, strlen(fwctl_prefix))
> == 0) {
> + found = true;
> + break;
> + }
> + }
> +
> + if (!entry || !found) {
If entry != NULL then found is true as no other way to get out
of the loop above. Which raises obvious question of why we need found?
> + rc = -ENOENT;
> + goto read_err;
> + }
> +
> + sprintf(chardev_path, "/dev/fwctl/%s", entry->d_name);
> +
> +read_err:
> + closedir(dir);
> + return rc;
> +}
> diff --git a/meson_options.txt b/meson_options.txt
> index 5c41b1abe1f1..acc19be4ff0a 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -28,3 +28,5 @@ option('iniparserdir', type : 'string',
> description : 'Path containing the iniparser header files')
> option('modprobedatadir', type : 'string',
> description : '''${sysconfdir}/modprobe.d/''')
> +option('fwctl', type : 'feature', value : 'enabled',
> + description : 'enable firmware control')
Just looking at what is in the diff, seems that indent isn't
matching local style.
Jonathan
