sb_edac 32-bit compile fail due to 64-bit divide

PATCH In review Josh Boyer 3 participants 11 messages Nov 3, 2011 – Nov 7, 2011

Subsystems (2)

EDAC-core EDAC-sbridge

lore

Share & tools

lore

Josh Boyer

author

2011-11-03 14:07 UTC #0 lore raw

Hi,

It seems the newly added EDAC driver for SandyBridge won't build on 32-bit x86 because of a 64-bit divide somewhere:

drivers/edac/sb_edac.c: In function 'get_memory_error_data':
drivers/edac/sb_edac.c:861:2: warning: left shift count >= width of type [enabled by default]
<snip>
ERROR: "__udivdi3" [drivers/edac/sb_edac.ko] undefined! make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

You can find the full build log here:

http://koji.fedoraproject.org/koji/getfile?taskID=3482579&name=build.log

Before I go digging into where this is done and what to do about it, I do want to confirm that it is supposed to work on a 32-bit kernel, correct?

josh

Mauro Carvalho Chehab

2011-11-03 14:32 UTC #1 lore raw

Em 03-11-2011 12:07, Josh Boyer escreveu:

Quoted 13 lines

Hi,
It seems the newly added EDAC driver for SandyBridge won't build on 32-bit x86 because of a 64-bit divide somewhere:
drivers/edac/sb_edac.c: In function 'get_memory_error_data':
drivers/edac/sb_edac.c:861:2: warning: left shift count >= width of type [enabled by default]
<snip>
ERROR: "__udivdi3" [drivers/edac/sb_edac.ko] undefined! make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

Quoted 7 lines

You can find the full build log here:
http://koji.fedoraproject.org/koji/getfile?taskID=3482579&name=build.log
Before I go digging into where this is done and what to do about it, I do want to confirm that it is supposed to work on a 32-bit kernel, correct?

Thanks for pointing it!

Well, I didn't test it on a 32-bit kernel. It should work through.

It is probably due to those debug lines:

	debugf0("CH#%d RIR#%d INTL#%d, offset %Lu.%03Lu GB (0x%016Lx), tgt: %d, reg=0x%08x\n",
        	i, j, k,
                tmp_mb / 1000, tmp_mb % 1000,
                ((u64)tmp_mb) << 20L,
                (u32)RIR_RNK_TGT(reg),
                reg);

(same kind of calculus is recurrent at the driver)

Maybe the easiest way would be to write a function that would call do_div() internally, and output a string with something similar to:

	sprintf(msg, "%Lu.%03Lu GB", tmp_mb / 1000, tmp_mb % 1000);

Of course, an interim fix would be to make it depend on CONFIG_64BIT or CONFIG_X86_64.

Regards,
Mauro

Josh Boyer

author

2011-11-03 14:43 UTC #2 lore raw

Quoted 26 lines · Mauro Carvalho Chehab

Em 03-11-2011 12:07, Josh Boyer escreveu:
Quoted 13 lines
Hi,
It seems the newly added EDAC driver for SandyBridge won't build on 32-bit x86 because of a 64-bit divide somewhere:
drivers/edac/sb_edac.c: In function 'get_memory_error_data':
drivers/edac/sb_edac.c:861:2: warning: left shift count >= width of type [enabled by default]
<snip>
ERROR: "__udivdi3" [drivers/edac/sb_edac.ko] undefined! make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2
Quoted 7 lines
You can find the full build log here:
http://koji.fedoraproject.org/koji/getfile?taskID=3482579&name=build.log
Before I go digging into where this is done and what to do about it, I do want to confirm that it is supposed to work on a 32-bit kernel, correct?
Thanks for pointing it!
Well, I didn't test it on a 32-bit kernel. It should work through.

Quoted 8 lines

It is probably due to those debug lines:

	debugf0("CH#%d RIR#%d INTL#%d, offset %Lu.%03Lu GB (0x%016Lx), tgt: %d, reg=0x%08x\n",
        	i, j, k,
                tmp_mb / 1000, tmp_mb % 1000,
                ((u64)tmp_mb) << 20L,
                (u32)RIR_RNK_TGT(reg),
                reg);

(same kind of calculus is recurrent at the driver)

All of that should basically not exist in this build because the debugf* macros depend on CONFIG_EDAC_DEBUG, which isn't set in this build. The problem seems to be somewhere else.

I would guess line 1056:

	addr /= sck_xch;

addr is a u64.

Quoted 6 lines

Maybe the easiest way would be to write a function that would call do_div() internally, and output a string with something similar to:
	sprintf(msg, "%Lu.%03Lu GB", tmp_mb / 1000, tmp_mb % 1000);
Of course, an interim fix would be to make it depend on CONFIG_64BIT or CONFIG_X86_64.

That would work I guess. It's up to you.

Quoted 3 lines

Regards,
Mauro

Luck, Tony

maintainer

2011-11-03 16:31 UTC #3 lore raw

Quoted 3 lines

Quoted 1 line
Of course, an interim fix would be to make it depend on CONFIG_64BIT or CONFIG_X86_64.
That would work I guess. It's up to you.

Typically the test platforms I get have pretty much the minimum sane memory configuration - my Sandy Bridge EP machine came with 32GB of memory (which is theoretically bootable with a 32-bit kernel, but really isn't going to be much fun to use). So I would think that time could be spent on more productive things than making this edac driver work on 32-bit. My vote is for the "depends on 64-bit" solution.

-Tony

Mauro Carvalho Chehab

2011-11-03 17:41 UTC #4 lore raw

Em 03-11-2011 14:31, Luck, Tony escreveu:

Quoted 10 lines

Quoted 3 lines
Quoted 1 line
Of course, an interim fix would be to make it depend on CONFIG_64BIT or CONFIG_X86_64.
That would work I guess. It's up to you.
Typically the test platforms I get have pretty much the minimum sane memory configuration - my Sandy Bridge EP machine came with 32GB of memory (which is theoretically bootable with a 32-bit kernel, but really isn't going to be much fun to use). So I would think that time could be spent on more productive things than making this edac driver work on 32-bit. My vote is for the "depends on 64-bit" solution.

Works for me. It would be a trivial patch. We can later fix it for 32 bits, if someone has any usage for it with 32 bits kernels.

Thanks,
Mauro.

Josh Boyer

author

2011-11-03 17:48 UTC #5 lore raw

Quoted 12 lines · Mauro Carvalho Chehab

Em 03-11-2011 14:31, Luck, Tony escreveu:
Quoted 10 lines
Quoted 3 lines
Quoted 1 line
Of course, an interim fix would be to make it depend on CONFIG_64BIT or CONFIG_X86_64.
That would work I guess. It's up to you.
Typically the test platforms I get have pretty much the minimum sane memory configuration - my Sandy Bridge EP machine came with 32GB of memory (which is theoretically bootable with a 32-bit kernel, but really isn't going to be much fun to use). So I would think that time could be spent on more productive things than making this edac driver work on 32-bit. My vote is for the "depends on 64-bit" solution.

Quoted 2 lines

Works for me. It would be a trivial patch. We can later fix it for 32 bits, if someone has any usage for it with 32 bits kernels.

How's the below?

josh

---

From: Josh Boyer <[email protected]>
Subject: [PATCH] edac: Only build sb_edac on 64-bit

The sb_edac driver is marginally useful on a 32-bit kernel, and currently has 64-bit divide compile errors when building that config. For now, make this build on only for 64-bit kernels.

Signed-off-by Josh Boyer <[email protected]>

---

drivers/edac/Kconfig +1 -1

index 203361e..5948a21 100644
@@ -214,7 +214,7 @@ config EDAC_I7300
 
 config EDAC_SBRIDGE
 	tristate "Intel Sandy-Bridge Integrated MC"
-	depends on EDAC_MM_EDAC && PCI && X86 && X86_MCE_INTEL
+	depends on EDAC_MM_EDAC && PCI && X86_64 && X86_MCE_INTEL
 	depends on EXPERIMENTAL
 	help
 	  Support for error detection and correction the Intel

Mauro Carvalho Chehab

2011-11-03 17:51 UTC #6 lore raw

Em 03-11-2011 15:48, Josh Boyer escreveu:

Quoted 1 line

How's the below?

Ack.

I'll add it on my tree and submit it to Linus for its addition.

Quoted 33 lines

josh
---
From: Josh Boyer <[email protected]>
Subject: [PATCH] edac: Only build sb_edac on 64-bit
The sb_edac driver is marginally useful on a 32-bit kernel, and currently has 64-bit divide compile errors when building that config. For now, make this build on only for 64-bit kernels.
Signed-off-by Josh Boyer <[email protected]>
---
drivers/edac/Kconfig +1 -2
index 203361e..5948a21 100644
@@ -214,7 +214,7 @@ config EDAC_I7300
 
 config EDAC_SBRIDGE
 	tristate "Intel Sandy-Bridge Integrated MC"
-	depends on EDAC_MM_EDAC && PCI && X86 && X86_MCE_INTEL
+	depends on EDAC_MM_EDAC && PCI && X86_64 && X86_MCE_INTEL
 	depends on EXPERIMENTAL
 	help
 	  Support for error detection and correction the Intel
--
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

Mauro Carvalho Chehab

2011-11-03 17:20 UTC #7 lore raw

In reply to Josh Boyer

Em 03-11-2011 12:43, Josh Boyer escreveu:

Quoted 36 lines · Mauro Carvalho Chehab

Quoted 26 lines
Em 03-11-2011 12:07, Josh Boyer escreveu:
Quoted 13 lines
Hi,
It seems the newly added EDAC driver for SandyBridge won't build on 32-bit x86 because of a 64-bit divide somewhere:
drivers/edac/sb_edac.c: In function 'get_memory_error_data':
drivers/edac/sb_edac.c:861:2: warning: left shift count >= width of type [enabled by default]
<snip>
ERROR: "__udivdi3" [drivers/edac/sb_edac.ko] undefined! make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2
Quoted 7 lines
You can find the full build log here:
http://koji.fedoraproject.org/koji/getfile?taskID=3482579&name=build.log
Before I go digging into where this is done and what to do about it, I do want to confirm that it is supposed to work on a 32-bit kernel, correct?
Thanks for pointing it!
Well, I didn't test it on a 32-bit kernel. It should work through.
Quoted 8 lines
It is probably due to those debug lines:
	debugf0("CH#%d RIR#%d INTL#%d, offset %Lu.%03Lu GB (0x%016Lx), tgt: %d, reg=0x%08x\n",
        	i, j, k,
                tmp_mb / 1000, tmp_mb % 1000,
                ((u64)tmp_mb) << 20L,
                (u32)RIR_RNK_TGT(reg),
                reg);
(same kind of calculus is recurrent at the driver)

Quoted 3 lines

All of that should basically not exist in this build because the debugf* macros depend on CONFIG_EDAC_DEBUG, which isn't set in this build. The problem seems to be somewhere else.

I strongly recommend you to enable EDAC_DEBUG if you're building this driver. This driver is experimental, and it may be blaming the wrong memory DIMMs, as Sandy Bridge bios allows setting a large myriad of configurations. Among them, only one were actually tested.

Quoted 6 lines

I would guess line 1056:
	addr /= sck_xch;
addr is a u64.

Ok.

Well, a patch fixing it should be done with debug enabled, to get all cases.

Quoted 8 lines

Quoted 6 lines
Maybe the easiest way would be to write a function that would call do_div() internally, and output a string with something similar to:
	sprintf(msg, "%Lu.%03Lu GB", tmp_mb / 1000, tmp_mb % 1000);
Of course, an interim fix would be to make it depend on CONFIG_64BIT or CONFIG_X86_64.
That would work I guess. It's up to you.

I prefer a patch fixing the issue. Not sure if I'll have time to work in it today, as I'm stuck with the second part of media patches for 3.2.

Quoted 7 lines

Quoted 3 lines
Regards,
Mauro
--
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

Mauro Carvalho Chehab

2011-11-07 21:29 UTC #8 lore raw

PATCH 1/1 Fix sb_edac compilation with 32 bits kernels

In reply to Josh Boyer

Em 03-11-2011 12:07, Josh Boyer escreveu:

Quoted 13 lines

Hi,
It seems the newly added EDAC driver for SandyBridge won't build on 32-bit x86 because of a 64-bit divide somewhere:
drivers/edac/sb_edac.c: In function 'get_memory_error_data':
drivers/edac/sb_edac.c:861:2: warning: left shift count >= width of type [enabled by default]
<snip>
ERROR: "__udivdi3" [drivers/edac/sb_edac.ko] undefined! make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

Quoted 7 lines

You can find the full build log here:
http://koji.fedoraproject.org/koji/getfile?taskID=3482579&name=build.log
Before I go digging into where this is done and what to do about it, I do want to confirm that it is supposed to work on a 32-bit kernel, correct?

Ok, I found some spare time to work on a patch for fixing it. As Tony pointed, it probably doesn't make much sense on running this driver with 32 bits, nor I tested the patch, but, in any case, the fix is trivial.

Regards,
Mauro

From: Mauro Carvalho Chehab <[email protected]>
Date: Mon, 7 Nov 2011 19:26:53 -0200

As reported by Josh Boyer <[email protected]>:

Quoted 7 lines

drivers/edac/sb_edac.c: In function 'get_memory_error_data':
drivers/edac/sb_edac.c:861:2: warning: left shift count >= width of type [enabled by default]
<snip>
ERROR: "__udivdi3" [drivers/edac/sb_edac.ko] undefined! make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

PS.: compile-tested only

Reported-by Josh Boyer <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/edac/Kconfig +1 -1

index 5948a21..203361e 100644
@@ -214,7 +214,7 @@ config EDAC_I7300
 
 config EDAC_SBRIDGE
 	tristate "Intel Sandy-Bridge Integrated MC"
-	depends on EDAC_MM_EDAC && PCI && X86_64 && X86_MCE_INTEL
+	depends on EDAC_MM_EDAC && PCI && X86 && X86_MCE_INTEL
 	depends on EXPERIMENTAL
 	help
 	  Support for error detection and correction the Intel

drivers/edac/sb_edac.c +29 ~~-19~~

132 lines not on this page · raw message

Josh Boyer

author

2011-11-07 21:37 UTC #9 lore raw

Re: [PATCH] Fix sb_edac compilation with 32 bits kernels

Quoted 9 lines · Mauro Carvalho Chehab

drivers/edac/sb_edac.c +1 -0

@@ -670,6 +671,7 @@ static void get_memory_layout(const struct mem_ctl_info *mci)
 	u32 reg;
 	u64 limit, prv = 0;
 	u64 tmp_mb;
+	u32 mb, kb;
 	u32 rir_way;
 
 	/*

drivers/edac/sb_edac.c static void get_memory_layout(const struct mem_ctl_info *mci) · line 689

Quoted 8 lines

drivers/edac/sb_edac.c +3 -2

@@ -682,8 +684,9 @@ static void get_memory_layout(const struct mem_ctl_info *mci)
 	pvt->tolm = GET_TOLM(reg);
 	tmp_mb = (1 + pvt->tolm) >> 20;
 
-	debugf0("TOLM: %Lu.%03Lu GB (0x%016Lx)\n",
-		tmp_mb / 1000, tmp_mb % 1000, (u64)pvt->tolm);
+	mb = div_u64_rem(tmp_mb, 1000, &kb);
+	debugf0("TOLM: %u.%03u GB (0x%016Lx)\n",
+		mb, kb, (u64)pvt->tolm);

So I realize you said that EDAC_DEBUG should (for now) always be enabled when this driver is built, but you just added a bunch of 64-bit divides outside of the debugfX stuff. Doesn't that mean you'll be doing those divides in the non-debug case as well, and then most likely getting an unused varible warning when you turn EDAC_DEBUG off?

Quoted 10 lines

drivers/edac/sb_edac.c +1 -1

@@ -858,7 +868,7 @@ static int get_memory_error_data(struct mem_ctl_info *mci,
 	 * range (e. g. VGA addresses). It is unlikely, however, that the
 	 * memory controller would generate an error on that range.
 	 */
-	if ((addr > (u64) pvt->tolm) && (addr < (1L << 32))) {
+	if ((addr > (u64) pvt->tolm) && (addr < (1LL << 32))) {
 		sprintf(msg, "Error at TOLM area, on addr 0x%08Lx", addr);
 		edac_mc_handle_ce_no_info(mci, msg);
 		return -EINVAL;

drivers/edac/sb_edac.c static int get_memory_error_data(struct mem_ctl_info *mci, · line 1069

Quoted 8 lines

drivers/edac/sb_edac.c +1 -1

@@ -1053,7 +1063,7 @@ static int get_memory_error_data(struct mem_ctl_info *mci,
 	ch_addr = addr & 0x7f;
 	/* Remove socket wayness and remove 6 bits */
 	addr >>= 6;
-	addr /= sck_xch;
+	addr = div_u64(addr, sck_xch);
 #if 0
 	/* Divide by channel way */
 	addr = addr / ch_way;

Those two parts look ok.

josh

Mauro Carvalho Chehab

2011-11-07 21:54 UTC #10 lore raw

Re: [PATCH] Fix sb_edac compilation with 32 bits kernels

Em 07-11-2011 19:37, Josh Boyer escreveu:

Quoted 13 lines · Mauro Carvalho Chehab

Quoted 12 lines

drivers/edac/sb_edac.c +1 -0

@@ -670,6 +671,7 @@ static void get_memory_layout(const struct mem_ctl_info *mci)
 	u32 reg;
 	u64 limit, prv = 0;
 	u64 tmp_mb;
+	u32 mb, kb;
 	u32 rir_way;
 
 	/*

@@ -682,8 +684,9 @@ static void get_memory_layout(const struct mem_ctl_info *mci)
 	pvt->tolm = GET_TOLM(reg);
 	tmp_mb = (1 + pvt->tolm) >> 20;

Quoted 11 lines

drivers/edac/sb_edac.c static void get_memory_layout(const struct mem_ctl_info *mci) · line 689
Quoted 5 lines
drivers/edac/sb_edac.c +3 -2
-	debugf0("TOLM: %Lu.%03Lu GB (0x%016Lx)\n",
-		tmp_mb / 1000, tmp_mb % 1000, (u64)pvt->tolm);
+	mb = div_u64_rem(tmp_mb, 1000, &kb);
+	debugf0("TOLM: %u.%03u GB (0x%016Lx)\n",
+		mb, kb, (u64)pvt->tolm);
So I realize you said that EDAC_DEBUG should (for now) always be enabled when this driver is built, but you just added a bunch of 64-bit divides outside of the debugfX stuff. Doesn't that mean you'll be doing those divides in the non-debug case as well, and then most likely getting an unused varible warning when you turn EDAC_DEBUG off?

You're right. Some additional logic is needed to remove the warnings and unneeded calculus when debug is disabled. I may eventually work on it when I have time. In any case, we gave a fix already for kernel 3.2, and this patch is an alternative for those brave enough to test this driver with a 32 bits PAE kernel.

Quoted 27 lines

Quoted 11 lines
drivers/edac/sb_edac.c +1 -1
@@ -858,7 +868,7 @@ static int get_memory_error_data(struct mem_ctl_info *mci,
 	 * range (e. g. VGA addresses). It is unlikely, however, that the
 	 * memory controller would generate an error on that range.
 	 */
-	if ((addr > (u64) pvt->tolm) && (addr < (1L << 32))) {
+	if ((addr > (u64) pvt->tolm) && (addr < (1LL << 32))) {
 		sprintf(msg, "Error at TOLM area, on addr 0x%08Lx", addr);
 		edac_mc_handle_ce_no_info(mci, msg);
 		return -EINVAL;
drivers/edac/sb_edac.c static int get_memory_error_data(struct mem_ctl_info *mci, · line 1069
Quoted 8 lines
drivers/edac/sb_edac.c +1 -1
@@ -1053,7 +1063,7 @@ static int get_memory_error_data(struct mem_ctl_info *mci,
 	ch_addr = addr & 0x7f;
 	/* Remove socket wayness and remove 6 bits */
 	addr >>= 6;
-	addr /= sck_xch;
+	addr = div_u64(addr, sck_xch);
 #if 0
 	/* Divide by channel way */
 	addr = addr / ch_way;
Those two parts look ok.
josh
--
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