Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1700369 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 3/3] EDAC, ghes: Make it a proper module "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2017-07-31 22:20 +0200
    Re: [PATCH 3/3] EDAC, ghes: Make it a proper module Borislav Petkov <bp@alien8.de> - 2017-08-01 11:50 +0200
      Re: [PATCH 3/3] EDAC, ghes: Make it a proper module "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2017-08-02 02:30 +0200
        Re: [PATCH 3/3] EDAC, ghes: Make it a proper module Borislav Petkov <bp@alien8.de> - 2017-08-02 05:20 +0200
          Re: [PATCH 3/3] EDAC, ghes: Make it a proper module "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2017-08-03 00:50 +0200

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

On Sat, 2017-07-29 at 08:47 +0200, Borislav Petkov wrote:
> On Fri, Jul 28, 2017 at 06:50:56PM +0000, Kani, Toshimitsu wrote:
> > This simply sets NULL to pvt, and does not initialize ghes_pvt.
> 
> Yeah, I guess we need this ontop:

Yes, this fix looks good.

> >  As Mauro pointed out, some type of GHES check needs to be in place
> > before making this change.
> 
> Your whitelist I guess.

We still need the two mechanisms provided by the existing code below. 
The whitelist simply compliments 1.

1. GHES-presence check.  (We can add APEI OSC bit check as well.)
2. Module priority.  ghes_edac has higher priority for registration.

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.

Thanks,
-Toshi

[toc] | [next] | [standalone]

#1700795

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.

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).

There we can set a "use_ghes" or so bool which the x86 platform drivers
would query through accessor functions and determine whether to load or
not.

In any case, something like that. I'm always open for better
suggestions, though.

I've pushed a rebased branch here:

https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=ghes

feel free to base your changes ontop.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[toc] | [prev] | [next] | [standalone]

#1701603

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

[toc] | [prev] | [next] | [standalone]

#1701658

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.

> 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.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[toc] | [prev] | [next] | [standalone]

#1702537

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web