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>

> 
> ---
> Changes:
> [iweiny: Change .../memX/dc/* to .../memX/dcY/*]
> [iweiny: add read only and shareable attributes from DSMAS]
> [djiang: Split sysfs docs]
> [iweiny: Adjust sysfs doc dates]
> [iweiny: Add qos details]
> ---
>  Documentation/ABI/testing/sysfs-bus-cxl |  45 ++++++++++++
>  drivers/cxl/core/memdev.c               | 126 
> ++++++++++++++++++++++++++++++++
>  2 files changed, 171 insertions(+)
> 
> 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?

> +             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.

> +}
> +
> +static ssize_t show_qos_class_dcN(struct cxl_memdev *cxlmd, char *buf, int 
> pos)
> +{
> +     struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
> +
> +     return sysfs_emit(buf, "%d\n", mds->dc_perf[pos].qos_class);
> +}
> +
> +#define CXL_MEMDEV_DC_ATTR_GROUP(n)                                          
> \
> +static ssize_t dc##n##_size_show(struct device *dev,                         
> \
> +                              struct device_attribute *attr,                 
> \
> +                              char *buf)                                     
> \
> +{                                                                            
> \
> +     return show_size_dcN(to_cxl_memdev(dev), buf, (n));                     
> \
> +}                                                                            
> \
> +struct device_attribute dc##n##_size = {                                     
> \
> +     .attr   = { .name = "size", .mode = 0444 },                             
> \
> +     .show   = dc##n##_size_show,                                            
> \
> +};                                                                           
> \
> +static ssize_t dc##n##_read_only_show(struct device *dev,                    
> \
> +                                   struct device_attribute *attr,            
> \
> +                                   char *buf)                                
> \
> +{                                                                            
> \
> +     return show_read_only_dcN(to_cxl_memdev(dev), buf, (n));                
> \
> +}                                                                            
> \
> +struct device_attribute dc##n##_read_only = {                                
>         \
> +     .attr   = { .name = "read_only", .mode = 0444 },                        
> \
> +     .show   = dc##n##_read_only_show,                                       
> \
> +};                                                                           
> \
> +static ssize_t dc##n##_shareable_show(struct device *dev,                    
> \
> +                                  struct device_attribute *attr,             
> \
> +                                  char *buf)                                 
> \
> +{                                                                            
> \
> +     return show_shareable_dcN(to_cxl_memdev(dev), buf, (n));                
> \
> +}                                                                            
> \
> +struct device_attribute dc##n##_shareable = {                                
>         \
> +     .attr   = { .name = "shareable", .mode = 0444 },                        
> \
> +     .show   = dc##n##_shareable_show,                                       
> \
> +};                                                                           
> \
> +static ssize_t dc##n##_qos_class_show(struct device *dev,                    
> \
> +                                   struct device_attribute *attr,            
> \
> +                                   char *buf)                                
> \
> +{                                                                            
> \
> +     return show_qos_class_dcN(to_cxl_memdev(dev), buf, (n));                
> \
> +}                                                                            
> \
> +struct device_attribute dc##n##_qos_class = {                                
>         \
> +     .attr   = { .name = "qos_class", .mode = 0444 },                        
> \
> +     .show   = dc##n##_qos_class_show,                                       
> \
> +};                                                                           
> \
> +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.

> +};                                                                           
> \
> +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!

We do have precedence in hdm.c 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.


> +             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;

> +}                                                                            
> \
>


Reply via email to