On 7/26/2019 7:49 PM, James Morse wrote:
Hi Hanna,
On 15/07/2019 14:24, Hanna Hawa wrote:
Adds support for Amazon's Annapurna Labs L1 EDAC driver to detect and
report L1 errors.
diff --git a/drivers/edac/al_l1_edac.c b/drivers/edac/al_l1_edac.c
new file mode 100644
index 0000000..70510ea
--- /dev/null
+++ b/drivers/edac/al_l1_edac.c
@@ -0,0 +1,156 @@
+#include <linux/bitfield.h>
You need <linux/smp.h> for on-each_cpu().
+#include "edac_device.h"
+#include "edac_module.h"
You need <asm/sysreg.h> for the sys_reg() macro. The ARCH_ALPINE dependency doesn't stop
this from being built on 32bit arm, where this sys_reg() won't work/exist.
Will fix.
[...]
+static void al_l1_edac_cpumerrsr(void *arg)
+{
+ struct edac_device_ctl_info *edac_dev = arg;
+ int cpu, i;
+ u32 ramid, repeat, other, fatal;
+ u64 val = read_sysreg_s(ARM_CA57_CPUMERRSR_EL1);
+ char msg[AL_L1_EDAC_MSG_MAX];
+ int space, count;
+ char *p;
+ if (!(FIELD_GET(ARM_CA57_CPUMERRSR_VALID, val)))
+ return;
+ space = sizeof(msg);
+ p = msg;
+ count = snprintf(p, space, "CPU%d L1 %serror detected", cpu,
+ (fatal) ? "Fatal " : "");
+ p += count;
+ space -= count;
snprintf() will return the number of characters it would have generated, even if that is
more than space. If this happen, space becomes negative, p points outside msg[] and msg[]
isn't NULL terminated...
It looks like you want scnprintf(), which returns the number of characters written to buf
instead. (I don't see how 256 characters would be printed by this code)
Will use scnprintf() instead.
+ switch (ramid) {
+ case ARM_CA57_L1_I_TAG_RAM:
+ count = snprintf(p, space, " RAMID='L1-I Tag RAM'");
+ break;
+ case ARM_CA57_L1_I_DATA_RAM:
+ count = snprintf(p, space, " RAMID='L1-I Data RAM'");
+ break;
+ case ARM_CA57_L1_D_TAG_RAM:
+ count = snprintf(p, space, " RAMID='L1-D Tag RAM'");
+ break;
+ case ARM_CA57_L1_D_DATA_RAM:
+ count = snprintf(p, space, " RAMID='L1-D Data RAM'");
+ break;
+ case ARM_CA57_L2_TLB_RAM:
+ count = snprintf(p, space, " RAMID='L2 TLB RAM'");
+ break;
+ default:
+ count = snprintf(p, space, " RAMID='unknown'");
+ break;
+ }
+
+ p += count;
+ space -= count;
+ count = snprintf(p, space,
+ " repeat=%d, other=%d (CPUMERRSR_EL1=0x%llx)",
+ repeat, other, val);
+
+ for (i = 0; i < repeat; i++) {
+ if (fatal)
+ edac_device_handle_ue(edac_dev, 0, 0, msg);
+ else
+ edac_device_handle_ce(edac_dev, 0, 0, msg);
+ }
+
+ write_sysreg_s(0, ARM_CA57_CPUMERRSR_EL1);
Writing 0 just after you've read the value would minimise the window where repeat could
have increased behind your back, or another error was counted as other, when it could have
been reported more accurately.
Good point, I will move the write after checking the valid bit.
+}
+static int al_l1_edac_probe(struct platform_device *pdev)
+{
+ struct edac_device_ctl_info *edac_dev;
+ struct device *dev = &pdev->dev;
+ int ret;
+
+ edac_dev = edac_device_alloc_ctl_info(0, (char *)dev_name(dev), 1, "L",
+ 1, 1, NULL, 0,
+ edac_device_alloc_index());
+ if (IS_ERR(edac_dev))
edac_device_alloc_ctl_info() returns NULL, or dev_ctl, which comes from kzalloc(). I think
you need to check for NULL here, IS_ERR() only catches the -errno range. (there is an
IS_ERR_OR_NULL() if you really needed both)
Will fix.
+ return -ENOMEM;
With the header-includes and edac_device_alloc_ctl_info() NULL check:
Reviewed-by: James Morse <james.morse@xxxxxxx>
Thanks James.
Thanks,
Hanna
Thanks,
James
