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

From: Dave Jiang <dave.jiang@intel.com>
To: wj28.lee@samsung.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 09:36:57 -0700	[thread overview]
Message-ID: <d821fa6e-da62-4b37-891d-409416b8e999@intel.com> (raw)
In-Reply-To: <20240214080033epcms2p49e131a0012d95c99591b60f36d4cda35@epcms2p4>


On 2/14/24 1:00 AM, Wonjae Lee wrote:
> 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?

Yes. I missed that one. Thank you!
> 
> 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