| Message ID | 1412100583.21730.31.camel@x220 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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' -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.