Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: James Morse <james.morse@arm.com>
To: "Wiebe, Wladislav (Nokia - DE/Ulm)" <wladislav.wiebe@nokia.com>
Cc: "mark.rutland@arm.com" <mark.rutland@arm.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"arnd@arndb.de" <arnd@arndb.de>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	Borislav Petkov <bp@alien8.de>,
	"mchehab+samsung@kernel.org" <mchehab+samsung@kernel.org>,
	"Sverdlin,
	Alexander \(Nokia - DE/Ulm\)" <alexander.sverdlin@nokia.com>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	"mchehab@kernel.org" <mchehab@kernel.org>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"linux-edac@vger.kernel.org" <linux-edac@vger.kernel.org>
Subject: Re: [PATCH 2/2] EDAC: add ARM Cortex A15 L2 internal asynchronous error detection driver
Date: Fri, 11 Jan 2019 18:11:04 +0000
Message-ID: <bba99e8c-5e3e-98a4-d0c9-9734216e66d2@arm.com> (raw)
In-Reply-To: <AM6PR0702MB37997C0FA1670DD1AF4C25C7FA8B0@AM6PR0702MB3799.eurprd07.prod.outlook.com>

Hi Wladislav,

On 09/01/2019 14:44, Wiebe, Wladislav (Nokia - DE/Ulm) wrote:
>> From: James Morse <james.morse@arm.com>
>> Sent: Tuesday, January 08, 2019 6:57 PM

>> On 08/01/2019 10:42, Borislav Petkov wrote:
>>> So the first thing to figure out here is how generic is this and if
>>> so, to make it a cortex_a15_edac.c driver which contains all the RAS
>>> functionality for A15. Definitely not an EDAC driver per functional
>>> unit but rather per vendor or even ARM core.
>>
>> This is implementation-defined/specific-to-A15 and is documented in the
>> TRM [0].
>> (On the 'all the RAS functionality for A15' front: there are two more registers:
>> L2MERRSR and CPUMERRSR. These are both accessible from the normal-
>> world, and don't appear to need enabling.)

After I sent this it occurred to me the core can't know about errors in the L3
cache (if there is one) or the memory-controller. These may have edac/ras
abilities, but they are selected by the soc integrator, so could be per soc.
This goes against Boris's no-per-functional-unit edac drivers. If we had to pick
one out of that set, I think the memory-controller is most useful as DRAM is the
most likely to be affected by errors.


>> But we have the usual pre-v8.2 problems, and in addition cluster-interrupts,
>> as this signal might be per-cluster, or it might be combined.
>>
>> Wladislav, I'm afraid we've had a few attempts at pre-8.2 EDAC drivers, the
>> below list of problems is what we've learnt along the way. The upshot is that
>> before the architected RAS extensions, the expectation is firmware will
>> handle all this, as its difficult for the OS to deal with.
>>
>>
>> My first question is how useful is a 'something bad happened' edac event?
> 
> We experienced sometimes random user-space crashes where we didn't 
> expect a bug in the application code. If there would be a notification 
> by such edac event,

Sure, but we always have to assume its the worst case: an uncontained error (to
use the v8.2 terms). A write has gone somewhere it shouldn't, we can't trust
memory anymore.

> we would at least know that something bad happened before.

>>> On Tue, Jan 08, 2019 at 08:10:45AM +0000, Wiebe, Wladislav (Nokia -
>> DE/Ulm) wrote:
>>>> This driver adds support for L2 internal asynchronous error detection
>>>> caused by L2 RAM double-bit ECC error or illegal writes to the
>>>> Interrupt Controller memory-map region on the Cortex A15.
>>
>>>> diff --git a/drivers/edac/cortex_a15_l2_async_edac.c
>>>> b/drivers/edac/cortex_a15_l2_async_edac.c
>>>> new file mode 100644
>>>> index 000000000000..26252568e961
>>>> --- /dev/null
>>>> +++ b/drivers/edac/cortex_a15_l2_async_edac.c
>>>> @@ -0,0 +1,134 @@

>>>> +static int cortex_a15_l2_async_edac_probe(struct platform_device
>>>> +*pdev) {
>>>> +	struct edac_device_ctl_info *dci;
>>>> +	struct device_node *np = pdev->dev.of_node;
>>>> +	char *ctl_name = (char *)np->name;
>>>> +	int i = 0, ret = 0, err_irq = 0, irq_count = 0;
>>>> +
>>>> +	/* We can have multiple CPU clusters with one INTERRIRQ per cluster
>>>> +*/
>>
>> Surely this an integration choice?
>>
>> You're accessing the cluster through a cpu register in the handler, what
>> happens if the interrupt is delivered to the wrong cluster?
>> How do we know which interrupt maps to which cluster?
>> How do we stop user-space 'balancing' the interrupts?
> 
> You are right, based on all your inputs I think we can stop using this driver
> as generic A15 solution

Handling this interrupt in firmware is probably the best for your soc. For a
generic a15 driver in the kernel, we would have to consider 'no interrupt',
(e.g. the interrupt is wired to some other SCP/BMC thing). Once we've got
polling code for these registers, we may as well always use it.


Thanks,

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-08  8:10 Wiebe, Wladislav (Nokia - DE/Ulm)
2019-01-08 10:42 ` Borislav Petkov
2019-01-08 17:57   ` James Morse
2019-01-08 18:12     ` gregkh
2019-01-09  9:57       ` James Morse
2019-01-09 14:44     ` Wiebe, Wladislav (Nokia - DE/Ulm)
2019-01-11 18:11       ` James Morse [this message]
2019-01-11 18:29         ` Borislav Petkov

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=bba99e8c-5e3e-98a4-d0c9-9734216e66d2@arm.com \
    --to=james.morse@arm.com \
    --cc=akpm@linux-foundation.org \
    --cc=alexander.sverdlin@nokia.com \
    --cc=arnd@arndb.de \
    --cc=bp@alien8.de \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-edac@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mchehab+samsung@kernel.org \
    --cc=mchehab@kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=wladislav.wiebe@nokia.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-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.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-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org infradead-linux-arm-kernel@archiver.kernel.org
	public-inbox-index linux-arm-kernel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


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