Re: [PATCH v2 1/2] EDAC/amd64: Check register values from all UMCs

[Yazen Ghannam]

On Wed, Dec 15, 2021 at 07:01:22PM +0100, Borislav Petkov wrote:
> On Wed, Dec 15, 2021 at 03:53:08PM +0000, Yazen Ghannam wrote:
> > -		if ((pvt->umc[0].dimm_cfg | pvt->umc[1].dimm_cfg) & BIT(5))
> > +		u32 umc_cfg = 0, dimm_cfg = 0, i = 0;
> > +
> > +		for_each_umc(i) {
> > +			umc_cfg  |= pvt->umc[i].umc_cfg;
> > +			dimm_cfg |= pvt->umc[i].dimm_cfg;
> > +		}
> > +
> > +		if (dimm_cfg & BIT(5))
> >  			pvt->dram_type = MEM_LRDDR4;
> > -		else if ((pvt->umc[0].dimm_cfg | pvt->umc[1].dimm_cfg) & BIT(4))
> > +		else if (dimm_cfg & BIT(4))
> 
> You're working here under the assumption that bit 4 and 5 will have the
> same value on all those UMCs.
> 
> You're probably going to say that that is how the BIOS is programming
> them so they should be all the same and any other configuration is
> invalid but lemme still ask about it explicitly.
> 
> And if so, this would probably need a comment above it which I can add
> when applying...
> 
> Hmm?
>

No, that's a good question. And actually the assumption is incorrect. It is
allowed to have different DIMM types in a system though all DIMMs on a single
UMC must match.

Do you recommend a follow up patch or should this one be reworked?

Thanks,
Yazen