From patchwork Fri May 13 08:09:09 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jean-Christophe PLAGNIOL-VILLARD X-Patchwork-Id: 781912 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by demeter1.kernel.org (8.14.4/8.14.3) with ESMTP id p4D8JP2L012118 for ; Fri, 13 May 2011 08:20:12 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757989Ab1EMIUL (ORCPT ); Fri, 13 May 2011 04:20:11 -0400 Received: from 64.mail-out.ovh.net ([91.121.185.65]:50406 "HELO 64.mail-out.ovh.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1757797Ab1EMIUJ (ORCPT ); Fri, 13 May 2011 04:20:09 -0400 Received: (qmail 8046 invoked by uid 503); 13 May 2011 07:49:39 -0000 Received: from b7.ovh.net (HELO mail31.ha.ovh.net) (213.186.33.57) by 64.mail-out.ovh.net with SMTP; 13 May 2011 07:49:39 -0000 Received: from b0.ovh.net (HELO queueout) (213.186.33.50) by b0.ovh.net with SMTP; 13 May 2011 10:20:06 +0200 Received: from ns32433.ovh.net (HELO localhost) (plagnioj%jcrosoft.com@213.251.161.87) by ns0.ovh.net with SMTP; 13 May 2011 10:20:03 +0200 Date: Fri, 13 May 2011 10:09:09 +0200 From: Jean-Christophe PLAGNIOL-VILLARD To: Michal Marek Cc: Arnaud Lacombe , linux-kernel@vger.kernel.org, linux-kbuild@vger.kernel.org, x86@kernel.org, Ingo Molnar Subject: Re: [PATCH v2] kconfig: autogenerated config_is_xxx macro Message-ID: <20110513080909.GO18952@game.jcrosoft.org> References: <1304658229-30820-1-git-send-email-plagnioj@jcrosoft.com> <20110507015041.GA21017@game.jcrosoft.org> <4DC7AB57.9050002@suse.cz> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <4DC7AB57.9050002@suse.cz> X-PGP-Key: http://uboot.jcrosoft.org/plagnioj.asc X-PGP-key-fingerprint: 6309 2BBA 16C8 3A07 1772 CC24 DEFC FFA3 279C CE7C User-Agent: Mutt/1.5.20 (2009-06-14) X-Ovh-Tracer-Id: 16839803433814174537 X-Ovh-Remote: 213.251.161.87 (ns32433.ovh.net) X-Ovh-Local: 213.186.33.20 (ns0.ovh.net) X-Spam-Check: DONE|U 0.5/N Sender: linux-kbuild-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kbuild@vger.kernel.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter1.kernel.org [140.211.167.41]); Fri, 13 May 2011 08:20:12 +0000 (UTC) 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_. 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_ notation inside a function > >>declaration, and CONFIG_ 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_ and use that > >>structure (or structure members) in config_is_ 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 Best Regards, J. 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