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", ¶m.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-le...@lists.01.org
