Linux-EDAC Archive on lore.kernel.org
 help / color / Atom feed
From: "Hawa, Hanna" <hhhawa@amazon.com>
To: James Morse <james.morse@arm.com>
Cc: <robh+dt@kernel.org>, <mark.rutland@arm.com>, <bp@alien8.de>,
	<mchehab@kernel.org>, <davem@davemloft.net>,
	<gregkh@linuxfoundation.org>, <linus.walleij@linaro.org>,
	<Jonathan.Cameron@huawei.com>, <nicolas.ferre@microchip.com>,
	<paulmck@linux.ibm.com>, <dwmw@amazon.co.uk>, <benh@amazon.com>,
	<ronenk@amazon.com>, <talel@amazon.com>, <jonnyc@amazon.com>,
	<hanochu@amazon.com>, <devicetree@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <linux-edac@vger.kernel.org>
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
Message-ID: <59075138-f819-a59c-a72a-663062c78c86@amazon.com> (raw)
In-Reply-To: <a2dc6760-50e2-6e98-5b61-002836d92dd2@arm.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
> 

  reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-15 13:24 [PATCH v3 0/4] Add support for Amazon's Annapurna Labs EDAC for L1/L2 Hanna Hawa
2019-07-15 13:24 ` [PATCH v3 1/4] dt-bindings: EDAC: Add Amazon's Annapurna Labs L1 EDAC Hanna Hawa
2019-07-15 13:24 ` [PATCH v3 2/4] edac: Add support for " Hanna Hawa
2019-07-26 16:49   ` James Morse
2019-08-01  8:20     ` Hawa, Hanna [this message]
2019-09-03  7:24   ` Robert Richter
2019-09-03  8:27     ` Hawa, Hanna
2019-07-15 13:24 ` [PATCH v3 3/4] dt-bindings: EDAC: Add Amazon's Annapurna Labs L2 EDAC Hanna Hawa
2019-07-15 13:24 ` [PATCH v3 4/4] edac: Add support for " Hanna Hawa
2019-09-03  7:27   ` Robert Richter
2019-09-03  8:28     ` Hawa, Hanna
2019-09-03  8:46       ` Robert Richter
2019-09-03  8:58         ` Borislav Petkov
2019-09-03 19:00           ` Robert Richter

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=59075138-f819-a59c-a72a-663062c78c86@amazon.com \
    --to=hhhawa@amazon.com \
    --cc=Jonathan.Cameron@huawei.com \
    --cc=benh@amazon.com \
    --cc=bp@alien8.de \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=dwmw@amazon.co.uk \
    --cc=gregkh@linuxfoundation.org \
    --cc=hanochu@amazon.com \
    --cc=james.morse@arm.com \
    --cc=jonnyc@amazon.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-edac@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mchehab@kernel.org \
    --cc=nicolas.ferre@microchip.com \
    --cc=paulmck@linux.ibm.com \
    --cc=robh+dt@kernel.org \
    --cc=ronenk@amazon.com \
    --cc=talel@amazon.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-EDAC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-edac/0 linux-edac/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-edac linux-edac/ https://lore.kernel.org/linux-edac \
		linux-edac@vger.kernel.org
	public-inbox-index linux-edac

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-edac


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git