>-----Original Message-----
>From: Yazen Ghannam <yazen.ghannam@xxxxxxx>
>Sent: 03 March 2025 21:07
>To: Shiju Jose <shiju.jose@xxxxxxxxxx>
>Cc: linux-edac@xxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx; rafael@xxxxxxxxxx;
>bp@xxxxxxxxx; tony.luck@xxxxxxxxx; lenb@xxxxxxxxxx; mchehab@xxxxxxxxxx;
>linux-mm@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-cxl@xxxxxxxxxxxxxxx;
>j.williams@xxxxxxxxx; dave@xxxxxxxxxxxx; Jonathan Cameron
><jonathan.cameron@xxxxxxxxxx>; dave.jiang@xxxxxxxxx;
>alison.schofield@xxxxxxxxx; vishal.l.verma@xxxxxxxxx; ira.weiny@xxxxxxxxx;
>david@xxxxxxxxxx; Vilas.Sridharan@xxxxxxx; leo.duran@xxxxxxx;
>rientjes@xxxxxxxxxx; jiaqiyan@xxxxxxxxxx; Jon.Grimm@xxxxxxx;
>dave.hansen@xxxxxxxxxxxxxxx; naoya.horiguchi@xxxxxxx;
>james.morse@xxxxxxx; jthoughton@xxxxxxxxxx; somasundaram.a@xxxxxxx;
>erdemaktas@xxxxxxxxxx; pgonda@xxxxxxxxxx; duenwen@xxxxxxxxxx;
>gthelen@xxxxxxxxxx; wschwartz@xxxxxxxxxxxxxxxxxxx;
>dferguson@xxxxxxxxxxxxxxxxxxx; wbs@xxxxxxxxxxxxxxxxxxxxxx;
>nifan.cxl@xxxxxxxxx; tanxiaofei <tanxiaofei@xxxxxxxxxx>; Zengtao (B)
><prime.zeng@xxxxxxxxxxxxx>; Roberto Sassu <roberto.sassu@xxxxxxxxxx>;
>kangkang.shen@xxxxxxxxxxxxx; wanghuiqiang <wanghuiqiang@xxxxxxxxxx>;
>Linuxarm <linuxarm@xxxxxxxxxx>
>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@xxxxxxxxxx wrote:
>> From: Shiju Jose <shiju.jose@xxxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> Co-developed-by: A Somasundaram <somasundaram.a@xxxxxxx>
>> Signed-off-by: A Somasundaram <somasundaram.a@xxxxxxx>
>> Co-developed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>> Tested-by: Daniel Ferguson <danielf@xxxxxxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Shiju Jose <shiju.jose@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
>
>> + *
>> + * 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
