[ndctl PATCH 0/2] cxl-list: Construct CXL topology graph images

[ndctl PATCH 0/2] cxl-list: Construct CXL topology graph images

[Fan Ni]

This patch series extends the `cxl list` subcommand to show the cxl
topology visually. Mattew Ho first worked on the code and provided an
initial patch as list below[1].

This patch series includes the following two patches,
1) Patch 1 adds a parent_dport attribute to ports and type 3 memory devices to
show which downstream port a component is attached. This attribute will be used
in patch 2 to generate the cxl topology graph.
2) Patch 2 extends the `cxl list` subcommand to dump the cxl topology to a
json format file or generate a graph showing the cxl topology. To use the
extended function, the option `-o output.suffix` is added. Acceptable output
suffixes include .jpeg, .jpg and .png for generating a graph and for other
suffix, it will dump the json-formatted cxl topology to the file, which
can be used the input file (with --input option) to generate the graph
later.

Patch 2 reuses the plotting functions in Matthew Ho's patch, which are updated
to work with parent_dport attribute in patch 1. Also, some bugs are
fixed. More detailed changes are listed in Patch 2's commit log.

The patch series is tested with the following different cxl topologies,
1) a single memdev attached the only root port of the single HB in the system;
2) two memdevs attached to the two root ports of the single HB in the system;
3) four memdevs attached to two HBs, each of which has two root ports;
4) four memdevs attached to the downstream ports of a cxl switch which is
attached to one of the two root ports of a HB in the system.


[1] Mattew Ho's patch:
https://lore.kernel.org/linux-cxl/cover.1660895649.git.sunfishho12@gmail.com/





Fan Ni (2):
  cxl-list: add `parent_dport` to cxl_port/memdev objects
  cxl-list: Construct CXL topology graph images

 Documentation/cxl/cxl-list.txt |  16 ++
 cxl/filter.c                   |  45 +++-
 cxl/filter.h                   |   5 +
 cxl/graph.c                    | 409 +++++++++++++++++++++++++++++++++
 cxl/graph.h                    |  20 ++
 cxl/json.c                     |  11 +
 cxl/lib/libcxl.c               |  43 ++++
 cxl/lib/libcxl.sym             |   6 +
 cxl/lib/private.h              |   1 +
 cxl/libcxl.h                   |   3 +
 cxl/list.c                     |  31 +++
 cxl/meson.build                |   2 +
 meson.build                    |   1 +
 util/json.c                    |   2 +-
 14 files changed, 590 insertions(+), 5 deletions(-)
 create mode 100644 cxl/graph.c
 create mode 100644 cxl/graph.h

-- 
2.25.1

[ndctl PATCH 1/2] cxl-list: add `parent_dport` to cxl_port/memdev objects

[Fan Ni]

The parent_dport attribute reprents the downstream port that the current
device (cxl_port/cxl_memdev) is attached to. It will help to track the
topology when plotting the cxl graph

Signed-off-by: Fan Ni <fan.ni@samsung.com>
---
 cxl/json.c         |  8 ++++++++
 cxl/lib/libcxl.c   | 39 +++++++++++++++++++++++++++++++++++++++
 cxl/lib/libcxl.sym |  6 ++++++
 cxl/lib/private.h  |  1 +
 cxl/libcxl.h       |  2 ++
 5 files changed, 56 insertions(+)

diff --git a/cxl/json.c b/cxl/json.c
index 63c1751..ac1b0b5 100644
--- a/cxl/json.c
+++ b/cxl/json.c
@@ -348,6 +348,10 @@ struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
 	if (jobj)
 		json_object_object_add(jdev, "host", jobj);
 
+	jobj = json_object_new_string(cxl_memdev_get_parent_dport(memdev));
+	if (jobj)
+		json_object_object_add(jdev, "parent_dport", jobj);
+
 	if (!cxl_memdev_is_enabled(memdev)) {
 		jobj = json_object_new_string("disabled");
 		if (jobj)
@@ -769,6 +773,10 @@ static struct json_object *__util_cxl_port_to_json(struct cxl_port *port,
 	if (jobj)
 		json_object_object_add(jport, "host", jobj);
 
+	jobj = json_object_new_string(cxl_port_get_parent_dport(port));
+	if (jobj)
+		json_object_object_add(jport, "parent_dport", jobj);
+
 	jobj = json_object_new_int(cxl_port_get_depth(port));
 	if (jobj)
 		json_object_object_add(jport, "depth", jobj);
diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index e8c5d44..27ffee0 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -1267,6 +1267,16 @@ CXL_EXPORT const char *cxl_memdev_get_firmware_verison(struct cxl_memdev *memdev
 	return memdev->firmware_version;
 }
 
+CXL_EXPORT const char *cxl_memdev_get_parent_dport(struct cxl_memdev *memdev)
+{
+	struct cxl_port *port = cxl_endpoint_get_port(memdev->endpoint);
+
+	if (port)
+		return cxl_port_get_parent_dport(port);
+
+	return NULL;
+}
+
 static void bus_invalidate(struct cxl_bus *bus)
 {
 	struct cxl_ctx *ctx = cxl_bus_get_ctx(bus);
@@ -1447,6 +1457,24 @@ CXL_EXPORT int cxl_memdev_nvdimm_bridge_active(struct cxl_memdev *memdev)
 	return is_enabled(path);
 }
 
+static struct cxl_dport *cxl_port_find_parent_dport(struct cxl_port *port,
+			struct cxl_port *parent_port)
+{
+	struct cxl_dport *dport;
+
+	if (parent_port && parent_port->nr_dports) {
+		cxl_dport_foreach(parent_port, dport) {
+			/* HB ACPI0016 can have only one dport and being itself */
+			if (!strcmp(port->uport, dport->dev_path))
+				return NULL;
+			if (strstr(port->uport, dport->dev_path))
+				return dport;
+		}
+	}
+
+	return NULL;
+}
+
 static int cxl_port_init(struct cxl_port *port, struct cxl_port *parent_port,
 			 enum cxl_port_type type, struct cxl_ctx *ctx, int id,
 			 const char *cxlport_base)
@@ -1486,6 +1514,8 @@ static int cxl_port_init(struct cxl_port *port, struct cxl_port *parent_port,
 	if (!port->uport)
 		goto err;
 
+	port->parent_dport = cxl_port_find_parent_dport(port, parent_port);
+
 	sprintf(path, "%s/modalias", cxlport_base);
 	if (sysfs_read_attr(ctx, path, buf) == 0)
 		port->module = util_modalias_to_module(ctx, buf);
@@ -2404,6 +2434,15 @@ CXL_EXPORT struct cxl_port *cxl_port_get_parent(struct cxl_port *port)
 	return port->parent;
 }
 
+CXL_EXPORT const char *cxl_port_get_parent_dport(struct cxl_port *port)
+{
+	if (port->parent_dport)
+		return devpath_to_devname(port->parent_dport->dev_path);
+	else if (port->parent)
+		return devpath_to_devname(port->parent->uport);
+	return NULL;
+}
+
 CXL_EXPORT bool cxl_port_is_root(struct cxl_port *port)
 {
 	return port->type == CXL_PORT_ROOT;
diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
index 8bb91e0..33f3bba 100644
--- a/cxl/lib/libcxl.sym
+++ b/cxl/lib/libcxl.sym
@@ -217,3 +217,9 @@ global:
 	cxl_decoder_get_max_available_extent;
 	cxl_decoder_get_region;
 } LIBCXL_2;
+
+LIBCXL_4 {
+global:
+	cxl_memdev_get_parent_dport;
+	cxl_port_get_parent_dport;
+}LIBCXL_3;
diff --git a/cxl/lib/private.h b/cxl/lib/private.h
index 437eade..ee7d769 100644
--- a/cxl/lib/private.h
+++ b/cxl/lib/private.h
@@ -71,6 +71,7 @@ struct cxl_port {
 	struct cxl_bus *bus;
 	enum cxl_port_type type;
 	struct cxl_port *parent;
+	struct cxl_dport *parent_dport;
 	struct kmod_module *module;
 	struct list_node list;
 	struct list_head child_ports;
diff --git a/cxl/libcxl.h b/cxl/libcxl.h
index 9fe4e99..588ad00 100644
--- a/cxl/libcxl.h
+++ b/cxl/libcxl.h
@@ -34,6 +34,7 @@ void cxl_set_private_data(struct cxl_ctx *ctx, void *data);
 void *cxl_get_private_data(struct cxl_ctx *ctx);
 
 struct cxl_memdev;
+const char *cxl_memdev_get_parent_dport(struct cxl_memdev *memdev);
 struct cxl_memdev *cxl_memdev_get_first(struct cxl_ctx *ctx);
 struct cxl_memdev *cxl_memdev_get_next(struct cxl_memdev *memdev);
 int cxl_memdev_get_id(struct cxl_memdev *memdev);
@@ -81,6 +82,7 @@ int cxl_bus_disable_invalidate(struct cxl_bus *bus);
 	     bus = cxl_bus_get_next(bus))
 
 struct cxl_port;
+const char *cxl_port_get_parent_dport(struct cxl_port *port);
 struct cxl_port *cxl_port_get_first(struct cxl_port *parent);
 struct cxl_port *cxl_port_get_next(struct cxl_port *port);
 const char *cxl_port_get_devname(struct cxl_port *port);
-- 
2.25.1

[ndctl PATCH 2/2] cxl-list: Construct CXL topology graph images

[Fan Ni]

The change is based on Matthew Ho' patch as list below.
The main changes include,
    1. remove the `root port` attribute for plotting the topology,
    instead leveraging the `parent_dport` attribute of the memdev and port
    objects to plot the topology.
    2. fix the error messages, using error() instead of printf.
    3. move all the graph related the functions to graph.c.
    4. change the command interface, remove the `--graph` option, and
    use output file type (indicated by the suffix of the file identified by
    the `-o` option) to determine plotting the graph or dump to json
    formatted plain file.
    5. fix some bugs in graph plotting related functions.

Matthew Ho's patch:
https://lore.kernel.org/linux-cxl/cover.1660895649.git.sunfishho12@gmail.com/

Signed-off-by: Fan Ni <fan.ni@samsung.com>
---
 Documentation/cxl/cxl-list.txt |  16 ++
 cxl/filter.c                   |  45 +++-
 cxl/filter.h                   |   5 +
 cxl/graph.c                    | 409 +++++++++++++++++++++++++++++++++
 cxl/graph.h                    |  20 ++
 cxl/json.c                     |   9 +-
 cxl/lib/libcxl.c               |   6 +-
 cxl/libcxl.h                   |   1 +
 cxl/list.c                     |  31 +++
 cxl/meson.build                |   2 +
 meson.build                    |   1 +
 util/json.c                    |   2 +-
 12 files changed, 538 insertions(+), 9 deletions(-)
 create mode 100644 cxl/graph.c
 create mode 100644 cxl/graph.h

diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt
index 14a2b4b..cb49206 100644
--- a/Documentation/cxl/cxl-list.txt
+++ b/Documentation/cxl/cxl-list.txt
@@ -362,6 +362,22 @@ OPTIONS
 	  Everything *-vv* provides, plus enable
 	  --health and --partition.
 
+-o::
+--output-file::
+	Create an image of the CXL topology or a json formatted representation of
+	the CXL. One can specify an output with -o or --output-file, when a
+	jpg/png/jpeg suffix is provided a graph containing a graph of the CXL
+	topology will be generated, and if the suffix is none/txt/json, the cxl
+	topology will be dumped in a json format to the provided file. Note this
+	option will automatically turn on the verbose option (verbose=3) and disable
+	the -E option.
+
+--input::
+	This option takes a json-formatted file with valid cxl topology and generate
+	a graph showing the CXL topology accordingly. The graph will be stored in a
+	file (.jpg, .png or .jpeg) identified by the `-o` or `--output-file` option
+	or in topology.png if no such output option is given.
+
 --debug::
 	If the cxl tool was built with debug enabled, turn on debug
 	messages.
diff --git a/cxl/filter.c b/cxl/filter.c
index 56c6599..554bec6 100644
--- a/cxl/filter.c
+++ b/cxl/filter.c
@@ -8,9 +8,11 @@
 #include <util/json.h>
 #include <cxl/libcxl.h>
 #include <json-c/json.h>
+#include <util/util.h>
 
 #include "filter.h"
 #include "json.h"
+#include "graph.h"
 
 static const char *which_sep(const char *filter)
 {
@@ -1180,16 +1182,22 @@ int cxl_filter_walk(struct cxl_ctx *ctx, struct cxl_filter_params *p)
 			}
 		}
 walk_children:
-		dbg(p, "walk decoders\n");
-		walk_decoders(port, p, pick_array(jchilddecoders, jbusdecoders),
-			      pick_array(jchildregions, jregions), flags);
-
+		/*
+		 * Need to walk ports before walking decoder so dport will be properly
+		 * initialized which is needed to get the correct parent_dport of a port.
+		 * This is needed if we plot the cxl graph after some region is created.
+		 */
 		dbg(p, "walk ports\n");
 		walk_child_ports(port, p, pick_array(jchildports, jports),
 				 pick_array(jchilddecoders, jportdecoders),
 				 pick_array(jchildeps, jeps),
 				 pick_array(jchilddecoders, jepdecoders),
 				 pick_array(jchilddevs, jdevs), flags);
+
+		dbg(p, "walk decoders\n");
+		walk_decoders(port, p, pick_array(jchilddecoders, jbusdecoders),
+			      pick_array(jchildregions, jregions), flags);
+
 		cond_add_put_array_suffix(jbus, "ports", devname, jchildports);
 		cond_add_put_array_suffix(jbus, "endpoints", devname,
 					  jchildeps);
@@ -1232,6 +1240,35 @@ walk_children:
 		     top_level_objs > 1);
 	splice_array(p, jregions, jplatform, "regions", top_level_objs > 1);
 
+	if (p->dump_to_file) {
+		switch (output_file_type(p->output_file)) {
+		case FILE_GRAPH:
+			create_image(p->output_file, jplatform);
+			break;
+		case FILE_PLAIN:
+			FILE *fp = NULL;
+
+			fp = fopen(p->output_file, "w+");
+			if (fp == NULL)
+				error("dump to output file %s failed", p->output_file);
+			else {
+				/***
+				 * we need increase the reference count as util_display_json_array
+				 * are called more than once in which the reference count will be
+				 * decreased by one each time it is called.
+				 ***/
+				json_object_get(jplatform);
+				util_display_json_array(fp, jplatform, flags);
+				fclose(fp);
+			}
+			break;
+		case FILE_UNSUPPORTED:
+			error("dump to output file %s skipped due to  unsupported file type"
+					, p->output_file);
+			break;
+		}
+	}
+
 	util_display_json_array(stdout, jplatform, flags);
 
 	return 0;
diff --git a/cxl/filter.h b/cxl/filter.h
index 256df49..f2d3e4b 100644
--- a/cxl/filter.h
+++ b/cxl/filter.h
@@ -5,6 +5,8 @@
 
 #include <stdbool.h>
 #include <util/log.h>
+#include <json-c/json.h>
+#include <graphviz/gvc.h>
 
 struct cxl_filter_params {
 	const char *memdev_filter;
@@ -14,6 +16,7 @@ struct cxl_filter_params {
 	const char *endpoint_filter;
 	const char *decoder_filter;
 	const char *region_filter;
+	const char *output_file;
 	bool single;
 	bool endpoints;
 	bool decoders;
@@ -26,6 +29,8 @@ struct cxl_filter_params {
 	bool human;
 	bool health;
 	bool partition;
+	bool dump_to_file;
+	const char *input_file;
 	int verbose;
 	struct log_ctx ctx;
 };
diff --git a/cxl/graph.c b/cxl/graph.c
new file mode 100644
index 0000000..e4584bd
--- /dev/null
+++ b/cxl/graph.c
@@ -0,0 +1,409 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2022 Fan Ni <fan.ni@samsung.com>
+// Copyright (C) 2022 Matthew Ho <sunfishho12@gmail.com>
+#include <graphviz/gvc.h>
+#include <util/util.h>
+#include <util/log.h>
+
+#include "graph.h"
+
+static Agnode_t *create_node(Agraph_t *graph, char *label, bool created)
+{
+	return agnode(graph, label, created);
+}
+
+static char *find_device_type(struct json_object *device)
+{
+	char *value;
+
+	json_object_object_foreach(device, property, value_json) {
+		value = (char *)json_object_get_string(value_json);
+		if (!strcmp(property, "bus") &&
+		    !strcmp(value, "root0"))
+			return "ACPI0017 Device";
+		if (!strcmp(property, "host") &&
+		    !strncmp(value, "ACPI0016", strlen("ACPI0016")))
+			return "Host Bridge";
+		if (!strcmp(property, "endpoint"))
+			return "Endpoint";
+		if (!strcmp(property, "host"))
+			return "Switch Port";
+		if (!strcmp(property, "memdev"))
+			return "Type 3 Memory Device";
+		if (!strcmp(property, "dport"))
+			return "dport";
+		if (!strcmp(property, "decoder"))
+			return "decoder";
+		if (!strcmp(property, "provider") &&
+		    !strcmp(value, "cxl_test"))
+			return "cxl_acpi.0";
+	}
+
+	return "unknown device";
+}
+
+static bool check_device_type(struct json_object *device, char *type)
+{
+	return !strcmp(find_device_type(device), type);
+}
+
+/* for labeling purposes */
+static const char *find_device_ID(struct json_object *device)
+{
+	char *dev_type = find_device_type(device);
+	json_object *ID = json_object_new_string("unknown");
+
+	if (!strcmp(dev_type, "ACPI0017 Device")) {
+		json_object_put(ID);
+		json_object_object_get_ex(device, "bus", &ID);
+	}
+
+	if (!strcmp(dev_type, "Host Bridge") || !strcmp(dev_type, "Switch Port")) {
+		json_object_put(ID);
+		json_object_object_get_ex(device, "host", &ID);
+	}
+
+	if (!strcmp(dev_type, "Endpoint")) {
+		json_object_put(ID);
+		json_object_object_get_ex(device, "endpoint", &ID);
+	}
+
+	if (!strcmp(dev_type, "Type 3 Memory Device")) {
+		json_object_put(ID);
+		json_object_object_get_ex(device, "memdev", &ID);
+	}
+
+	if (!strcmp(dev_type, "dport")) {
+		json_object_put(ID);
+		json_object_object_get_ex(device, "dport", &ID);
+	}
+
+	return json_object_get_string(ID);
+}
+
+static bool is_device(struct json_object *device)
+{
+	char *dev_type = find_device_type(device);
+
+	return (strcmp(dev_type, "dport") && strcmp(dev_type, "decoder"));
+}
+
+static char *remove_double_quote_from_json_str(const char *json_str)
+{
+	char *p = strdup(json_str);
+	size_t i, j = 0;
+
+	for (i = 0; i < strlen(json_str); i++) {
+		if (json_str[i] != '\"')
+			p[j++] = json_str[i];
+	}
+	p[j] = '\0';
+	return p;
+}
+
+static char *find_parent_dport(struct json_object *device)
+{
+	json_object *rp;
+	const char *dport_str;
+
+	rp = json_object_new_string("");
+	json_object_put(rp);
+	if (!json_object_object_get_ex(device, "parent_dport", &rp))
+		return NULL;
+
+	dport_str = json_object_to_json_string_ext(rp, JSON_C_TO_STRING_NOSLASHESCAPE);
+	return remove_double_quote_from_json_str(dport_str);
+}
+
+static char *find_parent_dport_label(struct json_object *device)
+{
+	char *rp_node_name;
+	char *id = find_parent_dport(device);
+
+	if (!id)
+		return NULL;
+	asprintf(&rp_node_name, "dPort\nID: %s", id);
+	free(id);
+	if (!rp_node_name)
+		error("asprintf failed in %s\n", __func__);
+	return rp_node_name;
+}
+
+static char *find_root_port_label(struct json_object *device)
+{
+	char *rp_node_name;
+	char *id = find_parent_dport(device);
+
+	if (!id)
+		return NULL;
+	asprintf(&rp_node_name, "Root Port\nID: %s", id);
+	free(id);
+	if (!rp_node_name)
+		error("asprintf failed in %s\n", __func__);
+	return rp_node_name;
+}
+
+static char *label_device(struct json_object *device)
+{
+	char *label;
+	const char *ID = find_device_ID(device);
+	const char *devname = find_device_type(device);
+
+	asprintf(&label, "%s\nID: %s", devname, ID);
+	if (!label)
+		error("label allocation failed in %s\n", __func__);
+	return label;
+}
+
+static void create_root_ports(struct json_object *host_bridge, Agraph_t *graph,
+		       Agnode_t *hb)
+{
+	json_object *rps, *rp, *id_json;
+	char *id, *dport_label;
+	Agnode_t *dport;
+	size_t nr_dports, idx;
+
+	assert(check_device_type(host_bridge, "Host Bridge"));
+	if (!json_object_object_get_ex(host_bridge, "dports", &rps))
+		return;
+
+	nr_dports = json_object_array_length(rps);
+	for (idx = 0; idx < nr_dports; idx++) {
+		rp = json_object_array_get_idx(rps, idx);
+		json_object_object_get_ex(rp, "dport", &id_json);
+		id = (char *)json_object_get_string(id_json);
+		asprintf(&dport_label, "Root Port\nID: %s", id);
+		if (!dport_label)
+			error("label allocation failed when creating root port nodes\n");
+		dport = create_node(graph, dport_label, 1);
+		agedge(graph, hb, dport, 0, 1);
+		free(dport_label);
+	}
+}
+
+static void create_downstream_ports(struct json_object *sw_port,
+		Agraph_t *graph, Agnode_t *sw)
+{
+	json_object *dps, *dp, *id_json;
+	char *id, *dport_label;
+	Agnode_t *dport;
+	size_t nr_dports, idx;
+
+	assert(check_device_type(sw_port, "Switch Port"));
+	if (!json_object_object_get_ex(sw_port, "dports", &dps))
+		return;
+
+	nr_dports = json_object_array_length(dps);
+	for (idx = 0; idx < nr_dports; idx++) {
+		dp = json_object_array_get_idx(dps, idx);
+		json_object_object_get_ex(dp, "dport", &id_json);
+		id = (char *)json_object_get_string(id_json);
+		asprintf(&dport_label, "dPort\nID: %s", id);
+		if (!dport_label)
+			error("label allocation failed when creating downstream port nodes\n");
+		dport = create_node(graph, dport_label, 1);
+		agedge(graph, sw, dport, 0, 1);
+		free(dport_label);
+	}
+}
+
+/* for determining number of devices listed in a json array */
+static size_t count_top_devices(struct json_object *top_array)
+{
+	size_t dev_counter = 0;
+	size_t top_array_len = json_object_array_length(top_array);
+
+	for (size_t idx = 0; idx < top_array_len; idx++)
+		if (is_device(json_object_array_get_idx(top_array, idx)))
+			dev_counter++;
+	return dev_counter;
+}
+
+static Agnode_t **draw_subtree(struct json_object *current_array,
+			       Agraph_t *graph)
+{
+	size_t json_array_len, nr_top_devices, obj_idx;
+	size_t idx, nr_sub_devs, nr_devs_connected;
+	char *label, *parent_dport_label;
+	Agnode_t **top_devices, **sub_devs, *parent_node;
+	bool is_hb, is_sw;
+	json_object *device, *subdev_arr, *subdev;
+	json_object_iter subdev_iter;
+
+	json_array_len = json_object_array_length(current_array);
+	nr_top_devices = count_top_devices(current_array);
+
+	if (!nr_top_devices)
+		return NULL;
+
+	top_devices = malloc(nr_top_devices * sizeof(device));
+	if (!top_devices)
+		error("unable to allocate memory for top_devices\n");
+
+	for (obj_idx = 0; obj_idx < json_array_len; obj_idx++) {
+		device = json_object_array_get_idx(current_array, obj_idx);
+		if (!is_device(device))
+			continue;
+
+		label = label_device(device);
+		top_devices[obj_idx] = create_node(graph, label, 1);
+
+		agsafeset(top_devices[obj_idx], "shape", "box", "");
+
+		is_hb = check_device_type(device, "Host Bridge");
+		is_sw = check_device_type(device, "Switch Port");
+
+		/* Create root port nodes if device is a host bridge */
+		if (is_hb)
+			create_root_ports(device, graph, top_devices[obj_idx]);
+		else if (is_sw)
+			create_downstream_ports(device, graph, top_devices[obj_idx]);
+
+		free(label);
+
+		/* Iterate through all keys and values of an object (device) */
+		json_object_object_foreachC(device, subdev_iter) {
+			subdev_arr = subdev_iter.val;
+			if (!json_object_is_type(subdev_arr, json_type_array))
+				continue;
+			nr_sub_devs = count_top_devices(subdev_arr);
+			sub_devs = draw_subtree(subdev_arr, graph);
+			if (!sub_devs)
+				continue;
+			if (!is_hb && !is_sw) {
+				for (idx = 0; idx < nr_sub_devs; idx++)
+					agedge(graph, top_devices[obj_idx], sub_devs[idx], 0, 1);
+				free(sub_devs);
+				continue;
+			}
+
+			nr_devs_connected = 0;
+			for (idx = 0;
+			     idx < json_object_array_length(subdev_arr);
+			     idx++) {
+				subdev = json_object_array_get_idx(subdev_arr, idx);
+				if (!is_device(subdev))
+					continue;
+
+				if (is_hb)
+					parent_dport_label = find_root_port_label(subdev);
+				else
+					parent_dport_label = find_parent_dport_label(subdev);
+				if (!parent_dport_label) {
+					error("please use cxl input with parent_dport attributes\n");
+					return NULL;
+				}
+				/* with flag = 0, it will search to locate an existing node */
+				parent_node = create_node(graph, parent_dport_label, 0);
+				agedge(graph, parent_node, sub_devs[nr_devs_connected], 0, 1);
+				free(parent_dport_label);
+				nr_devs_connected++;
+			}
+			free(sub_devs);
+		}
+	}
+
+	return top_devices;
+}
+
+struct json_object *parse_json_text(const char *path)
+{
+	FILE *fp;
+	char *json_as_string;
+	size_t file_len;
+	json_object *json;
+
+	fp = fopen(path, "r");
+	if (!fp)
+		error("could not read file\n");
+	fseek(fp, 0, SEEK_END);
+	file_len = ftell(fp);
+	fseek(fp, 0, SEEK_SET);
+	json_as_string = malloc(file_len + 1);
+	if (!json_as_string ||
+	    fread(json_as_string, 1, file_len, fp) != file_len) {
+		free(json_as_string);
+		error("could not read file %s\n", path);
+	}
+	json_as_string[file_len] = '\0';
+	json = json_tokener_parse(json_as_string);
+	return json;
+}
+
+int output_file_type(const char *file_name)
+{
+	/* skip ./, ../ in the path */
+	char *of_extension = strrchr(file_name, '/');
+
+	if (!of_extension)
+		of_extension = (char *)file_name;
+	else
+		of_extension += 1;
+
+	of_extension = strrchr(of_extension, '.');
+	if (!of_extension)
+		return FILE_PLAIN;
+	of_extension += 1;
+	if (!strcmp(of_extension, "json") ||
+			!strcmp(of_extension, "log") || !strcmp(of_extension, "txt"))
+		return FILE_PLAIN;
+	else if ((strcmp(of_extension, "png") && strcmp(of_extension, "jpeg") &&
+				strcmp(of_extension, "jpg"))) {
+		error("Unsupported output file type: %s", file_name);
+		return FILE_UNSUPPORTED;
+	} else
+		return FILE_GRAPH;
+}
+
+void create_image(const char *filename, json_object *platform)
+{
+	char *output_file = (char *)filename;
+	GVC_t *gvc;
+	Agraph_t *graph;
+	char *of_extension = strrchr(output_file, '.') + 1;
+	Agnode_t **top_devices;
+	FILE *FP;
+
+	gvc = gvContext();
+	if (!gvc) {
+		error("Creating gvContext failed when trying to create cxl image");
+		return;
+	}
+	graph = agopen("graph", Agdirected, 0);
+	if (!graph) {
+		error("Trying to agopen failed when trying to create cxl image");
+		goto free_ctx;
+	}
+
+	if (!of_extension || (strcmp(of_extension, "png") &&
+			      strcmp(of_extension, "jpeg") &&
+			      strcmp(of_extension, "jpg"))) {
+		error("Unacceptable output file type, please use .png, .jpeg, or .jpg\n");
+		goto close_graph;
+	}
+
+	top_devices = draw_subtree(platform, graph);
+	if (top_devices)
+		free(top_devices);
+
+	if (gvLayout(gvc, graph, "dot")) {
+		error("Calling gvLayout failed when trying to create cxl image");
+		goto close_graph;
+	}
+
+	FP = fopen(output_file, "w");
+	if (!FP) {
+		error("Creating %s for storing the graph failed", output_file);
+		goto create_exit;
+	} else {
+		gvRender(gvc, graph, strrchr(output_file, '.') + 1, FP);
+		fclose(FP);
+	}
+
+create_exit:
+	gvFreeLayout(gvc, graph);
+close_graph:
+	agclose(graph);
+free_ctx:
+	gvFreeContext(gvc);
+}
diff --git a/cxl/graph.h b/cxl/graph.h
new file mode 100644
index 0000000..1777ce9
--- /dev/null
+++ b/cxl/graph.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2022 Fan Ni <fan.ni@samsung.com> */
+/* Copyright (C) 2022 Matthew Ho <sunfishho12@gmail.com> */
+#ifndef _CXL_TOPOLOGY_GRAPH_H_
+#define _CXL_TOPOLOGY_GRAPH_H_
+
+#include <stdbool.h>
+#include <json-c/json.h>
+
+enum output_file_type {
+		FILE_PLAIN,
+		FILE_GRAPH,
+		FILE_UNSUPPORTED,
+};
+
+struct json_object;
+void create_image(const char *filename, json_object *platform);
+int output_file_type(const char *file_name);
+struct json_object *parse_json_text(const char *path);
+#endif
diff --git a/cxl/json.c b/cxl/json.c
index ac1b0b5..c8a2f89 100644
--- a/cxl/json.c
+++ b/cxl/json.c
@@ -306,6 +306,7 @@ struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
 	const char *devname = cxl_memdev_get_devname(memdev);
 	struct json_object *jdev, *jobj;
 	unsigned long long serial;
+	const char *parent_dport = cxl_memdev_get_parent_dport(memdev);
 	int numa_node;
 
 	jdev = json_object_new_object();
@@ -348,9 +349,11 @@ struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
 	if (jobj)
 		json_object_object_add(jdev, "host", jobj);
 
-	jobj = json_object_new_string(cxl_memdev_get_parent_dport(memdev));
-	if (jobj)
-		json_object_object_add(jdev, "parent_dport", jobj);
+	if (parent_dport) {
+		jobj = json_object_new_string(cxl_memdev_get_parent_dport(memdev));
+		if (jobj)
+			json_object_object_add(jdev, "parent_dport", jobj);
+	}
 
 	if (!cxl_memdev_is_enabled(memdev)) {
 		jobj = json_object_new_string("disabled");
diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index 27ffee0..78cd509 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -1269,8 +1269,12 @@ CXL_EXPORT const char *cxl_memdev_get_firmware_verison(struct cxl_memdev *memdev
 
 CXL_EXPORT const char *cxl_memdev_get_parent_dport(struct cxl_memdev *memdev)
 {
-	struct cxl_port *port = cxl_endpoint_get_port(memdev->endpoint);
+	struct cxl_port *port;
+
+	if (!memdev->endpoint)
+		return NULL;
 
+	port = cxl_endpoint_get_port(memdev->endpoint);
 	if (port)
 		return cxl_port_get_parent_dport(port);
 
diff --git a/cxl/libcxl.h b/cxl/libcxl.h
index 588ad00..7ad0928 100644
--- a/cxl/libcxl.h
+++ b/cxl/libcxl.h
@@ -61,6 +61,7 @@ int cxl_memdev_read_label(struct cxl_memdev *memdev, void *buf, size_t length,
 		size_t offset);
 int cxl_memdev_write_label(struct cxl_memdev *memdev, void *buf, size_t length,
 		size_t offset);
+const char *cxl_memdev_get_host_path(struct cxl_memdev *memdev);
 
 #define cxl_memdev_foreach(ctx, memdev) \
         for (memdev = cxl_memdev_get_first(ctx); \
diff --git a/cxl/list.c b/cxl/list.c
index 8c48fbb..005ff59 100644
--- a/cxl/list.c
+++ b/cxl/list.c
@@ -11,6 +11,7 @@
 #include <util/parse-options.h>
 
 #include "filter.h"
+#include "graph.h"
 
 static struct cxl_filter_params param;
 static bool debug;
@@ -52,6 +53,11 @@ static const struct option options[] = {
 		    "include memory device health information"),
 	OPT_BOOLEAN('I', "partition", &param.partition,
 		    "include memory device partition information"),
+	OPT_STRING(0, "input", &param.input_file,
+		   "input file path for creating topology image",
+		   "path to file containing a json array describing the topology"),
+	OPT_STRING('o', "output-file", &param.output_file, "output file path",
+		   "path to file to generate graph or dump cxl topology to"),
 	OPT_INCR('v', "verbose", &param.verbose,
 		 "increase output detail"),
 #ifdef ENABLE_DEBUG
@@ -73,6 +79,7 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
 		NULL
 	};
 	int i;
+	json_object *platform;
 
 	argc = parse_options(argc, argv, options, u, 0);
 	for (i = 0; i < argc; i++)
@@ -86,6 +93,24 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
 		usage_with_options(u, options);
 	}
 
+	if (param.output_file)
+		param.dump_to_file = true;
+
+	if (param.input_file) {
+		if (access(param.input_file, R_OK)) {
+			error("input file %s cannot be accessed\n", param.input_file);
+			return 0;
+		}
+		platform = parse_json_text(param.input_file);
+		if (!param.output_file) {
+			warning("no output file name given, using topology.png");
+			create_image("topology.png", platform);
+		} else
+			create_image(param.output_file, platform);
+
+		return 0;
+	}
+
 	if (num_list_flags() == 0) {
 		if (param.memdev_filter || param.serial_filter)
 			param.memdevs = true;
@@ -108,6 +133,12 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
 		param.memdevs = true;
 	}
 
+	if (param.dump_to_file) {
+		param.verbose = 3;
+		param.endpoints = false;
+		param.memdevs = true;
+	}
+
 	switch(param.verbose){
 	default:
 	case 3:
diff --git a/cxl/meson.build b/cxl/meson.build
index f2474aa..5decb11 100644
--- a/cxl/meson.build
+++ b/cxl/meson.build
@@ -7,6 +7,7 @@ cxl_src = [
   'memdev.c',
   'json.c',
   'filter.c',
+  'graph.c',
 ]
 
 cxl_tool = executable('cxl',
@@ -19,6 +20,7 @@ cxl_tool = executable('cxl',
     kmod,
     json,
     versiondep,
+    graphviz,
   ],
   install : true,
   install_dir : rootbindir,
diff --git a/meson.build b/meson.build
index 20a646d..9fb1161 100644
--- a/meson.build
+++ b/meson.build
@@ -142,6 +142,7 @@ kmod = dependency('libkmod')
 libudev = dependency('libudev')
 uuid = dependency('uuid')
 json = dependency('json-c')
+graphviz = dependency('libgvc')
 if get_option('docs').enabled()
   if get_option('asciidoctor').enabled()
     asciidoc = find_program('asciidoctor', required : true)
diff --git a/util/json.c b/util/json.c
index 1d5c6bc..da709a1 100644
--- a/util/json.c
+++ b/util/json.c
@@ -106,7 +106,7 @@ void util_display_json_array(FILE *f_out, struct json_object *jarray,
 		unsigned long flags)
 {
 	int len = json_object_array_length(jarray);
-	int jflag = JSON_C_TO_STRING_PRETTY;
+	int jflag = JSON_C_TO_STRING_PRETTY | JSON_C_TO_STRING_NOSLASHESCAPE;
 
 	if (len > 1 || !(flags & UTIL_JSON_HUMAN)) {
 		if (len == 0)
-- 
2.25.1

RE: [ndctl PATCH 0/2] cxl-list: Construct CXL topology graph images

[Dan Williams]

Fan Ni wrote:
> This patch series extends the `cxl list` subcommand to show the cxl
> topology visually. Mattew Ho first worked on the code and provided an
> initial patch as list below[1].

Thanks for picking this up, it looks like a useful addition to me.

> 
> This patch series includes the following two patches,
> 1) Patch 1 adds a parent_dport attribute to ports and type 3 memory devices to
> show which downstream port a component is attached. This attribute will be used
> in patch 2 to generate the cxl topology graph.

I had a similar patch to do this here:

http://lore.kernel.org/r/167053491908.582963.7783814693644991382.stgit@dwillia2-xfh.jf.intel.com

The biggest difference I see is that your version adds 'parent_dport' to
memdevs where mine keeps it purely an 'port' attribute. So the listing
needs to have --endpoints to get the memdev to 'parent_dport'
assocation.

> 2) Patch 2 extends the `cxl list` subcommand to dump the cxl topology to a
> json format file or generate a graph showing the cxl topology. To use the
> extended function, the option `-o output.suffix` is added. Acceptable output
> suffixes include .jpeg, .jpg and .png for generating a graph and for other
> suffix, it will dump the json-formatted cxl topology to the file, which
> can be used the input file (with --input option) to generate the graph
> later.

I notice that the implementation enforces some implicit listing options,
disables --endpoints, and does some magic with looking at the suffix to
know what to write to the output file. That feels too restrictive and
makes the 'list' command a bit more difficult to reason about. How about
decoupling the support from 'list'? I am thinking of a scheme like this:

   cxl list -vvv | cxl graph -o png > cxl-topo.png

This has a few advantages:
- The graphviz build dependency can be made optional where the user can
  detect if the support is present in the build by the availability of
  the 'graph' subcommand.
- Listings can be created on one system and graphed on another which
  among other usages is useful for debugging topologies over email.
- The 'graph' subcommand can parse and optionally warn about the input
  json rather than implicit forcing of listing options.
- The json can go through additional filtering before being graphed:

    cxl list -vvv $basic_filters | jq $advanced_filters | cxl graph -o png > out

Thoughts?

Re: [ndctl PATCH 0/2] cxl-list: Construct CXL topology graph images

[Fan Ni]

On Tue, Jan 03, 2023 at 11:05:22AM -0800, Dan Williams wrote:



> Fan Ni wrote:
> > This patch series extends the `cxl list` subcommand to show the cxl
> > topology visually. Mattew Ho first worked on the code and provided an
> > initial patch as list below[1].
> 
> Thanks for picking this up, it looks like a useful addition to me.
> 
> > 
> > This patch series includes the following two patches,
> > 1) Patch 1 adds a parent_dport attribute to ports and type 3 memory devices to
> > show which downstream port a component is attached. This attribute will be used
> > in patch 2 to generate the cxl topology graph.
> 
> I had a similar patch to do this here:
> 
> https://urldefense.com/v3/__http://lore.kernel.org/r/167053491908.582963.7783814693644991382.stgit@dwillia2-xfh.jf.intel.com__;!!EwVzqGoTKBqv-0DWAJBm!Xc_tWtX0GBqionoq7JRQxQ39MytJEW3jP-JcQnLrj7Op9_u5mMRg_hUoUvQabHNT_IXi3MbHF2d0TwCsiZckhvCToA$ 
> 
> The biggest difference I see is that your version adds 'parent_dport' to
> memdevs where mine keeps it purely an 'port' attribute. So the listing
> needs to have --endpoints to get the memdev to 'parent_dport'
> assocation.

Thanks for mentioning this, I missed the patch. It seems we can use this patch
to prepare the info for plotting the graph. Need to check more.

> 
> > 2) Patch 2 extends the `cxl list` subcommand to dump the cxl topology to a
> > json format file or generate a graph showing the cxl topology. To use the
> > extended function, the option `-o output.suffix` is added. Acceptable output
> > suffixes include .jpeg, .jpg and .png for generating a graph and for other
> > suffix, it will dump the json-formatted cxl topology to the file, which
> > can be used the input file (with --input option) to generate the graph
> > later.
> 
> I notice that the implementation enforces some implicit listing options,
> disables --endpoints, and does some magic with looking at the suffix to
> know what to write to the output file. That feels too restrictive and
> makes the 'list' command a bit more difficult to reason about. How about

Using file extention to decide whether to dump the output to a json-formatted
file or plot a cxl graph was suggested by Vishal in his previous
comments to Mattew's patch (
https://lore.kernel.org/all/093ebf126ad44920d9c7bfd1c30af5040e0c7020.camel@intel.com/
). We are neutral to either way as long as there is a consensus.

About the enforcements placed on "cxl list" command option, it should be easy to
address. We should be able to remove them and fix in graph functions.

> decoupling the support from 'list'? I am thinking of a scheme like this:
> 
>    cxl list -vvv | cxl graph -o png > cxl-topo.png
> 

Currently, we can do similar things as you mentioned above.
cxl list -vvv > cxl.json (with some changes to remove the option enforcement.)
cxl list -input cxl.json -o cxl-topo.png

But I agree that using a separate subcommand may be a better idea as the
advantage you mentioned below.

> This has a few advantages:
> - The graphviz build dependency can be made optional where the user can
>   detect if the support is present in the build by the availability of
>   the 'graph' subcommand.

IMO, this is the main advantage to introduce a new subcommand.

> - Listings can be created on one system and graphed on another which
>   among other usages is useful for debugging topologies over email.

Our current approach can achieve similar goal with "-input" option.

> - The 'graph' subcommand can parse and optionally warn about the input
>   json rather than implicit forcing of listing options.

Agree.

> - The json can go through additional filtering before being graphed:
> 
>     cxl list -vvv $basic_filters | jq $advanced_filters | cxl graph -o png > out
> 
> Thoughts?

Thanks for the comments. So based on the above comments and my
understanding, I think we need to decide two things before acting
further.
1. whether we want to use suffix to determine plot behaviour or use
explicit arguments.
2. Whether we want to introduce an extra subcommand for the graph
function or stick to existing approach and make changes as needed to
achieve similar goal.

Re: [ndctl PATCH 0/2] cxl-list: Construct CXL topology graph images

[Verma, Vishal L]

On Tue, 2023-01-03 at 21:43 +0000, Fan Ni wrote:
> On Tue, Jan 03, 2023 at 11:05:22AM -0800, Dan Williams wrote:
> > Fan Ni wrote:
> > > This patch series extends the `cxl list` subcommand to show the cxl
> > > topology visually. Mattew Ho first worked on the code and provided an
> > > initial patch as list below[1].
> > 
> > Thanks for picking this up, it looks like a useful addition to me.
> > 
> > > 
> > > This patch series includes the following two patches,
> > > 1) Patch 1 adds a parent_dport attribute to ports and type 3 memory devices to
> > > show which downstream port a component is attached. This attribute will be used
> > > in patch 2 to generate the cxl topology graph.
> > 
> > I had a similar patch to do this here:
> > 
> > https://urldefense.com/v3/__http://lore.kernel.org/r/167053491908.582963.7783814693644991382.stgit@dwillia2-xfh.jf.intel.com__;!!EwVzqGoTKBqv-0DWAJBm!Xc_tWtX0GBqionoq7JRQxQ39MytJEW3jP-JcQnLrj7Op9_u5mMRg_hUoUvQabHNT_IXi3MbHF2d0TwCsiZckhvCToA$ 
> > 
> > The biggest difference I see is that your version adds 'parent_dport' to
> > memdevs where mine keeps it purely an 'port' attribute. So the listing
> > needs to have --endpoints to get the memdev to 'parent_dport'
> > assocation.
> 
> Thanks for mentioning this, I missed the patch. It seems we can use this patch
> to prepare the info for plotting the graph. Need to check more.
> 
> > 
> > > 2) Patch 2 extends the `cxl list` subcommand to dump the cxl topology to a
> > > json format file or generate a graph showing the cxl topology. To use the
> > > extended function, the option `-o output.suffix` is added. Acceptable output
> > > suffixes include .jpeg, .jpg and .png for generating a graph and for other
> > > suffix, it will dump the json-formatted cxl topology to the file, which
> > > can be used the input file (with --input option) to generate the graph
> > > later.
> > 
> > I notice that the implementation enforces some implicit listing options,
> > disables --endpoints, and does some magic with looking at the suffix to
> > know what to write to the output file. That feels too restrictive and
> > makes the 'list' command a bit more difficult to reason about. How about
> 
> Using file extention to decide whether to dump the output to a json-formatted
> file or plot a cxl graph was suggested by Vishal in his previous
> comments to Mattew's patch (
> https://lore.kernel.org/all/093ebf126ad44920d9c7bfd1c30af5040e0c7020.camel@intel.com/
> ). We are neutral to either way as long as there is a consensus.
> 
> About the enforcements placed on "cxl list" command option, it should be easy to
> address. We should be able to remove them and fix in graph functions.
> 
> > decoupling the support from 'list'? I am thinking of a scheme like this:
> > 
> >    cxl list -vvv | cxl graph -o png > cxl-topo.png
> > 
> 
> Currently, we can do similar things as you mentioned above.
> cxl list -vvv > cxl.json (with some changes to remove the option enforcement.)
> cxl list -input cxl.json -o cxl-topo.png
> 
> But I agree that using a separate subcommand may be a better idea as the
> advantage you mentioned below.
> 
> > This has a few advantages:
> > - The graphviz build dependency can be made optional where the user can
> >   detect if the support is present in the build by the availability of
> >   the 'graph' subcommand.
> 
> IMO, this is the main advantage to introduce a new subcommand.
> 
> > - Listings can be created on one system and graphed on another which
> >   among other usages is useful for debugging topologies over email.
> 
> Our current approach can achieve similar goal with "-input" option.
> 
> > - The 'graph' subcommand can parse and optionally warn about the input
> >   json rather than implicit forcing of listing options.
> 
> Agree.
> 
> > - The json can go through additional filtering before being graphed:
> > 
> >     cxl list -vvv $basic_filters | jq $advanced_filters | cxl graph -o png > out
> > 
> > Thoughts?
> 
> Thanks for the comments. So based on the above comments and my
> understanding, I think we need to decide two things before acting
> further.

> 1. whether we want to use suffix to determine plot behaviour or use
> explicit arguments.

I think I was taking inspiration from pandoc when I suggested using
file extensions to deduce the output / input formats. However, I think
it can be valuable to have an explicit override too.

So maybe something like:
  -o topo.png : outputs as a png (deduced from extension)
  -o topo -f png : also outputs as a png
                   (specified explicitly by -f / --format)
  -o topo.png -f jpg : outputs as a jpg - i.e. -f overrides extension

> 2. Whether we want to introduce an extra subcommand for the graph
> function or stick to existing approach and make changes as needed to
> achieve similar goal.

I think the flexibility and decoupling gained from splitting this out
to a new command is quite valuable. Especially considering the ability
to pipe through jq filters. Thanks for this suggestion Dan!

Re: [ndctl PATCH 0/2] cxl-list: Construct CXL topology graph images

[Dan Williams]

Verma, Vishal L wrote:
> On Tue, 2023-01-03 at 21:43 +0000, Fan Ni wrote:
> > On Tue, Jan 03, 2023 at 11:05:22AM -0800, Dan Williams wrote:
> > > Fan Ni wrote:
> > > > This patch series extends the `cxl list` subcommand to show the cxl
> > > > topology visually. Mattew Ho first worked on the code and provided an
> > > > initial patch as list below[1].
> > > 
> > > Thanks for picking this up, it looks like a useful addition to me.
> > > 
> > > > 
> > > > This patch series includes the following two patches,
> > > > 1) Patch 1 adds a parent_dport attribute to ports and type 3 memory devices to
> > > > show which downstream port a component is attached. This attribute will be used
> > > > in patch 2 to generate the cxl topology graph.
> > > 
> > > I had a similar patch to do this here:
> > > 
> > > https://urldefense.com/v3/__http://lore.kernel.org/r/167053491908.582963.7783814693644991382.stgit@dwillia2-xfh.jf.intel.com__;!!EwVzqGoTKBqv-0DWAJBm!Xc_tWtX0GBqionoq7JRQxQ39MytJEW3jP-JcQnLrj7Op9_u5mMRg_hUoUvQabHNT_IXi3MbHF2d0TwCsiZckhvCToA$ 
> > > 
> > > The biggest difference I see is that your version adds 'parent_dport' to
> > > memdevs where mine keeps it purely an 'port' attribute. So the listing
> > > needs to have --endpoints to get the memdev to 'parent_dport'
> > > assocation.
> > 
> > Thanks for mentioning this, I missed the patch. It seems we can use this patch
> > to prepare the info for plotting the graph. Need to check more.
> > 
> > > 
> > > > 2) Patch 2 extends the `cxl list` subcommand to dump the cxl topology to a
> > > > json format file or generate a graph showing the cxl topology. To use the
> > > > extended function, the option `-o output.suffix` is added. Acceptable output
> > > > suffixes include .jpeg, .jpg and .png for generating a graph and for other
> > > > suffix, it will dump the json-formatted cxl topology to the file, which
> > > > can be used the input file (with --input option) to generate the graph
> > > > later.
> > > 
> > > I notice that the implementation enforces some implicit listing options,
> > > disables --endpoints, and does some magic with looking at the suffix to
> > > know what to write to the output file. That feels too restrictive and
> > > makes the 'list' command a bit more difficult to reason about. How about
> > 
> > Using file extention to decide whether to dump the output to a json-formatted
> > file or plot a cxl graph was suggested by Vishal in his previous
> > comments to Mattew's patch (
> > https://lore.kernel.org/all/093ebf126ad44920d9c7bfd1c30af5040e0c7020.camel@intel.com/
> > ). We are neutral to either way as long as there is a consensus.
> > 
> > About the enforcements placed on "cxl list" command option, it should be easy to
> > address. We should be able to remove them and fix in graph functions.
> > 
> > > decoupling the support from 'list'? I am thinking of a scheme like this:
> > > 
> > >    cxl list -vvv | cxl graph -o png > cxl-topo.png
> > > 
> > 
> > Currently, we can do similar things as you mentioned above.
> > cxl list -vvv > cxl.json (with some changes to remove the option enforcement.)
> > cxl list -input cxl.json -o cxl-topo.png
> > 
> > But I agree that using a separate subcommand may be a better idea as the
> > advantage you mentioned below.
> > 
> > > This has a few advantages:
> > > - The graphviz build dependency can be made optional where the user can
> > >   detect if the support is present in the build by the availability of
> > >   the 'graph' subcommand.
> > 
> > IMO, this is the main advantage to introduce a new subcommand.
> > 
> > > - Listings can be created on one system and graphed on another which
> > >   among other usages is useful for debugging topologies over email.
> > 
> > Our current approach can achieve similar goal with "-input" option.
> > 
> > > - The 'graph' subcommand can parse and optionally warn about the input
> > >   json rather than implicit forcing of listing options.
> > 
> > Agree.
> > 
> > > - The json can go through additional filtering before being graphed:
> > > 
> > >     cxl list -vvv $basic_filters | jq $advanced_filters | cxl graph -o png > out
> > > 
> > > Thoughts?
> > 
> > Thanks for the comments. So based on the above comments and my
> > understanding, I think we need to decide two things before acting
> > further.
> 
> > 1. whether we want to use suffix to determine plot behaviour or use
> > explicit arguments.
> 
> I think I was taking inspiration from pandoc when I suggested using
> file extensions to deduce the output / input formats. However, I think
> it can be valuable to have an explicit override too.
> 
> So maybe something like:
>   -o topo.png : outputs as a png (deduced from extension)
>   -o topo -f png : also outputs as a png
>                    (specified explicitly by -f / --format)

Yeah, that seems fine. I was more worried about the implications of 'cxl
list' filter options being silently changed just because someone changed
the output filename from $f.txt to $f.jpg.

>   -o topo.png -f jpg : outputs as a jpg - i.e. -f overrides extension

I think -f can be saved for later.

> > 2. Whether we want to introduce an extra subcommand for the graph
> > function or stick to existing approach and make changes as needed to
> > achieve similar goal.
> 
> I think the flexibility and decoupling gained from splitting this out
> to a new command is quite valuable. Especially considering the ability
> to pipe through jq filters. Thanks for this suggestion Dan!
> 

Yeah, the separation of concerns feels more unix-y.

Re: [ndctl PATCH 0/2] cxl-list: Construct CXL topology graph images

[Davidlohr Bueso]

On Wed, 04 Jan 2023, Verma, Vishal L wrote:

>So maybe something like:
>  -o topo.png : outputs as a png (deduced from extension)
>  -o topo -f png : also outputs as a png
>                   (specified explicitly by -f / --format)
>  -o topo.png -f jpg : outputs as a jpg - i.e. -f overrides extension

Makes sense to me.