-----Original Message-----
From: Daniel Ferguson <redacted>
Sent: 25 July 2025 17:36
To: Shiju Jose <redacted>; rafael@kernel.org; linux-
edac@vger.kernel.org; linux-acpi@vger.kernel.org; linux-doc@vger.kernel.org;
bp@alien8.de; tony.luck@intel.com; lenb@kernel.org; leo.duran@amd.com;
Yazen.Ghannam@amd.com; mchehab@kernel.org
Cc: Jonathan Cameron <jonathan.cameron@huawei.com>; linux-
mm@kvack.org; Linuxarm [off-list ref]; 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; dferguson@amperecomputing.com;
wbs@os.amperecomputing.com; nifan.cxl@gmail.com; tanxiaofei
[off-list ref]; Zengtao (B) [off-list ref]; Roberto
Sassu [off-list ref]; kangkang.shen@futurewei.com;
wanghuiqiang [off-list ref];
vanshikonda@os.amperecomputing.com
Subject: Re: [PATCH v9 2/2] ras: mem: Add memory ACPI RAS2 driver

quoted

+static int ras2_hw_scrub_write_addr(struct device *dev, void
+*drv_data, u64 base) {
+	struct ras2_mem_ctx *ras2_ctx = drv_data;
+	struct acpi_ras2_ps_shared_mem __iomem *ps_sm =
+		TO_ACPI_RAS2_PS_SHMEM(ras2_ctx->comm_addr);
+	bool running;
+	int ret;
+
+	if (ras2_ctx->bg_scrub)
+		return -EBUSY;
+
+	guard(mutex)(ras2_ctx->pcc_lock);
+	ps_sm->common.set_caps[0] = RAS2_SUPPORT_HW_PARTOL_SCRUB;
+
+	if (!ras2_ctx->size) {
+		dev_warn(ras2_ctx->dev,
+			 "%s: Invalid address range, base=0x%llx size=0x%llx\n",
+			 __func__, base, ras2_ctx->size);
+		return -ERANGE;
+	}
+
+	ret = ras2_get_patrol_scrub_running(ras2_ctx, &running);
+	if (ret)
+		return ret;
+
+	if (running)
+		return -EBUSY;
+
+	ras2_ctx->base = base;
+	ps_sm->params.scrub_params_in &= ~RAS2_PS_SC_HRS_IN_MASK;
+	ps_sm->params.scrub_params_in |=

FIELD_PREP(RAS2_PS_SC_HRS_IN_MASK,

quoted

+						    ras2_ctx->scrub_cycle_hrs);
+	ps_sm->params.req_addr_range[0] = ras2_ctx->base;
+	ps_sm->params.req_addr_range[1] = ras2_ctx->size;
+	ps_sm->params.scrub_params_in &= ~RAS2_PS_EN_BACKGROUND;
+	ps_sm->params.command = RAS2_START_PATROL_SCRUBBER;
+
+	ret = ras2_send_pcc_cmd(ras2_ctx, PCC_CMD_EXEC_RAS2);
+	if (ret) {
+		dev_err(ras2_ctx->dev, "Failed to start demand scrubbing

rc(%d)\n", ret);

quoted

+		if (ret != -EBUSY) {
+			ps_sm->params.req_addr_range[0] = 0;
+			ps_sm->params.req_addr_range[1] = 0;
+			ras2_ctx->base = 0;
+			ras2_ctx->size = 0;
+			ras2_ctx->od_scrub_sts = OD_SCRUB_STS_IDLE;
+		}
+		return ret;
+	}
+	ras2_ctx->od_scrub_sts = OD_SCRUB_STS_ACTIVE;
+
+	return ras2_update_patrol_scrub_params_cache(ras2_ctx);
+}

After a lot more discussion within Ampere, I've come to believe that using
base==0 && size==0 as a special case, when invoking
GET_PATROL_PARAMETERS after an error, is not a good idea.

There are three problems with the current solution.

1) This is undefined behavior and will likely behave differently on different
implementations. For example, 0 is an invalid physical address of the Ampere
systems, could be a valid address on some systems, or could be device memory
on another. In case it is a valid DDR address, the firmware can fill in the Scrub
Parameters, Extended Data Region (ACPI 6.6) with proper information. But in
case it is an invalid address, or device memory, the firmware has to indicate an
error to prevent the OS from consuming bad data reported through these output
parameters.

2) Pretend you are using acpi_ras2_mem0/scrub1, which corresponds to the 2nd
NUMA node in the system. If we use the current OS driver implementation as-is
and zero out base and size after an error, then when we issue the next
GET_PATROL_PARAMETERS we will have retrieved the parameters
corresponding to the wrong NUMA node. The driver will have fetched the
parameters for NUMA node 1, even though we are interacting with the scrubber
corresponding to NUMA node 2.
This could be problematic in a situation where patrol parameters differ across
different NUMA nodes. Additionally, and perhaps most importantly, using the
GET_PATROL_PARAMETERS to determine if a Scrubber is running require the
correct address range. The current implementation is not getting the running
status of the correct range.

3) This is a special case of #2. During driver load, ras2_probe issues a
GET_PATROL_PARAMETERS using a base and size equal to 0. The base and size
are zero because requested_address_range is allocated with kzalloc. In this case,
we may not be getting the initial values from the correct range.

In the probe stage, while issue GET_PATROL_PARAMETERS, driver has no information
about the  'requested_address_range' supposed to set other than set to 0 because
RAS2 ('RAS2 Platform Communication Channel Shared Memory Region' table?) does not
define fields for the platform to advertise the supported full address range for a scrubber.

Proposed Solution:
What we propose, is to instead of zeroing out the base and size after an error,
use the full range of the current NUMA node. We believe that a superset of a
currently active scrub range can properly report all the relevant and correct
information.
To be compliant with the specification, FW should set "Flags" field if there is any
on-demand scrub in progress on any memory range in the NUMA node. Again,
this solution assumes that the driver does not allow more than one scrubber to
run within a single NUMA node. All three problems can be solved in the same
way.

What do you think?

Hi Daniel,

Please check the v10 of the RAS2 series sent with changes for your requirements.
and request please test in your system and share the feedback.

https://lore.kernel.org/all/20250801172040.2175-1-shiju.jose@huawei.com/ (local)

Regards,
~Daniel

Thanks,
Shiju