linux-kernel - Re: [PATCH V4] edac, amd64_edac: Modify usage of amd64_read_dct_pci

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140915150823.GH3340@nazgul.tnic>
Date:	Mon, 15 Sep 2014 17:08:23 +0200
From:	Borislav Petkov <bp@...en8.de>
To:	Aravind Gopalakrishnan <aravind.gopalakrishnan@....com>
Cc:	dougthompson@...ssion.com, m.chehab@...sung.com,
	linux-edac@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V4] edac, amd64_edac: Modify usage of
 amd64_read_dct_pci_cfg()

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.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/