Re: [PATCH 3/3] EDAC, ghes: Make it a proper module

[Kani, Toshimitsu]

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