| Message ID | alpine.DEB.2.02.1403080857160.18573@ionos.tec.linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Mar 8, 2014 at 10:15 AM, Thomas Gleixner <tglx@linutronix.de> wrote: > On Fri, 7 Mar 2014, Linus Walleij wrote: >> I'll see what Jean-Jacques comes up with and take it from there unless >> he's interested in taking it all the way. > > Thought more about it while trying to come up with a persuasive > argument for the other Linus to take the irq_startup change that late > in the cycle: > > Using startup is the wrong point in __setup_irq() because we call > chip->irq_set_type() before we call chip->irq_startup(). And we want > this to be in that order to avoid spurious interrupts. > > Proper solution below. That leaves the startup/unmask logic untouched > and provides separate callbacks for this kind of requirements. That > makes it also simpler to have common functions for all gpios as you > don't have to do the mask/unmask dance in startup/shutdown. So you can > simply create gpio_irq_request/release_resources() and let the drivers > add those to their callbacks. > > If you agree, I put that into a separate branch based on an upstream > -rc so you can pull it into your gpiolib stuff and work from there. Yeah I really like the looks of this! Reviewed-by: Linus Walleij <linus.walleij@linaro.org> We need to think about whether the gpiolib changes are serious enough to be pushed this late in the -rc cycle though. The thing is that the flagging of GPIO lines as IRQs was to fix up the ages-old mess of sequential semantic dependencies between different unrelated gpiolib and irqchip calls, and these bugs have been around since the first irqchip was implemented in drivers/gpio I think :-( But if you prefer, I'll surely do it. Yours, Linus Walleij
On Wed, 12 Mar 2014, Linus Walleij wrote: > On Sat, Mar 8, 2014 at 10:15 AM, Thomas Gleixner <tglx@linutronix.de> wrote: > > > On Fri, 7 Mar 2014, Linus Walleij wrote: > >> I'll see what Jean-Jacques comes up with and take it from there unless > >> he's interested in taking it all the way. > > > > Thought more about it while trying to come up with a persuasive > > argument for the other Linus to take the irq_startup change that late > > in the cycle: > > > > Using startup is the wrong point in __setup_irq() because we call > > chip->irq_set_type() before we call chip->irq_startup(). And we want > > this to be in that order to avoid spurious interrupts. > > > > Proper solution below. That leaves the startup/unmask logic untouched > > and provides separate callbacks for this kind of requirements. That > > makes it also simpler to have common functions for all gpios as you > > don't have to do the mask/unmask dance in startup/shutdown. So you can > > simply create gpio_irq_request/release_resources() and let the drivers > > add those to their callbacks. > > > > If you agree, I put that into a separate branch based on an upstream > > -rc so you can pull it into your gpiolib stuff and work from there. > > Yeah I really like the looks of this! > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > We need to think about whether the gpiolib changes are serious > enough to be pushed this late in the -rc cycle though. > > The thing is that the flagging of GPIO lines as IRQs was to fix > up the ages-old mess of sequential semantic dependencies between > different unrelated gpiolib and irqchip calls, and these bugs have > been around since the first irqchip was implemented in > drivers/gpio I think :-( > > But if you prefer, I'll surely do it. Nah. We can just do that for 3.15. If people have the urge to backport it, it's simple enough to do so. I pushed out the patch to a separate branch git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip irq/for-gpio and merged that branch back into irq/core. So you can just pull irq/for-gpio into the gpio work and base the gpio related changes on top of that. Just mention it in the pull request to the other Linus that it contains an irq/core change which avoids merge dependencies. Thanks, tglx
On Wed, Mar 12, 2014 at 4:10 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > I pushed out the patch to a separate branch > > git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip irq/for-gpio > > and merged that branch back into irq/core. > > So you can just pull irq/for-gpio into the gpio work and base the gpio > related changes on top of that. Just mention it in the pull request to > the other Linus that it contains an irq/core change which avoids merge > dependencies. OK I first merged in the v3.14-rc6 as base and then pulled this in, so I will proceed to switch the calls over to request/release resources. Yours, Linus Walleij
Index: linux-2.6/include/linux/irq.h =================================================================== --- linux-2.6.orig/include/linux/irq.h +++ linux-2.6/include/linux/irq.h @@ -303,6 +303,10 @@ static inline irq_hw_number_t irqd_to_hw * @irq_pm_shutdown: function called from core code on shutdown once per chip * @irq_calc_mask: Optional function to set irq_data.mask for special cases * @irq_print_chip: optional to print special chip info in show_interrupts + * @irq_request_resources: optional to request resources before calling + * any other callback related to this irq + * @irq_release_resources: optional to release resources acquired with + * irq_request_resources * @flags: chip specific flags */ struct irq_chip { @@ -336,6 +340,8 @@ struct irq_chip { void (*irq_calc_mask)(struct irq_data *data); void (*irq_print_chip)(struct irq_data *data, struct seq_file *p); + int (*irq_request_resources)(struct irq_data *data); + void (*irq_release_resources)(struct irq_data *data); unsigned long flags; }; Index: linux-2.6/kernel/irq/manage.c =================================================================== --- linux-2.6.orig/kernel/irq/manage.c +++ linux-2.6/kernel/irq/manage.c @@ -897,6 +897,23 @@ static void irq_setup_forced_threading(s } } +static int irq_request_resources(struct irq_desc *desc) +{ + struct irq_data *d = &desc->irq_data; + struct irq_chip *c = d->chip; + + return c->irq_request_resources ? c->irq_request_resources(d) : 0; +} + +static void irq_release_resources(struct irq_desc *desc) +{ + struct irq_data *d = &desc->irq_data; + struct irq_chip *c = d->chip; + + if (c->irq_release_resources) + c->irq_release_resources(d); +} + /* * Internal function to register an irqaction - typically used to * allocate special interrupts that are part of the architecture. @@ -1092,6 +1109,13 @@ __setup_irq(unsigned int irq, struct irq } if (!shared) { + ret = irq_request_resources(desc); + if (ret) { + pr_err("Failed to request resources for %s (irq %d) on irqchip %s\n", + new->name, irq, desc->irq_data.chip->name); + goto out_mask; + } + init_waitqueue_head(&desc->wait_for_threads); /* Setup the type (level, edge polarity) if configured: */ @@ -1262,8 +1286,10 @@ static struct irqaction *__free_irq(unsi *action_ptr = action->next; /* If this was the last handler, shut down the IRQ line: */ - if (!desc->action) + if (!desc->action) { irq_shutdown(desc); + irq_release_resources(desc); + } #ifdef CONFIG_SMP /* make sure affinity_hint is cleaned up */