Re: [ndctl PATCH 1/2] cxl: Add root port attribute to cxl list output

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Fri, 2022-08-19 at 01:54 -0700, sunfishho12@gmail.com wrote:
> From: Matthew Ho <sunfishho12@gmail.com>
> 
> This adds a "root port" attribute to ports and type 3 memory devices
> located under a host bridge indicating which root port each device
> falls under. Previously, the cxl list command lists the various root
> ports as dports underneath each host bridge, and displays devices as
> being under a given host bridge, but did not indicate which specific
> root port corresponded to which device. Adding this information
> helps to map the CXL topology.

Hm is this patch actually needed - the json output can already list
port hierarchies. So would it instead be possible to have a 'walk to
root' helper that the graphing functions can instead call each time.

A library helper to get to the root port each time would be better than
just adding it to the JSON output, as that seems redundant.

Otherwise some general comments below:

> 
> Signed-off-by: Matthew Ho <sunfishho12@gmail.com>
> ---
>  cxl/json.c         | 16 ++++++++++-
>  cxl/lib/libcxl.c   | 66 ++++++++++++++++++++++++++++++++++++++++++++++
>  cxl/lib/libcxl.sym |  3 ++-
>  cxl/libcxl.h       |  2 ++
>  4 files changed, 85 insertions(+), 2 deletions(-)
> 
> diff --git a/cxl/json.c b/cxl/json.c
> index 9cec58b482b6..ad82f37fc955 100644
> --- a/cxl/json.c
> +++ b/cxl/json.c
> @@ -303,7 +303,7 @@ err_jobj:
>  struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
>                 unsigned long flags)
>  {
> -       const char *devname = cxl_memdev_get_devname(memdev);
> +       const char *devname = cxl_memdev_get_devname(memdev), *rp;
>         struct json_object *jdev, *jobj;
>         unsigned long long serial;
>         int numa_node;
> @@ -324,6 +324,13 @@ struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
>         if (jobj)
>                 json_object_object_add(jdev, "ram_size", jobj);
>  
> +       rp = cxl_memdev_get_root_port(memdev);
> +       if (rp) {
> +               jobj = json_object_new_string(rp);
> +               if (jobj)
> +                       json_object_object_add(jdev, "root port", jobj);
> +       }
> +
>         if (flags & UTIL_JSON_HEALTH) {
>                 jobj = util_cxl_memdev_health_to_json(memdev, flags);
>                 if (jobj)
> @@ -727,6 +734,7 @@ static struct json_object *__util_cxl_port_to_json(struct cxl_port *port,
>                                                    unsigned long flags)
>  {
>         const char *devname = cxl_port_get_devname(port);
> +       const char *rp = cxl_port_get_root_port(port);
>         struct json_object *jport, *jobj;
>  
>         jport = json_object_new_object();
> @@ -741,6 +749,12 @@ static struct json_object *__util_cxl_port_to_json(struct cxl_port *port,
>         if (jobj)
>                 json_object_object_add(jport, "host", jobj);
>  
> +       if (rp != NULL) {
> +               jobj = json_object_new_string(rp);
> +               if (jobj)
> +                       json_object_object_add(jport, "root port", jobj);
> +       }
> +
>         if (!cxl_port_is_enabled(port)) {
>                 jobj = json_object_new_string("disabled");
>                 if (jobj)
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index a40b0e6aee83..c5b8ff339eb8 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -1260,6 +1260,39 @@ CXL_EXPORT unsigned long long cxl_memdev_get_ram_size(struct cxl_memdev *memdev)
>         return memdev->ram_size;
>  }
>  
> +CXL_EXPORT const char *cxl_memdev_get_root_port(struct cxl_memdev *memdev)
> +{
> +       char *rp_name = strdup(memdev->host_path), *save;
> +       const char *rp;
> +       bool host_bridge_found = false;
> +
> +       if (!memdev->host_path)
> +               return NULL;
> +
> +       rp_name = strdup(memdev->host_path);
> +
> +       if (!rp_name) {
> +               printf("Error: strdup failed in cxl_memdev_get_Root_port\n");

As a general rule, only JSON formatted output goes to stdout.
Everything else goes to stderr, and use the logging helpers (e.g.
dbg(), err(), etc. ) for any output from libcxl, so that if logging is
being redirected to something other than stderr, everything stays
consistent.

> +               return NULL;
> +       }
> +
> +       rp = strtok_r(rp_name, "/", &save);
> +
> +       while (rp) {
> +               /* return the first substring after the host bridge */
> +
> +               if (host_bridge_found)
> +                       return (char *)rp;
> +
> +               if (!strncmp(rp, "pci", strlen("pci")))
> +                       host_bridge_found = true;
> +
> +               rp = strtok_r(NULL, "/", &save);
> +       }
> +       return NULL;
> +}
> +
> +
>  CXL_EXPORT const char *cxl_memdev_get_firmware_verison(struct cxl_memdev *memdev)
>  {
>         return memdev->firmware_version;
> @@ -2405,6 +2438,39 @@ CXL_EXPORT const char *cxl_port_get_host(struct cxl_port *port)
>         return devpath_to_devname(port->uport);
>  }
>  
> +CXL_EXPORT const char *cxl_port_get_root_port(struct cxl_port *port)
> +{
> +       char *rp_name, *save;
> +       const char *rp;
> +       bool host_bridge_found = false;
> +
> +       if (!port->uport)
> +               return NULL;
> +
> +       rp_name = strdup(port->uport);
> +
> +       if (!rp_name) {
> +               printf("Error: strdup failed in %s\n", __func__);
> +               return NULL;
> +       }
> +
> +       rp = strtok_r(rp_name, "/", &save);
> +
> +       while (rp) {
> +               /* return the first substring after the host bridge */
> +
> +               if (host_bridge_found)
> +                       return (char *)rp;
> +
> +               if (!strncmp(rp, "pci", strlen("pci")))
> +                       host_bridge_found = true;
> +
> +               rp = strtok_r(NULL, "/", &save);
> +       }
> +       return NULL;
> +}
> +
> +
>  CXL_EXPORT bool cxl_port_hosts_memdev(struct cxl_port *port,
>                                       struct cxl_memdev *memdev)
>  {
> diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
> index 549f88dae6ff..4f61686a3aff 100644
> --- a/cxl/lib/libcxl.sym
> +++ b/cxl/lib/libcxl.sym
> @@ -19,6 +19,7 @@ global:
>         cxl_memdev_get_ctx;
>         cxl_memdev_get_pmem_size;
>         cxl_memdev_get_ram_size;
> +       cxl_memdev_get_root_port;

The way the symbol script works is that each release gets a new
'section' for all the symbols that it adds. So v74's new symbols were
added to the 'LIBCXL_3' section. If this is targeting the next, v75
release, its new APIs will go into a new section - LIBCXL_4. If
LIBCXL_4 already existed on the pending branch, you can append into
that section at the bottom. But in this case since it doesn't exist,
you can create one, and add the new APIs in this patch there.

>         cxl_memdev_get_firmware_verison;
>         cxl_cmd_get_devname;
>         cxl_cmd_new_raw;
> @@ -101,7 +102,7 @@ global:
>         cxl_port_to_endpoint;
>         cxl_port_get_bus;
>         cxl_port_get_host;
> -       cxl_port_get_bus;

Was this deletion accidental?

> +       cxl_port_get_root_port;

Same as above regtarding the new section - there's no need to group
port things vs memdev things in this file.

>         cxl_port_hosts_memdev;
>         cxl_port_get_nr_dports;
>         cxl_port_disable_invalidate;
> diff --git a/cxl/libcxl.h b/cxl/libcxl.h
> index 61c7fc4e39b8..9ffa640bc832 100644
> --- a/cxl/libcxl.h
> +++ b/cxl/libcxl.h
> @@ -47,6 +47,7 @@ int cxl_memdev_get_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);
> +const char *cxl_memdev_get_root_port(struct cxl_memdev *memdev);
>  const char *cxl_memdev_get_firmware_verison(struct cxl_memdev *memdev);
>  size_t cxl_memdev_get_label_size(struct cxl_memdev *memdev);
>  int cxl_memdev_disable_invalidate(struct cxl_memdev *memdev);
> @@ -95,6 +96,7 @@ bool cxl_port_is_endpoint(struct cxl_port *port);
>  struct cxl_endpoint *cxl_port_to_endpoint(struct cxl_port *port);
>  struct cxl_bus *cxl_port_get_bus(struct cxl_port *port);
>  const char *cxl_port_get_host(struct cxl_port *port);
> +const char *cxl_port_get_root_port(struct cxl_port *port);
>  bool cxl_port_hosts_memdev(struct cxl_port *port, struct cxl_memdev *memdev);
>  int cxl_port_get_nr_dports(struct cxl_port *port);
>  int cxl_port_disable_invalidate(struct cxl_port *port);