RE: [PATCH v2 2/7] EDAC/amd64: Recognize DRAM device type with EDAC_CTL_CAP

[Ghannam, Yazen]

> -----Original Message-----
> From: linux-edac-owner@xxxxxxxxxxxxxxx <linux-edac-owner@xxxxxxxxxxxxxxx> On Behalf Of Borislav Petkov
> Sent: Friday, August 2, 2019 2:42 AM
> To: Ghannam, Yazen <Yazen.Ghannam@xxxxxxx>
> Cc: linux-edac@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 2/7] EDAC/amd64: Recognize DRAM device type with EDAC_CTL_CAP
> 
> On Tue, Jul 09, 2019 at 09:56:55PM +0000, Ghannam, Yazen wrote:
> > From: Yazen Ghannam <yazen.ghannam@xxxxxxx>
> >
> > AMD Family 17h systems support x4 and x16 DRAM devices. However, the
> > device type is not checked when setting EDAC_CTL_CAP.
> >
> > Set the appropriate EDAC_CTL_CAP flag based on the device type.
> >
> > Fixes: 2d09d8f301f5 ("EDAC, amd64: Determine EDAC MC capabilities on Fam17h")
> 
> This is better: a patch which fixes a previous patch and is simple,
> small and clear. That you can tag with Fixes: just fine.
> 
> > Signed-off-by: Yazen Ghannam <yazen.ghannam@xxxxxxx>
> > ---
> > Link:
> > https://lkml.kernel.org/r/20190531234501.32826-4-Yazen.Ghannam@xxxxxxx
> >
> > v1->v2:
> > * No change.
> >
> >  drivers/edac/amd64_edac.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> > index dd60cf5a3d96..125d6e2a828e 100644
> > --- a/drivers/edac/amd64_edac.c
> > +++ b/drivers/edac/amd64_edac.c
> > @@ -3150,12 +3150,15 @@ static bool ecc_enabled(struct pci_dev *F3, u16 nid)
> >  static inline void
> >  f17h_determine_edac_ctl_cap(struct mem_ctl_info *mci, struct amd64_pvt *pvt)
> >  {
> > -	u8 i, ecc_en = 1, cpk_en = 1;
> > +	u8 i, ecc_en = 1, cpk_en = 1, dev_x4 = 1, dev_x16 = 1;
> >
> >  	for_each_umc(i) {
> >  		if (pvt->umc[i].sdp_ctrl & UMC_SDP_INIT) {
> >  			ecc_en &= !!(pvt->umc[i].umc_cap_hi & UMC_ECC_ENABLED);
> >  			cpk_en &= !!(pvt->umc[i].umc_cap_hi & UMC_ECC_CHIPKILL_CAP);
> > +
> > +			dev_x4 &= !!(pvt->umc[i].dimm_cfg & BIT(6));
> > +			dev_x16 &= !!(pvt->umc[i].dimm_cfg & BIT(7));
> 
> Are those bits mutually exclusive?
> 
> I.e., so that you can do:
> 
> 	if (dev_x4)
> 		mci->edac_ctl_cap |= EDAC_FLAG_S4ECD4ED;
> 	else
> 		mci->edac_ctl_cap |= EDAC_FLAG_S16ECD16ED;
> 
> ?
> 

I don't think so. I believe they can both be zero. I'll verify and make the change if they are mutually exclusive.

Thanks,
Yazen