| Message ID | 1391721117-27446-1-git-send-email-carlo@caione.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 6 Feb 2014, Carlo Caione wrote: > Several irqchip drivers require the level-triggered interrupt to be > acked before unmasking to avoid that a second interrupt is immediately > triggered. This small patch introduces a new irqchip flags that is used > to ack the IRQ line before it is unmasked. And why are you not doing this in the unmask function of the affected chip in the first place? Thanks, tglx
On Thu, Feb 6, 2014 at 10:14 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > On Thu, 6 Feb 2014, Carlo Caione wrote: > >> Several irqchip drivers require the level-triggered interrupt to be >> acked before unmasking to avoid that a second interrupt is immediately >> triggered. This small patch introduces a new irqchip flags that is used >> to ack the IRQ line before it is unmasked. > > And why are you not doing this in the unmask function of the affected > chip in the first place? Because this is a common behavior of several irqchips (sunxi NMI controller, exynos, etc...) so I think it should be useful to have it in the core instead of replicating the same code structure in all the irqchip drivers. Thanks,
On Thu, 6 Feb 2014, Carlo Caione wrote: > On Thu, Feb 6, 2014 at 10:14 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > > On Thu, 6 Feb 2014, Carlo Caione wrote: > > > >> Several irqchip drivers require the level-triggered interrupt to be > >> acked before unmasking to avoid that a second interrupt is immediately > >> triggered. This small patch introduces a new irqchip flags that is used > >> to ack the IRQ line before it is unmasked. > > > > And why are you not doing this in the unmask function of the affected > > chip in the first place? > > Because this is a common behavior of several irqchips (sunxi NMI > controller, exynos, etc...) so I think it should be useful to have it > in the core instead of replicating the same code structure in all the > irqchip drivers. I'm all for making stuff generic, but you introduce this gem +void ack_unmask_irq(struct irq_desc *desc) +{ + if ((desc->irq_data.chip->flags & IRQCHIP_ACK_ON_UNMASK) && + (irqd_get_trigger_type(&desc->irq_data) & IRQ_TYPE_LEVEL_MASK) && + desc->irq_data.chip->irq_ack) This is totally backwards. 1) If level and edge are handled differently then you should provide different chips. 2) If a chip has IRQCHIP_ACK_ON_UNMASK set, then it better provides an irq_ack callback. So why do you need this complex conditional? + desc->irq_data.chip->irq_ack(&desc->irq_data); + + unmask_irq(desc); +} Now the even more confusing part is a single call site in irq_finalize_oneshot() if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) && irqd_irq_masked(&desc->irq_data)) - unmask_irq(desc); + ack_unmask_irq(desc); But you completely fail to explain the rationale. - Why is this only an issue for the threaded irq case? - Why are other sites where interrupts are masked/unmasked not affected? IOW, why is the handle_level_irq() logic for a non threaded interrupt different from the threaded case? In the non threaded case we do: interrupt(); mask_ack(); handle_irq(); unmask(); reti(); In the threaded case: interrupt(); mask_ack(); wake_thread(); reti(); run_thread(); handle_irq(); unmask(); The difference between those scenarios is: 1) The timing is different 2) In the threaded case we return from the exception with the irq line masked and reenable it later after the threaded handler has run. Do you have any sensible explanation for that requirement to ack before unmask while you already acked on mask? And why this is only an issue in the threaded case? What's the context of the problem you are trying to solve? Thanks, tglx
On Thu, Feb 6, 2014 at 10:54 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > On Thu, 6 Feb 2014, Carlo Caione wrote: >> On Thu, Feb 6, 2014 at 10:14 PM, Thomas Gleixner <tglx@linutronix.de> wrote: >> > On Thu, 6 Feb 2014, Carlo Caione wrote: >> > >> >> Several irqchip drivers require the level-triggered interrupt to be >> >> acked before unmasking to avoid that a second interrupt is immediately >> >> triggered. This small patch introduces a new irqchip flags that is used >> >> to ack the IRQ line before it is unmasked. >> > >> > And why are you not doing this in the unmask function of the affected >> > chip in the first place? >> >> Because this is a common behavior of several irqchips (sunxi NMI >> controller, exynos, etc...) so I think it should be useful to have it >> in the core instead of replicating the same code structure in all the >> irqchip drivers. > > I'm all for making stuff generic, but you introduce this gem > > +void ack_unmask_irq(struct irq_desc *desc) > +{ > + if ((desc->irq_data.chip->flags & IRQCHIP_ACK_ON_UNMASK) && > + (irqd_get_trigger_type(&desc->irq_data) & IRQ_TYPE_LEVEL_MASK) && > + desc->irq_data.chip->irq_ack) > > This is totally backwards. > > 1) If level and edge are handled differently then you should provide > different chips. Agree. I was trying to use the flag to trigger a different behavior since AFAIK the problem of the IRQ retrigger is specific for level-triggered interrupts. > 2) If a chip has IRQCHIP_ACK_ON_UNMASK set, then it better provides an > irq_ack callback. The check on irq_ack is done to be sure that the irq_ack callback is really defined by the irqchip driver before calling it. > So why do you need this complex conditional? I wanted to be sure to be in the right case: level-triggered interrupt with the flag defined in the driver (and the ack callback correctly defined). To the best of my knowledge this is the only case in which a flag like IRQCHIP_ACK_ON_UNMASK could make sense. > + desc->irq_data.chip->irq_ack(&desc->irq_data); > + > + unmask_irq(desc); > +} > > Now the even more confusing part is a single call site in > irq_finalize_oneshot() > > if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) && > irqd_irq_masked(&desc->irq_data)) > - unmask_irq(desc); > + ack_unmask_irq(desc); > > > But you completely fail to explain the rationale. You are right. I'll try to clarify it, > - Why is this only an issue for the threaded irq case? > > - Why are other sites where interrupts are masked/unmasked not > affected? <snip> > Do you have any sensible explanation for that requirement to ack > before unmask while you already acked on mask? And why this is only an > issue in the threaded case? > > What's the context of the problem you are trying to solve? The context and the rationale is fully explained in this thread http://www.spinics.net/lists/arm-kernel/msg299952.html and some answers are already given along the thread especially by Hans here http://www.spinics.net/lists/arm-kernel/msg303542.html Shortly, the double interrupt problem as seen on sunxi NMI controller (and other chips AFAIK) is specific for level-triggered interrupts when the hard interrupt handler is not able to ACK the interrupts on the originating device since this device can only be accessed via a bus (in our case it was a PMIC on I2C). This explains why my patch was specific for the threaded case and why the ACK on mask is not really effective in actually acking the IRQs (so that I need a second ACK before unmasking the line). In fact in our case (PMIC connected via I2C with an interrupt line on a special NMI controller) the implicit ACK done by the unmask is ignored if the NMI line is still high, and to have the line going down we have to ACK all the IRQs on the device accessing to it by I2C in the thread and then ACK the NMI controller with the second ACK before the unmasking callback. I hope this clarifies a bit. Thank you,
On Fri, 7 Feb 2014, Carlo Caione wrote: > The context and the rationale is fully explained in this thread > http://www.spinics.net/lists/arm-kernel/msg299952.html and some > answers are already given along the thread especially by Hans here > http://www.spinics.net/lists/arm-kernel/msg303542.html > Shortly, the double interrupt problem as seen on sunxi NMI controller > (and other chips AFAIK) is specific for level-triggered interrupts > when the hard interrupt handler is not able to ACK the interrupts on > the originating device since this device can only be accessed via a > bus (in our case it was a PMIC on I2C). > This explains why my patch was specific for the threaded case and why > the ACK on mask is not really effective in actually acking the IRQs > (so that I need a second ACK before unmasking the line). In fact in > our case (PMIC connected via I2C with an interrupt line on a special > NMI controller) the implicit ACK done by the unmask is ignored if the > NMI line is still high, and to have the line going down we have to ACK > all the IRQs on the device accessing to it by I2C in the thread and > then ACK the NMI controller with the second ACK before the unmasking > callback. I can't see why it would be specific for the threaded case. The explanation says that the NMI chip is ignoring the ack on mask, which is basically the entry of the interrupt handler. Now it does not matter whether you are threaded or not. The interrupt line at the NMI controller is asserted in both cases. So the same issue should be visible for a non threaded interrupt, if the NMI controller really needs an ack on unmask. But there is another detail: sunxi_sc_nmi_handle_irq() chained_irq_enter() NOP, because GIC uses EOI generic_handle_irq(); nmi->mask(); dev_handler(); nmi->unmask(); chained_irq_exit() gic->eoi(); In the threaded case this looks like: sunxi_sc_nmi_handle_irq() chained_irq_enter() NOP, because GIC uses EOI generic_handle_irq(); nmi->mask(); wake_thread(); chained_irq_exit() gic->eoi(); run_thread() dev_handler(); nmi->unmask(); So the difference is that in the non threaded case the gic->eoi is called after the device interrupt has been cleared and the nmi->interrupt has been unmasked. And in the threaded case its the other way round. I have no idea how that stuff is connected internaly, but I suspect that the gic->eoi is related to this as it might actually ack the NMI chip, which of course only works in the non threaded case. Now back to your patch. I'm not against having a flag, but this should be done less convoluted and have proper names which make the use case clear along with a good technical explanation of the flag in the comment. unmask_and_ack() plus IRQCHIP_ACK_ON_UNMASK are not really telling what the heck is going on. You can restrict it to level irqs in the core, but please use the proper functions and not some opencoded hackery. Thanks, tglx
On Fri, Feb 7, 2014 at 12:51 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > I can't see why it would be specific for the threaded case. > > The explanation says that the NMI chip is ignoring the ack on mask, > which is basically the entry of the interrupt handler. Now it does not > matter whether you are threaded or not. The interrupt line at the NMI > controller is asserted in both cases. So the same issue should be > visible for a non threaded interrupt, if the NMI controller really > needs an ack on unmask. Yes, you are right. I was focusing on the threaded case because it was the only case I have experienced with the hardware I have. The problem should be there also for the non threaded case (even though I don't know how to double check this). At this point I suspect that here the problem is that ACKing the NMI controller without ACKing ahead the device connected always fails (or at least is useless and the pending flag is not cleared), so when I unmask later the irqchip I have a spurious interrupt. That's why I need another ACK for the NMI controller _after_ having ACKed at device level. > But there is another detail: > > sunxi_sc_nmi_handle_irq() > chained_irq_enter() > NOP, because GIC uses EOI > > generic_handle_irq(); > nmi->mask(); > dev_handler(); > nmi->unmask(); > > chained_irq_exit() > gic->eoi(); > > In the threaded case this looks like: > > sunxi_sc_nmi_handle_irq() > chained_irq_enter() > NOP, because GIC uses EOI > > generic_handle_irq(); > nmi->mask(); > wake_thread(); > > chained_irq_exit() > gic->eoi(); > > run_thread() > dev_handler(); > nmi->unmask(); > > So the difference is that in the non threaded case the gic->eoi is > called after the device interrupt has been cleared and the > nmi->interrupt has been unmasked. And in the threaded case its the > other way round. I have no idea how that stuff is connected internaly, > but I suspect that the gic->eoi is related to this as it might > actually ack the NMI chip, which of course only works in the non > threaded case. Yeah, no really difference between threaded and non threaded. For the record, from a mail exchange with Allwinner's engineers: "the NMI module is a signal conversion module. It catches the NMI pin's state and generates irq to GIC", so GIC does not really ACK anything. BTW being a dummy "signal conversion module" this is probably why I still need to clear the pending status even though my IRQ line has already been cleared. > Now back to your patch. > > I'm not against having a flag, but this should be done less convoluted > and have proper names which make the use case clear along with a good > technical explanation of the flag in the comment. Ok, at this point do you think that a patch in the core could be useful or is it better to stick with modifying the unmask callback? > unmask_and_ack() plus IRQCHIP_ACK_ON_UNMASK are not really telling > what the heck is going on. You can restrict it to level irqs in the > core, but please use the proper functions and not some opencoded > hackery. I'll do in case of v2. Thanks,
On Fri, 7 Feb 2014, Carlo Caione wrote: > Yeah, no really difference between threaded and non threaded. > For the record, from a mail exchange with Allwinner's engineers: "the > NMI module is a signal conversion module. It catches the NMI pin's > state and generates irq to GIC", so GIC does not really ACK anything. > BTW being a dummy "signal conversion module" this is probably why I > still need to clear the pending status even though my IRQ line has > already been cleared. A pretty useless signal conversion module it seems creating a big mess for a single interrupt line :) > > I'm not against having a flag, but this should be done less convoluted > > and have proper names which make the use case clear along with a good > > technical explanation of the flag in the comment. > > Ok, at this point do you think that a patch in the core could be > useful or is it better to stick with modifying the unmask callback? Not sure, really, but I tend to a core patch. Though we really want to know whether the issue is threaded only or not. If it's a general issue then this wants to go into unmask_irq() itself and not into an extra unmask_threaded_irq() function. Thanks, tglx
On Fri, Feb 7, 2014 at 6:24 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > Not sure, really, but I tend to a core patch. Though we really want to > know whether the issue is threaded only or not. If it's a general > issue then this wants to go into unmask_irq() itself and not into an > extra unmask_threaded_irq() function. Unfortunately I cannot be sure whether the problem is still present for the non threaded case since in the hardware I have the NMI controller is only connected to the PMIC. Considering that I have no way of checking what do you suggest?
diff --git a/include/linux/irq.h b/include/linux/irq.h index 7dc1003..2cbe8d1 100644 --- a/include/linux/irq.h +++ b/include/linux/irq.h @@ -349,6 +349,7 @@ struct irq_chip { * IRQCHIP_ONOFFLINE_ENABLED: Only call irq_on/off_line callbacks * when irq enabled * IRQCHIP_SKIP_SET_WAKE: Skip chip.irq_set_wake(), for this irq chip + * IRQCHIP_ACK_ON_UNMASK: Ack level interrupts right before unmask */ enum { IRQCHIP_SET_TYPE_MASKED = (1 << 0), @@ -357,6 +358,7 @@ enum { IRQCHIP_ONOFFLINE_ENABLED = (1 << 3), IRQCHIP_SKIP_SET_WAKE = (1 << 4), IRQCHIP_ONESHOT_SAFE = (1 << 5), + IRQCHIP_ACK_ON_UNMASK = (1 << 6), }; /* This include will go away once we isolated irq_desc usage to core code */ diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index dc04c16..9d2d2e2 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -265,6 +265,16 @@ static inline void mask_ack_irq(struct irq_desc *desc) irq_state_set_masked(desc); } +void ack_unmask_irq(struct irq_desc *desc) +{ + if ((desc->irq_data.chip->flags & IRQCHIP_ACK_ON_UNMASK) && + (irqd_get_trigger_type(&desc->irq_data) & IRQ_TYPE_LEVEL_MASK) && + desc->irq_data.chip->irq_ack) + desc->irq_data.chip->irq_ack(&desc->irq_data); + + unmask_irq(desc); +} + void mask_irq(struct irq_desc *desc) { if (desc->irq_data.chip->irq_mask) { diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h index 001fa5b..06ff850 100644 --- a/kernel/irq/internals.h +++ b/kernel/irq/internals.h @@ -73,6 +73,7 @@ extern void irq_percpu_enable(struct irq_desc *desc, unsigned int cpu); extern void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu); extern void mask_irq(struct irq_desc *desc); extern void unmask_irq(struct irq_desc *desc); +extern void ack_unmask_irq(struct irq_desc *desc); extern void init_kstat_irqs(struct irq_desc *desc, int node, int nr); diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index 481a13c..39505d8 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -718,7 +718,7 @@ again: if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) && irqd_irq_masked(&desc->irq_data)) - unmask_irq(desc); + ack_unmask_irq(desc); out_unlock: raw_spin_unlock_irq(&desc->lock);
Several irqchip drivers require the level-triggered interrupt to be acked before unmasking to avoid that a second interrupt is immediately triggered. This small patch introduces a new irqchip flags that is used to ack the IRQ line before it is unmasked. Signed-off-by: Carlo Caione <carlo@caione.org> --- include/linux/irq.h | 2 ++ kernel/irq/chip.c | 10 ++++++++++ kernel/irq/internals.h | 1 + kernel/irq/manage.c | 2 +- 4 files changed, 14 insertions(+), 1 deletion(-)