[PATCH] ghes_edac: enable HIP08 platform edac driver

James Morse
2018-05-17T18:02:18Z

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 <james.morse@arm.com>

With the ENODEV/0 thing above:
Reviewed-by: James Morse <james.morse@arm.com>

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