linux-kernel - Re: [PATCH] amd64_edac: Rewrite unganged mode code of f10_early_channel

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: <20090807122916.GC29900@aftab>
Date:	Fri, 7 Aug 2009 14:29:16 +0200
From:	Borislav Petkov <borislav.petkov@....com>
To:	wan wei <onewayforever@...il.com>
CC:	dougthompson@...ssion.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] amd64_edac: Rewrite unganged mode code of
 f10_early_channel_count

Hi,

On Fri, Aug 07, 2009 at 01:36:30PM +0800, wan wei wrote:
>      I rechecked the f10_early_channel_count function, and found more
> bugs in unganged mode. This patch will fix  bugs under following
> conditions,
>     a. only one DIMM in each channelof the node,
>     b. two DIMMs are populated in the same channel of the node,
>     c. there is no DIMM in a node(other than node 0),
>     a and b  mean both dbam should be checked in any condition,  and c
> means even channels==0 is ok.
> 
> the patch has been tested and works well in  this  4 ways machine:
>    node 0:  2 DIMMS on each channel
>    node11: no DIMMS
>    node2:  2 DIMM on the chanel 0
>    node3:  one DIMM on each channel

let me preface this by saying the I generally like the idea, this
function is rather clumsy (and buggy for that matter) and cleaning it up
is very welcome. However...

Please, write a proper commit message when sending patches. You need to
exactly and succintly explain what (and not how) are you changing and
why. See <Documentation/SubmittingPatches>.

> ---
>  drivers/edac/amd64_edac.c |   46 ++++++++++++++------------------------------
>  1 files changed, 15 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index e2a10bc..7a328fa 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -1200,6 +1200,7 @@ static int f10_early_channel_count(struct amd64_pvt *pvt)
>  {
>  	int err = 0, channels = 0;
>  	u32 dbam;
> +	int i;
> 
>  	err = pci_read_config_dword(pvt->dram_f2_ctl, F10_DCLR_0, &pvt->dclr0);
>  	if (err)
> @@ -1236,40 +1237,23 @@ static int f10_early_channel_count(struct
> amd64_pvt *pvt)

your patch is garbled here and cannot be applied as such. See section 7)
in <Documentation/SubmittingPatches> for more info on how to avoid that.

>  	if (err)
>  		goto err_reg;
> 
> -	if (DBAM_DIMM(0, dbam) > 0)
> -		channels++;
> -	if (DBAM_DIMM(1, dbam) > 0)
> -		channels++;
> -	if (DBAM_DIMM(2, dbam) > 0)
> -		channels++;
> -	if (DBAM_DIMM(3, dbam) > 0)
> -		channels++;
> -
> -	/* If more than 2 DIMMs are present, then we have 2 channels */
> -	if (channels > 2)
> -		channels = 2;
> -	else if (channels == 0) {
> -		/* No DIMMs on DCT0, so look at DCT1 */
> -		err = pci_read_config_dword(pvt->dram_f2_ctl, DBAM1, &dbam);
> -		if (err)
> -			goto err_reg;
> -
> -		if (DBAM_DIMM(0, dbam) > 0)
> +	for (i = 0; i < 4; i++) {
> +		if (DBAM_DIMM(i, dbam) > 0) {
>  			channels++;
> -		if (DBAM_DIMM(1, dbam) > 0)
> -			channels++;
> -		if (DBAM_DIMM(2, dbam) > 0)
> -			channels++;
> -		if (DBAM_DIMM(3, dbam) > 0)
> -			channels++;
> -
> -		if (channels > 2)
> -			channels = 2;
> +			break;
> +		}
>  	}
> 
> -	/* If we found ALL 0 values, then assume just ONE DIMM-ONE Channel */
> -	if (channels == 0)
> -		channels = 1;
> +	err = pci_read_config_dword(pvt->dram_f2_ctl, DBAM1, &dbam);
> +	if (err)
> +		goto err_reg;
> +
> +	for (i = 0; i < 4; i++) {
> +		if (DBAM_DIMM(i, dbam) > 0) {
> +			channels++;
> +			break;
> +		}
> +	}

combine the two loops together like so:

        for (j = 0; j < ARRAY_SIZE(regs_dbam); j++) {
                err = pci_read_config_dword(pvt->dram_f2_ctl, regs_dbam[j],
                                            &dbam);

                for (i = 0; i < 4; i++) {
                        if (DBAM_DIMM(i, dbam) > 0) {
                                channels++;
                                break;
                        }
                }
        }

where regs_dbam[] is:

	int regs_dbam[] = { DBAM0, DBAM1 };


This way this function is finally starting to look ok.

Please redo your patch and resubmit.

Thanks.

-- 
Regards/Gruss,
Boris.

Operating | Advanced Micro Devices GmbH
  System  | Karl-Hammerschmidt-Str. 34, 85609 Dornach b. München, Germany
 Research | Geschäftsführer: Thomas M. McCoy, Giuliano Meroni
  Center  | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
  (OSRC)  | Registergericht München, HRB Nr. 43632

--
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/