Hi Sascha,

On 13/10/2020 13:50, 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.

+static void read_errors(void *data)
+{
+	struct edac_device_ctl_info *edac_ctl = data;
+	int cpu = smp_processor_id();
+	char msg[MESSAGE_SIZE];
+	u64 cpumerr, l2merr;
+
+	/* cpumerrsr_el1 */
+	asm volatile("mrs %0, s3_1_c15_c2_2" : "=r" (cpumerr));
+	asm volatile("msr s3_1_c15_c2_2, %0" :: "r" (0));

I think you've seen earlier comments on using the sys_reg macros for this. There were
versions of binutils out there that choke on this.

[...]

+}
+
+static void cortex_arm64_edac_check(struct edac_device_ctl_info *edac_ctl)
+{
+	struct arm64_pvt *pvt = edac_ctl->pvt_info;
+	call_single_data_t *csd;
+	int cpu;
+
+	get_online_cpus();
+	for_each_cpu_and(cpu, cpu_online_mask, &pvt->compat_mask) {
+		csd = per_cpu_ptr(pvt->csd_check, cpu);
+		csd->func = read_errors;
+		csd->info = edac_ctl;
+		csd->flags = 0;

+		/* Read CPU L1/L2 errors */
+		smp_call_function_single_async(cpu, csd);
+		/* Wait until flags cleared */
+		smp_cond_load_acquire(&csd->flags, !VAL);

Hmm. We end up waiting for each CPU to schedule something else. I can't see any reason we
can't sleep here.

Can't we use smp_call_function_many() here? It already considers cpu_online_mask, you'd
just need to deal with read_errors() being called in parallel with itself.

(concurrent calls into edac are one problem, but two CPUs read/writing the same L2
register could lead to double counting)

+	}
+	put_online_cpus();
+}

+static int cortex_arm64_edac_probe(struct platform_device *pdev)
+{
+	struct device_node *np, *dn = pdev->dev.of_node;
+	struct edac_device_ctl_info *edac_ctl;
+	struct device *dev = &pdev->dev;
+	struct of_phandle_iterator it;
+	struct arm64_pvt *pvt;
+	int rc, cpu;
+
+	edac_ctl = edac_device_alloc_ctl_info(sizeof(*pvt), "cpu_cache",
+					      1, "L", 2, 1, NULL, 0,
+					      edac_device_alloc_index());

I used this series to test on Juno to poke the user-space interface:
This chokes on a big-little system as it can't register "cpu_cache" a second time.

I think we should try to make the topology look like the one in edac_device.h. This means
calling it 'cpu', and registering all of them up front.
On a big/little system the second probe() call would need to be careful.

I can have a go at this if you don't have a platform to hand.


(The 'L2-cache' thing in edac_device.h turns out to be impossible and the 'Lx' you've done
here is the most popular option. I'll post a patch to change the documentation to what
people are doing)


[...]

+}

Thanks,

James


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