RE: [PATCH v12 1/2] ACPI:RAS2: Add ACPI RAS2 driver

[Shiju Jose <shiju.jose@huawei.com>]

>-----Original Message-----
>From: Borislav Petkov <bp@alien8.de>
>Sent: 12 September 2025 15:12
>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 v12 1/2] ACPI:RAS2: Add ACPI RAS2 driver
>
>On Fri, Sep 12, 2025 at 12:04:57PM +0000, Shiju Jose wrote:
>> >Why is this requirement here?
>> The physical memory address range retrieved here for the NUMA domain
>> is used in the subsequent patch  [PATCH v12 2/2] ras: mem: Add memory
>> ACPI RAS2 driver, 1. to set Requested Address Range(INPUT) field of
>> Table 5.87: Parameter Block Structure for PATROL_SCRUB when send
>> GET_PATROL_PARAMETERS command to the firmware, to get scrub
>parameters, running status, current scrub rate etc.
>> 2. for the validity check of the user requested memory address range to scrub.
>
>Again, why does it have to be *lowest* and *contiguous*?
>
>Your answer doesn't explain that.
This has been added as suggested by Jonathan considering the interleaved NUMA node.
Link to the related discussion in V11:
https://lore.kernel.org/all/20250821100655.00003942@huawei.com/#t

| node 0 | node 1 | node 0 |   PA address map.
Can you give your suggestion what we should do about it?

>
>> Also intended to expose this supported memory address range to the
>> userspace via EDAC scrub control interface, though it is not present now.
>
>Why? To tie ourselves with even more user ABI?!
>
>There better be a good reason and not a better design for what this is trying to
>do.

The "address_range_base" and "address_range_size" sysfs attributes
(until the v13 of EDAC scrub interface,) which we could be used for publish this
physical address range of the memory in NUMA domain to the userspace when the demand scrubbing
is not in progress, but "address_range_base" has changed to read the status of  on-demand
scrubbing based on the feedback here.
https://lore.kernel.org/all/4ee36d03a2894606a571b37f440da36f@huawei.com/#t   
Also to present this requirement for the RAS2,  there was no method found to retrieve the
memory physical address range until recent versions as it is not supported in the RAS2.

Use case for the RAS2 to publish the supported PA range of the node memory to the userspace:
Systems with multiple NUMA node domains with the support for the demand scrubbing
exposed to the user via the EDAC scrub interface as acpi_ras_mem0/scrub, acpi_ras_mem1/scrub,
acpi_ras_mem2/scrub, ... etc.   When the userspace tool (For e.g. rasdaemon)  or an admin detects
a faulty page or faulty address, system policy may decides to scrub the corresponding memory. 
However it is required to find out the EDAC scrub instance of  the corresponding  memory in the
NUMA domain, set scrub parameters and issue the scrub request.
There are two options present,
(1) Set the scrub parameters and issue scrub request in all the EDAC scrub instances present for RAS2. The
 scrub request should fail for the invalid cases.
(2)  Locate the corresponding EDAC scrub instances for the corresponding Node memory
      by read and check against the PA range published.

I think Option (2) seems better? 
If so, can the EDAC scrub interface  be updated to include attributes for publishing the supported
PA range for the memory device to scrub?

>
>> >What happens with the aux devices you created successfully here? Unwind?
>> Please see the previous discussions on this were about allowing the
>> successfully created auxiliary devices to exist.
>> https://lore.kernel.org/all/20250415210504.GA854098@yaz-khff2.amd.com/
>
>There's no discussion here. And nothing answers the question "why" this is ok to
>do this way.

This was changed  based on the feedback from the Yazen in v3 of the series,
Copy of the Yazen's feedback from the above link: 
=============================
> +	}
> +
> +	pcc_desc_list = (struct acpi_ras2_pcc_desc *)(ras2_tab + 1);
> +	for (i = 0; i < ras2_tab->num_pcc_descs; i++, pcc_desc_list++) {
> +		if (pcc_desc_list->feature_type != RAS2_FEAT_TYPE_MEMORY)
> +			continue;
> +
> +		rc = ras2_add_aux_device(RAS2_MEM_DEV_ID_NAME, pcc_desc_list->channel_id);
> +		if (rc)
> +			return rc;

This returns error on the first failure.

What if there was a success before? Does that aux_device need to be removed?

If not, then why return failure at all? Why not just try to add all devices? Some may fail and some may succeed.
============================= 

We thought second option is a better because a successfully added aux dev for a memory device and corresponding
EDAC interface continue exist and support the scrub/a memory feature. 
We do not mind doing stop on a failure adding an aux_device and free previously crated aux devices, though
it may require some additional dynamically allocated memory space to store the successfully created aux devices
so that free them on a failure later. Hope that is acceptable?

Thanks,
Shiju  

>
>--
>Regards/Gruss,
>    Boris.
>
>https://people.kernel.org/tglx/notes-about-netiquette