diff mbox

[1/2] powerpc: fix the dependency issue for CRASH_DUMP

Message ID 1430396997-14656-2-git-send-email-haokexin@gmail.com
State New
Headers show

Commit Message

Kevin Hao April 30, 2015, 12:29 p.m. UTC
In the current code, the RELOCATABLE will be forcedly enabled when
enabling CRASH_DUMP. But for ppc32, the RELOCABLE also depend on
ADVANCED_OPTIONS and select NONSTATIC_KERNEL. This will cause build
error when CRASH_DUMP=y && ADVANCED_OPTIONS=n. Even there is no such
issue for ppc64, but select is only for non-visible symbols and for
symbols with no dependencies. As for a symbol like RELOCATABLE, it is
definitely not suitable to select it. So choose to depend on it.

Also enable the RELOCATABLE explicitly for the defconfigs which has
CRASH_DUMP enabled.

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 arch/powerpc/Kconfig                 | 3 +--
 arch/powerpc/configs/ppc64_defconfig | 1 +
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Scott Wood May 4, 2015, 10:17 p.m. UTC | #1
On Thu, 2015-04-30 at 20:29 +0800, Kevin Hao wrote:
> In the current code, the RELOCATABLE will be forcedly enabled when
> enabling CRASH_DUMP. But for ppc32, the RELOCABLE also depend on
> ADVANCED_OPTIONS and select NONSTATIC_KERNEL. This will cause build
> error when CRASH_DUMP=y && ADVANCED_OPTIONS=n. Even there is no such
> issue for ppc64, but select is only for non-visible symbols and for
> symbols with no dependencies. As for a symbol like RELOCATABLE, it is
> definitely not suitable to select it. So choose to depend on it.

Why is it "definitely not suitable to select it", provided the
ADVANCED_OPTIONS dependency is removed, and the FLATMEM dependency is
moved to places that select RELOCATABLE?  It seems wrong that the user
should have to enable ADVANCED_OPTIONS to even see the option to build a
crash kernel.

-Scott
Kevin Hao May 5, 2015, 2:27 a.m. UTC | #2
On Mon, May 04, 2015 at 05:17:17PM -0500, Scott Wood wrote:
> On Thu, 2015-04-30 at 20:29 +0800, Kevin Hao wrote:
> > In the current code, the RELOCATABLE will be forcedly enabled when
> > enabling CRASH_DUMP. But for ppc32, the RELOCABLE also depend on
> > ADVANCED_OPTIONS and select NONSTATIC_KERNEL. This will cause build
> > error when CRASH_DUMP=y && ADVANCED_OPTIONS=n. Even there is no such
> > issue for ppc64, but select is only for non-visible symbols and for
> > symbols with no dependencies. As for a symbol like RELOCATABLE, it is
> > definitely not suitable to select it. So choose to depend on it.
> 
> Why is it "definitely not suitable to select it", provided the
> ADVANCED_OPTIONS dependency is removed, and the FLATMEM dependency is
> moved to places that select RELOCATABLE?

Even with this change, the definition of RELOCATABLE still be something like
this:
    config RELOCATABLE
           bool "Build a relocatable kernel"
           depends on (PPC64 && !COMPILE_TEST) || 44x || FSL_BOOKE
           select NONSTATIC_KERNEL

Quoted form Documentation/kbuild/kconfig-language.txt:
        select should be used with care. select will force
        a symbol to a value without visiting the dependencies.
        By abusing select you are able to select a symbol FOO even
        if FOO depends on BAR that is not set.
        In general use select only for non-visible symbols
        (no prompts anywhere) and for symbols with no dependencies.
        That will limit the usefulness but on the other hand avoid
        the illegal configurations all over.

So it is always error prone to select a kernel option like this.

>  It seems wrong that the user
> should have to enable ADVANCED_OPTIONS to even see the option to build a
> crash kernel.

Yes, it seems ridiculous. But this is fixed in the patch 2.

Thanks,
Kevin
Scott Wood May 5, 2015, 2:34 a.m. UTC | #3
On Tue, 2015-05-05 at 10:27 +0800, Kevin Hao wrote:
> On Mon, May 04, 2015 at 05:17:17PM -0500, Scott Wood wrote:
> > On Thu, 2015-04-30 at 20:29 +0800, Kevin Hao wrote:
> > > In the current code, the RELOCATABLE will be forcedly enabled when
> > > enabling CRASH_DUMP. But for ppc32, the RELOCABLE also depend on
> > > ADVANCED_OPTIONS and select NONSTATIC_KERNEL. This will cause build
> > > error when CRASH_DUMP=y && ADVANCED_OPTIONS=n. Even there is no such
> > > issue for ppc64, but select is only for non-visible symbols and for
> > > symbols with no dependencies. As for a symbol like RELOCATABLE, it is
> > > definitely not suitable to select it. So choose to depend on it.
> > 
> > Why is it "definitely not suitable to select it", provided the
> > ADVANCED_OPTIONS dependency is removed, and the FLATMEM dependency is
> > moved to places that select RELOCATABLE?
> 
> Even with this change, the definition of RELOCATABLE still be something like
> this:
>     config RELOCATABLE
>            bool "Build a relocatable kernel"
>            depends on (PPC64 && !COMPILE_TEST) || 44x || FSL_BOOKE
>            select NONSTATIC_KERNEL

That matches the cases where CRASH_DUMP selects RELOCATABLE.

> Quoted form Documentation/kbuild/kconfig-language.txt:
>         select should be used with care. select will force
>         a symbol to a value without visiting the dependencies.
>         By abusing select you are able to select a symbol FOO even
>         if FOO depends on BAR that is not set.
>         In general use select only for non-visible symbols
>         (no prompts anywhere) and for symbols with no dependencies.
>         That will limit the usefulness but on the other hand avoid
>         the illegal configurations all over.
> 
> So it is always error prone to select a kernel option like this.

Yes, but these days Kbuild does warn about selecting a symbol with unmet
dependencies, which IIRC wasn't the case when that was written.

> >  It seems wrong that the user
> > should have to enable ADVANCED_OPTIONS to even see the option to build a
> > crash kernel.
> 
> Yes, it seems ridiculous. But this is fixed in the patch 2.

OK...  Still non-obvious, but at least not *as* bad.

-Scott
diff mbox

Patch

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 190cc48abc0c..d6bbf4f6f869 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -429,8 +429,7 @@  config KEXEC
 
 config CRASH_DUMP
 	bool "Build a kdump crash kernel"
-	depends on PPC64 || 6xx || FSL_BOOKE || (44x && !SMP)
-	select RELOCATABLE if (PPC64 && !COMPILE_TEST) || 44x || FSL_BOOKE
+	depends on 6xx || ((PPC64 || FSL_BOOKE || (44x && !SMP)) && RELOCATABLE)
 	help
 	  Build a kernel suitable for use as a kdump capture kernel.
 	  The same kernel binary can be used as production kernel and dump
diff --git a/arch/powerpc/configs/ppc64_defconfig b/arch/powerpc/configs/ppc64_defconfig
index aad501ae3834..01f7b63f2df0 100644
--- a/arch/powerpc/configs/ppc64_defconfig
+++ b/arch/powerpc/configs/ppc64_defconfig
@@ -46,6 +46,7 @@  CONFIG_BINFMT_MISC=m
 CONFIG_PPC_TRANSACTIONAL_MEM=y
 CONFIG_KEXEC=y
 CONFIG_CRASH_DUMP=y
+CONFIG_RELOCATABLE=y
 CONFIG_IRQ_ALL_CPUS=y
 CONFIG_MEMORY_HOTREMOVE=y
 CONFIG_KSM=y