Messages in this thread | | | From | "Hawa, Hanna" <> | Subject | Re: [PATCH v3 2/4] edac: Add support for Amazon's Annapurna Labs L1 EDAC | Date | Thu, 1 Aug 2019 11:20:26 +0300 |
| |
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@arm.com>
Thanks James.
Thanks, Hanna > > > Thanks, > > James >
| |