Linux-EDAC Archive on lore.kernel.org
 help / color / Atom feed
From: James Morse <james.morse@arm.com>
To: Hanna Hawa <hhhawa@amazon.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: Fri, 26 Jul 2019 17:49:31 +0100
Message-ID: <a2dc6760-50e2-6e98-5b61-002836d92dd2@arm.com> (raw)
In-Reply-To: <1563197049-12679-3-git-send-email-hhhawa@amazon.com>

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.

[...]

> +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)


> +	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.


> +}


> +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)


> +		return -ENOMEM;


With the header-includes and edac_device_alloc_ctl_info() NULL check:
Reviewed-by: James Morse <james.morse@arm.com>


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 [this message]
2019-08-01  8:20     ` Hawa, Hanna
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=a2dc6760-50e2-6e98-5b61-002836d92dd2@arm.com \
    --to=james.morse@arm.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=hhhawa@amazon.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