[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <59075138-f819-a59c-a72a-663062c78c86@amazon.com>
Date: Thu, 1 Aug 2019 11:20:26 +0300
From: "Hawa, Hanna" <hhhawa@...zon.com>
To: James Morse <james.morse@....com>
CC: <robh+dt@...nel.org>, <mark.rutland@....com>, <bp@...en8.de>,
<mchehab@...nel.org>, <davem@...emloft.net>,
<gregkh@...uxfoundation.org>, <linus.walleij@...aro.org>,
<Jonathan.Cameron@...wei.com>, <nicolas.ferre@...rochip.com>,
<paulmck@...ux.ibm.com>, <dwmw@...zon.co.uk>, <benh@...zon.com>,
<ronenk@...zon.com>, <talel@...zon.com>, <jonnyc@...zon.com>,
<hanochu@...zon.com>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-edac@...r.kernel.org>
Subject: Re: [PATCH v3 2/4] edac: Add support for Amazon's Annapurna Labs L1
EDAC
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@....com>
Thanks James.
Thanks,
Hanna
>
>
> Thanks,
>
> James
>
Powered by blists - more mailing lists