Re: [PATCH v2 15/19] cxl/pmem: add id attribute to CXL based nvdimm

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Wed, 21 Sep 2022 08:32:57 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Add an id group attribute for CXL based nvdimm object. The addition allows
> ndctl to display the "unique id" for the nvdimm. The serial number for the
> CXL memory device will be used for this id.
> 
> [
>   {
>       "dev":"nmem10",
>       "id":"0x4",
>       "security":"disabled"
>   },
> ]
> 
> The id attribute is needed by the ndctl security key management to setup a
> keyblob with a unique file name tied to the mem device.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

One comment inline, but feel free to ignore.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/pmem.c |   29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
> index 24bec4ca3866..9f34f8701b57 100644
> --- a/drivers/cxl/pmem.c
> +++ b/drivers/cxl/pmem.c
> @@ -48,6 +48,32 @@ static void unregister_nvdimm(void *nvdimm)
>  	cxl_nvd->bridge = NULL;
>  }
>  
> +static ssize_t id_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct nvdimm *nvdimm = to_nvdimm(dev);
> +	struct cxl_nvdimm *cxl_nvd = nvdimm_provider_data(nvdimm);
> +	struct cxl_memdev *cxlmd = cxl_nvd->cxlmd;
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +
> +	return sysfs_emit(buf, "%lld\n", cxlds->serial);
Given single uses I'd be tempted to not bother with the local variables
except when it gets really olong

	struct cxl_nvdimm *cxl_nvd = nvdimm_provider_data(to_nvdimm(dev));
	struct cxl_dev_state *cxlds = cxl_nvd->cxlmd->cxlds;
maybe.  Up to you on what style you prefer though.



> +}