Re: [PATCH 3/5] cxl/mem : Expose dynamic capacity configuration to userspace

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

On 6/15/23 19:47, Ira Weiny wrote:
> Alison Schofield wrote:
>> On Wed, Jun 14, 2023 at 12:16:30PM -0700, Ira Weiny wrote:
>>> From: Navneet Singh <navneet.singh@intel.com>
>>>
>>> Exposing driver cached dynamic capacity configuration through sysfs
>>> attributes.User will create one or more dynamic capacity
>>> cxl regions based on this information and map the dynamic capacity of
>>> the device into HDM ranges using one or more HDM decoders.
>>>
>>> Signed-off-by: Navneet Singh <navneet.singh@intel.com>
>>>
>>> ---
>>> [iweiny: fixups]
>>> [djbw: fixups, no sign-off: preview only]
>>> ---
>>>   drivers/cxl/core/memdev.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 72 insertions(+)
>>
>> Add the documentation of these new attributes in this patch.
>> Documentation/ABI/testing/sysfs-bus-cxl
> 
> Good point.  And the region creation patch needs some updating for the
> sysfs documentation as well...
> 
> Thanks!  I'll work on those.
> 
> Writing the documentation it seems like 'dc_region_count' should just be
> 'region_count'.  Because the 'dc' is redundant with the directory.
> However, dcY_size has a redundant 'dc' but Y_size (ie 0_size) seems
> odd.[*]
> 
> Thoughts on the 'dc' prefix for these?
> 
> [*] example listing with 2 DC regions supported.
> 
> $ ll mem1/dc/
> total 0
> -r--r--r-- 1 root root 4096 Jun 15 19:26 dc0_size
> -r--r--r-- 1 root root 4096 Jun 15 19:26 dc1_size
> -r--r--r-- 1 root root 4096 Jun 15 19:26 dc_regions_count
> 
>>
>> A bit of my ignorance here, but when I keep seeing the word
>> 'regions' below, it makes me wonder whether these attributes
>> are in the right place?
> 
> There is a difference between 'DC region' and CXL 'Linux' region.  It has
> taken me some time to get used to the terminology.  So I think this is
> correct.

I think you answered your own question above here. If dc_region is 
different than CXL regions, then you'll have to keep the dc_ prefix to 
distinguish between the two. Unless you call it something different.

DJ

> 
>>
>>>
>>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>>> index 5d1ba7a72567..beeb5fa3a0aa 100644
>>> --- a/drivers/cxl/core/memdev.c
>>> +++ b/drivers/cxl/core/memdev.c
>>> @@ -99,6 +99,20 @@ static ssize_t pmem_size_show(struct device *dev, struct device_attribute *attr,
>>>   static struct device_attribute dev_attr_pmem_size =
>>>   	__ATTR(size, 0444, pmem_size_show, NULL);
>>>   
>>> +static ssize_t dc_regions_count_show(struct device *dev, struct device_attribute *attr,
>>> +		char *buf)
>>> +{
>>> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>>> +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
>>> +	int len = 0;
>>> +
>>> +	len = sysfs_emit(buf, "0x%x\n", mds->nr_dc_region);
>>
>> Prefer using this notation: %#llx
>> grep for the sysfs_emit's to see customary usage.
> 
> oh.  I did see this oddity when I was testing and forgot to change this.
> 
> However, I think %#llx needs to be used in show_size_regionN() and this
> needs to be %d.  This is just a count of the number of DC regions
> supported by the device.  I don't think that needs to be in hex.  Changed
> to %d.
> 
>>
>>> +	return len;
>>> +}
>>> +
>>> +struct device_attribute dev_attr_dc_regions_count =
>>> +	__ATTR(dc_regions_count, 0444, dc_regions_count_show, NULL);
>>> +
>>>   static ssize_t serial_show(struct device *dev, struct device_attribute *attr,
>>>   			   char *buf)
>>>   {
>>> @@ -362,6 +376,57 @@ static struct attribute *cxl_memdev_ram_attributes[] = {
>>>   	NULL,
>>>   };
>>>   
>>> +static ssize_t show_size_regionN(struct cxl_memdev *cxlmd, char *buf, int pos)
>>> +{
>>> +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
>>> +
>>> +	return sysfs_emit(buf, "0x%llx\n", mds->dc_region[pos].decode_len);
> 
> ... changed this one to %#llx.
> 
> Ira