Re: [PATCH v3 2/4] edac: Add support for Amazon's Annapurna Labs L1 EDAC

[Hawa, Hanna]

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