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
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 <[email protected]>
---
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:
On Tue, 07 Nov 2006 11:31:54 +0100
Jes Sorensen <[email protected]> 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'
On Tue, 2006-11-07 at 13:35 -0800, Andrew Morton wrote:
> On Tue, 07 Nov 2006 11:31:54 +0100
> Jes Sorensen <[email protected]> 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 <[email protected]>
---
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,
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
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
On Wed, 08 Nov 2006 18:45:30 +0900
Fernando Luis V?zquez Cao <[email protected]> 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.
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 <[email protected]> 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?
Andrew Morton wrote:
> On Wed, 08 Nov 2006 18:45:30 +0900
> Fernando Luis V?zquez Cao <[email protected]> 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
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 <[email protected]> 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).
>>>>> "Fernando" == Fernando Luis V?zquez Cao <[email protected]> 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 <[email protected]>
---
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: