Jonathan Cameron wrote: > On Mon, 07 Oct 2024 18:16:19 -0500 > ira.we...@intel.com wrote: > > > From: Navneet Singh <navneet.si...@intel.com> > > > > To properly configure CXL regions on Dynamic Capacity Devices (DCD), > > user space will need to know the details of the DC partitions available. > > > > Expose dynamic capacity capabilities through sysfs. > > > > Signed-off-by: Navneet Singh <navneet.si...@intel.com> > > Co-developed-by: Ira Weiny <ira.we...@intel.com> > > Signed-off-by: Ira Weiny <ira.we...@intel.com> > Some trivial stuff inline that I'm not that bothered about either way. > > Subject to answering Fan's query > Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com> >
[snip] > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl > > b/Documentation/ABI/testing/sysfs-bus-cxl > > index 3f5627a1210a..b865eefdb74c 100644 > > --- a/Documentation/ABI/testing/sysfs-bus-cxl > > +++ b/Documentation/ABI/testing/sysfs-bus-cxl > > @@ -54,6 +54,51 @@ Description: > > identically named field in the Identify Memory Device Output > > Payload in the CXL-2.0 specification. > > > > +What: /sys/bus/cxl/devices/memX/dcY/size > > +Date: December, 2024 > > +KernelVersion: v6.13 > > +Contact: linux-...@vger.kernel.org > > +Description: > > + (RO) Dynamic Capacity (DC) region information. Devices only > > + export dcY if DCD partition Y is supported. > > + dcY/size is the size of each of those partitions. > > + > > +What: /sys/bus/cxl/devices/memX/dcY/read_only > > +Date: December, 2024 > > +KernelVersion: v6.13 > > +Contact: linux-...@vger.kernel.org > > +Description: > > + (RO) Dynamic Capacity (DC) region information. Devices only > > + export dcY if DCD partition Y is supported. > > + dcY/read_only indicates true if the region is exported > > + read_only from the device. > > + > > +What: /sys/bus/cxl/devices/memX/dcY/shareable > > +Date: December, 2024 > > +KernelVersion: v6.13 > > +Contact: linux-...@vger.kernel.org > > +Description: > > + (RO) Dynamic Capacity (DC) region information. Devices only > > + export dcY if DCD partition Y is supported. > > + dcY/shareable indicates true if the region is exported > > + shareable from the device. > > + > > +What: /sys/bus/cxl/devices/memX/dcY/qos_class > > +Date: December, 2024 > > +KernelVersion: v6.13 > > +Contact: linux-...@vger.kernel.org > > +Description: > > + (RO) Dynamic Capacity (DC) region information. Devices only > > + export dcY if DCD partition Y is supported. > > You can document sysfs directories I think, e.g. > https://elixir.bootlin.com/linux/v6.12-rc2/source/Documentation/ABI/stable/sysfs-devices-node#L32 > so maybe > > What: /sys/bus/cxl/device/memX/dcY > Date: December, 2024 > KernelVersion: v6.13 > Contact: linux-...@vger.kernel.org > Description: > Directory containing Dynamic Capacity (DC) region information. > Devices only export dcY if DCD partition Y is supported. > > What: /sys/bus/cxl/devices/memX/dcY/qos_class > Date: December, 2024 > KernelVersion: v6.13 > Contact: linux-...@vger.kernel.org > Description: > For CXL host... > > To avoid the repetition of first bit of docs? The other docs don't do this. For example: /sys/bus/cxl/devices/memX /sys/bus/cxl/devices/memX/ram /sys/bus/cxl/devices/memX/pmem Are not documented like that. I'm inclined to leave it. > > > + platforms that support "QoS Telemmetry" this attribute conveys > > + a comma delimited list of platform specific cookies that > > + identifies a QoS performance class for the persistent partition > > + of the CXL mem device. These class-ids can be compared against > > + a similar "qos_class" published for a root decoder. While it is > > + not required that the endpoints map their local memory-class to > > + a matching platform class, mismatches are not recommended and > > + there are platform specific performance related side-effects > > + that may result. First class-id is displayed. > > > > What: /sys/bus/cxl/devices/memX/pmem/qos_class > > Date: May, 2023 > > > > +static ssize_t show_shareable_dcN(struct cxl_memdev *cxlmd, char *buf, int > > pos) > > +{ > > + struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds); > > + > > + return sysfs_emit(buf, "%s\n", > > + str_false_true(mds->dc_region[pos].shareable)); > > Fan has already raised that these seem backwards. Yep fixed. [snip] > > +static struct attribute *cxl_memdev_dc##n##_attributes[] = { > > \ > > + &dc##n##_size.attr, > > \ > > + &dc##n##_read_only.attr, > > \ > > + &dc##n##_shareable.attr, > > \ > > + &dc##n##_qos_class.attr, > > \ > > + NULL, > > \ > > No comma needed on terminator. Fixed. > > > +}; > > \ > > +static umode_t cxl_memdev_dc##n##_attr_visible(struct kobject *kobj, > > \ > > + struct attribute *a, > > \ > > + int pos) > > \ > > +{ > > \ > > + 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) > > \ > if (!to_cxl_memdev_state(cxlmd->cxlds)) > return 0; > > I dislike long macros so if we can shave them down that is always good! Agreed but this was the most straight forward way to deal with this. I could perhaps break it up by having a 'master macro' which is made of smaller macros... But this works. > > We do have precedence in hdm.c Not in hdm.c directly but all the 'to_XXX()' calls have a type check. So it is modeled that way and is called from other places. > > for just checking the type directly so maybe > if (cxlmd->cxlds->type != CXL_DEVTYPE_CLASSMEM) > > but the above is also fine as compiler should be able to figure out it > doesn't need to do the second half of the inline. I'm going to leave it. > > > > + return 0; > > \ > > + return a->mode; > > \ > > +} > > \ > > +static umode_t cxl_memdev_dc##n##_group_visible(struct kobject *kobj) > > \ > > +{ > > \ > > + 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 or partition not supported */ > > \ > > + if (!mds || n >= mds->nr_dc_region) > > \ > > + return false; > > \ > > + return true; > > \ > > /* Memory device and partition is supported */ > return mds && n < mds->nr_dc_region; Done. Ira
