Re: [PATCH 16/17] cxl/core: Add cache device attributes

["Cheatham, Benjamin" <benjamin.cheatham@amd.com>]

On 12/17/2025 10:12 AM, Jonathan Cameron wrote:
> On Tue, 11 Nov 2025 15:40:31 -0600
> Ben Cheatham <Benjamin.Cheatham@amd.com> wrote:
> 
>> Add sysfs attributes for getting the numa node, CXL cache unit, and
>> CXL cache size for a cachedev.
>>
>> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
> Documentation is king for ABI.
> 
> Documentation/ABI/testing/sysfs-bus-cxl probably.
> 
>> ---
>>  drivers/cxl/core/cachedev.c | 85 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 85 insertions(+)
>>
>> diff --git a/drivers/cxl/core/cachedev.c b/drivers/cxl/core/cachedev.c
>> index 0b7430450b4e..d8bf18ec0579 100644
>> --- a/drivers/cxl/core/cachedev.c
>> +++ b/drivers/cxl/core/cachedev.c
> 
>> +static ssize_t cache_size_show(struct device *dev, struct device_attribute *attr,
>> +			       char *buf)
>> +{
>> +	struct cxl_cachedev *cxlcd = to_cxl_cachedev(dev);
>> +	struct cxl_dev_state *cxlds = cxlcd->cxlds;
>> +	struct cxl_cache_state cstate = cxlds->cstate;
> 
> Why copy it?  &cxlds->cstate;
> If there is a reason to copy add a comment.

It's a typo, it's supposed to be a pointer. I'll fix for v1.

> 
>> +
>> +	return sysfs_emit(buf, "%llu\n", cstate.size);
> 
>> +}
>> +static DEVICE_ATTR_RO(cache_size);
>> +
>> +static ssize_t cache_unit_show(struct device *dev, struct device_attribute *attr,
>> +			       char *buf)
>> +{
>> +	struct cxl_cachedev *cxlcd = to_cxl_cachedev(dev);
>> +	struct cxl_dev_state *cxlds = cxlcd->cxlds;
>> +	struct cxl_cache_state cstate = cxlds->cstate;
>> +	char unit_buf[32];
>> +	int rc;
>> +
>> +	rc = string_get_size(cstate.size, 1, STRING_UNITS_2, unit_buf,
>> +			     sizeof(unit_buf) - 1);
> 
> This reflects back to comment in earlier patch.  Do we care about unit
> other than as a means to establish the size?  I'm not sure they have
> meaning beyond allowing that register to express a larger range than
> if we did it on 64K always.
> 

I'm not sure. I don't think it matters functionally, but it would probably be useful
for debug of cache devices. I don't think that is a big enough deal to warrant a
sysfs attribute though so I'll remove it (and the cxl_cache_state member). It can
always get added back later as a debugfs attribute when someone wants it.

Thanks,
Ben

>> +	if (rc <= 0)
>> +		return -ENXIO;
>> +
>> +	return sysfs_emit(buf, "%s\n", unit_buf);
>> +}
>> +static DEVICE_ATTR_RO(cache_unit);