diff mbox

EDAC, ghes: Make platform-based whitelisting x86-only

Message ID 20180518112028.GD17285@pd.tnic (mailing list archive)
State New, archived
Headers show

Commit Message

Borislav Petkov May 18, 2018, 11:20 a.m. UTC
From: Borislav Petkov <bp@suse.de>

ARM machines all have DMI tables so if they request hw error reporting
through GHES, then the driver should be able to detect DIMMs and report
errors successfully (famous last words :)).

Make the platform-based list x86-specific so that ghes_edac can load on
ARM.

Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: James Morse <james.morse@arm.com>
Tested-by: James Morse <james.morse@arm.com>
Cc: Qiang Zheng <zhengqiang10@huawei.com>
Link: https://lkml.kernel.org/r/1526039543-180996-1-git-send-email-zhengqiang10@huawei.com
---
 drivers/edac/ghes_edac.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Comments

Zhengqiang May 21, 2018, 9:39 a.m. UTC | #1
Thanks, it works for me.

On 2018/5/18 19:20, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> ARM machines all have DMI tables so if they request hw error reporting
> through GHES, then the driver should be able to detect DIMMs and report
> errors successfully (famous last words :)).
> 
> Make the platform-based list x86-specific so that ghes_edac can load on
> ARM.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Reviewed-by: James Morse <james.morse@arm.com>
> Tested-by: James Morse <james.morse@arm.com>
> Cc: Qiang Zheng <zhengqiang10@huawei.com>
> Link: https://lkml.kernel.org/r/1526039543-180996-1-git-send-email-zhengqiang10@huawei.com

Tested-by: Qiang Zheng <zhengqiang10@huawei.com>

> ---
>  drivers/edac/ghes_edac.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> 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;
> +	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.
>
Tyler Baicar May 21, 2018, 1:48 p.m. UTC | #2
On 5/18/2018 7:20 AM, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
>
> ARM machines all have DMI tables so if they request hw error reporting
> through GHES, then the driver should be able to detect DIMMs and report
> errors successfully (famous last words :)).
>
> Make the platform-based list x86-specific so that ghes_edac can load on
> ARM.
>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Reviewed-by: James Morse <james.morse@arm.com>
> Tested-by: James Morse <james.morse@arm.com>
> Cc: Qiang Zheng <zhengqiang10@huawei.com>
Tested-by: Tyler Baicar <tbaicar@codeaurora.org>

Was it intentional to make idx=0 so that the pr_info later in this function no 
longer hits?

I don't see an issue with not printing out the long BIOS statement, but the 
number of DIMM sockets print could still be useful.

Thanks,
Tyler
Borislav Petkov May 21, 2018, 5:15 p.m. UTC | #3
On Mon, May 21, 2018 at 09:48:23AM -0400, Tyler Baicar wrote:
> I don't see an issue with not printing out the long BIOS statement, but the
> number of DIMM sockets print could still be useful.

Well, if you wanna dump the number of DIMMs - then maybe that line
should issue unconditionally. However, "DIMM sockets" is silly - it
should simply say:

	"%d DIMMs detected"

or so.
Tyler Baicar May 21, 2018, 8:34 p.m. UTC | #4
On 5/21/2018 1:15 PM, Borislav Petkov wrote:
> On Mon, May 21, 2018 at 09:48:23AM -0400, Tyler Baicar wrote:
>> I don't see an issue with not printing out the long BIOS statement, but the
>> number of DIMM sockets print could still be useful.
> Well, if you wanna dump the number of DIMMs - then maybe that line
> should issue unconditionally. However, "DIMM sockets" is silly - it
> should simply say:
>
> 	"%d DIMMs detected"
>
> or so.
Agreed. Sounds good to me.
Borislav Petkov May 21, 2018, 8:44 p.m. UTC | #5
On Mon, May 21, 2018 at 04:34:11PM -0400, Tyler Baicar wrote:
> Agreed. Sounds good to me.

Meaning you're sending me a patch soon or ...?

:-D
Tyler Baicar May 22, 2018, 2:10 a.m. UTC | #6
On 5/21/2018 4:44 PM, Borislav Petkov wrote:
> On Mon, May 21, 2018 at 04:34:11PM -0400, Tyler Baicar wrote:
>> Agreed. Sounds good to me.
> Meaning you're sending me a patch soon or ...?
>
> :-D
>
Yes, I'll put a patch together and send it out tomorrow.
diff mbox

Patch

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