| Message ID | 20110513080909.GO18952@game.jcrosoft.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > -#ifdef CONFIG_PCI_BIOS > - if (!rt->signature) { > + if (config_is_pci_bios() && !rt->signature) { Makes sense - but please name it in a more obvious way, such as: pci_bios_enabled() Thanks, Ingo -- 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 10:30 Fri 13 May , Ingo Molnar wrote: > > * Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > > > -#ifdef CONFIG_PCI_BIOS > > - if (!rt->signature) { > > + if (config_is_pci_bios() && !rt->signature) { > > Makes sense - but please name it in a more obvious way, such as: > > pci_bios_enabled() the idea to generate the macro via Kconfig so I propose config_is_pci_bios() and if it's a module config_is_pci_bios_module() if you have a better convention naming I'm fine to change it maybe config_pci_bios_enabled() / config_pci_bios_module_enabled or pci_bios_enabled() / pci_bios_module_enabled() Best Regards, J. -- 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
* Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > On 10:30 Fri 13 May , Ingo Molnar wrote: > > > > * Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > > > > > -#ifdef CONFIG_PCI_BIOS > > > - if (!rt->signature) { > > > + if (config_is_pci_bios() && !rt->signature) { > > > > Makes sense - but please name it in a more obvious way, such as: > > > > pci_bios_enabled() > the idea to generate the macro via Kconfig Okay, and there we are stuck with whatever the Kconfig name is. (we could rename that but not needed really) Why not the canonical config_pci_bios() variant? It's the shortest one to write. The '_is' looks pretty superfluous to me. Hm, i guess it could be mixed up with a function that configures the pci_bios. I guess since i don't have any better idea config_is_pci_bios() sounds like a good choice after all. Thanks, Ingo -- 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 Fri, May 13, 2011 at 1:01 PM, Ingo Molnar <mingo@elte.hu> wrote: > > * Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > >> On 10:30 Fri 13 May , Ingo Molnar wrote: >> > >> > * Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: >> > >> > > -#ifdef CONFIG_PCI_BIOS >> > > - if (!rt->signature) { >> > > + if (config_is_pci_bios() && !rt->signature) { >> > >> > Makes sense - but please name it in a more obvious way, such as: >> > >> > pci_bios_enabled() >> the idea to generate the macro via Kconfig > > Okay, and there we are stuck with whatever the Kconfig name is. (we could > rename that but not needed really) > > Why not the canonical config_pci_bios() variant? It's the shortest one to > write. The '_is' looks pretty superfluous to me. > > Hm, i guess it could be mixed up with a function that configures the pci_bios. > > I guess since i don't have any better idea config_is_pci_bios() sounds like a > good choice after all. But we don't name config options like CONFIG_IS_PCI_BIOS, do we? One should lowercase config option to minimize confusion, nothing more if lowercased variant is OK. Why it looks like a function call? In fact one can even do if (CONFIG_PCI_BIOS && !rt->signature) { for boolean options. -- 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 13.5.2011 12:21, Alexey Dobriyan wrote: > Why it looks like a function call? > > In fact one can even do > > if (CONFIG_PCI_BIOS&& !rt->signature) { > > for boolean options. That unfortunately doesn't work, CONFIG_* macros are not defined if not enabled. 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
* Alexey Dobriyan <adobriyan@gmail.com> wrote: > On Fri, May 13, 2011 at 1:01 PM, Ingo Molnar <mingo@elte.hu> wrote: > > > > * Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > > > >> On 10:30 Fri 13 May , Ingo Molnar wrote: > >> > > >> > * Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > >> > > >> > > -#ifdef CONFIG_PCI_BIOS > >> > > - if (!rt->signature) { > >> > > + if (config_is_pci_bios() && !rt->signature) { > >> > > >> > Makes sense - but please name it in a more obvious way, such as: > >> > > >> > pci_bios_enabled() > >> the idea to generate the macro via Kconfig > > > > Okay, and there we are stuck with whatever the Kconfig name is. (we could > > rename that but not needed really) > > > > Why not the canonical config_pci_bios() variant? It's the shortest one to > > write. The '_is' looks pretty superfluous to me. > > > > Hm, i guess it could be mixed up with a function that configures the pci_bios. > > > > I guess since i don't have any better idea config_is_pci_bios() sounds like a > > good choice after all. > > But we don't name config options like CONFIG_IS_PCI_BIOS, do we? The problem is that 'config' can be misunderstood as a verb - it's often used for function names to say 'to configure'. By naming it 'config_is_()' it unambiguously becomes a noun. Thanks, Ingo -- 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
Hi, On Fri, May 13, 2011 at 4:09 AM, Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > On 10:52 Mon 09 May , Michal Marek wrote: >> On 7.5.2011 03:50, Jean-Christophe PLAGNIOL-VILLARD wrote: >> >On 12:19 Fri 06 May , Arnaud Lacombe wrote: >> >>Why would it be a good thing ? >> >> >> >>Most configuration-dependent code inside functions tends to be moved >> >>to a static inline already, which get conditionally defined based on >> >>the CONFIG_<foo>. If it is not, then the code is badly architectured >> >>(-> bad). Using that if(xxx) notation would also lead to yet more >> >>heavily indented function (-> bad). Moreover, this introduces >> >>yet-another way to check for an information (-> bad), and you will end >> >>up with mixing the config_is_<xxx> notation inside a function >> >>declaration, and CONFIG_<xxx> when not inside a function (-> bad) >> >> >> >>Actually, this is even worse than that as you'll not be able to hide >> >>structure (or structure members) inside CONFIG_<xxx> and use that >> >>structure (or structure members) in config_is_<xxx> protected block >> >>without causing compile-time failure. >> >sorry but conditionnal structure members is bad practice >> >you save nearly no space nut for the test of the code in multiple >> >configuration. Use union for this. >> > >> >the compile-time failure is good here. it's means your code is not generic. >> > >> >specially when you want to keep code running on multiple soc/arch keep compiling >> >no matter the configuration >> > >> >#ifdef in the code is a really bad habit >> >> Do you have proof of concept patches that make use of the >> config_is_xxx macros? Acked by the respective subsystem maintainers? >> It would be a good idea to send them along to show that this feature >> is going to be actually used. > I've seen thousands of place in the kernel we can use > so I'll just take one example on x86 > > the patch attached is just an example > An you get a nice build error, at least from: void pcibios_penalize_isa_irq(int irq, int active) { -#ifdef CONFIG_ACPI - if (!acpi_noirq) + if (config_is_pci_bios() && !acpi_noirq) acpi_penalize_isa_irq(irq, active); else -#endif pirq_penalize_isa_irq(irq, active); } as acpi_penalize_isa_irq() is only declared if CONFIG_ACPI is. So be prepared to fix a lot of code. I don't really care about the good or the bad, of each solution. These are just tools, they are not intrinsically good or bad, only their (over/under)usage might eventually get criticized. To further extend, I am not sure you can keep x86-64 and x86-32 merged in the same `arch/x86' tree without using a single #ifdef in struct definition and function declaration. - Arnaud > Best Regards, > J. > -- 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 14:21 Fri 13 May , Ingo Molnar wrote: > > * Alexey Dobriyan <adobriyan@gmail.com> wrote: > > > On Fri, May 13, 2011 at 1:01 PM, Ingo Molnar <mingo@elte.hu> wrote: > > > > > > * Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > > > > > >> On 10:30 Fri 13 May , Ingo Molnar wrote: > > >> > > > >> > * Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > > >> > > > >> > > -#ifdef CONFIG_PCI_BIOS > > >> > > - if (!rt->signature) { > > >> > > + if (config_is_pci_bios() && !rt->signature) { > > >> > > > >> > Makes sense - but please name it in a more obvious way, such as: > > >> > > > >> > pci_bios_enabled() > > >> the idea to generate the macro via Kconfig > > > > > > Okay, and there we are stuck with whatever the Kconfig name is. (we could > > > rename that but not needed really) > > > > > > Why not the canonical config_pci_bios() variant? It's the shortest one to > > > write. The '_is' looks pretty superfluous to me. > > > > > > Hm, i guess it could be mixed up with a function that configures the pci_bios. > > > > > > I guess since i don't have any better idea config_is_pci_bios() sounds like a > > > good choice after all. > > > > But we don't name config options like CONFIG_IS_PCI_BIOS, do we? > > The problem is that 'config' can be misunderstood as a verb - it's often used > for function names to say 'to configure'. > > By naming it 'config_is_()' it unambiguously becomes a noun. with Andi and Ingo ack can I assume this patch go the -next and the .40 if yes I rebase some of my patch against it for at91 I'll regulary send patch to switch to it Best Regards, J. -- 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 Fri, 13 May 2011 10:09:09 +0200, Jean-Christophe PLAGNIOL-VILLARD said: > On 10:52 Mon 09 May , Michal Marek wrote: > > Do you have proof of concept patches that make use of the > > config_is_xxx macros? Acked by the respective subsystem maintainers? > > It would be a good idea to send them along to show that this feature > > is going to be actually used. > I've seen thousands of place in the kernel we can use > so I'll just take one example on x86 > > the patch attached is just an example Out of curiosity, will this Do The Right Thing for cases where things simply won't build for some configs? For example, consider this code snippet from kernel/timer.c, in __mod_timer() (near line 682): debug_activate(timer, expires); cpu = smp_processor_id(); #if defined(CONFIG_NO_HZ) && defined(CONFIG_SMP) if (!pinned && get_sysctl_timer_migration() && idle_cpu(cpu)) cpu = get_nohz_timer_target(); #endif new_base = per_cpu(tvec_bases, cpu); If you convert this to an if statement, will it still compile? Which will happen first, dead code elimination, or the warning that get_nohz_timer_target() is an implicit declaration because the definition in the .h file is also guarded by #ifdef CONFIG_NO_HZ?
diff --git a/arch/x86/pci/irq.c b/arch/x86/pci/irq.c index 8201165..ce4bd8a 100644 --- a/arch/x86/pci/irq.c +++ b/arch/x86/pci/irq.c @@ -527,14 +527,14 @@ static int pirq_pico_set(struct pci_dev *router, struct pci_dev *dev, int pirq, } #ifdef CONFIG_PCI_BIOS - static int pirq_bios_set(struct pci_dev *router, struct pci_dev *dev, int pirq, int irq) { struct pci_dev *bridge; int pin = pci_get_interrupt_pin(dev, &bridge); return pcibios_set_irq_routing(bridge, pin - 1, irq); } - +#else +static int pirq_bios_set(struct pci_dev *router, struct pci_dev *dev, int pirq, int irq) {} #endif static __init int intel_router_probe(struct irq_router *r, struct pci_dev *router, u16 device) @@ -821,14 +821,12 @@ static void __init pirq_find_router(struct irq_router *r) struct irq_routing_table *rt = pirq_table; struct irq_router_handler *h; -#ifdef CONFIG_PCI_BIOS - if (!rt->signature) { + if (config_is_pci_bios() && !rt->signature) { printk(KERN_INFO "PCI: Using BIOS for IRQ routing\n"); r->set = pirq_bios_set; r->name = "BIOS"; return; } -#endif /* Default unless a driver reloads it */ r->name = "default"; @@ -1126,10 +1124,10 @@ void __init pcibios_irq_init(void) pirq_table = pirq_find_routing_table(); -#ifdef CONFIG_PCI_BIOS - if (!pirq_table && (pci_probe & PCI_BIOS_IRQ_SCAN)) + if (config_is_pci_bios() && + (!pirq_table && (pci_probe & PCI_BIOS_IRQ_SCAN))) pirq_table = pcibios_get_irq_routing_table(); -#endif + if (pirq_table) { pirq_peer_trick(); pirq_find_router(&pirq_router); @@ -1178,11 +1176,9 @@ static void pirq_penalize_isa_irq(int irq, int active) void pcibios_penalize_isa_irq(int irq, int active) { -#ifdef CONFIG_ACPI - if (!acpi_noirq) + if (config_is_pci_bios() && !acpi_noirq) acpi_penalize_isa_irq(irq, active); else -#endif pirq_penalize_isa_irq(irq, active); } @@ -1197,8 +1193,7 @@ static int pirq_enable_irq(struct pci_dev *dev) if (!io_apic_assign_pci_irqs && dev->irq) return 0; - if (io_apic_assign_pci_irqs) { -#ifdef CONFIG_X86_IO_APIC + if (config_is_x86_io_apic() && io_apic_assign_pci_irqs) { struct pci_dev *temp_dev; int irq; struct io_apic_irq_attr irq_attr; @@ -1237,7 +1232,6 @@ static int pirq_enable_irq(struct pci_dev *dev) return 0; } else msg = "; probably buggy MP table"; -#endif } else if (pci_probe & PCI_BIOS_IRQ_SCAN) msg = ""; else