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

Borislav Petkov
2026-03-12T16:53:46Z

On Wed, Mar 11, 2026 at 03:55:16PM +0000, shiju.jose@huawei.com wrote:
> From: Shiju Jose <shiju.jose@huawei.com>
>
> ACPI 6.5 Specification, section 5.2.21, defined RAS2 feature table (RAS2).
> Driver adds support for RAS2 feature table, which provides interfaces for
> platform RAS features, e.g., for HW-based memory scrubbing, and logical to
> PA translation service. RAS2 uses PCC channel subspace for communicating
> with the ACPI compliant HW platform.
>
> 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/bus.c | 3 +
> drivers/acpi/ras2.c | 433 ++++++++++++++++++++++++++++++++++++++++++
> include/acpi/ras2.h | 57 ++++++
> 5 files changed, 505 insertions(+)
> create mode 100644 drivers/acpi/ras2.c
> create mode 100644 include/acpi/ras2.h

First of all, what about this:

https://lore.kernel.org/r/df5fe0ed-3483-4ac5-8096-447e4e560816@os.amperecomputing.com

?

> +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;
> +
> + /*
> + * As per ACPI spec, the PCC space will be initialized by the
> + * platform and should have set the command completion bit when
> + * PCC can be used by OSPM.
> + *
> + * Poll PCC status register every PCC_MIN_POLL_USECS for maximum of
> + * PCC_NUM_RETRIES * PCC channel latency until PCC command complete
> + * bit is set.
> + */
> + rc = readw_relaxed_poll_timeout(&gen_comm_base->status, status,
> + status & PCC_STATUS_CMD_COMPLETE,
> + PCC_MIN_POLL_USECS, sspcc->deadline_us);
> + if (rc) {
> + pr_warn("PCC ID: 0x%x: PCC check channel timeout for last command: 0x%x rc=%d\n",
> + sspcc->pcc_id, sspcc->last_cmd, rc);
> +
> + return rc;
> + }
> +
> + if (status & PCC_STATUS_ERROR) {
> + pr_warn("PCC ID: 0x%x: Error in executing last command: 0x%x\n",
> + sspcc->pcc_id, sspcc->last_cmd);
> + status &= ~PCC_STATUS_ERROR;
> + writew_relaxed(status, &gen_comm_base->status);
> + return -EIO;
> + }
> +
> + cap_status = readw_relaxed(&gen_comm_base->set_caps_status);

The AI caught this:

"This is reading only 16 bits of a 32-bit field."

You're doing readw which returns u16 but you're writing it into a u32. Why?

> + switch (cap_status) {
> + case ACPI_RAS2_NOT_VALID:
> + case ACPI_RAS2_NOT_SUPPORTED:
> + rc = -EPERM;
> + break;
> + case ACPI_RAS2_BUSY:
> + rc = -EBUSY;
> + break;
> + case ACPI_RAS2_FAILED:
> + case ACPI_RAS2_ABORTED:
> + case ACPI_RAS2_INVALID_DATA:
> + rc = -EINVAL;
> + break;
> + default:
> + rc = 0;
> + }
> +
> + writew_relaxed(0x0, &gen_comm_base->set_caps_status);
> +
> + return rc;
> +}

...

> +static int register_pcc_channel(struct ras2_mem_ctx *ras2_ctx, int pcc_id)
> +{
> + struct pcc_mbox_chan *pcc_chan;
> + struct mbox_client *mbox_cl;
> + struct ras2_sspcc *sspcc;
> +
> + if (pcc_id < 0)
> + return -EINVAL;
> +
> + sspcc = kzalloc(sizeof(*sspcc), GFP_KERNEL);
> + if (!sspcc)
> + return -ENOMEM;
> +
> + mbox_cl = &sspcc->mbox_client;
> + mbox_cl->knows_txdone = true;
> +
> + pcc_chan = pcc_mbox_request_channel(mbox_cl, pcc_id);
> + if (IS_ERR(pcc_chan)) {
> + kfree(sspcc);
> + return PTR_ERR(pcc_chan);
> + }
> +
> + sspcc->pcc_id = pcc_id;
> + sspcc->pcc_chan = pcc_chan;
> + sspcc->comm_addr = pcc_chan->shmem;
> + sspcc->deadline_us = PCC_NUM_RETRIES * pcc_chan->latency;
> + sspcc->pcc_mrtt = pcc_chan->min_turnaround_time;
> + sspcc->pcc_mpar = pcc_chan->max_access_rate;
> + sspcc->mbox_client.knows_txdone = true;

"2. **Double initialization of mbox_client.knows_txdone**:
- Line 251: `mbox_cl->knows_txdone = true;`
- Line 265: `sspcc->mbox_client.knows_txdone = true;`

`mbox_cl` is `&sspcc->mbox_client`, so this sets the same field twice. This is redundant but not a bug."

Hohumm, sounds about right. That's a good catch. No one saw it until now. Good
job Claude :-P

> + sspcc->pcc_chnl_acq = true;

"4. **Unused field `pcc_chnl_acq`**:
- Line 266: `sspcc->pcc_chnl_acq = true;` is set
- Looking for uses... nowhere else in the code uses this field!"

I couldn't find any either.

> +
> + ras2_ctx->sspcc = sspcc;
> + ras2_ctx->comm_addr = sspcc->comm_addr;
> + ras2_ctx->dev = pcc_chan->mchan->mbox->dev;
> +
> + mutex_init(&sspcc->pcc_lock);
> + ras2_ctx->pcc_lock = &sspcc->pcc_lock;
> +
> + return 0;
> +}
> +
> +static DEFINE_IDA(ras2_ida);
> +static void ras2_release(struct device *device)
> +{
> + struct auxiliary_device *auxdev = to_auxiliary_dev(device);
> + struct ras2_mem_ctx *ras2_ctx = container_of(auxdev, struct ras2_mem_ctx, adev);
> + struct ras2_sspcc *sspcc;
> +
> + ida_free(&ras2_ida, auxdev->id);
> + sspcc = ras2_ctx->sspcc;
> + pcc_mbox_free_channel(sspcc->pcc_chan);
> + kfree(sspcc);
> + kfree(ras2_ctx);
> +}
> +
> +static struct ras2_mem_ctx *add_aux_device(char *name, int channel, u32 pxm_inst)

Another good catch:

"5. **The `name` parameter is unused in add_aux_device()**:
Looking at the function signature:
```c
static struct ras2_mem_ctx *add_aux_device(char *name, int channel, u32 pxm_inst)
```
The `name` parameter is passed in but never used in the function body. The function uses the constant `RAS2_MEM_DEV_ID_NAME` instead on line 325.

This is dead code - parameter is never used."

> +{
> + struct ras2_mem_ctx *ras2_ctx;
> + struct ras2_sspcc *sspcc;
> + u32 comp_nid;
> + int id, rc;
> +
> + comp_nid = pxm_to_node(pxm_inst);
> + if (comp_nid == NUMA_NO_NODE) {
> + pr_debug("Invalid NUMA node, channel=%d pxm_inst=%d\n", channel, pxm_inst);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + ras2_ctx = kzalloc(sizeof(*ras2_ctx), GFP_KERNEL);
> + if (!ras2_ctx)
> + return ERR_PTR(-ENOMEM);
> +
> + ras2_ctx->sys_comp_nid = comp_nid;
> +
> + rc = register_pcc_channel(ras2_ctx, channel);
> + if (rc < 0) {
> + pr_debug("Failed to register PCC channel=%d pxm_inst=%d rc=%d\n", channel,
> + pxm_inst, rc);
> + goto ctx_free;
> + }
> +
> + id = ida_alloc(&ras2_ida, GFP_KERNEL);
> + if (id < 0) {
> + rc = id;
> + goto pcc_free;
> + }
> +
> + ras2_ctx->adev.id = id;
> + ras2_ctx->adev.name = RAS2_MEM_DEV_ID_NAME;
^^^^^^^^^^^^^^^^^^^^^

--
Regards/Gruss,
Boris.