[PATCH] amd64_edac: Rewrite unganged mode code of f10_early_channel_count

[PATCH] amd64_edac: Rewrite unganged mode code of f10_early_channel_count

[wan wei]

Hi,
     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

thanks :-)

Signed-off-by: Wan Wei<wanwei@mail.dawning.com.cn>

---
 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)
 	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;
+		}
+	}

 	debugf0("MCT channel count: %d\n", channels);

-- 
1.4.4.2

Re: [PATCH] amd64_edac: Rewrite unganged mode code of f10_early_channel_count

[Borislav Petkov]

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

[PATCH] amd64_edac: Rewrite unganged mode code of f10_early_channel_count

[wan wei]

simplify the procedure by checking if there is any dimm  in each channel.
this patch will fix the bugs such as when there is no dimms  under
certain node,  two dimms in the same channel, and only one dimm in
each channel of the node.

Signed-off-by: Wan Wei<wanwei@mail.dawning.com.cn>
---
 drivers/edac/amd64_edac.c |   43 ++++++++++---------------------------------
 1 files changed, 10 insertions(+), 33 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index e2a10bc..acf9cfe 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1200,6 +1200,8 @@ static int f10_early_channel_count(struct amd64_pvt *pvt)
 {
 	int err = 0, channels = 0;
 	u32 dbam;
+	int i, j;
+	int dbams[] = { DBAM0, DBAM1 };

 	err = pci_read_config_dword(pvt->dram_f2_ctl, F10_DCLR_0, &pvt->dclr0);
 	if (err)
@@ -1232,45 +1234,20 @@ static int f10_early_channel_count(struct
amd64_pvt *pvt)
 	 * both controllers since DIMMs can be placed in either one.
 	 */
 	channels = 0;
-	err = pci_read_config_dword(pvt->dram_f2_ctl, DBAM0, &dbam);
-	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);
+	for (i = 0; i < ARRAY_SIZE(dbams); i++) {
+		err = pci_read_config_dword(pvt->dram_f2_ctl, dbams[i], &dbam);
 		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 (channels > 2)
-			channels = 2;
+		for (j = 0; j < 4; j++) {
+			if (DBAM_DIMM(j, dbam) > 0) {
+				channels++;
+				break;
+			}
+		}
 	}

-	/* If we found ALL 0 values, then assume just ONE DIMM-ONE Channel */
-	if (channels == 0)
-		channels = 1;
-
 	debugf0("MCT channel count: %d\n", channels);

 	return channels;
-- 
1.4.4.2

Re: [PATCH] amd64_edac: Rewrite unganged mode code of f10_early_channel_count

[Borislav Petkov]

On Fri, Aug 07, 2009 at 10:39:39PM +0800, wan wei wrote:
> simplify the procedure by checking if there is any dimm  in each channel.
> this patch will fix the bugs such as when there is no dimms  under
> certain node,  two dimms in the same channel, and only one dimm in
> each channel of the node.
> 
> Signed-off-by: Wan Wei<wanwei@mail.dawning.com.cn>
> ---
>  drivers/edac/amd64_edac.c |   43 ++++++++++---------------------------------
>  1 files changed, 10 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index e2a10bc..acf9cfe 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -1200,6 +1200,8 @@ static int f10_early_channel_count(struct amd64_pvt *pvt)
>  {
>  	int err = 0, channels = 0;
>  	u32 dbam;
> +	int i, j;
> +	int dbams[] = { DBAM0, DBAM1 };
> 
>  	err = pci_read_config_dword(pvt->dram_f2_ctl, F10_DCLR_0, &pvt->dclr0);
>  	if (err)
> @@ -1232,45 +1234,20 @@ static int f10_early_channel_count(struct
> amd64_pvt *pvt)

Thanks.

patching file drivers/edac/amd64_edac.c
patch: **** malformed patch at line 28: amd64_pvt *pvt)

Your patch is still garbled, I've applied it by hand this time. For the
future, please send a patch privately to yourself first and try applying
it to check whether your mail client mangles it.

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