Re: [PATCH V4] edac, amd64_edac: Modify usage of amd64_read_dct_pci_cfg()

[Borislav Petkov <bp@alien8.de>]

On Mon, Sep 15, 2014 at 09:55:22AM -0500, Aravind Gopalakrishnan wrote:
> >>+		 * Note: If ganging is enabled, barring the regs
> >>+		 * F2x[1,0]98 and F2x[1,0]9C; reads reads to F2x1xx
> >Why aren't we dealing with those registers here then?
> 
> According to BKDG, these registers are used to control DRAM electrical
> parameters..
> Afaict, we have never had to use these regs here..

... until someone decides to access them and copies the code here, which
would be wrong. Oh well, I guess someone will hopefully read the note
too, while copying...

> >>  	amd64_read_pci_cfg(pvt->F3, F10_ONLINE_SPARE, &pvt->online_spare);
> >>-	amd64_read_dct_pci_cfg(pvt, DCLR0, &pvt->dclr0);
> >>-	amd64_read_dct_pci_cfg(pvt, DCHR0, &pvt->dchr0);
> >>-
> >>-	if (!dct_ganging_enabled(pvt)) {
> >>-		amd64_read_dct_pci_cfg(pvt, DCLR1, &pvt->dclr1);
> >>-		amd64_read_dct_pci_cfg(pvt, DCHR1, &pvt->dchr1);
> >>-	}
> >>+	amd64_read_dct_pci_cfg(pvt, 0, DCLR0, &pvt->dclr0);
> >>+	amd64_read_dct_pci_cfg(pvt, 0, DCHR0, &pvt->dchr0);
> >>  	pvt->ecc_sym_sz = 4;
> >>  	if (pvt->fam >= 0x10) {
> >>+		amd64_read_dct_pci_cfg(pvt, 1, DCLR0, &pvt->dclr1);
> >>+		amd64_read_dct_pci_cfg(pvt, 1, DCHR0, &pvt->dchr1);
> >This doesn't look equivalent - above we're checking whether we're ganged
> >now you're doing it for >= F10h. Why?
> >
> >Because only F10h supports ganging?
> 
> That's right.
> If ganging is enabled (which is a condition we check for in
> amd64_read_dct_pci_cfg();
> Then we return 0.

Right, but you're reading them now even if you don't have to. So why are
you even changing this? What's wrong with doing:

	if (!dct_ganging_enabled(pvt)) {
		amd64_read_dct_pci_cfg(pvt, 1, DCLR0, &pvt->dclr1);
		amd64_read_dct_pci_cfg(pvt, 1, DCHR0, &pvt->dchr1);
	}

which is much clearer than doing the family check.

-- 
Regards/Gruss,
    Boris.
--