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

[Ira Weiny <ira.weiny@intel.com>]

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.

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