Re: [ndctl PATCH v2 01/13] cxl: add a cxl utility and libcxl library

From: "Verma, Vishal L" <vishal.l.verma@intel.com>
To: "Widawsky, Ben" <ben.widawsky@intel.com>
Cc: "linux-nvdimm@lists.01.org" <linux-nvdimm@lists.01.org>,
	"linux-cxl@vger.kernel.org" <linux-cxl@vger.kernel.org>
Subject: Re: [ndctl PATCH v2 01/13] cxl: add a cxl utility and libcxl library
Date: Tue, 23 Feb 2021 19:23:03 +0000	[thread overview]
Message-ID: <f62d4e8dd4cd89f68c733b0b108b5b3464908862.camel@intel.com> (raw)
In-Reply-To: <20210222213615.lwjansmxclewb3xo@intel.com>

On Mon, 2021-02-22 at 13:36 -0800, Ben Widawsky wrote:
[..]
> 
> > +SYNOPSIS
> > +--------
> > +[verse]
> > +'cxl list' [<options>]
> > +
> > +Walk the CXL capable device hierarchy in the system and list all device
> > +instances along with some of their major attributes.
> 
> This doesn't seem to match the above. Here it's just devices and above you talk
> about bridges and switches as well.

Good catch - those can be added in later when we have a sysfs
representation for them. I'll change it to say just devices for now.

[..]
> > +
> > +static void *add_cxl_memdev(void *parent, int id, const char *cxlmem_base)
> > +{
> > +	const char *devname = devpath_to_devname(cxlmem_base);
> > +	char *path = calloc(1, strlen(cxlmem_base) + 100);
> > +	struct cxl_ctx *ctx = parent;
> > +	struct cxl_memdev *memdev, *memdev_dup;
> > +	char buf[SYSFS_ATTR_SIZE];
> > +	struct stat st;
> > +
> > +	if (!path)
> > +		return NULL;
> > +	dbg(ctx, "%s: base: \'%s\'\n", __func__, cxlmem_base);
> > +
> > +	memdev = calloc(1, sizeof(*memdev));
> > +	if (!memdev)
> > +		goto err_dev;
> > +	memdev->id = id;
> > +	memdev->ctx = ctx;
> > +
> > +	sprintf(path, "/dev/cxl/%s", devname);
> > +	if (stat(path, &st) < 0)
> > +		goto err_read;
> > +	memdev->major = major(st.st_rdev);
> > +	memdev->minor = minor(st.st_rdev);
> > +
> > +	sprintf(path, "%s/pmem/size", cxlmem_base);
> > +	if (sysfs_read_attr(ctx, path, buf) < 0)
> > +		goto err_read;
> > +	memdev->pmem_size = strtoull(buf, NULL, 0);
> 
> For strtoull usage and below - it certainly doesn't matter much but maybe using
> 10 for base would better since sysfs is ABI and therefore anything other than
> base 10 is incorrect.

Hm, I followed what libndctl does, but I think there is value in
accepting valid hex even if it is technically 'wrong' per the robustness
principle. How much do we want libcxl/libndctl to be a kernel validation
vehicle vs. just work if you can?

[..]
> > +
> > +static int cmd_help(int argc, const char **argv, struct cxl_ctx *ctx)
> > +{
> > +	const char * const builtin_help_subcommands[] = {
> > +		"list", NULL,
> > +	};
> 
> Move NULL to newline.

Yep.

> 
> > +int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
> > +{
> > +	const struct option options[] = {
> > +		OPT_STRING('d', "memdev", &param.memdev, "memory device name",
> > +			   "filter by CXL memory device name"),
> > +		OPT_BOOLEAN('D', "memdevs", &list.memdevs,
> > +			    "include CXL memory device info"),
> > +		OPT_BOOLEAN('i', "idle", &list.idle, "include idle devices"),
> > +		OPT_BOOLEAN('u', "human", &list.human,
> > +				"use human friendly number formats "),
> > +		OPT_END(),
> > +	};
> > +	const char * const u[] = {
> > +		"cxl list [<options>]",
> > +		NULL
> > +	};
> > +	struct json_object *jdevs = NULL;
> > +	unsigned long list_flags;
> > +	struct cxl_memdev *memdev;
> > +	int i;
> > +
> > +        argc = parse_options(argc, argv, options, u, 0);
> 
> Tab.
> 
> /me looks for .clang-format

Thanks - let me see if I can quickly adapt the kernel's .clang-format
for this and add it in for the next revision.

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org