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

["Hawa, Hanna" <hhhawa@amazon.com>]

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
>