| Message ID | 167564536587.847146.12703125206459604597.stgit@dwillia2-xfh.jf.intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | CXL RAM and the 'Soft Reserved' => 'System RAM' default | expand |
On Sun, 05 Feb 2023 17:02:45 -0800 Dan Williams <dan.j.williams@intel.com> 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. 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 is new functionality, so do we need the backwards compatibility? Or does that cxl list -R not just skip these, but fail hard? It's inelegant to have this visible at all (and it confused me when I was running tests as I assumed I needed to poke something in it). If nothing else, good to have a comment to remind us in the code why there is the oddity of setting it to read only. Jonathan > > 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); > + if (cxlr->mode != CXL_DECODER_PMEM) > + rc = sysfs_emit(buf, "\n"); > + else > + rc = sysfs_emit(buf, "%pUb\n", &p->uuid); > up_read(&cxl_region_rwsem); > > return rc; > @@ -301,7 +304,7 @@ static umode_t cxl_region_visible(struct kobject *kobj, struct attribute *a, > struct cxl_region *cxlr = to_cxl_region(dev); > > if (a == &dev_attr_uuid.attr && cxlr->mode != CXL_DECODER_PMEM) > - return 0; > + return 0444; Definitely a comment here as this is very odd and we may forget why it is like this. > return a->mode; > } > >
Jonathan Cameron wrote: > On Sun, 05 Feb 2023 17:02:45 -0800 > Dan Williams <dan.j.williams@intel.com> 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. 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 is new functionality, so do we need the backwards compatibility? > Or does that cxl list -R not just skip these, but fail hard? It does this: # cxl list -R libcxl: __sysfs_device_parse: region4: add_dev() failed Warning: no matching devices found [ ] ...which in hindsight is not a helpful message. However, as an end user I hate forced upgrades, so a small kernel behavior change to extend the useful life of an installed tool is worthwhile. > It's inelegant to have this visible at all (and it confused me when > I was running tests as I assumed I needed to poke something in it). It is RO to indicate that, but I do notice I neglected to update Documentation/ABI/testing/sysfs-bus-cxl with this behavior. > > If nothing else, good to have a comment to remind us in the code > why there is the oddity of setting it to read only. Sure, I'll add that too.
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? > 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? Ira
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.
On Sun, 2023-02-05 at 17:02 -0800, 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. 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. > > 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(-) Looks good, Reviewed-by: Vishal Verma <vishal.l.verma@intel.com> > > 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); > + if (cxlr->mode != CXL_DECODER_PMEM) > + rc = sysfs_emit(buf, "\n"); > + else > + rc = sysfs_emit(buf, "%pUb\n", &p->uuid); > up_read(&cxl_region_rwsem); > > return rc; > @@ -301,7 +304,7 @@ static umode_t cxl_region_visible(struct kobject *kobj, struct attribute *a, > struct cxl_region *cxlr = to_cxl_region(dev); > > if (a == &dev_attr_uuid.attr && cxlr->mode != CXL_DECODER_PMEM) > - return 0; > + return 0444; > return a->mode; > } > > >
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); + if (cxlr->mode != CXL_DECODER_PMEM) + rc = sysfs_emit(buf, "\n"); + else + rc = sysfs_emit(buf, "%pUb\n", &p->uuid); up_read(&cxl_region_rwsem); return rc; @@ -301,7 +304,7 @@ static umode_t cxl_region_visible(struct kobject *kobj, struct attribute *a, struct cxl_region *cxlr = to_cxl_region(dev); if (a == &dev_attr_uuid.attr && cxlr->mode != CXL_DECODER_PMEM) - return 0; + return 0444; return a->mode; }
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. 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. 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(-)