[RFC PATCH 0/6] cxl: Add support for CXL feature commands, CXL device patrol scrub control and DDR5 ECS control features

[RFC PATCH 0/6] cxl: Add support for CXL feature commands, CXL device patrol scrub control and DDR5 ECS control features

[shiju.jose]

From: Shiju Jose <shiju.jose@huawei.com>

1. Add support for CXL feature commands(CXL spec 3.0 section 8.2.9.6).
2. Add CXL device scrub driver supporting patrol scrub control feature
(CXL spec 3.1 section 8.2.9.9.11.1) and DDR5 ECS feature(CXL spec 3.1
section 8.2.9.9.11.2).
3. Add scrub attributes for DDR5 ECS control to the memory scrub driver.

The attributes for the CXL scrub control features is exposed to the user
in the sysfs and is based on the previous code submitted for the memory
scrub control feature.
The patch and series for supporting memory scrub control feature are here,
https://lore.kernel.org/lkml/20230915172818.761-2-shiju.jose@huawei.com/ 
https://lore.kernel.org/lkml/20230915172818.761-1-shiju.jose@huawei.com/

The QEMU series to support these features is available here,
https://lore.kernel.org/qemu-devel/20231114124711.1128-1-shiju.jose@huawei.com/T/#t

Shiju Jose (6):
  cxl/mbox: Add GET_SUPPORTED_FEATURES, GET_FEATURE and SET_FEATURE
    mailbox commands
  cxl/memscrub: Add CXL device patrol scrub control feature
  memory: scrub: Add function to show scrub attributes in decimal
  memory: scrub: Add scrub control attributes for the DDR5 ECS
  cxl/memscrub: Add CXL device DDR5 ECS control feature
  cxl: scrub: sysfs: Add Documentation for CXL memory device scrub
    control attributes

 .../testing/sysfs-class-cxl-scrub-configure   |  135 +++
 drivers/cxl/Kconfig                           |   23 +
 drivers/cxl/core/Makefile                     |    1 +
 drivers/cxl/core/mbox.c                       |   74 ++
 drivers/cxl/core/memscrub.c                   | 1008 +++++++++++++++++
 drivers/cxl/cxlmem.h                          |  104 ++
 drivers/cxl/pci.c                             |    8 +
 drivers/memory/scrub/memory-scrub.c           |   36 +-
 include/memory/memory-scrub.h                 |   11 +
 include/uapi/linux/cxl_mem.h                  |    3 +
 10 files changed, 1400 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-cxl-scrub-configure
 create mode 100644 drivers/cxl/core/memscrub.c

-- 
2.34.1

[RFC PATCH 1/6] cxl/mbox: Add GET_SUPPORTED_FEATURES, GET_FEATURE and SET_FEATURE mailbox commands

[shiju.jose]

From: Shiju Jose <shiju.jose@huawei.com>

Add support for GET_SUPPORTED_FEATURES, GET_FEATURE and SET_FEATURE mailbox
commands.

CXL spec 3.0 section 8.2.9.6 describes optional device specific features.
CXL devices supports features with changeable attributes.
Get Supported Features retrieves the list of supported device specific
features. The settings of a feature can be retrieved using Get
Feature and optionally modified using Set Feature.

Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
---
 drivers/cxl/core/mbox.c      | 74 ++++++++++++++++++++++++++++
 drivers/cxl/cxlmem.h         | 95 ++++++++++++++++++++++++++++++++++++
 include/uapi/linux/cxl_mem.h |  3 ++
 3 files changed, 172 insertions(+)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 36270dcfb42e..2a6d8ab927bd 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -63,6 +63,9 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
 	CXL_CMD(GET_SHUTDOWN_STATE, 0, 0x1, 0),
 	CXL_CMD(SET_SHUTDOWN_STATE, 0x1, 0, 0),
 	CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0),
+	CXL_CMD(GET_SUPPORTED_FEATURES, 0x8, CXL_VARIABLE_PAYLOAD, 0),
+	CXL_CMD(GET_FEATURE, 0x15, CXL_VARIABLE_PAYLOAD, 0),
+	CXL_CMD(SET_FEATURE, CXL_VARIABLE_PAYLOAD, 0, 0),
 };
 
 /*
@@ -1303,6 +1306,77 @@ int cxl_set_timestamp(struct cxl_memdev_state *mds)
 }
 EXPORT_SYMBOL_NS_GPL(cxl_set_timestamp, CXL);
 
+struct cxl_mbox_get_supp_feats_out *cxl_get_supported_features(struct cxl_memdev_state *mds,
+						struct cxl_mbox_get_supp_feats_in *pi)
+{
+	int rc;
+	struct cxl_mbox_cmd mbox_cmd;
+	struct cxl_mbox_get_supp_feats_out *feats_out;
+
+	feats_out = kvmalloc(pi->count, GFP_KERNEL);
+	if (!feats_out)
+		return ERR_PTR(-ENOMEM);
+
+	mbox_cmd = (struct cxl_mbox_cmd) {
+		.opcode = CXL_MBOX_OP_GET_SUPPORTED_FEATURES,
+		.size_in = sizeof(*pi),
+		.payload_in = pi,
+		.size_out = pi->count,
+		.payload_out = feats_out,
+		.min_out = sizeof(struct cxl_mbox_get_supp_feats_out),
+	};
+	rc = cxl_internal_send_cmd(mds, &mbox_cmd);
+	if (rc < 0) {
+		kvfree(feats_out);
+		return ERR_PTR(rc);
+	}
+
+	return feats_out;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_get_supported_features, CXL);
+
+void *cxl_get_feature(struct cxl_memdev_state *mds, struct cxl_mbox_get_feat_in *pi)
+{
+	int rc;
+	void *feat_out;
+	struct cxl_mbox_cmd mbox_cmd;
+
+	feat_out = kvmalloc(pi->count, GFP_KERNEL);
+	if (!feat_out)
+		return ERR_PTR(-ENOMEM);
+
+	mbox_cmd = (struct cxl_mbox_cmd) {
+		.opcode = CXL_MBOX_OP_GET_FEATURE,
+		.size_in = sizeof(*pi),
+		.payload_in = pi,
+		.size_out = pi->count,
+		.payload_out = feat_out,
+		.min_out = pi->count,
+	};
+	rc = cxl_internal_send_cmd(mds, &mbox_cmd);
+	if (rc < 0) {
+		kvfree(feat_out);
+		return ERR_PTR(rc);
+	}
+
+	return feat_out;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_get_feature, CXL);
+
+int cxl_set_feature(struct cxl_memdev_state *mds, void *feat_in, size_t size)
+{
+	struct cxl_mbox_cmd mbox_cmd;
+
+	mbox_cmd = (struct cxl_mbox_cmd) {
+		.opcode = CXL_MBOX_OP_SET_FEATURE,
+		.size_in = size,
+		.payload_in = feat_in,
+	};
+
+	return cxl_internal_send_cmd(mds, &mbox_cmd);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_set_feature, CXL);
+
 int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
 		       struct cxl_region *cxlr)
 {
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index a2fcbca253f3..fdac686560d4 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -506,6 +506,9 @@ enum cxl_opcode {
 	CXL_MBOX_OP_SET_TIMESTAMP	= 0x0301,
 	CXL_MBOX_OP_GET_SUPPORTED_LOGS	= 0x0400,
 	CXL_MBOX_OP_GET_LOG		= 0x0401,
+	CXL_MBOX_OP_GET_SUPPORTED_FEATURES	= 0x0500,
+	CXL_MBOX_OP_GET_FEATURE		= 0x0501,
+	CXL_MBOX_OP_SET_FEATURE		= 0x0502,
 	CXL_MBOX_OP_IDENTIFY		= 0x4000,
 	CXL_MBOX_OP_GET_PARTITION_INFO	= 0x4100,
 	CXL_MBOX_OP_SET_PARTITION_INFO	= 0x4101,
@@ -740,6 +743,94 @@ struct cxl_mbox_set_timestamp_in {
 
 } __packed;
 
+/* Get Supported Features CXL 3.0 Spec 8.2.9.6.1 */
+/*
+ * Get Supported Features input payload
+ * CXL rev 3.0 section 8.2.9.6.1; Table 8-75
+ */
+struct cxl_mbox_get_supp_feats_in {
+	__le32 count;
+	__le16 start_index;
+	u16 reserved;
+} __packed;
+
+/*
+ * Get Supported Features Supported Feature Entry
+ * CXL rev 3.0 section 8.2.9.6.1; Table 8-77
+ */
+/* Supported Feature Entry : Payload out attribute flags */
+#define CXL_FEAT_ENTRY_FLAG_CHANGABLE	BIT(0)
+#define CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_MASK	GENMASK(3, 1)
+#define CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_NONE	0x0
+#define CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_CXL_RESET	0x1
+#define CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_HOT_RESET	0x2
+#define CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_WARM_RESET	0x3
+#define CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_COLD_RESET	0x4
+#define CXL_FEAT_ENTRY_FLAG_PERSISTENCE_ACROSS_FW_UPDATE_MASK	BIT(4)
+#define CXL_FEAT_ENTRY_FLAG_PERSISTENCE_DEFAULT_SEL_SUPPORT_MASK	BIT(5)
+#define CXL_FEAT_ENTRY_FLAG_PERSISTENCE_SAVED_SEL_SUPPORT_MASK	BIT(6)
+
+struct cxl_mbox_supp_feat_entry {
+	uuid_t uuid;
+	__le16 feat_index;
+	__le16 get_feat_size;
+	__le16 set_feat_size;
+	__le32 attrb_flags;
+	u8 get_feat_version;
+	u8 set_feat_version;
+	__le16 set_feat_effects;
+	u8 rsvd[18];
+}  __packed;
+
+/*
+ * Get Supported Features output payload
+ * CXL rev 3.0 section 8.2.9.6.1; Table 8-76
+ */
+struct cxl_mbox_get_supp_feats_out {
+	__le16 entries;
+	__le16 nsuppfeats_dev;
+	u32 reserved;
+	struct cxl_mbox_supp_feat_entry feat_entries[];
+} __packed;
+
+/* Get Feature CXL 3.0 Spec 8.2.9.6.2 */
+/*
+ * Get Feature input payload
+ * CXL rev 3.0 section 8.2.9.6.2; Table 8-79
+ */
+/* Get Feature : Payload in selection */
+#define CXL_GET_FEAT_CURRENT_VALUE	0x00
+#define CXL_GET_FEAT_DEFAULT_VALUE	0x01
+#define CXL_GET_FEAT_SAVED_VALUE	0x02
+
+struct cxl_mbox_get_feat_in {
+	uuid_t uuid;
+	__le16 offset;
+	__le16 count;
+	u8 selection;
+}  __packed;
+
+/* Set Feature CXL 3.0 Spec 8.2.9.6.3 */
+/*
+ * Set Feature input payload
+ * CXL rev 3.0 section 8.2.9.6.3; Table 8-81
+ */
+/* Set Feature : Payload in flags */
+#define CXL_SET_FEAT_FLAG_ACTION_MASK	GENMASK(2, 0)
+#define CXL_SET_FEAT_FLAG_ACTION_FULL_DATA_TRANSFER	0x0
+#define CXL_SET_FEAT_FLAG_ACTION_INITIATE_DATA_TRANSFER	0x1
+#define CXL_SET_FEAT_FLAG_ACTION_CONTINUE_DATA_TRANSFER	0x2
+#define CXL_SET_FEAT_FLAG_ACTION_FINISH_DATA_TRANSFER	0x3
+#define CXL_SET_FEAT_FLAG_ACTION_ABORT_DATA_TRANSFER	0x4
+#define CXL_SET_FEAT_FLAG_MOD_VALUE_SAVED_ACROSS_RESET	BIT(3)
+struct cxl_mbox_set_feat_in {
+	uuid_t uuid;
+	__le32 flags;
+	__le16 offset;
+	u8 version;
+	u8 rsvd[9];
+}  __packed;
+
 /* Get Poison List  CXL 3.0 Spec 8.2.9.8.4.1 */
 struct cxl_mbox_poison_in {
 	__le64 offset;
@@ -867,6 +958,10 @@ void clear_exclusive_cxl_commands(struct cxl_memdev_state *mds,
 				  unsigned long *cmds);
 void cxl_mem_get_event_records(struct cxl_memdev_state *mds, u32 status);
 int cxl_set_timestamp(struct cxl_memdev_state *mds);
+struct cxl_mbox_get_supp_feats_out *cxl_get_supported_features(struct cxl_memdev_state *mds,
+						struct cxl_mbox_get_supp_feats_in *pi);
+void *cxl_get_feature(struct cxl_memdev_state *mds, struct cxl_mbox_get_feat_in *pi);
+int cxl_set_feature(struct cxl_memdev_state *mds, void *feat_in, size_t size);
 int cxl_poison_state_init(struct cxl_memdev_state *mds);
 int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
 		       struct cxl_region *cxlr);
diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
index 14bc6e742148..8c89d323cc41 100644
--- a/include/uapi/linux/cxl_mem.h
+++ b/include/uapi/linux/cxl_mem.h
@@ -46,6 +46,9 @@
 	___C(GET_SCAN_MEDIA_CAPS, "Get Scan Media Capabilities"),         \
 	___DEPRECATED(SCAN_MEDIA, "Scan Media"),                          \
 	___DEPRECATED(GET_SCAN_MEDIA, "Get Scan Media Results"),          \
+	___C(GET_SUPPORTED_FEATURES, "Get Supported Features"),           \
+	___C(GET_FEATURE, "Get Feature"),                                 \
+	___C(SET_FEATURE, "Set Feature"),                                 \
 	___C(MAX, "invalid / last command")
 
 #define ___C(a, b) CXL_MEM_COMMAND_ID_##a
-- 
2.34.1

[RFC PATCH 2/6] cxl/memscrub: Add CXL device patrol scrub control feature

[shiju.jose]

From: Shiju Jose <shiju.jose@huawei.com>

CXL spec 3.1 section 8.2.9.9.11.1 describes the device patrol scrub control
feature. The device patrol scrub proactively locates and makes corrections
to errors in regular cycle. The patrol scrub control allows the request to
configure patrol scrub input configurations.

The patrol scrub control allows the requester to specify the number of
hours for which the patrol scrub cycles must be completed, provided that
the requested number is not less than the minimum number of hours for the
patrol scrub cycle that the device is capable of. In addition, the patrol
scrub controls allow the host to disable and enable the feature in case
disabling of the feature is needed for other purposes such as
performance-aware operations which require the background operations to be
turned off.

Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
---
 drivers/cxl/Kconfig         |  23 ++
 drivers/cxl/core/Makefile   |   1 +
 drivers/cxl/core/memscrub.c | 455 ++++++++++++++++++++++++++++++++++++
 drivers/cxl/cxlmem.h        |   7 +
 drivers/cxl/pci.c           |   6 +
 5 files changed, 492 insertions(+)
 create mode 100644 drivers/cxl/core/memscrub.c

diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
index 8ea1d340e438..45ee6d57d899 100644
--- a/drivers/cxl/Kconfig
+++ b/drivers/cxl/Kconfig
@@ -154,4 +154,27 @@ config CXL_PMU
 	  monitoring units and provide standard perf based interfaces.
 
 	  If unsure say 'm'.
+
+config CXL_SCRUB
+	tristate "CXL: Memory scrub feature"
+	depends on CXL_PCI
+	depends on CXL_MEM
+	depends on SCRUB
+	help
+	  The CXL memory scrub control is an optional feature allows host to
+	  control the scrub configurations of CXL Type 3 devices, which
+	  support patrol scrub and/or DDR5 ECS(Error Check Scrub).
+
+	  Register with the scrub configure driver to provide sysfs interfaces
+	  for configuring the CXL device memory patrol and DDR5 ECS scrubs.
+	  Provides the interface functions support configuring the CXL memory
+	  device patrol and ECS scrubs.
+
+	  Say 'y/m' to enable the CXL memory scrub driver that will attach to
+	  CXL.mem devices for memory scrub control feature. See sections
+	  8.2.9.9.11.1 and 8.2.9.9.11.2 in the CXL 3.1 specification for a
+	  detailed description of CXL memory scrub control features.
+
+	  If unsure say 'm'.
+
 endif
diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
index 1f66b5d4d935..99e3202f868f 100644
--- a/drivers/cxl/core/Makefile
+++ b/drivers/cxl/core/Makefile
@@ -15,3 +15,4 @@ cxl_core-y += hdm.o
 cxl_core-y += pmu.o
 cxl_core-$(CONFIG_TRACING) += trace.o
 cxl_core-$(CONFIG_CXL_REGION) += region.o
+cxl_core-$(CONFIG_CXL_SCRUB) += memscrub.o
diff --git a/drivers/cxl/core/memscrub.c b/drivers/cxl/core/memscrub.c
new file mode 100644
index 000000000000..ec67ffc81363
--- /dev/null
+++ b/drivers/cxl/core/memscrub.c
@@ -0,0 +1,455 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * cxl_memscrub.c - CXL memory scrub driver
+ *
+ * Copyright (c) 2023 HiSilicon Limited.
+ *
+ *  - Provides functions to configure patrol scrub
+ *    feature of the CXL memory devices.
+ *  - Registers with the scrub driver for the
+ *    memory patrol scrub feature.
+ */
+
+#define pr_fmt(fmt)	"CXL_MEM_SCRUB: " fmt
+
+#include <cxlmem.h>
+#include <memory/memory-scrub.h>
+
+/* CXL memory scrub feature common definitions */
+#define CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH	128
+#define CXL_MEMDEV_MAX_NAME_LENGTH	128
+
+static int cxl_mem_get_supported_feature_entry(struct cxl_memdev *cxlmd, const uuid_t *feat_uuid,
+					       struct cxl_mbox_supp_feat_entry *feat_entry_out)
+{
+	int nentries; /* number of supported feature entries in output payload */
+	int feat_index, count;
+	bool is_support_feature = false;
+	struct cxl_mbox_get_supp_feats_in pi;
+	struct cxl_mbox_supp_feat_entry *feat_entry;
+	struct cxl_mbox_get_supp_feats_out *feats_out;
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
+	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
+
+	feat_index = 0;
+	do {
+		pi.count = sizeof(struct cxl_mbox_get_supp_feats_out) +
+				  sizeof(struct cxl_mbox_supp_feat_entry);
+		pi.start_index = feat_index;
+		nentries = 0;
+		feats_out = cxl_get_supported_features(mds, &pi);
+		if (PTR_ERR_OR_ZERO(feats_out))
+			return  PTR_ERR_OR_ZERO(feats_out);
+		nentries = feats_out->entries;
+		if (!nentries) {
+			kvfree(feats_out);
+			break;
+		}
+		/* Check CXL memdev supports the feature */
+		feat_entry = (void *)feats_out->feat_entries;
+		for (count = 0; count < nentries; count++, feat_entry++) {
+			if (uuid_equal(&feat_entry->uuid, feat_uuid)) {
+				is_support_feature = true;
+				memcpy(feat_entry_out, feat_entry, sizeof(*feat_entry_out));
+				break;
+			}
+		}
+		kvfree(feats_out);
+		if (is_support_feature)
+			break;
+		feat_index += nentries;
+	} while (nentries);
+
+	if (!is_support_feature)
+		return -ENOTSUPP;
+
+	return 0;
+}
+
+/* CXL memory patrol scrub control definitions */
+#define CXL_MEMDEV_PS_GET_FEAT_VERSION	0x01
+#define CXL_MEMDEV_PS_SET_FEAT_VERSION	0x01
+
+#define CXL_PATROL_SCRUB	"cxl_patrol_scrub"
+
+/* The default number of regions for CXL memory device patrol scrubber
+ * Patrol scrub is a feature where the device controller scrubs the
+ * memory at a regular interval accroding to the CXL specification.
+ * Hence the number of memory regions to scrub assosiated to the patrol
+ * scrub is 1.
+ */
+#define CXL_MEMDEV_PATROL_SCRUB_NUM_REGIONS	1
+
+static const uuid_t cxl_patrol_scrub_uuid =
+	UUID_INIT(0x96dad7d6, 0xfde8, 0x482b, 0xa7, 0x33, 0x75, 0x77, 0x4e,     \
+		  0x06, 0xdb, 0x8a);
+
+/* CXL memory patrol scrub control functions */
+struct cxl_patrol_scrub_context {
+	struct device *dev;
+	u16 get_feat_size;
+	u16 set_feat_size;
+	bool scrub_cycle_changable;
+};
+
+/**
+ * struct cxl_memdev_ps_params - CXL memory patrol scrub parameter data structure.
+ * @enable:     [IN] enable(1)/disable(0) patrol scrub.
+ * @scrub_cycle_changable: [OUT] scrub cycle attribute of patrol scrub is changeable.
+ * @speed:      [IN] Requested patrol scrub cycle in hours.
+ *              [OUT] Current patrol scrub cycle in hours.
+ * @min_speed:[OUT] minimum patrol scrub cycle, in hours, supported.
+ * @speed_avail:[OUT] Supported patrol scrub cycle in hours.
+ */
+struct cxl_memdev_ps_params {
+	bool enable;
+	bool scrub_cycle_changable;
+	u16 speed;
+	u16 min_speed;
+	char speed_avail[CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH];
+};
+
+enum {
+	CXL_MEMDEV_PS_PARAM_ENABLE = 0,
+	CXL_MEMDEV_PS_PARAM_SPEED,
+};
+
+#define	CXL_MEMDEV_PS_SCRUB_CYCLE_CHANGE_CAP_MASK	BIT(0)
+#define	CXL_MEMDEV_PS_SCRUB_CYCLE_REALTIME_REPORT_CAP_MASK	BIT(1)
+#define	CXL_MEMDEV_PS_CUR_SCRUB_CYCLE_MASK	GENMASK(7, 0)
+#define	CXL_MEMDEV_PS_MIN_SCRUB_CYCLE_MASK	GENMASK(15, 8)
+#define	CXL_MEMDEV_PS_FLAG_ENABLED_MASK	BIT(0)
+
+struct cxl_memdev_ps_feat_read_attrbs {
+	u8 scrub_cycle_cap;
+	__le16 scrub_cycle;
+	u8 scrub_flags;
+}  __packed;
+
+struct cxl_memdev_ps_set_feat_pi {
+	struct cxl_mbox_set_feat_in pi;
+	u8 scrub_cycle_hr;
+	u8 scrub_flags;
+}  __packed;
+
+static int cxl_mem_ps_get_attrbs(struct device *dev,
+				 struct cxl_memdev_ps_params *params)
+{
+	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
+	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
+	struct cxl_mbox_get_feat_in pi = {
+		.uuid = cxl_patrol_scrub_uuid,
+		.offset = 0,
+		.count = sizeof(struct cxl_memdev_ps_feat_read_attrbs),
+		.selection = CXL_GET_FEAT_CURRENT_VALUE,
+	};
+	struct cxl_memdev_ps_feat_read_attrbs *rd_attrbs;
+
+	if (!mds)
+		return -EFAULT;
+
+	rd_attrbs = cxl_get_feature(mds, &pi);
+	if (PTR_ERR_OR_ZERO(rd_attrbs)) {
+		params->enable = 0;
+		params->speed = 0;
+		snprintf(params->speed_avail, CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH,
+			"Unavailable");
+		return PTR_ERR_OR_ZERO(rd_attrbs);
+	}
+	params->scrub_cycle_changable = FIELD_GET(CXL_MEMDEV_PS_SCRUB_CYCLE_CHANGE_CAP_MASK,
+						  rd_attrbs->scrub_cycle_cap);
+	params->enable = FIELD_GET(CXL_MEMDEV_PS_FLAG_ENABLED_MASK,
+				   rd_attrbs->scrub_flags);
+	params->speed = FIELD_GET(CXL_MEMDEV_PS_CUR_SCRUB_CYCLE_MASK,
+				  rd_attrbs->scrub_cycle);
+	params->min_speed  = FIELD_GET(CXL_MEMDEV_PS_MIN_SCRUB_CYCLE_MASK,
+				       rd_attrbs->scrub_cycle);
+	snprintf(params->speed_avail, CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH,
+		 "Minimum scrub cycle = %d hour", params->min_speed);
+	kvfree(rd_attrbs);
+
+	return 0;
+}
+
+static int cxl_mem_ps_set_attrbs(struct device *dev,
+				 struct cxl_memdev_ps_params *params, u8 param_type)
+{
+	int ret;
+	struct cxl_memdev_ps_params rd_params;
+	struct cxl_memdev_ps_set_feat_pi set_pi = {
+		.pi.uuid = cxl_patrol_scrub_uuid,
+		.pi.flags = CXL_SET_FEAT_FLAG_MOD_VALUE_SAVED_ACROSS_RESET |
+			    CXL_SET_FEAT_FLAG_ACTION_FULL_DATA_TRANSFER,
+		.pi.offset = 0,
+		.pi.version = CXL_MEMDEV_PS_SET_FEAT_VERSION,
+	};
+	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
+	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
+
+	if (!mds)
+		return -EFAULT;
+
+	ret = 0;
+	ret = cxl_mem_ps_get_attrbs(dev, &rd_params);
+	if (ret) {
+		dev_err(dev, "Get cxlmemdev patrol scrub params fail ret=%d\n",
+			ret);
+		return ret;
+	}
+
+	if (param_type == CXL_MEMDEV_PS_PARAM_ENABLE) {
+		set_pi.scrub_flags = FIELD_PREP(CXL_MEMDEV_PS_FLAG_ENABLED_MASK,
+						   params->enable);
+		set_pi.scrub_cycle_hr = FIELD_PREP(CXL_MEMDEV_PS_CUR_SCRUB_CYCLE_MASK,
+						      rd_params.speed);
+	} else if (param_type == CXL_MEMDEV_PS_PARAM_SPEED) {
+		if (params->speed < rd_params.min_speed) {
+			dev_err(dev, "Invalid CXL patrol scrub cycle(%d) to set\n",
+				params->speed);
+			dev_err(dev, "Minimum supported CXL patrol scrub cycle in hour %d\n",
+			       params->min_speed);
+			return -EINVAL;
+		}
+		set_pi.scrub_cycle_hr = FIELD_PREP(CXL_MEMDEV_PS_CUR_SCRUB_CYCLE_MASK,
+						      params->speed);
+		set_pi.scrub_flags = FIELD_PREP(CXL_MEMDEV_PS_FLAG_ENABLED_MASK,
+						   rd_params.enable);
+	} else {
+		dev_err(dev, "Invalid CXL patrol scrub parameter to set\n");
+		return -EINVAL;
+	}
+
+	ret = 0;
+	ret = cxl_set_feature(mds, &set_pi, sizeof(set_pi));
+	if (ret) {
+		dev_err(dev, "CXL patrol scrub set feature fail ret=%d\n",
+			ret);
+		return ret;
+	}
+
+	/* Verify attribute set successfully */
+	if (param_type == CXL_MEMDEV_PS_PARAM_SPEED) {
+		ret = cxl_mem_ps_get_attrbs(dev, &rd_params);
+		if (ret) {
+			dev_err(dev, "Get cxlmemdev patrol scrub params fail ret=%d\n", ret);
+			return ret;
+		}
+		if (rd_params.speed != params->speed)
+			return -EFAULT;
+	}
+
+	return 0;
+}
+
+static int cxl_mem_ps_enable_write(struct device *dev, long val)
+{
+	int ret;
+	struct cxl_memdev_ps_params params;
+
+	params.enable = val;
+	ret = cxl_mem_ps_set_attrbs(dev, &params, CXL_MEMDEV_PS_PARAM_ENABLE);
+	if (ret) {
+		dev_err(dev, "CXL patrol scrub enable fail, enable=%d ret=%d\n",
+		       params.enable, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int cxl_mem_ps_speed_read(struct device *dev, u64 *val)
+{
+	int ret;
+	struct cxl_memdev_ps_params params;
+
+	ret = cxl_mem_ps_get_attrbs(dev, &params);
+	if (ret) {
+		dev_err(dev, "Get CXL patrol scrub params fail ret=%d\n",
+			ret);
+		return ret;
+	}
+	*val = params.speed;
+
+	return 0;
+}
+
+static int cxl_mem_ps_speed_write(struct device *dev, long val)
+{
+	int ret;
+	struct cxl_memdev_ps_params params;
+
+	params.speed = val;
+	ret = cxl_mem_ps_set_attrbs(dev, &params, CXL_MEMDEV_PS_PARAM_SPEED);
+	if (ret) {
+		dev_err(dev, "Set CXL patrol scrub params for speed fail ret=%d\n",
+			ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int cxl_mem_ps_speed_available_read(struct device *dev, char *buf)
+{
+	int ret;
+	struct cxl_memdev_ps_params params;
+
+	ret = cxl_mem_ps_get_attrbs(dev, &params);
+	if (ret) {
+		dev_err(dev, "Get CXL patrol scrub params fail ret=%d\n",
+			ret);
+		return ret;
+	}
+
+	sprintf(buf, "%s\n", params.speed_avail);
+
+	return 0;
+}
+
+/**
+ * cxl_mem_patrol_scrub_is_visible() - Callback to return attribute visibility
+ * @drv_data: Pointer to driver-private data structure passed
+ *	      as argument to devm_scrub_device_register().
+ * @attr: Scrub attribute
+ * @region_id: ID of the memory region
+ *
+ * Returns: 0 on success, an error otherwise
+ */
+umode_t cxl_mem_patrol_scrub_is_visible(const void *drv_data, u32 attr, int region_id)
+{
+	const struct cxl_patrol_scrub_context *cxl_ps_ctx = drv_data;
+
+	if (attr == scrub_speed_available ||
+	    attr == scrub_speed) {
+		if (!cxl_ps_ctx->scrub_cycle_changable)
+			return 0;
+	}
+
+	switch (attr) {
+	case scrub_speed_available:
+		return 0444;
+	case scrub_enable:
+		return 0200;
+	case scrub_speed:
+		return 0644;
+	default:
+		return 0;
+	}
+}
+
+/**
+ * cxl_mem_patrol_scrub_read() - Read callback for data attributes
+ * @dev: Pointer to scrub device
+ * @attr: Scrub attribute
+ * @region_id: ID of the memory region
+ * @val: Pointer to the returned data
+ *
+ * Returns: 0 on success, an error otherwise
+ */
+int cxl_mem_patrol_scrub_read(struct device *dev, u32 attr, int region_id, u64 *val)
+{
+
+	switch (attr) {
+	case scrub_speed:
+		return cxl_mem_ps_speed_read(dev->parent, val);
+	default:
+		return -ENOTSUPP;
+	}
+}
+
+/**
+ * cxl_mem_patrol_scrub_write() - Write callback for data attributes
+ * @dev: Pointer to scrub device
+ * @attr: Scrub attribute
+ * @region_id: ID of the memory region
+ * @val: Value to write
+ *
+ * Returns: 0 on success, an error otherwise
+ */
+int cxl_mem_patrol_scrub_write(struct device *dev, u32 attr, int region_id, u64 val)
+{
+	switch (attr) {
+	case scrub_enable:
+		return cxl_mem_ps_enable_write(dev->parent, val);
+	case scrub_speed:
+		return cxl_mem_ps_speed_write(dev->parent, val);
+	default:
+		return -ENOTSUPP;
+	}
+}
+
+/**
+ * cxl_mem_patrol_scrub_read_strings() - Read callback for string attributes
+ * @dev: Pointer to scrub device
+ * @attr: Scrub attribute
+ * @region_id: ID of the memory region
+ * @buf: Pointer to the buffer for copying returned string
+ *
+ * Returns: 0 on success, an error otherwise
+ */
+int cxl_mem_patrol_scrub_read_strings(struct device *dev, u32 attr, int region_id,
+				      char *buf)
+{
+	switch (attr) {
+	case scrub_speed_available:
+		return cxl_mem_ps_speed_available_read(dev->parent, buf);
+	default:
+		return -ENOTSUPP;
+	}
+}
+
+static const struct scrub_ops cxl_ps_scrub_ops = {
+	.is_visible = cxl_mem_patrol_scrub_is_visible,
+	.read = cxl_mem_patrol_scrub_read,
+	.write = cxl_mem_patrol_scrub_write,
+	.read_string = cxl_mem_patrol_scrub_read_strings,
+};
+
+int cxl_mem_patrol_scrub_init(struct cxl_memdev *cxlmd)
+{
+	int ret = 0;
+	struct device *cxl_scrub_dev;
+	struct cxl_memdev_ps_params params;
+	struct cxl_mbox_supp_feat_entry feat_entry;
+	char scrub_name[CXL_MEMDEV_MAX_NAME_LENGTH];
+	struct cxl_patrol_scrub_context *cxl_ps_ctx;
+
+	ret = cxl_mem_get_supported_feature_entry(cxlmd, &cxl_patrol_scrub_uuid,
+						  &feat_entry);
+	if (ret < 0)
+		goto cxl_ps_init_exit;
+
+	if (!(feat_entry.attrb_flags & CXL_FEAT_ENTRY_FLAG_CHANGABLE))
+		goto cxl_ps_init_exit;
+
+	cxl_ps_ctx = devm_kzalloc(&cxlmd->dev, sizeof(*cxl_ps_ctx), GFP_KERNEL);
+	if (!cxl_ps_ctx)
+		goto cxl_ps_init_exit;
+
+	cxl_ps_ctx->get_feat_size = feat_entry.get_feat_size;
+	cxl_ps_ctx->set_feat_size = feat_entry.set_feat_size;
+	ret = cxl_mem_ps_get_attrbs(&cxlmd->dev, &params);
+	if (ret) {
+		dev_err(&cxlmd->dev, "Get CXL patrol scrub params fail ret=%d\n",
+			ret);
+		goto cxl_ps_init_exit;
+	}
+	cxl_ps_ctx->scrub_cycle_changable =  params.scrub_cycle_changable;
+
+	snprintf(scrub_name, sizeof(scrub_name), "%s_%s",
+		 CXL_PATROL_SCRUB, dev_name(&cxlmd->dev));
+	cxl_scrub_dev = devm_scrub_device_register(&cxlmd->dev, scrub_name,
+						   cxl_ps_ctx, &cxl_ps_scrub_ops,
+						   CXL_MEMDEV_PATROL_SCRUB_NUM_REGIONS);
+	if (PTR_ERR_OR_ZERO(cxl_scrub_dev)) {
+		ret = PTR_ERR_OR_ZERO(cxl_scrub_dev);
+		goto cxl_ps_reg_fail;
+	}
+
+cxl_ps_reg_fail:
+cxl_ps_init_exit:
+	return ret;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_mem_patrol_scrub_init, CXL);
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index fdac686560d4..1d0fad0dc5ae 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -969,6 +969,13 @@ int cxl_trigger_poison_list(struct cxl_memdev *cxlmd);
 int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa);
 int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa);
 
+/* cxl memory scrub functions */
+#ifdef CONFIG_CXL_SCRUB
+int cxl_mem_patrol_scrub_init(struct cxl_memdev *cxlmd);
+#else
+int cxl_mem_patrol_scrub_init(struct cxl_memdev *cxlmd) { return -ENOTSUP; }
+#endif
+
 #ifdef CONFIG_CXL_SUSPEND
 void cxl_mem_active_inc(void);
 void cxl_mem_active_dec(void);
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 0155fb66b580..86bba8794bb4 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -881,6 +881,12 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (rc)
 		return rc;
 
+	/*
+	 * Initialize optional CXL scrub features
+	 */
+	if (cxl_mem_patrol_scrub_init(cxlmd))
+		dev_dbg(&pdev->dev, "cxl_mem_patrol_scrub_init failed\n");
+
 	rc = devm_cxl_sanitize_setup_notifier(&pdev->dev, cxlmd);
 	if (rc)
 		return rc;
-- 
2.34.1

[RFC PATCH 3/6] memory: scrub: Add function to show scrub attributes in decimal

[shiju.jose]

From: Shiju Jose <shiju.jose@huawei.com>

Add function to show scrub control attributes to the user
in decimal.

Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
---
 drivers/memory/scrub/memory-scrub.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/memory/scrub/memory-scrub.c b/drivers/memory/scrub/memory-scrub.c
index b3011e7cd062..ff5b4a52d2da 100755
--- a/drivers/memory/scrub/memory-scrub.c
+++ b/drivers/memory/scrub/memory-scrub.c
@@ -130,6 +130,20 @@ static ssize_t scrub_attr_show(struct device *dev,
 	u64 val;
 	struct scrub_device_attribute *hattr = to_scrub_attr(devattr);
 
+	ret = hattr->ops->read(dev, hattr->attr, hattr->region_id, &val);
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%lld\n", val);
+}
+
+static ssize_t scrub_attr_show_hex(struct device *dev,
+				   struct device_attribute *devattr, char *buf)
+{
+	int ret;
+	u64 val;
+	struct scrub_device_attribute *hattr = to_scrub_attr(devattr);
+
 	ret = hattr->ops->read(dev, hattr->attr, hattr->region_id, &val);
 	if (ret < 0)
 		return ret;
@@ -236,8 +250,12 @@ static struct attribute *scrub_genattr(const void *drvdata,
 	hattr->region_id = region_id;
 
 	dattr = &hattr->dev_attr;
-	dattr->show = is_string ? scrub_attr_show_string : scrub_attr_show;
-	dattr->store = is_hex ? scrub_attr_store_hex : scrub_attr_store;
+	if (is_string) {
+		dattr->show = scrub_attr_show_string;
+	} else {
+		dattr->show = is_hex ? scrub_attr_show_hex : scrub_attr_show;
+		dattr->store = is_hex ? scrub_attr_store_hex : scrub_attr_store;
+	}
 
 	a = &dattr->attr;
 	sysfs_attr_init(a);
-- 
2.34.1

[RFC PATCH 4/6] memory: scrub: Add scrub control attributes for the DDR5 ECS

[shiju.jose]

From: Shiju Jose <shiju.jose@huawei.com>

Add scrub control attributes for the DDR5 ECS feature.

The Error Check Scrub (ECS) is a feature defined in JEDEC DDR5 SDRAM
Specification (JESD79-5) and allows the DRAM to internally read, correct
single-bit errors, and write back corrected data bits to the DRAM array
while providing transparency to error counts. The ECS control feature
allows the request to configure ECS input configurations during system
boot or at run-time.

The ECS control allows the requester to change the ECS threshold count
provided that the request is within the definition specified in DDR5 mode
registers, change mode between codeword mode and row count mode, and reset
the ECS counter.

Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
---
 drivers/memory/scrub/memory-scrub.c | 14 +++++++++++++-
 include/memory/memory-scrub.h       | 11 +++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/memory/scrub/memory-scrub.c b/drivers/memory/scrub/memory-scrub.c
index ff5b4a52d2da..5f7b1fd73fb1 100755
--- a/drivers/memory/scrub/memory-scrub.c
+++ b/drivers/memory/scrub/memory-scrub.c
@@ -211,7 +211,8 @@ static bool is_hex_attr(u32 attr)
 
 static bool is_string_attr(u32 attr)
 {
-	return	attr == scrub_speed_available;
+	return	attr == scrub_speed_available ||
+		attr == scrub_threshold_available;
 }
 
 static struct attribute *scrub_genattr(const void *drvdata,
@@ -266,11 +267,22 @@ static struct attribute *scrub_genattr(const void *drvdata,
 }
 
 static const char * const scrub_common_attrs[] = {
+	/* scrub attributes - common */
 	[scrub_addr_base] = "addr_base",
 	[scrub_addr_size] = "addr_size",
 	[scrub_enable] = "enable",
 	[scrub_speed] = "speed",
 	[scrub_speed_available] = "speed_available",
+	/* scrub attributes - DDR5 ECS/common */
+	[scrub_ecs_log_entry_type] = "ecs_log_entry_type",
+	[scrub_ecs_log_entry_type_per_dram] = "ecs_log_entry_type_per_dram",
+	[scrub_ecs_log_entry_type_per_memory_media] = "ecs_log_entry_type_per_memory_media",
+	[scrub_mode] = "mode",
+	[scrub_mode_counts_rows] = "mode_counts_rows",
+	[scrub_mode_counts_codewords] = "mode_counts_codewords",
+	[scrub_reset_counter] = "reset_counter",
+	[scrub_threshold] = "threshold",
+	[scrub_threshold_available] = "threshold_available",
 };
 
 static struct attribute **
diff --git a/include/memory/memory-scrub.h b/include/memory/memory-scrub.h
index 8e999c9daaed..2e223e29a5bc 100755
--- a/include/memory/memory-scrub.h
+++ b/include/memory/memory-scrub.h
@@ -17,11 +17,22 @@ enum scrub_types {
 };
 
 enum scrub_attributes {
+	/* scrub attributes - common */
 	scrub_addr_base,
 	scrub_addr_size,
 	scrub_enable,
 	scrub_speed,
 	scrub_speed_available,
+	/* scrub attributes - DDR5 ECS/common */
+	scrub_ecs_log_entry_type,
+	scrub_ecs_log_entry_type_per_dram,
+	scrub_ecs_log_entry_type_per_memory_media,
+	scrub_mode,
+	scrub_mode_counts_rows,
+	scrub_mode_counts_codewords,
+	scrub_reset_counter,
+	scrub_threshold,
+	scrub_threshold_available,
 	max_attrs,
 };
 
-- 
2.34.1

[RFC PATCH 5/6] cxl/memscrub: Add CXL device DDR5 ECS control feature

[shiju.jose]

From: Shiju Jose <shiju.jose@huawei.com>

CXL spec 3.1 section 8.2.9.9.11.2 describes the DDR5 Error Check Scrub (ECS)
control feature.

The Error Check Scrub (ECS) is a feature defined in JEDEC DDR5 SDRAM
Specification (JESD79-5) and allows the DRAM to internally read, correct
single-bit errors, and write back corrected data bits to the DRAM array
while providing transparency to error counts. The ECS control feature
allows the request to configure ECS input configurations during system
boot or at run-time.

The ECS control allows the requester to change the log entry type, the ECS
threshold count provided that the request is within the definition
specified in DDR5 mode registers, change mode between codeword mode and
row count mode, and reset the ECS counter.

Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
---
 drivers/cxl/core/memscrub.c | 557 +++++++++++++++++++++++++++++++++++-
 drivers/cxl/cxlmem.h        |   2 +
 drivers/cxl/pci.c           |   2 +
 3 files changed, 559 insertions(+), 2 deletions(-)

diff --git a/drivers/cxl/core/memscrub.c b/drivers/cxl/core/memscrub.c
index ec67ffc81363..1c7031166ba0 100644
--- a/drivers/cxl/core/memscrub.c
+++ b/drivers/cxl/core/memscrub.c
@@ -5,9 +5,9 @@
  * Copyright (c) 2023 HiSilicon Limited.
  *
  *  - Provides functions to configure patrol scrub
- *    feature of the CXL memory devices.
+ *    and DDR5 ECS features of the CXL memory devices.
  *  - Registers with the scrub driver for the
- *    memory patrol scrub feature.
+ *    memory patrol scrub and DDR5 ECS features.
  */
 
 #define pr_fmt(fmt)	"CXL_MEM_SCRUB: " fmt
@@ -453,3 +453,556 @@ int cxl_mem_patrol_scrub_init(struct cxl_memdev *cxlmd)
 	return ret;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_mem_patrol_scrub_init, CXL);
+
+/* CXL DDR5 ECS control definitions */
+#define CXL_MEMDEV_ECS_GET_FEAT_VERSION	0x01
+#define CXL_MEMDEV_ECS_SET_FEAT_VERSION	0x01
+
+#define CXL_DDR5_ECS	"cxl_ecs"
+
+/* The default number of regions for CXL memory device patrol scrubber */
+#define CXL_MEMDEV_ECS_NUM_REGIONS	1
+
+static const uuid_t cxl_ecs_uuid =
+	UUID_INIT(0xe5b13f22, 0x2328, 0x4a14, 0xb8, 0xba, 0xb9, 0x69, 0x1e,     \
+		  0x89, 0x33, 0x86);
+
+/* CXL DDR5 ECS control functions */
+struct cxl_ecs_context {
+	struct device *dev;
+	u16 nregions;
+	u16 get_feat_size;
+	u16 set_feat_size;
+};
+
+/**
+ * struct cxl_memdev_ecs_params - CXL memory DDR5 ECS parameter data structure.
+ * @log_entry_type: ECS log entry type, per DRAM or per memory media FRU.
+ * @threshold: ECS threshold count per GB of memory cells.
+ * @mode:	codeword/row count mode
+ *		0 : ECS counts rows with errors
+ *		1 : ECS counts codeword with errors
+ * @reset_counter: [IN] reset ECC counter to default value.
+ * @log_entry_type_avail: Supported ECS log entry types.
+ * @threshold_avail: Supported ECS threshold counts.
+ * @mode_avail: Supported ECS count mode.
+ */
+struct cxl_memdev_ecs_params {
+	u8 log_entry_type;
+	u16 threshold;
+	u8 mode;
+	bool reset_counter;
+	char log_entry_type_avail[CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH];
+	char threshold_avail[CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH];
+	char mode_avail[CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH];
+};
+
+enum {
+	CXL_MEMDEV_ECS_PARAM_LOG_ENTRY_TYPE = 0,
+	CXL_MEMDEV_ECS_PARAM_THRESHOLD,
+	CXL_MEMDEV_ECS_PARAM_MODE,
+	CXL_MEMDEV_ECS_PARAM_RESET_COUNTER,
+};
+
+#define	CXL_MEMDEV_ECS_LOG_ENTRY_TYPE_MASK	GENMASK(1, 0)
+#define	CXL_MEMDEV_ECS_REALTIME_REPORT_CAP_MASK	BIT(0)
+#define	CXL_MEMDEV_ECS_THRESHOLD_COUNT_MASK	GENMASK(2, 0)
+#define	CXL_MEMDEV_ECS_MODE_MASK	BIT(3)
+#define	CXL_MEMDEV_ECS_RESET_COUNTER_MASK	BIT(4)
+
+static const u16 ecs_supp_threshold[] = { 0, 0, 0, 256, 1024, 4096 };
+
+enum {
+	ECS_LOG_ENTRY_TYPE_DRAM = 0x0,
+	ECS_LOG_ENTRY_TYPE_MEM_MEDIA_FRU = 0x1,
+};
+
+enum {
+	ECS_THRESHOLD_256 = 3,
+	ECS_THRESHOLD_1024 = 4,
+	ECS_THRESHOLD_4096 = 5,
+};
+
+enum {
+	ECS_MODE_COUNTS_ROWS = 0,
+	ECS_MODE_COUNTS_CODEWORDS = 1,
+};
+
+struct cxl_memdev_ecs_feat_read_attrbs {
+	u8 ecs_log_cap;
+	u8 ecs_cap;
+	__le16 ecs_config;
+	u8 ecs_flags;
+}  __packed;
+
+struct cxl_memdev_ecs_set_feat_pi {
+	struct cxl_mbox_set_feat_in pi;
+	struct cxl_memdev_ecs_feat_wr_attrbs {
+		u8 ecs_log_cap;
+		__le16 ecs_config;
+	} __packed wr_attrbs[];
+}  __packed;
+
+static int cxl_mem_ecs_get_attrbs(struct device *dev, int fru_id,
+				  struct cxl_memdev_ecs_params *params)
+{
+	struct cxl_memdev *cxlmd = to_cxl_memdev(dev->parent);
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
+	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
+	struct cxl_mbox_get_feat_in pi = {
+		.uuid = cxl_ecs_uuid,
+		.offset = 0,
+		.selection = CXL_GET_FEAT_CURRENT_VALUE,
+	};
+	struct cxl_memdev_ecs_feat_read_attrbs *rd_attrbs;
+	struct cxl_ecs_context *cxl_ecs_ctx;
+	u8 threshold_index;
+
+	if (!mds)
+		return -EFAULT;
+	cxl_ecs_ctx = dev_get_drvdata(dev);
+
+	pi.count = cxl_ecs_ctx->get_feat_size;
+	rd_attrbs = cxl_get_feature(mds, &pi);
+	if (PTR_ERR_OR_ZERO(rd_attrbs)) {
+		params->log_entry_type = 0;
+		params->threshold = 0;
+		params->mode = 0;
+		snprintf(params->log_entry_type_avail, CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH,
+			 "Unavailable");
+		snprintf(params->threshold_avail, CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH,
+			 "Unavailable");
+		snprintf(params->mode_avail, CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH,
+			 "Unavailable");
+		return PTR_ERR_OR_ZERO(rd_attrbs);
+	}
+	params->log_entry_type = FIELD_GET(CXL_MEMDEV_ECS_LOG_ENTRY_TYPE_MASK,
+					   rd_attrbs[fru_id].ecs_log_cap);
+	threshold_index = FIELD_GET(CXL_MEMDEV_ECS_THRESHOLD_COUNT_MASK,
+				    rd_attrbs[fru_id].ecs_config);
+	params->threshold = ecs_supp_threshold[threshold_index];
+	params->mode = FIELD_GET(CXL_MEMDEV_ECS_MODE_MASK,
+				 rd_attrbs[fru_id].ecs_config);
+	snprintf(params->log_entry_type_avail, CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH,
+		 "Log Entry Type 0:per DRAM  1:per Memory Media FRU");
+	snprintf(params->threshold_avail, CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH,
+		 "Threshold count per Gb of memory cells: 256,1024,4096");
+	snprintf(params->mode_avail, CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH,
+		 "Mode 0:ECS counts rows with errors  1:ECS counts codewords with errors");
+
+	kvfree(rd_attrbs);
+
+	return 0;
+}
+
+static int cxl_mem_ecs_set_attrbs(struct device *dev, int fru_id,
+				  struct cxl_memdev_ecs_params *params, u8 param_type)
+{
+	int ret = 0;
+	u32 set_pi_size;
+	u16 nmedia_frus, count;
+	struct cxl_memdev_ecs_set_feat_pi *set_pi;
+	struct cxl_memdev *cxlmd = to_cxl_memdev(dev->parent);
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
+	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
+	struct cxl_mbox_get_feat_in pi = {
+		.uuid = cxl_ecs_uuid,
+		.offset = 0,
+		.selection = CXL_GET_FEAT_CURRENT_VALUE,
+	};
+	struct cxl_memdev_ecs_feat_read_attrbs *rd_attrbs;
+	struct cxl_memdev_ecs_feat_wr_attrbs *wr_attrbs;
+	struct cxl_ecs_context *cxl_ecs_ctx;
+	struct cxl_memdev_ecs_params rd_params;
+
+	if (!mds)
+		return -EFAULT;
+
+	cxl_ecs_ctx = dev_get_drvdata(dev);
+	nmedia_frus = cxl_ecs_ctx->nregions;
+
+	pi.count = cxl_ecs_ctx->get_feat_size;
+	rd_attrbs = cxl_get_feature(mds, &pi);
+	if (PTR_ERR_OR_ZERO(rd_attrbs))
+		return PTR_ERR_OR_ZERO(rd_attrbs);
+	set_pi_size = sizeof(struct cxl_mbox_set_feat_in) +
+				cxl_ecs_ctx->set_feat_size;
+	set_pi = kvmalloc(set_pi_size, GFP_KERNEL);
+	if (!set_pi) {
+		kvfree(rd_attrbs);
+		return -ENOMEM;
+	}
+
+	set_pi->pi.uuid = cxl_ecs_uuid;
+	set_pi->pi.flags = CXL_SET_FEAT_FLAG_MOD_VALUE_SAVED_ACROSS_RESET |
+				CXL_SET_FEAT_FLAG_ACTION_FULL_DATA_TRANSFER;
+	set_pi->pi.offset = 0;
+	set_pi->pi.version = CXL_MEMDEV_ECS_SET_FEAT_VERSION;
+	/* Fill writable attributes from the current attributes read for all the media FRUs */
+	count = 0;
+	wr_attrbs = set_pi->wr_attrbs;
+	while (count < nmedia_frus) {
+		wr_attrbs[count].ecs_log_cap = rd_attrbs[count].ecs_log_cap;
+		wr_attrbs[count].ecs_config = rd_attrbs[count].ecs_config;
+		count++;
+	}
+	kvfree(rd_attrbs);
+
+	/* Fill attribute to be set for the media FRU */
+	if (param_type == CXL_MEMDEV_ECS_PARAM_LOG_ENTRY_TYPE) {
+		if (params->log_entry_type != ECS_LOG_ENTRY_TYPE_DRAM &&
+		    params->log_entry_type != ECS_LOG_ENTRY_TYPE_MEM_MEDIA_FRU) {
+			dev_err(dev->parent,
+				"Invalid CXL ECS scrub log entry type(%d) to set\n",
+			       params->log_entry_type);
+			dev_err(dev->parent,
+				"Log Entry Type 0: per DRAM  1: per Memory Media FRU\n");
+			ret = -EINVAL;
+			goto set_attrbs_exit;
+		}
+		wr_attrbs[fru_id].ecs_log_cap = FIELD_PREP(CXL_MEMDEV_ECS_LOG_ENTRY_TYPE_MASK,
+							   params->log_entry_type);
+	} else if (param_type == CXL_MEMDEV_ECS_PARAM_THRESHOLD) {
+		wr_attrbs[fru_id].ecs_config &= ~CXL_MEMDEV_ECS_THRESHOLD_COUNT_MASK;
+		switch (params->threshold) {
+		case 256:
+			wr_attrbs[fru_id].ecs_config |= FIELD_PREP(
+						CXL_MEMDEV_ECS_THRESHOLD_COUNT_MASK,
+						ECS_THRESHOLD_256);
+			break;
+		case 1024:
+			wr_attrbs[fru_id].ecs_config |= FIELD_PREP(
+						CXL_MEMDEV_ECS_THRESHOLD_COUNT_MASK,
+						ECS_THRESHOLD_1024);
+			break;
+		case 4096:
+			wr_attrbs[fru_id].ecs_config |= FIELD_PREP(
+						CXL_MEMDEV_ECS_THRESHOLD_COUNT_MASK,
+						ECS_THRESHOLD_4096);
+			break;
+		default:
+			dev_err(dev->parent,
+				"Invalid CXL ECS scrub threashol count(%d) to set\n",
+				params->threshold);
+			dev_err(dev->parent,
+				"Threshold count per Gb pf memory cells: 256,1024,4096\n");
+			ret = -EINVAL;
+			goto set_attrbs_exit;
+		}
+	} else if (param_type == CXL_MEMDEV_ECS_PARAM_MODE) {
+		if (params->mode != ECS_MODE_COUNTS_ROWS &&
+		    params->mode != ECS_MODE_COUNTS_CODEWORDS) {
+			dev_err(dev->parent,
+				"Invalid CXL ECS scrub mode(%d) to set\n",
+				params->mode);
+			dev_err(dev->parent,
+				"Mode 0: ECS counts rows with errors"
+				" 1: ECS counts codewords with errors\n");
+			ret = -EINVAL;
+			goto set_attrbs_exit;
+		}
+		wr_attrbs[fru_id].ecs_config &= ~CXL_MEMDEV_ECS_MODE_MASK;
+		wr_attrbs[fru_id].ecs_config |= FIELD_PREP(CXL_MEMDEV_ECS_MODE_MASK,
+							  params->mode);
+	} else if (param_type == CXL_MEMDEV_ECS_PARAM_RESET_COUNTER) {
+		wr_attrbs[fru_id].ecs_config &= ~CXL_MEMDEV_ECS_RESET_COUNTER_MASK;
+		wr_attrbs[fru_id].ecs_config |= FIELD_PREP(CXL_MEMDEV_ECS_RESET_COUNTER_MASK,
+							   params->reset_counter);
+	} else {
+		dev_err(dev->parent, "Invalid CXL ECS parameter to set\n");
+		ret = -EINVAL;
+		goto set_attrbs_exit;
+	}
+
+	ret = cxl_set_feature(mds, set_pi, set_pi_size);
+	if (ret) {
+		dev_err(dev->parent, "CXL ECS set feature fail ret=%d\n", ret);
+		goto set_attrbs_exit;
+	}
+
+	/* Verify attribute is set successfully */
+	ret = cxl_mem_ecs_get_attrbs(dev, fru_id, &rd_params);
+	if (ret) {
+		dev_err(dev->parent, "Get cxlmemdev ECS params fail ret=%d\n", ret);
+		return ret;
+	}
+	switch (param_type) {
+	case CXL_MEMDEV_ECS_PARAM_LOG_ENTRY_TYPE:
+		if (rd_params.log_entry_type != params->log_entry_type)
+			return -EFAULT;
+		break;
+	case CXL_MEMDEV_ECS_PARAM_THRESHOLD:
+		if (rd_params.threshold != params->threshold)
+			return -EFAULT;
+		break;
+	case CXL_MEMDEV_ECS_PARAM_MODE:
+		if (rd_params.mode != params->mode)
+			return -EFAULT;
+		break;
+	}
+
+set_attrbs_exit:
+	kvfree(set_pi);
+	return ret;
+}
+
+static int cxl_mem_ecs_log_entry_type_write(struct device *dev, int region_id, long val)
+{
+	int ret;
+	struct cxl_memdev_ecs_params params;
+
+	params.log_entry_type = val;
+	ret = cxl_mem_ecs_set_attrbs(dev, region_id, &params,
+				     CXL_MEMDEV_ECS_PARAM_LOG_ENTRY_TYPE);
+	if (ret) {
+		dev_err(dev->parent, "Set CXL ECS params for log entry type fail ret=%d\n",
+			ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int cxl_mem_ecs_threshold_write(struct device *dev, int region_id, long val)
+{
+	int ret;
+	struct cxl_memdev_ecs_params params;
+
+	params.threshold = val;
+	ret = cxl_mem_ecs_set_attrbs(dev, region_id, &params,
+				     CXL_MEMDEV_ECS_PARAM_THRESHOLD);
+	if (ret) {
+		dev_err(dev->parent, "Set CXL ECS params for threshold fail ret=%d\n",
+			ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int cxl_mem_ecs_mode_write(struct device *dev, int region_id, long val)
+{
+	int ret;
+	struct cxl_memdev_ecs_params params;
+
+	params.mode = val;
+	ret = cxl_mem_ecs_set_attrbs(dev, region_id, &params,
+				     CXL_MEMDEV_ECS_PARAM_MODE);
+	if (ret) {
+		dev_err(dev->parent, "Set CXL ECS params for mode fail ret=%d\n",
+			ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int cxl_mem_ecs_reset_counter_write(struct device *dev, int region_id, long val)
+{
+	int ret;
+	struct cxl_memdev_ecs_params params;
+
+	params.reset_counter = val;
+	ret = cxl_mem_ecs_set_attrbs(dev, region_id, &params,
+				     CXL_MEMDEV_ECS_PARAM_RESET_COUNTER);
+	if (ret) {
+		dev_err(dev->parent, "Set CXL ECS params for reset ECC counter fail ret=%d\n",
+			ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * cxl_mem_ecs_is_visible() - Callback to return attribute visibility
+ * @drv_data: Pointer to driver-private data structure passed
+ *	      as argument to devm_scrub_device_register().
+ * @attr: Scrub attribute
+ * @region_id: ID of the memory region
+ *
+ * Returns: 0 on success, an error otherwise
+ */
+static umode_t cxl_mem_ecs_is_visible(const void *drv_data, u32 attr, int region_id)
+{
+	switch (attr) {
+	case scrub_reset_counter:
+		return 0200;
+	case scrub_ecs_log_entry_type_per_dram:
+	case scrub_ecs_log_entry_type_per_memory_media:
+	case scrub_mode_counts_rows:
+	case scrub_mode_counts_codewords:
+	case scrub_threshold_available:
+		return 0444;
+	case scrub_ecs_log_entry_type:
+	case scrub_mode:
+	case scrub_threshold:
+		return 0644;
+	default:
+		return 0;
+	}
+}
+
+/**
+ * cxl_mem_ecs_read() - Read callback for data attributes
+ * @dev: Pointer to scrub device
+ * @attr: Scrub attribute
+ * @region_id: ID of the memory region
+ * @val: Pointer to the returned data
+ *
+ * Returns: 0 on success, an error otherwise
+ */
+static int cxl_mem_ecs_read(struct device *dev, u32 attr, int region_id, u64 *val)
+{
+	int ret;
+	struct cxl_memdev_ecs_params params;
+
+	if (attr == scrub_ecs_log_entry_type ||
+	    attr == scrub_ecs_log_entry_type_per_dram ||
+	    attr == scrub_ecs_log_entry_type_per_memory_media ||
+	    attr == scrub_mode ||
+	    attr == scrub_mode_counts_rows ||
+	    attr == scrub_mode_counts_codewords ||
+	    attr == scrub_threshold) {
+		ret = cxl_mem_ecs_get_attrbs(dev, region_id, &params);
+		if (ret) {
+			dev_err(dev->parent, "Get CXL ECS params fail ret=%d\n", ret);
+			return ret;
+		}
+	}
+
+	switch (attr) {
+	case scrub_ecs_log_entry_type:
+		*val = params.log_entry_type;
+		return 0;
+	case scrub_ecs_log_entry_type_per_dram:
+		if (params.log_entry_type == ECS_LOG_ENTRY_TYPE_DRAM)
+			*val = 1;
+		else
+			*val = 0;
+		return 0;
+	case scrub_ecs_log_entry_type_per_memory_media:
+		if (params.log_entry_type == ECS_LOG_ENTRY_TYPE_MEM_MEDIA_FRU)
+			*val = 1;
+		else
+			*val = 0;
+		return 0;
+	case scrub_mode:
+		*val = params.mode;
+		return 0;
+	case scrub_mode_counts_rows:
+		if (params.mode == ECS_MODE_COUNTS_ROWS)
+			*val = 1;
+		else
+			*val = 0;
+		return 0;
+	case scrub_mode_counts_codewords:
+		if (params.mode == ECS_MODE_COUNTS_CODEWORDS)
+			*val = 1;
+		else
+			*val = 0;
+		return 0;
+	case scrub_threshold:
+		*val = params.threshold;
+		return 0;
+	default:
+		return -ENOTSUPP;
+	}
+}
+
+/**
+ * cxl_mem_ecs_write() - Write callback for data attributes
+ * @dev: Pointer to scrub device
+ * @attr: Scrub attribute
+ * @region_id: ID of the memory region
+ * @val: Value to write
+ *
+ * Returns: 0 on success, an error otherwise
+ */
+static int cxl_mem_ecs_write(struct device *dev, u32 attr, int region_id, u64 val)
+{
+	switch (attr) {
+	case scrub_ecs_log_entry_type:
+		return cxl_mem_ecs_log_entry_type_write(dev, region_id, val);
+	case scrub_mode:
+		return cxl_mem_ecs_mode_write(dev, region_id, val);
+	case scrub_reset_counter:
+		return cxl_mem_ecs_reset_counter_write(dev, region_id, val);
+	case scrub_threshold:
+		return cxl_mem_ecs_threshold_write(dev, region_id, val);
+	default:
+		return -ENOTSUPP;
+	}
+}
+
+/**
+ * cxl_mem_ecs_read_strings() - Read callback for DDR5 ECS string attributes
+ * @dev: Pointer to ECS scrub device
+ * @attr: ECS scrub attribute
+ * @region_id: ID of the memory media FRU.
+ * @buf: Pointer to the buffer for copying returned string
+ *
+ * Returns: 0 on success, an error otherwise
+ */
+static int cxl_mem_ecs_read_strings(struct device *dev, u32 attr,
+				    int region_id, char *buf)
+{
+
+	switch (attr) {
+	case scrub_threshold_available:
+		sprintf(buf, "256,1024,4096\n");
+		return 0;
+	default:
+		return -ENOTSUPP;
+	}
+}
+
+static const struct scrub_ops cxl_ecs_ops = {
+	.is_visible = cxl_mem_ecs_is_visible,
+	.read = cxl_mem_ecs_read,
+	.write = cxl_mem_ecs_write,
+	.read_string = cxl_mem_ecs_read_strings,
+};
+
+int cxl_mem_ddr5_ecs_init(struct cxl_memdev *cxlmd)
+{
+	int nmedia_frus; /* number of memory media FRUs in the device */
+	int ret = 0;
+	struct device *cxl_scrub_dev;
+	char scrub_name[CXL_MEMDEV_MAX_NAME_LENGTH];
+	struct cxl_mbox_supp_feat_entry feat_entry;
+	struct cxl_ecs_context *cxl_ecs_ctx;
+
+	ret = cxl_mem_get_supported_feature_entry(cxlmd, &cxl_ecs_uuid, &feat_entry);
+	if (ret < 0)
+		goto cxl_ecs_reg_fail;
+
+	if (!(feat_entry.attrb_flags & CXL_FEAT_ENTRY_FLAG_CHANGABLE))
+		goto cxl_ecs_init_exit;
+	nmedia_frus = feat_entry.get_feat_size/
+				sizeof(struct cxl_memdev_ecs_feat_read_attrbs);
+	if (nmedia_frus) {
+		cxl_ecs_ctx = devm_kzalloc(&cxlmd->dev, sizeof(*cxl_ecs_ctx), GFP_KERNEL);
+		if (!cxl_ecs_ctx)
+			goto cxl_ecs_init_exit;
+
+		cxl_ecs_ctx->nregions = nmedia_frus;
+		cxl_ecs_ctx->get_feat_size = feat_entry.get_feat_size;
+		cxl_ecs_ctx->set_feat_size = feat_entry.set_feat_size;
+
+		snprintf(scrub_name, sizeof(scrub_name), "%s_%s",
+			 CXL_DDR5_ECS, dev_name(&cxlmd->dev));
+		cxl_scrub_dev = devm_scrub_device_register(&cxlmd->dev, scrub_name,
+							  cxl_ecs_ctx, &cxl_ecs_ops,
+							  cxl_ecs_ctx->nregions);
+		if (PTR_ERR_OR_ZERO(cxl_scrub_dev)) {
+			ret = PTR_ERR_OR_ZERO(cxl_scrub_dev);
+			goto cxl_ecs_reg_fail;
+		}
+	}
+
+cxl_ecs_reg_fail:
+cxl_ecs_init_exit:
+	return ret;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_mem_ddr5_ecs_init, CXL);
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 1d0fad0dc5ae..4da0b0ec3b1e 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -972,8 +972,10 @@ int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa);
 /* cxl memory scrub functions */
 #ifdef CONFIG_CXL_SCRUB
 int cxl_mem_patrol_scrub_init(struct cxl_memdev *cxlmd);
+int cxl_mem_ddr5_ecs_init(struct cxl_memdev *cxlmd);
 #else
 int cxl_mem_patrol_scrub_init(struct cxl_memdev *cxlmd) { return -ENOTSUP; }
+int cxl_mem_ddr5_ecs_init(struct cxl_memdev *cxlmd) { return -ENOTSUP; }
 #endif
 
 #ifdef CONFIG_CXL_SUSPEND
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 86bba8794bb4..75ce4f41c5c0 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -886,6 +886,8 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	 */
 	if (cxl_mem_patrol_scrub_init(cxlmd))
 		dev_dbg(&pdev->dev, "cxl_mem_patrol_scrub_init failed\n");
+	if (cxl_mem_ddr5_ecs_init(cxlmd))
+		dev_dbg(&pdev->dev, "cxl_mem_ddr5_ecs_init failed\n");
 
 	rc = devm_cxl_sanitize_setup_notifier(&pdev->dev, cxlmd);
 	if (rc)
-- 
2.34.1

[RFC PATCH 6/6] cxl: scrub: sysfs: Add Documentation for CXL memory device scrub control attributes

[shiju.jose]

From: Shiju Jose <shiju.jose@huawei.com>

Add sysfs documentation entries for the CXL memory device scrub
control attributes those are exposed in /sys/class/scrub/ by the
scrub driver. These attributes support configuring a CXL memory
device scrub.

Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
---
 .../testing/sysfs-class-cxl-scrub-configure   | 135 ++++++++++++++++++
 1 file changed, 135 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-cxl-scrub-configure

diff --git a/Documentation/ABI/testing/sysfs-class-cxl-scrub-configure b/Documentation/ABI/testing/sysfs-class-cxl-scrub-configure
new file mode 100644
index 000000000000..9dd0c18451aa
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-cxl-scrub-configure
@@ -0,0 +1,135 @@
+What:		/sys/class/scrub/
+Date:		November 2023
+KernelVersion:	6.8
+Contact:	linux-kernel@vger.kernel.org
+Description:
+		The scrub/ class subdirectory belongs to the scrub
+		subsystem.
+
+What:		/sys/class/scrub/scrubX/
+Date:		November 2023
+KernelVersion:	6.8
+Contact:	linux-kernel@vger.kernel.org
+Description:
+		The /sys/class/scrub/scrub{0,1,2,3,...} directories
+		correspond to each scrub device.
+
+What:		/sys/class/scrub/scrubX/name
+Date:		November 2023
+KernelVersion:	6.8
+Contact:	linux-kernel@vger.kernel.org
+Description:
+		(RO) name of the memory scrub device
+
+What:		/sys/class/scrub/scrubX/regionY/
+Date:		November 2023
+KernelVersion:	6.8
+Contact:	linux-kernel@vger.kernel.org
+Description:
+		The /sys/class/scrub/scrubX/region{0,1,2,3,...}
+		directories correspond to each scrub region under a scrub device.
+		Scrub region is a physical address range or for example
+		memory media FRU of DDR5 ECS feature for which scrub may be
+		separately controlled.
+
+What:		/sys/class/scrub/scrubX/regionY/enable
+Date:		November 2023
+KernelVersion:	6.8
+Contact:	linux-kernel@vger.kernel.org
+Description:
+		(WO) Start/Stop CXL memory patrol scrub.
+		1 - enable the CXL memory patrol scrub.
+		0 - disable the CXL memory patrol scrub.
+
+What:		/sys/class/scrub/scrubX/regionY/speed
+Date:		November 2023
+KernelVersion:	6.8
+Contact:	linux-kernel@vger.kernel.org
+Description:
+		(RW) The scrub cycle to set for the CXL memory
+		patrol scrub and it must be within the supported
+		range. The unit of the scrub cycle is hour.
+
+What:		/sys/class/scrub/scrubX/regionY/speed_available
+Date:		November 2023
+KernelVersion:	6.8
+Contact:	linux-kernel@vger.kernel.org
+Description:
+		(RO) Supported range of the scrub cycle by the
+		CXL memory patrol scrub.
+		The unit of the scrub cycle is hour.
+
+What:		/sys/class/scrub/scrubX/regionY/ecs_log_entry_type
+Date:		November 2023
+KernelVersion:	6.8
+Contact:	linux-kernel@vger.kernel.org
+Description:
+		(RW) The log entry type of how the DDR5 ECS log is
+		reported.
+		00b - per DRAM.
+		01b - per memory media FRU.
+
+What:		/sys/class/scrub/scrubX/regionY/ecs_log_entry_type_per_dram
+Date:		November 2023
+KernelVersion:	6.8
+Contact:	linux-kernel@vger.kernel.org
+Description:
+		(RO) Returns true if current log entry type of DDR5 ECS
+		region is per DRAM.
+
+What:		/sys/class/scrub/scrubX/regionY/ecs_log_entry_type_per_memory_media
+Date:		November 2023
+KernelVersion:	6.8
+Contact:	linux-kernel@vger.kernel.org
+Description:
+		(RO) Returns true if current log entry type of DDR5 ECS
+		region is per memory media FRU.
+
+What:		/sys/class/scrub/scrubX/regionY/mode
+Date:		November 2023
+KernelVersion:	6.8
+Contact:	linux-kernel@vger.kernel.org
+Description:
+		(RW) The mode of how the DDR5 ECS counts the errors.
+		0 - ECS counts rows with errors.
+		1 - ECS counts codewords with errors.
+
+What:		/sys/class/scrub/scrubX/regionY/mode_counts_rows
+Date:		November 2023
+KernelVersion:	6.8
+Contact:	linux-kernel@vger.kernel.org
+Description:
+		(RO) Returns true if current mode of DDR5 ECS region
+		is counts rows with errors.
+
+What:		/sys/class/scrub/scrubX/regionY/mode_counts_codewords
+Date:		November 2023
+KernelVersion:	6.8
+Contact:	linux-kernel@vger.kernel.org
+Description:
+		(RO) Returns true if current mode of DDR5 ECS region
+		is counts codewords with errors.
+
+What:		/sys/class/scrub/scrubX/regionY/reset_counter
+Date:		November 2023
+KernelVersion:	6.8
+Contact:	linux-kernel@vger.kernel.org
+Description:
+		(WO) DDR5 ECS reset ECC counter.
+		0 - normal, ECC counter running actively.
+		1 - reset ECC counter to the default value.
+
+What:		/sys/class/scrub/scrubX/regionY/threshold
+Date:		November 2023
+KernelVersion:	6.8
+Contact:	linux-kernel@vger.kernel.org
+Description:
+		(RW) DDR5 ECS threshold count per GB of memory cells.
+
+What:		/sys/class/scrub/scrubX/regionY/threshold_available
+Date:		November 2023
+KernelVersion:	6.8
+Contact:	linux-kernel@vger.kernel.org
+Description:
+		(RO) Supported list of DDR5 ECS threshold count per GB of
+		memory cells.
-- 
2.34.1

Re: [RFC PATCH 1/6] cxl/mbox: Add GET_SUPPORTED_FEATURES, GET_FEATURE and SET_FEATURE mailbox commands

[Dave Jiang]

On 11/14/23 05:56, shiju.jose@huawei.com wrote:
> From: Shiju Jose <shiju.jose@huawei.com>
> 
> Add support for GET_SUPPORTED_FEATURES, GET_FEATURE and SET_FEATURE mailbox
> commands.
> 
> CXL spec 3.0 section 8.2.9.6 describes optional device specific features.
> CXL devices supports features with changeable attributes.
> Get Supported Features retrieves the list of supported device specific
> features. The settings of a feature can be retrieved using Get
> Feature and optionally modified using Set Feature.

Maybe split this patch out into 3 for each of the individual commands you are enabling. That'll make git bisect easier for debugging.

A few nits but the rest LGTM.

> 
> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> ---
>  drivers/cxl/core/mbox.c      | 74 ++++++++++++++++++++++++++++
>  drivers/cxl/cxlmem.h         | 95 ++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/cxl_mem.h |  3 ++
>  3 files changed, 172 insertions(+)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 36270dcfb42e..2a6d8ab927bd 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -63,6 +63,9 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
>  	CXL_CMD(GET_SHUTDOWN_STATE, 0, 0x1, 0),
>  	CXL_CMD(SET_SHUTDOWN_STATE, 0x1, 0, 0),
>  	CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0),
> +	CXL_CMD(GET_SUPPORTED_FEATURES, 0x8, CXL_VARIABLE_PAYLOAD, 0),
> +	CXL_CMD(GET_FEATURE, 0x15, CXL_VARIABLE_PAYLOAD, 0),
> +	CXL_CMD(SET_FEATURE, CXL_VARIABLE_PAYLOAD, 0, 0),
>  };
>  
>  /*
> @@ -1303,6 +1306,77 @@ int cxl_set_timestamp(struct cxl_memdev_state *mds)
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_set_timestamp, CXL);
>  
> +struct cxl_mbox_get_supp_feats_out *cxl_get_supported_features(struct cxl_memdev_state *mds,
> +						struct cxl_mbox_get_supp_feats_in *pi)
> +{
> +	int rc;
> +	struct cxl_mbox_cmd mbox_cmd;
> +	struct cxl_mbox_get_supp_feats_out *feats_out;

reverse xmas tree declarations

> +
> +	feats_out = kvmalloc(pi->count, GFP_KERNEL);
> +	if (!feats_out)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_GET_SUPPORTED_FEATURES,
> +		.size_in = sizeof(*pi),
> +		.payload_in = pi,
> +		.size_out = pi->count,
> +		.payload_out = feats_out,
> +		.min_out = sizeof(struct cxl_mbox_get_supp_feats_out),
> +	};
> +	rc = cxl_internal_send_cmd(mds, &mbox_cmd);
> +	if (rc < 0) {
> +		kvfree(feats_out);
> +		return ERR_PTR(rc);
> +	}
> +
> +	return feats_out;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_get_supported_features, CXL);
> +
> +void *cxl_get_feature(struct cxl_memdev_state *mds, struct cxl_mbox_get_feat_in *pi)
> +{
> +	int rc;
> +	void *feat_out;
> +	struct cxl_mbox_cmd mbox_cmd;

reverse xmas tree declarations


> +
> +	feat_out = kvmalloc(pi->count, GFP_KERNEL);
> +	if (!feat_out)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_GET_FEATURE,
> +		.size_in = sizeof(*pi),
> +		.payload_in = pi,
> +		.size_out = pi->count,
> +		.payload_out = feat_out,
> +		.min_out = pi->count,
> +	};
> +	rc = cxl_internal_send_cmd(mds, &mbox_cmd);
> +	if (rc < 0) {
> +		kvfree(feat_out);
> +		return ERR_PTR(rc);
> +	}
> +
> +	return feat_out;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_get_feature, CXL);
> +
> +int cxl_set_feature(struct cxl_memdev_state *mds, void *feat_in, size_t size)
> +{
> +	struct cxl_mbox_cmd mbox_cmd;
> +
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_SET_FEATURE,
> +		.size_in = size,
> +		.payload_in = feat_in,
> +	};
> +
> +	return cxl_internal_send_cmd(mds, &mbox_cmd);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_set_feature, CXL);
> +
>  int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
>  		       struct cxl_region *cxlr)
>  {
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index a2fcbca253f3..fdac686560d4 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -506,6 +506,9 @@ enum cxl_opcode {
>  	CXL_MBOX_OP_SET_TIMESTAMP	= 0x0301,
>  	CXL_MBOX_OP_GET_SUPPORTED_LOGS	= 0x0400,
>  	CXL_MBOX_OP_GET_LOG		= 0x0401,
> +	CXL_MBOX_OP_GET_SUPPORTED_FEATURES	= 0x0500,
> +	CXL_MBOX_OP_GET_FEATURE		= 0x0501,
> +	CXL_MBOX_OP_SET_FEATURE		= 0x0502,
>  	CXL_MBOX_OP_IDENTIFY		= 0x4000,
>  	CXL_MBOX_OP_GET_PARTITION_INFO	= 0x4100,
>  	CXL_MBOX_OP_SET_PARTITION_INFO	= 0x4101,
> @@ -740,6 +743,94 @@ struct cxl_mbox_set_timestamp_in {
>  
>  } __packed;
>  
> +/* Get Supported Features CXL 3.0 Spec 8.2.9.6.1 */
> +/*
> + * Get Supported Features input payload
> + * CXL rev 3.0 section 8.2.9.6.1; Table 8-75
> + */
> +struct cxl_mbox_get_supp_feats_in {
> +	__le32 count;
> +	__le16 start_index;
> +	u16 reserved;
> +} __packed;
> +
> +/*
> + * Get Supported Features Supported Feature Entry
> + * CXL rev 3.0 section 8.2.9.6.1; Table 8-77
> + */
> +/* Supported Feature Entry : Payload out attribute flags */
> +#define CXL_FEAT_ENTRY_FLAG_CHANGABLE	BIT(0)
> +#define CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_MASK	GENMASK(3, 1)
> +#define CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_NONE	0x0
> +#define CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_CXL_RESET	0x1
> +#define CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_HOT_RESET	0x2
> +#define CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_WARM_RESET	0x3
> +#define CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_COLD_RESET	0x4

I think you can declare the flags as enums?

> +#define CXL_FEAT_ENTRY_FLAG_PERSISTENCE_ACROSS_FW_UPDATE_MASK	BIT(4)
> +#define CXL_FEAT_ENTRY_FLAG_PERSISTENCE_DEFAULT_SEL_SUPPORT_MASK	BIT(5)
> +#define CXL_FEAT_ENTRY_FLAG_PERSISTENCE_SAVED_SEL_SUPPORT_MASK	BIT(6)
> +
> +struct cxl_mbox_supp_feat_entry {
> +	uuid_t uuid;
> +	__le16 feat_index;
> +	__le16 get_feat_size;
> +	__le16 set_feat_size;
> +	__le32 attrb_flags;
> +	u8 get_feat_version;
> +	u8 set_feat_version;
> +	__le16 set_feat_effects;
> +	u8 rsvd[18];
> +}  __packed;
> +
> +/*
> + * Get Supported Features output payload
> + * CXL rev 3.0 section 8.2.9.6.1; Table 8-76
> + */
> +struct cxl_mbox_get_supp_feats_out {
> +	__le16 entries;
> +	__le16 nsuppfeats_dev;
> +	u32 reserved;
> +	struct cxl_mbox_supp_feat_entry feat_entries[];
> +} __packed;
> +
> +/* Get Feature CXL 3.0 Spec 8.2.9.6.2 */
> +/*
> + * Get Feature input payload
> + * CXL rev 3.0 section 8.2.9.6.2; Table 8-79
> + */
> +/* Get Feature : Payload in selection */
> +#define CXL_GET_FEAT_CURRENT_VALUE	0x00
> +#define CXL_GET_FEAT_DEFAULT_VALUE	0x01
> +#define CXL_GET_FEAT_SAVED_VALUE	0x02
> +
> +struct cxl_mbox_get_feat_in {
> +	uuid_t uuid;
> +	__le16 offset;
> +	__le16 count;
> +	u8 selection;
> +}  __packed;
> +
> +/* Set Feature CXL 3.0 Spec 8.2.9.6.3 */
> +/*
> + * Set Feature input payload
> + * CXL rev 3.0 section 8.2.9.6.3; Table 8-81
> + */
> +/* Set Feature : Payload in flags */
> +#define CXL_SET_FEAT_FLAG_ACTION_MASK	GENMASK(2, 0)
> +#define CXL_SET_FEAT_FLAG_ACTION_FULL_DATA_TRANSFER	0x0
> +#define CXL_SET_FEAT_FLAG_ACTION_INITIATE_DATA_TRANSFER	0x1
> +#define CXL_SET_FEAT_FLAG_ACTION_CONTINUE_DATA_TRANSFER	0x2
> +#define CXL_SET_FEAT_FLAG_ACTION_FINISH_DATA_TRANSFER	0x3
> +#define CXL_SET_FEAT_FLAG_ACTION_ABORT_DATA_TRANSFER	0x4
> +#define CXL_SET_FEAT_FLAG_MOD_VALUE_SAVED_ACROSS_RESET	BIT(3)
> +struct cxl_mbox_set_feat_in {
> +	uuid_t uuid;
> +	__le32 flags;
> +	__le16 offset;
> +	u8 version;
> +	u8 rsvd[9];
> +}  __packed;
> +
>  /* Get Poison List  CXL 3.0 Spec 8.2.9.8.4.1 */
>  struct cxl_mbox_poison_in {
>  	__le64 offset;
> @@ -867,6 +958,10 @@ void clear_exclusive_cxl_commands(struct cxl_memdev_state *mds,
>  				  unsigned long *cmds);
>  void cxl_mem_get_event_records(struct cxl_memdev_state *mds, u32 status);
>  int cxl_set_timestamp(struct cxl_memdev_state *mds);
> +struct cxl_mbox_get_supp_feats_out *cxl_get_supported_features(struct cxl_memdev_state *mds,
> +						struct cxl_mbox_get_supp_feats_in *pi);
> +void *cxl_get_feature(struct cxl_memdev_state *mds, struct cxl_mbox_get_feat_in *pi);
> +int cxl_set_feature(struct cxl_memdev_state *mds, void *feat_in, size_t size);
>  int cxl_poison_state_init(struct cxl_memdev_state *mds);
>  int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
>  		       struct cxl_region *cxlr);
> diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
> index 14bc6e742148..8c89d323cc41 100644
> --- a/include/uapi/linux/cxl_mem.h
> +++ b/include/uapi/linux/cxl_mem.h
> @@ -46,6 +46,9 @@
>  	___C(GET_SCAN_MEDIA_CAPS, "Get Scan Media Capabilities"),         \
>  	___DEPRECATED(SCAN_MEDIA, "Scan Media"),                          \
>  	___DEPRECATED(GET_SCAN_MEDIA, "Get Scan Media Results"),          \
> +	___C(GET_SUPPORTED_FEATURES, "Get Supported Features"),           \
> +	___C(GET_FEATURE, "Get Feature"),                                 \
> +	___C(SET_FEATURE, "Set Feature"),                                 \
>  	___C(MAX, "invalid / last command")
>  
>  #define ___C(a, b) CXL_MEM_COMMAND_ID_##a

Re: [RFC PATCH 2/6] cxl/memscrub: Add CXL device patrol scrub control feature

[Dave Jiang]

On 11/14/23 05:56, shiju.jose@huawei.com wrote:
> From: Shiju Jose <shiju.jose@huawei.com>
> 
> CXL spec 3.1 section 8.2.9.9.11.1 describes the device patrol scrub control
> feature. The device patrol scrub proactively locates and makes corrections
> to errors in regular cycle. The patrol scrub control allows the request to
> configure patrol scrub input configurations.
> 
> The patrol scrub control allows the requester to specify the number of
> hours for which the patrol scrub cycles must be completed, provided that
> the requested number is not less than the minimum number of hours for the
> patrol scrub cycle that the device is capable of. In addition, the patrol
> scrub controls allow the host to disable and enable the feature in case
> disabling of the feature is needed for other purposes such as
> performance-aware operations which require the background operations to be
> turned off.
> 
> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> ---
>  drivers/cxl/Kconfig         |  23 ++
>  drivers/cxl/core/Makefile   |   1 +
>  drivers/cxl/core/memscrub.c | 455 ++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxlmem.h        |   7 +
>  drivers/cxl/pci.c           |   6 +
>  5 files changed, 492 insertions(+)


Maybe this patch can be split up? Awfully large. Maybe a patch with support functions and then another with usages?

>  create mode 100644 drivers/cxl/core/memscrub.c
> 
> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> index 8ea1d340e438..45ee6d57d899 100644
> --- a/drivers/cxl/Kconfig
> +++ b/drivers/cxl/Kconfig
> @@ -154,4 +154,27 @@ config CXL_PMU
>  	  monitoring units and provide standard perf based interfaces.
>  
>  	  If unsure say 'm'.
> +
> +config CXL_SCRUB
> +	tristate "CXL: Memory scrub feature"
> +	depends on CXL_PCI
> +	depends on CXL_MEM
> +	depends on SCRUB
> +	help
> +	  The CXL memory scrub control is an optional feature allows host to
> +	  control the scrub configurations of CXL Type 3 devices, which
> +	  support patrol scrub and/or DDR5 ECS(Error Check Scrub).
> +
> +	  Register with the scrub configure driver to provide sysfs interfaces
> +	  for configuring the CXL device memory patrol and DDR5 ECS scrubs.
> +	  Provides the interface functions support configuring the CXL memory
> +	  device patrol and ECS scrubs.
> +
> +	  Say 'y/m' to enable the CXL memory scrub driver that will attach to
> +	  CXL.mem devices for memory scrub control feature. See sections
> +	  8.2.9.9.11.1 and 8.2.9.9.11.2 in the CXL 3.1 specification for a
> +	  detailed description of CXL memory scrub control features.
> +
> +	  If unsure say 'm'.
> +
>  endif
> diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
> index 1f66b5d4d935..99e3202f868f 100644
> --- a/drivers/cxl/core/Makefile
> +++ b/drivers/cxl/core/Makefile
> @@ -15,3 +15,4 @@ cxl_core-y += hdm.o
>  cxl_core-y += pmu.o
>  cxl_core-$(CONFIG_TRACING) += trace.o
>  cxl_core-$(CONFIG_CXL_REGION) += region.o
> +cxl_core-$(CONFIG_CXL_SCRUB) += memscrub.o
> diff --git a/drivers/cxl/core/memscrub.c b/drivers/cxl/core/memscrub.c
> new file mode 100644
> index 000000000000..ec67ffc81363
> --- /dev/null
> +++ b/drivers/cxl/core/memscrub.c
> @@ -0,0 +1,455 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * cxl_memscrub.c - CXL memory scrub driver
> + *
> + * Copyright (c) 2023 HiSilicon Limited.
> + *
> + *  - Provides functions to configure patrol scrub
> + *    feature of the CXL memory devices.
> + *  - Registers with the scrub driver for the
> + *    memory patrol scrub feature.
> + */
> +
> +#define pr_fmt(fmt)	"CXL_MEM_SCRUB: " fmt
> +
> +#include <cxlmem.h>
> +#include <memory/memory-scrub.h>
> +
> +/* CXL memory scrub feature common definitions */
> +#define CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH	128
> +#define CXL_MEMDEV_MAX_NAME_LENGTH	128
> +
> +static int cxl_mem_get_supported_feature_entry(struct cxl_memdev *cxlmd, const uuid_t *feat_uuid,
> +					       struct cxl_mbox_supp_feat_entry *feat_entry_out)
> +{
> +	int nentries; /* number of supported feature entries in output payload */
> +	int feat_index, count;
> +	bool is_support_feature = false;
> +	struct cxl_mbox_get_supp_feats_in pi;
> +	struct cxl_mbox_supp_feat_entry *feat_entry;
> +	struct cxl_mbox_get_supp_feats_out *feats_out;
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> +
> +	feat_index = 0;
> +	do {
> +		pi.count = sizeof(struct cxl_mbox_get_supp_feats_out) +
> +				  sizeof(struct cxl_mbox_supp_feat_entry);
> +		pi.start_index = feat_index;
> +		nentries = 0;

Is this needed since you init it to feats_out->entries a few lines below

> +		feats_out = cxl_get_supported_features(mds, &pi);
> +		if (PTR_ERR_OR_ZERO(feats_out))
> +			return  PTR_ERR_OR_ZERO(feats_out);
> +		nentries = feats_out->entries;
> +		if (!nentries) {
> +			kvfree(feats_out);
> +			break;
> +		}
> +		/* Check CXL memdev supports the feature */
> +		feat_entry = (void *)feats_out->feat_entries;
> +		for (count = 0; count < nentries; count++, feat_entry++) {
> +			if (uuid_equal(&feat_entry->uuid, feat_uuid)) {
> +				is_support_feature = true;
> +				memcpy(feat_entry_out, feat_entry, sizeof(*feat_entry_out));
> +				break;
> +			}
> +		}
> +		kvfree(feats_out);
> +		if (is_support_feature)
> +			break;
> +		feat_index += nentries;
> +	} while (nentries);
> +
> +	if (!is_support_feature)
> +		return -ENOTSUPP;
> +
> +	return 0;
> +}
> +
> +/* CXL memory patrol scrub control definitions */
> +#define CXL_MEMDEV_PS_GET_FEAT_VERSION	0x01
> +#define CXL_MEMDEV_PS_SET_FEAT_VERSION	0x01
> +
> +#define CXL_PATROL_SCRUB	"cxl_patrol_scrub"
> +
> +/* The default number of regions for CXL memory device patrol scrubber
> + * Patrol scrub is a feature where the device controller scrubs the
> + * memory at a regular interval accroding to the CXL specification.
> + * Hence the number of memory regions to scrub assosiated to the patrol
> + * scrub is 1.
> + */
> +#define CXL_MEMDEV_PATROL_SCRUB_NUM_REGIONS	1
> +
> +static const uuid_t cxl_patrol_scrub_uuid =
> +	UUID_INIT(0x96dad7d6, 0xfde8, 0x482b, 0xa7, 0x33, 0x75, 0x77, 0x4e,     \
> +		  0x06, 0xdb, 0x8a);
> +
> +/* CXL memory patrol scrub control functions */
> +struct cxl_patrol_scrub_context {
> +	struct device *dev;
> +	u16 get_feat_size;
> +	u16 set_feat_size;
> +	bool scrub_cycle_changable;
> +};
> +
> +/**
> + * struct cxl_memdev_ps_params - CXL memory patrol scrub parameter data structure.
> + * @enable:     [IN] enable(1)/disable(0) patrol scrub.
> + * @scrub_cycle_changable: [OUT] scrub cycle attribute of patrol scrub is changeable.
> + * @speed:      [IN] Requested patrol scrub cycle in hours.
> + *              [OUT] Current patrol scrub cycle in hours.
> + * @min_speed:[OUT] minimum patrol scrub cycle, in hours, supported.
> + * @speed_avail:[OUT] Supported patrol scrub cycle in hours.
> + */
> +struct cxl_memdev_ps_params {
> +	bool enable;
> +	bool scrub_cycle_changable;
> +	u16 speed;
> +	u16 min_speed;
> +	char speed_avail[CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH];
> +};
> +
> +enum {
> +	CXL_MEMDEV_PS_PARAM_ENABLE = 0,
> +	CXL_MEMDEV_PS_PARAM_SPEED,
> +};
> +
> +#define	CXL_MEMDEV_PS_SCRUB_CYCLE_CHANGE_CAP_MASK	BIT(0)
> +#define	CXL_MEMDEV_PS_SCRUB_CYCLE_REALTIME_REPORT_CAP_MASK	BIT(1)
> +#define	CXL_MEMDEV_PS_CUR_SCRUB_CYCLE_MASK	GENMASK(7, 0)
> +#define	CXL_MEMDEV_PS_MIN_SCRUB_CYCLE_MASK	GENMASK(15, 8)
> +#define	CXL_MEMDEV_PS_FLAG_ENABLED_MASK	BIT(0)
> +
> +struct cxl_memdev_ps_feat_read_attrbs {
> +	u8 scrub_cycle_cap;
> +	__le16 scrub_cycle;
> +	u8 scrub_flags;
> +}  __packed;
> +
> +struct cxl_memdev_ps_set_feat_pi {
> +	struct cxl_mbox_set_feat_in pi;
> +	u8 scrub_cycle_hr;
> +	u8 scrub_flags;
> +}  __packed;
> +
> +static int cxl_mem_ps_get_attrbs(struct device *dev,
> +				 struct cxl_memdev_ps_params *params)
> +{
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> +	struct cxl_mbox_get_feat_in pi = {
> +		.uuid = cxl_patrol_scrub_uuid,
> +		.offset = 0,
> +		.count = sizeof(struct cxl_memdev_ps_feat_read_attrbs),
> +		.selection = CXL_GET_FEAT_CURRENT_VALUE,
> +	};
> +	struct cxl_memdev_ps_feat_read_attrbs *rd_attrbs;
> +
> +	if (!mds)
> +		return -EFAULT;
> +
> +	rd_attrbs = cxl_get_feature(mds, &pi);
> +	if (PTR_ERR_OR_ZERO(rd_attrbs)) {
> +		params->enable = 0;
> +		params->speed = 0;
> +		snprintf(params->speed_avail, CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH,
> +			"Unavailable");
> +		return PTR_ERR_OR_ZERO(rd_attrbs);
> +	}
> +	params->scrub_cycle_changable = FIELD_GET(CXL_MEMDEV_PS_SCRUB_CYCLE_CHANGE_CAP_MASK,
> +						  rd_attrbs->scrub_cycle_cap);
> +	params->enable = FIELD_GET(CXL_MEMDEV_PS_FLAG_ENABLED_MASK,
> +				   rd_attrbs->scrub_flags);
> +	params->speed = FIELD_GET(CXL_MEMDEV_PS_CUR_SCRUB_CYCLE_MASK,
> +				  rd_attrbs->scrub_cycle);
> +	params->min_speed  = FIELD_GET(CXL_MEMDEV_PS_MIN_SCRUB_CYCLE_MASK,
> +				       rd_attrbs->scrub_cycle);
> +	snprintf(params->speed_avail, CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH,
> +		 "Minimum scrub cycle = %d hour", params->min_speed);
> +	kvfree(rd_attrbs);
> +
> +	return 0;
> +}
> +
> +static int cxl_mem_ps_set_attrbs(struct device *dev,
> +				 struct cxl_memdev_ps_params *params, u8 param_type)
> +{
> +	int ret;
> +	struct cxl_memdev_ps_params rd_params;
> +	struct cxl_memdev_ps_set_feat_pi set_pi = {
> +		.pi.uuid = cxl_patrol_scrub_uuid,
> +		.pi.flags = CXL_SET_FEAT_FLAG_MOD_VALUE_SAVED_ACROSS_RESET |
> +			    CXL_SET_FEAT_FLAG_ACTION_FULL_DATA_TRANSFER,
> +		.pi.offset = 0,
> +		.pi.version = CXL_MEMDEV_PS_SET_FEAT_VERSION,
> +	};
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> +
> +	if (!mds)
> +		return -EFAULT;
> +
> +	ret = 0;

Why set to 0 and then overwrite it in the next line?

> +	ret = cxl_mem_ps_get_attrbs(dev, &rd_params);
> +	if (ret) {
> +		dev_err(dev, "Get cxlmemdev patrol scrub params fail ret=%d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	if (param_type == CXL_MEMDEV_PS_PARAM_ENABLE) {
> +		set_pi.scrub_flags = FIELD_PREP(CXL_MEMDEV_PS_FLAG_ENABLED_MASK,
> +						   params->enable);
> +		set_pi.scrub_cycle_hr = FIELD_PREP(CXL_MEMDEV_PS_CUR_SCRUB_CYCLE_MASK,
> +						      rd_params.speed);
> +	} else if (param_type == CXL_MEMDEV_PS_PARAM_SPEED) {
> +		if (params->speed < rd_params.min_speed) {
> +			dev_err(dev, "Invalid CXL patrol scrub cycle(%d) to set\n",
> +				params->speed);
> +			dev_err(dev, "Minimum supported CXL patrol scrub cycle in hour %d\n",
> +			       params->min_speed);
> +			return -EINVAL;
> +		}
> +		set_pi.scrub_cycle_hr = FIELD_PREP(CXL_MEMDEV_PS_CUR_SCRUB_CYCLE_MASK,
> +						      params->speed);
> +		set_pi.scrub_flags = FIELD_PREP(CXL_MEMDEV_PS_FLAG_ENABLED_MASK,
> +						   rd_params.enable);
> +	} else {
> +		dev_err(dev, "Invalid CXL patrol scrub parameter to set\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = 0;

unnecessary init?

> +	ret = cxl_set_feature(mds, &set_pi, sizeof(set_pi));
> +	if (ret) {
> +		dev_err(dev, "CXL patrol scrub set feature fail ret=%d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	/* Verify attribute set successfully */
> +	if (param_type == CXL_MEMDEV_PS_PARAM_SPEED) {
> +		ret = cxl_mem_ps_get_attrbs(dev, &rd_params);
> +		if (ret) {
> +			dev_err(dev, "Get cxlmemdev patrol scrub params fail ret=%d\n", ret);
> +			return ret;
> +		}
> +		if (rd_params.speed != params->speed)
> +			return -EFAULT;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cxl_mem_ps_enable_write(struct device *dev, long val)
> +{
> +	int ret;
> +	struct cxl_memdev_ps_params params;
> +
> +	params.enable = val;
> +	ret = cxl_mem_ps_set_attrbs(dev, &params, CXL_MEMDEV_PS_PARAM_ENABLE);
> +	if (ret) {
> +		dev_err(dev, "CXL patrol scrub enable fail, enable=%d ret=%d\n",
> +		       params.enable, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cxl_mem_ps_speed_read(struct device *dev, u64 *val)
> +{
> +	int ret;
> +	struct cxl_memdev_ps_params params;
> +
> +	ret = cxl_mem_ps_get_attrbs(dev, &params);
> +	if (ret) {
> +		dev_err(dev, "Get CXL patrol scrub params fail ret=%d\n",
> +			ret);
> +		return ret;
> +	}
> +	*val = params.speed;
> +
> +	return 0;
> +}
> +
> +static int cxl_mem_ps_speed_write(struct device *dev, long val)
> +{
> +	int ret;
> +	struct cxl_memdev_ps_params params;
> +
> +	params.speed = val;
> +	ret = cxl_mem_ps_set_attrbs(dev, &params, CXL_MEMDEV_PS_PARAM_SPEED);
> +	if (ret) {
> +		dev_err(dev, "Set CXL patrol scrub params for speed fail ret=%d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cxl_mem_ps_speed_available_read(struct device *dev, char *buf)
> +{
> +	int ret;
> +	struct cxl_memdev_ps_params params;
> +
> +	ret = cxl_mem_ps_get_attrbs(dev, &params);
> +	if (ret) {
> +		dev_err(dev, "Get CXL patrol scrub params fail ret=%d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	sprintf(buf, "%s\n", params.speed_avail);
> +
> +	return 0;
> +}
> +
> +/**
> + * cxl_mem_patrol_scrub_is_visible() - Callback to return attribute visibility
> + * @drv_data: Pointer to driver-private data structure passed
> + *	      as argument to devm_scrub_device_register().
> + * @attr: Scrub attribute
> + * @region_id: ID of the memory region
> + *
> + * Returns: 0 on success, an error otherwise
> + */
> +umode_t cxl_mem_patrol_scrub_is_visible(const void *drv_data, u32 attr, int region_id)
> +{
> +	const struct cxl_patrol_scrub_context *cxl_ps_ctx = drv_data;
> +
> +	if (attr == scrub_speed_available ||
> +	    attr == scrub_speed) {
> +		if (!cxl_ps_ctx->scrub_cycle_changable)
> +			return 0;
> +	}
> +
> +	switch (attr) {
> +	case scrub_speed_available:
> +		return 0444;
> +	case scrub_enable:
> +		return 0200;
> +	case scrub_speed:
> +		return 0644;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +/**
> + * cxl_mem_patrol_scrub_read() - Read callback for data attributes
> + * @dev: Pointer to scrub device
> + * @attr: Scrub attribute
> + * @region_id: ID of the memory region
> + * @val: Pointer to the returned data
> + *
> + * Returns: 0 on success, an error otherwise
> + */
> +int cxl_mem_patrol_scrub_read(struct device *dev, u32 attr, int region_id, u64 *val)
> +{
> +
> +	switch (attr) {
> +	case scrub_speed:
> +		return cxl_mem_ps_speed_read(dev->parent, val);
> +	default:
> +		return -ENOTSUPP;
> +	}
> +}
> +
> +/**
> + * cxl_mem_patrol_scrub_write() - Write callback for data attributes
> + * @dev: Pointer to scrub device
> + * @attr: Scrub attribute
> + * @region_id: ID of the memory region
> + * @val: Value to write
> + *
> + * Returns: 0 on success, an error otherwise
> + */
> +int cxl_mem_patrol_scrub_write(struct device *dev, u32 attr, int region_id, u64 val)
> +{
> +	switch (attr) {
> +	case scrub_enable:
> +		return cxl_mem_ps_enable_write(dev->parent, val);
> +	case scrub_speed:
> +		return cxl_mem_ps_speed_write(dev->parent, val);
> +	default:
> +		return -ENOTSUPP;
> +	}
> +}
> +
> +/**
> + * cxl_mem_patrol_scrub_read_strings() - Read callback for string attributes
> + * @dev: Pointer to scrub device
> + * @attr: Scrub attribute
> + * @region_id: ID of the memory region
> + * @buf: Pointer to the buffer for copying returned string
> + *
> + * Returns: 0 on success, an error otherwise
> + */
> +int cxl_mem_patrol_scrub_read_strings(struct device *dev, u32 attr, int region_id,
> +				      char *buf)
> +{
> +	switch (attr) {
> +	case scrub_speed_available:
> +		return cxl_mem_ps_speed_available_read(dev->parent, buf);
> +	default:
> +		return -ENOTSUPP;
> +	}
> +}
> +
> +static const struct scrub_ops cxl_ps_scrub_ops = {
> +	.is_visible = cxl_mem_patrol_scrub_is_visible,
> +	.read = cxl_mem_patrol_scrub_read,
> +	.write = cxl_mem_patrol_scrub_write,
> +	.read_string = cxl_mem_patrol_scrub_read_strings,
> +};
> +
> +int cxl_mem_patrol_scrub_init(struct cxl_memdev *cxlmd)
> +{
> +	int ret = 0;
> +	struct device *cxl_scrub_dev;
> +	struct cxl_memdev_ps_params params;
> +	struct cxl_mbox_supp_feat_entry feat_entry;
> +	char scrub_name[CXL_MEMDEV_MAX_NAME_LENGTH];
> +	struct cxl_patrol_scrub_context *cxl_ps_ctx;
> +
> +	ret = cxl_mem_get_supported_feature_entry(cxlmd, &cxl_patrol_scrub_uuid,
> +						  &feat_entry);
> +	if (ret < 0)
> +		goto cxl_ps_init_exit;
> +
> +	if (!(feat_entry.attrb_flags & CXL_FEAT_ENTRY_FLAG_CHANGABLE))
> +		goto cxl_ps_init_exit;
> +
> +	cxl_ps_ctx = devm_kzalloc(&cxlmd->dev, sizeof(*cxl_ps_ctx), GFP_KERNEL);
> +	if (!cxl_ps_ctx)
> +		goto cxl_ps_init_exit;
> +
> +	cxl_ps_ctx->get_feat_size = feat_entry.get_feat_size;
> +	cxl_ps_ctx->set_feat_size = feat_entry.set_feat_size;
> +	ret = cxl_mem_ps_get_attrbs(&cxlmd->dev, &params);
> +	if (ret) {
> +		dev_err(&cxlmd->dev, "Get CXL patrol scrub params fail ret=%d\n",
> +			ret);
> +		goto cxl_ps_init_exit;
> +	}
> +	cxl_ps_ctx->scrub_cycle_changable =  params.scrub_cycle_changable;
> +
> +	snprintf(scrub_name, sizeof(scrub_name), "%s_%s",
> +		 CXL_PATROL_SCRUB, dev_name(&cxlmd->dev));
> +	cxl_scrub_dev = devm_scrub_device_register(&cxlmd->dev, scrub_name,
> +						   cxl_ps_ctx, &cxl_ps_scrub_ops,
> +						   CXL_MEMDEV_PATROL_SCRUB_NUM_REGIONS);
> +	if (PTR_ERR_OR_ZERO(cxl_scrub_dev)) {
> +		ret = PTR_ERR_OR_ZERO(cxl_scrub_dev);
> +		goto cxl_ps_reg_fail;
> +	}
> +
> +cxl_ps_reg_fail:
> +cxl_ps_init_exit:
> +	return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_mem_patrol_scrub_init, CXL);
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index fdac686560d4..1d0fad0dc5ae 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -969,6 +969,13 @@ int cxl_trigger_poison_list(struct cxl_memdev *cxlmd);
>  int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa);
>  int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa);
>  
> +/* cxl memory scrub functions */
> +#ifdef CONFIG_CXL_SCRUB
> +int cxl_mem_patrol_scrub_init(struct cxl_memdev *cxlmd);
> +#else
> +int cxl_mem_patrol_scrub_init(struct cxl_memdev *cxlmd) { return -ENOTSUP; }
> +#endif
> +
>  #ifdef CONFIG_CXL_SUSPEND
>  void cxl_mem_active_inc(void);
>  void cxl_mem_active_dec(void);
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 0155fb66b580..86bba8794bb4 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -881,6 +881,12 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (rc)
>  		return rc;
>  
> +	/*
> +	 * Initialize optional CXL scrub features
> +	 */
> +	if (cxl_mem_patrol_scrub_init(cxlmd))
> +		dev_dbg(&pdev->dev, "cxl_mem_patrol_scrub_init failed\n");
> +
>  	rc = devm_cxl_sanitize_setup_notifier(&pdev->dev, cxlmd);
>  	if (rc)
>  		return rc;

RE: [RFC PATCH 1/6] cxl/mbox: Add GET_SUPPORTED_FEATURES, GET_FEATURE and SET_FEATURE mailbox commands

[Shiju Jose]

Hi Dave,

Thanks for the feedbacks.

>-----Original Message-----
>From: Dave Jiang <dave.jiang@intel.com>
>Sent: 15 November 2023 18:21
>To: Shiju Jose <shiju.jose@huawei.com>; linux-cxl@vger.kernel.org
>Cc: Jonathan Cameron <jonathan.cameron@huawei.com>;
>Vilas.Sridharan@amd.com; leo.duran@amd.com; Yazen.Ghannam@amd.com;
>rientjes@google.com; jiaqiyan@google.com; tony.luck@intel.com;
>Jon.Grimm@amd.com; dave.hansen@linux.intel.com; linux-
>acpi@vger.kernel.org; linux-mm@kvack.org; linux-kernel@vger.kernel.org;
>rafael@kernel.org; lenb@kernel.org; naoya.horiguchi@nec.com;
>james.morse@arm.com; david@redhat.com; jthoughton@google.com;
>somasundaram.a@hpe.com; erdemaktas@google.com;
>"pgonda@pgonda"@google.com; duenwen@google.com;
>mike.malvestuto@intel.com; gthelen@google.com; tanxiaofei
><tanxiaofei@huawei.com>; Zengtao (B) <prime.zeng@hisilicon.com>;
>kangkang.shen@futurewei.com; wanghuiqiang <wanghuiqiang@huawei.com>;
>Linuxarm <linuxarm@huawei.com>
>Subject: Re: [RFC PATCH 1/6] cxl/mbox: Add GET_SUPPORTED_FEATURES,
>GET_FEATURE and SET_FEATURE mailbox commands
>
>
>
>On 11/14/23 05:56, shiju.jose@huawei.com wrote:
>> From: Shiju Jose <shiju.jose@huawei.com>
>>
>> Add support for GET_SUPPORTED_FEATURES, GET_FEATURE and
>SET_FEATURE
>> mailbox commands.
>>
>> CXL spec 3.0 section 8.2.9.6 describes optional device specific features.
>> CXL devices supports features with changeable attributes.
>> Get Supported Features retrieves the list of supported device specific
>> features. The settings of a feature can be retrieved using Get Feature
>> and optionally modified using Set Feature.
>
>Maybe split this patch out into 3 for each of the individual commands you are
>enabling. That'll make git bisect easier for debugging.
Sure. I will do this.

>
>A few nits but the rest LGTM.
>
>>
>> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
>> ---
>>  drivers/cxl/core/mbox.c      | 74 ++++++++++++++++++++++++++++
>>  drivers/cxl/cxlmem.h         | 95 ++++++++++++++++++++++++++++++++++++
>>  include/uapi/linux/cxl_mem.h |  3 ++
>>  3 files changed, 172 insertions(+)
>>
>> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index
>> 36270dcfb42e..2a6d8ab927bd 100644
>> --- a/drivers/cxl/core/mbox.c
>> +++ b/drivers/cxl/core/mbox.c
>> @@ -63,6 +63,9 @@ static struct cxl_mem_command
>cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
>>  	CXL_CMD(GET_SHUTDOWN_STATE, 0, 0x1, 0),
>>  	CXL_CMD(SET_SHUTDOWN_STATE, 0x1, 0, 0),
>>  	CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0),
>> +	CXL_CMD(GET_SUPPORTED_FEATURES, 0x8, CXL_VARIABLE_PAYLOAD,
>0),
>> +	CXL_CMD(GET_FEATURE, 0x15, CXL_VARIABLE_PAYLOAD, 0),
>> +	CXL_CMD(SET_FEATURE, CXL_VARIABLE_PAYLOAD, 0, 0),
>>  };
>>
>>  /*
>> @@ -1303,6 +1306,77 @@ int cxl_set_timestamp(struct cxl_memdev_state
>> *mds)  }  EXPORT_SYMBOL_NS_GPL(cxl_set_timestamp, CXL);
>>
>> +struct cxl_mbox_get_supp_feats_out *cxl_get_supported_features(struct
>cxl_memdev_state *mds,
>> +						struct
>cxl_mbox_get_supp_feats_in *pi) {
>> +	int rc;
>> +	struct cxl_mbox_cmd mbox_cmd;
>> +	struct cxl_mbox_get_supp_feats_out *feats_out;
>
>reverse xmas tree declarations
Sure. Will change.

>
>> +
>> +	feats_out = kvmalloc(pi->count, GFP_KERNEL);
>> +	if (!feats_out)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	mbox_cmd = (struct cxl_mbox_cmd) {
>> +		.opcode = CXL_MBOX_OP_GET_SUPPORTED_FEATURES,
>> +		.size_in = sizeof(*pi),
>> +		.payload_in = pi,
>> +		.size_out = pi->count,
>> +		.payload_out = feats_out,
>> +		.min_out = sizeof(struct cxl_mbox_get_supp_feats_out),
>> +	};
>> +	rc = cxl_internal_send_cmd(mds, &mbox_cmd);
>> +	if (rc < 0) {
>> +		kvfree(feats_out);
>> +		return ERR_PTR(rc);
>> +	}
>> +
>> +	return feats_out;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_get_supported_features, CXL);
>> +
>> +void *cxl_get_feature(struct cxl_memdev_state *mds, struct
>> +cxl_mbox_get_feat_in *pi) {
>> +	int rc;
>> +	void *feat_out;
>> +	struct cxl_mbox_cmd mbox_cmd;
>
>reverse xmas tree declarations
Will change.

>
>
>> +
>> +	feat_out = kvmalloc(pi->count, GFP_KERNEL);
>> +	if (!feat_out)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	mbox_cmd = (struct cxl_mbox_cmd) {
>> +		.opcode = CXL_MBOX_OP_GET_FEATURE,
>> +		.size_in = sizeof(*pi),
>> +		.payload_in = pi,
>> +		.size_out = pi->count,
>> +		.payload_out = feat_out,
>> +		.min_out = pi->count,
>> +	};
>> +	rc = cxl_internal_send_cmd(mds, &mbox_cmd);
>> +	if (rc < 0) {
>> +		kvfree(feat_out);
>> +		return ERR_PTR(rc);
>> +	}
>> +
>> +	return feat_out;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_get_feature, CXL);
>> +
>> +int cxl_set_feature(struct cxl_memdev_state *mds, void *feat_in,
>> +size_t size) {
>> +	struct cxl_mbox_cmd mbox_cmd;
>> +
>> +	mbox_cmd = (struct cxl_mbox_cmd) {
>> +		.opcode = CXL_MBOX_OP_SET_FEATURE,
>> +		.size_in = size,
>> +		.payload_in = feat_in,
>> +	};
>> +
>> +	return cxl_internal_send_cmd(mds, &mbox_cmd); }
>> +EXPORT_SYMBOL_NS_GPL(cxl_set_feature, CXL);
>> +
>>  int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
>>  		       struct cxl_region *cxlr)
>>  {
>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index
>> a2fcbca253f3..fdac686560d4 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -506,6 +506,9 @@ enum cxl_opcode {
>>  	CXL_MBOX_OP_SET_TIMESTAMP	= 0x0301,
>>  	CXL_MBOX_OP_GET_SUPPORTED_LOGS	= 0x0400,
>>  	CXL_MBOX_OP_GET_LOG		= 0x0401,
>> +	CXL_MBOX_OP_GET_SUPPORTED_FEATURES	= 0x0500,
>> +	CXL_MBOX_OP_GET_FEATURE		= 0x0501,
>> +	CXL_MBOX_OP_SET_FEATURE		= 0x0502,
>>  	CXL_MBOX_OP_IDENTIFY		= 0x4000,
>>  	CXL_MBOX_OP_GET_PARTITION_INFO	= 0x4100,
>>  	CXL_MBOX_OP_SET_PARTITION_INFO	= 0x4101,
>> @@ -740,6 +743,94 @@ struct cxl_mbox_set_timestamp_in {
>>
>>  } __packed;
>>
>> +/* Get Supported Features CXL 3.0 Spec 8.2.9.6.1 */
>> +/*
>> + * Get Supported Features input payload
>> + * CXL rev 3.0 section 8.2.9.6.1; Table 8-75  */ struct
>> +cxl_mbox_get_supp_feats_in {
>> +	__le32 count;
>> +	__le16 start_index;
>> +	u16 reserved;
>> +} __packed;
>> +
>> +/*
>> + * Get Supported Features Supported Feature Entry
>> + * CXL rev 3.0 section 8.2.9.6.1; Table 8-77  */
>> +/* Supported Feature Entry : Payload out attribute flags */
>> +#define CXL_FEAT_ENTRY_FLAG_CHANGABLE	BIT(0)
>> +#define CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_MASK
>	GENMASK(3, 1)
>> +#define CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_NONE
>	0x0
>> +#define CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_CXL_RESET
>	0x1
>> +#define CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_HOT_RESET
>	0x2
>> +#define
>CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_WARM_RESET	0x3
>> +#define
>CXL_FEAT_ENTRY_FLAG_DEEPEST_RESET_PERSISTENCE_COLD_RESET	0x4
>
>I think you can declare the flags as enums?
Sure. Will check.

>
>> +#define CXL_FEAT_ENTRY_FLAG_PERSISTENCE_ACROSS_FW_UPDATE_MASK
>	BIT(4)
>> +#define
>CXL_FEAT_ENTRY_FLAG_PERSISTENCE_DEFAULT_SEL_SUPPORT_MASK	BIT(5)
>> +#define CXL_FEAT_ENTRY_FLAG_PERSISTENCE_SAVED_SEL_SUPPORT_MASK
>	BIT(6)
>> +
>> +struct cxl_mbox_supp_feat_entry {
>> +	uuid_t uuid;
>> +	__le16 feat_index;
>> +	__le16 get_feat_size;
>> +	__le16 set_feat_size;
>> +	__le32 attrb_flags;
>> +	u8 get_feat_version;
>> +	u8 set_feat_version;
>> +	__le16 set_feat_effects;
>> +	u8 rsvd[18];
>> +}  __packed;
>> +
>> +/*
>> + * Get Supported Features output payload
>> + * CXL rev 3.0 section 8.2.9.6.1; Table 8-76  */ struct
>> +cxl_mbox_get_supp_feats_out {
>> +	__le16 entries;
>> +	__le16 nsuppfeats_dev;
>> +	u32 reserved;
>> +	struct cxl_mbox_supp_feat_entry feat_entries[]; } __packed;
>> +
>> +/* Get Feature CXL 3.0 Spec 8.2.9.6.2 */
>> +/*
>> + * Get Feature input payload
>> + * CXL rev 3.0 section 8.2.9.6.2; Table 8-79  */
>> +/* Get Feature : Payload in selection */
>> +#define CXL_GET_FEAT_CURRENT_VALUE	0x00
>> +#define CXL_GET_FEAT_DEFAULT_VALUE	0x01
>> +#define CXL_GET_FEAT_SAVED_VALUE	0x02
>> +
>> +struct cxl_mbox_get_feat_in {
>> +	uuid_t uuid;
>> +	__le16 offset;
>> +	__le16 count;
>> +	u8 selection;
>> +}  __packed;
>> +
>> +/* Set Feature CXL 3.0 Spec 8.2.9.6.3 */
>> +/*
>> + * Set Feature input payload
>> + * CXL rev 3.0 section 8.2.9.6.3; Table 8-81  */
>> +/* Set Feature : Payload in flags */
>> +#define CXL_SET_FEAT_FLAG_ACTION_MASK	GENMASK(2, 0)
>> +#define CXL_SET_FEAT_FLAG_ACTION_FULL_DATA_TRANSFER	0x0
>> +#define CXL_SET_FEAT_FLAG_ACTION_INITIATE_DATA_TRANSFER	0x1
>> +#define CXL_SET_FEAT_FLAG_ACTION_CONTINUE_DATA_TRANSFER	0x2
>> +#define CXL_SET_FEAT_FLAG_ACTION_FINISH_DATA_TRANSFER	0x3
>> +#define CXL_SET_FEAT_FLAG_ACTION_ABORT_DATA_TRANSFER	0x4
>> +#define CXL_SET_FEAT_FLAG_MOD_VALUE_SAVED_ACROSS_RESET	BIT(3)
>> +struct cxl_mbox_set_feat_in {
>> +	uuid_t uuid;
>> +	__le32 flags;
>> +	__le16 offset;
>> +	u8 version;
>> +	u8 rsvd[9];
>> +}  __packed;
>> +
>>  /* Get Poison List  CXL 3.0 Spec 8.2.9.8.4.1 */  struct
>> cxl_mbox_poison_in {
>>  	__le64 offset;
>> @@ -867,6 +958,10 @@ void clear_exclusive_cxl_commands(struct
>cxl_memdev_state *mds,
>>  				  unsigned long *cmds);
>>  void cxl_mem_get_event_records(struct cxl_memdev_state *mds, u32
>> status);  int cxl_set_timestamp(struct cxl_memdev_state *mds);
>> +struct cxl_mbox_get_supp_feats_out *cxl_get_supported_features(struct
>cxl_memdev_state *mds,
>> +						struct
>cxl_mbox_get_supp_feats_in *pi); void
>> +*cxl_get_feature(struct cxl_memdev_state *mds, struct
>> +cxl_mbox_get_feat_in *pi); int cxl_set_feature(struct
>> +cxl_memdev_state *mds, void *feat_in, size_t size);
>>  int cxl_poison_state_init(struct cxl_memdev_state *mds);  int
>> cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
>>  		       struct cxl_region *cxlr);
>> diff --git a/include/uapi/linux/cxl_mem.h
>> b/include/uapi/linux/cxl_mem.h index 14bc6e742148..8c89d323cc41 100644
>> --- a/include/uapi/linux/cxl_mem.h
>> +++ b/include/uapi/linux/cxl_mem.h
>> @@ -46,6 +46,9 @@
>>  	___C(GET_SCAN_MEDIA_CAPS, "Get Scan Media Capabilities"),         \
>>  	___DEPRECATED(SCAN_MEDIA, "Scan Media"),                          \
>>  	___DEPRECATED(GET_SCAN_MEDIA, "Get Scan Media Results"),          \
>> +	___C(GET_SUPPORTED_FEATURES, "Get Supported Features"),           \
>> +	___C(GET_FEATURE, "Get Feature"),                                 \
>> +	___C(SET_FEATURE, "Set Feature"),                                 \
>>  	___C(MAX, "invalid / last command")
>>
>>  #define ___C(a, b) CXL_MEM_COMMAND_ID_##a

Thanks,
Shiju

RE: [RFC PATCH 2/6] cxl/memscrub: Add CXL device patrol scrub control feature

[Shiju Jose]

Hi Dave,

>-----Original Message-----
>From: Dave Jiang <dave.jiang@intel.com>
>Sent: 15 November 2023 21:24
>To: Shiju Jose <shiju.jose@huawei.com>; linux-cxl@vger.kernel.org
>Cc: Jonathan Cameron <jonathan.cameron@huawei.com>;
>Vilas.Sridharan@amd.com; leo.duran@amd.com; Yazen.Ghannam@amd.com;
>rientjes@google.com; jiaqiyan@google.com; tony.luck@intel.com;
>Jon.Grimm@amd.com; dave.hansen@linux.intel.com; linux-
>acpi@vger.kernel.org; linux-mm@kvack.org; linux-kernel@vger.kernel.org;
>rafael@kernel.org; lenb@kernel.org; naoya.horiguchi@nec.com;
>james.morse@arm.com; david@redhat.com; jthoughton@google.com;
>somasundaram.a@hpe.com; erdemaktas@google.com; duenwen@google.com;
>mike.malvestuto@intel.com; gthelen@google.com; tanxiaofei
><tanxiaofei@huawei.com>; Zengtao (B) <prime.zeng@hisilicon.com>;
>kangkang.shen@futurewei.com; wanghuiqiang <wanghuiqiang@huawei.com>;
>Linuxarm <linuxarm@huawei.com>
>Subject: Re: [RFC PATCH 2/6] cxl/memscrub: Add CXL device patrol scrub control
>feature
>
>
>
>On 11/14/23 05:56, shiju.jose@huawei.com wrote:
>> From: Shiju Jose <shiju.jose@huawei.com>
>>
>> CXL spec 3.1 section 8.2.9.9.11.1 describes the device patrol scrub
>> control feature. The device patrol scrub proactively locates and makes
>> corrections to errors in regular cycle. The patrol scrub control
>> allows the request to configure patrol scrub input configurations.
>>
>> The patrol scrub control allows the requester to specify the number of
>> hours for which the patrol scrub cycles must be completed, provided
>> that the requested number is not less than the minimum number of hours
>> for the patrol scrub cycle that the device is capable of. In addition,
>> the patrol scrub controls allow the host to disable and enable the
>> feature in case disabling of the feature is needed for other purposes
>> such as performance-aware operations which require the background
>> operations to be turned off.
>>
>> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
>> ---
>>  drivers/cxl/Kconfig         |  23 ++
>>  drivers/cxl/core/Makefile   |   1 +
>>  drivers/cxl/core/memscrub.c | 455
>++++++++++++++++++++++++++++++++++++
>>  drivers/cxl/cxlmem.h        |   7 +
>>  drivers/cxl/pci.c           |   6 +
>>  5 files changed, 492 insertions(+)
>
>
>Maybe this patch can be split up? Awfully large. Maybe a patch with support
>functions and then another with usages?
Sure. I will check this.

>
>>  create mode 100644 drivers/cxl/core/memscrub.c
>>
>> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig index
>> 8ea1d340e438..45ee6d57d899 100644
>> --- a/drivers/cxl/Kconfig
>> +++ b/drivers/cxl/Kconfig
>> @@ -154,4 +154,27 @@ config CXL_PMU
>>  	  monitoring units and provide standard perf based interfaces.
>>
>>  	  If unsure say 'm'.
>> +
>> +config CXL_SCRUB
>> +	tristate "CXL: Memory scrub feature"
>> +	depends on CXL_PCI
>> +	depends on CXL_MEM
>> +	depends on SCRUB
>> +	help
>> +	  The CXL memory scrub control is an optional feature allows host to
>> +	  control the scrub configurations of CXL Type 3 devices, which
>> +	  support patrol scrub and/or DDR5 ECS(Error Check Scrub).
>> +
>> +	  Register with the scrub configure driver to provide sysfs interfaces
>> +	  for configuring the CXL device memory patrol and DDR5 ECS scrubs.
>> +	  Provides the interface functions support configuring the CXL memory
>> +	  device patrol and ECS scrubs.
>> +
>> +	  Say 'y/m' to enable the CXL memory scrub driver that will attach to
>> +	  CXL.mem devices for memory scrub control feature. See sections
>> +	  8.2.9.9.11.1 and 8.2.9.9.11.2 in the CXL 3.1 specification for a
>> +	  detailed description of CXL memory scrub control features.
>> +
>> +	  If unsure say 'm'.
>> +
>>  endif
>> diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
>> index 1f66b5d4d935..99e3202f868f 100644
>> --- a/drivers/cxl/core/Makefile
>> +++ b/drivers/cxl/core/Makefile
>> @@ -15,3 +15,4 @@ cxl_core-y += hdm.o
>>  cxl_core-y += pmu.o
>>  cxl_core-$(CONFIG_TRACING) += trace.o
>>  cxl_core-$(CONFIG_CXL_REGION) += region.o
>> +cxl_core-$(CONFIG_CXL_SCRUB) += memscrub.o
>> diff --git a/drivers/cxl/core/memscrub.c b/drivers/cxl/core/memscrub.c
>> new file mode 100644 index 000000000000..ec67ffc81363
>> --- /dev/null
>> +++ b/drivers/cxl/core/memscrub.c
>> @@ -0,0 +1,455 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * cxl_memscrub.c - CXL memory scrub driver
>> + *
>> + * Copyright (c) 2023 HiSilicon Limited.
>> + *
>> + *  - Provides functions to configure patrol scrub
>> + *    feature of the CXL memory devices.
>> + *  - Registers with the scrub driver for the
>> + *    memory patrol scrub feature.
>> + */
>> +
>> +#define pr_fmt(fmt)	"CXL_MEM_SCRUB: " fmt
>> +
>> +#include <cxlmem.h>
>> +#include <memory/memory-scrub.h>
>> +
>> +/* CXL memory scrub feature common definitions */
>> +#define CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH	128
>> +#define CXL_MEMDEV_MAX_NAME_LENGTH	128
>> +
>> +static int cxl_mem_get_supported_feature_entry(struct cxl_memdev *cxlmd,
>const uuid_t *feat_uuid,
>> +					       struct cxl_mbox_supp_feat_entry
>*feat_entry_out) {
>> +	int nentries; /* number of supported feature entries in output payload */
>> +	int feat_index, count;
>> +	bool is_support_feature = false;
>> +	struct cxl_mbox_get_supp_feats_in pi;
>> +	struct cxl_mbox_supp_feat_entry *feat_entry;
>> +	struct cxl_mbox_get_supp_feats_out *feats_out;
>> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>> +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
>> +
>> +	feat_index = 0;
>> +	do {
>> +		pi.count = sizeof(struct cxl_mbox_get_supp_feats_out) +
>> +				  sizeof(struct cxl_mbox_supp_feat_entry);
>> +		pi.start_index = feat_index;
>> +		nentries = 0;
>
>Is this needed since you init it to feats_out->entries a few lines below
Will remove.

>
>> +		feats_out = cxl_get_supported_features(mds, &pi);
>> +		if (PTR_ERR_OR_ZERO(feats_out))
>> +			return  PTR_ERR_OR_ZERO(feats_out);
>> +		nentries = feats_out->entries;
>> +		if (!nentries) {
>> +			kvfree(feats_out);
>> +			break;
>> +		}
>> +		/* Check CXL memdev supports the feature */
>> +		feat_entry = (void *)feats_out->feat_entries;
>> +		for (count = 0; count < nentries; count++, feat_entry++) {
>> +			if (uuid_equal(&feat_entry->uuid, feat_uuid)) {
>> +				is_support_feature = true;
>> +				memcpy(feat_entry_out, feat_entry,
>sizeof(*feat_entry_out));
>> +				break;
>> +			}
>> +		}
>> +		kvfree(feats_out);
>> +		if (is_support_feature)
>> +			break;
>> +		feat_index += nentries;
>> +	} while (nentries);
>> +
>> +	if (!is_support_feature)
>> +		return -ENOTSUPP;
>> +
>> +	return 0;
>> +}
>> +
>> +/* CXL memory patrol scrub control definitions */
>> +#define CXL_MEMDEV_PS_GET_FEAT_VERSION	0x01
>> +#define CXL_MEMDEV_PS_SET_FEAT_VERSION	0x01
>> +
>> +#define CXL_PATROL_SCRUB	"cxl_patrol_scrub"
>> +
>> +/* The default number of regions for CXL memory device patrol
>> +scrubber
>> + * Patrol scrub is a feature where the device controller scrubs the
>> + * memory at a regular interval accroding to the CXL specification.
>> + * Hence the number of memory regions to scrub assosiated to the
>> +patrol
>> + * scrub is 1.
>> + */
>> +#define CXL_MEMDEV_PATROL_SCRUB_NUM_REGIONS	1
>> +
>> +static const uuid_t cxl_patrol_scrub_uuid =
>> +	UUID_INIT(0x96dad7d6, 0xfde8, 0x482b, 0xa7, 0x33, 0x75, 0x77, 0x4e,
>\
>> +		  0x06, 0xdb, 0x8a);
>> +
>> +/* CXL memory patrol scrub control functions */ struct
>> +cxl_patrol_scrub_context {
>> +	struct device *dev;
>> +	u16 get_feat_size;
>> +	u16 set_feat_size;
>> +	bool scrub_cycle_changable;
>> +};
>> +
>> +/**
>> + * struct cxl_memdev_ps_params - CXL memory patrol scrub parameter data
>structure.
>> + * @enable:     [IN] enable(1)/disable(0) patrol scrub.
>> + * @scrub_cycle_changable: [OUT] scrub cycle attribute of patrol scrub is
>changeable.
>> + * @speed:      [IN] Requested patrol scrub cycle in hours.
>> + *              [OUT] Current patrol scrub cycle in hours.
>> + * @min_speed:[OUT] minimum patrol scrub cycle, in hours, supported.
>> + * @speed_avail:[OUT] Supported patrol scrub cycle in hours.
>> + */
>> +struct cxl_memdev_ps_params {
>> +	bool enable;
>> +	bool scrub_cycle_changable;
>> +	u16 speed;
>> +	u16 min_speed;
>> +	char speed_avail[CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH];
>> +};
>> +
>> +enum {
>> +	CXL_MEMDEV_PS_PARAM_ENABLE = 0,
>> +	CXL_MEMDEV_PS_PARAM_SPEED,
>> +};
>> +
>> +#define	CXL_MEMDEV_PS_SCRUB_CYCLE_CHANGE_CAP_MASK	BIT(0)
>> +#define
>	CXL_MEMDEV_PS_SCRUB_CYCLE_REALTIME_REPORT_CAP_MASK
>	BIT(1)
>> +#define	CXL_MEMDEV_PS_CUR_SCRUB_CYCLE_MASK	GENMASK(7, 0)
>> +#define	CXL_MEMDEV_PS_MIN_SCRUB_CYCLE_MASK	GENMASK(15,
>8)
>> +#define	CXL_MEMDEV_PS_FLAG_ENABLED_MASK	BIT(0)
>> +
>> +struct cxl_memdev_ps_feat_read_attrbs {
>> +	u8 scrub_cycle_cap;
>> +	__le16 scrub_cycle;
>> +	u8 scrub_flags;
>> +}  __packed;
>> +
>> +struct cxl_memdev_ps_set_feat_pi {
>> +	struct cxl_mbox_set_feat_in pi;
>> +	u8 scrub_cycle_hr;
>> +	u8 scrub_flags;
>> +}  __packed;
>> +
>> +static int cxl_mem_ps_get_attrbs(struct device *dev,
>> +				 struct cxl_memdev_ps_params *params) {
>> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>> +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
>> +	struct cxl_mbox_get_feat_in pi = {
>> +		.uuid = cxl_patrol_scrub_uuid,
>> +		.offset = 0,
>> +		.count = sizeof(struct cxl_memdev_ps_feat_read_attrbs),
>> +		.selection = CXL_GET_FEAT_CURRENT_VALUE,
>> +	};
>> +	struct cxl_memdev_ps_feat_read_attrbs *rd_attrbs;
>> +
>> +	if (!mds)
>> +		return -EFAULT;
>> +
>> +	rd_attrbs = cxl_get_feature(mds, &pi);
>> +	if (PTR_ERR_OR_ZERO(rd_attrbs)) {
>> +		params->enable = 0;
>> +		params->speed = 0;
>> +		snprintf(params->speed_avail,
>CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH,
>> +			"Unavailable");
>> +		return PTR_ERR_OR_ZERO(rd_attrbs);
>> +	}
>> +	params->scrub_cycle_changable =
>FIELD_GET(CXL_MEMDEV_PS_SCRUB_CYCLE_CHANGE_CAP_MASK,
>> +						  rd_attrbs->scrub_cycle_cap);
>> +	params->enable =
>FIELD_GET(CXL_MEMDEV_PS_FLAG_ENABLED_MASK,
>> +				   rd_attrbs->scrub_flags);
>> +	params->speed =
>FIELD_GET(CXL_MEMDEV_PS_CUR_SCRUB_CYCLE_MASK,
>> +				  rd_attrbs->scrub_cycle);
>> +	params->min_speed  =
>FIELD_GET(CXL_MEMDEV_PS_MIN_SCRUB_CYCLE_MASK,
>> +				       rd_attrbs->scrub_cycle);
>> +	snprintf(params->speed_avail,
>CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH,
>> +		 "Minimum scrub cycle = %d hour", params->min_speed);
>> +	kvfree(rd_attrbs);
>> +
>> +	return 0;
>> +}
>> +
>> +static int cxl_mem_ps_set_attrbs(struct device *dev,
>> +				 struct cxl_memdev_ps_params *params, u8
>param_type) {
>> +	int ret;
>> +	struct cxl_memdev_ps_params rd_params;
>> +	struct cxl_memdev_ps_set_feat_pi set_pi = {
>> +		.pi.uuid = cxl_patrol_scrub_uuid,
>> +		.pi.flags =
>CXL_SET_FEAT_FLAG_MOD_VALUE_SAVED_ACROSS_RESET |
>> +
>CXL_SET_FEAT_FLAG_ACTION_FULL_DATA_TRANSFER,
>> +		.pi.offset = 0,
>> +		.pi.version = CXL_MEMDEV_PS_SET_FEAT_VERSION,
>> +	};
>> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>> +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
>> +
>> +	if (!mds)
>> +		return -EFAULT;
>> +
>> +	ret = 0;
>
>Why set to 0 and then overwrite it in the next line?
Will remove.

>
>> +	ret = cxl_mem_ps_get_attrbs(dev, &rd_params);
>> +	if (ret) {
>> +		dev_err(dev, "Get cxlmemdev patrol scrub params fail
>ret=%d\n",
>> +			ret);
>> +		return ret;
>> +	}
>> +
>> +	if (param_type == CXL_MEMDEV_PS_PARAM_ENABLE) {
>> +		set_pi.scrub_flags =
>FIELD_PREP(CXL_MEMDEV_PS_FLAG_ENABLED_MASK,
>> +						   params->enable);
>> +		set_pi.scrub_cycle_hr =
>FIELD_PREP(CXL_MEMDEV_PS_CUR_SCRUB_CYCLE_MASK,
>> +						      rd_params.speed);
>> +	} else if (param_type == CXL_MEMDEV_PS_PARAM_SPEED) {
>> +		if (params->speed < rd_params.min_speed) {
>> +			dev_err(dev, "Invalid CXL patrol scrub cycle(%d) to
>set\n",
>> +				params->speed);
>> +			dev_err(dev, "Minimum supported CXL patrol scrub
>cycle in hour %d\n",
>> +			       params->min_speed);
>> +			return -EINVAL;
>> +		}
>> +		set_pi.scrub_cycle_hr =
>FIELD_PREP(CXL_MEMDEV_PS_CUR_SCRUB_CYCLE_MASK,
>> +						      params->speed);
>> +		set_pi.scrub_flags =
>FIELD_PREP(CXL_MEMDEV_PS_FLAG_ENABLED_MASK,
>> +						   rd_params.enable);
>> +	} else {
>> +		dev_err(dev, "Invalid CXL patrol scrub parameter to set\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = 0;
>
>unnecessary init?
Ok. Will remove.

>
>> +	ret = cxl_set_feature(mds, &set_pi, sizeof(set_pi));
>> +	if (ret) {
>> +		dev_err(dev, "CXL patrol scrub set feature fail ret=%d\n",
>> +			ret);
>> +		return ret;
>> +	}
>> +
>> +	/* Verify attribute set successfully */
>> +	if (param_type == CXL_MEMDEV_PS_PARAM_SPEED) {
>> +		ret = cxl_mem_ps_get_attrbs(dev, &rd_params);
>> +		if (ret) {
>> +			dev_err(dev, "Get cxlmemdev patrol scrub params fail
>ret=%d\n", ret);
>> +			return ret;
>> +		}
>> +		if (rd_params.speed != params->speed)
>> +			return -EFAULT;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int cxl_mem_ps_enable_write(struct device *dev, long val) {
>> +	int ret;
>> +	struct cxl_memdev_ps_params params;
>> +
>> +	params.enable = val;
>> +	ret = cxl_mem_ps_set_attrbs(dev, &params,
>CXL_MEMDEV_PS_PARAM_ENABLE);
>> +	if (ret) {
>> +		dev_err(dev, "CXL patrol scrub enable fail, enable=%d
>ret=%d\n",
>> +		       params.enable, ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int cxl_mem_ps_speed_read(struct device *dev, u64 *val) {
>> +	int ret;
>> +	struct cxl_memdev_ps_params params;
>> +
>> +	ret = cxl_mem_ps_get_attrbs(dev, &params);
>> +	if (ret) {
>> +		dev_err(dev, "Get CXL patrol scrub params fail ret=%d\n",
>> +			ret);
>> +		return ret;
>> +	}
>> +	*val = params.speed;
>> +
>> +	return 0;
>> +}
>> +
>> +static int cxl_mem_ps_speed_write(struct device *dev, long val) {
>> +	int ret;
>> +	struct cxl_memdev_ps_params params;
>> +
>> +	params.speed = val;
>> +	ret = cxl_mem_ps_set_attrbs(dev, &params,
>CXL_MEMDEV_PS_PARAM_SPEED);
>> +	if (ret) {
>> +		dev_err(dev, "Set CXL patrol scrub params for speed fail
>ret=%d\n",
>> +			ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int cxl_mem_ps_speed_available_read(struct device *dev, char
>> +*buf) {
>> +	int ret;
>> +	struct cxl_memdev_ps_params params;
>> +
>> +	ret = cxl_mem_ps_get_attrbs(dev, &params);
>> +	if (ret) {
>> +		dev_err(dev, "Get CXL patrol scrub params fail ret=%d\n",
>> +			ret);
>> +		return ret;
>> +	}
>> +
>> +	sprintf(buf, "%s\n", params.speed_avail);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * cxl_mem_patrol_scrub_is_visible() - Callback to return attribute
>> +visibility
>> + * @drv_data: Pointer to driver-private data structure passed
>> + *	      as argument to devm_scrub_device_register().
>> + * @attr: Scrub attribute
>> + * @region_id: ID of the memory region
>> + *
>> + * Returns: 0 on success, an error otherwise  */ umode_t
>> +cxl_mem_patrol_scrub_is_visible(const void *drv_data, u32 attr, int
>> +region_id) {
>> +	const struct cxl_patrol_scrub_context *cxl_ps_ctx = drv_data;
>> +
>> +	if (attr == scrub_speed_available ||
>> +	    attr == scrub_speed) {
>> +		if (!cxl_ps_ctx->scrub_cycle_changable)
>> +			return 0;
>> +	}
>> +
>> +	switch (attr) {
>> +	case scrub_speed_available:
>> +		return 0444;
>> +	case scrub_enable:
>> +		return 0200;
>> +	case scrub_speed:
>> +		return 0644;
>> +	default:
>> +		return 0;
>> +	}
>> +}
>> +
>> +/**
>> + * cxl_mem_patrol_scrub_read() - Read callback for data attributes
>> + * @dev: Pointer to scrub device
>> + * @attr: Scrub attribute
>> + * @region_id: ID of the memory region
>> + * @val: Pointer to the returned data
>> + *
>> + * Returns: 0 on success, an error otherwise  */ int
>> +cxl_mem_patrol_scrub_read(struct device *dev, u32 attr, int
>> +region_id, u64 *val) {
>> +
>> +	switch (attr) {
>> +	case scrub_speed:
>> +		return cxl_mem_ps_speed_read(dev->parent, val);
>> +	default:
>> +		return -ENOTSUPP;
>> +	}
>> +}
>> +
>> +/**
>> + * cxl_mem_patrol_scrub_write() - Write callback for data attributes
>> + * @dev: Pointer to scrub device
>> + * @attr: Scrub attribute
>> + * @region_id: ID of the memory region
>> + * @val: Value to write
>> + *
>> + * Returns: 0 on success, an error otherwise  */ int
>> +cxl_mem_patrol_scrub_write(struct device *dev, u32 attr, int
>> +region_id, u64 val) {
>> +	switch (attr) {
>> +	case scrub_enable:
>> +		return cxl_mem_ps_enable_write(dev->parent, val);
>> +	case scrub_speed:
>> +		return cxl_mem_ps_speed_write(dev->parent, val);
>> +	default:
>> +		return -ENOTSUPP;
>> +	}
>> +}
>> +
>> +/**
>> + * cxl_mem_patrol_scrub_read_strings() - Read callback for string
>> +attributes
>> + * @dev: Pointer to scrub device
>> + * @attr: Scrub attribute
>> + * @region_id: ID of the memory region
>> + * @buf: Pointer to the buffer for copying returned string
>> + *
>> + * Returns: 0 on success, an error otherwise  */ int
>> +cxl_mem_patrol_scrub_read_strings(struct device *dev, u32 attr, int
>region_id,
>> +				      char *buf)
>> +{
>> +	switch (attr) {
>> +	case scrub_speed_available:
>> +		return cxl_mem_ps_speed_available_read(dev->parent, buf);
>> +	default:
>> +		return -ENOTSUPP;
>> +	}
>> +}
>> +
>> +static const struct scrub_ops cxl_ps_scrub_ops = {
>> +	.is_visible = cxl_mem_patrol_scrub_is_visible,
>> +	.read = cxl_mem_patrol_scrub_read,
>> +	.write = cxl_mem_patrol_scrub_write,
>> +	.read_string = cxl_mem_patrol_scrub_read_strings,
>> +};
>> +
>> +int cxl_mem_patrol_scrub_init(struct cxl_memdev *cxlmd) {
>> +	int ret = 0;
>> +	struct device *cxl_scrub_dev;
>> +	struct cxl_memdev_ps_params params;
>> +	struct cxl_mbox_supp_feat_entry feat_entry;
>> +	char scrub_name[CXL_MEMDEV_MAX_NAME_LENGTH];
>> +	struct cxl_patrol_scrub_context *cxl_ps_ctx;
>> +
>> +	ret = cxl_mem_get_supported_feature_entry(cxlmd,
>&cxl_patrol_scrub_uuid,
>> +						  &feat_entry);
>> +	if (ret < 0)
>> +		goto cxl_ps_init_exit;
>> +
>> +	if (!(feat_entry.attrb_flags & CXL_FEAT_ENTRY_FLAG_CHANGABLE))
>> +		goto cxl_ps_init_exit;
>> +
>> +	cxl_ps_ctx = devm_kzalloc(&cxlmd->dev, sizeof(*cxl_ps_ctx),
>GFP_KERNEL);
>> +	if (!cxl_ps_ctx)
>> +		goto cxl_ps_init_exit;
>> +
>> +	cxl_ps_ctx->get_feat_size = feat_entry.get_feat_size;
>> +	cxl_ps_ctx->set_feat_size = feat_entry.set_feat_size;
>> +	ret = cxl_mem_ps_get_attrbs(&cxlmd->dev, &params);
>> +	if (ret) {
>> +		dev_err(&cxlmd->dev, "Get CXL patrol scrub params fail
>ret=%d\n",
>> +			ret);
>> +		goto cxl_ps_init_exit;
>> +	}
>> +	cxl_ps_ctx->scrub_cycle_changable =  params.scrub_cycle_changable;
>> +
>> +	snprintf(scrub_name, sizeof(scrub_name), "%s_%s",
>> +		 CXL_PATROL_SCRUB, dev_name(&cxlmd->dev));
>> +	cxl_scrub_dev = devm_scrub_device_register(&cxlmd->dev,
>scrub_name,
>> +						   cxl_ps_ctx,
>&cxl_ps_scrub_ops,
>> +
>CXL_MEMDEV_PATROL_SCRUB_NUM_REGIONS);
>> +	if (PTR_ERR_OR_ZERO(cxl_scrub_dev)) {
>> +		ret = PTR_ERR_OR_ZERO(cxl_scrub_dev);
>> +		goto cxl_ps_reg_fail;
>> +	}
>> +
>> +cxl_ps_reg_fail:
>> +cxl_ps_init_exit:
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_mem_patrol_scrub_init, CXL);
>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index
>> fdac686560d4..1d0fad0dc5ae 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -969,6 +969,13 @@ int cxl_trigger_poison_list(struct cxl_memdev
>> *cxlmd);  int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa);
>> int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa);
>>
>> +/* cxl memory scrub functions */
>> +#ifdef CONFIG_CXL_SCRUB
>> +int cxl_mem_patrol_scrub_init(struct cxl_memdev *cxlmd); #else int
>> +cxl_mem_patrol_scrub_init(struct cxl_memdev *cxlmd) { return
>> +-ENOTSUP; } #endif
>> +
>>  #ifdef CONFIG_CXL_SUSPEND
>>  void cxl_mem_active_inc(void);
>>  void cxl_mem_active_dec(void);
>> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index
>> 0155fb66b580..86bba8794bb4 100644
>> --- a/drivers/cxl/pci.c
>> +++ b/drivers/cxl/pci.c
>> @@ -881,6 +881,12 @@ static int cxl_pci_probe(struct pci_dev *pdev, const
>struct pci_device_id *id)
>>  	if (rc)
>>  		return rc;
>>
>> +	/*
>> +	 * Initialize optional CXL scrub features
>> +	 */
>> +	if (cxl_mem_patrol_scrub_init(cxlmd))
>> +		dev_dbg(&pdev->dev, "cxl_mem_patrol_scrub_init failed\n");
>> +
>>  	rc = devm_cxl_sanitize_setup_notifier(&pdev->dev, cxlmd);
>>  	if (rc)
>>  		return rc;

Thanks,
Shiju

Re: [RFC PATCH 5/6] cxl/memscrub: Add CXL device DDR5 ECS control feature

[Dave Jiang]

On 11/14/23 05:56, shiju.jose@huawei.com wrote:
> From: Shiju Jose <shiju.jose@huawei.com>
> 
> CXL spec 3.1 section 8.2.9.9.11.2 describes the DDR5 Error Check Scrub (ECS)
> control feature.
> 
> The Error Check Scrub (ECS) is a feature defined in JEDEC DDR5 SDRAM
> Specification (JESD79-5) and allows the DRAM to internally read, correct
> single-bit errors, and write back corrected data bits to the DRAM array
> while providing transparency to error counts. The ECS control feature
> allows the request to configure ECS input configurations during system
> boot or at run-time.
> 
> The ECS control allows the requester to change the log entry type, the ECS
> threshold count provided that the request is within the definition
> specified in DDR5 mode registers, change mode between codeword mode and
> row count mode, and reset the ECS counter.
> 
> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> ---
>  drivers/cxl/core/memscrub.c | 557 +++++++++++++++++++++++++++++++++++-
>  drivers/cxl/cxlmem.h        |   2 +
>  drivers/cxl/pci.c           |   2 +
>  3 files changed, 559 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/memscrub.c b/drivers/cxl/core/memscrub.c
> index ec67ffc81363..1c7031166ba0 100644
> --- a/drivers/cxl/core/memscrub.c
> +++ b/drivers/cxl/core/memscrub.c
> @@ -5,9 +5,9 @@
>   * Copyright (c) 2023 HiSilicon Limited.
>   *
>   *  - Provides functions to configure patrol scrub
> - *    feature of the CXL memory devices.
> + *    and DDR5 ECS features of the CXL memory devices.
>   *  - Registers with the scrub driver for the
> - *    memory patrol scrub feature.
> + *    memory patrol scrub and DDR5 ECS features.
>   */
>  
>  #define pr_fmt(fmt)	"CXL_MEM_SCRUB: " fmt
> @@ -453,3 +453,556 @@ int cxl_mem_patrol_scrub_init(struct cxl_memdev *cxlmd)
>  	return ret;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_mem_patrol_scrub_init, CXL);
> +
> +/* CXL DDR5 ECS control definitions */
> +#define CXL_MEMDEV_ECS_GET_FEAT_VERSION	0x01
> +#define CXL_MEMDEV_ECS_SET_FEAT_VERSION	0x01
> +
> +#define CXL_DDR5_ECS	"cxl_ecs"
> +
> +/* The default number of regions for CXL memory device patrol scrubber */
> +#define CXL_MEMDEV_ECS_NUM_REGIONS	1
> +
> +static const uuid_t cxl_ecs_uuid =
> +	UUID_INIT(0xe5b13f22, 0x2328, 0x4a14, 0xb8, 0xba, 0xb9, 0x69, 0x1e,     \
> +		  0x89, 0x33, 0x86);
> +
> +/* CXL DDR5 ECS control functions */
> +struct cxl_ecs_context {
> +	struct device *dev;
> +	u16 nregions;
> +	u16 get_feat_size;
> +	u16 set_feat_size;
> +};
> +
> +/**
> + * struct cxl_memdev_ecs_params - CXL memory DDR5 ECS parameter data structure.
> + * @log_entry_type: ECS log entry type, per DRAM or per memory media FRU.
> + * @threshold: ECS threshold count per GB of memory cells.
> + * @mode:	codeword/row count mode
> + *		0 : ECS counts rows with errors
> + *		1 : ECS counts codeword with errors
> + * @reset_counter: [IN] reset ECC counter to default value.
> + * @log_entry_type_avail: Supported ECS log entry types.
> + * @threshold_avail: Supported ECS threshold counts.
> + * @mode_avail: Supported ECS count mode.
> + */
> +struct cxl_memdev_ecs_params {
> +	u8 log_entry_type;
> +	u16 threshold;
> +	u8 mode;
> +	bool reset_counter;
> +	char log_entry_type_avail[CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH];
> +	char threshold_avail[CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH];
> +	char mode_avail[CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH];
> +};
> +
> +enum {
> +	CXL_MEMDEV_ECS_PARAM_LOG_ENTRY_TYPE = 0,
> +	CXL_MEMDEV_ECS_PARAM_THRESHOLD,
> +	CXL_MEMDEV_ECS_PARAM_MODE,
> +	CXL_MEMDEV_ECS_PARAM_RESET_COUNTER,
> +};
> +
> +#define	CXL_MEMDEV_ECS_LOG_ENTRY_TYPE_MASK	GENMASK(1, 0)
> +#define	CXL_MEMDEV_ECS_REALTIME_REPORT_CAP_MASK	BIT(0)
> +#define	CXL_MEMDEV_ECS_THRESHOLD_COUNT_MASK	GENMASK(2, 0)
> +#define	CXL_MEMDEV_ECS_MODE_MASK	BIT(3)
> +#define	CXL_MEMDEV_ECS_RESET_COUNTER_MASK	BIT(4)
> +
> +static const u16 ecs_supp_threshold[] = { 0, 0, 0, 256, 1024, 4096 };
> +
> +enum {
> +	ECS_LOG_ENTRY_TYPE_DRAM = 0x0,
> +	ECS_LOG_ENTRY_TYPE_MEM_MEDIA_FRU = 0x1,
> +};
> +
> +enum {
> +	ECS_THRESHOLD_256 = 3,
> +	ECS_THRESHOLD_1024 = 4,
> +	ECS_THRESHOLD_4096 = 5,
> +};
> +
> +enum {
> +	ECS_MODE_COUNTS_ROWS = 0,
> +	ECS_MODE_COUNTS_CODEWORDS = 1,
> +};
> +
> +struct cxl_memdev_ecs_feat_read_attrbs {
> +	u8 ecs_log_cap;
> +	u8 ecs_cap;
> +	__le16 ecs_config;
> +	u8 ecs_flags;
> +}  __packed;
> +
> +struct cxl_memdev_ecs_set_feat_pi {
> +	struct cxl_mbox_set_feat_in pi;
> +	struct cxl_memdev_ecs_feat_wr_attrbs {
> +		u8 ecs_log_cap;
> +		__le16 ecs_config;
> +	} __packed wr_attrbs[];
> +}  __packed;
> +
> +static int cxl_mem_ecs_get_attrbs(struct device *dev, int fru_id,
> +				  struct cxl_memdev_ecs_params *params)
> +{
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev->parent);
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> +	struct cxl_mbox_get_feat_in pi = {
> +		.uuid = cxl_ecs_uuid,
> +		.offset = 0,
> +		.selection = CXL_GET_FEAT_CURRENT_VALUE,
> +	};
> +	struct cxl_memdev_ecs_feat_read_attrbs *rd_attrbs;
> +	struct cxl_ecs_context *cxl_ecs_ctx;
> +	u8 threshold_index;
> +
> +	if (!mds)
> +		return -EFAULT;
> +	cxl_ecs_ctx = dev_get_drvdata(dev);
> +
> +	pi.count = cxl_ecs_ctx->get_feat_size;
> +	rd_attrbs = cxl_get_feature(mds, &pi);
> +	if (PTR_ERR_OR_ZERO(rd_attrbs)) {

Why not just IS_ERR()? 
> +		params->log_entry_type = 0;
> +		params->threshold = 0;
> +		params->mode = 0;
> +		snprintf(params->log_entry_type_avail, CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH,
> +			 "Unavailable");
> +		snprintf(params->threshold_avail, CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH,
> +			 "Unavailable");
> +		snprintf(params->mode_avail, CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH,
> +			 "Unavailable");

Given that these are static values, maybe change the string params to enums and then have a static string table you can index to?

> +		return PTR_ERR_OR_ZERO(rd_attrbs);

at this point you can just return PTR_ERR()

> +	}
> +	params->log_entry_type = FIELD_GET(CXL_MEMDEV_ECS_LOG_ENTRY_TYPE_MASK,
> +					   rd_attrbs[fru_id].ecs_log_cap);
> +	threshold_index = FIELD_GET(CXL_MEMDEV_ECS_THRESHOLD_COUNT_MASK,
> +				    rd_attrbs[fru_id].ecs_config);
> +	params->threshold = ecs_supp_threshold[threshold_index];
> +	params->mode = FIELD_GET(CXL_MEMDEV_ECS_MODE_MASK,
> +				 rd_attrbs[fru_id].ecs_config);
> +	snprintf(params->log_entry_type_avail, CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH,
> +		 "Log Entry Type 0:per DRAM  1:per Memory Media FRU");
> +	snprintf(params->threshold_avail, CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH,
> +		 "Threshold count per Gb of memory cells: 256,1024,4096");
> +	snprintf(params->mode_avail, CXL_SCRUB_MAX_ATTRB_RANGE_LENGTH,
> +		 "Mode 0:ECS counts rows with errors  1:ECS counts codewords with errors");
> +
> +	kvfree(rd_attrbs);
> +
> +	return 0;
> +}
> +
> +static int cxl_mem_ecs_set_attrbs(struct device *dev, int fru_id,
> +				  struct cxl_memdev_ecs_params *params, u8 param_type)
> +{
> +	int ret = 0;
> +	u32 set_pi_size;
> +	u16 nmedia_frus, count;
> +	struct cxl_memdev_ecs_set_feat_pi *set_pi;
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev->parent);
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> +	struct cxl_mbox_get_feat_in pi = {
> +		.uuid = cxl_ecs_uuid,
> +		.offset = 0,
> +		.selection = CXL_GET_FEAT_CURRENT_VALUE,
> +	};
> +	struct cxl_memdev_ecs_feat_read_attrbs *rd_attrbs;
> +	struct cxl_memdev_ecs_feat_wr_attrbs *wr_attrbs;
> +	struct cxl_ecs_context *cxl_ecs_ctx;
> +	struct cxl_memdev_ecs_params rd_params;
> +
> +	if (!mds)
> +		return -EFAULT;
> +
> +	cxl_ecs_ctx = dev_get_drvdata(dev);
> +	nmedia_frus = cxl_ecs_ctx->nregions;
> +
> +	pi.count = cxl_ecs_ctx->get_feat_size;
> +	rd_attrbs = cxl_get_feature(mds, &pi);
> +	if (PTR_ERR_OR_ZERO(rd_attrbs))
> +		return PTR_ERR_OR_ZERO(rd_attrbs);

if (IS_ERR(rd_attribs))
	return PTR_ERR(rd_attrbs);


> +	set_pi_size = sizeof(struct cxl_mbox_set_feat_in) +
> +				cxl_ecs_ctx->set_feat_size;
> +	set_pi = kvmalloc(set_pi_size, GFP_KERNEL);

I think you can use the auto clean declaration. Then you don't need to call kvfree() every time you return with error or use the goto.

struct cxl_memdev_ecs_set_feat_pi *set_pi __free(kvfree) = kvmalloc(set_pi_size, GFP_KERNEL);

https://lwn.net/Articles/934679/

> +	if (!set_pi) {
> +		kvfree(rd_attrbs);
> +		return -ENOMEM;
> +	}
> +
> +	set_pi->pi.uuid = cxl_ecs_uuid;
> +	set_pi->pi.flags = CXL_SET_FEAT_FLAG_MOD_VALUE_SAVED_ACROSS_RESET |
> +				CXL_SET_FEAT_FLAG_ACTION_FULL_DATA_TRANSFER;
> +	set_pi->pi.offset = 0;
> +	set_pi->pi.version = CXL_MEMDEV_ECS_SET_FEAT_VERSION;
> +	/* Fill writable attributes from the current attributes read for all the media FRUs */
> +	count = 0;
> +	wr_attrbs = set_pi->wr_attrbs;
> +	while (count < nmedia_frus) {

for loop?
> +		wr_attrbs[count].ecs_log_cap = rd_attrbs[count].ecs_log_cap;
> +		wr_attrbs[count].ecs_config = rd_attrbs[count].ecs_config;
> +		count++;
> +	}
> +	kvfree(rd_attrbs);

Maybe cxl_get_feature() should take a ptr to an allocated rd_attrbs rather than doing the allocation then expect the caller to free it.
 
> +
> +	/* Fill attribute to be set for the media FRU */
> +	if (param_type == CXL_MEMDEV_ECS_PARAM_LOG_ENTRY_TYPE) {

I think this entire if/else block can be setup as a switch block since it's checking aainst param_type for all branches?

> +		if (params->log_entry_type != ECS_LOG_ENTRY_TYPE_DRAM &&
> +		    params->log_entry_type != ECS_LOG_ENTRY_TYPE_MEM_MEDIA_FRU) {
> +			dev_err(dev->parent,
> +				"Invalid CXL ECS scrub log entry type(%d) to set\n",
> +			       params->log_entry_type);
> +			dev_err(dev->parent,
> +				"Log Entry Type 0: per DRAM  1: per Memory Media FRU\n");
> +			ret = -EINVAL;
> +			goto set_attrbs_exit;
> +		}
> +		wr_attrbs[fru_id].ecs_log_cap = FIELD_PREP(CXL_MEMDEV_ECS_LOG_ENTRY_TYPE_MASK,
> +							   params->log_entry_type);
> +	} else if (param_type == CXL_MEMDEV_ECS_PARAM_THRESHOLD) {
> +		wr_attrbs[fru_id].ecs_config &= ~CXL_MEMDEV_ECS_THRESHOLD_COUNT_MASK;
> +		switch (params->threshold) {
> +		case 256:
> +			wr_attrbs[fru_id].ecs_config |= FIELD_PREP(
> +						CXL_MEMDEV_ECS_THRESHOLD_COUNT_MASK,
> +						ECS_THRESHOLD_256);
> +			break;
> +		case 1024:
> +			wr_attrbs[fru_id].ecs_config |= FIELD_PREP(
> +						CXL_MEMDEV_ECS_THRESHOLD_COUNT_MASK,
> +						ECS_THRESHOLD_1024);
> +			break;
> +		case 4096:
> +			wr_attrbs[fru_id].ecs_config |= FIELD_PREP(
> +						CXL_MEMDEV_ECS_THRESHOLD_COUNT_MASK,
> +						ECS_THRESHOLD_4096);
> +			break;
> +		default:
> +			dev_err(dev->parent,
> +				"Invalid CXL ECS scrub threashol count(%d) to set\n",
> +				params->threshold);
> +			dev_err(dev->parent,
> +				"Threshold count per Gb pf memory cells: 256,1024,4096\n");
> +			ret = -EINVAL;
> +			goto set_attrbs_exit;
> +		}
> +	} else if (param_type == CXL_MEMDEV_ECS_PARAM_MODE) {
> +		if (params->mode != ECS_MODE_COUNTS_ROWS &&
> +		    params->mode != ECS_MODE_COUNTS_CODEWORDS) {
> +			dev_err(dev->parent,
> +				"Invalid CXL ECS scrub mode(%d) to set\n",
> +				params->mode);
> +			dev_err(dev->parent,
> +				"Mode 0: ECS counts rows with errors"
> +				" 1: ECS counts codewords with errors\n");
> +			ret = -EINVAL;
> +			goto set_attrbs_exit;
> +		}
> +		wr_attrbs[fru_id].ecs_config &= ~CXL_MEMDEV_ECS_MODE_MASK;
> +		wr_attrbs[fru_id].ecs_config |= FIELD_PREP(CXL_MEMDEV_ECS_MODE_MASK,
> +							  params->mode);
> +	} else if (param_type == CXL_MEMDEV_ECS_PARAM_RESET_COUNTER) {
> +		wr_attrbs[fru_id].ecs_config &= ~CXL_MEMDEV_ECS_RESET_COUNTER_MASK;
> +		wr_attrbs[fru_id].ecs_config |= FIELD_PREP(CXL_MEMDEV_ECS_RESET_COUNTER_MASK,
> +							   params->reset_counter);
> +	} else {
> +		dev_err(dev->parent, "Invalid CXL ECS parameter to set\n");
> +		ret = -EINVAL;
> +		goto set_attrbs_exit;
> +	}
> +
> +	ret = cxl_set_feature(mds, set_pi, set_pi_size);
> +	if (ret) {
> +		dev_err(dev->parent, "CXL ECS set feature fail ret=%d\n", ret);
> +		goto set_attrbs_exit;
> +	}
> +
> +	/* Verify attribute is set successfully */
> +	ret = cxl_mem_ecs_get_attrbs(dev, fru_id, &rd_params);
> +	if (ret) {
> +		dev_err(dev->parent, "Get cxlmemdev ECS params fail ret=%d\n", ret);
> +		return ret;
Leaking set_pi, but shouldn't be an issue once you go to autoclean. Same with switch block below.

> +	}
> +	switch (param_type) {
> +	case CXL_MEMDEV_ECS_PARAM_LOG_ENTRY_TYPE:
> +		if (rd_params.log_entry_type != params->log_entry_type)
> +			return -EFAULT;
> +		break;
> +	case CXL_MEMDEV_ECS_PARAM_THRESHOLD:
> +		if (rd_params.threshold != params->threshold)
> +			return -EFAULT;
> +		break;
> +	case CXL_MEMDEV_ECS_PARAM_MODE:
> +		if (rd_params.mode != params->mode)
> +			return -EFAULT;
> +		break;
> +	}
> +
> +set_attrbs_exit:
> +	kvfree(set_pi);
> +	return ret;
> +}
> +
> +static int cxl_mem_ecs_log_entry_type_write(struct device *dev, int region_id, long val)
> +{
> +	int ret;
> +	struct cxl_memdev_ecs_params params;
> +
> +	params.log_entry_type = val;
> +	ret = cxl_mem_ecs_set_attrbs(dev, region_id, &params,
> +				     CXL_MEMDEV_ECS_PARAM_LOG_ENTRY_TYPE);
> +	if (ret) {
> +		dev_err(dev->parent, "Set CXL ECS params for log entry type fail ret=%d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cxl_mem_ecs_threshold_write(struct device *dev, int region_id, long val)
> +{
> +	int ret;
> +	struct cxl_memdev_ecs_params params;
> +
> +	params.threshold = val;
> +	ret = cxl_mem_ecs_set_attrbs(dev, region_id, &params,
> +				     CXL_MEMDEV_ECS_PARAM_THRESHOLD);
> +	if (ret) {
> +		dev_err(dev->parent, "Set CXL ECS params for threshold fail ret=%d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cxl_mem_ecs_mode_write(struct device *dev, int region_id, long val)
> +{
> +	int ret;
> +	struct cxl_memdev_ecs_params params;
> +
> +	params.mode = val;
> +	ret = cxl_mem_ecs_set_attrbs(dev, region_id, &params,
> +				     CXL_MEMDEV_ECS_PARAM_MODE);
> +	if (ret) {
> +		dev_err(dev->parent, "Set CXL ECS params for mode fail ret=%d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cxl_mem_ecs_reset_counter_write(struct device *dev, int region_id, long val)
> +{
> +	int ret;
> +	struct cxl_memdev_ecs_params params;
> +
> +	params.reset_counter = val;
> +	ret = cxl_mem_ecs_set_attrbs(dev, region_id, &params,
> +				     CXL_MEMDEV_ECS_PARAM_RESET_COUNTER);
> +	if (ret) {
> +		dev_err(dev->parent, "Set CXL ECS params for reset ECC counter fail ret=%d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * cxl_mem_ecs_is_visible() - Callback to return attribute visibility
> + * @drv_data: Pointer to driver-private data structure passed
> + *	      as argument to devm_scrub_device_register().
> + * @attr: Scrub attribute
> + * @region_id: ID of the memory region
> + *
> + * Returns: 0 on success, an error otherwise
> + */
> +static umode_t cxl_mem_ecs_is_visible(const void *drv_data, u32 attr, int region_id)
> +{
> +	switch (attr) {
> +	case scrub_reset_counter:
> +		return 0200;
> +	case scrub_ecs_log_entry_type_per_dram:
> +	case scrub_ecs_log_entry_type_per_memory_media:
> +	case scrub_mode_counts_rows:
> +	case scrub_mode_counts_codewords:
> +	case scrub_threshold_available:
> +		return 0444;
> +	case scrub_ecs_log_entry_type:
> +	case scrub_mode:
> +	case scrub_threshold:
> +		return 0644;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +/**
> + * cxl_mem_ecs_read() - Read callback for data attributes
> + * @dev: Pointer to scrub device
> + * @attr: Scrub attribute
> + * @region_id: ID of the memory region
> + * @val: Pointer to the returned data
> + *
> + * Returns: 0 on success, an error otherwise
> + */
> +static int cxl_mem_ecs_read(struct device *dev, u32 attr, int region_id, u64 *val)
> +{
> +	int ret;
> +	struct cxl_memdev_ecs_params params;
> +
> +	if (attr == scrub_ecs_log_entry_type ||
> +	    attr == scrub_ecs_log_entry_type_per_dram ||
> +	    attr == scrub_ecs_log_entry_type_per_memory_media ||
> +	    attr == scrub_mode ||
> +	    attr == scrub_mode_counts_rows ||
> +	    attr == scrub_mode_counts_codewords ||
> +	    attr == scrub_threshold) {
> +		ret = cxl_mem_ecs_get_attrbs(dev, region_id, &params);
> +		if (ret) {
> +			dev_err(dev->parent, "Get CXL ECS params fail ret=%d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	switch (attr) {
> +	case scrub_ecs_log_entry_type:
> +		*val = params.log_entry_type;
> +		return 0;
> +	case scrub_ecs_log_entry_type_per_dram:
> +		if (params.log_entry_type == ECS_LOG_ENTRY_TYPE_DRAM)
> +			*val = 1;
> +		else
> +			*val = 0;
> +		return 0;
> +	case scrub_ecs_log_entry_type_per_memory_media:
> +		if (params.log_entry_type == ECS_LOG_ENTRY_TYPE_MEM_MEDIA_FRU)
> +			*val = 1;
> +		else
> +			*val = 0;
> +		return 0;
> +	case scrub_mode:
> +		*val = params.mode;
> +		return 0;
> +	case scrub_mode_counts_rows:
> +		if (params.mode == ECS_MODE_COUNTS_ROWS)
> +			*val = 1;
> +		else
> +			*val = 0;
> +		return 0;
> +	case scrub_mode_counts_codewords:
> +		if (params.mode == ECS_MODE_COUNTS_CODEWORDS)
> +			*val = 1;
> +		else
> +			*val = 0;
> +		return 0;
> +	case scrub_threshold:
> +		*val = params.threshold;
> +		return 0;
> +	default:
> +		return -ENOTSUPP;
> +	}
> +}
> +
> +/**
> + * cxl_mem_ecs_write() - Write callback for data attributes
> + * @dev: Pointer to scrub device
> + * @attr: Scrub attribute
> + * @region_id: ID of the memory region
> + * @val: Value to write
> + *
> + * Returns: 0 on success, an error otherwise
> + */
> +static int cxl_mem_ecs_write(struct device *dev, u32 attr, int region_id, u64 val)
> +{
> +	switch (attr) {
> +	case scrub_ecs_log_entry_type:
> +		return cxl_mem_ecs_log_entry_type_write(dev, region_id, val);
> +	case scrub_mode:
> +		return cxl_mem_ecs_mode_write(dev, region_id, val);
> +	case scrub_reset_counter:
> +		return cxl_mem_ecs_reset_counter_write(dev, region_id, val);
> +	case scrub_threshold:
> +		return cxl_mem_ecs_threshold_write(dev, region_id, val);
> +	default:
> +		return -ENOTSUPP;
> +	}
> +}
> +
> +/**
> + * cxl_mem_ecs_read_strings() - Read callback for DDR5 ECS string attributes
> + * @dev: Pointer to ECS scrub device
> + * @attr: ECS scrub attribute
> + * @region_id: ID of the memory media FRU.
> + * @buf: Pointer to the buffer for copying returned string
> + *
> + * Returns: 0 on success, an error otherwise
> + */
> +static int cxl_mem_ecs_read_strings(struct device *dev, u32 attr,
> +				    int region_id, char *buf)
> +{
> +
> +	switch (attr) {
> +	case scrub_threshold_available:
> +		sprintf(buf, "256,1024,4096\n");
sysfs_emit()?

> +		return 0;
> +	default:
> +		return -ENOTSUPP;
> +	}
> +}
> +
> +static const struct scrub_ops cxl_ecs_ops = {
> +	.is_visible = cxl_mem_ecs_is_visible,
> +	.read = cxl_mem_ecs_read,
> +	.write = cxl_mem_ecs_write,
> +	.read_string = cxl_mem_ecs_read_strings,
> +};
> +
> +int cxl_mem_ddr5_ecs_init(struct cxl_memdev *cxlmd)
> +{
> +	int nmedia_frus; /* number of memory media FRUs in the device */
> +	int ret = 0;
> +	struct device *cxl_scrub_dev;
> +	char scrub_name[CXL_MEMDEV_MAX_NAME_LENGTH];
> +	struct cxl_mbox_supp_feat_entry feat_entry;
> +	struct cxl_ecs_context *cxl_ecs_ctx;
> +
> +	ret = cxl_mem_get_supported_feature_entry(cxlmd, &cxl_ecs_uuid, &feat_entry);
> +	if (ret < 0)
> +		goto cxl_ecs_reg_fail;
> +
> +	if (!(feat_entry.attrb_flags & CXL_FEAT_ENTRY_FLAG_CHANGABLE))
> +		goto cxl_ecs_init_exit;
> +	nmedia_frus = feat_entry.get_feat_size/
> +				sizeof(struct cxl_memdev_ecs_feat_read_attrbs);
> +	if (nmedia_frus) {
> +		cxl_ecs_ctx = devm_kzalloc(&cxlmd->dev, sizeof(*cxl_ecs_ctx), GFP_KERNEL);
> +		if (!cxl_ecs_ctx)
> +			goto cxl_ecs_init_exit;
> +
> +		cxl_ecs_ctx->nregions = nmedia_frus;
> +		cxl_ecs_ctx->get_feat_size = feat_entry.get_feat_size;
> +		cxl_ecs_ctx->set_feat_size = feat_entry.set_feat_size;
> +
> +		snprintf(scrub_name, sizeof(scrub_name), "%s_%s",
> +			 CXL_DDR5_ECS, dev_name(&cxlmd->dev));
> +		cxl_scrub_dev = devm_scrub_device_register(&cxlmd->dev, scrub_name,
> +							  cxl_ecs_ctx, &cxl_ecs_ops,
> +							  cxl_ecs_ctx->nregions);
> +		if (PTR_ERR_OR_ZERO(cxl_scrub_dev)) {
IS_ERR()?

> +			ret = PTR_ERR_OR_ZERO(cxl_scrub_dev);

PTR_ERR()
> +			goto cxl_ecs_reg_fail;
> +		}
> +	}
> +
> +cxl_ecs_reg_fail:
> +cxl_ecs_init_exit:
> +	return ret;

Why goto label if it just return directly w/o additional operations?

> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_mem_ddr5_ecs_init, CXL);
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 1d0fad0dc5ae..4da0b0ec3b1e 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -972,8 +972,10 @@ int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa);
>  /* cxl memory scrub functions */
>  #ifdef CONFIG_CXL_SCRUB
>  int cxl_mem_patrol_scrub_init(struct cxl_memdev *cxlmd);
> +int cxl_mem_ddr5_ecs_init(struct cxl_memdev *cxlmd);
>  #else
>  int cxl_mem_patrol_scrub_init(struct cxl_memdev *cxlmd) { return -ENOTSUP; }
> +int cxl_mem_ddr5_ecs_init(struct cxl_memdev *cxlmd) { return -ENOTSUP; }
>  #endif
>  
>  #ifdef CONFIG_CXL_SUSPEND
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 86bba8794bb4..75ce4f41c5c0 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -886,6 +886,8 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	 */
>  	if (cxl_mem_patrol_scrub_init(cxlmd))
>  		dev_dbg(&pdev->dev, "cxl_mem_patrol_scrub_init failed\n");
> +	if (cxl_mem_ddr5_ecs_init(cxlmd))
> +		dev_dbg(&pdev->dev, "cxl_mem_ddr5_ecs_init failed\n");
>  
>  	rc = devm_cxl_sanitize_setup_notifier(&pdev->dev, cxlmd);
>  	if (rc)