Messages in this thread | | | From | "Kani, Toshimitsu" <> | Subject | Re: [PATCH 3/3] EDAC, ghes: Make it a proper module | Date | Wed, 2 Aug 2017 22:41:14 +0000 |
| |
On Wed, 2017-08-02 at 05:18 +0200, Borislav Petkov wrote: > On Wed, Aug 02, 2017 at 12:19:29AM +0000, Kani, Toshimitsu wrote: > > 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. > > Nonsense. There are a lot of examples where driver probing depends on > outside modalities like built-in quirks and such. > > > 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. > > Except that GHES wants to report into the EDAC infrastructure so it > better has a wrapper for it. > > One of the directions I explored when looking at this is to stick > ghes_edac functionality into ghes.c or so and make it completely > independent from EDAC. Would've been much cleaner.
Agreed. I think the current model aimed at this direction while it was needed to depend on EDAC.
> > 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. > > They're independent because before GHES we needed to load one driver > per system. Until the bolted-on thing came. And it is bolted on > because the already overwhelmed firmware decided to do error > reporting too. > > So the only real reason why I'm fine with keeping the current > situation is the whitelist. Because then, we can at least control > what loads and what not. > > But then we need: > > 1. A clean mechanism for the platform drivers to query whether > another agent is loaded (ghes_edac) and not do any probing then. > > 2. ghes_edac needs to drop that multiple probing thing as its > dmi_walk(ghes_edac_count_dimms, &num_dimm) already probes *all* DIMMs > on the system so no need to do that multiple times.
Sounds good. I will keep the current model and address the above points.
Thanks, -Toshi
| |