Re: [PATCH 03/18] cxl/region: Support empty uuids for non-pmem regions

[Dan Williams <dan.j.williams@intel.com>]

Ira Weiny wrote:
> Dan Williams wrote:
> > Shipping versions of the cxl-cli utility expect all regions to have a
> > 'uuid' attribute. In preparation for 'ram' regions, update the 'uuid'
> > attribute to return an empty string which satisfies the current
> > expectations of 'cxl list -R'. Otherwise, 'cxl list -R' fails in the
> > presence of regions with the 'uuid' attribute missing.
> 
> Would this be more appropriate as a change to cxl-cli?

The point is already shipped cxl-cli can not be changed. So if the
kernel carries this workaround it will carry it forever even if
userspace updates.

Here is an illustration of the different update cadences of
distributions that ship ndctl / cxl-cli:

https://repology.org/project/ndctl/versions

> 
> > Force the
> > attribute to be read-only as there is no facility or expectation for a
> > 'ram' region to recall its uuid from one boot to the next.
> 
> This seems reasonable.
> 
> > 
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-cxl |    3 ++-
> >  drivers/cxl/core/region.c               |    7 +++++--
> >  2 files changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> > index 058b0c45001f..4c4e1cbb1169 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-cxl
> > +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> > @@ -317,7 +317,8 @@ Contact:	linux-cxl@vger.kernel.org
> >  Description:
> >  		(RW) Write a unique identifier for the region. This field must
> >  		be set for persistent regions and it must not conflict with the
> > -		UUID of another region.
> > +		UUID of another region. For volatile ram regions this
> > +		attribute is a read-only empty string.
> >  
> >  
> >  What:		/sys/bus/cxl/devices/regionZ/interleave_granularity
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 17d2d0c12725..c9e7f05caa0f 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -45,7 +45,10 @@ static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
> >  	rc = down_read_interruptible(&cxl_region_rwsem);
> >  	if (rc)
> >  		return rc;
> > -	rc = sysfs_emit(buf, "%pUb\n", &p->uuid);
> 
> I guess it all depends on what p->uuid is...  Shouldn't this be all 0's
> for a ram region?  Does sysfs_emit() choke on that?

...but the uuid isn't all zeros for ram-regions, it does not exist so an
empty string is more appropriate.