[PATCH v3] cxl: Add committed sysfs attribute to CXL decoder

[PATCH v3] cxl: Add committed sysfs attribute to CXL decoder

[Dave Jiang]

This attribute allows cxl-cli to determine whether a decoder is
actively participating in a region. This is only a snapshot of the
state, and doesn't offer any protection or serialization against a
concurrent disable-region operation.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>

---
v3:
- Update commit log verbiage. (Vishal)
- Change code to conform with existing code. (Dan)
v2:
- Use FIELD_GET() (Jonathan)
---
 Documentation/ABI/testing/sysfs-bus-cxl |    7 +++++++
 drivers/cxl/core/port.c                 |   12 ++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
index 087f762ebfd5..ef3fc9fe9d0d 100644
--- a/Documentation/ABI/testing/sysfs-bus-cxl
+++ b/Documentation/ABI/testing/sysfs-bus-cxl
@@ -369,6 +369,13 @@ Description:
 		provided it is currently idle / not bound to a driver.
 
 
+What:		/sys/bus/cxl/devices/decoderX.Y/committed
+Date:		Sep, 2023
+KernelVersion:	v6.7
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		(RO) Indicates whether the decoder is committed.
+
 What:		/sys/bus/cxl/devices/regionZ/uuid
 Date:		May, 2022
 KernelVersion:	v6.0
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 724be8448eb4..20919ffbc1a4 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -277,12 +277,24 @@ static ssize_t interleave_ways_show(struct device *dev,
 
 static DEVICE_ATTR_RO(interleave_ways);
 
+static ssize_t committed_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct cxl_decoder *cxld = to_cxl_decoder(dev);
+	bool enabled = cxld->flags & CXL_DECODER_F_ENABLE;
+
+	return sysfs_emit(buf, "%d\n", enabled);
+}
+
+static DEVICE_ATTR_RO(committed);
+
 static struct attribute *cxl_decoder_base_attrs[] = {
 	&dev_attr_start.attr,
 	&dev_attr_size.attr,
 	&dev_attr_locked.attr,
 	&dev_attr_interleave_granularity.attr,
 	&dev_attr_interleave_ways.attr,
+	&dev_attr_committed.attr,
 	NULL,
 };
 

Re: [PATCH v3] cxl: Add committed sysfs attribute to CXL decoder

[Davidlohr Bueso]

On Tue, 26 Sep 2023, Dave Jiang wrote:

>This attribute allows cxl-cli to determine whether a decoder is
>actively participating in a region. This is only a snapshot of the
>state, and doesn't offer any protection or serialization against a
>concurrent disable-region operation.
>

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

>Suggested-by: Dan Williams <dan.j.williams@intel.com>
>Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
>Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>
>---
>v3:
>- Update commit log verbiage. (Vishal)
>- Change code to conform with existing code. (Dan)
>v2:
>- Use FIELD_GET() (Jonathan)
>---
> Documentation/ABI/testing/sysfs-bus-cxl |    7 +++++++
> drivers/cxl/core/port.c                 |   12 ++++++++++++
> 2 files changed, 19 insertions(+)
>
>diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
>index 087f762ebfd5..ef3fc9fe9d0d 100644
>--- a/Documentation/ABI/testing/sysfs-bus-cxl
>+++ b/Documentation/ABI/testing/sysfs-bus-cxl
>@@ -369,6 +369,13 @@ Description:
>		provided it is currently idle / not bound to a driver.
>
>
>+What:		/sys/bus/cxl/devices/decoderX.Y/committed
>+Date:		Sep, 2023
>+KernelVersion:	v6.7
>+Contact:	linux-cxl@vger.kernel.org
>+Description:
>+		(RO) Indicates whether the decoder is committed.
>+
> What:		/sys/bus/cxl/devices/regionZ/uuid
> Date:		May, 2022
> KernelVersion:	v6.0
>diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>index 724be8448eb4..20919ffbc1a4 100644
>--- a/drivers/cxl/core/port.c
>+++ b/drivers/cxl/core/port.c
>@@ -277,12 +277,24 @@ static ssize_t interleave_ways_show(struct device *dev,
>
> static DEVICE_ATTR_RO(interleave_ways);
>
>+static ssize_t committed_show(struct device *dev,
>+			      struct device_attribute *attr, char *buf)
>+{
>+	struct cxl_decoder *cxld = to_cxl_decoder(dev);
>+	bool enabled = cxld->flags & CXL_DECODER_F_ENABLE;
>+
>+	return sysfs_emit(buf, "%d\n", enabled);
>+}
>+
>+static DEVICE_ATTR_RO(committed);
>+
> static struct attribute *cxl_decoder_base_attrs[] = {
>	&dev_attr_start.attr,
>	&dev_attr_size.attr,
>	&dev_attr_locked.attr,
>	&dev_attr_interleave_granularity.attr,
>	&dev_attr_interleave_ways.attr,
>+	&dev_attr_committed.attr,
>	NULL,
> };
>
>
>

RE: [PATCH v3] cxl: Add committed sysfs attribute to CXL decoder

[Dan Williams]

Dave Jiang wrote:
> This attribute allows cxl-cli to determine whether a decoder is
> actively participating in a region. This is only a snapshot of the
> state, and doesn't offer any protection or serialization against a
> concurrent disable-region operation.

A random thought occurred while realizing that the kernel uses a check
of:

    port->commit_end != -1

...to determine that a given port (switch or endpoint) has committed
decoders. If the goal here is to determine when it is safe to disable an
entire memdev maybe it is better to check all of the decoders at once at
the port level rather than one at a time. It is already the case that
CXL prevents decoders from being committed out of order so what do you
think of replacing decoderX.Y/committed with portX/decoders_committed,
where it just does:

    down_read(&cxl_region_rwsem);
    sysfs_emit("%d\n", port->commit_end + 1);
    up_read(&cxl_region_rwsem);

...and cxl-cli aborts destructive processes on that attribute being
non-zero.

Is there any use case for userspace to check for individual decoders
being committed? It can infer "decoder committed" from "decoder_id <
decoders_commited".

Re: [PATCH v3] cxl: Add committed sysfs attribute to CXL decoder

[Dave Jiang]

On 10/3/23 15:40, Dan Williams wrote:
> Dave Jiang wrote:
>> This attribute allows cxl-cli to determine whether a decoder is
>> actively participating in a region. This is only a snapshot of the
>> state, and doesn't offer any protection or serialization against a
>> concurrent disable-region operation.
> 
> A random thought occurred while realizing that the kernel uses a check
> of:
> 
>     port->commit_end != -1
> 
> ...to determine that a given port (switch or endpoint) has committed
> decoders. If the goal here is to determine when it is safe to disable an
> entire memdev maybe it is better to check all of the decoders at once at
> the port level rather than one at a time. It is already the case that
> CXL prevents decoders from being committed out of order so what do you
> think of replacing decoderX.Y/committed with portX/decoders_committed,
> where it just does:
> 
>     down_read(&cxl_region_rwsem);
>     sysfs_emit("%d\n", port->commit_end + 1);
>     up_read(&cxl_region_rwsem);
> 
> ...and cxl-cli aborts destructive processes on that attribute being
> non-zero.

Seems reasonable. I'll rework the patch and the cxl cli code. 

> 
> Is there any use case for userspace to check for individual decoders
> being committed? It can infer "decoder committed" from "decoder_id <
> decoders_commited".