Re: [PATCH v2] cxl/region: Add support to indicate region has extended linear cache

[Dave Jiang <dave.jiang@intel.com>]

On 10/21/25 12:53 PM, Cheatham, Benjamin wrote:
> On 10/17/2025 5:25 PM, Dave Jiang wrote:
>> Add a region sysfs attribute to show the size of the extended linear
>> cache if there is any. The attribute is invisible when the cache
>> size is 0, which indicates it does not exist.
>>
>> Moved the cxl_region_visible() location in order to pick up the
>> new sysfs attribute definition.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>> v2:
>> - Add documentation. (Alison)
>> ---
>>  Documentation/ABI/testing/sysfs-bus-cxl |  8 ++++
>>  drivers/cxl/core/region.c               | 63 +++++++++++++++++++------
>>  2 files changed, 56 insertions(+), 15 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
>> index 6b4e8c7a963d..8d529d68dfcd 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-cxl
>> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
>> @@ -498,6 +498,14 @@ Description:
>>  		there is no guarantee that a free followed by an allocate
>>  		results in the same address being allocated.
>>  
>> +What:		/sys/bus/cxl/devices/regionZ/extended_linear_cache_size
>> +Date:		October, 2025
>> +KernelVersion:	v6.19
>> +Contact:	linux-cxl@vger.kernel.org
>> +Description:
>> +		(RO) The size of extended linear cache if there is one present.
> 
> s/if there is one present/if present

ok

> 
>> +		The region 'size' attribute would reflect the total size where
>> +		the CXL region size plus the extended linear cache size.
> 
> Bit messy, maybe: "The region 'size' attribute is the CXL region size plus the extended linear cache size."

ok

> 
> You should probably add a blurb to the size attribute's description about the extended linear cache size.
> Changing the meaning of the size attribute outside its own description seems wrong.
> 
> At that point you could probably drop the second sentence altogether (I'll leave that one up to you though).

I'll add something to 'size' and drop the additional stuff here.

> 
>>  
>>  What:		/sys/bus/cxl/devices/regionZ/mode
>>  Date:		January, 2023
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index b06fee1978ba..531f790b9510 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -461,21 +461,6 @@ static ssize_t commit_show(struct device *dev, struct device_attribute *attr,
>>  }
>>  static DEVICE_ATTR_RW(commit);
>>  
>> -static umode_t cxl_region_visible(struct kobject *kobj, struct attribute *a,
>> -				  int n)
>> -{
>> -	struct device *dev = kobj_to_dev(kobj);
>> -	struct cxl_region *cxlr = to_cxl_region(dev);
>> -
>> -	/*
>> -	 * Support tooling that expects to find a 'uuid' attribute for all
>> -	 * regions regardless of mode.
>> -	 */
>> -	if (a == &dev_attr_uuid.attr && cxlr->mode != CXL_PARTMODE_PMEM)
>> -		return 0444;
>> -	return a->mode;
>> -}
>> -
>>  static ssize_t interleave_ways_show(struct device *dev,
>>  				    struct device_attribute *attr, char *buf)
>>  {
>> @@ -754,6 +739,21 @@ static ssize_t size_show(struct device *dev, struct device_attribute *attr,
>>  }
>>  static DEVICE_ATTR_RW(size);
>>  
>> +static ssize_t extended_linear_cache_size_show(struct device *dev,
>> +					       struct device_attribute *attr,
>> +					       char *buf)
>> +{
>> +	struct cxl_region *cxlr = to_cxl_region(dev);
>> +	struct cxl_region_params *p = &cxlr->params;
>> +	ssize_t rc;
>> +
>> +	ACQUIRE(rwsem_read_intr, rwsem)(&cxl_rwsem.region);
>> +	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &rwsem)))
>> +		return rc;
>> +	return sysfs_emit(buf, "%#llx\n", p->cache_size);
>> +}
>> +static DEVICE_ATTR_RO(extended_linear_cache_size);
>> +
>>  static struct attribute *cxl_region_attrs[] = {
>>  	&dev_attr_uuid.attr,
>>  	&dev_attr_commit.attr,
>> @@ -762,14 +762,44 @@ static struct attribute *cxl_region_attrs[] = {
>>  	&dev_attr_resource.attr,
>>  	&dev_attr_size.attr,
>>  	&dev_attr_mode.attr,
>> +	&dev_attr_extended_linear_cache_size.attr,
>>  	NULL,
>>  };
>>  
>> +static umode_t cxl_region_visible(struct kobject *kobj, struct attribute *a,
>> +				  int n)
>> +{
>> +	struct device *dev = kobj_to_dev(kobj);
>> +	struct cxl_region *cxlr = to_cxl_region(dev);
>> +
>> +	/*
>> +	 * Support tooling that expects to find a 'uuid' attribute for all
>> +	 * regions regardless of mode.
>> +	 */
>> +	if (a == &dev_attr_uuid.attr && cxlr->mode != CXL_PARTMODE_PMEM)
>> +		return 0444;
>> +
>> +	/*
>> +	 * Don't dispaly extended linear cache attribute if there is no
>> +	 * extended linear cache.
>> +	 */
>> +	if (a == &dev_attr_extended_linear_cache_size.attr &&
>> +	    cxlr->params.cache_size == 0)
>> +		return 0;
>> +
>> +	return a->mode;
>> +}
>> +
>>  static const struct attribute_group cxl_region_group = {
>>  	.attrs = cxl_region_attrs,
>>  	.is_visible = cxl_region_visible,
>>  };
>>  
>> +static const struct attribute_group *get_cxl_region_group(void)
>> +{
>> +	return &cxl_region_group;
>> +}
>> +
> 
> Why introduce this function instead of using &cxl_region_group directly? I see there's
> a precedent with the target group functions, but only get_cxl_region_target_group() needs
> the function. The other two (get_cxl_region_accessX_group()) are only ever called after
> the attribute definition and should probably get cleaned up at some point.

Yeah I can drop that.

> 
>>  static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
>>  {
>>  	struct cxl_region_params *p = &cxlr->params;
>> @@ -3478,6 +3508,9 @@ static int __construct_region(struct cxl_region *cxlr,
>>  		dev_warn(cxlmd->dev.parent,
>>  			 "Extended linear cache calculation failed rc:%d\n", rc);
>>  	}
>> +	rc = sysfs_update_group(&cxlr->dev.kobj, get_cxl_region_group());
> 
> Missing whitespace above this line?

yes.

Thanks for the review Ben.

DJ

> 
> Thanks,
> Ben
> 
>> +	if (rc)
>> +		return rc;
>>  
>>  	rc = insert_resource(cxlrd->res, res);
>>  	if (rc) {
>>
>> base-commit: a4bbb493a3247ef32f6191fd8b2a0657139f8e08
>