amd64_edac: fix size decoding error on K8

Use a lookup table to calculate the size of a chip select. The old method of calculating size return erroneous values for some memory configurations.

Quoted 3 lines · Niklas Söderlund

Use a lookup table to calculate the size of a chip select. The old method of calculating size return erroneous values for some memory configurations.

Can you elaborate more on those erroneous values, please?

Thanks.

The decoding table from the documentation:

cs_mode size (mb)
0 32
1 64
2 128
3 128
4 256
5 512
6 256
7 512
8 1024
9 1024
10 2048

The original code:

if (cs_mode == 3 || cs_mode == 8)

	return 32 << (cs_mode - 1);
else
	return 32 << cs_mode;

The code tries to compensate for the irregularity in the decoding table but fails for cs_mode greater or equal to 4. It fails because:

- It do not consider the cumulating effect of reoccurring sizes for all values of cs_mode. It only tries for the special cases and it only succeeds in one.

- It do not consider the out of order values of cs_mode values 4-7:
256, 512, 256, 512.

+               diff = cs_mode / 3 + (unsigned)(cs_mode > 5); +
+               return 32 << (cs_mode - diff);

Quoted 3 lines · Borislav Petkov

+               diff = cs_mode / 3 + (unsigned)(cs_mode > 5); +
+               return 32 << (cs_mode - diff);

Code is correct - but looks like an entry for an obfuscated C competition.

Perhaps it deserves a comment!

Fix DRAM chip select sizes calculation for K8, revisions D and E.

Reported-by Niklas Söderlund<[email protected]

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Borislav Petkov<[email protected]>

---

@Niklas: would you please verify this patch fixes your issue?

Thanks.

  drivers/edac/amd64_edac.c |   32 ++++++++++++++++++++++++++++----
  1 files changed, 28 insertions(+), 4 deletions(-)

drivers/edac/amd64_edac.c +21 -4

index c9eee6d..6a83d49 100644
@@ -1132,12 +1132,36 @@ static int k8_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
  		return ddr2_cs_size(cs_mode, dclr&  WIDTH_128);
  	}
  	else if (pvt->ext_model>= K8_REV_D) {
+		unsigned diff;
  		WARN_ON(cs_mode>  10);
 
-		if (cs_mode == 3 || cs_mode == 8)
-			return 32<<  (cs_mode - 1);
-		else
-			return 32<<  cs_mode;
+		/*
+		 * the below calculation, besides trying to win an obfuscated C
+		 * contest, maps cs_mode values to DIMM chip select sizes. The
+		 * mappings are:
+		 *
+		 * cs_mode	CS size (mb)
+		 * =======	============
+		 * 0		32
+		 * 1		64
+		 * 2		128
+		 * 3		128
+		 * 4		256
+		 * 5		512
+		 * 6		256
+		 * 7		512
+		 * 8		1024
+		 * 9		1024
+		 * 10		2048
+		 *
+		 * Basically, it calculates a value with which to shift the

Hi Niklas,

Quoted 7 lines · Niklas Söderlund

The decoding table from the documentation:

<snip>

- It do not consider the out of order values of cs_mode values 4-7:
256, 512, 256, 512.

Agreed, thanks for reporting this. However, I removed the static tables at the time because I didn't want to carry unnecessary weight around for no reason (tables were static back then). And I know, they're small but dead weight like that tends to accumulate.

Your approach is ok with creating the lookup table on the stack but how about the following instead - it is much smaller and it is only code, no need for stack memory at all. It should give you the proper mapping, IINM.

--

drivers/edac/amd64_edac.c +4 -5

index c9eee6d33e9a..3702f20d784e 100644
@@ -1132,12 +1132,12 @@ static int k8_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
 		return ddr2_cs_size(cs_mode, dclr & WIDTH_128);
 	}
 	else if (pvt->ext_model >= K8_REV_D) {
+		unsigned diff;
 		WARN_ON(cs_mode > 10);
 
-		if (cs_mode == 3 || cs_mode == 8)
-			return 32 << (cs_mode - 1);
-		else
-			return 32 << cs_mode;
+		diff = cs_mode / 3 + (unsigned)(cs_mode > 5);
+
+		return 32 << (cs_mode - diff);
 	}
 	else {
 		WARN_ON(cs_mode > 6);
--

Thanks.

Signature · 12 lines

Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551
--
To unsubscribe from this list: send the line "unsubscribe linux-edac" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html