Re: [PATCH 08/26] cxl/mem: Expose device dynamic capacity capabilities

[Jonathan Cameron]

On Sun, 24 Mar 2024 16:18:11 -0700
ira.weiny@xxxxxxxxx wrote:

> From: Navneet Singh <navneet.singh@xxxxxxxxx>
> 
> To properly configure CXL regions on Dynamic Capacity Devices (DCD),
> user space will need to know the details of the DC Regions available on
> a device.
> 
> Expose driver dynamic capacity capabilities through sysfs
> attributes.
> 
> Signed-off-by: Navneet Singh <navneet.singh@xxxxxxxxx>
> Co-developed-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>

Trivial comments inline.
Whilst I'd like the directory hidden as per the other suggestions,
I don't mind that much.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>


> 
> ---
> Changes for v1:
> [iweiny: remove review tags]
> [iweiny: mark sysfs for 6.10 kernel]
> ---
>  Documentation/ABI/testing/sysfs-bus-cxl | 17 ++++++++
>  drivers/cxl/core/memdev.c               | 76 +++++++++++++++++++++++++++++++++
>  2 files changed, 93 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index 8b3efaf6563c..8a4f572c8498 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -54,6 +54,23 @@ Description:
>  		identically named field in the Identify Memory Device Output
>  		Payload in the CXL-2.0 specification.
>  
> +What:		/sys/bus/cxl/devices/memX/dc/region_count
> +Date:		June, 2024
> +KernelVersion:	v6.10
> +Contact:	linux-cxl@xxxxxxxxxxxxxxx
> +Description:
> +		(RO) Number of Dynamic Capacity (DC) regions supported on the
> +		device.  May be 0 if the device does not support Dynamic
> +		Capacity.

That will change if you go ahead and hide the directory as per suggestions.

> +
> +What:		/sys/bus/cxl/devices/memX/dc/regionY_size
> +Date:		June, 2024
> +KernelVersion:	v6.10
> +Contact:	linux-cxl@xxxxxxxxxxxxxxx
> +Description:
> +		(RO) Size of the Dynamic Capacity (DC) region Y.  Only

Units always good to have in docs even if somewhat obvious.

> +		available on devices which support DC and only for those
> +		region indexes supported by the device.
>  
>  What:		/sys/bus/cxl/devices/memX/pmem/qos_class
>  Date:		May, 2023
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index d4e259f3a7e9..a7b880e33a7e 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -101,6 +101,18 @@ static ssize_t pmem_size_show(struct device *dev, struct device_attribute *attr,

..

> +static struct attribute *cxl_memdev_dc_attributes[] = {
> +	&dev_attr_region0_size.attr,
> +	&dev_attr_region1_size.attr,
> +	&dev_attr_region2_size.attr,
> +	&dev_attr_region3_size.attr,
> +	&dev_attr_region4_size.attr,
> +	&dev_attr_region5_size.attr,
> +	&dev_attr_region6_size.attr,
> +	&dev_attr_region7_size.attr,
> +	&dev_attr_region_count.attr,
> +	NULL,
> +};
> +
> +static umode_t cxl_dc_visible(struct kobject *kobj, struct attribute *a, int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
> +
> +	/* Not a memory device */
> +	if (!mds)
> +		return 0;
> +
> +	if (a == &dev_attr_region_count.attr)
> +		return a->mode;
> +
> +	/* Show only the regions supported */
> +	if (n < mds->nr_dc_region)
> +		return a->mode;

This feels a bit fragile if anyone adds new attrs in future and for whatever reason
does it before these.

Maybe add a comment at top of cxl_memdev_dc_attributes()?  Say they must be first.

> +
> +	return 0;
> +}
> +

>