| Message ID | 4BA6E4D1.6080704@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Sun, Mar 21, 2010 at 08:32:33PM -0700, Yinghai Lu wrote: > diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c > index 64f6f20..cafd378 100644 > --- a/arch/powerpc/kernel/irq.c > +++ b/arch/powerpc/kernel/irq.c > @@ -1088,7 +1088,7 @@ int arch_early_irq_init(void) > return 0; > } > > -int arch_init_chip_data(struct irq_desc *desc, int node) > +int arch_init_irq_desc(struct irq_desc *desc, int node, init_chip_data_fn fn) > { > desc->status |= IRQ_NOREQUEST; > return 0; > > so this patch only change one line with powerpc code. This API seems to be going from bad to worse. Initially we had arch_init_chip_data(), which had precisely nothing to do with chip_data and only concerned itself with irq_desc, and now you're renaming it to something sensible but also trying to bolt on some ad-hoc chip_data relation at the same time thereby nullifying any benefit obtained from renaming the function in the first place. Renaming to xxx_irq_desc() while preserving the existing prototypes would make sense, even if it's ultimately just unecessary churn. > > It looks to me like this should just be done via a current machine > > vector or platform routine, in the same way as powerpc and (I think) > > ia64, ie: > > > >> +int arch_init_irq_desc(struct irq_desc *desc, int node) > >> +{ > >> + return current_machine->init_chip_data(desc, node); > >> +} > > > looks like xen in same run time, some irqs need x86_init_chip_data, > and some may need xen_init_chip_data later. > I'm afraid I am unable to grasp the meaning of this sentence, or what precisely this has to do with not being able to utilize platform ops to get this sorted out on x86. You're effectively trying to have the hardirq code pass around a function pointer for you that ultimately only serves to bail out on certain code paths if you're running under xen, which is a concern for how the platform chooses to initialize the irq desc, none of this has any value or relevance to the hardirq code outside of that. The fact that the hardirq code doesn't do anything with this information other than pass it around for your platform should already be a clear indicator that this is the wrong way to go. From a cursory look at the x86 code, this looks like precisely the sort of thing that arch/x86/include/asm/x86_init.h is well suited for, and indeed you already have a x86_init_irqs to expand on as needed. The function pointer thing itself is also a bit unorthodox to say the least. You're introducing and passing around an opaque type just so you can get to a 'return 0' in the xen case as far as I can tell, so you could also just make arch_init_chip_data() a weak symbol and clobber it in the xen case, no?
On Tue, 2010-03-23 at 07:10 +0000, Paul Mundt wrote: > The function pointer thing itself is also a bit unorthodox to say the > least. You're introducing and passing around an opaque type just so you > can get to a 'return 0' in the xen case as far as I can tell, The ultimate aim is to have Xen use the chip data to store the event channel information relating to each IRQ instead of keeping it in a static NR_IRQs array, the new function only returns 0 as a placeholder until this can be put in place (which depends on these changes), it could as well have been left out for the time being (e.g. passing NULL function pointer in the Xen case or whatever). > so you > could also just make arch_init_chip_data() a weak symbol and clobber it > in the xen case, no? A single kernel is able to boot native or Xen so a weak symbol doesn't really work, having the function pointer at the arch level is one similar option, I've replied to Thomas' similar suggestion. Ian.
diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c index 64f6f20..cafd378 100644 --- a/arch/powerpc/kernel/irq.c +++ b/arch/powerpc/kernel/irq.c @@ -1088,7 +1088,7 @@ int arch_early_irq_init(void) return 0; } -int arch_init_chip_data(struct irq_desc *desc, int node) +int arch_init_irq_desc(struct irq_desc *desc, int node, init_chip_data_fn fn) { desc->status |= IRQ_NOREQUEST; return 0;