Re: [PATCH v13 1/2] ACPI:RAS2: Add driver for the ACPI RAS2 feature table

[Borislav Petkov <bp@alien8.de>]

On Tue, Nov 25, 2025 at 01:28:19PM +0000, Shiju Jose wrote:
> I will change to depends. I followed the existing CONFIG ACPI_CPPC_LIB. 

Read the "Note:" under 

"- reverse dependencies: "select" <symbol> ["if" <expr>]"

here pls: Documentation/kbuild/kconfig-language.rst

Now, some of the Kconfig symbols:

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 2322b0470d07..7f846c22fc30 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -295,7 +295,7 @@ config ACPI_CPPC_LIB
 
 config ACPI_RAS2
        bool "ACPI RAS2 driver"
-       depends on AUXILIARY_BUS
+       select AUXILIARY_BUS
        depends on MAILBOX
        depends on PCC
        help
diff --git a/drivers/ras/Kconfig b/drivers/ras/Kconfig
index dfc3a899280e..a1e6aed8bcc8 100644
--- a/drivers/ras/Kconfig
+++ b/drivers/ras/Kconfig
@@ -51,7 +51,7 @@ config MEM_ACPI_RAS2
        depends on ACPI_RAS2
        depends on EDAC
        depends on EDAC_SCRUB
-       depends on NUMA_KEEP_MEMINFO
+       select NUMA_KEEP_MEMINFO
        help
          The driver binds to the auxiliary device added by the ACPI RAS2
          feature table parser. The driver uses a PCC channel subspace to

are made to be selectable only and so you should select them because they're
non-visible. Just remember that blindly selecting things is evil.


> >> +			sspcc->last_cmd, sspcc->pcc_id);
> >> +		status &= ~PCC_STATUS_ERROR;
> >> +		writew_relaxed(status, &gen_comm_base->status);
> >> +		return -EIO;
> >> +	}
> >> +
> >> +	cap_status = readw_relaxed(&gen_comm_base->set_caps_status);
> >
> >Is that register read always successful or you need to handle errors here too?
> 
> Return value of 'set capability status'  is decoded and return error code on error case
> in the below function call  'return decode_cap_error(cap_status)'

Yah, this is not a common coding pattern. What you do is something like this:

diff --git a/drivers/acpi/ras2.c b/drivers/acpi/ras2.c
index 627895fee143..4caef7f2c4ea 100644
--- a/drivers/acpi/ras2.c
+++ b/drivers/acpi/ras2.c
@@ -85,7 +85,6 @@ static int decode_cap_error(u32 cap_status)
 static int check_pcc_chan(struct ras2_sspcc *sspcc)
 {
        struct acpi_ras2_shmem __iomem *gen_comm_base = sspcc->comm_addr;
-       u32 cap_status;
        u16 status;
        int rc;
 
@@ -114,9 +113,11 @@ static int check_pcc_chan(struct ras2_sspcc *sspcc)
                return -EIO;
        }
 
-       cap_status = readw_relaxed(&gen_comm_base->set_caps_status);
+       rc = decode_cap_error(readw_relaxed(&gen_comm_base->set_caps_status));
+
        writew_relaxed(0x0, &gen_comm_base->set_caps_status);
-       return decode_cap_error(cap_status);
+
+       return rc;
 }


> >> +	 */
> >> +	if (cmd == PCC_CMD_EXEC_RAS2 || sspcc->pcc_mrtt) {
> >> +		rc = check_pcc_chan(sspcc);
> >> +		if (sspcc->pcc_mrtt)
> >> +			sspcc->last_cmd_cmpl_time = ktime_get();
> >> +	}
> >> +
> >> +	if (pcc_channel->mbox->txdone_irq)
> >> +		mbox_chan_txdone(pcc_channel, rc);
> >> +	else
> >> +		mbox_client_txdone(pcc_channel, rc);
> >> +
> >> +	return rc < 0 ? rc : 0;
> >
> >So you mean simply
> >
> >	return rc;
> >
> >no? rc can be 0 too so what's the point of the ternary expression?
> 
> This was added to handle the case rc = check_pcc_chan(sspcc); is not called
> and last rc is returned from mbox_send_message() call because mbox_send_message()
> return non-negative value for success and negative value for failure as per the documentation.
> https://elixir.bootlin.com/linux/v6.18-rc7/source/drivers/mailbox/mailbox.c#L241

Why do you keep pointing to some indexing service? What's wrong with simply
pasting the code snippet you mean so that I can find it myself too?

Anyway, what's wrong with:

        /* Ring doorbell */
        rc = mbox_send_message(pcc_channel, &cmd);
        if (rc < 0) {
                dev_warn(ras2_ctx->dev,
                         "Error sending PCC mbox message cmd: 0x%x, rc:%d\n", cmd, rc);
                return rc;
        }

Also, cmds in hex please.

> >And what's the logic here? You'd capture rc above from check_pcc_chan() and
> >even if it is != 0, you'd pass it into the mbox* functions? I guess that weirdness
> >deserves a comment...
> 
> Both mbox_chan_txdone() and  mbox_client_txdone() required the status of the
> last transmission as second argument.

Yah, comment please!

s> >
> >> +{
> >> +	struct acpi_ras2_pcc_desc *pcc_desc_list;
> >> +	struct ras2_mem_ctx *ras2_ctx;
> >> +	u16 i, count;
> >> +
> >> +	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;
> >> +	}
> >> +
> >> +	if (!ras2_tab->num_pcc_descs) {
> >> +		pr_warn(FW_WARN "No PCC descs in ACPI RAS2 table\n");
> >> +		return;
> >> +	}
> >
> >You need to sanity-check the number of descs so that the below allocation
> >doesn't go nuts.
> Sorry, can you give more information?
> I am wondering the above check  'if (!ras2_tab->num_pcc_descs)' { } is not enough? 

You've done what I wanted:

        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;
        }

The RAS2_MAX_NUM_PCC_DESCS thing.

> >Also, what's the point of that pctx_list array at all? So that you can do uninit on
> >the ->adev in case you encounter a failure?
> Local variable ras2_ctx  is updated when calling add_aux_device() in each iteration as
> add_aux_device()  allocates memory for struct ras2_mem_ctx  for the corresponding PCC
> descriptor in the RAS2 table. 
> Thus storing pointer to each ras2_ctx  in pctx_list[] to uninit all the previously added auxiliary devices
> using auxiliary_device_uninit(->adev); when encounter a failure in a later iteration.   

Looks weird. Lemme look at your new submission and see whether I can make it
better.


> >> +		return;
> >> +	}
> >> +
> >> +	acpi_ras2_parse(ras2_tab);
> >
> >This function does some table sanity checking and warns. What it should do is fail
> >the driver load if the table is broken.
> 
> Sure. 
> If acpi_ras2_parse() and thus acpi_ras2_init() return error, can you guide
> how to handle this error in acpi_init(void) where  acpi_ras2_init() is called?  
> Something similar to this below,
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c index b02ceb2837c6..8b4fc572a05b 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -1475,7 +1475,12 @@ static int __init acpi_init(void)
>         acpi_debugger_init();
>         acpi_setup_sb_notify_handler();
>         acpi_viot_init();
> -       acpi_ras2_init();
> +       result = acpi_ras2_init();
> +       if (result) {
> +               kobject_put(acpi_kobj);
> +               disable_acpi();

No, you certainly won't disable ACPI if that RAS2 thing parsing fails. What
you should do is not allow the RAS2 memory driver to load.

Lemme look at your new version.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette