Messages in this thread | | | From | "Kani, Toshimitsu" <> | Subject | Re: [PATCH 3/3] EDAC, ghes: Make it a proper module | Date | Wed, 2 Aug 2017 00:19:29 +0000 |
| |
On Tue, 2017-08-01 at 11:46 +0200, Borislav Petkov wrote: > On Mon, Jul 31, 2017 at 08:19:32PM +0000, Kani, Toshimitsu wrote: > > I'd prefer to add the whitelist check to ghes_edac first. This > > makes the existing code to work. We can then work on refactoring > > changes like this on top of it without breaking the functionality. > > Yes, but we want only the whitelist - not the FF testing because, as > we said, BIOS is notoriously buggy so we're going to load ghes_edac > only on known-good platforms.
This GHES-probe itself is appropriate and should remain. Since not all GHES firmware can be trusted, we will add the white-list as an additional condition to complement this check.
> Which brings the question about the priority. > > And I *think* the easiest would be if the whitelist were in the core > edac.ko module, perhaps in edac_module.c (even though it doesn't > really matter, technically).
I agree that adding the white-list into the core edac module is the easiest when we make the change on top of yours. Thinking further on this, though, I now think that keeping the current implementation is more reasonable with the reasons below.
1. Device-probing-logic should belong to a driver, and should remain private to a driver. When we add the white-list, it should be added to ghes_edac.
2. ghes_edac is an extension to the ghes driver as they both are specific to ghes. ghes_edac is merely ghes driver's edac error- reporting wrapper than an independent edac driver. It looks OK to let ghes_edac get registered as part of ghes_probe() and leave it as an unconventional edac driver.
3. EDAC does not have its managed probe-chain. All edac drivers are called from module_init list. They independently probe the hardware and get unloaded when not needed. The core edac is simply a set of library to them. I think it's good to keep them independent, and not to introduce a new central mechanism for a special case like ghes_edac.
Thanks, -Toshi
| |