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", ¶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-leave@lists.01.org
next prev parent reply other threads:[~2021-02-23 19:23 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-19 2:03 [ndctl PATCH v2 00/13] Initial CXL support Vishal Verma
2021-02-19 2:03 ` [ndctl PATCH v2 01/13] cxl: add a cxl utility and libcxl library Vishal Verma
2021-02-22 21:36 ` Ben Widawsky
2021-02-23 19:23 ` Verma, Vishal L [this message]
2021-02-19 2:03 ` [ndctl PATCH v2 02/13] cxl: add a local copy of the cxl_mem UAPI header Vishal Verma
2021-02-22 21:41 ` Ben Widawsky
2021-02-19 2:03 ` [ndctl PATCH v2 03/13] libcxl: add support for command query and submission Vishal Verma
2021-02-22 21:55 ` Ben Widawsky
2021-02-19 2:03 ` [ndctl PATCH v2 04/13] libcxl: add support for the 'Identify Device' command Vishal Verma
2021-02-22 22:02 ` Ben Widawsky
2021-02-19 2:03 ` [ndctl PATCH v2 05/13] test: rename 'ndctl_test' to 'test_ctx' Vishal Verma
2021-02-19 2:03 ` [ndctl PATCH v2 06/13] test: rename 'ndctl_test_*' helpers to 'test_*' Vishal Verma
2021-02-19 2:03 ` [ndctl PATCH v2 07/13] test: introduce a libcxl unit test Vishal Verma
2021-02-22 22:15 ` Ben Widawsky
2021-02-19 2:03 ` [ndctl PATCH v2 08/13] libcxl: add GET_HEALTH_INFO mailbox command and accessors Vishal Verma
2021-02-19 2:03 ` [ndctl PATCH v2 09/13] libcxl: add support for the 'GET_LSA' command Vishal Verma
2021-02-19 2:03 ` [ndctl PATCH v2 10/13] util/hexdump: Add a util helper to print a buffer in hex Vishal Verma
2021-02-19 2:03 ` [ndctl PATCH v2 11/13] test/libcxl: add a test for {set, get}_lsa commands Vishal Verma
2021-02-19 2:03 ` [ndctl PATCH v2 12/13] Documentation/cxl: add library API documentation Vishal Verma
2021-02-19 2:03 ` [ndctl PATCH v2 13/13] test/libcxl: introduce a command size fuzzing test Vishal Verma
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f62d4e8dd4cd89f68c733b0b108b5b3464908862.camel@intel.com \
--to=vishal.l.verma@intel.com \
--cc=ben.widawsky@intel.com \
--cc=linux-cxl@vger.kernel.org \
--cc=linux-nvdimm@lists.01.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).