From: "Verma, Vishal L" <vishal.l.verma@intel.com>
To: "Jiang, Dave" <dave.jiang@intel.com>,
"linux-cxl@vger.kernel.org" <linux-cxl@vger.kernel.org>,
"nvdimm@lists.linux.dev" <nvdimm@lists.linux.dev>
Cc: "Williams, Dan J" <dan.j.williams@intel.com>,
"Lee, Wonjae" <wj28.lee@samsung.com>
Subject: Re: [NDCTL PATCH v2] ndctl: cxl: Remove dependency for attributes derived from IDENTIFY command
Date: Thu, 18 Apr 2024 00:55:19 +0000 [thread overview]
Message-ID: <769ecd8ee591fac68f45b40f73c5b76c0d35f63f.camel@intel.com> (raw)
In-Reply-To: <20240215161620.2739089-1-dave.jiang@intel.com>
On Thu, 2024-02-15 at 09:16 -0700, Dave Jiang wrote:
> A memdev may optionally not host a mailbox and therefore not able to execute
> the IDENTIFY command. Currently the kernel emits empty strings for some of
> the attributes instead of making them invisible in order to keep backward
> compatibility for CXL CLI. Remove dependency of CXL CLI on the existance of
> these attributes and only expose them if they exist. Without the dependency
> the kernel will be able to make the non-existant attributes invisible.
>
> Link: https://lore.kernel.org/all/20230606121534.00003870@Huawei.com/
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v2:
> - Add lable size check for action_write(). (Wonjae)
> ---
> cxl/lib/libcxl.c | 48 ++++++++++++++++++++++++++----------------------
> cxl/memdev.c | 18 +++++++++++++-----
> 2 files changed, 39 insertions(+), 27 deletions(-)
>
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index 1537a33d370e..f807ec4ed4e6 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -1251,28 +1251,30 @@ static void *add_cxl_memdev(void *parent, int id, const char *cxlmem_base)
> 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);
> + if (sysfs_read_attr(ctx, path, buf) == 0)
> + memdev->pmem_size = strtoull(buf, NULL, 0);
>
> sprintf(path, "%s/ram/size", cxlmem_base);
> - if (sysfs_read_attr(ctx, path, buf) < 0)
> - goto err_read;
> - memdev->ram_size = strtoull(buf, NULL, 0);
> + if (sysfs_read_attr(ctx, path, buf) == 0)
> + memdev->ram_size = strtoull(buf, NULL, 0);
>
> sprintf(path, "%s/payload_max", cxlmem_base);
> - if (sysfs_read_attr(ctx, path, buf) < 0)
> - goto err_read;
> - memdev->payload_max = strtoull(buf, NULL, 0);
> - if (memdev->payload_max < 0)
> - goto err_read;
> + if (sysfs_read_attr(ctx, path, buf) == 0) {
> + memdev->payload_max = strtoull(buf, NULL, 0);
> + if (memdev->payload_max < 0)
> + goto err_read;
> + } else {
> + memdev->payload_max = -1;
> + }
>
> sprintf(path, "%s/label_storage_size", cxlmem_base);
> - if (sysfs_read_attr(ctx, path, buf) < 0)
> - goto err_read;
> - memdev->lsa_size = strtoull(buf, NULL, 0);
> - if (memdev->lsa_size == ULLONG_MAX)
> - goto err_read;
> + if (sysfs_read_attr(ctx, path, buf) == 0) {
> + memdev->lsa_size = strtoull(buf, NULL, 0);
> + if (memdev->lsa_size == ULLONG_MAX)
> + goto err_read;
> + } else {
> + memdev->lsa_size = ULLONG_MAX;
> + }
>
> sprintf(path, "%s/serial", cxlmem_base);
> if (sysfs_read_attr(ctx, path, buf) < 0)
> @@ -1299,12 +1301,11 @@ static void *add_cxl_memdev(void *parent, int id, const char *cxlmem_base)
> host[0] = '\0';
>
> sprintf(path, "%s/firmware_version", cxlmem_base);
> - if (sysfs_read_attr(ctx, path, buf) < 0)
> - goto err_read;
> -
> - memdev->firmware_version = strdup(buf);
> - if (!memdev->firmware_version)
> - goto err_read;
> + if (sysfs_read_attr(ctx, path, buf) == 0) {
> + memdev->firmware_version = strdup(buf);
> + if (!memdev->firmware_version)
> + goto err_read;
> + }
>
> memdev->dev_buf = calloc(1, strlen(cxlmem_base) + 50);
> if (!memdev->dev_buf)
> @@ -4543,6 +4544,9 @@ static int lsa_op(struct cxl_memdev *memdev, int op, void *buf,
> if (length == 0)
> return 0;
>
> + if (memdev->payload_max < 0)
> + return -EINVAL;
> +
> label_iter_max = memdev->payload_max - sizeof(struct cxl_cmd_set_lsa);
> while (remaining) {
> cur_len = min((size_t)label_iter_max, remaining);
> diff --git a/cxl/memdev.c b/cxl/memdev.c
> index 81f07991da06..08b6aa50175f 100644
> --- a/cxl/memdev.c
> +++ b/cxl/memdev.c
> @@ -473,10 +473,13 @@ static int action_zero(struct cxl_memdev *memdev, struct action_context *actx)
> size_t size;
> int rc;
>
> - if (param.len)
> + if (param.len) {
> size = param.len;
> - else
> + } else {
> size = cxl_memdev_get_label_size(memdev);
> + if (size == ULLONG_MAX)
Inconsistency between ULLONG_MAX here and ULONG_MAX below. This should
actually be SIZE_MAX considering the return type.
Similarly, when setting lsa_size in add_cxl_memdev() above, in the
'else' case where the sysfs attr was absent, we should use SIZE_MAX.
In the error case for strtoull(), it's probably fine to leave the check
for ULLONG_MAX since that is the max value strtoull() will return, and
we're erroring out at that point anyway. I suppose the right thing to
do there would be to use sscanf() with %zu to parse it correctly for
size_t (of course this incorrectness predates this patch..)
> + return -EINVAL;
> + }
>
> if (cxl_memdev_nvdimm_bridge_active(memdev)) {
> log_err(&ml,
> @@ -509,6 +512,9 @@ static int action_write(struct cxl_memdev *memdev, struct action_context *actx)
> if (!size) {
> size_t label_size = cxl_memdev_get_label_size(memdev);
>
> + if (label_size == ULONG_MAX)
> + return -EINVAL;
> +
> fseek(actx->f_in, 0L, SEEK_END);
> size = ftell(actx->f_in);
> fseek(actx->f_in, 0L, SEEK_SET);
> @@ -547,11 +553,13 @@ static int action_read(struct cxl_memdev *memdev, struct action_context *actx)
> char *buf;
> int rc;
>
> - if (param.len)
> + if (param.len) {
> size = param.len;
> - else
> + } else {
> size = cxl_memdev_get_label_size(memdev);
> -
> + if (size == ULLONG_MAX)
> + return -EINVAL;
> + }
> buf = calloc(1, size);
> if (!buf)
> return -ENOMEM;
prev parent reply other threads:[~2024-04-18 0:55 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-15 16:16 [NDCTL PATCH v2] ndctl: cxl: Remove dependency for attributes derived from IDENTIFY command Dave Jiang
2024-04-18 0:55 ` Verma, Vishal L [this message]
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=769ecd8ee591fac68f45b40f73c5b76c0d35f63f.camel@intel.com \
--to=vishal.l.verma@intel.com \
--cc=dan.j.williams@intel.com \
--cc=dave.jiang@intel.com \
--cc=linux-cxl@vger.kernel.org \
--cc=nvdimm@lists.linux.dev \
--cc=wj28.lee@samsung.com \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).