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

[Dan Williams <dan.j.williams@intel.com>]

Dave Jiang 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.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v5:
> - Add documentation. (Alison)
> - Alloc path on stack. (Alison)
> - Deal with out of entry and no match case. (Alison)
> - Make fwctl operations part of 'struct cxl_fwctl' (Dan)
> ---
>  Documentation/cxl/lib/libcxl.txt | 21 +++++++++
>  cxl/lib/libcxl.c                 | 78 ++++++++++++++++++++++++++++++++
>  cxl/lib/libcxl.sym               |  3 ++
>  cxl/lib/private.h                |  8 ++++
>  cxl/libcxl.h                     |  5 ++
>  5 files changed, 115 insertions(+)
> 
> diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt
> index 25ff406c2920..1e00e2dd1abc 100644
> --- a/Documentation/cxl/lib/libcxl.txt
> +++ b/Documentation/cxl/lib/libcxl.txt
> @@ -42,6 +42,7 @@ struct cxl_memdev *cxl_memdev_get_next(struct cxl_memdev *memdev);
>  struct cxl_ctx *cxl_memdev_get_ctx(struct cxl_memdev *memdev);
>  const char *cxl_memdev_get_host(struct cxl_memdev *memdev)
>  struct cxl_memdev *cxl_endpoint_get_memdev(struct cxl_endpoint *endpoint);
> +struct cxl_fwctl *cxl_memdev_get_fwctl(struct cxl_memdev *memdev);
>  
>  #define cxl_memdev_foreach(ctx, memdev) \
>          for (memdev = cxl_memdev_get_first(ctx); \
> @@ -59,6 +60,9 @@ specific memdev.
>  The host of a memdev is the PCIe Endpoint device that registered its CXL
>  capabilities with the Linux CXL core.
>  
> +A memdev may host a 'struct cxl_fwctl' if CXL Features are supported and
> +Firmware Contrl (FWCTL) is also enabled.

s/Contrl/Control/

Also, do you mean CONFIG_CXL_FEATURES instead of "FWCTL", because
CONFIG_FWCTL is not sufficient.

> +
>  === MEMDEV: Attributes
>  ----
>  int cxl_memdev_get_id(struct cxl_memdev *memdev);
> @@ -185,6 +189,23 @@ device is in use. When CXL_SETPART_NEXTBOOT mode is set, the change
>  in partitioning shall become the “next” configuration, to become
>  active on the next device reset.
>  
> +FWCTL
> +-----
> +The object representing a Firmware Control (FWCTL) device is 'struct cxl_fwctl'.
> +Library interfaces related to these devices have the prefix 'cxl_fwctl_'.
> +These interfaces are associated with retrieving attributes related to the
> +CXL FWCTL character device that is a child of the memdev.

I assume any other fwctl helpers that might get added in the future
would use the object so I would give yourself some wiggle room for
cxl_fwctl to be used for more than just conveying chardev major / minor.

> +
> +=== FWCTL: Attributes
> +----
> +int cxl_fwctl_get_major(struct cxl_memdev *memdev);
> +int cxl_fwctl_get_minor(struct cxl_memdev *memdev);
> +----
> +
> +The character device node for Feature (firmware) control can be found by
> +default at /dev/fwctl/fwctl%d, or created with a major / minor returned
> +from cxl_memdev_get_fwctl_{major,minor}().
> +
>  BUSES
>  -----
>  The CXL Memory space is CPU and Device coherent. The address ranges that
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index bab7343e8a4a..be54db1b9bb9 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -88,6 +88,7 @@ static void free_memdev(struct cxl_memdev *memdev, struct list_head *head)
>  	free(memdev->dev_buf);
>  	free(memdev->dev_path);
>  	free(memdev->host_path);
> +	free(memdev->fwctl);
>  	free(memdev);
>  }
>  
> @@ -1253,6 +1254,64 @@ 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[CXL_PATH_MAX];

This CXL_PATH_MAX approach stands out as different than all the other
path buffer management in the rest of the implementation.

In all other paths ->dev_buf has the scratch space for printing buffers.

It's not broken, but it's also odd for one off functions to switch
schemes.