lkml.org 
[lkml]   [2019]   [Aug]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v3 2/4] edac: Add support for Amazon's Annapurna Labs L1 EDAC
Date

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
>

\
 
 \ /
  Last update: 2019-08-01 10:21    [W:0.091 / U:0.964 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site