From: Stephen Rothwell <hidden> Date: 2008-07-07 08:40:53
Hi Sam,
Today's linux-next build (powerpc ppc64_defconfig) failed like this:
arch/powerpc/platforms/cell/spu_base.c: In function '__spu_trap_data_seg':
arch/powerpc/platforms/cell/spu_base.c:194: error: duplicate case value
arch/powerpc/platforms/cell/spu_base.c:177: error: previously used here
This means that USER_REGION_ID (line 177) is the same as KERNEL_REGION_ID
(line 194).
From: Sam Ravnborg <hidden> Date: 2008-07-07 12:50:51
On Mon, Jul 07, 2008 at 06:40:38PM +1000, Stephen Rothwell wrote:
Hi Sam,
Today's linux-next build (powerpc ppc64_defconfig) failed like this:
arch/powerpc/platforms/cell/spu_base.c: In function '__spu_trap_data_seg':
arch/powerpc/platforms/cell/spu_base.c:194: error: duplicate case value
arch/powerpc/platforms/cell/spu_base.c:177: error: previously used here
This means that USER_REGION_ID (line 177) is the same as KERNEL_REGION_ID
(line 194).
From include/asm-powerpc/pgtable-ppc64.h:
#define REGION_SHIFT 60UL
#define REGION_MASK (0xfUL << REGION_SHIFT)
#define REGION_ID(ea) (((unsigned long)(ea)) >> REGION_SHIFT)
#define VMALLOC_REGION_ID (REGION_ID(VMALLOC_START))
#define KERNEL_REGION_ID (REGION_ID(PAGE_OFFSET))
#define VMEMMAP_REGION_ID (0xfUL)
#define USER_REGION_ID (0UL)
From include/asm-powerpc/page.h:
#define PAGE_OFFSET ASM_CONST(CONFIG_PAGE_OFFSET)
From .config:
CONFIG_PAGE_OFFSET=0xffffffff
Not good.
From arch/powerpc/Kconfig (ADVANCED_OPTIONS is not set):
config PAGE_OFFSET_BOOL
bool "Set custom page offset address"
depends on ADVANCED_OPTIONS
config PAGE_OFFSET
hex "Virtual address of memory base" if PAGE_OFFSET_BOOL
default "0xc0000000"
if PPC64
config PAGE_OFFSET
hex
default "0xc000000000000000"
endif
Reverting commit 9ba4ff5059e1a26698786373c8c71321c37ef7f9 ("kconfig:
normalize int/hex values") seems to have fixed it.
From .config:
CONFIG_PAGE_OFFSET=0xc000000000000000
I see what is wrong - we use a int to hold the value above
and it does not fit.
Will fix tonight or tomorrow.
Sam
From: Roman Zippel <hidden> Date: 2008-07-07 16:25:42
Hi,
On Mon, 7 Jul 2008, Stephen Rothwell wrote:
Hi Sam,
Today's linux-next build (powerpc ppc64_defconfig) failed like this:
arch/powerpc/platforms/cell/spu_base.c: In function '__spu_trap_data_seg':
arch/powerpc/platforms/cell/spu_base.c:194: error: duplicate case value
arch/powerpc/platforms/cell/spu_base.c:177: error: previously used here
I guess there also has been a kconfig warning somewhere. :)
I should have gone through all archs to test this, sorry about that.
Luckily it's only powerpc that uses 64bit values. I would prefer to
standardize on 32bit values, as it doesn't really make sense to expect
from the user to input full 64bit values and it's easy to generate the
full value in a header. This would also ease on any portability issues
(kconfig is compiled with the host compiler not the target compiler).
Below is a patch that fixes this for all archs (generated against the git
tree). The powerpc parts need a more careful review, the rest isn't really
critical.
bye, Roman
Fix remaining warnings generated kconfig to normalize all constant values.
Generate powerpc 64bit page offset via header instead of kconfig.
Signed-off-by: Roman Zippel <redacted>
---
arch/arm/Kconfig | 4 ++--
arch/arm/Kconfig-nommu | 10 +++++-----
arch/arm/mm/Kconfig | 4 ++--
arch/blackfin/Kconfig | 12 ++++++------
arch/blackfin/mach-bf548/Kconfig | 8 ++++----
arch/cris/arch-v10/Kconfig | 26 +++++++++++++-------------
arch/cris/arch-v10/drivers/Kconfig | 14 +++++++-------
arch/cris/arch-v32/Kconfig | 28 ++++++++++++++--------------
arch/cris/arch-v32/drivers/Kconfig | 30 ++++++++++++------------------
arch/cris/arch-v32/mach-a3/Kconfig | 14 +++++++-------
arch/cris/arch-v32/mach-fs/Kconfig | 32 ++++++++++++++++----------------
arch/frv/Kconfig | 4 ++--
arch/m32r/Kconfig | 36 ++++++++++++++++++------------------
arch/powerpc/Kconfig | 16 ++++++----------
arch/ppc/Kconfig | 8 ++++----
arch/sh/Kconfig | 10 +++++-----
arch/sh/Kconfig.debug | 2 +-
arch/sh/mm/Kconfig | 6 +++---
include/asm-powerpc/page.h | 10 ++++++++--
19 files changed, 135 insertions(+), 139 deletions(-)
Index: linux-2.6/arch/arm/Kconfig
===================================================================
@@ -714,8 +714,8 @@ config PHYSICAL_START_BOOLconfigPHYSICAL_STARThex"Physical address where the kernel is loaded"ifPHYSICAL_START_BOOL-default"0x02000000"ifPPC_STD_MMU&&CRASH_DUMP-default"0x00000000"+default"0x2000000"ifPPC_STD_MMU&&CRASH_DUMP+default"0"configPHYSICAL_ALIGNhex
From: Sam Ravnborg <hidden> Date: 2008-07-07 21:00:17
On Mon, Jul 07, 2008 at 06:13:55PM +0200, Roman Zippel wrote:
Hi,
On Mon, 7 Jul 2008, Stephen Rothwell wrote:
quoted
Hi Sam,
Today's linux-next build (powerpc ppc64_defconfig) failed like this:
arch/powerpc/platforms/cell/spu_base.c: In function '__spu_trap_data_seg':
arch/powerpc/platforms/cell/spu_base.c:194: error: duplicate case value
arch/powerpc/platforms/cell/spu_base.c:177: error: previously used here
I guess there also has been a kconfig warning somewhere. :)
I should have gone through all archs to test this, sorry about that.
Luckily it's only powerpc that uses 64bit values. I would prefer to
standardize on 32bit values, as it doesn't really make sense to expect
from the user to input full 64bit values and it's easy to generate the
full value in a header. This would also ease on any portability issues
(kconfig is compiled with the host compiler not the target compiler).
Below is a patch that fixes this for all archs (generated against the git
tree). The powerpc parts need a more careful review, the rest isn't really
critical.
bye, Roman
But ths change actually decrease readability of the
value. Before we had 4x2 hex characters. But now we have 3x2 + 1x1.
Does this check really by us enough to warrant this?
I am aware that the check is implmented by:
+ str = strdup_type(def_sym->name, sym->type);
+ if (strcmp(str, def_sym->name)) {
+ prop_warn(prop,
+ "default for config symbol '%s'"
+ " should be %s", sym->name, str);
+ prop->expr->left.sym = sym_lookup(str, SYMBOL_CO
+ }
+ free(str);
break;
Could we soften this check a little maybe?
At least so we accept leading null's.
As for the patch in general I will await an ack from powerpc
before applying it.
Sam
From: Michael Ellerman <hidden> Date: 2008-07-07 23:36:41
On Mon, 2008-07-07 at 18:13 +0200, Roman Zippel wrote:
Hi,
On Mon, 7 Jul 2008, Stephen Rothwell wrote:
quoted
Hi Sam,
Today's linux-next build (powerpc ppc64_defconfig) failed like this:
arch/powerpc/platforms/cell/spu_base.c: In function '__spu_trap_data_seg':
arch/powerpc/platforms/cell/spu_base.c:194: error: duplicate case value
arch/powerpc/platforms/cell/spu_base.c:177: error: previously used here
I guess there also has been a kconfig warning somewhere. :)
I should have gone through all archs to test this, sorry about that.
Luckily it's only powerpc that uses 64bit values. I would prefer to
standardize on 32bit values, as it doesn't really make sense to expect
from the user to input full 64bit values and it's easy to generate the
full value in a header. This would also ease on any portability issues
(kconfig is compiled with the host compiler not the target compiler).
Hi Roman,
I don't really see why it "doesn't make sense" for users to input 64-bit
values, they're configuring addresses for a 64-bit kernel, so some of
the values are going to be 64 bit.
Perhaps all the current values can be generated by shifting 32-bit
constants, but that seems like a hack to me.
Another comment below ..
@@ -714,8 +714,8 @@ config PHYSICAL_START_BOOLconfigPHYSICAL_STARThex"Physical address where the kernel is loaded"ifPHYSICAL_START_BOOL-default"0x02000000"ifPPC_STD_MMU&&CRASH_DUMP-default"0x00000000"+default"0x2000000"ifPPC_STD_MMU&&CRASH_DUMP+default"0"configPHYSICAL_ALIGNhex
I don't see where you cope with the "if CRASH_DUMP" case, and in fact my
config changes for the worse when I apply your patch and regenerate my
config:
@@ -370,9 +370,8 @@ CONFIG_HOTPLUG_PCI_RPA=m CONFIG_HOTPLUG_PCI_RPA_DLPAR=m # CONFIG_HAS_RAPIDIO is not set-CONFIG_PAGE_OFFSET=0xc000000000000000-CONFIG_KERNEL_START=0xc000000002000000-CONFIG_PHYSICAL_START=0x02000000+CONFIG_PAGE_OFFSET=0xc0000000+CONFIG_PHYSICAL_START=0x2000000
cheers
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
From: Roman Zippel <hidden> Date: 2008-07-08 02:56:13
Hi,
On Tue, 8 Jul 2008, Michael Ellerman wrote:
I don't really see why it "doesn't make sense" for users to input 64-bit
values, they're configuring addresses for a 64-bit kernel, so some of
the values are going to be 64 bit.
Do you really expect users to insert random 64bit addresses without making
a mistake?
Please make a realistic case, where the user input of a full 64bit value
is required (i.e. where it's not easier to just offer the user a few
choices).
Currently there is simply no need for this, as even powerpc only used them
as constants here.
@@ -714,8 +714,8 @@ config PHYSICAL_START_BOOLconfigPHYSICAL_STARThex"Physical address where the kernel is loaded"ifPHYSICAL_START_BOOL-default"0x02000000"ifPPC_STD_MMU&&CRASH_DUMP-default"0x00000000"+default"0x2000000"ifPPC_STD_MMU&&CRASH_DUMP+default"0"configPHYSICAL_ALIGNhex
I don't see where you cope with the "if CRASH_DUMP" case, and in fact my
config changes for the worse when I apply your patch and regenerate my
config:
@@ -370,9 +370,8 @@ CONFIG_HOTPLUG_PCI_RPA=m CONFIG_HOTPLUG_PCI_RPA_DLPAR=m # CONFIG_HAS_RAPIDIO is not set-CONFIG_PAGE_OFFSET=0xc000000000000000-CONFIG_KERNEL_START=0xc000000002000000-CONFIG_PHYSICAL_START=0x02000000+CONFIG_PAGE_OFFSET=0xc0000000+CONFIG_PHYSICAL_START=0x2000000
Why is this worse? These are constants, you're not supposed to change them
anyway.
The remaining values are generated in page.h and should be the same as
before. If that isn't the case and this patch produces a nonworking
kernel, I'd like to hear about it.
bye, Roman
From: Sam Ravnborg <hidden> Date: 2008-07-08 21:18:43
Hi Roman.
I thought a bit more about this.
I should have gone through all archs to test this, sorry about that.
Luckily it's only powerpc that uses 64bit values. I would prefer to
standardize on 32bit values, as it doesn't really make sense to expect
from the user to input full 64bit values and it's easy to generate the
full value in a header.
We use Kconfig for a mixture of user editable values and fixed
configuration values.
And I agree that asking the user to input a 64 bit number is not usefull.
But keeping support for 64 bit values is what I would consider
expected functionality.
So removing support for 64 bit is not good IMO.
This would also ease on any portability issues
(kconfig is compiled with the host compiler not the target compiler).
We use strtol() in a few places in symbol.c already where we do an
implicit conversion to int. Why did this not cause us problems before?
Is it because these code paths are only triggered when we deal with ranges?
If so we could 'fix' strdup_type() to not use strto{,u}l() so it
is 64 bit clean and we are back to old behaviour.
Sam
From: Michael Ellerman <hidden> Date: 2008-07-10 00:51:24
On Tue, 2008-07-08 at 04:55 +0200, Roman Zippel wrote:
Hi,
On Tue, 8 Jul 2008, Michael Ellerman wrote:
quoted
I don't really see why it "doesn't make sense" for users to input 64-bit
values, they're configuring addresses for a 64-bit kernel, so some of
the values are going to be 64 bit.
Do you really expect users to insert random 64bit addresses without making
a mistake?
Well yes :) But I think that's because you're thinking of
"end-users" and I'm thinking of "users" like myself - ie. _I_ use
Kconfig and I do expect myself to be able to type a 64-bit address.
@@ -370,9 +370,8 @@ CONFIG_HOTPLUG_PCI_RPA=m CONFIG_HOTPLUG_PCI_RPA_DLPAR=m # CONFIG_HAS_RAPIDIO is not set-CONFIG_PAGE_OFFSET=0xc000000000000000-CONFIG_KERNEL_START=0xc000000002000000-CONFIG_PHYSICAL_START=0x02000000+CONFIG_PAGE_OFFSET=0xc0000000+CONFIG_PHYSICAL_START=0x2000000
Why is this worse? These are constants, you're not supposed to change them
anyway.
The remaining values are generated in page.h and should be the same as
before. If that isn't the case and this patch produces a nonworking
kernel, I'd like to hear about it.
You're right the built kernel is fine. So it's not a bug, but I think it
is nicer to have the real values in the .config.
cheers
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
From: Roman Zippel <hidden> Date: 2008-07-10 14:52:25
Hi,
On Tue, 8 Jul 2008, Sam Ravnborg wrote:
We use Kconfig for a mixture of user editable values and fixed
configuration values.
And I agree that asking the user to input a 64 bit number is not usefull.
But keeping support for 64 bit values is what I would consider
expected functionality.
Isn't that a bit of a stretch? That's a rather generic argument, do we
really have to keep everything, because someone _might_ want to use it?
I'd rather look at how useful it really is to keep that functionality and
so far we exactly one user, which doesn't strictly require it.
So far it's more a convenience feature rather than a required feature. We
don't lose any functionality, it's just shifted from kconfig to the header
file.
quoted
This would also ease on any portability issues
(kconfig is compiled with the host compiler not the target compiler).
We use strtol() in a few places in symbol.c already where we do an
implicit conversion to int. Why did this not cause us problems before?
Is it because these code paths are only triggered when we deal with ranges?
If so we could 'fix' strdup_type() to not use strto{,u}l() so it
is 64 bit clean and we are back to old behaviour.
Ranges are the primary reason I made it consistent with this.
If we really wanted to support 64bit numbers, it would create only more
problems. First you have to make sure that on every build host (i.e also
non-Linux) strtoll() is available. Then how it should these numbers be
represented? On 32bit these may need a 'll' postfix, but the powerpc
example already shows, that there are different requirements, so they use
ASM_CONST for that. How should this postprecessing be integrated into
kconfig?
I would seriously prefer to keep things as simple as possible, unless
there is a real requirement for it, but so far there isn't any.
If we really needed something like this, I'd rather introduce a new
expression type, which works more like an unquoted string (e.g. you
could define something like (1l<<$PAGE_SHIFT)), where things like ranges
wouldn't work, but it also wouldn't have any content restrictions.
bye, Roman
From: Roman Zippel <hidden> Date: 2008-07-10 15:00:24
Hi,
On Thu, 10 Jul 2008, Michael Ellerman wrote:
Well yes :) But I think that's because you're thinking of
"end-users" and I'm thinking of "users" like myself - ie. _I_ use
Kconfig and I do expect myself to be able to type a 64-bit address.
That doesn't really answer my question, why you need this.
@@ -370,9 +370,8 @@ CONFIG_HOTPLUG_PCI_RPA=m CONFIG_HOTPLUG_PCI_RPA_DLPAR=m # CONFIG_HAS_RAPIDIO is not set-CONFIG_PAGE_OFFSET=0xc000000000000000-CONFIG_KERNEL_START=0xc000000002000000-CONFIG_PHYSICAL_START=0x02000000+CONFIG_PAGE_OFFSET=0xc0000000+CONFIG_PHYSICAL_START=0x2000000
Why is this worse? These are constants, you're not supposed to change them
anyway.
The remaining values are generated in page.h and should be the same as
before. If that isn't the case and this patch produces a nonworking
kernel, I'd like to hear about it.
You're right the built kernel is fine. So it's not a bug,
Good, could someone please ack whether the powerpc changes are acceptable?
but I think it is nicer to have the real values in the .config.
From: Milton Miller <hidden> Date: 2008-07-14 16:52:24
Hi Roman.
I saw your reply on the list archives but can not find
it in my inbox.
On Sun Jul 13 at 09:21:08 EST 2008, Roman Zippel wrote:
On Sat, 12 Jul 2008, Milton Miller wrote:
quoted
(1) #define PAGE_OFFSET (ASM_CONST(CONFIG_PAGE_OFFSET) << 32)
It creates unreadable code, where two defines with almost the same
name (the
quoted
only difference being
the CONFIG_ prefix, which is often ignored when scanning) contains
radically
quoted
different values.
(2) #define PAGE_OFFSET ASM_CONST(CONFIG_PAGE_OFFSET)
Giving it different names is not really difficult. Any objections to
CONFIG_PAGE_HIGH_OFFSET?
Once you remove the symmetry and add the ifdef to page.h, we need to
reevaluate its presence in Kconfig. I can't think of a reason to
have a partial value, and therefore would instead say take out the
config variable on PPC64 as the PPC32 address split reason is moot.
is harder to read. The value is a list of 4 1 byte values, but you
have hidden the first nibble making parsing the rest of the value
hard.
Sam mentioned that already, but that's a situation where the warning
can
be relaxed.
The warning can be relaxed? What are you talking about?
I was trying to make a case for leading zeros without actually stating
what I was asking for.
You changes did s/(0x)?0*/0x/, y/A-F/a-f/ -- that is you made all hex
constants conform to "0x%x". I was arguing for 0x%8.8x, or just
leaving
them as formatted.
quoted
If you are worried about users tring to set values that are too high,
then make the types be hex8, hex16, hex32, and hex64.
It's not this, I value consistency as much as you and the values are
sometimes used as integers, so a working range is needed. Using simple
integers keeps things much simpler and as the ASM_CONST example shows
any
bigger values are not necessarily directly usable anyway.
How many places want to range check hex numbers? Is it just setting
the number of digits input? If strtoull is to hard, can we just do
the check as strings of digits?
If we keep this new restriction on hex numbers, it needs to be in
the Documentation, even if its prefixed by "currently".
milton
From: Stephen Rothwell <hidden> Date: 2008-07-25 04:13:42
Hi Sam,
On Mon, 7 Jul 2008 18:40:38 +1000 Stephen Rothwell [off-list ref] wrote:
Today's linux-next build (powerpc ppc64_defconfig) failed like this:
arch/powerpc/platforms/cell/spu_base.c: In function '__spu_trap_data_seg':
arch/powerpc/platforms/cell/spu_base.c:194: error: duplicate case value
arch/powerpc/platforms/cell/spu_base.c:177: error: previously used here
This still fails in linux-next today. We need some solution before you
send these commits to Linus.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
From: Sam Ravnborg <hidden> Date: 2008-07-26 10:07:40
On Fri, Jul 25, 2008 at 02:13:30PM +1000, Stephen Rothwell wrote:
Hi Sam,
On Mon, 7 Jul 2008 18:40:38 +1000 Stephen Rothwell [off-list ref] wrote:
quoted
Today's linux-next build (powerpc ppc64_defconfig) failed like this:
arch/powerpc/platforms/cell/spu_base.c: In function '__spu_trap_data_seg':
arch/powerpc/platforms/cell/spu_base.c:194: error: duplicate case value
arch/powerpc/platforms/cell/spu_base.c:177: error: previously used here
This still fails in linux-next today. We need some solution before you
send these commits to Linus.
I removed the offending commits from kbuild-next before I
sent the pull request.
I will though revisit the issue after -rc1.
Sam