RE: [PATCH] ARM: use "depends on" for SoC configs instead of "if" after prompt

RE: [PATCH] ARM: use "depends on" for SoC configs instead of "if" after prompt

[yamada.masahiro]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3205 bytes --]

Hi Arnd,

 
> On Monday 16 November 2015 12:06:10 Masahiro Yamada wrote:
> > Many ARM sub-architectures use prompts followed by "if" conditional,
> > but it is wrong.
> >
> > Please notice the difference between
> >
> >     config ARCH_FOO
> >             bool "Foo SoCs" if ARCH_MULTI_V7
> >
> > and
> >
> >     config ARCH_FOO
> >             bool "Foo SoCs"
> >             depends on ARCH_MULTI_V7
> >
> > These two are *not* equivalent!
> >
> > In the former statement, it is not ARCH_FOO, but its prompt that
> > depends on ARCH_MULTI_V7.  So, it is completely valid that ARCH_FOO is
> > selected by another, but ARCH_MULTI_V7 is still disabled. As it is not
> > unmet dependency, Kconfig never warns.  This is probably not what you
> > want.
> 
> Did you encounter a case where someone actually did a 'select' on one of
> those symbols? I probably introduced a lot of them and did not expect that
> to happen.

No, for ARM sub-architectures.
But, yes for the ARM core part.


For example, the following entry in arch/arm/Kconfig is suspicous.

config PCI
        bool "PCI support" if MIGHT_HAVE_PCI
        help
          Find out whether you have a PCI motherboard. PCI is the name of a
          bus system, i.e. the way the CPU talks to the other stuff inside
          your box. Other bus systems are ISA, EISA, MicroChannel (MCA) or
          VESA. If you have PCI, say Y, otherwise N.




Try "make ARCH=arm footbridge_defconfig" and check the .config file.

It defines CONFIG_PCI=y, but not CONFIG_MIGHT_HAVE_PCI.
I am not sure this is a sane .config or not.

But, anyway, Kconfig does not complain about it.


We have similar issues for CPU_V6, CPU_V6K, CPU_V7, etc.
The config is selected, but the "if" conditional is unmet.
(I decided to postpone this problem because it would take some time to understand
complicated dependency.)

The use of a prompt followed by "if" without correct understanding is dangerous
because it could hide the unmet dependency problem.

I want to eliminate the potential problem by this patch
before somebody introduce insane dependency.


> 
> > diff --git a/arch/arm/mach-integrator/Kconfig
> > b/arch/arm/mach-integrator/Kconfig
> > index 02d0834..2fa9d11 100644
> > --- a/arch/arm/mach-integrator/Kconfig
> > +++ b/arch/arm/mach-integrator/Kconfig
> > @@ -1,5 +1,6 @@
> >  config ARCH_INTEGRATOR
> > -	bool "ARM Ltd. Integrator family" if (ARCH_MULTI_V4T ||
> ARCH_MULTI_V5 || ARCH_MULTI_V6)
> > +	bool "ARM Ltd. Integrator family"
> > +	depends on ARCH_MULTI_V4T || ARCH_MULTI_V5 || ARCH_MULTI_V6
> >  	select ARM_AMBA
> >  	select ARM_PATCH_PHYS_VIRT if MMU
> >  	select AUTO_ZRELADDR
> 
> There is one related change that I would like to see, and that is to convert
> all top-level 'config' statements that have sub-options into 'menuconfig'
> statements for consistency. At the moment, the platform menu has a mix of
> platform-selection and platform-specific options, and I'd like to make that
> more consistent.

I agree, but in another patch (or series)?



Best Regards
Mashairo Yamada

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] ARM: use "depends on" for SoC configs instead of "if" after prompt

[Russell King - ARM Linux]

On Mon, Nov 16, 2015 at 10:32:51AM +0000, yamada.masahiro@socionext.com wrote:
> Hi Arnd,
> 
>  
> > On Monday 16 November 2015 12:06:10 Masahiro Yamada wrote:
> > > Many ARM sub-architectures use prompts followed by "if" conditional,
> > > but it is wrong.
> > >
> > > Please notice the difference between
> > >
> > >     config ARCH_FOO
> > >             bool "Foo SoCs" if ARCH_MULTI_V7
> > >
> > > and
> > >
> > >     config ARCH_FOO
> > >             bool "Foo SoCs"
> > >             depends on ARCH_MULTI_V7
> > >
> > > These two are *not* equivalent!
> > >
> > > In the former statement, it is not ARCH_FOO, but its prompt that
> > > depends on ARCH_MULTI_V7.  So, it is completely valid that ARCH_FOO is
> > > selected by another, but ARCH_MULTI_V7 is still disabled. As it is not
> > > unmet dependency, Kconfig never warns.  This is probably not what you
> > > want.
> > 
> > Did you encounter a case where someone actually did a 'select' on one of
> > those symbols? I probably introduced a lot of them and did not expect that
> > to happen.
> 
> No, for ARM sub-architectures.
> But, yes for the ARM core part.
> 
> 
> For example, the following entry in arch/arm/Kconfig is suspicous.
> 
> config PCI
>         bool "PCI support" if MIGHT_HAVE_PCI
>         help
>           Find out whether you have a PCI motherboard. PCI is the name of a
>           bus system, i.e. the way the CPU talks to the other stuff inside
>           your box. Other bus systems are ISA, EISA, MicroChannel (MCA) or
>           VESA. If you have PCI, say Y, otherwise N.
> 
> 
> 
> 
> Try "make ARCH=arm footbridge_defconfig" and check the .config file.
> 
> It defines CONFIG_PCI=y, but not CONFIG_MIGHT_HAVE_PCI.
> I am not sure this is a sane .config or not.

It's correct.  "MIGHT_HAVE_PCI" is used by platforms which _might_ _have_
_PCI_, not by platforms which _do_ _have_ _PCI_.  Platforms which _do_
_have_ _PCI_ select PCI directly, and because "MIGHT_HAVE_PCI" is not
set, users are not offered an option that they can never disable.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

RE: [PATCH] ARM: use "depends on" for SoC configs instead of "if" after prompt

[yamada.masahiro]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2182 bytes --]

Hi Arnd,

Could you apply this patch with Acked-by storm
so that other subarchtectures can base on it?

Also, the following series:

1/2: ARM: hisi: do not export smp_operations structures
2/2: ARM: use const and __initconst for smp_operations





> -----Original Message-----
> From: Thierry Reding [mailto:thierry.reding@gmail.com]
> Sent: Wednesday, November 25, 2015 12:59 AM
> To: Yamada, Masahiro
> Cc: linux-arm-kernel@lists.infradead.org; arm@kernel.org; Michal Marek;
> Arnd Bergmann; Olof Johansson; linux-kbuild@vger.kernel.org; Jamie Iles;
> Andy Gross; Sascha Hauer; Gregory Clement; linux-sh@vger.kernel.org;
> Tsahee Zidenberg; linux-arm-msm@vger.kernel.org; Linus Walleij; Kumar
> Gala; Jean-Christophe Plagniol-Villard; Dinh Nguyen; Barry Song; Shiraz
> Hashim; Ray Jui; Patrice Chotard; kernel@stlinux.com; Sören Brinkmann;
> Simon Horman; Michal Simek; Heiko Stuebner;
> linux-rockchip@lists.infradead.org; linux-soc@vger.kernel.org; David
> Brown; Scott Branden; Viresh Kumar; Alexandre Belloni; Kyungmin Park;
> Krzysztof Kozlowski; bcm-kernel-feedback-list@broadcom.com; Florian
> Fainelli; Alexandre Courbot; Matthias Brugger; Kukjin Kim; Rob Herring;
> linux-kernel@vger.kernel.org; spear-devel@list.st.com; Sebastian
> Hesselbarth; Sudeep Holla; Andrew Lunn; Russell King;
> linux-samsung-soc@vger.kernel.org; linux-mediatek@lists.infradead.org;
> linux-tegra@vger.kernel.org; Maxime Coquelin; Srinivas Kandagatla; Shawn
> Guo; Liviu Dudau; Jun Nie; Lorenzo Pieralisi; Nicolas Ferre; Jason Cooper;
> Stephen Warren; Krzysztof Halasa; Wei Xu; Magnus Damm; Chen-Yu Tsai; Maxime
> Ripard
> Subject: Re: [PATCH] ARM: use "depends on" for SoC configs instead of "if"
> after prompt
> 
> On Mon, Nov 16, 2015 at 12:06:10PM +0900, Masahiro Yamada wrote:
> [...]
> >  arch/arm/mach-tegra/Kconfig      |  3 ++-
> 
> There will be a slight conflict between this and a patch in the tegra tree,
> but nothing that the ARM SoC maintainers can't handle, so:
> 
> Acked-by: Thierry Reding <treding@nvidia.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥