RE: [NDCTL PATCH] ndctl: cxl: Remove dependency for attributes derived from IDENTIFY command

From: Wonjae Lee <wj28.lee@samsung.com>
To: Dave Jiang <dave.jiang@intel.com>
Cc: "linux-cxl@vger.kernel.org" <linux-cxl@vger.kernel.org>,
	"nvdimm@lists.linux.dev" <nvdimm@lists.linux.dev>,
	"vishal.l.verma@intel.com" <vishal.l.verma@intel.com>,
	"dan.j.williams@intel.com" <dan.j.williams@intel.com>,
	Hojin Nam <hj96.nam@samsung.com>,
	KyungSan Kim <ks0204.kim@samsung.com>
Subject: RE: [NDCTL PATCH] ndctl: cxl: Remove dependency for attributes derived from IDENTIFY command
Date: Wed, 14 Feb 2024 17:00:33 +0900	[thread overview]
Message-ID: <20240214080033epcms2p49e131a0012d95c99591b60f36d4cda35@epcms2p4> (raw)
In-Reply-To: <20240209213917.2288994-1-dave.jiang@intel.com>

On Fri, Feb 09, 2024 at 02:39:17PM -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>
> ---
>  cxl/lib/libcxl.c | 48 ++++++++++++++++++++++++++----------------------
>  cxl/memdev.c     | 15 ++++++++++-----
>  2 files changed, 36 insertions(+), 27 deletions(-)
>

[snip]

> diff --git a/cxl/memdev.c b/cxl/memdev.c
> index 81f07991da06..feab7ea76e78 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)
> +         return -EINVAL;
> + }

Hello,

Doesn't action_write() also need to check the return value of
cxl_memdev_get_label_size() like below?

diff --git a/cxl/memdev.c b/cxl/memdev.c
index feab7ea..de46edc 100644
--- a/cxl/memdev.c
+++ b/cxl/memdev.c
@@ -511,6 +511,8 @@ 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 == ULLONG_MAX)
+                       return -EINVAL;

                fseek(actx->f_in, 0L, SEEK_END);
                size = ftell(actx->f_in);

Thanks,
Wonjae
