6:23 a.m.
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.