Re: [PATCH EDAC 03/13] ghes: add the needed hooks for EDAC error report

[Mauro Carvalho Chehab <mchehab@redhat.com>]

Em Fri, 22 Feb 2013 08:45:11 +0800
Huang Ying <ying.huang@intel.com> escreveu:

> On Thu, 2013-02-21 at 09:04 -0300, Mauro Carvalho Chehab wrote:
> > Em Thu, 21 Feb 2013 09:26:07 +0800
> > Huang Ying <ying.huang@intel.com> escreveu:
> > 

> > There is also an advantage on taking this approach: this patch can have just
> > the ghes changes, and will still compile fine, as CONFIG_EDAC_GHES is not
> > defined yet. So, the patch becomes is simpler and easier to review.
> 
> Thanks for your explanation.  I agree to keep this in header file.
> 
> My original suggestion is that it may be better to move this from ghes.h
> to some place like "ghes_edac.h", because these functions are
> implemented in "ghes_edac.c" instead of ghes.c.
> 

Ah! Well, I considered a separate header when writing this patch, but adding 
another header for just 3 functions where the parameters are more related to
ghes than with ghes_edac seemed overkill to me.

IMO, a single comment at the header pointing to ghes_edac.c would improve it.

> >  	switch (generic->notify.type) {
> >  	case ACPI_HEST_NOTIFY_POLLED:
> >  		ghes->timer.function = ghes_poll_func;
> > @@ -911,13 +920,13 @@ static int ghes_probe(struct platform_device *ghes_dev)
> >  		if (acpi_gsi_to_irq(generic->notify.vector, &ghes->irq)) {
> >  			pr_err(GHES_PFX "Failed to map GSI to IRQ for generic hardware error source: %d\n",
> >  			       generic->header.source_id);
> > -			goto err;
> > +			goto err2;
> >  		}
> >  		if (request_irq(ghes->irq, ghes_irq_func,
> >  				0, "GHES IRQ", ghes)) {
> >  			pr_err(GHES_PFX "Failed to register IRQ for generic hardware error source: %d\n",
> >  			       generic->header.source_id);
> > -			goto err;
> > +			goto err2;
> >  		}
> >  		break;
> >  	case ACPI_HEST_NOTIFY_SCI:
> > @@ -943,6 +952,8 @@ static int ghes_probe(struct platform_device *ghes_dev)
> >  	platform_set_drvdata(ghes_dev, ghes);
> >  
> >  	return 0;
> > +err2:
> 
> Suggest to rename it to err_edac_unreg or something like that.  Just a
> suggestion.

Done.

I'm folding the enclosed patch on it.

Thanks for review!

Regards,
Mauro

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 19092dc..d668a8a 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -920,13 +920,13 @@ static int ghes_probe(struct platform_device *ghes_dev)
 		if (acpi_gsi_to_irq(generic->notify.vector, &ghes->irq)) {
 			pr_err(GHES_PFX "Failed to map GSI to IRQ for generic hardware error source: %d\n",
 			       generic->header.source_id);
-			goto err2;
+			goto err_edac_unreg;
 		}
 		if (request_irq(ghes->irq, ghes_irq_func,
 				0, "GHES IRQ", ghes)) {
 			pr_err(GHES_PFX "Failed to register IRQ for generic hardware error source: %d\n",
 			       generic->header.source_id);
-			goto err2;
+			goto err_edac_unreg;
 		}
 		break;
 	case ACPI_HEST_NOTIFY_SCI:
@@ -952,7 +952,7 @@ static int ghes_probe(struct platform_device *ghes_dev)
 	platform_set_drvdata(ghes_dev, ghes);
 
 	return 0;
-err2:
+err_edac_unreg:
 	ghes_edac_unregister(ghes);
 err:
 	if (ghes) {
diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
index 9015ec2..d11d952 100644
--- a/include/acpi/ghes.h
+++ b/include/acpi/ghes.h
@@ -45,6 +45,8 @@ enum {
 	GHES_SEV_PANIC = 0x3,
 };
 
+/* From drivers/edac/ghes_acpi.h */
+
 #ifdef CONFIG_EDAC_GHES
 void ghes_edac_report_mem_error(struct ghes *ghes, int sev,
 				struct cper_sec_mem_err *mem_err);



-- 

Cheers,
Mauro