RE: [PATCH 1/3] EDAC/amd64: Print ECC enabled/disabled for nodes with enabled MCs

[Ghannam, Yazen]

> -----Original Message-----
> From: linux-edac-owner@xxxxxxxxxxxxxxx <linux-edac-
> owner@xxxxxxxxxxxxxxx> On Behalf Of Borislav Petkov
> Sent: Wednesday, March 28, 2018 9:00 AM
> To: Ghannam, Yazen <Yazen.Ghannam@xxxxxxx>
> Cc: linux-edac@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 1/3] EDAC/amd64: Print ECC enabled/disabled for nodes
> with enabled MCs
> 
> On Wed, Mar 21, 2018 at 02:13:33PM -0500, Yazen Ghannam wrote:
> > From: Yazen Ghannam <yazen.ghannam@xxxxxxx>
> >
> > It's possible that a system can be used without any DRAM populated on
> > one or more physical Dies on multi-die systems. Firmware will not
> > enable DRAM ECC on Dies without DRAM. Users will then see a message
> > about DRAM ECC disabled on those nodes without DRAM. However, DRAM
> ECC
> > may, in fact, be enabled on the other Dies that have DRAM.
> >
> > Only print ECC enabled/disabled information for nodes that have at least
> > one enabled memory channel.
> 
> So if the only reason for this is make the error messages more precise,
> then let's not make it uglier than it is.
> 
> The right way to do it would be to push those checks down to
> debug_display_dimm_sizes* which looks at the CS rows and the chip select
> enable bits and there to differentiate between
> 
> * memory controller doesn't have DIMMs
> 
> and
> 
> * memory controller has DIMMs but ECC is disabled in the BIOS
> 
> and then print the respective informative error message. But not with a
> yet another boolean which kinda takes care of F17h only and leaves the
> old families as they were.
> 

In either of those cases we won't get to debug_display_dimm_sizes*
because we won't initialize the instance.

We have a message for "memory controller doesn't have DIMMs".
edac_dbg(0, "Node %d: No enabled UMCs.\n", nid);

We can change the wording of this to make it more clear. Also, we can
make this a pr_info and return from here.

So something like this:

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 329cb96f886f..6e211ed6d419 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -3061,10 +3061,12 @@ static bool ecc_enabled(struct pci_dev *F3, u16 nid)
                }
 
                /* Check whether at least one UMC is enabled: */
-               if (umc_en_mask)
+               if (umc_en_mask) {
                        ecc_en = umc_en_mask == ecc_en_mask;
-               else
-                       edac_dbg(0, "Node %d: No enabled UMCs.\n", nid);
+               } else {
+                       amd64_info("Node %d: No DIMMs populated on memory controller.\n", nid);
+                       return false;
+               }
 
                /* Assume UMC MCA banks are enabled. */
                nb_mce_en = true;

This would work for Fam17h. For older systems I think we can look at
D18F2x94_dct[1:0][DisDramInterface]

Or maybe we have a separate function to check for enabled memory controllers
before we check for ECC?

Thanks,
Yazen