RE: [PATCH v16 1/2] ACPI:RAS2: Add driver for the ACPI RAS2 feature table

[Shiju Jose <shiju.jose@huawei.com>]

>-----Original Message-----
>From: Borislav Petkov <bp@alien8.de>
>Sent: 26 January 2026 17:16
>To: Shiju Jose <shiju.jose@huawei.com>
>Cc: rafael@kernel.org; akpm@linux-foundation.org; rppt@kernel.org;
>dferguson@amperecomputing.com; linux-edac@vger.kernel.org; linux-
>acpi@vger.kernel.org; linux-mm@kvack.org; linux-doc@vger.kernel.org;
>tony.luck@intel.com; lenb@kernel.org; leo.duran@amd.com;
>Yazen.Ghannam@amd.com; mchehab@kernel.org; Jonathan Cameron
><jonathan.cameron@huawei.com>; Linuxarm <linuxarm@huawei.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;
>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>
>Subject: Re: [PATCH v16 1/2] ACPI:RAS2: Add driver for the ACPI RAS2 feature
>table
>
>On Fri, Jan 23, 2026 at 05:55:07PM +0000, shiju.jose@huawei.com wrote:
>> +static int parse_ras2_table(struct acpi_table_ras2 *ras2_tab) {
>> +	struct acpi_ras2_pcc_desc *pcc_desc_list;
>> +	struct ras2_mem_ctx **pctx_list;
>> +	struct ras2_mem_ctx *ras2_ctx;
>> +	u16 i;
>> +
>> +	if (ras2_tab->header.length < sizeof(*ras2_tab)) {
>> +		pr_warn(FW_WARN "ACPI RAS2 table present but broken (too
>short, size=%u)\n",
>> +			ras2_tab->header.length);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!ras2_tab->num_pcc_descs || ras2_tab->num_pcc_descs >
>RAS2_MAX_NUM_PCC_DESCS) {
>> +		pr_warn(FW_WARN "No/Invalid number of PCC descs(%d) in
>ACPI RAS2 table\n",
>> +			ras2_tab->num_pcc_descs);
>> +		return -EINVAL;
>> +	}
>> +
>> +	pctx_list = kcalloc(ras2_tab->num_pcc_descs, sizeof(*pctx_list),
>GFP_KERNEL);
>> +	if (!pctx_list)
>> +		return -ENOMEM;
>> +
>> +	pcc_desc_list = (struct acpi_ras2_pcc_desc *)(ras2_tab + 1);
>> +	for (i = 0; i < ras2_tab->num_pcc_descs; i++, pcc_desc_list++) {
>> +		if (pcc_desc_list->feature_type != RAS2_FEAT_TYPE_MEMORY)
>> +			continue;
>> +
>> +		ras2_ctx = add_aux_device(RAS2_MEM_DEV_ID_NAME,
>pcc_desc_list->channel_id,
>> +					  pcc_desc_list->instance);
>> +		if (IS_ERR(ras2_ctx)) {
>> +			pr_warn("Failed to add RAS2 auxiliary device rc=%ld\n",
>PTR_ERR(ras2_ctx));
>> +			for (; i > 0; i--) {
>> +				if (pctx_list[i - 1])
>> +					auxiliary_device_uninit(&pctx_list[i - 1]-
>>adev);
>
>This is wrong - there should be a function called remove_aux_device() which
>unwinds everything add_aux_device() does for all those devices.

Hi Borislav,

Thanks for comments and changes.

I added remove_aux_device()  as below, which would call ras2_release() and free
add_aux_device() does for that auxiliary device. Hope it is acceptable?

+static void remove_aux_device(struct ras2_mem_ctx *ras2_ctx) {
+    if (!ras2_ctx)
+        return;
+
+    auxiliary_device_delete(&ras2_ctx->adev);
+    auxiliary_device_uninit(&ras2_ctx->adev);
+}
+
>
>In addition, I did a bunch of cleanups ontop, see below. I can't test them so pls
>have a look and run them on your hw and if all good, merge them with your
>patch.

I tested your changes and merged.
I got a checkpatch warning for below change.

Thanks,
Shiju

>
>Thx.
>
>---
>
>diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index
>7f846c22fc30..0010b38e8f81 100644
>--- a/drivers/acpi/Kconfig
>+++ b/drivers/acpi/Kconfig
>@@ -299,9 +299,10 @@ config ACPI_RAS2
> 	depends on MAILBOX
> 	depends on PCC
>@@ -187,22 +197,24 @@ int ras2_send_pcc_cmd(struct ras2_mem_ctx
>*ras2_ctx, u16 cmd)
>
> 	/* Ring doorbell */
> 	rc = mbox_send_message(pcc_channel, &cmd);
>+
> 	/*
>-	 * mbox_send_message() return non-negative integer for successful
>submission
>-	 * and negative value on failure.
>+	 * mbox_send_message() returns a non-negative integer for successful
>submission
>+	 * and a negative value on failure.
> 	 */
>-	rc = rc < 0 ? rc : 0;
> 	if (rc < 0) {
> 		dev_warn(ras2_ctx->dev,
> 			 "Error sending PCC mbox message command: 0x%x,
>rc:%d\n", cmd, rc);
> 		return rc;
>+	} else {
>+		rc = 0;
> 	}
>
Checkpatch warning:

WARNING: else is not generally useful after a break or return
#207: FILE: drivers/acpi/ras2.c:207:
+        return rc;
+    } else {

and no warning with as below,

+if (rc >= 0) {
+	rc = 0;
+} else {
+	dev_warn(ras2_ctx->dev,
+		     "Error sending PCC mbox message command: 0x%x, rc:%d\n", cmd, rc);
+	return rc;
+}