[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 4 Apr 2024 09:51:38 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: <ira.weiny@...el.com>
CC: Dave Jiang <dave.jiang@...el.com>, Fan Ni <fan.ni@...sung.com>, "Navneet
Singh" <navneet.singh@...el.com>, Dan Williams <dan.j.williams@...el.com>,
Davidlohr Bueso <dave@...olabs.net>, Alison Schofield
<alison.schofield@...el.com>, Vishal Verma <vishal.l.verma@...el.com>,
<linux-btrfs@...r.kernel.org>, <linux-cxl@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 08/26] cxl/mem: Expose device dynamic capacity
capabilities
On Sun, 24 Mar 2024 16:18:11 -0700
ira.weiny@...el.com wrote:
> From: Navneet Singh <navneet.singh@...el.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@...el.com>
> Co-developed-by: Ira Weiny <ira.weiny@...el.com>
> Signed-off-by: Ira Weiny <ira.weiny@...el.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@...wei.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@...r.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@...r.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;
> +}
> +
>
Powered by blists - more mailing lists