ghes, EDAC: Fix ghes_edac registration

ghes, EDAC: Fix ghes_edac registration

[Borislav Petkov]

On Mon, Apr 30, 2018 at 10:55:02PM +0000, Kani, Toshi wrote:
> I think we need the same 'if (!ghes_pvt)' check in
> ghes_edac_unregister().

https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/commit/?h=edac&id=6c382c46119a17ce9237e2e0a7f0cb5491d5c053

ghes, EDAC: Fix ghes_edac registration

[Toshi Kani]

On Tue, 2018-05-01 at 10:15 +0200, Borislav Petkov wrote:
> On Mon, Apr 30, 2018 at 10:55:02PM +0000, Kani, Toshi wrote:
> > I think we need the same 'if (!ghes_pvt)' check in
> > ghes_edac_unregister().
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/commit/?h=edac&id=6c382c46119a17ce9237e2e0a7f0cb5491d5c053

Ah, thanks!  For both patches:

Reviewed-by: Toshi Kani <toshi.kani@hpe.com>

-Toshi

ghes, EDAC: Fix ghes_edac registration

[Toshi Kani]

On Mon, 2018-04-30 at 12:52 +0200, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> Tony reported seeing
> 
>   "Internal error: Can't find EDAC structure"
> 
> when injecting correctable errors due to the fact that ghes_edac would
> still load even if the whitelist won't hit. Drop the pr_err() in
> ghes_edac_report_mem_error() for now due to the hacky way how ghes_edac
> depends on ghes.c.
> 
> While at it, make ghes_edac_register() return an error if it doesn't hit
> in the whitelist as it is the only sensible thing to do in that
> situation.
> 
> Furthermore, move the call to it to happen last in ghes_probe() so that
> GHES initializing properly does not depend on ghes_edac init at all
> as latter is only reporting errors and not required for GHES's proper
> functioning.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Toshi Kani <toshi.kani@hpe.com>
> Link: https://lkml.kernel.org/r/20180420182015.zao3olss4tvvlxki@agluck-desk
> ---
>  drivers/acpi/apei/ghes.c | 14 ++++++--------
>  drivers/edac/ghes_edac.c |  6 ++----
>  include/acpi/ghes.h      |  2 +-
>  3 files changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 1efefe919555..88103333ee1b 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -1087,10 +1087,6 @@ static int ghes_probe(struct platform_device *ghes_dev)
>  		goto err;
>  	}
>  
> -	rc = ghes_edac_register(ghes, &ghes_dev->dev);
> -	if (rc < 0)
> -		goto err;
> -
>  	switch (generic->notify.type) {
>  	case ACPI_HEST_NOTIFY_POLLED:
>  		timer_setup(&ghes->timer, ghes_poll_func, TIMER_DEFERRABLE);
> @@ -1102,14 +1098,14 @@ static int ghes_probe(struct platform_device *ghes_dev)
>  		if (rc) {
>  			pr_err(GHES_PFX "Failed to map GSI to IRQ for generic hardware error source: %d\n",
>  			       generic->header.source_id);
> -			goto err_edac_unreg;
> +			goto err;
>  		}
>  		rc = request_irq(ghes->irq, ghes_irq_func, IRQF_SHARED,
>  				 "GHES IRQ", ghes);
>  		if (rc) {
>  			pr_err(GHES_PFX "Failed to register IRQ for generic hardware error source: %d\n",
>  			       generic->header.source_id);
> -			goto err_edac_unreg;
> +			goto err;
>  		}
>  		break;
>  
> @@ -1132,14 +1128,16 @@ static int ghes_probe(struct platform_device *ghes_dev)
>  	default:
>  		BUG();
>  	}
> +
>  	platform_set_drvdata(ghes_dev, ghes);
>  
> +	ghes_edac_register(ghes, &ghes_dev->dev);
> +
>  	/* Handle any pending errors right away */
>  	ghes_proc(ghes);
>  
>  	return 0;
> -err_edac_unreg:
> -	ghes_edac_unregister(ghes);
> +
>  err:
>  	if (ghes) {
>  		ghes_fini(ghes);
> diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> index 68b6ee18bea6..7fdbfe871f5c 100644
> --- a/drivers/edac/ghes_edac.c
> +++ b/drivers/edac/ghes_edac.c
> @@ -183,10 +183,8 @@ void ghes_edac_report_mem_error(struct ghes *ghes, int sev,
>  	char *p;
>  	u8 grain_bits;
>  
> -	if (!pvt) {
> -		pr_err("Internal error: Can't find EDAC structure\n");
> +	if (!pvt)
>  		return;
> -	}
>  

I think we need the same 'if (!ghes_pvt)' check in
ghes_edac_unregister().

Thanks,
-Toshi

ghes, EDAC: Fix ghes_edac registration

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Tony reported seeing

  "Internal error: Can't find EDAC structure"

when injecting correctable errors due to the fact that ghes_edac would
still load even if the whitelist won't hit. Drop the pr_err() in
ghes_edac_report_mem_error() for now due to the hacky way how ghes_edac
depends on ghes.c.

While at it, make ghes_edac_register() return an error if it doesn't hit
in the whitelist as it is the only sensible thing to do in that
situation.

Furthermore, move the call to it to happen last in ghes_probe() so that
GHES initializing properly does not depend on ghes_edac init at all
as latter is only reporting errors and not required for GHES's proper
functioning.

Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Toshi Kani <toshi.kani@hpe.com>
Link: https://lkml.kernel.org/r/20180420182015.zao3olss4tvvlxki@agluck-desk
---
 drivers/acpi/apei/ghes.c | 14 ++++++--------
 drivers/edac/ghes_edac.c |  6 ++----
 include/acpi/ghes.h      |  2 +-
 3 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 1efefe919555..88103333ee1b 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -1087,10 +1087,6 @@ static int ghes_probe(struct platform_device *ghes_dev)
 		goto err;
 	}
 
-	rc = ghes_edac_register(ghes, &ghes_dev->dev);
-	if (rc < 0)
-		goto err;
-
 	switch (generic->notify.type) {
 	case ACPI_HEST_NOTIFY_POLLED:
 		timer_setup(&ghes->timer, ghes_poll_func, TIMER_DEFERRABLE);
@@ -1102,14 +1098,14 @@ static int ghes_probe(struct platform_device *ghes_dev)
 		if (rc) {
 			pr_err(GHES_PFX "Failed to map GSI to IRQ for generic hardware error source: %d\n",
 			       generic->header.source_id);
-			goto err_edac_unreg;
+			goto err;
 		}
 		rc = request_irq(ghes->irq, ghes_irq_func, IRQF_SHARED,
 				 "GHES IRQ", ghes);
 		if (rc) {
 			pr_err(GHES_PFX "Failed to register IRQ for generic hardware error source: %d\n",
 			       generic->header.source_id);
-			goto err_edac_unreg;
+			goto err;
 		}
 		break;
 
@@ -1132,14 +1128,16 @@ static int ghes_probe(struct platform_device *ghes_dev)
 	default:
 		BUG();
 	}
+
 	platform_set_drvdata(ghes_dev, ghes);
 
+	ghes_edac_register(ghes, &ghes_dev->dev);
+
 	/* Handle any pending errors right away */
 	ghes_proc(ghes);
 
 	return 0;
-err_edac_unreg:
-	ghes_edac_unregister(ghes);
+
 err:
 	if (ghes) {
 		ghes_fini(ghes);
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 68b6ee18bea6..7fdbfe871f5c 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -183,10 +183,8 @@ void ghes_edac_report_mem_error(struct ghes *ghes, int sev,
 	char *p;
 	u8 grain_bits;
 
-	if (!pvt) {
-		pr_err("Internal error: Can't find EDAC structure\n");
+	if (!pvt)
 		return;
-	}
 
 	/*
 	 * We can do the locking below because GHES defers error processing
@@ -439,7 +437,7 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
 	/* Check if safe to enable on this system */
 	idx = acpi_match_platform_list(plat_list);
 	if (!force_load && idx < 0)
-		return 0;
+		return -ENODEV;
 
 	/*
 	 * We have only one logical memory controller to which all DIMMs belong.
diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
index 8feb0c866ee0..4fc92289f7f8 100644
--- a/include/acpi/ghes.h
+++ b/include/acpi/ghes.h
@@ -70,7 +70,7 @@ static inline void ghes_edac_report_mem_error(struct ghes *ghes, int sev,
 
 static inline int ghes_edac_register(struct ghes *ghes, struct device *dev)
 {
-	return 0;
+	return -ENODEV;
 }
 
 static inline void ghes_edac_unregister(struct ghes *ghes)