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

Shiju Jose
2026-03-16T14:27:58Z

>-----Original Message-----
>From: Borislav Petkov <bp@alien8.de>
>Sent: 12 March 2026 16:53
>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 v17 1/2] ACPI:RAS2: Add driver for the ACPI RAS2 feature
>table
>
>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
Hi Boris,

Thanks for the feedback.

In v13, support for address range has removed for the code simplification requested
with the expectation that a firmware will do the full node demand scrubbing and
may enable these attributes in the follow-up patches.
I do not mind adding support for address range now or else later after first version of
RAS2 code will be merged.

>
>?
>
>> +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?

set_caps_status is 4 bytes, thus need to use readl_relaxed() instead.

>
>> + 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
Will remove the duplication.

>
>> + 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.
Will remove redundant pcc_chnl_acq as it was added for an old use case.

>
>> +
>> + 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."
Sure. Will remove.

>
>> +{
>> + 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.
>
>https://people.kernel.org/tglx/notes-about-netiquette

Thanks,
Shiju