RE: [PATCH v17 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: 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