From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: ghes_edac: enable HIP08 platform edac driver From: Zhengqiang Message-Id: <5AFE7D24.3090408@huawei.com> Date: Fri, 18 May 2018 15:13:40 +0800 To: James Morse , Borislav Petkov , Tyler Baicar Cc: mchehab@kernel.org, toshi.kani@hpe.com, linux-edac@vger.kernel.org, linuxarm@huawei.com, "linux-arm-kernel@lists.infradead.org" List-ID: T24gMjAxOC81LzE4IDI6MDIsIEphbWVzIE1vcnNlIHdyb3RlOgo+IEhpIGd1eXMsCj4gCj4gVHls ZXIsIFpoZW5ncWlhbmcsIEkgYXNzdW1lIGFsbCB5b3VyIHNoaXBwZWQgcGxhdGZvcm1zIHdpdGgg SEVTVC0+R0hFUyBlbnRyaWVzCj4gYWxzbyBoYXZlIERNSSB0YWJsZXMuCj4gCgpTdXJlLCBPdXIg QVJNNjQgcGxhdGZvcm0gaGF2ZSBETUkgdGFibGVzLiB0aGFua3MuCgo+IAo+IE9uIDE2LzA1LzE4 IDE5OjI5LCBCb3Jpc2xhdiBQZXRrb3Ygd3JvdGU6Cj4+IE9uIFdlZCwgTWF5IDE2LCAyMDE4IGF0 IDAyOjM4OjM4UE0gKzAxMDAsIEphbWVzIE1vcnNlIHdyb3RlOgo+Pj4gWEdlbmUgaGFzIGl0cyBv d24gZWRhYyBkcml2ZXIsIGJ1dCBpdCBkb2Vzbid0IHByb2JlIHdoZW4gYm9vdGVkIHZpYSBBQ1BJ IHNvCj4+PiB3b24ndCBjb25mbGljdCB3aXRoIGdoZXNfZWRhYy4KPj4KPj4gQWN0dWFsbHkgaXQg d2lsbC4gRURBQyBjb3JlIGNhbiBoYXZlIG9ubHkgb25lIEVEQUMgZHJpdmVyIGxvYWRlZC4gRG9u J3QKPj4gYXNrIG1lIHdoeSAtIGl0IGhhcyBiZWVuIHRoYXQgd2F5IHNpbmNlIGZvcmV2ZXIuCj4g Cj4gQnkgd29uJ3QgcHJvYmUgSSBtZWFuIGl0IG9ubHkgd29ya3Mgb24gRFQgc3lzdGVtczoKPiAK PiB8IHN0YXRpYyBjb25zdCBzdHJ1Y3Qgb2ZfZGV2aWNlX2lkIHhnZW5lX2VkYWNfb2ZfbWF0Y2hb XSA9IHsKPiB8CXsgLmNvbXBhdGlibGUgPSAiYXBtLHhnZW5lLWVkYWMiIH0sCj4gfAl7fSwKPiB8 IH07Cj4gCj4gfAkuZHJpdmVyID0gewo+IHwJCS5uYW1lID0gInhnZW5lLWVkYWMiLAo+IHwJCS5v Zl9tYXRjaF90YWJsZSA9IHhnZW5lX2VkYWNfb2ZfbWF0Y2gsCj4gfAl9LAo+IAo+IFRvIHdvcmsg b24gYSBzeXN0ZW0gd2l0aCBHSEVTIGl0IHdvdWxkIG5lZWQgYW4gJ3N0cnVjdCBhY3BpX2Rldmlj ZV9pZCcgdG8KPiBkZXNjcmliZSB0aGUgSElEICg/KSBhbmQgcG9wdWxhdGUgZHJpdmVyJ3MgYWNw aV9tYXRjaF90YWJsZS4KPiAKPiAKPj4gV2UgY2FuIGNoYW5nZSBpdCBzb21lCj4+IGRheSBidXQg ZnJhbmtseSwgSSBkb24ndCBzZWUgcmVhc29uaW5nIGZvciBpdC4gT25lIGRyaXZlciBjYW4gZWFz aWx5Cj4+IG1hbmFnZSAqYWxsKiBlcnJvciBzb3VyY2VzIG9uIGEgc3lzdGVtLCBJJ2Qgc2F5Lgo+ IAo+IEkgYWdyZWUsIHRoZXJlIGlzIG5vIHJlYXNvbiB0byBzdXBwb3J0IHR3byBhdCB0aGUgc2Ft ZSB0aW1lLCBpZiB0aGlzIGhhcHBlbnMKPiB0aGVuIHRoZXJlIGlzIHByb2JhYmx5IHNvbWV0aGlu ZyB3cm9uZyB3aXRoIHRoZSBwbGF0Zm9ybSAoZS5nLiByYWNlcyB3aXRoCj4gZmlybXdhcmUgcmVh ZGluZyB0aGUgc2FtZSBoYXJkd2FyZSByZWdpc3RlcnMpLCBzbyB3ZSBzaG91bGQgbWFrZSBzb21l IG5vaXNlLgo+IAo+IFhnZW5lJ3MgZWRhYyBkcml2ZXIgd291bGQgYmUgYSBnb29kIGV4YW1wbGUg b2YgdGhpcywgaXQgbG9va3MgbGlrZSBpdCByZWFkcyBkYXRhCj4gZnJvbSBzb21lIG1taW8gcmVn aW9uLCBpZiBzb21ldGhpbmcgZWxzZSBpcyBkb2luZyB0aGUgc2FtZSB3ZSdyZSBnb2luZyB0byBt YWtlIGEKPiBtZXNzLgo+IAo+IAo+Pj4gU28gSSB0aGluayB3ZSdyZSBnb29kIHRvIG1ha2UgdGhl IHdoaXRlbGlzdCB4ODYgb25seS4KPj4+IFlvdXIgZGlmZi1odW5rIG1ha2VzICdpZHg9LTEnLCBz byB3ZSBhbHdheXMgZ2V0IHRoZSAnVW5mb3J0dW5hdGVseScgd2FybmluZy4gSSdkCj4+PiBsaWtl IHRvIHN1cHByZXNzIHRoaXMgdW5sZXNzIGZvcmNlX2xvYWQgaGFzIGJlZW4gdXNlZC4KPj4KPj4g WWVhaCwgd2Ugc2hvdWxkIGhhbmRsZSB0aGF0IGRpZmZlcmVudGx5IGZvciBBUk0uIFRvc2hpIGFk ZGVkIHRoZSBpZHgKPj4gdGhpbmcgaW4KPj4KPj4gICA1ZGVlZDZiNmE0NzkgKCJFREFDLCBnaGVz OiBBZGQgcGxhdGZvcm0gY2hlY2siKQo+Pgo+PiB0byBkdW1wIHRoaXMgd2hlbiB0aGUgcGxhdGZv cm0gaXMgbm90IHdoaXRlbGlzdGVkLiBTbyBsZXQncyBkbyB0aGF0Ogo+Pgo+PiAtLS0KPj4gZGlm ZiAtLWdpdCBhL2RyaXZlcnMvZWRhYy9naGVzX2VkYWMuYyBiL2RyaXZlcnMvZWRhYy9naGVzX2Vk YWMuYwo+PiBpbmRleCA4NjNmYmYzZGIyOWYuLjQ3M2FlZWM0YjFkYSAxMDA2NDQKPj4gLS0tIGEv ZHJpdmVycy9lZGFjL2doZXNfZWRhYy5jCj4+ICsrKyBiL2RyaXZlcnMvZWRhYy9naGVzX2VkYWMu Ywo+PiBAQCAtNDQwLDEyICs0NDAsMTYgQEAgaW50IGdoZXNfZWRhY19yZWdpc3RlcihzdHJ1Y3Qg Z2hlcyAqZ2hlcywgc3RydWN0IGRldmljZSAqZGV2KQo+PiAgCXN0cnVjdCBtZW1fY3RsX2luZm8g Km1jaTsKPj4gIAlzdHJ1Y3QgZWRhY19tY19sYXllciBsYXllcnNbMV07Cj4+ICAJc3RydWN0IGdo ZXNfZWRhY19kaW1tX2ZpbGwgZGltbV9maWxsOwo+PiAtCWludCBpZHg7Cj4+ICsJaW50IGlkeCA9 IC0xOwo+PiAgCj4+IC0JLyogQ2hlY2sgaWYgc2FmZSB0byBlbmFibGUgb24gdGhpcyBzeXN0ZW0g Ki8KPj4gLQlpZHggPSBhY3BpX21hdGNoX3BsYXRmb3JtX2xpc3QocGxhdF9saXN0KTsKPj4gLQlp ZiAoIWZvcmNlX2xvYWQgJiYgaWR4IDwgMCkKPj4gLQkJcmV0dXJuIC1FTk9ERVY7Cj4gCj4gdjQu MTctcmM1IGhhcyAncmV0dXJuIDAnIGhlcmUuIFdvdWxkbid0IHRoaXMgY2hhbmdlIG1lYW5zIG5v IGdoZXMgY2FuIGJlCj4gcmVnaXN0ZXJlZCB1bmxlc3MgZ2hlc19lZGFjIGlzIGFsc28gc3VwcG9y dGVkIGJ5IHRoZSBwbGF0Zm9ybT8KPiBTaG91bGRuJ3QgdGhpcyBiZSAnMCcgZm9yIGEgc2lsZW50 IGZhaWx1cmU/Cj4gCj4gCj4+ICsJaWYgKElTX0VOQUJMRUQoQ09ORklHX1g4NikpIHsKPj4gKwkJ LyogQ2hlY2sgaWYgc2FmZSB0byBlbmFibGUgb24gdGhpcyBzeXN0ZW0gKi8KPj4gKwkJaWR4ID0g YWNwaV9tYXRjaF9wbGF0Zm9ybV9saXN0KHBsYXRfbGlzdCk7Cj4+ICsJCWlmICghZm9yY2VfbG9h ZCAmJiBpZHggPCAwKQo+PiArCQkJcmV0dXJuIC1FTk9ERVY7Cj4+ICsJfSBlbHNlIHsKPj4gKwkJ aWR4ID0gMDsKPj4gKwl9Cj4+ICAKPj4gIAkvKgo+PiAgCSAqIFdlIGhhdmUgb25seSBvbmUgbG9n aWNhbCBtZW1vcnkgY29udHJvbGxlciB0byB3aGljaCBhbGwgRElNTXMgYmVsb25nLgo+IAo+IFRl c3RlZCBvbiBTZWF0dGxlIGFuZCBzb21lIGNyYW5reSBob21lYnJldy1uby1ETUkgZmlybXdhcmU6 Cj4gVGVzdGVkLWJ5OiBKYW1lcyBNb3JzZSA8amFtZXMubW9yc2VAYXJtLmNvbT4KPiAKPiBXaXRo IHRoZSBFTk9ERVYvMCB0aGluZyBhYm92ZToKPiBSZXZpZXdlZC1ieTogSmFtZXMgTW9yc2UgPGph bWVzLm1vcnNlQGFybS5jb20+Cj4gCj4gCj4+PiBJdCBsb29rcyBsaWtlIGV2ZW4gdGhlIG9sZGVz dCBBcm02NCBBQ1BJIHN5c3RlbXMgaGF2ZSBkbWkgdGFibGVzLCBzbyB3ZSBjYW4KPj4+IHByb2Jh Ymx5IHJlcXVpcmUgRE1JIG9yIHRoZSAnZm9yY2UnIGZsYWcuCj4+Cj4+IFdlbGwsIHdpdGggdGhl IGh1bmsgYWJvdmUgaXQgd291bGQgc3RpbGwgZG8gZ2hlc19lZGFjX2NvdW50X2RpbW1zKCkgb24K Pj4gQVJNIGFuZCBpZiBpdCBmYWlscyB0byBmaW5kIHNvbWV0aGluZywgaXQgd2lsbCBzZXQgZmFr ZSwgd2hpY2ggaXMgYSBnb29kCj4+IHNhbml0eS1jaGVjayBhcyBpdCBzY3JlYW1zIGxvdWRseS4g OikKPiAKPiAKPiBUaGFua3MsCj4gCj4gSmFtZXMKPiAKPiAuCj4KLS0tClRvIHVuc3Vic2NyaWJl IGZyb20gdGhpcyBsaXN0OiBzZW5kIHRoZSBsaW5lICJ1bnN1YnNjcmliZSBsaW51eC1lZGFjIiBp bgp0aGUgYm9keSBvZiBhIG1lc3NhZ2UgdG8gbWFqb3Jkb21vQHZnZXIua2VybmVsLm9yZwpNb3Jl IG1ham9yZG9tbyBpbmZvIGF0ICBodHRwOi8vdmdlci5rZXJuZWwub3JnL21ham9yZG9tby1pbmZv Lmh0bWwK From mboxrd@z Thu Jan 1 00:00:00 1970 From: zhengqiang10@huawei.com (Zhengqiang) Date: Fri, 18 May 2018 15:13:40 +0800 Subject: [PATCH] ghes_edac: enable HIP08 platform edac driver In-Reply-To: <8602b133-e0fa-57e2-5159-9d34a1ded85f@arm.com> References: <1526039543-180996-1-git-send-email-zhengqiang10@huawei.com> <20180511121901.GA12705@pd.tnic> <5AF90C70.408@huawei.com> <20180514094709.GC23049@pd.tnic> <20180514164720.GH23049@pd.tnic> <20180516182958.GB17092@pd.tnic> <8602b133-e0fa-57e2-5159-9d34a1ded85f@arm.com> Message-ID: <5AFE7D24.3090408@huawei.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 2018/5/18 2:02, James Morse wrote: > Hi guys, > > Tyler, Zhengqiang, I assume all your shipped platforms with HEST->GHES entries > also have DMI tables. > Sure, Our ARM64 platform have DMI tables. thanks. > > On 16/05/18 19:29, Borislav Petkov wrote: >> On Wed, May 16, 2018 at 02:38:38PM +0100, James Morse wrote: >>> XGene has its own edac driver, but it doesn't probe when booted via ACPI so >>> won't conflict with ghes_edac. >> >> Actually it will. EDAC core can have only one EDAC driver loaded. Don't >> ask me why - it has been that way since forever. > > By won't probe I mean it only works on DT systems: > > | static const struct of_device_id xgene_edac_of_match[] = { > | { .compatible = "apm,xgene-edac" }, > | {}, > | }; > > | .driver = { > | .name = "xgene-edac", > | .of_match_table = xgene_edac_of_match, > | }, > > To work on a system with GHES it would need an 'struct acpi_device_id' to > describe the HID (?) and populate driver's acpi_match_table. > > >> We can change it some >> day but frankly, I don't see reasoning for it. One driver can easily >> manage *all* error sources on a system, I'd say. > > I agree, there is no reason to support two at the same time, if this happens > then there is probably something wrong with the platform (e.g. races with > firmware reading the same hardware registers), so we should make some noise. > > Xgene's edac driver would be a good example of this, it looks like it reads data > from some mmio region, if something else is doing the same we're going to make a > mess. > > >>> So I think we're good to make the whitelist x86 only. >>> Your diff-hunk makes 'idx=-1', so we always get the 'Unfortunately' warning. I'd >>> like to suppress this unless force_load has been used. >> >> Yeah, we should handle that differently for ARM. Toshi added the idx >> thing in >> >> 5deed6b6a479 ("EDAC, ghes: Add platform check") >> >> to dump this when the platform is not whitelisted. So let's do that: >> >> --- >> diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c >> index 863fbf3db29f..473aeec4b1da 100644 >> --- a/drivers/edac/ghes_edac.c >> +++ b/drivers/edac/ghes_edac.c >> @@ -440,12 +440,16 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev) >> struct mem_ctl_info *mci; >> struct edac_mc_layer layers[1]; >> struct ghes_edac_dimm_fill dimm_fill; >> - int idx; >> + int idx = -1; >> >> - /* Check if safe to enable on this system */ >> - idx = acpi_match_platform_list(plat_list); >> - if (!force_load && idx < 0) >> - return -ENODEV; > > v4.17-rc5 has 'return 0' here. Wouldn't this change means no ghes can be > registered unless ghes_edac is also supported by the platform? > Shouldn't this be '0' for a silent failure? > > >> + if (IS_ENABLED(CONFIG_X86)) { >> + /* Check if safe to enable on this system */ >> + idx = acpi_match_platform_list(plat_list); >> + if (!force_load && idx < 0) >> + return -ENODEV; >> + } else { >> + idx = 0; >> + } >> >> /* >> * We have only one logical memory controller to which all DIMMs belong. > > Tested on Seattle and some cranky homebrew-no-DMI firmware: > Tested-by: James Morse > > With the ENODEV/0 thing above: > Reviewed-by: James Morse > > >>> It looks like even the oldest Arm64 ACPI systems have dmi tables, so we can >>> probably require DMI or the 'force' flag. >> >> Well, with the hunk above it would still do ghes_edac_count_dimms() on >> ARM and if it fails to find something, it will set fake, which is a good >> sanity-check as it screams loudly. :) > > > Thanks, > > James > > . >