> -----Original Message----- > From: [email protected] <[email protected]> On > Behalf Of Borislav Petkov > Sent: Friday, August 2, 2019 2:42 AM > To: Ghannam, Yazen <[email protected]> > Cc: [email protected]; [email protected] > 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 <[email protected]> > > > > 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 <[email protected]> > > --- > > Link: > > https://lkml.kernel.org/r/[email protected] > > > > 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
