Hi Boris,

Thanks for the feedback.
Apologies for the delay.

-----Original Message-----
From: Borislav Petkov [mailto:bp@alien8.de]
Sent: 31 December 2020 16:44
To: Shiju Jose <redacted>
Cc: linux-edac@vger.kernel.org; linux-acpi@vger.kernel.org; linux-
kernel@vger.kernel.org; james.morse@arm.com;
mchehab+huawei@kernel.org; tony.luck@intel.com; rjw@rjwysocki.net;
lenb@kernel.org; rrichter@marvell.com; Linuxarm [off-list ref];
xuwei (O) [off-list ref]; Jonathan Cameron
[off-list ref]; John Garry [off-list ref];
tanxiaofei [off-list ref]; Shameerali Kolothum Thodi
[off-list ref]; Salil Mehta
[off-list ref]
Subject: Re: [RFC PATCH 1/2] EDAC/ghes: Add EDAC device for the CPU
caches

On Tue, Dec 08, 2020 at 05:29:58PM +0000, Shiju Jose wrote:

quoted

The corrected error count on the CPU caches required reporting to the
user-space for the predictive failure analysis. For this purpose, add
an EDAC device and device blocks for the CPU caches found.
The cache's corrected error count would be stored in the
/sys/devices/system/edac/cpu/cpu*/cache*/ce_count.

This still doesn't begin to explain why the kernel needs this. I had already
asked whether errors in CPU caches are something which happen often
enough so that software should count them but nothing came. So pls justify
first why this wants to be added to the kernel.

L2 cache corrected errors are detected occasionally on few of
our ARM64 hardware boards. Though it is rare, the probability of
the CPU cache errors frequently occurring can't be avoided.
The earlier failure detection by monitoring the cache corrected
errors for the frequent occurrences and taking preventive
action could prevent more serious hardware faults.

On Intel architectures, cache corrected errors are reported and
the affected cores are offline in the architecture specific method.
http://www.mcelog.org/cache.html

However for the firmware-first error reporting, specifically on
ARM64 architectures, there is no provision present for reporting
the cache corrected error count to the user-space and taking
preventive action such as offline the affected cores.

quoted

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig index

7a47680d6f07..c73eeab27ac9 100644

--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig

@@ -74,6 +74,16 @@ config EDAC_GHES

 	  In doubt, say 'Y'.

+config EDAC_GHES_CPU_ERROR
+	bool "EDAC device for reporting firmware-first BIOS detected CPU

error count"

Why a separate Kconfig item?

CONFIG_EDAC_GHES_CPU_CACHE_ERROR is added to make this
feature optional only for the platforms which need this and supported.

quoted

+	depends on EDAC_GHES
+	help
+	  EDAC device for the firmware-first BIOS detected CPU error count
+reported

Well this is not what it is doing - you're talking about cache errors.
"CPU errors" can be a lot more than just cache errors.

Sure. I will change.

quoted

+static void ghes_edac_create_cpu_device(struct device *dev) {
+	int cpu;
+	struct cpu_cacheinfo *this_cpu_ci;
+
+	/*
+	 * Find the maximum number of caches present in the CPU heirarchy
+	 * among the online CPUs.
+	 */
+	for_each_online_cpu(cpu) {
+		this_cpu_ci = get_cpu_cacheinfo(cpu);
+		if (!this_cpu_ci)
+			continue;
+		if (max_number_of_caches < this_cpu_ci->num_leaves)
+			max_number_of_caches = this_cpu_ci->num_leaves;

So this is counting the number of cache levels on the system? So you want to
count the errors per cache levels?

Yes. This was the suggestion from James and to offline the affected cores for the shared cache.

quoted

+	}
+	if (!max_number_of_caches)
+		return;
+
+	/*
+	 * EDAC device interface only supports creating the CPU cache

hierarchy for alls

quoted

+	 * the CPUs together. Thus need to allocate cpu_edac_block_list for

the

quoted

+	 * max_number_of_caches among all the CPUs irrespective of the

number of caches

quoted

+	 * per CPU might vary.
+	 */

So this is lumping all the caches together into a single list? What for?
To untangle to the proper ones when the error gets reported?

Have you heard of percpu variables?

Yes. Changed the list to the percpu variable.

quoted

@@ -624,6 +787,10 @@ int ghes_edac_register(struct ghes *ghes, struct

device *dev)

quoted

 	ghes_pvt = pvt;
 	spin_unlock_irqrestore(&ghes_lock, flags);

+#if defined(CONFIG_EDAC_GHES_CPU_ERROR)
+	ghes_edac_create_cpu_device(dev);
+#endif
+

Init stuff belongs into ghes_scan_system().

Did you mean calling  ghes_edac_create_cpu_device() in the ghes_scan_system()?

...

Ok, I've seen enough. "required reporting to the user-space for the predictive
failure analysis." is not even trying to explain *why* you're doing this, what
*actual* problem it is addressing and why should the kernel get it.

And without a proper problem definition of what you're trying to solve, this
is not going anywhere.

--
Regards/Gruss,
   Boris.

Thanks,
Shiju