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

Jonathan Cameron
2024-04-04T08:51:42Z

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

> From: Navneet Singh <navneet.singh@intel.com>
>
> 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@intel.com>
> Co-developed-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

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@huawei.com>

>
> ---
> 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@vger.kernel.org
> +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@vger.kernel.org
> +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;
> +}
> +

>