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: James Morse Message-Id: <8602b133-e0fa-57e2-5159-9d34a1ded85f@arm.com> Date: Thu, 17 May 2018 19:02:18 +0100 To: Borislav Petkov , Zhengqiang , 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: SGkgZ3V5cywKClR5bGVyLCBaaGVuZ3FpYW5nLCBJIGFzc3VtZSBhbGwgeW91ciBzaGlwcGVkIHBs YXRmb3JtcyB3aXRoIEhFU1QtPkdIRVMgZW50cmllcwphbHNvIGhhdmUgRE1JIHRhYmxlcy4KCgpP biAxNi8wNS8xOCAxOToyOSwgQm9yaXNsYXYgUGV0a292IHdyb3RlOgo+IE9uIFdlZCwgTWF5IDE2 LCAyMDE4IGF0IDAyOjM4OjM4UE0gKzAxMDAsIEphbWVzIE1vcnNlIHdyb3RlOgo+PiBYR2VuZSBo YXMgaXRzIG93biBlZGFjIGRyaXZlciwgYnV0IGl0IGRvZXNuJ3QgcHJvYmUgd2hlbiBib290ZWQg dmlhIEFDUEkgc28KPj4gd29uJ3QgY29uZmxpY3Qgd2l0aCBnaGVzX2VkYWMuCj4gCj4gQWN0dWFs bHkgaXQgd2lsbC4gRURBQyBjb3JlIGNhbiBoYXZlIG9ubHkgb25lIEVEQUMgZHJpdmVyIGxvYWRl ZC4gRG9uJ3QKPiBhc2sgbWUgd2h5IC0gaXQgaGFzIGJlZW4gdGhhdCB3YXkgc2luY2UgZm9yZXZl ci4KCkJ5IHdvbid0IHByb2JlIEkgbWVhbiBpdCBvbmx5IHdvcmtzIG9uIERUIHN5c3RlbXM6Cgp8 IHN0YXRpYyBjb25zdCBzdHJ1Y3Qgb2ZfZGV2aWNlX2lkIHhnZW5lX2VkYWNfb2ZfbWF0Y2hbXSA9 IHsKfAl7IC5jb21wYXRpYmxlID0gImFwbSx4Z2VuZS1lZGFjIiB9LAp8CXt9LAp8IH07Cgp8CS5k cml2ZXIgPSB7CnwJCS5uYW1lID0gInhnZW5lLWVkYWMiLAp8CQkub2ZfbWF0Y2hfdGFibGUgPSB4 Z2VuZV9lZGFjX29mX21hdGNoLAp8CX0sCgpUbyB3b3JrIG9uIGEgc3lzdGVtIHdpdGggR0hFUyBp dCB3b3VsZCBuZWVkIGFuICdzdHJ1Y3QgYWNwaV9kZXZpY2VfaWQnIHRvCmRlc2NyaWJlIHRoZSBI SUQgKD8pIGFuZCBwb3B1bGF0ZSBkcml2ZXIncyBhY3BpX21hdGNoX3RhYmxlLgoKCj4gV2UgY2Fu IGNoYW5nZSBpdCBzb21lCj4gZGF5IGJ1dCBmcmFua2x5LCBJIGRvbid0IHNlZSByZWFzb25pbmcg Zm9yIGl0LiBPbmUgZHJpdmVyIGNhbiBlYXNpbHkKPiBtYW5hZ2UgKmFsbCogZXJyb3Igc291cmNl cyBvbiBhIHN5c3RlbSwgSSdkIHNheS4KCkkgYWdyZWUsIHRoZXJlIGlzIG5vIHJlYXNvbiB0byBz dXBwb3J0IHR3byBhdCB0aGUgc2FtZSB0aW1lLCBpZiB0aGlzIGhhcHBlbnMKdGhlbiB0aGVyZSBp cyBwcm9iYWJseSBzb21ldGhpbmcgd3Jvbmcgd2l0aCB0aGUgcGxhdGZvcm0gKGUuZy4gcmFjZXMg d2l0aApmaXJtd2FyZSByZWFkaW5nIHRoZSBzYW1lIGhhcmR3YXJlIHJlZ2lzdGVycyksIHNvIHdl IHNob3VsZCBtYWtlIHNvbWUgbm9pc2UuCgpYZ2VuZSdzIGVkYWMgZHJpdmVyIHdvdWxkIGJlIGEg Z29vZCBleGFtcGxlIG9mIHRoaXMsIGl0IGxvb2tzIGxpa2UgaXQgcmVhZHMgZGF0YQpmcm9tIHNv bWUgbW1pbyByZWdpb24sIGlmIHNvbWV0aGluZyBlbHNlIGlzIGRvaW5nIHRoZSBzYW1lIHdlJ3Jl IGdvaW5nIHRvIG1ha2UgYQptZXNzLgoKCj4+IFNvIEkgdGhpbmsgd2UncmUgZ29vZCB0byBtYWtl IHRoZSB3aGl0ZWxpc3QgeDg2IG9ubHkuCj4+IFlvdXIgZGlmZi1odW5rIG1ha2VzICdpZHg9LTEn LCBzbyB3ZSBhbHdheXMgZ2V0IHRoZSAnVW5mb3J0dW5hdGVseScgd2FybmluZy4gSSdkCj4+IGxp a2UgdG8gc3VwcHJlc3MgdGhpcyB1bmxlc3MgZm9yY2VfbG9hZCBoYXMgYmVlbiB1c2VkLgo+IAo+ IFllYWgsIHdlIHNob3VsZCBoYW5kbGUgdGhhdCBkaWZmZXJlbnRseSBmb3IgQVJNLiBUb3NoaSBh ZGRlZCB0aGUgaWR4Cj4gdGhpbmcgaW4KPiAKPiAgIDVkZWVkNmI2YTQ3OSAoIkVEQUMsIGdoZXM6 IEFkZCBwbGF0Zm9ybSBjaGVjayIpCj4gCj4gdG8gZHVtcCB0aGlzIHdoZW4gdGhlIHBsYXRmb3Jt IGlzIG5vdCB3aGl0ZWxpc3RlZC4gU28gbGV0J3MgZG8gdGhhdDoKPiAKPiAtLS0KPiBkaWZmIC0t Z2l0IGEvZHJpdmVycy9lZGFjL2doZXNfZWRhYy5jIGIvZHJpdmVycy9lZGFjL2doZXNfZWRhYy5j Cj4gaW5kZXggODYzZmJmM2RiMjlmLi40NzNhZWVjNGIxZGEgMTAwNjQ0Cj4gLS0tIGEvZHJpdmVy cy9lZGFjL2doZXNfZWRhYy5jCj4gKysrIGIvZHJpdmVycy9lZGFjL2doZXNfZWRhYy5jCj4gQEAg LTQ0MCwxMiArNDQwLDE2IEBAIGludCBnaGVzX2VkYWNfcmVnaXN0ZXIoc3RydWN0IGdoZXMgKmdo ZXMsIHN0cnVjdCBkZXZpY2UgKmRldikKPiAgCXN0cnVjdCBtZW1fY3RsX2luZm8gKm1jaTsKPiAg CXN0cnVjdCBlZGFjX21jX2xheWVyIGxheWVyc1sxXTsKPiAgCXN0cnVjdCBnaGVzX2VkYWNfZGlt bV9maWxsIGRpbW1fZmlsbDsKPiAtCWludCBpZHg7Cj4gKwlpbnQgaWR4ID0gLTE7Cj4gIAo+IC0J LyogQ2hlY2sgaWYgc2FmZSB0byBlbmFibGUgb24gdGhpcyBzeXN0ZW0gKi8KPiAtCWlkeCA9IGFj cGlfbWF0Y2hfcGxhdGZvcm1fbGlzdChwbGF0X2xpc3QpOwo+IC0JaWYgKCFmb3JjZV9sb2FkICYm IGlkeCA8IDApCj4gLQkJcmV0dXJuIC1FTk9ERVY7Cgp2NC4xNy1yYzUgaGFzICdyZXR1cm4gMCcg aGVyZS4gV291bGRuJ3QgdGhpcyBjaGFuZ2UgbWVhbnMgbm8gZ2hlcyBjYW4gYmUKcmVnaXN0ZXJl ZCB1bmxlc3MgZ2hlc19lZGFjIGlzIGFsc28gc3VwcG9ydGVkIGJ5IHRoZSBwbGF0Zm9ybT8KU2hv dWxkbid0IHRoaXMgYmUgJzAnIGZvciBhIHNpbGVudCBmYWlsdXJlPwoKCj4gKwlpZiAoSVNfRU5B QkxFRChDT05GSUdfWDg2KSkgewo+ICsJCS8qIENoZWNrIGlmIHNhZmUgdG8gZW5hYmxlIG9uIHRo aXMgc3lzdGVtICovCj4gKwkJaWR4ID0gYWNwaV9tYXRjaF9wbGF0Zm9ybV9saXN0KHBsYXRfbGlz dCk7Cj4gKwkJaWYgKCFmb3JjZV9sb2FkICYmIGlkeCA8IDApCj4gKwkJCXJldHVybiAtRU5PREVW Owo+ICsJfSBlbHNlIHsKPiArCQlpZHggPSAwOwo+ICsJfQo+ICAKPiAgCS8qCj4gIAkgKiBXZSBo YXZlIG9ubHkgb25lIGxvZ2ljYWwgbWVtb3J5IGNvbnRyb2xsZXIgdG8gd2hpY2ggYWxsIERJTU1z IGJlbG9uZy4KClRlc3RlZCBvbiBTZWF0dGxlIGFuZCBzb21lIGNyYW5reSBob21lYnJldy1uby1E TUkgZmlybXdhcmU6ClRlc3RlZC1ieTogSmFtZXMgTW9yc2UgPGphbWVzLm1vcnNlQGFybS5jb20+ CgpXaXRoIHRoZSBFTk9ERVYvMCB0aGluZyBhYm92ZToKUmV2aWV3ZWQtYnk6IEphbWVzIE1vcnNl IDxqYW1lcy5tb3JzZUBhcm0uY29tPgoKCj4+IEl0IGxvb2tzIGxpa2UgZXZlbiB0aGUgb2xkZXN0 IEFybTY0IEFDUEkgc3lzdGVtcyBoYXZlIGRtaSB0YWJsZXMsIHNvIHdlIGNhbgo+PiBwcm9iYWJs eSByZXF1aXJlIERNSSBvciB0aGUgJ2ZvcmNlJyBmbGFnLgo+IAo+IFdlbGwsIHdpdGggdGhlIGh1 bmsgYWJvdmUgaXQgd291bGQgc3RpbGwgZG8gZ2hlc19lZGFjX2NvdW50X2RpbW1zKCkgb24KPiBB Uk0gYW5kIGlmIGl0IGZhaWxzIHRvIGZpbmQgc29tZXRoaW5nLCBpdCB3aWxsIHNldCBmYWtlLCB3 aGljaCBpcyBhIGdvb2QKPiBzYW5pdHktY2hlY2sgYXMgaXQgc2NyZWFtcyBsb3VkbHkuIDopCgoK VGhhbmtzLAoKSmFtZXMKLS0tClRvIHVuc3Vic2NyaWJlIGZyb20gdGhpcyBsaXN0OiBzZW5kIHRo ZSBsaW5lICJ1bnN1YnNjcmliZSBsaW51eC1lZGFjIiBpbgp0aGUgYm9keSBvZiBhIG1lc3NhZ2Ug dG8gbWFqb3Jkb21vQHZnZXIua2VybmVsLm9yZwpNb3JlIG1ham9yZG9tbyBpbmZvIGF0ICBodHRw Oi8vdmdlci5rZXJuZWwub3JnL21ham9yZG9tby1pbmZvLmh0bWwK From mboxrd@z Thu Jan 1 00:00:00 1970 From: james.morse@arm.com (James Morse) Date: Thu, 17 May 2018 19:02:18 +0100 Subject: [PATCH] ghes_edac: enable HIP08 platform edac driver In-Reply-To: <20180516182958.GB17092@pd.tnic> 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> Message-ID: <8602b133-e0fa-57e2-5159-9d34a1ded85f@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi guys, Tyler, Zhengqiang, I assume all your shipped platforms with HEST->GHES entries also have DMI tables. 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