[PATCH 0/1] mspec driver: compile error

[PATCH 0/1] mspec driver: compile error

[Fernando Luis Vázquez Cao]

Hi Jes,

After selecting CONFIG_MSPEC as a module I stumbled onto the compile
error below.

WARNING: "bte_copy" [drivers/char/mspec.ko] undefined!
WARNING: "physical_node_map" [drivers/char/mspec.ko] undefined!
WARNING: "uncached_free_page" [drivers/char/mspec.ko] undefined!
WARNING: "per_cpu____sn_hub_info" [drivers/char/mspec.ko] undefined!
WARNING: "uncached_alloc_page" [drivers/char/mspec.ko] undefined!
make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

The problem is that the Kconfig dependencies for MSPEC are a bit too
loose. The mspec driver needs bte_copy (a sn-specific function) as well
as some functions of the uncached page allocator.

I solved the issue by making the dependencies explicit in
drivers/char/Kconfig:
--- Current Kconfig entry
config MSPEC
	tristate "Memory special operations driver"
	depends on IA64
	help
	  If you have an ia64 and you want to enable memory special
	  operations support (formerly known as fetchop), say Y here,
	  otherwise say N.
---
--- Proposed Kconfig entry
config MSPEC
        tristate "Memory special operations driver"
        depends on IA64 && (IA64_GENERIC || IA64_SGI_SN2)
        select IA64_UNCACHED_ALLOCATOR
        help
          If you have an ia64 and you want to enable memory special
          operations support (formerly known as fetchop), say Y here,
          otherwise say N.
---

I'll be replying to this message with a patch that implements this. I
would appreciate your review and comments.

Regards,

Fernando

Re: [PATCH 0/1] mspec driver: compile error

[Jes Sorensen]

[-- Attachment #1: Type: text/plain, Size: 927 bytes --]

Fernando Luis Vázquez Cao wrote:
> Hi Jes,
> 
> After selecting CONFIG_MSPEC as a module I stumbled onto the compile
> error below.
[snip]
> I'll be replying to this message with a patch that implements this. I
> would appreciate your review and comments.

Hi Fernando,

As I also mentioned in my reply to Peter Chubb, this is the wrong fix
for this problem as the driver should operate in cached and uncached
mode on non SN2 systems.

Here is the correct fix. I have test it on SN2 and made sure it builds
for DIG, but I can't easily test it. However I see no reason why it
should cause problems.

This is ending up as an attachment since it ended up in the IMAP only
mailbox. Somehow I am willing to bet Thunderbug will do something stupid
to it, like base64 encode it.

Cheers,
Jes

PS: When you post fixes for an IA64 specific driver like this, please
    always remember to CC the ia64 list and the ia64 maintainer on it!

[-- Attachment #2: mspec-dig-build.diff --]
[-- Type: text/plain, Size: 2599 bytes --]

Fix MSPEC driver to build for non SN2 enabled configs as the driver
should work in cached and uncached modes (no fetchop) on these systems.
In addition make MSPEC select IA64_UNCACHED_ALLOCATOR, which is required
for it.

Signed-off-by: Jes Sorensen <jes@sgi.com>

---
 drivers/char/Kconfig        |    1 +
 drivers/char/mspec.c        |    8 +++++++-
 include/asm-ia64/sn/addrs.h |    6 +++++-
 3 files changed, 13 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/char/Kconfig
===================================================================
--- linux-2.6.orig/drivers/char/Kconfig
+++ linux-2.6/drivers/char/Kconfig
@@ -412,6 +412,7 @@ config SGI_MBCS
 config MSPEC
 	tristate "Memory special operations driver"
 	depends on IA64
+	select IA64_UNCACHED_ALLOCATOR
 	help
 	  If you have an ia64 and you want to enable memory special
 	  operations support (formerly known as fetchop), say Y here,
Index: linux-2.6/drivers/char/mspec.c
===================================================================
--- linux-2.6.orig/drivers/char/mspec.c
+++ linux-2.6/drivers/char/mspec.c
@@ -72,7 +72,11 @@ enum {
 	MSPEC_UNCACHED
 };
 
+#ifdef CONFIG_SGI_SN
 static int is_sn2;
+#else
+#define is_sn2		0
+#endif
 
 /*
  * One of these structures is allocated when an mspec region is mmaped. The
@@ -211,7 +215,7 @@ mspec_nopfn(struct vm_area_struct *vma, 
 	if (vdata->type == MSPEC_FETCHOP)
 		paddr = TO_AMO(maddr);
 	else
-		paddr = __pa(TO_CAC(maddr));
+		paddr = maddr & ~__IA64_UNCACHED_OFFSET;
 
 	pfn = paddr >> PAGE_SHIFT;
 
@@ -335,6 +339,7 @@ mspec_init(void)
 	 * The fetchop device only works on SN2 hardware, uncached and cached
 	 * memory drivers should both be valid on all ia64 hardware
 	 */
+#ifdef CONFIG_SGI_SN
 	if (ia64_platform_is("sn2")) {
 		is_sn2 = 1;
 		if (is_shub2()) {
@@ -363,6 +368,7 @@ mspec_init(void)
 			goto free_scratch_pages;
 		}
 	}
+#endif
 	ret = misc_register(&cached_miscdev);
 	if (ret) {
 		printk(KERN_ERR "%s: failed to register device %i\n",
Index: linux-2.6/include/asm-ia64/sn/addrs.h
===================================================================
--- linux-2.6.orig/include/asm-ia64/sn/addrs.h
+++ linux-2.6/include/asm-ia64/sn/addrs.h
@@ -136,9 +136,13 @@
  */
 #define TO_PHYS(x)		(TO_PHYS_MASK & (x))
 #define TO_CAC(x)		(CAC_BASE     | TO_PHYS(x))
+#ifdef CONFIG_SGI_SN
 #define TO_AMO(x)		(AMO_BASE     | TO_PHYS(x))
 #define TO_GET(x)		(GET_BASE     | TO_PHYS(x))
-
+#else
+#define TO_AMO(x)		({ BUG(); x; })
+#define TO_GET(x)		({ BUG(); x; })
+#endif
 
 /*
  * Covert from processor physical address to II/TIO physical address:

Re: [PATCH 0/1] mspec driver: compile error

[Andrew Morton]

On Tue, 07 Nov 2006 11:31:54 +0100
Jes Sorensen <jes@sgi.com> wrote:

> Fix MSPEC driver to build for non SN2 enabled configs as the driver
> should work in cached and uncached modes (no fetchop) on these systems.
> In addition make MSPEC select IA64_UNCACHED_ALLOCATOR, which is required
> for it.

i386 `make allmodconfig' says:

drivers/char/Kconfig:425:warning: 'select' used by config symbol 'MSPEC' refer to undefined symbol 'IA64_UNCACHED_ALLOCATOR'

Re: [PATCH 0/1] mspec driver: compile error

[Fernando Luis Vázquez Cao]

On Tue, 2006-11-07 at 13:35 -0800, Andrew Morton wrote:
> On Tue, 07 Nov 2006 11:31:54 +0100
> Jes Sorensen <jes@sgi.com> wrote:
> 
> > Fix MSPEC driver to build for non SN2 enabled configs as the driver
> > should work in cached and uncached modes (no fetchop) on these systems.
> > In addition make MSPEC select IA64_UNCACHED_ALLOCATOR, which is required
> > for it.
> 
> i386 `make allmodconfig' says:
> 
> drivers/char/Kconfig:425:warning: 'select' used by config symbol 'MSPEC' refer to undefined symbol 'IA64_UNCACHED_ALLOCATOR'
The problem occurs because i386 (as expected) does not define
IA64_UNCACHED_ALLOCATOR. I thought that making the select expression
depend on IA64 as shown below might silence allmodconfig:

  select IA64_UNCACHED_ALLOCATOR if IA64

But my guess was wrong and the same warning appeared. It seems that "if"
expressions do not prevent allmodconfig from checking the symbol
indicated by the select the "if" is conditioning. By the way, is this
the expected behaviour? If so, we need to get rid of the reverse
dependency, modify the "depends on" line accordingly, and make
IA64_UNCACHED_ALLOCATOR selectable. I may be missing the whole point so
please correct if I am wrong.

Regards,

Fernando

---

The mspec driver's Kconfig entry has a reverse dependency on IA64-specific code, which causes "allmodconfig" to yell on non-Itanium architectures.

Signed-off-by: Fernando Vazquez <fernando@intellilink.co.jp>
---

diff -urNp linux-2.6.19-rc4-mm2-orig/arch/ia64/Kconfig linux-2.6.19-rc4-mm2/arch/ia64/Kconfig
--- linux-2.6.19-rc4-mm2-orig/arch/ia64/Kconfig	2006-11-08 17:51:19.000000000 +0900
+++ linux-2.6.19-rc4-mm2/arch/ia64/Kconfig	2006-11-08 18:11:14.000000000 +0900
@@ -74,10 +74,6 @@ config SCHED_NO_NO_OMIT_FRAME_POINTER
 	bool
 	default y
 
-config IA64_UNCACHED_ALLOCATOR
-	bool
-	select GENERIC_ALLOCATOR
-
 config AUDIT_ARCH
 	bool
 	default y
@@ -226,6 +222,13 @@ config IOSAPIC
 	depends on !IA64_HP_SIM
 	default y
 
+config IA64_UNCACHED_ALLOCATOR
+	bool "Uncached allocator"
+	select GENERIC_ALLOCATOR
+	help
+	  A simple uncached page allocator using the generic allocator.
+	  It is needed to compile the special memory driver (mspec).
+
 config IA64_SGI_SN_XP
 	tristate "Support communication between SGI SSIs"
 	depends on IA64_GENERIC || IA64_SGI_SN2
diff -urNp linux-2.6.19-rc4-mm2-orig/drivers/char/Kconfig linux-2.6.19-rc4-mm2/drivers/char/Kconfig
--- linux-2.6.19-rc4-mm2-orig/drivers/char/Kconfig	2006-11-08 17:51:36.000000000 +0900
+++ linux-2.6.19-rc4-mm2/drivers/char/Kconfig	2006-11-08 16:31:55.000000000 +0900
@@ -436,8 +436,7 @@ config SGI_MBCS
 
 config MSPEC
 	tristate "Memory special operations driver"
-	depends on IA64
-	select IA64_UNCACHED_ALLOCATOR
+	depends on IA64_UNCACHED_ALLOCATOR
 	help
 	  If you have an ia64 and you want to enable memory special
 	  operations support (formerly known as fetchop), say Y here,

Re: [PATCH 0/1] mspec driver: compile error

[Jes Sorensen]

Fernando Luis Vázquez Cao wrote:
> On Tue, 2006-11-07 at 13:35 -0800, Andrew Morton wrote:
> The problem occurs because i386 (as expected) does not define
> IA64_UNCACHED_ALLOCATOR. I thought that making the select expression
> depend on IA64 as shown below might silence allmodconfig:
> 
>   select IA64_UNCACHED_ALLOCATOR if IA64
> 
> But my guess was wrong and the same warning appeared. It seems that "if"
> expressions do not prevent allmodconfig from checking the symbol
> indicated by the select the "if" is conditioning. By the way, is this
> the expected behaviour? If so, we need to get rid of the reverse
> dependency, modify the "depends on" line accordingly, and make
> IA64_UNCACHED_ALLOCATOR selectable. I may be missing the whole point so
> please correct if I am wrong.

This patch is a bad solution as it requires people to manually select
the uncached allocator. It should be enabled automatically by MSPEC,
not the other way round.

Given that MSPEC is clearly marked as depending on IA64, it seems bogus
for i386 allmodconfig to barf over it and the problem should be fixed
there instead IMHO.

Cheers,
Jes

Re: [PATCH 0/1] mspec driver: compile error

[Fernando Luis Vázquez Cao]

On Wed, 2006-11-08 at 10:42 +0100, Jes Sorensen wrote:
> Fernando Luis Vázquez Cao wrote:
> > On Tue, 2006-11-07 at 13:35 -0800, Andrew Morton wrote:
> > The problem occurs because i386 (as expected) does not define
> > IA64_UNCACHED_ALLOCATOR. I thought that making the select expression
> > depend on IA64 as shown below might silence allmodconfig:
> > 
> >   select IA64_UNCACHED_ALLOCATOR if IA64
> > 
> > But my guess was wrong and the same warning appeared. It seems that "if"
> > expressions do not prevent allmodconfig from checking the symbol
> > indicated by the select the "if" is conditioning. By the way, is this
> > the expected behaviour? If so, we need to get rid of the reverse
> > dependency, modify the "depends on" line accordingly, and make
> > IA64_UNCACHED_ALLOCATOR selectable. I may be missing the whole point so
> > please correct if I am wrong.
> 
> This patch is a bad solution as it requires people to manually select
> the uncached allocator. It should be enabled automatically by MSPEC,
> not the other way round.
> 
> Given that MSPEC is clearly marked as depending on IA64, it seems bogus
> for i386 allmodconfig to barf over it and the problem should be fixed
> there instead IMHO.
Agreed. That is why I asked if that was allmodconfig's expected
behaviour. Andrew?

Cheers,

Fernando

Re: [PATCH 0/1] mspec driver: compile error

[Andrew Morton]

On Wed, 08 Nov 2006 18:45:30 +0900
Fernando Luis Vázquez Cao <fernando@oss.ntt.co.jp> wrote:

> On Wed, 2006-11-08 at 10:42 +0100, Jes Sorensen wrote:
> > Fernando Luis Vázquez Cao wrote:
> > > On Tue, 2006-11-07 at 13:35 -0800, Andrew Morton wrote:
> > > The problem occurs because i386 (as expected) does not define
> > > IA64_UNCACHED_ALLOCATOR. I thought that making the select expression
> > > depend on IA64 as shown below might silence allmodconfig:
> > > 
> > >   select IA64_UNCACHED_ALLOCATOR if IA64
> > > 
> > > But my guess was wrong and the same warning appeared. It seems that "if"
> > > expressions do not prevent allmodconfig from checking the symbol
> > > indicated by the select the "if" is conditioning. By the way, is this
> > > the expected behaviour? If so, we need to get rid of the reverse
> > > dependency, modify the "depends on" line accordingly, and make
> > > IA64_UNCACHED_ALLOCATOR selectable. I may be missing the whole point so
> > > please correct if I am wrong.
> > 
> > This patch is a bad solution as it requires people to manually select
> > the uncached allocator. It should be enabled automatically by MSPEC,
> > not the other way round.
> > 
> > Given that MSPEC is clearly marked as depending on IA64, it seems bogus
> > for i386 allmodconfig to barf over it and the problem should be fixed
> > there instead IMHO.
> Agreed. That is why I asked if that was allmodconfig's expected
> behaviour. Andrew?
> 

kconfig's `select' isn't very smart.  This is one of the reasons why one
should avoid using it.

Re: [PATCH 0/1] mspec driver: compile error

[Fernando Luis Vázquez Cao]

On Wed, 2006-11-08 at 01:56 -0800, Andrew Morton wrote:
> On Wed, 08 Nov 2006 18:45:30 +0900
> Fernando Luis Vázquez Cao <fernando@oss.ntt.co.jp> wrote:
> 
> > On Wed, 2006-11-08 at 10:42 +0100, Jes Sorensen wrote:
> > > Fernando Luis Vázquez Cao wrote:
> > > > On Tue, 2006-11-07 at 13:35 -0800, Andrew Morton wrote:
> > > > The problem occurs because i386 (as expected) does not define
> > > > IA64_UNCACHED_ALLOCATOR. I thought that making the select expression
> > > > depend on IA64 as shown below might silence allmodconfig:
> > > > 
> > > >   select IA64_UNCACHED_ALLOCATOR if IA64
> > > > 
> > > > But my guess was wrong and the same warning appeared. It seems that "if"
> > > > expressions do not prevent allmodconfig from checking the symbol
> > > > indicated by the select the "if" is conditioning. By the way, is this
> > > > the expected behaviour? If so, we need to get rid of the reverse
> > > > dependency, modify the "depends on" line accordingly, and make
> > > > IA64_UNCACHED_ALLOCATOR selectable. I may be missing the whole point so
> > > > please correct if I am wrong.
> > > 
> > > This patch is a bad solution as it requires people to manually select
> > > the uncached allocator. It should be enabled automatically by MSPEC,
> > > not the other way round.
> > > 
> > > Given that MSPEC is clearly marked as depending on IA64, it seems bogus
> > > for i386 allmodconfig to barf over it and the problem should be fixed
> > > there instead IMHO.
> > Agreed. That is why I asked if that was allmodconfig's expected
> > behaviour. Andrew?
> > 
> 
> kconfig's `select' isn't very smart.  This is one of the reasons why one
> should avoid using it.
Is my previous patch an acceptable workaround then? Or should we take a
different approach in such cases?

Re: [PATCH 0/1] mspec driver: compile error

[Jes Sorensen]

Andrew Morton wrote:
> On Wed, 08 Nov 2006 18:45:30 +0900
> Fernando Luis Vázquez Cao <fernando@oss.ntt.co.jp> wrote:
>> On Wed, 2006-11-08 at 10:42 +0100, Jes Sorensen wrote:
>>> Given that MSPEC is clearly marked as depending on IA64, it seems bogus
>>> for i386 allmodconfig to barf over it and the problem should be fixed
>>> there instead IMHO.
>> Agreed. That is why I asked if that was allmodconfig's expected
>> behaviour. Andrew?
> 
> kconfig's `select' isn't very smart.  This is one of the reasons why one
> should avoid using it.

Hmmm, so what do we do? I really don't like the idea that one has to
manually select the uncached allocator in order for mspec to be
available.

Alternatively can move the Kconfig field for MSPEC to arch/ia64/Kconfig,
but that seems a bit dodgy too.

Cheers,
Jes

Re: [PATCH 0/1] mspec driver: compile error

[Fernando Luis Vázquez Cao]

2006-11-08 (水) の 11:31 +0100 に Jes Sorensen さんは書きました:
> Andrew Morton wrote:
> > On Wed, 08 Nov 2006 18:45:30 +0900
> > Fernando Luis Vázquez Cao <fernando@oss.ntt.co.jp> wrote:
> >> On Wed, 2006-11-08 at 10:42 +0100, Jes Sorensen wrote:
> >>> Given that MSPEC is clearly marked as depending on IA64, it seems bogus
> >>> for i386 allmodconfig to barf over it and the problem should be fixed
> >>> there instead IMHO.
> >> Agreed. That is why I asked if that was allmodconfig's expected
> >> behaviour. Andrew?
> > 
> > kconfig's `select' isn't very smart.  This is one of the reasons why one
> > should avoid using it.
> 
> Hmmm, so what do we do? I really don't like the idea that one has to
> manually select the uncached allocator in order for mspec to be
> available.
> 
> Alternatively can move the Kconfig field for MSPEC to arch/ia64/Kconfig,
> but that seems a bit dodgy too.
The whole thing could be considered an "allmodconfig" bug. In this case,
"select" is working as expected on IA64 and automatically sets
IA64_UNCACHED_ALLOCATOR when MSPEC is selected.

Perhaps the right fix would be modifying allmodconfig so that it takes
into account dependecies (i.e. "depends on", "select", etc).

Re: [PATCH 0/1] mspec driver: compile error

[Jes Sorensen]

>>>>> "Fernando" == Fernando Luis Vázquez Cao <fernando@oss.ntt.co.jp> writes:

Fernando> Perhaps the right fix would be modifying allmodconfig so
Fernando> that it takes into account dependecies (i.e. "depends on",
Fernando> "select", etc).

Thats probably it.

In the mean time I thought about this and I think the best solution
until the allxxxconfig is fixed, is to move the MSPEC option to
arch/ia64/Kconfig.

Andrew will you be ok with this version ?

Cheers,
Jes

Fix MSPEC driver to build for non SN2 enabled configs as the driver
should work in cached and uncached modes (no fetchop) on these systems.
In addition make MSPEC select IA64_UNCACHED_ALLOCATOR, which is required
for it and move it to arch/ia64/Kconfig to avoid warnings on non ia64
architectures running allmodconfig. Once the Kconfig code is fixed, we
can move it back.

Signed-off-by: Jes Sorensen <jes@sgi.com>

---
 arch/ia64/Kconfig           |    9 +++++++++
 drivers/char/Kconfig        |    8 --------
 drivers/char/mspec.c        |    8 +++++++-
 include/asm-ia64/sn/addrs.h |    6 +++++-
 4 files changed, 21 insertions(+), 10 deletions(-)

Index: linux-2.6/arch/ia64/Kconfig
===================================================================
--- linux-2.6.orig/arch/ia64/Kconfig
+++ linux-2.6/arch/ia64/Kconfig
@@ -484,6 +484,15 @@ source "net/Kconfig"
 
 source "drivers/Kconfig"
 
+config MSPEC
+	tristate "Memory special operations driver"
+	depends on IA64
+	select IA64_UNCACHED_ALLOCATOR
+	help
+	  If you have an ia64 and you want to enable memory special
+	  operations support (formerly known as fetchop), say Y here,
+	  otherwise say N.
+
 source "fs/Kconfig"
 
 source "lib/Kconfig"
Index: linux-2.6/drivers/char/Kconfig
===================================================================
--- linux-2.6.orig/drivers/char/Kconfig
+++ linux-2.6/drivers/char/Kconfig
@@ -409,14 +409,6 @@ config SGI_MBCS
          If you have an SGI Altix with an attached SABrick
          say Y or M here, otherwise say N.
 
-config MSPEC
-	tristate "Memory special operations driver"
-	depends on IA64
-	help
-	  If you have an ia64 and you want to enable memory special
-	  operations support (formerly known as fetchop), say Y here,
-	  otherwise say N.
-
 source "drivers/serial/Kconfig"
 
 config UNIX98_PTYS
Index: linux-2.6/drivers/char/mspec.c
===================================================================
--- linux-2.6.orig/drivers/char/mspec.c
+++ linux-2.6/drivers/char/mspec.c
@@ -72,7 +72,11 @@ enum {
 	MSPEC_UNCACHED
 };
 
+#ifdef CONFIG_SGI_SN
 static int is_sn2;
+#else
+#define is_sn2		0
+#endif
 
 /*
  * One of these structures is allocated when an mspec region is mmaped. The
@@ -211,7 +215,7 @@ mspec_nopfn(struct vm_area_struct *vma, 
 	if (vdata->type == MSPEC_FETCHOP)
 		paddr = TO_AMO(maddr);
 	else
-		paddr = __pa(TO_CAC(maddr));
+		paddr = maddr & ~__IA64_UNCACHED_OFFSET;
 
 	pfn = paddr >> PAGE_SHIFT;
 
@@ -335,6 +339,7 @@ mspec_init(void)
 	 * The fetchop device only works on SN2 hardware, uncached and cached
 	 * memory drivers should both be valid on all ia64 hardware
 	 */
+#ifdef CONFIG_SGI_SN
 	if (ia64_platform_is("sn2")) {
 		is_sn2 = 1;
 		if (is_shub2()) {
@@ -363,6 +368,7 @@ mspec_init(void)
 			goto free_scratch_pages;
 		}
 	}
+#endif
 	ret = misc_register(&cached_miscdev);
 	if (ret) {
 		printk(KERN_ERR "%s: failed to register device %i\n",
Index: linux-2.6/include/asm-ia64/sn/addrs.h
===================================================================
--- linux-2.6.orig/include/asm-ia64/sn/addrs.h
+++ linux-2.6/include/asm-ia64/sn/addrs.h
@@ -136,9 +136,13 @@
  */
 #define TO_PHYS(x)		(TO_PHYS_MASK & (x))
 #define TO_CAC(x)		(CAC_BASE     | TO_PHYS(x))
+#ifdef CONFIG_SGI_SN
 #define TO_AMO(x)		(AMO_BASE     | TO_PHYS(x))
 #define TO_GET(x)		(GET_BASE     | TO_PHYS(x))
-
+#else
+#define TO_AMO(x)		({ BUG(); x; })
+#define TO_GET(x)		({ BUG(); x; })
+#endif
 
 /*
  * Covert from processor physical address to II/TIO physical address: