* [PATCH linux-next 1/2] ACPI:RAS2: Add ACPI RAS2 driver
2025-02-28 12:27 [PATCH linux-next 0/2] ACPI: Add support for ACPI RAS2 feature table shiju.jose
@ 2025-02-28 12:27 ` shiju.jose
2025-03-03 21:06 ` Yazen Ghannam
2025-02-28 12:27 ` [PATCH linux-next 2/2] ras: mem: Add memory " shiju.jose
2025-03-03 9:35 ` [PATCH linux-next 0/2] ACPI: Add support for ACPI RAS2 feature table Jonathan Cameron
2 siblings, 1 reply; 11+ messages in thread
From: shiju.jose @ 2025-02-28 12:27 UTC (permalink / raw)
To: linux-edac, linux-acpi, rafael, bp, tony.luck, lenb, mchehab
Cc: linux-mm, linux-kernel, linux-cxl, j.williams, dave,
jonathan.cameron, dave.jiang, alison.schofield, vishal.l.verma,
ira.weiny, david, Vilas.Sridharan, leo.duran, Yazen.Ghannam,
rientjes, jiaqiyan, Jon.Grimm, dave.hansen, naoya.horiguchi,
james.morse, jthoughton, somasundaram.a, erdemaktas, pgonda,
duenwen, gthelen, wschwartz, dferguson, wbs, nifan.cxl,
yazen.ghannam, tanxiaofei, prime.zeng, roberto.sassu,
kangkang.shen, wanghuiqiang, linuxarm, shiju.jose
From: Shiju Jose <shiju.jose@huawei.com>
Add support for ACPI RAS2 feature table (RAS2) defined in the
ACPI 6.5 Specification, section 5.2.21.
Driver contains RAS2 Init, which extracts the RAS2 table and driver
adds auxiliary device for each memory feature which binds to the
RAS2 memory driver.
Driver uses PCC mailbox to communicate with the ACPI HW and the
driver adds OSPM interfaces to send RAS2 commands.
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Co-developed-by: A Somasundaram <somasundaram.a@hpe.com>
Signed-off-by: A Somasundaram <somasundaram.a@hpe.com>
Co-developed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Tested-by: Daniel Ferguson <danielf@os.amperecomputing.com>
Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
---
drivers/acpi/Kconfig | 11 ++
drivers/acpi/Makefile | 1 +
drivers/acpi/ras2.c | 417 +++++++++++++++++++++++++++++++++++++++
include/acpi/ras2_acpi.h | 47 +++++
4 files changed, 476 insertions(+)
create mode 100755 drivers/acpi/ras2.c
create mode 100644 include/acpi/ras2_acpi.h
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 7f10aa38269d..7b470cf2fd71 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -293,6 +293,17 @@ config ACPI_CPPC_LIB
If your platform does not support CPPC in firmware,
leave this option disabled.
+config ACPI_RAS2
+ bool "ACPI RAS2 driver"
+ select AUXILIARY_BUS
+ select MAILBOX
+ select PCC
+ help
+ The driver adds support for ACPI RAS2 feature table(extracts RAS2
+ table from OS system table) and OSPM interfaces to send RAS2
+ commands via PCC mailbox subspace. Driver adds platform device for
+ the RAS2 memory features which binds to the RAS2 memory driver.
+
config ACPI_PROCESSOR
tristate "Processor"
depends on X86 || ARM64 || LOONGARCH || RISCV
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 40208a0f5dfb..797b38cdc2f3 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -100,6 +100,7 @@ obj-$(CONFIG_ACPI_EC_DEBUGFS) += ec_sys.o
obj-$(CONFIG_ACPI_BGRT) += bgrt.o
obj-$(CONFIG_ACPI_CPPC_LIB) += cppc_acpi.o
obj-$(CONFIG_ACPI_SPCR_TABLE) += spcr.o
+obj-$(CONFIG_ACPI_RAS2) += ras2.o
obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
obj-$(CONFIG_ACPI_PPTT) += pptt.o
obj-$(CONFIG_ACPI_PFRUT) += pfr_update.o pfr_telemetry.o
diff --git a/drivers/acpi/ras2.c b/drivers/acpi/ras2.c
new file mode 100755
index 000000000000..cc8eef49c158
--- /dev/null
+++ b/drivers/acpi/ras2.c
@@ -0,0 +1,417 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Implementation of ACPI RAS2 driver.
+ *
+ * Copyright (c) 2024-2025 HiSilicon Limited.
+ *
+ * Support for RAS2 - ACPI 6.5 Specification, section 5.2.21
+ *
+ * Driver contains ACPI RAS2 init, which extracts the ACPI RAS2 table and
+ * get the PCC channel subspace for communicating with the ACPI compliant
+ * HW platform which supports ACPI RAS2. Driver adds platform devices
+ * for each RAS2 memory feature which binds to the memory ACPI RAS2 driver.
+ */
+
+#define pr_fmt(fmt) "ACPI RAS2: " fmt
+
+#include <linux/delay.h>
+#include <linux/export.h>
+#include <linux/ktime.h>
+#include <linux/platform_device.h>
+#include <acpi/pcc.h>
+#include <acpi/ras2_acpi.h>
+
+/* Data structure for PCC communication */
+struct ras2_pcc_subspace {
+ int pcc_subspace_id;
+ struct mbox_client mbox_client;
+ struct pcc_mbox_chan *pcc_chan;
+ struct acpi_ras2_shared_memory __iomem *pcc_comm_addr;
+ bool pcc_channel_acquired;
+ ktime_t deadline;
+ unsigned int pcc_mpar;
+ unsigned int pcc_mrtt;
+ struct list_head elem;
+ u16 ref_count;
+};
+
+/*
+ * Arbitrary Retries for PCC commands because the
+ * remote processor could be much slower to reply.
+ */
+#define RAS2_NUM_RETRIES 600
+
+#define RAS2_FEATURE_TYPE_MEMORY 0x00
+
+/* global variables for the RAS2 PCC subspaces */
+static DEFINE_MUTEX(ras2_pcc_subspace_lock);
+static LIST_HEAD(ras2_pcc_subspaces);
+
+static int ras2_report_cap_error(u32 cap_status)
+{
+ switch (cap_status) {
+ case ACPI_RAS2_NOT_VALID:
+ case ACPI_RAS2_NOT_SUPPORTED:
+ return -EPERM;
+ case ACPI_RAS2_BUSY:
+ return -EBUSY;
+ case ACPI_RAS2_FAILED:
+ case ACPI_RAS2_ABORTED:
+ case ACPI_RAS2_INVALID_DATA:
+ return -EINVAL;
+ default: /* 0 or other, Success */
+ return 0;
+ }
+}
+
+static int ras2_check_pcc_chan(struct ras2_pcc_subspace *pcc_subspace)
+{
+ struct acpi_ras2_shared_memory __iomem *generic_comm_base = pcc_subspace->pcc_comm_addr;
+ ktime_t next_deadline = ktime_add(ktime_get(), pcc_subspace->deadline);
+ u32 cap_status;
+ u16 status;
+ u32 ret;
+
+ while (!ktime_after(ktime_get(), next_deadline)) {
+ /*
+ * As per ACPI spec, the PCC space will be initialized by
+ * platform and should have set the command completion bit when
+ * PCC can be used by OSPM
+ */
+ status = readw_relaxed(&generic_comm_base->status);
+ if (status & RAS2_PCC_CMD_ERROR) {
+ cap_status = readw_relaxed(&generic_comm_base->set_capabilities_status);
+ ret = ras2_report_cap_error(cap_status);
+
+ status &= ~RAS2_PCC_CMD_ERROR;
+ writew_relaxed(status, &generic_comm_base->status);
+ return ret;
+ }
+ if (status & RAS2_PCC_CMD_COMPLETE)
+ return 0;
+ /*
+ * Reducing the bus traffic in case this loop takes longer than
+ * a few retries.
+ */
+ msleep(10);
+ }
+
+ return -EIO;
+}
+
+/**
+ * ras2_send_pcc_cmd() - Send RAS2 command via PCC channel
+ * @ras2_ctx: pointer to the RAS2 context structure
+ * @cmd: command to send
+ *
+ * Returns: 0 on success, an error otherwise
+ */
+int ras2_send_pcc_cmd(struct ras2_mem_ctx *ras2_ctx, u16 cmd)
+{
+ struct ras2_pcc_subspace *pcc_subspace = ras2_ctx->pcc_subspace;
+ struct acpi_ras2_shared_memory *generic_comm_base = pcc_subspace->pcc_comm_addr;
+ static ktime_t last_cmd_cmpl_time, last_mpar_reset;
+ struct mbox_chan *pcc_channel;
+ unsigned int time_delta;
+ static int mpar_count;
+ int ret;
+
+ guard(mutex)(&ras2_pcc_subspace_lock);
+ ret = ras2_check_pcc_chan(pcc_subspace);
+ if (ret < 0)
+ return ret;
+ pcc_channel = pcc_subspace->pcc_chan->mchan;
+
+ /*
+ * Handle the Minimum Request Turnaround Time(MRTT)
+ * "The minimum amount of time that OSPM must wait after the completion
+ * of a command before issuing the next command, in microseconds"
+ */
+ if (pcc_subspace->pcc_mrtt) {
+ time_delta = ktime_us_delta(ktime_get(), last_cmd_cmpl_time);
+ if (pcc_subspace->pcc_mrtt > time_delta)
+ udelay(pcc_subspace->pcc_mrtt - time_delta);
+ }
+
+ /*
+ * Handle the non-zero Maximum Periodic Access Rate(MPAR)
+ * "The maximum number of periodic requests that the subspace channel can
+ * support, reported in commands per minute. 0 indicates no limitation."
+ *
+ * This parameter should be ideally zero or large enough so that it can
+ * handle maximum number of requests that all the cores in the system can
+ * collectively generate. If it is not, we will follow the spec and just
+ * not send the request to the platform after hitting the MPAR limit in
+ * any 60s window
+ */
+ if (pcc_subspace->pcc_mpar) {
+ if (mpar_count == 0) {
+ time_delta = ktime_ms_delta(ktime_get(), last_mpar_reset);
+ if (time_delta < 60 * MSEC_PER_SEC) {
+ dev_dbg(ras2_ctx->dev,
+ "PCC cmd not sent due to MPAR limit");
+ return -EIO;
+ }
+ last_mpar_reset = ktime_get();
+ mpar_count = pcc_subspace->pcc_mpar;
+ }
+ mpar_count--;
+ }
+
+ /* Write to the shared comm region. */
+ writew_relaxed(cmd, &generic_comm_base->command);
+
+ /* Flip CMD COMPLETE bit */
+ writew_relaxed(0, &generic_comm_base->status);
+
+ /* Ring doorbell */
+ ret = mbox_send_message(pcc_channel, &cmd);
+ if (ret < 0) {
+ dev_err(ras2_ctx->dev,
+ "Err sending PCC mbox message. cmd:%d, ret:%d\n",
+ cmd, ret);
+ return ret;
+ }
+
+ /*
+ * If Minimum Request Turnaround Time is non-zero, we need
+ * to record the completion time of both READ and WRITE
+ * command for proper handling of MRTT, so we need to check
+ * for pcc_mrtt in addition to CMD_READ
+ */
+ if (cmd == RAS2_PCC_CMD_EXEC || pcc_subspace->pcc_mrtt) {
+ ret = ras2_check_pcc_chan(pcc_subspace);
+ if (pcc_subspace->pcc_mrtt)
+ last_cmd_cmpl_time = ktime_get();
+ }
+
+ if (pcc_channel->mbox->txdone_irq)
+ mbox_chan_txdone(pcc_channel, ret);
+ else
+ mbox_client_txdone(pcc_channel, ret);
+
+ return ret >= 0 ? 0 : ret;
+}
+EXPORT_SYMBOL_GPL(ras2_send_pcc_cmd);
+
+static int ras2_register_pcc_channel(struct ras2_mem_ctx *ras2_ctx, int pcc_subspace_id)
+{
+ struct ras2_pcc_subspace *pcc_subspace;
+ struct pcc_mbox_chan *pcc_chan;
+ struct mbox_client *mbox_cl;
+
+ if (pcc_subspace_id < 0)
+ return -EINVAL;
+
+ mutex_lock(&ras2_pcc_subspace_lock);
+ list_for_each_entry(pcc_subspace, &ras2_pcc_subspaces, elem) {
+ if (pcc_subspace->pcc_subspace_id != pcc_subspace_id)
+ continue;
+ ras2_ctx->pcc_subspace = pcc_subspace;
+ pcc_subspace->ref_count++;
+ mutex_unlock(&ras2_pcc_subspace_lock);
+ return 0;
+ }
+ mutex_unlock(&ras2_pcc_subspace_lock);
+
+ pcc_subspace = kcalloc(1, sizeof(*pcc_subspace), GFP_KERNEL);
+ if (!pcc_subspace)
+ return -ENOMEM;
+ mbox_cl = &pcc_subspace->mbox_client;
+ mbox_cl->knows_txdone = true;
+
+ pcc_chan = pcc_mbox_request_channel(mbox_cl, pcc_subspace_id);
+ if (IS_ERR(pcc_chan)) {
+ kfree(pcc_subspace);
+ return PTR_ERR(pcc_chan);
+ }
+ *pcc_subspace = (struct ras2_pcc_subspace) {
+ .pcc_subspace_id = pcc_subspace_id,
+ .pcc_chan = pcc_chan,
+ .pcc_comm_addr = acpi_os_ioremap(pcc_chan->shmem_base_addr,
+ pcc_chan->shmem_size),
+ .deadline = ns_to_ktime(RAS2_NUM_RETRIES *
+ pcc_chan->latency *
+ NSEC_PER_USEC),
+ .pcc_mrtt = pcc_chan->min_turnaround_time,
+ .pcc_mpar = pcc_chan->max_access_rate,
+ .mbox_client = {
+ .knows_txdone = true,
+ },
+ .pcc_channel_acquired = true,
+ };
+ mutex_lock(&ras2_pcc_subspace_lock);
+ list_add(&pcc_subspace->elem, &ras2_pcc_subspaces);
+ pcc_subspace->ref_count++;
+ mutex_unlock(&ras2_pcc_subspace_lock);
+ ras2_ctx->pcc_subspace = pcc_subspace;
+ ras2_ctx->pcc_comm_addr = pcc_subspace->pcc_comm_addr;
+ ras2_ctx->dev = pcc_chan->mchan->mbox->dev;
+
+ return 0;
+}
+
+static DEFINE_IDA(ras2_ida);
+static void ras2_remove_pcc(struct ras2_mem_ctx *ras2_ctx)
+{
+ struct ras2_pcc_subspace *pcc_subspace = ras2_ctx->pcc_subspace;
+
+ guard(mutex)(&ras2_pcc_subspace_lock);
+ if (pcc_subspace->ref_count > 0)
+ pcc_subspace->ref_count--;
+ if (!pcc_subspace->ref_count) {
+ list_del(&pcc_subspace->elem);
+ pcc_mbox_free_channel(pcc_subspace->pcc_chan);
+ kfree(pcc_subspace);
+ }
+}
+
+static void ras2_release(struct device *device)
+{
+ struct auxiliary_device *auxdev = container_of(device, struct auxiliary_device, dev);
+ struct ras2_mem_ctx *ras2_ctx = container_of(auxdev, struct ras2_mem_ctx, adev);
+
+ ida_free(&ras2_ida, auxdev->id);
+ ras2_remove_pcc(ras2_ctx);
+ kfree(ras2_ctx);
+}
+
+static struct ras2_mem_ctx *ras2_add_aux_device(char *name, int channel)
+{
+ struct ras2_mem_ctx *ras2_ctx;
+ int id, ret;
+
+ ras2_ctx = kzalloc(sizeof(*ras2_ctx), GFP_KERNEL);
+ if (!ras2_ctx)
+ return ERR_PTR(-ENOMEM);
+
+ mutex_init(&ras2_ctx->lock);
+
+ ret = ras2_register_pcc_channel(ras2_ctx, channel);
+ if (ret < 0) {
+ pr_debug("failed to register pcc channel ret=%d\n", ret);
+ goto ctx_free;
+ }
+
+ id = ida_alloc(&ras2_ida, GFP_KERNEL);
+ if (id < 0) {
+ ret = id;
+ goto pcc_free;
+ }
+ ras2_ctx->id = id;
+ ras2_ctx->adev.id = id;
+ ras2_ctx->adev.name = RAS2_MEM_DEV_ID_NAME;
+ ras2_ctx->adev.dev.release = ras2_release;
+ ras2_ctx->adev.dev.parent = ras2_ctx->dev;
+
+ ret = auxiliary_device_init(&ras2_ctx->adev);
+ if (ret)
+ goto ida_free;
+
+ ret = auxiliary_device_add(&ras2_ctx->adev);
+ if (ret) {
+ auxiliary_device_uninit(&ras2_ctx->adev);
+ return ERR_PTR(ret);
+ }
+
+ return ras2_ctx;
+
+ida_free:
+ ida_free(&ras2_ida, id);
+pcc_free:
+ ras2_remove_pcc(ras2_ctx);
+ctx_free:
+ kfree(ras2_ctx);
+ return ERR_PTR(ret);
+}
+
+static int ras2_acpi_parse_table(struct acpi_table_header *pAcpiTable)
+{
+ struct acpi_ras2_pcc_desc *pcc_desc_list;
+ struct acpi_table_ras2 *pRas2Table;
+ struct ras2_mem_ctx *ras2_ctx;
+ int pcc_subspace_id;
+ acpi_size ras2_size;
+ acpi_status status;
+ u8 count = 0, i;
+
+ status = acpi_get_table("RAS2", 0, &pAcpiTable);
+ if (ACPI_FAILURE(status) || !pAcpiTable) {
+ pr_err("ACPI RAS2 driver failed to initialize, get table failed\n");
+ return -EINVAL;
+ }
+
+ ras2_size = pAcpiTable->length;
+ if (ras2_size < sizeof(struct acpi_table_ras2)) {
+ pr_err("ACPI RAS2 table present but broken (too short #1)\n");
+ return -EINVAL;
+ }
+
+ pRas2Table = (struct acpi_table_ras2 *)pAcpiTable;
+ if (pRas2Table->num_pcc_descs <= 0) {
+ pr_err("ACPI RAS2 table does not contain PCC descriptors\n");
+ return -EINVAL;
+ }
+
+ pcc_desc_list = (struct acpi_ras2_pcc_desc *)(pRas2Table + 1);
+ /* Double scan for the case of only one actual controller */
+ pcc_subspace_id = -1;
+ count = 0;
+ for (i = 0; i < pRas2Table->num_pcc_descs; i++, pcc_desc_list++) {
+ if (pcc_desc_list->feature_type != RAS2_FEATURE_TYPE_MEMORY)
+ continue;
+ if (pcc_subspace_id == -1) {
+ pcc_subspace_id = pcc_desc_list->channel_id;
+ count++;
+ }
+ if (pcc_desc_list->channel_id != pcc_subspace_id)
+ count++;
+ }
+ /*
+ * Workaround for the client platform with multiple scrub devices
+ * but uses single PCC subspace for communication.
+ */
+ if (count == 1) {
+ /* Add auxiliary device and bind ACPI RAS2 memory driver */
+ ras2_ctx = ras2_add_aux_device(RAS2_MEM_DEV_ID_NAME, pcc_subspace_id);
+ if (IS_ERR(ras2_ctx))
+ return PTR_ERR(ras2_ctx);
+ return 0;
+ }
+
+ pcc_desc_list = (struct acpi_ras2_pcc_desc *)(pRas2Table + 1);
+ count = 0;
+ for (i = 0; i < pRas2Table->num_pcc_descs; i++, pcc_desc_list++) {
+ if (pcc_desc_list->feature_type != RAS2_FEATURE_TYPE_MEMORY)
+ continue;
+ pcc_subspace_id = pcc_desc_list->channel_id;
+ /* Add auxiliary device and bind ACPI RAS2 memory driver */
+ ras2_ctx = ras2_add_aux_device(RAS2_MEM_DEV_ID_NAME, pcc_subspace_id);
+ if (IS_ERR(ras2_ctx))
+ return PTR_ERR(ras2_ctx);
+ }
+
+ return 0;
+}
+
+static int __init ras2_acpi_init(void)
+{
+ struct acpi_table_header *pAcpiTable = NULL;
+ acpi_status status;
+ int ret;
+
+ status = acpi_get_table("RAS2", 0, &pAcpiTable);
+ if (ACPI_FAILURE(status) || !pAcpiTable) {
+ pr_err("ACPI RAS2 driver failed to initialize, get table failed\n");
+ return -EINVAL;
+ }
+
+ ret = ras2_acpi_parse_table(pAcpiTable);
+ if (ret)
+ pr_err("ras2_acpi_parse_table failed\n");
+
+ acpi_put_table(pAcpiTable);
+
+ return ret;
+}
+late_initcall(ras2_acpi_init)
diff --git a/include/acpi/ras2_acpi.h b/include/acpi/ras2_acpi.h
new file mode 100644
index 000000000000..0d24c42eb34f
--- /dev/null
+++ b/include/acpi/ras2_acpi.h
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * RAS2 ACPI driver header file
+ *
+ * (C) Copyright 2014, 2015 Hewlett-Packard Enterprises
+ *
+ * Copyright (c) 2024-2025 HiSilicon Limited
+ */
+
+#ifndef _RAS2_ACPI_H
+#define _RAS2_ACPI_H
+
+#include <linux/acpi.h>
+#include <linux/auxiliary_bus.h>
+#include <linux/mailbox_client.h>
+#include <linux/mutex.h>
+#include <linux/types.h>
+
+#define RAS2_PCC_CMD_COMPLETE BIT(0)
+#define RAS2_PCC_CMD_ERROR BIT(2)
+
+/* RAS2 specific PCC commands */
+#define RAS2_PCC_CMD_EXEC 0x01
+
+#define RAS2_AUX_DEV_NAME "ras2"
+#define RAS2_MEM_DEV_ID_NAME "acpi_ras2_mem"
+
+/* Data structure RAS2 table */
+struct ras2_mem_ctx {
+ struct auxiliary_device adev;
+ /* Lock to provide mutually exclusive access to PCC channel */
+ struct mutex lock;
+ struct device *dev;
+ struct device *scrub_dev;
+ struct acpi_ras2_shared_memory __iomem *pcc_comm_addr;
+ void *pcc_subspace;
+ u64 base, size;
+ int id;
+ u8 instance;
+ u8 scrub_cycle_hrs;
+ u8 min_scrub_cycle;
+ u8 max_scrub_cycle;
+ bool bg;
+};
+
+int ras2_send_pcc_cmd(struct ras2_mem_ctx *ras2_ctx, u16 cmd);
+#endif /* _RAS2_ACPI_H */
--
2.43.0
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH linux-next 1/2] ACPI:RAS2: Add ACPI RAS2 driver
2025-02-28 12:27 ` [PATCH linux-next 1/2] ACPI:RAS2: Add ACPI RAS2 driver shiju.jose
@ 2025-03-03 21:06 ` Yazen Ghannam
2025-03-04 16:59 ` Shiju Jose
0 siblings, 1 reply; 11+ messages in thread
From: Yazen Ghannam @ 2025-03-03 21:06 UTC (permalink / raw)
To: shiju.jose
Cc: linux-edac, linux-acpi, rafael, bp, tony.luck, lenb, mchehab,
linux-mm, linux-kernel, linux-cxl, j.williams, dave,
jonathan.cameron, dave.jiang, alison.schofield, vishal.l.verma,
ira.weiny, david, Vilas.Sridharan, leo.duran, rientjes, jiaqiyan,
Jon.Grimm, dave.hansen, naoya.horiguchi, james.morse, jthoughton,
somasundaram.a, erdemaktas, pgonda, duenwen, gthelen, wschwartz,
dferguson, wbs, nifan.cxl, tanxiaofei, prime.zeng, roberto.sassu,
kangkang.shen, wanghuiqiang, linuxarm
On Fri, Feb 28, 2025 at 12:27:49PM +0000, shiju.jose@huawei.com wrote:
> From: Shiju Jose <shiju.jose@huawei.com>
>
> Add support for ACPI RAS2 feature table (RAS2) defined in the
> ACPI 6.5 Specification, section 5.2.21.
> Driver contains RAS2 Init, which extracts the RAS2 table and driver
> adds auxiliary device for each memory feature which binds to the
> RAS2 memory driver.
>
> Driver uses PCC mailbox to communicate with the ACPI HW and the
> driver adds OSPM interfaces to send RAS2 commands.
>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Co-developed-by: A Somasundaram <somasundaram.a@hpe.com>
> Signed-off-by: A Somasundaram <somasundaram.a@hpe.com>
> Co-developed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Tested-by: Daniel Ferguson <danielf@os.amperecomputing.com>
> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> ---
> drivers/acpi/Kconfig | 11 ++
> drivers/acpi/Makefile | 1 +
> drivers/acpi/ras2.c | 417 +++++++++++++++++++++++++++++++++++++++
> include/acpi/ras2_acpi.h | 47 +++++
> 4 files changed, 476 insertions(+)
> create mode 100755 drivers/acpi/ras2.c
> create mode 100644 include/acpi/ras2_acpi.h
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 7f10aa38269d..7b470cf2fd71 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -293,6 +293,17 @@ config ACPI_CPPC_LIB
> If your platform does not support CPPC in firmware,
> leave this option disabled.
>
> +config ACPI_RAS2
> + bool "ACPI RAS2 driver"
> + select AUXILIARY_BUS
> + select MAILBOX
> + select PCC
> + help
> + The driver adds support for ACPI RAS2 feature table(extracts RAS2
> + table from OS system table) and OSPM interfaces to send RAS2
> + commands via PCC mailbox subspace. Driver adds platform device for
> + the RAS2 memory features which binds to the RAS2 memory driver.
> +
> config ACPI_PROCESSOR
> tristate "Processor"
> depends on X86 || ARM64 || LOONGARCH || RISCV
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 40208a0f5dfb..797b38cdc2f3 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -100,6 +100,7 @@ obj-$(CONFIG_ACPI_EC_DEBUGFS) += ec_sys.o
> obj-$(CONFIG_ACPI_BGRT) += bgrt.o
> obj-$(CONFIG_ACPI_CPPC_LIB) += cppc_acpi.o
> obj-$(CONFIG_ACPI_SPCR_TABLE) += spcr.o
> +obj-$(CONFIG_ACPI_RAS2) += ras2.o
> obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
> obj-$(CONFIG_ACPI_PPTT) += pptt.o
> obj-$(CONFIG_ACPI_PFRUT) += pfr_update.o pfr_telemetry.o
> diff --git a/drivers/acpi/ras2.c b/drivers/acpi/ras2.c
> new file mode 100755
> index 000000000000..cc8eef49c158
> --- /dev/null
> +++ b/drivers/acpi/ras2.c
> @@ -0,0 +1,417 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Implementation of ACPI RAS2 driver.
> + *
> + * Copyright (c) 2024-2025 HiSilicon Limited.
> + *
> + * Support for RAS2 - ACPI 6.5 Specification, section 5.2.21
> + *
> + * Driver contains ACPI RAS2 init, which extracts the ACPI RAS2 table and
> + * get the PCC channel subspace for communicating with the ACPI compliant
> + * HW platform which supports ACPI RAS2. Driver adds platform devices
> + * for each RAS2 memory feature which binds to the memory ACPI RAS2 driver.
> + */
> +
> +#define pr_fmt(fmt) "ACPI RAS2: " fmt
> +
> +#include <linux/delay.h>
> +#include <linux/export.h>
> +#include <linux/ktime.h>
> +#include <linux/platform_device.h>
> +#include <acpi/pcc.h>
> +#include <acpi/ras2_acpi.h>
> +
> +/* Data structure for PCC communication */
> +struct ras2_pcc_subspace {
> + int pcc_subspace_id;
> + struct mbox_client mbox_client;
> + struct pcc_mbox_chan *pcc_chan;
> + struct acpi_ras2_shared_memory __iomem *pcc_comm_addr;
> + bool pcc_channel_acquired;
> + ktime_t deadline;
> + unsigned int pcc_mpar;
> + unsigned int pcc_mrtt;
> + struct list_head elem;
> + u16 ref_count;
> +};
> +
> +/*
> + * Arbitrary Retries for PCC commands because the
> + * remote processor could be much slower to reply.
> + */
> +#define RAS2_NUM_RETRIES 600
> +
> +#define RAS2_FEATURE_TYPE_MEMORY 0x00
> +
> +/* global variables for the RAS2 PCC subspaces */
> +static DEFINE_MUTEX(ras2_pcc_subspace_lock);
> +static LIST_HEAD(ras2_pcc_subspaces);
> +
> +static int ras2_report_cap_error(u32 cap_status)
> +{
> + switch (cap_status) {
> + case ACPI_RAS2_NOT_VALID:
> + case ACPI_RAS2_NOT_SUPPORTED:
> + return -EPERM;
> + case ACPI_RAS2_BUSY:
> + return -EBUSY;
> + case ACPI_RAS2_FAILED:
> + case ACPI_RAS2_ABORTED:
> + case ACPI_RAS2_INVALID_DATA:
> + return -EINVAL;
> + default: /* 0 or other, Success */
> + return 0;
> + }
> +}
> +
> +static int ras2_check_pcc_chan(struct ras2_pcc_subspace *pcc_subspace)
> +{
> + struct acpi_ras2_shared_memory __iomem *generic_comm_base = pcc_subspace->pcc_comm_addr;
> + ktime_t next_deadline = ktime_add(ktime_get(), pcc_subspace->deadline);
> + u32 cap_status;
> + u16 status;
> + u32 ret;
> +
> + while (!ktime_after(ktime_get(), next_deadline)) {
> + /*
> + * As per ACPI spec, the PCC space will be initialized by
> + * platform and should have set the command completion bit when
> + * PCC can be used by OSPM
> + */
> + status = readw_relaxed(&generic_comm_base->status);
> + if (status & RAS2_PCC_CMD_ERROR) {
> + cap_status = readw_relaxed(&generic_comm_base->set_capabilities_status);
> + ret = ras2_report_cap_error(cap_status);
> +
> + status &= ~RAS2_PCC_CMD_ERROR;
> + writew_relaxed(status, &generic_comm_base->status);
> + return ret;
> + }
> + if (status & RAS2_PCC_CMD_COMPLETE)
> + return 0;
> + /*
> + * Reducing the bus traffic in case this loop takes longer than
> + * a few retries.
> + */
> + msleep(10);
> + }
> +
> + return -EIO;
> +}
> +
> +/**
> + * ras2_send_pcc_cmd() - Send RAS2 command via PCC channel
> + * @ras2_ctx: pointer to the RAS2 context structure
> + * @cmd: command to send
> + *
> + * Returns: 0 on success, an error otherwise
> + */
> +int ras2_send_pcc_cmd(struct ras2_mem_ctx *ras2_ctx, u16 cmd)
> +{
> + struct ras2_pcc_subspace *pcc_subspace = ras2_ctx->pcc_subspace;
> + struct acpi_ras2_shared_memory *generic_comm_base = pcc_subspace->pcc_comm_addr;
> + static ktime_t last_cmd_cmpl_time, last_mpar_reset;
> + struct mbox_chan *pcc_channel;
> + unsigned int time_delta;
> + static int mpar_count;
> + int ret;
> +
> + guard(mutex)(&ras2_pcc_subspace_lock);
> + ret = ras2_check_pcc_chan(pcc_subspace);
> + if (ret < 0)
> + return ret;
> + pcc_channel = pcc_subspace->pcc_chan->mchan;
> +
> + /*
> + * Handle the Minimum Request Turnaround Time(MRTT)
> + * "The minimum amount of time that OSPM must wait after the completion
> + * of a command before issuing the next command, in microseconds"
> + */
> + if (pcc_subspace->pcc_mrtt) {
> + time_delta = ktime_us_delta(ktime_get(), last_cmd_cmpl_time);
> + if (pcc_subspace->pcc_mrtt > time_delta)
> + udelay(pcc_subspace->pcc_mrtt - time_delta);
> + }
> +
> + /*
> + * Handle the non-zero Maximum Periodic Access Rate(MPAR)
> + * "The maximum number of periodic requests that the subspace channel can
> + * support, reported in commands per minute. 0 indicates no limitation."
> + *
> + * This parameter should be ideally zero or large enough so that it can
> + * handle maximum number of requests that all the cores in the system can
> + * collectively generate. If it is not, we will follow the spec and just
> + * not send the request to the platform after hitting the MPAR limit in
> + * any 60s window
> + */
> + if (pcc_subspace->pcc_mpar) {
> + if (mpar_count == 0) {
> + time_delta = ktime_ms_delta(ktime_get(), last_mpar_reset);
> + if (time_delta < 60 * MSEC_PER_SEC) {
> + dev_dbg(ras2_ctx->dev,
> + "PCC cmd not sent due to MPAR limit");
> + return -EIO;
> + }
> + last_mpar_reset = ktime_get();
> + mpar_count = pcc_subspace->pcc_mpar;
> + }
> + mpar_count--;
> + }
> +
> + /* Write to the shared comm region. */
> + writew_relaxed(cmd, &generic_comm_base->command);
> +
> + /* Flip CMD COMPLETE bit */
> + writew_relaxed(0, &generic_comm_base->status);
> +
> + /* Ring doorbell */
> + ret = mbox_send_message(pcc_channel, &cmd);
> + if (ret < 0) {
> + dev_err(ras2_ctx->dev,
> + "Err sending PCC mbox message. cmd:%d, ret:%d\n",
> + cmd, ret);
> + return ret;
> + }
> +
> + /*
> + * If Minimum Request Turnaround Time is non-zero, we need
> + * to record the completion time of both READ and WRITE
> + * command for proper handling of MRTT, so we need to check
> + * for pcc_mrtt in addition to CMD_READ
> + */
> + if (cmd == RAS2_PCC_CMD_EXEC || pcc_subspace->pcc_mrtt) {
> + ret = ras2_check_pcc_chan(pcc_subspace);
> + if (pcc_subspace->pcc_mrtt)
> + last_cmd_cmpl_time = ktime_get();
> + }
> +
> + if (pcc_channel->mbox->txdone_irq)
> + mbox_chan_txdone(pcc_channel, ret);
> + else
> + mbox_client_txdone(pcc_channel, ret);
> +
> + return ret >= 0 ? 0 : ret;
> +}
> +EXPORT_SYMBOL_GPL(ras2_send_pcc_cmd);
> +
> +static int ras2_register_pcc_channel(struct ras2_mem_ctx *ras2_ctx, int pcc_subspace_id)
> +{
> + struct ras2_pcc_subspace *pcc_subspace;
> + struct pcc_mbox_chan *pcc_chan;
> + struct mbox_client *mbox_cl;
> +
> + if (pcc_subspace_id < 0)
> + return -EINVAL;
> +
> + mutex_lock(&ras2_pcc_subspace_lock);
> + list_for_each_entry(pcc_subspace, &ras2_pcc_subspaces, elem) {
> + if (pcc_subspace->pcc_subspace_id != pcc_subspace_id)
> + continue;
> + ras2_ctx->pcc_subspace = pcc_subspace;
> + pcc_subspace->ref_count++;
> + mutex_unlock(&ras2_pcc_subspace_lock);
> + return 0;
> + }
> + mutex_unlock(&ras2_pcc_subspace_lock);
> +
> + pcc_subspace = kcalloc(1, sizeof(*pcc_subspace), GFP_KERNEL);
> + if (!pcc_subspace)
> + return -ENOMEM;
> + mbox_cl = &pcc_subspace->mbox_client;
> + mbox_cl->knows_txdone = true;
> +
> + pcc_chan = pcc_mbox_request_channel(mbox_cl, pcc_subspace_id);
> + if (IS_ERR(pcc_chan)) {
> + kfree(pcc_subspace);
> + return PTR_ERR(pcc_chan);
> + }
> + *pcc_subspace = (struct ras2_pcc_subspace) {
> + .pcc_subspace_id = pcc_subspace_id,
> + .pcc_chan = pcc_chan,
> + .pcc_comm_addr = acpi_os_ioremap(pcc_chan->shmem_base_addr,
> + pcc_chan->shmem_size),
> + .deadline = ns_to_ktime(RAS2_NUM_RETRIES *
> + pcc_chan->latency *
> + NSEC_PER_USEC),
> + .pcc_mrtt = pcc_chan->min_turnaround_time,
> + .pcc_mpar = pcc_chan->max_access_rate,
> + .mbox_client = {
> + .knows_txdone = true,
> + },
> + .pcc_channel_acquired = true,
> + };
There are quite a few places where I'd expect a newline after "}" and
"return" statements. This is just one example.
> + mutex_lock(&ras2_pcc_subspace_lock);
> + list_add(&pcc_subspace->elem, &ras2_pcc_subspaces);
> + pcc_subspace->ref_count++;
> + mutex_unlock(&ras2_pcc_subspace_lock);
> + ras2_ctx->pcc_subspace = pcc_subspace;
> + ras2_ctx->pcc_comm_addr = pcc_subspace->pcc_comm_addr;
> + ras2_ctx->dev = pcc_chan->mchan->mbox->dev;
> +
> + return 0;
> +}
> +
> +static DEFINE_IDA(ras2_ida);
> +static void ras2_remove_pcc(struct ras2_mem_ctx *ras2_ctx)
> +{
> + struct ras2_pcc_subspace *pcc_subspace = ras2_ctx->pcc_subspace;
> +
> + guard(mutex)(&ras2_pcc_subspace_lock);
> + if (pcc_subspace->ref_count > 0)
> + pcc_subspace->ref_count--;
> + if (!pcc_subspace->ref_count) {
> + list_del(&pcc_subspace->elem);
> + pcc_mbox_free_channel(pcc_subspace->pcc_chan);
> + kfree(pcc_subspace);
> + }
> +}
> +
> +static void ras2_release(struct device *device)
> +{
> + struct auxiliary_device *auxdev = container_of(device, struct auxiliary_device, dev);
> + struct ras2_mem_ctx *ras2_ctx = container_of(auxdev, struct ras2_mem_ctx, adev);
> +
> + ida_free(&ras2_ida, auxdev->id);
> + ras2_remove_pcc(ras2_ctx);
> + kfree(ras2_ctx);
> +}
> +
> +static struct ras2_mem_ctx *ras2_add_aux_device(char *name, int channel)
Why is the return type "struct ras2_mem_ctx *"? It is not used by the
calling function.
Just use return type "int".
> +{
> + struct ras2_mem_ctx *ras2_ctx;
> + int id, ret;
> +
> + ras2_ctx = kzalloc(sizeof(*ras2_ctx), GFP_KERNEL);
> + if (!ras2_ctx)
> + return ERR_PTR(-ENOMEM);
> +
> + mutex_init(&ras2_ctx->lock);
> +
> + ret = ras2_register_pcc_channel(ras2_ctx, channel);
> + if (ret < 0) {
> + pr_debug("failed to register pcc channel ret=%d\n", ret);
> + goto ctx_free;
> + }
> +
> + id = ida_alloc(&ras2_ida, GFP_KERNEL);
> + if (id < 0) {
> + ret = id;
> + goto pcc_free;
> + }
> + ras2_ctx->id = id;
> + ras2_ctx->adev.id = id;
> + ras2_ctx->adev.name = RAS2_MEM_DEV_ID_NAME;
> + ras2_ctx->adev.dev.release = ras2_release;
> + ras2_ctx->adev.dev.parent = ras2_ctx->dev;
> +
> + ret = auxiliary_device_init(&ras2_ctx->adev);
> + if (ret)
> + goto ida_free;
> +
> + ret = auxiliary_device_add(&ras2_ctx->adev);
> + if (ret) {
> + auxiliary_device_uninit(&ras2_ctx->adev);
> + return ERR_PTR(ret);
> + }
> +
> + return ras2_ctx;
> +
> +ida_free:
> + ida_free(&ras2_ida, id);
> +pcc_free:
> + ras2_remove_pcc(ras2_ctx);
> +ctx_free:
> + kfree(ras2_ctx);
> + return ERR_PTR(ret);
> +}
> +
> +static int ras2_acpi_parse_table(struct acpi_table_header *pAcpiTable)
> +{
> + struct acpi_ras2_pcc_desc *pcc_desc_list;
> + struct acpi_table_ras2 *pRas2Table;
> + struct ras2_mem_ctx *ras2_ctx;
> + int pcc_subspace_id;
> + acpi_size ras2_size;
> + acpi_status status;
> + u8 count = 0, i;
> +
> + status = acpi_get_table("RAS2", 0, &pAcpiTable);
> + if (ACPI_FAILURE(status) || !pAcpiTable) {
> + pr_err("ACPI RAS2 driver failed to initialize, get table failed\n");
> + return -EINVAL;
> + }
You already got the table in the init function and passed its pointer.
Why do you need to get it again?
Also, you can just save a global pointer to the table if you need to
access it multiple times. Please see my comments for the init function.
You can do something similar to acpi_hest_init().
> +
> + ras2_size = pAcpiTable->length;
> + if (ras2_size < sizeof(struct acpi_table_ras2)) {
> + pr_err("ACPI RAS2 table present but broken (too short #1)\n");
This should include a "FW_WARN" prefix since the firmware should have
constructed a valid table.
> + return -EINVAL;
> + }
> +
> + pRas2Table = (struct acpi_table_ras2 *)pAcpiTable;
> + if (pRas2Table->num_pcc_descs <= 0) {
num_pcc_descs is a u16. It cannot be "< 0".
> + pr_err("ACPI RAS2 table does not contain PCC descriptors\n");
Please include "FW_WARN" prefix.
> + return -EINVAL;
> + }
> +
> + pcc_desc_list = (struct acpi_ras2_pcc_desc *)(pRas2Table + 1);
> + /* Double scan for the case of only one actual controller */
> + pcc_subspace_id = -1;
> + count = 0;
> + for (i = 0; i < pRas2Table->num_pcc_descs; i++, pcc_desc_list++) {
> + if (pcc_desc_list->feature_type != RAS2_FEATURE_TYPE_MEMORY)
> + continue;
> + if (pcc_subspace_id == -1) {
> + pcc_subspace_id = pcc_desc_list->channel_id;
> + count++;
> + }
> + if (pcc_desc_list->channel_id != pcc_subspace_id)
> + count++;
> + }
> + /*
> + * Workaround for the client platform with multiple scrub devices
> + * but uses single PCC subspace for communication.
> + */
> + if (count == 1) {
> + /* Add auxiliary device and bind ACPI RAS2 memory driver */
> + ras2_ctx = ras2_add_aux_device(RAS2_MEM_DEV_ID_NAME, pcc_subspace_id);
> + if (IS_ERR(ras2_ctx))
> + return PTR_ERR(ras2_ctx);
> + return 0;
> + }
> +
> + pcc_desc_list = (struct acpi_ras2_pcc_desc *)(pRas2Table + 1);
> + count = 0;
> + for (i = 0; i < pRas2Table->num_pcc_descs; i++, pcc_desc_list++) {
> + if (pcc_desc_list->feature_type != RAS2_FEATURE_TYPE_MEMORY)
> + continue;
> + pcc_subspace_id = pcc_desc_list->channel_id;
> + /* Add auxiliary device and bind ACPI RAS2 memory driver */
> + ras2_ctx = ras2_add_aux_device(RAS2_MEM_DEV_ID_NAME, pcc_subspace_id);
> + if (IS_ERR(ras2_ctx))
> + return PTR_ERR(ras2_ctx);
> + }
Why not just try to register *every* PCC identifier you find and have
the register function ignore duplicates? Does it already do this?
> +
> + return 0;
> +}
> +
> +static int __init ras2_acpi_init(void)
> +{
> + struct acpi_table_header *pAcpiTable = NULL;
> + acpi_status status;
> + int ret;
> +
> + status = acpi_get_table("RAS2", 0, &pAcpiTable);
> + if (ACPI_FAILURE(status) || !pAcpiTable) {
> + pr_err("ACPI RAS2 driver failed to initialize, get table failed\n");
> + return -EINVAL;
> + }
> +
> + ret = ras2_acpi_parse_table(pAcpiTable);
> + if (ret)
> + pr_err("ras2_acpi_parse_table failed\n");
> +
> + acpi_put_table(pAcpiTable);
> +
> + return ret;
> +}
> +late_initcall(ras2_acpi_init)
Can this init function be called in acpi_init() along with other ACPI
tables?
I think the function can follow a similar structure to acpi_hest_init()
and acpi_ghes_init().
For example, the name could be acpi_ras2_init() to follow the same
format as other acpi_*_init() functions.
Most of these functions are called after other plumbing, like PCC, is
already initialized.
> diff --git a/include/acpi/ras2_acpi.h b/include/acpi/ras2_acpi.h
> new file mode 100644
> index 000000000000..0d24c42eb34f
> --- /dev/null
> +++ b/include/acpi/ras2_acpi.h
> @@ -0,0 +1,47 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * RAS2 ACPI driver header file
> + *
> + * (C) Copyright 2014, 2015 Hewlett-Packard Enterprises
Are these years correct?
> + *
> + * Copyright (c) 2024-2025 HiSilicon Limited
> + */
> +
> +#ifndef _RAS2_ACPI_H
> +#define _RAS2_ACPI_H
> +
> +#include <linux/acpi.h>
> +#include <linux/auxiliary_bus.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/mutex.h>
> +#include <linux/types.h>
> +
> +#define RAS2_PCC_CMD_COMPLETE BIT(0)
> +#define RAS2_PCC_CMD_ERROR BIT(2)
> +
> +/* RAS2 specific PCC commands */
> +#define RAS2_PCC_CMD_EXEC 0x01
> +
> +#define RAS2_AUX_DEV_NAME "ras2"
> +#define RAS2_MEM_DEV_ID_NAME "acpi_ras2_mem"
> +
> +/* Data structure RAS2 table */
> +struct ras2_mem_ctx {
> + struct auxiliary_device adev;
> + /* Lock to provide mutually exclusive access to PCC channel */
> + struct mutex lock;
> + struct device *dev;
> + struct device *scrub_dev;
> + struct acpi_ras2_shared_memory __iomem *pcc_comm_addr;
> + void *pcc_subspace;
> + u64 base, size;
> + int id;
> + u8 instance;
> + u8 scrub_cycle_hrs;
> + u8 min_scrub_cycle;
> + u8 max_scrub_cycle;
> + bool bg;
What is "bg"?
It's not used in this patch. Maybe it can be added in the patch when it
is first used? Same for others.
> +};
> +
> +int ras2_send_pcc_cmd(struct ras2_mem_ctx *ras2_ctx, u16 cmd);
> +#endif /* _RAS2_ACPI_H */
> --
Thanks,
Yazen
^ permalink raw reply [flat|nested] 11+ messages in thread* RE: [PATCH linux-next 1/2] ACPI:RAS2: Add ACPI RAS2 driver
2025-03-03 21:06 ` Yazen Ghannam
@ 2025-03-04 16:59 ` Shiju Jose
0 siblings, 0 replies; 11+ messages in thread
From: Shiju Jose @ 2025-03-04 16:59 UTC (permalink / raw)
To: Yazen Ghannam
Cc: linux-edac, linux-acpi, rafael, bp, tony.luck, lenb, mchehab,
linux-mm, linux-kernel, linux-cxl, j.williams, dave,
Jonathan Cameron, dave.jiang, alison.schofield, vishal.l.verma,
ira.weiny, david, Vilas.Sridharan, leo.duran, rientjes, jiaqiyan,
Jon.Grimm, dave.hansen, naoya.horiguchi, james.morse, jthoughton,
somasundaram.a, erdemaktas, pgonda, duenwen, gthelen, wschwartz,
dferguson, wbs, nifan.cxl, tanxiaofei, Zengtao (B),
Roberto Sassu, kangkang.shen, wanghuiqiang, Linuxarm
>-----Original Message-----
>From: Yazen Ghannam <yazen.ghannam@amd.com>
>Sent: 03 March 2025 21:07
>To: Shiju Jose <shiju.jose@huawei.com>
>Cc: linux-edac@vger.kernel.org; linux-acpi@vger.kernel.org; rafael@kernel.org;
>bp@alien8.de; tony.luck@intel.com; lenb@kernel.org; mchehab@kernel.org;
>linux-mm@kvack.org; linux-kernel@vger.kernel.org; linux-cxl@vger.kernel.org;
>j.williams@intel.com; dave@stgolabs.net; Jonathan Cameron
><jonathan.cameron@huawei.com>; dave.jiang@intel.com;
>alison.schofield@intel.com; vishal.l.verma@intel.com; ira.weiny@intel.com;
>david@redhat.com; Vilas.Sridharan@amd.com; leo.duran@amd.com;
>rientjes@google.com; jiaqiyan@google.com; Jon.Grimm@amd.com;
>dave.hansen@linux.intel.com; naoya.horiguchi@nec.com;
>james.morse@arm.com; jthoughton@google.com; somasundaram.a@hpe.com;
>erdemaktas@google.com; pgonda@google.com; duenwen@google.com;
>gthelen@google.com; wschwartz@amperecomputing.com;
>dferguson@amperecomputing.com; wbs@os.amperecomputing.com;
>nifan.cxl@gmail.com; tanxiaofei <tanxiaofei@huawei.com>; Zengtao (B)
><prime.zeng@hisilicon.com>; Roberto Sassu <roberto.sassu@huawei.com>;
>kangkang.shen@futurewei.com; wanghuiqiang <wanghuiqiang@huawei.com>;
>Linuxarm <linuxarm@huawei.com>
>Subject: Re: [PATCH linux-next 1/2] ACPI:RAS2: Add ACPI RAS2 driver
>
>On Fri, Feb 28, 2025 at 12:27:49PM +0000, shiju.jose@huawei.com wrote:
>> From: Shiju Jose <shiju.jose@huawei.com>
>>
>> Add support for ACPI RAS2 feature table (RAS2) defined in the ACPI 6.5
>> Specification, section 5.2.21.
>> Driver contains RAS2 Init, which extracts the RAS2 table and driver
>> adds auxiliary device for each memory feature which binds to the
>> RAS2 memory driver.
>>
>> Driver uses PCC mailbox to communicate with the ACPI HW and the driver
>> adds OSPM interfaces to send RAS2 commands.
>>
>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Co-developed-by: A Somasundaram <somasundaram.a@hpe.com>
>> Signed-off-by: A Somasundaram <somasundaram.a@hpe.com>
>> Co-developed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Tested-by: Daniel Ferguson <danielf@os.amperecomputing.com>
>> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
>> ---
>> drivers/acpi/Kconfig | 11 ++
>> drivers/acpi/Makefile | 1 +
>> drivers/acpi/ras2.c | 417 +++++++++++++++++++++++++++++++++++++++
>> include/acpi/ras2_acpi.h | 47 +++++
>> 4 files changed, 476 insertions(+)
>> create mode 100755 drivers/acpi/ras2.c create mode 100644
>> include/acpi/ras2_acpi.h
>>
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index
>> 7f10aa38269d..7b470cf2fd71 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -293,6 +293,17 @@ config ACPI_CPPC_LIB
[...] },
>> + .pcc_channel_acquired = true,
>> + };
>
>There are quite a few places where I'd expect a newline after "}" and "return"
>statements. This is just one example.
Hi Yazen,
Thanks for the feedbacks.
Will check and modify.
>
>> + mutex_lock(&ras2_pcc_subspace_lock);
>> + list_add(&pcc_subspace->elem, &ras2_pcc_subspaces);
>> + pcc_subspace->ref_count++;
>> + mutex_unlock(&ras2_pcc_subspace_lock);
>> + ras2_ctx->pcc_subspace = pcc_subspace;
>> + ras2_ctx->pcc_comm_addr = pcc_subspace->pcc_comm_addr;
>> + ras2_ctx->dev = pcc_chan->mchan->mbox->dev;
>> +
>> + return 0;
>> +}
>> +
>> +static DEFINE_IDA(ras2_ida);
>> +static void ras2_remove_pcc(struct ras2_mem_ctx *ras2_ctx) {
>> + struct ras2_pcc_subspace *pcc_subspace = ras2_ctx->pcc_subspace;
>> +
>> + guard(mutex)(&ras2_pcc_subspace_lock);
>> + if (pcc_subspace->ref_count > 0)
>> + pcc_subspace->ref_count--;
>> + if (!pcc_subspace->ref_count) {
>> + list_del(&pcc_subspace->elem);
>> + pcc_mbox_free_channel(pcc_subspace->pcc_chan);
>> + kfree(pcc_subspace);
>> + }
>> +}
>> +
>> +static void ras2_release(struct device *device) {
>> + struct auxiliary_device *auxdev = container_of(device, struct
>auxiliary_device, dev);
>> + struct ras2_mem_ctx *ras2_ctx = container_of(auxdev, struct
>> +ras2_mem_ctx, adev);
>> +
>> + ida_free(&ras2_ida, auxdev->id);
>> + ras2_remove_pcc(ras2_ctx);
>> + kfree(ras2_ctx);
>> +}
>> +
>> +static struct ras2_mem_ctx *ras2_add_aux_device(char *name, int
>> +channel)
>
>Why is the return type "struct ras2_mem_ctx *"? It is not used by the calling
>function.
>
>Just use return type "int".
Modified.
>
>> +{
>> + struct ras2_mem_ctx *ras2_ctx;
>> + int id, ret;
>> +
>> + ras2_ctx = kzalloc(sizeof(*ras2_ctx), GFP_KERNEL);
>> + if (!ras2_ctx)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + mutex_init(&ras2_ctx->lock);
>> +
>> + ret = ras2_register_pcc_channel(ras2_ctx, channel);
>> + if (ret < 0) {
>> + pr_debug("failed to register pcc channel ret=%d\n", ret);
>> + goto ctx_free;
>> + }
>> +
>> + id = ida_alloc(&ras2_ida, GFP_KERNEL);
>> + if (id < 0) {
>> + ret = id;
>> + goto pcc_free;
>> + }
>> + ras2_ctx->id = id;
>> + ras2_ctx->adev.id = id;
>> + ras2_ctx->adev.name = RAS2_MEM_DEV_ID_NAME;
>> + ras2_ctx->adev.dev.release = ras2_release;
>> + ras2_ctx->adev.dev.parent = ras2_ctx->dev;
>> +
>> + ret = auxiliary_device_init(&ras2_ctx->adev);
>> + if (ret)
>> + goto ida_free;
>> +
>> + ret = auxiliary_device_add(&ras2_ctx->adev);
>> + if (ret) {
>> + auxiliary_device_uninit(&ras2_ctx->adev);
>> + return ERR_PTR(ret);
>> + }
>> +
>> + return ras2_ctx;
>> +
>> +ida_free:
>> + ida_free(&ras2_ida, id);
>> +pcc_free:
>> + ras2_remove_pcc(ras2_ctx);
>> +ctx_free:
>> + kfree(ras2_ctx);
>> + return ERR_PTR(ret);
>> +}
>> +
>> +static int ras2_acpi_parse_table(struct acpi_table_header
>> +*pAcpiTable) {
>> + struct acpi_ras2_pcc_desc *pcc_desc_list;
>> + struct acpi_table_ras2 *pRas2Table;
>> + struct ras2_mem_ctx *ras2_ctx;
>> + int pcc_subspace_id;
>> + acpi_size ras2_size;
>> + acpi_status status;
>> + u8 count = 0, i;
>> +
>> + status = acpi_get_table("RAS2", 0, &pAcpiTable);
>> + if (ACPI_FAILURE(status) || !pAcpiTable) {
>> + pr_err("ACPI RAS2 driver failed to initialize, get table failed\n");
>> + return -EINVAL;
>> + }
>
>You already got the table in the init function and passed its pointer.
>Why do you need to get it again?
Deleted. This was a duplication when parsing table separated into another function in v19.
>
>Also, you can just save a global pointer to the table if you need to access it
>multiple times. Please see my comments for the init function.
>You can do something similar to acpi_hest_init().
Ok.
>
>> +
>> + ras2_size = pAcpiTable->length;
>> + if (ras2_size < sizeof(struct acpi_table_ras2)) {
>> + pr_err("ACPI RAS2 table present but broken (too short #1)\n");
>
>This should include a "FW_WARN" prefix since the firmware should have
>constructed a valid table.
Added.
>
>> + return -EINVAL;
>> + }
>> +
>> + pRas2Table = (struct acpi_table_ras2 *)pAcpiTable;
>> + if (pRas2Table->num_pcc_descs <= 0) {
>
>num_pcc_descs is a u16. It cannot be "< 0".
>
>> + pr_err("ACPI RAS2 table does not contain PCC descriptors\n");
>
>Please include "FW_WARN" prefix.
Added.
>
>> + return -EINVAL;
>> + }
>> +
>> + pcc_desc_list = (struct acpi_ras2_pcc_desc *)(pRas2Table + 1);
>> + /* Double scan for the case of only one actual controller */
>> + pcc_subspace_id = -1;
>> + count = 0;
>> + for (i = 0; i < pRas2Table->num_pcc_descs; i++, pcc_desc_list++) {
>> + if (pcc_desc_list->feature_type !=
>RAS2_FEATURE_TYPE_MEMORY)
>> + continue;
>> + if (pcc_subspace_id == -1) {
>> + pcc_subspace_id = pcc_desc_list->channel_id;
>> + count++;
>> + }
>> + if (pcc_desc_list->channel_id != pcc_subspace_id)
>> + count++;
>> + }
>> + /*
>> + * Workaround for the client platform with multiple scrub devices
>> + * but uses single PCC subspace for communication.
>> + */
>> + if (count == 1) {
>> + /* Add auxiliary device and bind ACPI RAS2 memory driver */
>> + ras2_ctx = ras2_add_aux_device(RAS2_MEM_DEV_ID_NAME,
>pcc_subspace_id);
>> + if (IS_ERR(ras2_ctx))
>> + return PTR_ERR(ras2_ctx);
>> + return 0;
>> + }
>> +
>> + pcc_desc_list = (struct acpi_ras2_pcc_desc *)(pRas2Table + 1);
>> + count = 0;
>> + for (i = 0; i < pRas2Table->num_pcc_descs; i++, pcc_desc_list++) {
>> + if (pcc_desc_list->feature_type !=
>RAS2_FEATURE_TYPE_MEMORY)
>> + continue;
>> + pcc_subspace_id = pcc_desc_list->channel_id;
>> + /* Add auxiliary device and bind ACPI RAS2 memory driver */
>> + ras2_ctx = ras2_add_aux_device(RAS2_MEM_DEV_ID_NAME,
>pcc_subspace_id);
>> + if (IS_ERR(ras2_ctx))
>> + return PTR_ERR(ras2_ctx);
>> + }
>
>Why not just try to register *every* PCC identifier you find and have the register
>function ignore duplicates? Does it already do this?
Presently driver registers all PCC IDs for the memory RAS Feature Type only, as ACPI spec rev 6.5
RAS2 supports memory features only. Also duplication of the PCC IDs are taken care during registration
in the ras2_register_pcc_channel().
>
>> +
>> + return 0;
>> +}
>> +
>> +static int __init ras2_acpi_init(void) {
>> + struct acpi_table_header *pAcpiTable = NULL;
>> + acpi_status status;
>> + int ret;
>> +
>> + status = acpi_get_table("RAS2", 0, &pAcpiTable);
>> + if (ACPI_FAILURE(status) || !pAcpiTable) {
>> + pr_err("ACPI RAS2 driver failed to initialize, get table failed\n");
>> + return -EINVAL;
>> + }
>> +
>> + ret = ras2_acpi_parse_table(pAcpiTable);
>> + if (ret)
>> + pr_err("ras2_acpi_parse_table failed\n");
>> +
>> + acpi_put_table(pAcpiTable);
>> +
>> + return ret;
>> +}
>> +late_initcall(ras2_acpi_init)
>
>Can this init function be called in acpi_init() along with other ACPI tables?
>
>I think the function can follow a similar structure to acpi_hest_init() and
>acpi_ghes_init().
>
>For example, the name could be acpi_ras2_init() to follow the same format as
>other acpi_*_init() functions.
>
>Most of these functions are called after other plumbing, like PCC, is already
>initialized.
Modified to call from acpi_init().
>
>> diff --git a/include/acpi/ras2_acpi.h b/include/acpi/ras2_acpi.h new
>> file mode 100644 index 000000000000..0d24c42eb34f
>> --- /dev/null
>> +++ b/include/acpi/ras2_acpi.h
>> @@ -0,0 +1,47 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * RAS2 ACPI driver header file
>> + *
>> + * (C) Copyright 2014, 2015 Hewlett-Packard Enterprises
>
>Are these years correct?
Yes. The RAS2 driver was started from the following ACPI RASF driver (which is the old version of RAS2)
https://patchwork.kernel.org/project/linux-arm-kernel/patch/CS1PR84MB0038718F49DBC0FF03919E1184390@CS1PR84MB0038.NAMPRD84.PROD.OUTLOOK.COM/
>
>> + *
>> + * Copyright (c) 2024-2025 HiSilicon Limited */
>> +
>> +#ifndef _RAS2_ACPI_H
>> +#define _RAS2_ACPI_H
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/auxiliary_bus.h>
>> +#include <linux/mailbox_client.h>
>> +#include <linux/mutex.h>
>> +#include <linux/types.h>
>> +
>> +#define RAS2_PCC_CMD_COMPLETE BIT(0)
>> +#define RAS2_PCC_CMD_ERROR BIT(2)
>> +
>> +/* RAS2 specific PCC commands */
>> +#define RAS2_PCC_CMD_EXEC 0x01
>> +
>> +#define RAS2_AUX_DEV_NAME "ras2"
>> +#define RAS2_MEM_DEV_ID_NAME "acpi_ras2_mem"
>> +
>> +/* Data structure RAS2 table */
>> +struct ras2_mem_ctx {
>> + struct auxiliary_device adev;
>> + /* Lock to provide mutually exclusive access to PCC channel */
>> + struct mutex lock;
>> + struct device *dev;
>> + struct device *scrub_dev;
>> + struct acpi_ras2_shared_memory __iomem *pcc_comm_addr;
>> + void *pcc_subspace;
>> + u64 base, size;
>> + int id;
>> + u8 instance;
>> + u8 scrub_cycle_hrs;
>> + u8 min_scrub_cycle;
>> + u8 max_scrub_cycle;
>> + bool bg;
>
>What is "bg"?
>
>It's not used in this patch. Maybe it can be added in the patch when it is first
>used? Same for others.
bg for background (patrol) scrubbing, which is used in the next patch in the series.
Sure. Will add in the next patch.
>
>> +};
>> +
>> +int ras2_send_pcc_cmd(struct ras2_mem_ctx *ras2_ctx, u16 cmd); #endif
>> +/* _RAS2_ACPI_H */
>> --
>
>Thanks,
>Yazen
Thanks,
Shiju
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH linux-next 2/2] ras: mem: Add memory ACPI RAS2 driver
2025-02-28 12:27 [PATCH linux-next 0/2] ACPI: Add support for ACPI RAS2 feature table shiju.jose
2025-02-28 12:27 ` [PATCH linux-next 1/2] ACPI:RAS2: Add ACPI RAS2 driver shiju.jose
@ 2025-02-28 12:27 ` shiju.jose
2025-03-03 9:35 ` [PATCH linux-next 0/2] ACPI: Add support for ACPI RAS2 feature table Jonathan Cameron
2 siblings, 0 replies; 11+ messages in thread
From: shiju.jose @ 2025-02-28 12:27 UTC (permalink / raw)
To: linux-edac, linux-acpi, rafael, bp, tony.luck, lenb, mchehab
Cc: linux-mm, linux-kernel, linux-cxl, j.williams, dave,
jonathan.cameron, dave.jiang, alison.schofield, vishal.l.verma,
ira.weiny, david, Vilas.Sridharan, leo.duran, Yazen.Ghannam,
rientjes, jiaqiyan, Jon.Grimm, dave.hansen, naoya.horiguchi,
james.morse, jthoughton, somasundaram.a, erdemaktas, pgonda,
duenwen, gthelen, wschwartz, dferguson, wbs, nifan.cxl,
yazen.ghannam, tanxiaofei, prime.zeng, roberto.sassu,
kangkang.shen, wanghuiqiang, linuxarm, shiju.jose
From: Shiju Jose <shiju.jose@huawei.com>
Memory ACPI RAS2 auxiliary driver binds to the auxiliary device
add by the ACPI RAS2 table parser.
Driver uses a PCC subspace for communicating with the ACPI compliant
platform.
Device with ACPI RAS2 scrub feature registers with EDAC device driver,
which retrieves the scrub descriptor from EDAC scrub and exposes
the scrub control attributes for RAS2 scrub instance to userspace in
/sys/bus/edac/devices/acpi_ras_mem0/scrubX/.
Co-developed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Tested-by: Daniel Ferguson <danielf@os.amperecomputing.com>
Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
---
Documentation/edac/scrub.rst | 73 +++++++
drivers/ras/Kconfig | 11 +
drivers/ras/Makefile | 1 +
drivers/ras/acpi_ras2.c | 383 +++++++++++++++++++++++++++++++++++
4 files changed, 468 insertions(+)
create mode 100644 drivers/ras/acpi_ras2.c
diff --git a/Documentation/edac/scrub.rst b/Documentation/edac/scrub.rst
index daab929cdba1..7978aa653aa0 100644
--- a/Documentation/edac/scrub.rst
+++ b/Documentation/edac/scrub.rst
@@ -264,3 +264,76 @@ Sysfs files are documented in
`Documentation/ABI/testing/sysfs-edac-scrub`
`Documentation/ABI/testing/sysfs-edac-ecs`
+
+Examples
+--------
+
+The usage takes the form shown in these examples:
+
+1. ACPI RAS2
+
+1.1 On demand scrubbing for a specific memory region.
+
+1.1.1. Query what is device default/current scrub cycle setting.
+
+ Applicable to both on-demand and background scrubbing.
+
+# cat /sys/bus/edac/devices/acpi_ras_mem0/scrub0/current_cycle_duration
+
+36000
+
+1.1.2 Query the range of device supported scrub cycle for a memory region.
+
+# cat /sys/bus/edac/devices/acpi_ras_mem0/scrub0/min_cycle_duration
+
+3600
+
+# cat /sys/bus/edac/devices/acpi_ras_mem0/scrub0/max_cycle_duration
+
+86400
+
+1.1.3. Program scrubbing for the memory region in RAS2 device to repeat every 43200 seconds (half a day).
+
+# echo 43200 > /sys/bus/edac/devices/acpi_ras_mem0/scrub0/current_cycle_duration
+
+1.1.4. Program address and size of the memory region to scrub
+
+Readback 'addr', non-zero - demand scrub is in progress, zero - scrub is finished.
+
+# cat /sys/bus/edac/devices/acpi_ras_mem0/scrub0/addr
+
+0
+
+Write 'size' of the memory region to scrub.
+
+# echo 0x300000 > /sys/bus/edac/devices/acpi_ras_mem0/scrub0/size
+
+Write 'addr' starts demand scrubbing, please make sure other attributes are set prior to that.
+
+# echo 0x200000 > /sys/bus/edac/devices/acpi_ras_mem0/scrub0/addr
+
+Readback 'addr', non-zero - demand scrub is in progress, zero - scrub is finished.
+
+# cat /sys/bus/edac/devices/acpi_ras_mem0/scrub0/addr
+
+0x200000
+
+# cat /sys/bus/edac/devices/acpi_ras_mem0/scrub0/addr
+
+0
+
+1.2 Background scrubbing the entire memory
+
+1.2.3 Query the status of background scrubbing.
+
+# cat /sys/bus/edac/devices/acpi_ras_mem0/scrub0/enable_background
+
+0
+
+1.2.4. Program background scrubbing for RAS2 device to repeat in every 21600 seconds (quarter of a day).
+
+# echo 21600 > /sys/bus/edac/devices/acpi_ras_mem0/scrub0/current_cycle_duration
+
+1.2.5. Start 'background scrubbing'.
+
+# echo 1 > /sys/bus/edac/devices/acpi_ras_mem0/scrub0/enable_background
diff --git a/drivers/ras/Kconfig b/drivers/ras/Kconfig
index fc4f4bb94a4c..a88002f1f462 100644
--- a/drivers/ras/Kconfig
+++ b/drivers/ras/Kconfig
@@ -46,4 +46,15 @@ config RAS_FMPM
Memory will be retired during boot time and run time depending on
platform-specific policies.
+config MEM_ACPI_RAS2
+ tristate "Memory ACPI RAS2 driver"
+ depends on ACPI_RAS2
+ depends on EDAC
+ depends on EDAC_SCRUB
+ help
+ The driver binds to the platform device added by the ACPI RAS2
+ table parser. Use a PCC channel subspace for communicating with
+ the ACPI compliant platform to provide control of memory scrub
+ parameters to the user via the EDAC scrub.
+
endif
diff --git a/drivers/ras/Makefile b/drivers/ras/Makefile
index 11f95d59d397..a0e6e903d6b0 100644
--- a/drivers/ras/Makefile
+++ b/drivers/ras/Makefile
@@ -2,6 +2,7 @@
obj-$(CONFIG_RAS) += ras.o
obj-$(CONFIG_DEBUG_FS) += debugfs.o
obj-$(CONFIG_RAS_CEC) += cec.o
+obj-$(CONFIG_MEM_ACPI_RAS2) += acpi_ras2.o
obj-$(CONFIG_RAS_FMPM) += amd/fmpm.o
obj-y += amd/atl/
diff --git a/drivers/ras/acpi_ras2.c b/drivers/ras/acpi_ras2.c
new file mode 100644
index 000000000000..99cbe5f74b9d
--- /dev/null
+++ b/drivers/ras/acpi_ras2.c
@@ -0,0 +1,383 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * ACPI RAS2 memory driver
+ *
+ * Copyright (c) 2024-2025 HiSilicon Limited.
+ *
+ */
+
+#define pr_fmt(fmt) "MEMORY ACPI RAS2: " fmt
+
+#include <linux/bitfield.h>
+#include <linux/edac.h>
+#include <linux/platform_device.h>
+#include <acpi/ras2_acpi.h>
+
+#define RAS2_DEV_NUM_RAS_FEATURES 1
+
+#define RAS2_SUPPORT_HW_PARTOL_SCRUB BIT(0)
+#define RAS2_TYPE_PATROL_SCRUB 0x0000
+
+#define RAS2_GET_PATROL_PARAMETERS 0x01
+#define RAS2_START_PATROL_SCRUBBER 0x02
+#define RAS2_STOP_PATROL_SCRUBBER 0x03
+
+#define RAS2_PATROL_SCRUB_SCHRS_IN_MASK GENMASK(15, 8)
+#define RAS2_PATROL_SCRUB_EN_BACKGROUND BIT(0)
+#define RAS2_PATROL_SCRUB_SCHRS_OUT_MASK GENMASK(7, 0)
+#define RAS2_PATROL_SCRUB_MIN_SCHRS_OUT_MASK GENMASK(15, 8)
+#define RAS2_PATROL_SCRUB_MAX_SCHRS_OUT_MASK GENMASK(23, 16)
+#define RAS2_PATROL_SCRUB_FLAG_SCRUBBER_RUNNING BIT(0)
+
+#define RAS2_SCRUB_NAME_LEN 128
+#define RAS2_HOUR_IN_SECS 3600
+
+struct acpi_ras2_ps_shared_mem {
+ struct acpi_ras2_shared_memory common;
+ struct acpi_ras2_patrol_scrub_parameter params;
+};
+
+static int ras2_is_patrol_scrub_support(struct ras2_mem_ctx *ras2_ctx)
+{
+ struct acpi_ras2_shared_memory __iomem *common =
+ (void *)ras2_ctx->pcc_comm_addr;
+
+ guard(mutex)(&ras2_ctx->lock);
+ common->set_capabilities[0] = 0;
+
+ return common->features[0] & RAS2_SUPPORT_HW_PARTOL_SCRUB;
+}
+
+static int ras2_update_patrol_scrub_params_cache(struct ras2_mem_ctx *ras2_ctx)
+{
+ struct acpi_ras2_ps_shared_mem __iomem *ps_sm =
+ (void *)ras2_ctx->pcc_comm_addr;
+ int ret;
+
+ ps_sm->common.set_capabilities[0] = RAS2_SUPPORT_HW_PARTOL_SCRUB;
+ ps_sm->params.patrol_scrub_command = RAS2_GET_PATROL_PARAMETERS;
+
+ ret = ras2_send_pcc_cmd(ras2_ctx, RAS2_PCC_CMD_EXEC);
+ if (ret) {
+ dev_err(ras2_ctx->dev, "failed to read parameters\n");
+ return ret;
+ }
+
+ ras2_ctx->min_scrub_cycle = FIELD_GET(RAS2_PATROL_SCRUB_MIN_SCHRS_OUT_MASK,
+ ps_sm->params.scrub_params_out);
+ ras2_ctx->max_scrub_cycle = FIELD_GET(RAS2_PATROL_SCRUB_MAX_SCHRS_OUT_MASK,
+ ps_sm->params.scrub_params_out);
+ if (!ras2_ctx->bg) {
+ ras2_ctx->base = ps_sm->params.actual_address_range[0];
+ ras2_ctx->size = ps_sm->params.actual_address_range[1];
+ }
+ ras2_ctx->scrub_cycle_hrs = FIELD_GET(RAS2_PATROL_SCRUB_SCHRS_OUT_MASK,
+ ps_sm->params.scrub_params_out);
+
+ return 0;
+}
+
+/* Context - lock must be held */
+static int ras2_get_patrol_scrub_running(struct ras2_mem_ctx *ras2_ctx,
+ bool *running)
+{
+ struct acpi_ras2_ps_shared_mem __iomem *ps_sm =
+ (void *)ras2_ctx->pcc_comm_addr;
+ int ret;
+
+ ps_sm->common.set_capabilities[0] = RAS2_SUPPORT_HW_PARTOL_SCRUB;
+ ps_sm->params.patrol_scrub_command = RAS2_GET_PATROL_PARAMETERS;
+
+ ret = ras2_send_pcc_cmd(ras2_ctx, RAS2_PCC_CMD_EXEC);
+ if (ret) {
+ dev_err(ras2_ctx->dev, "failed to read parameters\n");
+ return ret;
+ }
+
+ *running = ps_sm->params.flags & RAS2_PATROL_SCRUB_FLAG_SCRUBBER_RUNNING;
+
+ return 0;
+}
+
+static int ras2_hw_scrub_read_min_scrub_cycle(struct device *dev, void *drv_data,
+ u32 *min)
+{
+ struct ras2_mem_ctx *ras2_ctx = drv_data;
+
+ *min = ras2_ctx->min_scrub_cycle * RAS2_HOUR_IN_SECS;
+
+ return 0;
+}
+
+static int ras2_hw_scrub_read_max_scrub_cycle(struct device *dev, void *drv_data,
+ u32 *max)
+{
+ struct ras2_mem_ctx *ras2_ctx = drv_data;
+
+ *max = ras2_ctx->max_scrub_cycle * RAS2_HOUR_IN_SECS;
+
+ return 0;
+}
+
+static int ras2_hw_scrub_cycle_read(struct device *dev, void *drv_data,
+ u32 *scrub_cycle_secs)
+{
+ struct ras2_mem_ctx *ras2_ctx = drv_data;
+
+ *scrub_cycle_secs = ras2_ctx->scrub_cycle_hrs * RAS2_HOUR_IN_SECS;
+
+ return 0;
+}
+
+static int ras2_hw_scrub_cycle_write(struct device *dev, void *drv_data,
+ u32 scrub_cycle_secs)
+{
+ u8 scrub_cycle_hrs = scrub_cycle_secs / RAS2_HOUR_IN_SECS;
+ struct ras2_mem_ctx *ras2_ctx = drv_data;
+ bool running;
+ int ret;
+
+ guard(mutex)(&ras2_ctx->lock);
+ ret = ras2_get_patrol_scrub_running(ras2_ctx, &running);
+ if (ret)
+ return ret;
+
+ if (running)
+ return -EBUSY;
+
+ if (scrub_cycle_hrs < ras2_ctx->min_scrub_cycle ||
+ scrub_cycle_hrs > ras2_ctx->max_scrub_cycle)
+ return -EINVAL;
+
+ ras2_ctx->scrub_cycle_hrs = scrub_cycle_hrs;
+
+ return 0;
+}
+
+static int ras2_hw_scrub_read_addr(struct device *dev, void *drv_data, u64 *base)
+{
+ struct ras2_mem_ctx *ras2_ctx = drv_data;
+ int ret;
+
+ /*
+ * When BG scrubbing is enabled the actual address range is not valid.
+ * Return -EBUSY now unless find out a method to retrieve actual full PA range.
+ */
+ if (ras2_ctx->bg)
+ return -EBUSY;
+
+ /*
+ * When demand scrubbing is finished firmware must reset actual
+ * address range to 0. Otherwise userspace assumes demand scrubbing
+ * is in progress.
+ */
+ ret = ras2_update_patrol_scrub_params_cache(ras2_ctx);
+ if (ret)
+ return ret;
+ *base = ras2_ctx->base;
+
+ return 0;
+}
+
+static int ras2_hw_scrub_read_size(struct device *dev, void *drv_data, u64 *size)
+{
+ struct ras2_mem_ctx *ras2_ctx = drv_data;
+ int ret;
+
+ if (ras2_ctx->bg)
+ return -EBUSY;
+
+ ret = ras2_update_patrol_scrub_params_cache(ras2_ctx);
+ if (ret)
+ return ret;
+ *size = ras2_ctx->size;
+
+ return 0;
+}
+
+static int ras2_hw_scrub_write_addr(struct device *dev, void *drv_data, u64 base)
+{
+ struct ras2_mem_ctx *ras2_ctx = drv_data;
+ struct acpi_ras2_ps_shared_mem __iomem *ps_sm =
+ (void *)ras2_ctx->pcc_comm_addr;
+ bool running;
+ int ret;
+
+ guard(mutex)(&ras2_ctx->lock);
+ ps_sm->common.set_capabilities[0] = RAS2_SUPPORT_HW_PARTOL_SCRUB;
+ if (ras2_ctx->bg)
+ return -EBUSY;
+
+ if (!base || !ras2_ctx->size) {
+ dev_warn(ras2_ctx->dev,
+ "%s: Invalid address range, base=0x%llx "
+ "size=0x%llx\n", __func__,
+ base, ras2_ctx->size);
+ return -ERANGE;
+ }
+
+ ret = ras2_get_patrol_scrub_running(ras2_ctx, &running);
+ if (ret)
+ return ret;
+
+ if (running)
+ return -EBUSY;
+
+ ps_sm->params.scrub_params_in &= ~RAS2_PATROL_SCRUB_SCHRS_IN_MASK;
+ ps_sm->params.scrub_params_in |= FIELD_PREP(RAS2_PATROL_SCRUB_SCHRS_IN_MASK,
+ ras2_ctx->scrub_cycle_hrs);
+ ps_sm->params.requested_address_range[0] = base;
+ ps_sm->params.requested_address_range[1] = ras2_ctx->size;
+ ps_sm->params.scrub_params_in &= ~RAS2_PATROL_SCRUB_EN_BACKGROUND;
+ ps_sm->params.patrol_scrub_command = RAS2_START_PATROL_SCRUBBER;
+
+ ret = ras2_send_pcc_cmd(ras2_ctx, RAS2_PCC_CMD_EXEC);
+ if (ret) {
+ dev_err(ras2_ctx->dev, "Failed to start demand scrubbing\n");
+ return ret;
+ }
+
+ return ras2_update_patrol_scrub_params_cache(ras2_ctx);
+}
+
+static int ras2_hw_scrub_write_size(struct device *dev, void *drv_data, u64 size)
+{
+ struct ras2_mem_ctx *ras2_ctx = drv_data;
+ bool running;
+ int ret;
+
+ guard(mutex)(&ras2_ctx->lock);
+ ret = ras2_get_patrol_scrub_running(ras2_ctx, &running);
+ if (ret)
+ return ret;
+
+ if (running)
+ return -EBUSY;
+
+ if (!size) {
+ dev_warn(dev, "%s: Invalid address range size=0x%llx\n",
+ __func__, size);
+ return -EINVAL;
+ }
+
+ ras2_ctx->size = size;
+
+ return 0;
+}
+
+static int ras2_hw_scrub_set_enabled_bg(struct device *dev, void *drv_data, bool enable)
+{
+ struct ras2_mem_ctx *ras2_ctx = drv_data;
+ struct acpi_ras2_ps_shared_mem __iomem *ps_sm =
+ (void *)ras2_ctx->pcc_comm_addr;
+ bool running;
+ int ret;
+
+ guard(mutex)(&ras2_ctx->lock);
+ ps_sm->common.set_capabilities[0] = RAS2_SUPPORT_HW_PARTOL_SCRUB;
+ ret = ras2_get_patrol_scrub_running(ras2_ctx, &running);
+ if (ret)
+ return ret;
+ if (enable) {
+ if (ras2_ctx->bg || running)
+ return -EBUSY;
+ ps_sm->params.requested_address_range[0] = 0;
+ ps_sm->params.requested_address_range[1] = 0;
+ ps_sm->params.scrub_params_in &= ~RAS2_PATROL_SCRUB_SCHRS_IN_MASK;
+ ps_sm->params.scrub_params_in |= FIELD_PREP(RAS2_PATROL_SCRUB_SCHRS_IN_MASK,
+ ras2_ctx->scrub_cycle_hrs);
+ ps_sm->params.patrol_scrub_command = RAS2_START_PATROL_SCRUBBER;
+ } else {
+ if (!ras2_ctx->bg)
+ return -EPERM;
+ ps_sm->params.patrol_scrub_command = RAS2_STOP_PATROL_SCRUBBER;
+ }
+ ps_sm->params.scrub_params_in &= ~RAS2_PATROL_SCRUB_EN_BACKGROUND;
+ ps_sm->params.scrub_params_in |= FIELD_PREP(RAS2_PATROL_SCRUB_EN_BACKGROUND,
+ enable);
+ ret = ras2_send_pcc_cmd(ras2_ctx, RAS2_PCC_CMD_EXEC);
+ if (ret) {
+ dev_err(ras2_ctx->dev, "Failed to %s background scrubbing\n",
+ str_enable_disable(enable));
+ return ret;
+ }
+ if (enable) {
+ ras2_ctx->bg = true;
+ /* Update the cache to account for rounding of supplied parameters and similar */
+ ret = ras2_update_patrol_scrub_params_cache(ras2_ctx);
+ } else {
+ ret = ras2_update_patrol_scrub_params_cache(ras2_ctx);
+ ras2_ctx->bg = false;
+ }
+
+ return ret;
+}
+
+static int ras2_hw_scrub_get_enabled_bg(struct device *dev, void *drv_data, bool *enabled)
+{
+ struct ras2_mem_ctx *ras2_ctx = drv_data;
+
+ *enabled = ras2_ctx->bg;
+
+ return 0;
+}
+
+static const struct edac_scrub_ops ras2_scrub_ops = {
+ .read_addr = ras2_hw_scrub_read_addr,
+ .read_size = ras2_hw_scrub_read_size,
+ .write_addr = ras2_hw_scrub_write_addr,
+ .write_size = ras2_hw_scrub_write_size,
+ .get_enabled_bg = ras2_hw_scrub_get_enabled_bg,
+ .set_enabled_bg = ras2_hw_scrub_set_enabled_bg,
+ .get_min_cycle = ras2_hw_scrub_read_min_scrub_cycle,
+ .get_max_cycle = ras2_hw_scrub_read_max_scrub_cycle,
+ .get_cycle_duration = ras2_hw_scrub_cycle_read,
+ .set_cycle_duration = ras2_hw_scrub_cycle_write,
+};
+
+static int ras2_probe(struct auxiliary_device *auxdev,
+ const struct auxiliary_device_id *id)
+{
+ struct ras2_mem_ctx *ras2_ctx = container_of(auxdev, struct ras2_mem_ctx, adev);
+ struct edac_dev_feature ras_features[RAS2_DEV_NUM_RAS_FEATURES];
+ char scrub_name[RAS2_SCRUB_NAME_LEN];
+ int num_ras_features = 0;
+ int ret;
+
+ if (!ras2_is_patrol_scrub_support(ras2_ctx))
+ return -EOPNOTSUPP;
+
+ ret = ras2_update_patrol_scrub_params_cache(ras2_ctx);
+ if (ret)
+ return ret;
+
+ snprintf(scrub_name, sizeof(scrub_name), "acpi_ras_mem%d",
+ ras2_ctx->id);
+
+ ras_features[num_ras_features].ft_type = RAS_FEAT_SCRUB;
+ ras_features[num_ras_features].instance = ras2_ctx->instance;
+ ras_features[num_ras_features].scrub_ops = &ras2_scrub_ops;
+ ras_features[num_ras_features].ctx = ras2_ctx;
+ num_ras_features++;
+
+ return edac_dev_register(&auxdev->dev, scrub_name, NULL,
+ num_ras_features, ras_features);
+}
+
+static const struct auxiliary_device_id ras2_mem_dev_id_table[] = {
+ { .name = RAS2_AUX_DEV_NAME "." RAS2_MEM_DEV_ID_NAME, },
+ { }
+};
+
+MODULE_DEVICE_TABLE(auxiliary, ras2_mem_dev_id_table);
+
+static struct auxiliary_driver ras2_mem_driver = {
+ .name = RAS2_MEM_DEV_ID_NAME,
+ .probe = ras2_probe,
+ .id_table = ras2_mem_dev_id_table,
+};
+module_auxiliary_driver(ras2_mem_driver);
+
+MODULE_IMPORT_NS("ACPI_RAS2");
+MODULE_DESCRIPTION("ACPI RAS2 memory driver");
+MODULE_LICENSE("GPL");
--
2.43.0
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH linux-next 0/2] ACPI: Add support for ACPI RAS2 feature table
2025-02-28 12:27 [PATCH linux-next 0/2] ACPI: Add support for ACPI RAS2 feature table shiju.jose
2025-02-28 12:27 ` [PATCH linux-next 1/2] ACPI:RAS2: Add ACPI RAS2 driver shiju.jose
2025-02-28 12:27 ` [PATCH linux-next 2/2] ras: mem: Add memory " shiju.jose
@ 2025-03-03 9:35 ` Jonathan Cameron
2025-03-03 10:21 ` Shiju Jose
2025-03-03 10:35 ` Borislav Petkov
2 siblings, 2 replies; 11+ messages in thread
From: Jonathan Cameron @ 2025-03-03 9:35 UTC (permalink / raw)
To: shiju.jose
Cc: linux-edac, linux-acpi, rafael, bp, tony.luck, lenb, mchehab,
linux-mm, linux-kernel, linux-cxl, j.williams, dave, dave.jiang,
alison.schofield, vishal.l.verma, ira.weiny, david,
Vilas.Sridharan, leo.duran, Yazen.Ghannam, rientjes, jiaqiyan,
Jon.Grimm, dave.hansen, naoya.horiguchi, james.morse, jthoughton,
somasundaram.a, erdemaktas, pgonda, duenwen, gthelen, wschwartz,
dferguson, wbs, nifan.cxl, tanxiaofei, prime.zeng, roberto.sassu,
kangkang.shen, wanghuiqiang, linuxarm
On Fri, 28 Feb 2025 12:27:48 +0000
<shiju.jose@huawei.com> wrote:
> From: Shiju Jose <shiju.jose@huawei.com>
>
> Add support for ACPI RAS2 feature table (RAS2) defined in the ACPI 6.5
> specification, section 5.2.21 and RAS2 HW based memory scrubbing feature.
>
> ACPI RAS2 patches were part of the EDAC series [1].
Whilst linux-next now contains the EDAC patches, we shouldn't base
a feature submission on it. This should be the same as you
did for the CXL tree with a statement that it depends on
https://web.git.kernel.org/pub/scm/linux/kernel/git/ras/ras.git/log/?h=edac-cxl
which is the immutable tag / branch Borislav provided.
I doubt there is anything else hitting this code so
shouldn't be any need to rebase (I could be wrong though!)
Assuming everyone is happy with this series, who is going to pick
it up?
Borislav via ras.git, or Rafael via acpi.git? I don't really
have any preference other than making sure it doesn't fall down
the cracks!
Jonathan
>
> 1. https://lore.kernel.org/linux-cxl/20250212143654.1893-1-shiju.jose@huawei.com/
>
> Shiju Jose (2):
> ACPI:RAS2: Add ACPI RAS2 driver
> ras: mem: Add memory ACPI RAS2 driver
>
> Documentation/edac/scrub.rst | 73 ++++++
> drivers/acpi/Kconfig | 11 +
> drivers/acpi/Makefile | 1 +
> drivers/acpi/ras2.c | 417 +++++++++++++++++++++++++++++++++++
> drivers/ras/Kconfig | 11 +
> drivers/ras/Makefile | 1 +
> drivers/ras/acpi_ras2.c | 383 ++++++++++++++++++++++++++++++++
> include/acpi/ras2_acpi.h | 47 ++++
> 8 files changed, 944 insertions(+)
> create mode 100755 drivers/acpi/ras2.c
> create mode 100644 drivers/ras/acpi_ras2.c
> create mode 100644 include/acpi/ras2_acpi.h
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH linux-next 0/2] ACPI: Add support for ACPI RAS2 feature table
2025-03-03 9:35 ` [PATCH linux-next 0/2] ACPI: Add support for ACPI RAS2 feature table Jonathan Cameron
@ 2025-03-03 10:21 ` Shiju Jose
2025-03-03 10:35 ` Borislav Petkov
1 sibling, 0 replies; 11+ messages in thread
From: Shiju Jose @ 2025-03-03 10:21 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-edac, linux-acpi, rafael, bp, tony.luck, lenb, mchehab,
linux-mm, linux-kernel, linux-cxl, j.williams, dave, dave.jiang,
alison.schofield, vishal.l.verma, ira.weiny, david,
Vilas.Sridharan, leo.duran, Yazen.Ghannam, rientjes, jiaqiyan,
Jon.Grimm, dave.hansen, naoya.horiguchi, james.morse, jthoughton,
somasundaram.a, erdemaktas, pgonda, duenwen, gthelen, wschwartz,
dferguson, wbs, nifan.cxl, tanxiaofei, Zengtao (B),
Roberto Sassu, kangkang.shen, wanghuiqiang, Linuxarm
>-----Original Message-----
>From: Jonathan Cameron <jonathan.cameron@huawei.com>
>Sent: 03 March 2025 09:36
>To: Shiju Jose <shiju.jose@huawei.com>
>Cc: linux-edac@vger.kernel.org; linux-acpi@vger.kernel.org; rafael@kernel.org;
>bp@alien8.de; tony.luck@intel.com; lenb@kernel.org; mchehab@kernel.org;
>linux-mm@kvack.org; linux-kernel@vger.kernel.org; linux-cxl@vger.kernel.org;
>j.williams@intel.com; dave@stgolabs.net; dave.jiang@intel.com;
>alison.schofield@intel.com; vishal.l.verma@intel.com; ira.weiny@intel.com;
>david@redhat.com; Vilas.Sridharan@amd.com; leo.duran@amd.com;
>Yazen.Ghannam@amd.com; rientjes@google.com; jiaqiyan@google.com;
>Jon.Grimm@amd.com; dave.hansen@linux.intel.com;
>naoya.horiguchi@nec.com; james.morse@arm.com; jthoughton@google.com;
>somasundaram.a@hpe.com; erdemaktas@google.com; pgonda@google.com;
>duenwen@google.com; gthelen@google.com;
>wschwartz@amperecomputing.com; dferguson@amperecomputing.com;
>wbs@os.amperecomputing.com; nifan.cxl@gmail.com; tanxiaofei
><tanxiaofei@huawei.com>; Zengtao (B) <prime.zeng@hisilicon.com>; Roberto
>Sassu <roberto.sassu@huawei.com>; kangkang.shen@futurewei.com;
>wanghuiqiang <wanghuiqiang@huawei.com>; Linuxarm
><linuxarm@huawei.com>
>Subject: Re: [PATCH linux-next 0/2] ACPI: Add support for ACPI RAS2 feature
>table
>
>On Fri, 28 Feb 2025 12:27:48 +0000
><shiju.jose@huawei.com> wrote:
>
>> From: Shiju Jose <shiju.jose@huawei.com>
>>
>> Add support for ACPI RAS2 feature table (RAS2) defined in the ACPI 6.5
>> specification, section 5.2.21 and RAS2 HW based memory scrubbing feature.
>>
>> ACPI RAS2 patches were part of the EDAC series [1].
>
>Whilst linux-next now contains the EDAC patches, we shouldn't base a feature
>submission on it. This should be the same as you did for the CXL tree with a
>statement that it depends on
>
>https://web.git.kernel.org/pub/scm/linux/kernel/git/ras/ras.git/log/?h=edac-cxl
>
>which is the immutable tag / branch Borislav provided.
Hi Jonathan,
These RAS2 patches are applied cleanly, built and tested fine in the
immutable ras.git: 'edac-cxl' branch Borislav provided.
(https://web.git.kernel.org/pub/scm/linux/kernel/git/ras/ras.git/log/?h=edac-cxl).
Thanks,
Shiju
>
>I doubt there is anything else hitting this code so shouldn't be any need to rebase
>(I could be wrong though!)
>
>Assuming everyone is happy with this series, who is going to pick it up?
>
>Borislav via ras.git, or Rafael via acpi.git? I don't really have any preference
>other than making sure it doesn't fall down the cracks!
>
>Jonathan
>
>>
>> 1.
>> https://lore.kernel.org/linux-cxl/20250212143654.1893-1-shiju.jose@hua
>> wei.com/
>>
>> Shiju Jose (2):
>> ACPI:RAS2: Add ACPI RAS2 driver
>> ras: mem: Add memory ACPI RAS2 driver
>>
>> Documentation/edac/scrub.rst | 73 ++++++
>> drivers/acpi/Kconfig | 11 +
>> drivers/acpi/Makefile | 1 +
>> drivers/acpi/ras2.c | 417 +++++++++++++++++++++++++++++++++++
>> drivers/ras/Kconfig | 11 +
>> drivers/ras/Makefile | 1 +
>> drivers/ras/acpi_ras2.c | 383 ++++++++++++++++++++++++++++++++
>> include/acpi/ras2_acpi.h | 47 ++++
>> 8 files changed, 944 insertions(+)
>> create mode 100755 drivers/acpi/ras2.c create mode 100644
>> drivers/ras/acpi_ras2.c create mode 100644 include/acpi/ras2_acpi.h
>>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH linux-next 0/2] ACPI: Add support for ACPI RAS2 feature table
2025-03-03 9:35 ` [PATCH linux-next 0/2] ACPI: Add support for ACPI RAS2 feature table Jonathan Cameron
2025-03-03 10:21 ` Shiju Jose
@ 2025-03-03 10:35 ` Borislav Petkov
2025-03-04 18:19 ` Shiju Jose
1 sibling, 1 reply; 11+ messages in thread
From: Borislav Petkov @ 2025-03-03 10:35 UTC (permalink / raw)
To: Jonathan Cameron
Cc: shiju.jose, linux-edac, linux-acpi, rafael, tony.luck, lenb,
mchehab, linux-mm, linux-kernel, linux-cxl, j.williams, dave,
dave.jiang, alison.schofield, vishal.l.verma, ira.weiny, david,
Vilas.Sridharan, leo.duran, Yazen.Ghannam, rientjes, jiaqiyan,
Jon.Grimm, dave.hansen, naoya.horiguchi, james.morse, jthoughton,
somasundaram.a, erdemaktas, pgonda, duenwen, gthelen, wschwartz,
dferguson, wbs, nifan.cxl, tanxiaofei, prime.zeng, roberto.sassu,
kangkang.shen, wanghuiqiang, linuxarm
On Mon, Mar 03, 2025 at 05:35:38PM +0800, Jonathan Cameron wrote:
> Borislav via ras.git, or Rafael via acpi.git? I don't really
> have any preference other than making sure it doesn't fall down
> the cracks!
It's probably easier if I take it.
However, just from a cursory look, it would need some scrubbing. There's stuff
like:
+ ps_sm->params.requested_address_range[0] = 0;
+ ps_sm->params.requested_address_range[1] = 0;
+ ps_sm->params.scrub_params_in &= ~RAS2_PATROL_SCRUB_SCHRS_IN_MASK;
+ ps_sm->params.scrub_params_in |= FIELD_PREP(RAS2_PATROL_SCRUB_SCHRS_IN_MASK,
+ ras2_ctx->scrub_cycle_hrs);
+ ps_sm->params.patrol_scrub_command = RAS2_START_PATROL_SCRUBBER;
which definitely needs shortening. There's no need for a wholly written out
"requested_address_range". I know variables should have meaningfull names but
writing fiction shouldn't be either.
+static int ras2_acpi_parse_table(struct acpi_table_header *pAcpiTable)
Yuck, CamelCase?!
And I'm pretty sure if I start looking more, I'll find more funky stuff.
HTH.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 11+ messages in thread* RE: [PATCH linux-next 0/2] ACPI: Add support for ACPI RAS2 feature table
2025-03-03 10:35 ` Borislav Petkov
@ 2025-03-04 18:19 ` Shiju Jose
2025-03-04 20:30 ` Borislav Petkov
0 siblings, 1 reply; 11+ messages in thread
From: Shiju Jose @ 2025-03-04 18:19 UTC (permalink / raw)
To: Borislav Petkov, Jonathan Cameron
Cc: linux-edac, linux-acpi, rafael, tony.luck, lenb, mchehab,
linux-mm, linux-kernel, linux-cxl, dave, dave.jiang,
alison.schofield, vishal.l.verma, ira.weiny, david,
Vilas.Sridharan, leo.duran, Yazen.Ghannam, rientjes, jiaqiyan,
Jon.Grimm, dave.hansen, naoya.horiguchi, james.morse, jthoughton,
somasundaram.a, erdemaktas, pgonda, duenwen, gthelen, wschwartz,
dferguson, wbs, nifan.cxl, tanxiaofei, Zengtao (B),
Roberto Sassu, kangkang.shen, wanghuiqiang, Linuxarm
>-----Original Message-----
>From: Borislav Petkov <bp@alien8.de>
>Sent: 03 March 2025 10:35
>To: Jonathan Cameron <jonathan.cameron@huawei.com>
>Cc: Shiju Jose <shiju.jose@huawei.com>; linux-edac@vger.kernel.org; linux-
>acpi@vger.kernel.org; rafael@kernel.org; tony.luck@intel.com;
>lenb@kernel.org; mchehab@kernel.org; linux-mm@kvack.org; linux-
>kernel@vger.kernel.org; linux-cxl@vger.kernel.org; j.williams@intel.com;
>dave@stgolabs.net; dave.jiang@intel.com; alison.schofield@intel.com;
>vishal.l.verma@intel.com; ira.weiny@intel.com; david@redhat.com;
>Vilas.Sridharan@amd.com; leo.duran@amd.com; Yazen.Ghannam@amd.com;
>rientjes@google.com; jiaqiyan@google.com; Jon.Grimm@amd.com;
>dave.hansen@linux.intel.com; naoya.horiguchi@nec.com;
>james.morse@arm.com; jthoughton@google.com; somasundaram.a@hpe.com;
>erdemaktas@google.com; pgonda@google.com; duenwen@google.com;
>gthelen@google.com; wschwartz@amperecomputing.com;
>dferguson@amperecomputing.com; wbs@os.amperecomputing.com;
>nifan.cxl@gmail.com; tanxiaofei <tanxiaofei@huawei.com>; Zengtao (B)
><prime.zeng@hisilicon.com>; Roberto Sassu <roberto.sassu@huawei.com>;
>kangkang.shen@futurewei.com; wanghuiqiang <wanghuiqiang@huawei.com>;
>Linuxarm <linuxarm@huawei.com>
>Subject: Re: [PATCH linux-next 0/2] ACPI: Add support for ACPI RAS2 feature
>table
>
[...]
>
>However, just from a cursory look, it would need some scrubbing. There's stuff
>like:
>
>+ ps_sm->params.requested_address_range[0] = 0;
>+ ps_sm->params.requested_address_range[1] = 0;
>+ ps_sm->params.scrub_params_in &=
>~RAS2_PATROL_SCRUB_SCHRS_IN_MASK;
>+ ps_sm->params.scrub_params_in |=
>FIELD_PREP(RAS2_PATROL_SCRUB_SCHRS_IN_MASK,
>+ ras2_ctx->scrub_cycle_hrs);
>+ ps_sm->params.patrol_scrub_command =
>+ RAS2_START_PATROL_SCRUBBER;
>
>
>which definitely needs shortening. There's no need for a wholly written out
>"requested_address_range". I know variables should have meaningfull names
>but writing fiction shouldn't be either.
Hi Borislav,
Some of these variables, for e.g. requested_address_range are not defined
in this patch, but in the 'include/acpi/actbl2.h'.
My understanding is that those changes required to upstream first via
https://github.com/acpica/acpica ?
>
>+static int ras2_acpi_parse_table(struct acpi_table_header *pAcpiTable)
>
>Yuck, CamelCase?!
Fixed.
>
>And I'm pretty sure if I start looking more, I'll find more funky stuff.
Will check and fix.
>
>HTH.
>
>--
>Regards/Gruss,
> Boris.
>
>https://people.kernel.org/tglx/notes-about-netiquette
Thanks,
Shiju
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH linux-next 0/2] ACPI: Add support for ACPI RAS2 feature table
2025-03-04 18:19 ` Shiju Jose
@ 2025-03-04 20:30 ` Borislav Petkov
2025-03-04 20:46 ` Luck, Tony
0 siblings, 1 reply; 11+ messages in thread
From: Borislav Petkov @ 2025-03-04 20:30 UTC (permalink / raw)
To: Shiju Jose
Cc: Jonathan Cameron, linux-edac, linux-acpi, rafael, tony.luck,
lenb, mchehab, linux-mm, linux-kernel, linux-cxl, dave,
dave.jiang, alison.schofield, vishal.l.verma, ira.weiny, david,
Vilas.Sridharan, leo.duran, Yazen.Ghannam, rientjes, jiaqiyan,
Jon.Grimm, dave.hansen, naoya.horiguchi, james.morse, jthoughton,
somasundaram.a, erdemaktas, pgonda, duenwen, gthelen, wschwartz,
dferguson, wbs, nifan.cxl, tanxiaofei, Zengtao (B),
Roberto Sassu, kangkang.shen, wanghuiqiang, Linuxarm
On Tue, Mar 04, 2025 at 06:19:58PM +0000, Shiju Jose wrote:
> Some of these variables, for e.g. requested_address_range are not defined
> in this patch, but in the 'include/acpi/actbl2.h'.
> My understanding is that those changes required to upstream first via
> https://github.com/acpica/acpica ?
Are you sure?
...
* Additional ACPI Tables (2)
*
* These tables are not consumed directly by the ACPICA subsystem, but are
* included here to support device drivers and the AML disassembler.
...
In any case, if this goes through me, I will have to review it first as it
looks funky.
Your call guys.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 11+ messages in thread* RE: [PATCH linux-next 0/2] ACPI: Add support for ACPI RAS2 feature table
2025-03-04 20:30 ` Borislav Petkov
@ 2025-03-04 20:46 ` Luck, Tony
0 siblings, 0 replies; 11+ messages in thread
From: Luck, Tony @ 2025-03-04 20:46 UTC (permalink / raw)
To: Borislav Petkov, Shiju Jose
Cc: Jonathan Cameron, linux-edac, linux-acpi, rafael, lenb, mchehab,
linux-mm, linux-kernel, linux-cxl, dave, Jiang, Dave, Schofield,
Alison, Verma, Vishal L, Weiny, Ira, david, Vilas.Sridharan,
leo.duran, Yazen.Ghannam, rientjes, jiaqiyan, Jon.Grimm,
dave.hansen, naoya.horiguchi, james.morse, jthoughton,
Somasundaram A, Aktas, Erdem, pgonda, duenwen, gthelen,
wschwartz, dferguson, wbs, nifan.cxl, tanxiaofei, Zengtao (B),
Roberto Sassu, kangkang.shen, wanghuiqiang, Linuxarm
> > Some of these variables, for e.g. requested_address_range are not defined
> > in this patch, but in the 'include/acpi/actbl2.h'.
> > My understanding is that those changes required to upstream first via
> > https://github.com/acpica/acpica ?
>
> Are you sure?
>
> ...
> * Additional ACPI Tables (2)
> *
> * These tables are not consumed directly by the ACPICA subsystem, but are
> * included here to support device drivers and the AML disassembler.
> ...
>
> In any case, if this goes through me, I will have to review it first as it
> looks funky.
That requested_address_range field has been in the
acpi_rasf_patrol_scrub_parameter structure since 2018
Got moved/copied into the acpi_ras2_patrol_scrub_parameter structure
in 2023.
$ git blame include/acpi/actbl2.h | grep requested_address_range
e62f8227851da (Erik Kaneda 2018-02-15 13:09:26 -0800 2722) u64 requested_address_range[2];
2e94dc1189804 (Shiju Jose 2023-09-27 17:41:52 +0100 2829) u64 requested_address_range[2];
-Tony
^ permalink raw reply [flat|nested] 11+ messages in thread