Re: [PATCH 1/2] drivers/edac: Add L1 and L2 error detection for A53 and A57

On Thu, Aug 13, 2020 at 09:57:20AM +0200, Sascha Hauer wrote:

The Cortex A53 and A57 cores have error detection capabilities for the
L1/L2 Caches, this patch adds a driver for them.

Unfortunately there is no robust way to inject errors into the caches,
so this driver doesn't contain any code to actually test it. It has
been tested though with code taken from an older version of this driver
found here: https://lkml.org/lkml/2018/3/14/1203. For reasons stated
in this thread the error injection code is not suitable for mainline,
so it is removed from the driver.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 .../bindings/edac/arm,cortex-a5x-edac.yaml    |  32 +++
 drivers/edac/Kconfig                          |   6 +
 drivers/edac/Makefile                         |   1 +
 drivers/edac/cortex_arm64_l1_l2.c             | 208 ++++++++++++++++++
 4 files changed, 247 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/edac/arm,cortex-a5x-edac.yaml
 create mode 100644 drivers/edac/cortex_arm64_l1_l2.c

Just nitpicks below. James'd need to look at this too before it goes
anywhere.

Checkpatch is trying to tell me something here:

WARNING: DT compatible string "arm,cortex-a53-edac" appears un-documented -- check ./Documentation/devicetree/bindings/
#296: FILE: drivers/edac/cortex_arm64_l1_l2.c:190:
+       { .compatible = "arm,cortex-a53-edac" },

WARNING: DT compatible string "arm,cortex-a57-edac" appears un-documented -- check ./Documentation/devicetree/bindings/
#297: FILE: drivers/edac/cortex_arm64_l1_l2.c:191:
+       { .compatible = "arm,cortex-a57-edac" },

for 2/2 too:

WARNING: DT compatible string "arm,cortex-a53-edac" appears un-documented -- check ./Documentation/devicetree/bindings/
#39: FILE: arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi:842:
+               compatible = "arm,cortex-a53-edac";

WARNING: DT compatible string "arm,cortex-a57-edac" appears un-documented -- check ./Documentation/devicetree/bindings/
#56: FILE: arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi:805:
+               compatible = "arm,cortex-a57-edac";


False positive or valid?

...

+static void read_errors(void *data)
+{
+	u64 cpumerr, l2merr;
+	int cpu = smp_processor_id();
+	char msg[MESSAGE_SIZE];
+	struct edac_device_ctl_info *edac_ctl = data;

Please sort function local variables declaration in a reverse christmas
tree order:

	<type A> longest_variable_name;
	<type B> shorter_var_name;
	<type C> even_shorter;
	<type D> i;

Check your other functions too pls.

+	/* cpumerrsr_el1 */
+	asm volatile("mrs %0, s3_1_c15_c2_2" : "=r" (cpumerr));
+	asm volatile("msr s3_1_c15_c2_2, %0" :: "r" (0));
+
+	if (cpumerr & CPUMERRSR_EL1_VALID) {
+		const char *str;
+		int fatal = (cpumerr & CPUMERRSR_EL1_FATAL) != 0;

Don't need "!= 0" and fatal can be bool.

+		switch (FIELD_GET(CPUMERRSR_EL1_RAMID, cpumerr)) {
+		case L1_I_TAG_RAM:
+			str = "L1-I Tag RAM";
+			break;
+		case L1_I_DATA_RAM:
+			str = "L1-I Data RAM";
+			break;
+		case L1_D_TAG_RAM:
+			str = "L1-D Tag RAM";
+			break;
+		case L1_D_DATA_RAM:
+			str = "L1-D Data RAM";
+			break;
+		case L1_D_DIRTY_RAM:
+			str = "L1 Dirty RAM";
+			break;
+		case TLB_RAM:
+			str = "TLB RAM";
+			break;
+		default:
+			str = "unknown";
+			break;
+		}
+
+		snprintf(msg, MESSAGE_SIZE, "%s %s error(s) on CPU %d",
+			 str, fatal ? "fatal" : "correctable", cpu);
+
+		if (fatal)
+			edac_device_handle_ue(edac_ctl, 0, 0, msg);
+		else
+			edac_device_handle_ce(edac_ctl, 0, 0, msg);
+	}
+
+	/* l2merrsr_el1 */
+	asm volatile("mrs %0, s3_1_c15_c2_3" : "=r" (l2merr));
+	asm volatile("msr s3_1_c15_c2_3, %0" :: "r" (0));
+
+	if (l2merr & L2MERRSR_EL1_VALID) {
+		int fatal = (l2merr & L2MERRSR_EL1_FATAL) != 0;

See above.

+
+		snprintf(msg, MESSAGE_SIZE, "L2 %s error(s) on CPU %d",
+			 fatal ? "fatal" : "correctable", cpu);
+		if (fatal)
+			edac_device_handle_ue(edac_ctl, 0, 1, msg);
+		else
+			edac_device_handle_ce(edac_ctl, 0, 1, msg);
+	}
+}

...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel