[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-edac
Subject:    Re: [PATCH] EDAC, ghes: Do not register instance if platform is not whitelisted
From:       "Kani, Toshi" <toshi.kani () hpe ! com>
Date:       2018-04-27 20:54:17
Message-ID: 1524862406.2693.538.camel () hpe ! com
[Download RAW message or body]

On Fri, 2018-04-27 at 13:43 +0200, Borislav Petkov wrote:
> On Mon, Apr 23, 2018 at 08:35:31PM +0000, Kani, Toshi wrote:
> > On Mon, 2018-04-23 at 22:28 +0200, Borislav Petkov wrote:
> > > On Mon, Apr 23, 2018 at 08:13:23PM +0000, Kani, Toshi wrote:
> > > > Umm..., I do not think this change is correct.  This causes an error to
> > > > ghes_probe(), which disables GHES itself.  ghes and ghes_edac are
> > > > separate modules.  That is, ghes_edac works on top of ghes as an
> > > > optional module.
> > > > 
> > > > I think the right fix is to simply remove the error message.  This will
> > > > make it consistent with the case of CONFIG_EDAC_GHES disabled.  Please
> > > > see include/acpi/ghes.h.
> > > 
> > > I guess. Although an even right-er fix would be to not call
> > > ghes_edac_report_mem_error() at all, if there's no ghes_edac module
> > > registered.
> > > 
> > > That would need more dissecting though....
> > 
> > That's cleaner, but will lead to a module ordering issue...  For now,
> > I'd simply fix ghes_edac_report_mem_error() to handle the !pvt case as a
> > normal case.
> 
> Well, the !pvt case is not a normal case. The only justification for
> removing the pr_err() because of the hack how ghes_edac is grafted onto
> ghes.c
> 
> Actually, what I'm thinking is the below along with taking Sughosh's
> patch here:
> 
> https://marc.info/?l=linux-edac&m=152473783708615
> 
> Also, ghes_edac_register() needs to return an error when it does not hit
> the whitelist - there's no other way to see it.
> 
> Which means, ghes.c can register ghes_edac as the last thing it does and
> the outcome of that registration should not have any influence on GHES
> itself because ghes_edac does only reporting.

I haven't had chance to test your change, but it looks good to me.
Please also change ghes_edac_register() in include/acpi/ghes.h to return
-ENODEV as well.

Thanks,
-Toshi
--
To unsubscribe from this list: send the line "unsubscribe linux-edac" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]