| Message ID | 1409662853-29313-2-git-send-email-daniel.thompson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 02, 2014 at 02:00:35PM +0100, Daniel Thompson wrote: > void enable_fiq(int fiq) > { > + struct fiq_data *data = lookup_fiq_data(fiq); > + > + if (data) { > + if (data->fiq_chip->fiq_enable) > + data->fiq_chip->fiq_enable(data->irq_data); > + enable_irq(fiq); Why do we call the FIQ chip's enable and enable_irq() as well? > void disable_fiq(int fiq) > { > + struct fiq_data *data = lookup_fiq_data(fiq); > + > + if (data) { > + if (data->fiq_chip->fiq_disable) > + data->fiq_chip->fiq_disable(data->irq_data); > + disable_irq(fiq); Same question here. > +bool has_fiq(int fiq) > +{ > + struct fiq_data *data = lookup_fiq_data(fiq); > + > + if (data) > + return true; > + > + if (fiq_start == -1) > + return false; > + > + return fiq >= fiq_start; Are you sure this is correct... it looks wrong to me.
On Tue, 2 Sep 2014, Daniel Thompson wrote: > Currently enable_fiq/disable_fiq use a simple offset to convert an IRQ > virq into a FIQ virq. This is too inflexible for multi-platform kernels > and makes runtime error checking impossible. You are missing to explain WHY it is too inflexible .... > We solve this by introducing a flexible mapping that allows interrupt Flexible in that context reads like random semantics. And that's definitely nothing we want to see in the context of interrupts/fiqs > controllers that support FIQ to register those mappings. This, in turn, > makes it much possible for drivers in DT kernels to install FIQ handlers > without knowing anything about the interrupt controller. > +#include <linux/irq.h> > #include <asm/ptrace.h> > > +struct fiq_chip { > + void (*fiq_enable)(struct irq_data *data); > + void (*fiq_disable)(struct irq_data *data); > +}; So you define an ARM specific extension to the irq infrastructure which is completely ignoring that there might be other architectures which have the same issue? Aside of that you define that structure w/o documenting in any way what the semantics of the callbacks are and how struct irq_data is supposed to be (ab)used there. > +struct fiq_data { > + struct fiq_chip *fiq_chip; > + struct irq_data *irq_data; > +}; Nice. Another completely undocumented data structure. > static unsigned long no_fiq_insn; > +static int fiq_start = -1; > +static RADIX_TREE(fiq_data_tree, GFP_KERNEL); Along with a radix tree, which again lacks any form of documentation. > -static int fiq_start; > +static struct fiq_data *lookup_fiq_data(int fiq) > +{ > + struct fiq_data *data; > + > + rcu_read_lock(); > + data = radix_tree_lookup(&fiq_data_tree, fiq); > + rcu_read_unlock(); > + > + return data; That makes a lot of sense. NOT! What kind of protection is that rcu_read_lock/unlock pair providing for the return value? Just that it has been valid at lookup time? What protects against concurrent calls to the various usage sites which shine by the lack of serialization ... > void enable_fiq(int fiq) > { > + struct fiq_data *data = lookup_fiq_data(fiq); > + > + if (data) { > + if (data->fiq_chip->fiq_enable) > + data->fiq_chip->fiq_enable(data->irq_data); > + enable_irq(fiq); This is ass backwards, really. The FIQ target is a property of the interrupt line right? So why the heck do you need a separate chip/data representation including a radix tree and whatever? Just because you can? What is wrong to tell the irq core code via a flag that a particular interrupt should be targeted at FIQ instead of a regular interrupt? Nothing as far as I can tell. And that would even allow to retarget an already enabled interrupt to FIQ or vice versa. So all your patch does is trying to (ab)use an existing interface which was created in the last millenium for totally different reasons and in a totally different context. But you fail completely to understand what that interface is doing: It merily maps the Linux virq to a FIQ via an offset. While I understand that this is not workable on multiplatform kernels, I really cannot understand your attempt to change this by adding complex functionality to a primitive offset mapping function. You just add some randomly defined new chip/data combination which abuses the irq_data structure under the hood instead of properly integrating that irq chip property into irqchip/irqdata itself. So instead of thinking about the properties of the hardware and questioning the existing mechanism, you just bolt some crap onto it and inflict a gazillion of pointless modifications to the irq chip drivers without any value. So all it needs is 1) Adding a property flag to the irq chip which signals that it supports FIQ/NMI targets 2) Adding a interface which allows to flag a particular interrupt line targeted to either the regular or the FIQ/NMI which consults the flag added by #1 3) Let the chip driver deal with the target flag at a minimal intrusive level w/o registering racy callbacks designed in hell and imposing arch specific data structures, storage etc. for no value Thanks, tglx
On 03/09/14 01:03, Thomas Gleixner wrote: > On Tue, 2 Sep 2014, Daniel Thompson wrote: > >> Currently enable_fiq/disable_fiq use a simple offset to convert an IRQ >> virq into a FIQ virq. This is too inflexible for multi-platform kernels >> and makes runtime error checking impossible. > > You are missing to explain WHY it is too inflexible .... > >> We solve this by introducing a flexible mapping that allows interrupt > > Flexible in that context reads like random semantics. And that's > definitely nothing we want to see in the context of interrupts/fiqs > >> controllers that support FIQ to register those mappings. This, in turn, >> makes it much possible for drivers in DT kernels to install FIQ handlers >> without knowing anything about the interrupt controller. > >> +#include <linux/irq.h> >> #include <asm/ptrace.h> >> >> +struct fiq_chip { >> + void (*fiq_enable)(struct irq_data *data); >> + void (*fiq_disable)(struct irq_data *data); >> +}; > > So you define an ARM specific extension to the irq infrastructure > which is completely ignoring that there might be other architectures > which have the same issue? > > Aside of that you define that structure w/o documenting in any way > what the semantics of the callbacks are and how struct irq_data is > supposed to be (ab)used there. > >> +struct fiq_data { >> + struct fiq_chip *fiq_chip; >> + struct irq_data *irq_data; >> +}; > > Nice. Another completely undocumented data structure. > >> static unsigned long no_fiq_insn; >> +static int fiq_start = -1; >> +static RADIX_TREE(fiq_data_tree, GFP_KERNEL); > > Along with a radix tree, which again lacks any form of documentation. > >> -static int fiq_start; >> +static struct fiq_data *lookup_fiq_data(int fiq) >> +{ >> + struct fiq_data *data; >> + >> + rcu_read_lock(); >> + data = radix_tree_lookup(&fiq_data_tree, fiq); >> + rcu_read_unlock(); >> + >> + return data; > > That makes a lot of sense. NOT! > > What kind of protection is that rcu_read_lock/unlock pair providing > for the return value? Just that it has been valid at lookup time? > > What protects against concurrent calls to the various usage sites > which shine by the lack of serialization ... The locking here ought to safe because there is no means to unregister fiq data (the two irqchip drivers I looked at has no need for that) so once looked up the data will be valid. Thus it's only purpose was to prevent incorrect traversals of the radix tree. That said, this doesn't make it right. >> void enable_fiq(int fiq) >> { >> + struct fiq_data *data = lookup_fiq_data(fiq); >> + >> + if (data) { >> + if (data->fiq_chip->fiq_enable) >> + data->fiq_chip->fiq_enable(data->irq_data); >> + enable_irq(fiq); > > This is ass backwards, really. > > The FIQ target is a property of the interrupt line right? > > So why the heck do you need a separate chip/data representation > including a radix tree and whatever? Just because you can? > > What is wrong to tell the irq core code via a flag that a particular > interrupt should be targeted at FIQ instead of a regular interrupt? > > Nothing as far as I can tell. And that would even allow to retarget an > already enabled interrupt to FIQ or vice versa. > > So all your patch does is trying to (ab)use an existing interface > which was created in the last millenium for totally different reasons > and in a totally different context. > > But you fail completely to understand what that interface is doing: > > It merily maps the Linux virq to a FIQ via an offset. > > While I understand that this is not workable on multiplatform kernels, > I really cannot understand your attempt to change this by adding > complex functionality to a primitive offset mapping function. > > You just add some randomly defined new chip/data combination which > abuses the irq_data structure under the hood instead of properly > integrating that irq chip property into irqchip/irqdata itself. > > So instead of thinking about the properties of the hardware and > questioning the existing mechanism, you just bolt some crap onto it > and inflict a gazillion of pointless modifications to the irq chip > drivers without any value. > > So all it needs is > > 1) Adding a property flag to the irq chip which signals that it > supports FIQ/NMI targets > > 2) Adding a interface which allows to flag a particular interrupt > line targeted to either the regular or the FIQ/NMI which consults > the flag added by #1 > > 3) Let the chip driver deal with the target flag at a minimal > intrusive level w/o registering racy callbacks designed in hell > and imposing arch specific data structures, storage etc. for no > value Thanks for the comments. I'll get working along these lines.
diff --git a/arch/arm/include/asm/fiq.h b/arch/arm/include/asm/fiq.h index d493d0b..ed44528 100644 --- a/arch/arm/include/asm/fiq.h +++ b/arch/arm/include/asm/fiq.h @@ -16,8 +16,14 @@ #ifndef __ASM_FIQ_H #define __ASM_FIQ_H +#include <linux/irq.h> #include <asm/ptrace.h> +struct fiq_chip { + void (*fiq_enable)(struct irq_data *data); + void (*fiq_disable)(struct irq_data *data); +}; + struct fiq_handler { struct fiq_handler *next; /* Name @@ -38,6 +44,8 @@ extern void release_fiq(struct fiq_handler *f); extern void set_fiq_handler(void *start, unsigned int length); extern void enable_fiq(int fiq); extern void disable_fiq(int fiq); +extern bool has_fiq(int fiq); +extern void fiq_register_mapping(int irq, struct fiq_chip *chip); /* helpers defined in fiqasm.S: */ extern void __set_fiq_regs(unsigned long const *regs); diff --git a/arch/arm/kernel/fiq.c b/arch/arm/kernel/fiq.c index 918875d..5d831cf 100644 --- a/arch/arm/kernel/fiq.c +++ b/arch/arm/kernel/fiq.c @@ -40,6 +40,9 @@ #include <linux/init.h> #include <linux/interrupt.h> #include <linux/seq_file.h> +#include <linux/irq.h> +#include <linux/radix-tree.h> +#include <linux/slab.h> #include <asm/cacheflush.h> #include <asm/cp15.h> @@ -52,7 +55,15 @@ (unsigned)&vector_fiq_offset; \ }) +struct fiq_data { + struct fiq_chip *fiq_chip; + struct irq_data *irq_data; +}; + static unsigned long no_fiq_insn; +static int fiq_start = -1; +static RADIX_TREE(fiq_data_tree, GFP_KERNEL); +static DEFINE_MUTEX(fiq_data_mutex); /* Default reacquire function * - we always relinquish FIQ control @@ -127,18 +138,65 @@ void release_fiq(struct fiq_handler *f) while (current_fiq->fiq_op(current_fiq->dev_id, 0)); } -static int fiq_start; +static struct fiq_data *lookup_fiq_data(int fiq) +{ + struct fiq_data *data; + + rcu_read_lock(); + data = radix_tree_lookup(&fiq_data_tree, fiq); + rcu_read_unlock(); + + return data; +} void enable_fiq(int fiq) { + struct fiq_data *data = lookup_fiq_data(fiq); + + if (data) { + if (data->fiq_chip->fiq_enable) + data->fiq_chip->fiq_enable(data->irq_data); + enable_irq(fiq); + return; + } + + if (WARN_ON(fiq_start == -1)) + return; + enable_irq(fiq + fiq_start); } void disable_fiq(int fiq) { + struct fiq_data *data = lookup_fiq_data(fiq); + + if (data) { + if (data->fiq_chip->fiq_disable) + data->fiq_chip->fiq_disable(data->irq_data); + disable_irq(fiq); + return; + } + + if (WARN_ON(fiq_start == -1)) + return; + disable_irq(fiq + fiq_start); } +bool has_fiq(int fiq) +{ + struct fiq_data *data = lookup_fiq_data(fiq); + + if (data) + return true; + + if (fiq_start == -1) + return false; + + return fiq >= fiq_start; +} +EXPORT_SYMBOL(has_fiq); + EXPORT_SYMBOL(set_fiq_handler); EXPORT_SYMBOL(__set_fiq_regs); /* defined in fiqasm.S */ EXPORT_SYMBOL(__get_fiq_regs); /* defined in fiqasm.S */ @@ -147,9 +205,50 @@ EXPORT_SYMBOL(release_fiq); EXPORT_SYMBOL(enable_fiq); EXPORT_SYMBOL(disable_fiq); +/* + * Add a mapping from a Linux irq to the fiq data. + */ +void fiq_register_mapping(int irq, struct fiq_chip *chip) +{ + struct fiq_data *fiq_data = NULL; + int res; + + /* fiq_register_mapping can't be mixed with init_FIQ */ + BUG_ON(fiq_start != -1); + + fiq_data = kmalloc(sizeof(*fiq_data), GFP_KERNEL); + if (!fiq_data) + goto err; + + fiq_data->fiq_chip = chip; + fiq_data->irq_data = irq_get_irq_data(irq); + BUG_ON(!fiq_data->irq_data); + + mutex_lock(&fiq_data_mutex); + res = radix_tree_insert(&fiq_data_tree, irq, fiq_data); + mutex_unlock(&fiq_data_mutex); + if (res) + goto err; + + return; + +err: + kfree(fiq_data); + pr_err("fiq: Cannot register mapping %d\n", irq); +} + +/* + * Set the offset between normal IRQs and their FIQ shadows. + */ void __init init_FIQ(int start) { + fiq_start = start; +} + +static int __init init_default_fiq_handler(void) +{ unsigned offset = FIQ_OFFSET; no_fiq_insn = *(unsigned long *)(0xffff0000 + offset); - fiq_start = start; + return 0; } +pure_initcall(init_default_fiq_handler);