LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/1] kconfig: warn if an unknown symbol is selected
@ 2014-09-30 18:09 Paul Bolle
  2014-11-03 11:38 ` Paul Bolle
  0 siblings, 1 reply; 5+ messages in thread
From: Paul Bolle @ 2014-09-30 18:09 UTC (permalink / raw)
  To: Yann E. MORIN; +Cc: Michal Marek, linux-kbuild, linux-kernel

A select of an unknown symbol is basically treated as a nop and is
silently skipped. This is annoying if the selected symbol contains a
typo. It can also hide the fact that a treewide symbol cleanup was only
done partially.

There are also a few cases were this might have been done on purpose.
But that anti-pattern should be discouraged. Almost all select
statements point to a known and reachable symbol. So people will likely
assume that any selected symbol is actually set. Selects that violate
this assumption can only be spotted by checking multiple Kconfig files,
often across architectures. It is unlikely that people will do this
regularly.

So let's warn when we notice a select of a symbol that is not known in
the configuration we're creating.

Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
Acked-by: Michal Marek <mmarek@suse.cz>
---
First sent as an RFC in https://lkml.org/lkml/2014/9/26/887 (and
https://lkml.org/lkml/2014/9/26/886 for the cover letter).

Small modification to the commit explanation in comparison to the RFC,
but identical patch. Minor edits to the cover-letter too.

 scripts/kconfig/conf.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
index fef75fc756f4..de8406287531 100644
--- a/scripts/kconfig/conf.c
+++ b/scripts/kconfig/conf.c
@@ -446,6 +446,45 @@ static void check_conf(struct menu *menu)
 		check_conf(child);
 }
 
+static void check_selects(struct menu *menu)
+{
+	struct symbol *sym, *sel;
+	struct property *prop;
+
+	while (menu) {
+		sym = menu->sym;
+
+		if (sym && !sym_is_choice(sym) &&
+		    sym->type != S_UNKNOWN &&
+		    sym->flags & SYMBOL_WRITE) {
+			for_all_properties(sym, prop, P_SELECT) {
+				sel = prop->expr->left.sym;
+				if (sel->type == S_UNKNOWN &&
+				    expr_calc_value(prop->visible.expr) != no) {
+					fprintf(stderr, "%s:%d:warning: ",
+						prop->file->name,
+						prop->lineno);
+					fprintf(stderr,
+						"'%s' selects unknown symbol '%s'\n",
+						sym->name,
+						sel->name);
+				}
+			}
+		}
+
+		if (menu->list) {
+			menu = menu->list;
+		} else if (menu->next) {
+			menu = menu->next;
+		} else while ((menu = menu->parent)) {
+			if (menu->next) {
+				menu = menu->next;
+				break;
+			}
+		}
+	}
+}
+
 static struct option long_opts[] = {
 	{"oldaskconfig",    no_argument,       NULL, oldaskconfig},
 	{"oldconfig",       no_argument,       NULL, oldconfig},
@@ -681,6 +720,8 @@ int main(int ac, char **av)
 		break;
 	}
 
+	check_selects(rootmenu.list);
+
 	if (sync_kconfig) {
 		/* silentoldconfig is used during the build so we shall update autoconf.
 		 * All other commands are only used to generate a config.
-- 
1.9.3


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/1] kconfig: warn if an unknown symbol is selected
  2014-09-30 18:09 [PATCH 1/1] kconfig: warn if an unknown symbol is selected Paul Bolle
@ 2014-11-03 11:38 ` Paul Bolle
       [not found]   ` <1422438897.5666.23.camel@x220>
  0 siblings, 1 reply; 5+ messages in thread
From: Paul Bolle @ 2014-11-03 11:38 UTC (permalink / raw)
  To: Yann E. MORIN; +Cc: Michal Marek, linux-kbuild, linux-kernel

Hi Yann,

On Tue, 2014-09-30 at 20:09 +0200, Paul Bolle wrote:
> A select of an unknown symbol is basically treated as a nop and is
> silently skipped. This is annoying if the selected symbol contains a
> typo. It can also hide the fact that a treewide symbol cleanup was only
> done partially.
> 
> There are also a few cases were this might have been done on purpose.
> But that anti-pattern should be discouraged. Almost all select
> statements point to a known and reachable symbol. So people will likely
> assume that any selected symbol is actually set. Selects that violate
> this assumption can only be spotted by checking multiple Kconfig files,
> often across architectures. It is unlikely that people will do this
> regularly.
> 
> So let's warn when we notice a select of a symbol that is not known in
> the configuration we're creating.
> 
> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> Acked-by: Michal Marek <mmarek@suse.cz>
> ---
> First sent as an RFC in https://lkml.org/lkml/2014/9/26/887 (and
> https://lkml.org/lkml/2014/9/26/886 for the cover letter).
> 
> Small modification to the commit explanation in comparison to the RFC,
> but identical patch. Minor edits to the cover-letter too.

I've gotten no reaction from you yet on this patch nor on the preceding
RFC. There have been a handful of other kconfig patches flying by the
last half year or so (at least, that I know of). None of those have
gotten any reaction from you, as far as I can tell.

Do you expect to handle this patch (and whatever has been submitted
lately by other people) in the near future?

>  scripts/kconfig/conf.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
> index fef75fc756f4..de8406287531 100644
> --- a/scripts/kconfig/conf.c
> +++ b/scripts/kconfig/conf.c
> @@ -446,6 +446,45 @@ static void check_conf(struct menu *menu)
>  		check_conf(child);
>  }
>  
> +static void check_selects(struct menu *menu)
> +{
> +	struct symbol *sym, *sel;
> +	struct property *prop;
> +
> +	while (menu) {
> +		sym = menu->sym;
> +
> +		if (sym && !sym_is_choice(sym) &&
> +		    sym->type != S_UNKNOWN &&
> +		    sym->flags & SYMBOL_WRITE) {
> +			for_all_properties(sym, prop, P_SELECT) {
> +				sel = prop->expr->left.sym;
> +				if (sel->type == S_UNKNOWN &&
> +				    expr_calc_value(prop->visible.expr) != no) {
> +					fprintf(stderr, "%s:%d:warning: ",
> +						prop->file->name,
> +						prop->lineno);
> +					fprintf(stderr,
> +						"'%s' selects unknown symbol '%s'\n",
> +						sym->name,
> +						sel->name);
> +				}
> +			}
> +		}
> +
> +		if (menu->list) {
> +			menu = menu->list;
> +		} else if (menu->next) {
> +			menu = menu->next;
> +		} else while ((menu = menu->parent)) {
> +			if (menu->next) {
> +				menu = menu->next;
> +				break;
> +			}
> +		}
> +	}
> +}
> +
>  static struct option long_opts[] = {
>  	{"oldaskconfig",    no_argument,       NULL, oldaskconfig},
>  	{"oldconfig",       no_argument,       NULL, oldconfig},
> @@ -681,6 +720,8 @@ int main(int ac, char **av)
>  		break;
>  	}
>  
> +	check_selects(rootmenu.list);
> +
>  	if (sync_kconfig) {
>  		/* silentoldconfig is used during the build so we shall update autoconf.
>  		 * All other commands are only used to generate a config.

Regards,


Paul Bolle


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/1] kconfig: warn if an unknown symbol is selected
       [not found]   ` <1422438897.5666.23.camel@x220>
@ 2015-01-28 15:26     ` Michal Marek
  2015-01-28 21:32       ` Paul Bolle
  0 siblings, 1 reply; 5+ messages in thread
From: Michal Marek @ 2015-01-28 15:26 UTC (permalink / raw)
  To: Paul Bolle; +Cc: linux-kbuild, linux-kernel

On 2015-01-28 10:54, Paul Bolle wrote:
> Now that we've been told Yann has disappeared, would you consider taking
> this patch into one of your trees? It would be nice to have people
> actually use it for a while.
> 
> Or should we first clean up (most of) the warnings it generates? There

It seems your fixes have been accepted in the meantime, but there is one
new:
*** Default configuration is based on 'x86_64_defconfig'
drivers/misc/cxl/Kconfig:8:warning: 'CXL_BASE' selects unknown symbol
'PPC_COPRO_BASE'

PPC_COPRO_BASE is not known on x86, but at the same time, there is no
way for CXL_BASE to be selected on x86:

config CXL_BASE
	bool
	default n
	select PPC_COPRO_BASE

config CXL
	tristate "Support for IBM Coherent Accelerators (CXL)"
	depends on PPC_POWERNV && PCI_MSI
	select CXL_BASE
	default m

Shouldn't we only warn about a select when it is triggered? An
allyesconfig would still report all bogus selects for given architecture.

Michal

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/1] kconfig: warn if an unknown symbol is selected
  2015-01-28 15:26     ` Michal Marek
@ 2015-01-28 21:32       ` Paul Bolle
  2015-01-29 12:39         ` Michal Marek
  0 siblings, 1 reply; 5+ messages in thread
From: Paul Bolle @ 2015-01-28 21:32 UTC (permalink / raw)
  To: Michal Marek; +Cc: linux-kbuild, linux-kernel

On Wed, 2015-01-28 at 16:26 +0100, Michal Marek wrote:
> On 2015-01-28 10:54, Paul Bolle wrote:
> > Now that we've been told Yann has disappeared, would you consider taking
> > this patch into one of your trees? It would be nice to have people
> > actually use it for a while.
> > 
> > Or should we first clean up (most of) the warnings it generates? There
> 
> It seems your fixes have been accepted in the meantime, but there is one
> new:
> *** Default configuration is based on 'x86_64_defconfig'
> drivers/misc/cxl/Kconfig:8:warning: 'CXL_BASE' selects unknown symbol
> 'PPC_COPRO_BASE'
> 
> PPC_COPRO_BASE is not known on x86, but at the same time, there is no
> way for CXL_BASE to be selected on x86:
> 
> config CXL_BASE
> 	bool
> 	default n
> 	select PPC_COPRO_BASE
> 
> config CXL
> 	tristate "Support for IBM Coherent Accelerators (CXL)"
> 	depends on PPC_POWERNV && PCI_MSI
> 	select CXL_BASE
> 	default m
> 
> Shouldn't we only warn about a select when it is triggered? An
> allyesconfig would still report all bogus selects for given architecture.

Good catch! I think checking only for selects done by Kconfig symbols
that are not "n" might do the trick. Expect a v2 one of these days.

The current load of issues (for allyesconfig on all arches except um) is
pasted at the bottom of this message (with some comments added). Looking
at that load I think it might be preferable to first most of fix those
issues before adding this to kconfig.


Paul Bolle

alpha:
drivers/remoteproc/Kconfig:10:warning: 'REMOTEPROC' selects unknown symbol 'VIRTUALIZATION'

REMOTEPROC should be ARM or OMAP specific?

lib/Kconfig.debug:1024:warning: 'DEBUG_ATOMIC_SLEEP' selects unknown symbol 'PREEMPT_COUNT'

Perhaps fixed by selecting PREEMPT_COUNT only for architectures that
support it.

arc:
drivers/remoteproc/Kconfig:10:warning: 'REMOTEPROC' selects unknown symbol 'VIRTUALIZATION'
arm:
arch/arm/mach-mvebu/Kconfig:40:warning: 'MACH_ARMADA_375' selects unknown symbol 'ARM_ERRATA_753970'
arch/arm/mach-mvebu/Kconfig:55:warning: 'MACH_ARMADA_38X' selects unknown symbol 'ARM_ERRATA_753970'

I'm trying for quite some time to get this fixed.

arm64:
arch/arm64/Kconfig:183:warning: 'ARCH_TEGRA' selects unknown symbol 'HAVE_SMP'
arch/arm64/Kconfig:193:warning: 'ARCH_TEGRA_132_SOC' selects unknown symbol 'USB_ARCH_HAS_EHCI'

Patches pending, I've been told.

drivers/irqchip/Kconfig:9:warning: 'ARM_GIC' selects unknown symbol 'MULTI_IRQ_HANDLER'
drivers/irqchip/Kconfig:23:warning: 'ARM_GIC_V3' selects unknown symbol 'MULTI_IRQ_HANDLER'

MULTI_IRQ_HANDLER is arm specific.

avr32:
drivers/remoteproc/Kconfig:10:warning: 'REMOTEPROC' selects unknown symbol 'VIRTUALIZATION'
blackfin:
drivers/remoteproc/Kconfig:10:warning: 'REMOTEPROC' selects unknown symbol 'VIRTUALIZATION'
c6x:
drivers/remoteproc/Kconfig:10:warning: 'REMOTEPROC' selects unknown symbol 'VIRTUALIZATION'
cris:
drivers/remoteproc/Kconfig:10:warning: 'REMOTEPROC' selects unknown symbol 'VIRTUALIZATION'
frv:
drivers/remoteproc/Kconfig:10:warning: 'REMOTEPROC' selects unknown symbol 'VIRTUALIZATION'
lib/Kconfig.debug:1024:warning: 'DEBUG_ATOMIC_SLEEP' selects unknown symbol 'PREEMPT_COUNT'
hexagon:
arch/hexagon/Kconfig:22:warning: 'HEXAGON' selects unknown symbol 'NO_IOPORT_MAP'

Fixing obscure architectures requires patience!

drivers/remoteproc/Kconfig:10:warning: 'REMOTEPROC' selects unknown symbol 'VIRTUALIZATION'
lib/Kconfig.debug:1024:warning: 'DEBUG_ATOMIC_SLEEP' selects unknown symbol 'PREEMPT_COUNT'
ia64:
drivers/acpi/Kconfig:165:warning: 'ACPI_PROCESSOR' selects unknown symbol 'CPU_IDLE'

Perhaps, again, fixed by only selecting CPU_IDLE on architectures that
support it.

drivers/remoteproc/Kconfig:10:warning: 'REMOTEPROC' selects unknown symbol 'VIRTUALIZATION'
m32r:
m68k:
drivers/remoteproc/Kconfig:10:warning: 'REMOTEPROC' selects unknown symbol 'VIRTUALIZATION'
metag:
drivers/remoteproc/Kconfig:10:warning: 'REMOTEPROC' selects unknown symbol 'VIRTUALIZATION'
microblaze:
drivers/remoteproc/Kconfig:10:warning: 'REMOTEPROC' selects unknown symbol 'VIRTUALIZATION'
mips:
mn10300:
drivers/remoteproc/Kconfig:10:warning: 'REMOTEPROC' selects unknown symbol 'VIRTUALIZATION'
nios2:
drivers/remoteproc/Kconfig:10:warning: 'REMOTEPROC' selects unknown symbol 'VIRTUALIZATION'
openrisc:
drivers/remoteproc/Kconfig:10:warning: 'REMOTEPROC' selects unknown symbol 'VIRTUALIZATION'
parisc:
sound/pci/Kconfig:28:warning: 'SND_ALS300' selects unknown symbol 'ZONE_DMA'
sound/pci/Kconfig:53:warning: 'SND_ALI5451' selects unknown symbol 'ZONE_DMA'
sound/pci/Kconfig:158:warning: 'SND_AZT3328' selects unknown symbol 'ZONE_DMA'
sound/pci/Kconfig:466:warning: 'SND_EMU10K1' selects unknown symbol 'ZONE_DMA'
sound/pci/Kconfig:482:warning: 'SND_EMU10K1X' selects unknown symbol 'ZONE_DMA'
sound/pci/Kconfig:516:warning: 'SND_ES1938' selects unknown symbol 'ZONE_DMA'
sound/pci/Kconfig:528:warning: 'SND_ES1968' selects unknown symbol 'ZONE_DMA'
sound/pci/Kconfig:615:warning: 'SND_ICE1712' selects unknown symbol 'ZONE_DMA'
sound/pci/Kconfig:703:warning: 'SND_MAESTRO3' selects unknown symbol 'ZONE_DMA'
sound/pci/Kconfig:821:warning: 'SND_SONICVIBES' selects unknown symbol 'ZONE_DMA'
sound/pci/Kconfig:833:warning: 'SND_TRIDENT' selects unknown symbol 'ZONE_DMA'

I guess one or more cases of !PARISC might do the trick. But the
underlying problem is quite complicated. See commit 80ab8eae70e5 ("ALSA:
Enable CONFIG_ZONE_DMA for smaller PCI DMA masks"). Rather nice quote
from Andrew Morton about adding these selects (instead of fixing the
underlying problem):
    But then we'll just forget about it :(

(https://bugzilla.kernel.org/show_bug.cgi?id=68221#c19 ).

drivers/remoteproc/Kconfig:10:warning: 'REMOTEPROC' selects unknown symbol 'VIRTUALIZATION'
powerpc:
s390:
score:
arch/score/Kconfig:28:warning: 'MACH_SPCT6600' selects unknown symbol 'SYS_SUPPORTS_32BIT_KERNEL'

Fixing obscure architectures requires patience!

sh:
drivers/remoteproc/Kconfig:10:warning: 'REMOTEPROC' selects unknown symbol 'VIRTUALIZATION'
sparc:
drivers/remoteproc/Kconfig:10:warning: 'REMOTEPROC' selects unknown symbol 'VIRTUALIZATION'
tile:
unicore32:
drivers/remoteproc/Kconfig:10:warning: 'REMOTEPROC' selects unknown symbol 'VIRTUALIZATION'
x86:
xtensa:
drivers/remoteproc/Kconfig:10:warning: 'REMOTEPROC' selects unknown symbol 'VIRTUALIZATION'


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/1] kconfig: warn if an unknown symbol is selected
  2015-01-28 21:32       ` Paul Bolle
@ 2015-01-29 12:39         ` Michal Marek
  0 siblings, 0 replies; 5+ messages in thread
From: Michal Marek @ 2015-01-29 12:39 UTC (permalink / raw)
  To: Paul Bolle; +Cc: linux-kbuild, linux-kernel

On 2015-01-28 22:32, Paul Bolle wrote:
> On Wed, 2015-01-28 at 16:26 +0100, Michal Marek wrote:
>> On 2015-01-28 10:54, Paul Bolle wrote:
>>> Now that we've been told Yann has disappeared, would you consider taking
>>> this patch into one of your trees? It would be nice to have people
>>> actually use it for a while.
>>>
>>> Or should we first clean up (most of) the warnings it generates? There
>>
>> It seems your fixes have been accepted in the meantime, but there is one
>> new:
>> *** Default configuration is based on 'x86_64_defconfig'
>> drivers/misc/cxl/Kconfig:8:warning: 'CXL_BASE' selects unknown symbol
>> 'PPC_COPRO_BASE'
>>
>> PPC_COPRO_BASE is not known on x86, but at the same time, there is no
>> way for CXL_BASE to be selected on x86:
>>
>> config CXL_BASE
>> 	bool
>> 	default n
>> 	select PPC_COPRO_BASE
>>
>> config CXL
>> 	tristate "Support for IBM Coherent Accelerators (CXL)"
>> 	depends on PPC_POWERNV && PCI_MSI
>> 	select CXL_BASE
>> 	default m
>>
>> Shouldn't we only warn about a select when it is triggered? An
>> allyesconfig would still report all bogus selects for given architecture.
> 
> Good catch! I think checking only for selects done by Kconfig symbols
> that are not "n" might do the trick. Expect a v2 one of these days.
> 
> The current load of issues (for allyesconfig on all arches except um) is
> pasted at the bottom of this message (with some comments added). Looking
> at that load I think it might be preferable to first most of fix those
> issues before adding this to kconfig.

Maybe. OTOH, you have no warnings on x86 and powerpc and you already
have patches for the arm(64) warnings, so I think this can go in.

Michal

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2015-01-29 12:40 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-30 18:09 [PATCH 1/1] kconfig: warn if an unknown symbol is selected Paul Bolle
2014-11-03 11:38 ` Paul Bolle
     [not found]   ` <1422438897.5666.23.camel@x220>
2015-01-28 15:26     ` Michal Marek
2015-01-28 21:32       ` Paul Bolle
2015-01-29 12:39         ` Michal Marek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).