| Message ID | 1421174781-4340-2-git-send-email-boris.brezillon@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 13 Jan 2015, Boris Brezillon wrote: > + ret = irq_set_handler_data(irq, demux); > + if (ret) { > + pr_err("Failed to assign handler data\n"); > + goto err_free_domain; > + } > + > + irq_set_chained_handler(irq, irq_dumb_demux_handler); > + > + /* > + * Disable the src irq (automatically enabled by > + * irq_set_chained_handler) to prevent irqs from happening while > + * nobody requested any of the demuxed irqs. > + */ > + disable_irq(irq); We rather prevent the startup of the irq line right away. enum { IRQ_CHAINED_NONE, IRQ_CHAINED_STARTUP, IRQ_CHAINED_NOSTARTUP, }; -__irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained, - const char *name); +__irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int chained_mode, + const char *name); { .... if (handle != handle_bad_irq && chained_mode) { irq_settings_set_noprobe(desc); irq_settings_set_norequest(desc); irq_settings_set_nothread(desc); - irq_startup(desc, true); + if (chained_mode == IRQ_CHAINED_STARTUP) + irq_startup(desc, true); } .... } Hmm? tglx
On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Some interrupt controllers are multiplexing several peripheral IRQs on > a single interrupt line. > While this is not a problem for most IRQs (as long as all peripherals > request the interrupt with IRQF_SHARED flag set), multiplexing timers and > other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND > and !IRQF_NO_SUSPEND is prohibited). > > Create a dumb irq demultiplexer which simply forwards interrupts to all > peripherals (exactly what's happening with IRQ_SHARED) but keep a unique > irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND > and !IRQF_NO_SUSPEND mix on a given interrupt. This really seems like a work-around for how IRQF_SHARED works. It seems like what is really desired is just per handler disabling. It is fragile in that devices can deadlock the system if the drivers don't disable the interrupt source before calling disable_irq. But unlike IRQF_SHARED, there is nothing explicit in the driver indicating it is designed to work properly with a shared interrupt line. I see no reason to accept this into DT either. We already can support shared lines and modeling an OR gate as an interrupt controller is pointless. Rob > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > drivers/irqchip/Kconfig | 4 ++ > drivers/irqchip/Makefile | 1 + > drivers/irqchip/irq-dumb-demux.c | 70 ++++++++++++++++++++ > include/linux/irq.h | 49 ++++++++++++++ > include/linux/irqdomain.h | 1 + > kernel/irq/Kconfig | 5 ++ > kernel/irq/Makefile | 1 + > kernel/irq/chip.c | 41 ++++++++++++ > kernel/irq/dumb-demux-chip.c | 140 +++++++++++++++++++++++++++++++++++++++ > kernel/irq/handle.c | 31 ++++++++- > kernel/irq/internals.h | 3 + > 11 files changed, 344 insertions(+), 2 deletions(-) > create mode 100644 drivers/irqchip/irq-dumb-demux.c > create mode 100644 kernel/irq/dumb-demux-chip.c > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index cc79d2a..8a9df88 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -70,6 +70,10 @@ config BRCMSTB_L2_IRQ > select GENERIC_IRQ_CHIP > select IRQ_DOMAIN > > +config DUMB_DEMUX_IRQ > + bool > + select DUMB_IRQ_DEMUX_CHIP > + > config DW_APB_ICTL > bool > select GENERIC_IRQ_CHIP > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index 9516a32..77f3c51 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -8,6 +8,7 @@ obj-$(CONFIG_ARCH_MVEBU) += irq-armada-370-xp.o > obj-$(CONFIG_ARCH_MXS) += irq-mxs.o > obj-$(CONFIG_ARCH_S3C24XX) += irq-s3c24xx.o > obj-$(CONFIG_DW_APB_ICTL) += irq-dw-apb-ictl.o > +obj-$(CONFIG_DUMB_DEMUX_IRQ) += irq-dumb-demux.o > obj-$(CONFIG_METAG) += irq-metag-ext.o > obj-$(CONFIG_METAG_PERFCOUNTER_IRQS) += irq-metag.o > obj-$(CONFIG_ARCH_MOXART) += irq-moxart.o > diff --git a/drivers/irqchip/irq-dumb-demux.c b/drivers/irqchip/irq-dumb-demux.c > new file mode 100644 > index 0000000..dfa05ce > --- /dev/null > +++ b/drivers/irqchip/irq-dumb-demux.c > @@ -0,0 +1,70 @@ > +#include <linux/interrupt.h> > +#include <linux/irq.h> > +#include <linux/irqdomain.h> > +#include <linux/of_irq.h> > +#include <linux/slab.h> > + > +#include "irqchip.h" > + > +static int __init dumb_irq_demux_of_init(struct device_node *node, > + struct device_node *parent) > +{ > + struct irq_chip_dumb_demux *demux; > + unsigned int irq; > + u32 valid_irqs; > + int ret; > + > + irq = irq_of_parse_and_map(node, 0); > + if (!irq) { > + pr_err("Failed to retrieve dumb irq demuxer source\n"); > + return -EINVAL; > + } > + > + ret = of_property_read_u32(node, "irqs", &valid_irqs); > + if (ret) { > + pr_err("Invalid of missing 'irqs' property\n"); > + return ret; > + } > + > + demux = irq_alloc_dumb_demux_chip(irq, valid_irqs, > + IRQ_NOREQUEST | IRQ_NOPROBE | > + IRQ_NOAUTOEN, 0, 0); > + if (!demux) { > + pr_err("Failed to allocate dumb irq demuxer struct\n"); > + return -ENOMEM; > + } > + > + demux->domain = irq_domain_add_linear(node, BITS_PER_LONG, > + &irq_dumb_demux_domain_ops, > + demux); > + if (!demux->domain) { > + ret = -ENOMEM; > + goto err_free_demux; > + } > + > + ret = irq_set_handler_data(irq, demux); > + if (ret) { > + pr_err("Failed to assign handler data\n"); > + goto err_free_domain; > + } > + > + irq_set_chained_handler(irq, irq_dumb_demux_handler); > + > + /* > + * Disable the src irq (automatically enabled by > + * irq_set_chained_handler) to prevent irqs from happening while > + * nobody requested any of the demuxed irqs. > + */ > + disable_irq(irq); > + > + return 0; > + > +err_free_domain: > + irq_domain_remove(demux->domain); > + > +err_free_demux: > + kfree(demux); > + > + return ret; > +} > +IRQCHIP_DECLARE(dumb_irq_demux, "irqchip-dumb-demux", dumb_irq_demux_of_init); > diff --git a/include/linux/irq.h b/include/linux/irq.h > index d09ec7a..ae8fa21 100644 > --- a/include/linux/irq.h > +++ b/include/linux/irq.h > @@ -445,6 +445,10 @@ extern void handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc); > extern void handle_edge_irq(unsigned int irq, struct irq_desc *desc); > extern void handle_edge_eoi_irq(unsigned int irq, struct irq_desc *desc); > extern void handle_simple_irq(unsigned int irq, struct irq_desc *desc); > +#ifdef CONFIG_DUMB_IRQ_DEMUX_CHIP > +extern irqreturn_t handle_dumb_demux_irq(unsigned int irq, > + struct irq_desc *desc); > +#endif > extern void handle_percpu_irq(unsigned int irq, struct irq_desc *desc); > extern void handle_percpu_devid_irq(unsigned int irq, struct irq_desc *desc); > extern void handle_bad_irq(unsigned int irq, struct irq_desc *desc); > @@ -862,4 +866,49 @@ static inline u32 irq_reg_readl(struct irq_chip_generic *gc, > return readl(gc->reg_base + reg_offset); > } > > +/** > + * enum irq_dumb_demux_flags - Initialization flags for generic irq chips > + * @IRQ_DD_INIT_NESTED_LOCK: Set the lock class of the irqs to nested for > + * irq chips which need to call irq_set_wake() on > + * the parent irq. Usually GPIO implementations > + */ > +enum irq_dumb_demux_flags { > + IRQ_DD_INIT_NESTED_LOCK = 1 << 0, > +}; > + > +#ifdef CONFIG_DUMB_IRQ_DEMUX_CHIP > +/** > + * struct irq_chip_dumb_demux - Dumb demultiplexer irq chip data structure > + * @domain: irq domain pointer > + * @available: Bitfield of valid irqs > + * @unmasked: Bitfield containing irqs status > + * @flags: irq_dumb_demux_flags flags > + * @src_irq: irq feeding the dumb demux chip > + * > + * Note, that irq_chip_generic can have multiple irq_chip_type > + * implementations which can be associated to a particular irq line of > + * an irq_chip_generic instance. That allows to share and protect > + * state in an irq_chip_generic instance when we need to implement > + * different flow mechanisms (level/edge) for it. > + */ > +struct irq_chip_dumb_demux { > + struct irq_domain *domain; > + unsigned long available; > + unsigned long unmasked; > + unsigned int flags; > + unsigned int src_irq; > + unsigned int irq_flags_to_clear; > + unsigned int irq_flags_to_set; > +}; > + > +void irq_dumb_demux_handler(unsigned int irq, struct irq_desc *desc); > + > +struct irq_chip_dumb_demux * > +irq_alloc_dumb_demux_chip(unsigned int src_irq, > + unsigned long valid_irqs, > + unsigned int clr_flags, > + unsigned int set_flags, > + unsigned int dd_flags); > +#endif /* CONFIG_DUMB_IRQ_DEMUX_CHIP */ > + > #endif /* _LINUX_IRQ_H */ > diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h > index 676d730..1de3808 100644 > --- a/include/linux/irqdomain.h > +++ b/include/linux/irqdomain.h > @@ -80,6 +80,7 @@ struct irq_domain_ops { > }; > > extern struct irq_domain_ops irq_generic_chip_ops; > +extern struct irq_domain_ops irq_dumb_demux_domain_ops; > > struct irq_domain_chip_generic; > > diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig > index 9a76e3b..d01554a 100644 > --- a/kernel/irq/Kconfig > +++ b/kernel/irq/Kconfig > @@ -51,6 +51,11 @@ config GENERIC_IRQ_CHIP > bool > select IRQ_DOMAIN > > +# Dumb interrupt demuxer chip implementation > +config DUMB_IRQ_DEMUX_CHIP > + bool > + select IRQ_DOMAIN > + > # Generic irq_domain hw <--> linux irq number translation > config IRQ_DOMAIN > bool > diff --git a/kernel/irq/Makefile b/kernel/irq/Makefile > index d121235..1cd4e42 100644 > --- a/kernel/irq/Makefile > +++ b/kernel/irq/Makefile > @@ -7,3 +7,4 @@ obj-$(CONFIG_PROC_FS) += proc.o > obj-$(CONFIG_GENERIC_PENDING_IRQ) += migration.o > obj-$(CONFIG_PM_SLEEP) += pm.o > obj-$(CONFIG_GENERIC_MSI_IRQ) += msi.o > +obj-$(CONFIG_DUMB_IRQ_DEMUX_CHIP) += dumb-demux-chip.o > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c > index 6f1c7a5..d2a5c96 100644 > --- a/kernel/irq/chip.c > +++ b/kernel/irq/chip.c > @@ -405,6 +405,47 @@ out_unlock: > } > EXPORT_SYMBOL_GPL(handle_simple_irq); > > +#ifdef CONFIG_DUMB_IRQ_DEMUX_CHIP > +/** > + * handle_dumb_demux_irq - Dumb demuxer irq handle function. > + * @irq: the interrupt number > + * @desc: the interrupt description structure for this irq > + * > + * Dumb demux interrupts are sent from a demultiplexing interrupt handler > + * which is not able to decide which child interrupt interrupt handler > + * should be called. > + * > + * Note: The caller is expected to handle the ack, clear, mask and > + * unmask issues if necessary. > + */ > +irqreturn_t > +handle_dumb_demux_irq(unsigned int irq, struct irq_desc *desc) > +{ > + irqreturn_t retval = IRQ_NONE; > + > + raw_spin_lock(&desc->lock); > + > + if (!irq_may_run(desc)) > + goto out_unlock; > + > + desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING); > + kstat_incr_irqs_this_cpu(irq, desc); > + > + if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) { > + desc->istate |= IRQS_PENDING; > + goto out_unlock; > + } > + > + retval = handle_irq_event_no_spurious_check(desc); > + > +out_unlock: > + raw_spin_unlock(&desc->lock); > + > + return retval; > +} > +EXPORT_SYMBOL_GPL(handle_dumb_demux_irq); > +#endif /* CONFIG_DUMB_IRQ_DEMUX_CHIP */ > + > /* > * Called unconditionally from handle_level_irq() and only for oneshot > * interrupts from handle_fasteoi_irq() > diff --git a/kernel/irq/dumb-demux-chip.c b/kernel/irq/dumb-demux-chip.c > new file mode 100644 > index 0000000..8e2de1d > --- /dev/null > +++ b/kernel/irq/dumb-demux-chip.c > @@ -0,0 +1,140 @@ > +/* > + * Library implementing common dumb irq demux chip functions > + * > + * Copyright (C) 2015, Boris Brezillon > + */ > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/irq.h> > +#include <linux/slab.h> > +#include <linux/export.h> > +#include <linux/irq.h> > +#include <linux/irqdomain.h> > +#include <linux/irqchip/chained_irq.h> > +#include <linux/interrupt.h> > +#include <linux/kernel_stat.h> > +#include <linux/syscore_ops.h> > + > +#include "internals.h" > + > +static void irq_dumb_demux_mask(struct irq_data *d) > +{ > + struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d); > + > + clear_bit(d->hwirq, &demux->unmasked); > + > + if (!demux->unmasked) > + disable_irq_nosync(demux->src_irq); > +} > + > +static void irq_dumb_demux_unmask(struct irq_data *d) > +{ > + struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d); > + bool enable_src_irq = !demux->unmasked; > + > + set_bit(d->hwirq, &demux->unmasked); > + > + if (enable_src_irq) > + enable_irq(demux->src_irq); > +} > + > +static struct irq_chip irq_dumb_demux_chip = { > + .name = "dumb-demux-irq", > + .irq_mask = irq_dumb_demux_mask, > + .irq_unmask = irq_dumb_demux_unmask, > +}; > + > +/* > + * Separate lockdep class for interrupt chip which can nest irq_desc > + * lock. > + */ > +static struct lock_class_key irq_nested_lock_class; > + > +/* > + * irq_map_dumb_demux_chip - Map a dumb demux chip for an irq domain > + */ > +static int irq_map_dumb_demux_chip(struct irq_domain *d, > + unsigned int virq, > + irq_hw_number_t hw_irq) > +{ > + struct irq_chip_dumb_demux *demux = d->host_data; > + > + if (!test_bit(hw_irq, &demux->available)) > + return -EINVAL; > + > + if (demux->flags & IRQ_DD_INIT_NESTED_LOCK) > + irq_set_lockdep_class(virq, &irq_nested_lock_class); > + > + irq_set_chip(virq, &irq_dumb_demux_chip); > + irq_set_chip_data(virq, demux); > + irq_modify_status(virq, demux->irq_flags_to_clear, > + demux->irq_flags_to_set); > + > + return 0; > +} > + > +struct irq_domain_ops irq_dumb_demux_domain_ops = { > + .map = irq_map_dumb_demux_chip, > + .xlate = irq_domain_xlate_onecell, > +}; > +EXPORT_SYMBOL_GPL(irq_dumb_demux_domain_ops); > + > +/** > + * irq_dumb_demux_handler - Dumb demux flow handler > + * @irq: Virtual irq number > + * @irq_desc: irq descriptor > + */ > +void irq_dumb_demux_handler(unsigned int irq, struct irq_desc *desc) > +{ > + struct irq_chip_dumb_demux *demux = irq_get_handler_data(irq); > + struct irq_chip *chip = irq_desc_get_chip(desc); > + irqreturn_t ret = IRQ_NONE; > + int i; > + > + chained_irq_enter(chip, desc); > + for_each_set_bit(i, &demux->unmasked, BITS_PER_LONG) { > + int demuxed_irq = irq_find_mapping(demux->domain, i); > + struct irq_desc *desc = irq_to_desc(demuxed_irq); > + > + ret |= handle_dumb_demux_irq(demuxed_irq, desc); > + } > + chained_irq_exit(chip, desc); > + > + if (!noirqdebug) > + note_interrupt(irq, desc, ret); > +} > +EXPORT_SYMBOL_GPL(irq_dumb_demux_handler); > + > +/** > + * irq_alloc_dumb_demux_chip - Allocate a dumb demux chip > + * @src_irq: irq feeding the dumb demux chip > + * @dd_flags: irq_dumb_demux_flags flags > + * @valid_irqs: Bitmask representing valid irqs > + * @clr_flags: irq_flags to clear when mapping an interrupt > + * @set_flags: irq_flags to set when mapping an interrupt > + */ > +struct irq_chip_dumb_demux * > +irq_alloc_dumb_demux_chip(unsigned int src_irq, > + unsigned long valid_irqs, > + unsigned int clr_flags, > + unsigned int set_flags, > + unsigned int dd_flags) > +{ > + struct irq_chip_dumb_demux *demux; > + > + if (!src_irq) > + return ERR_PTR(-EINVAL); > + > + demux = kzalloc(sizeof(*demux), GFP_KERNEL); > + if (!demux) > + return ERR_PTR(-ENOMEM); > + > + demux->available = valid_irqs; > + demux->flags = dd_flags; > + demux->src_irq = src_irq; > + demux->irq_flags_to_clear = clr_flags; > + demux->irq_flags_to_set = set_flags; > + > + return demux; > +} > +EXPORT_SYMBOL_GPL(irq_alloc_dumb_demux_chip); > diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c > index 6354802..f786850 100644 > --- a/kernel/irq/handle.c > +++ b/kernel/irq/handle.c > @@ -131,7 +131,8 @@ void __irq_wake_thread(struct irq_desc *desc, struct irqaction *action) > } > > irqreturn_t > -handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action) > +handle_irq_event_percpu_no_spurious_check(struct irq_desc *desc, > + struct irqaction *action) > { > irqreturn_t retval = IRQ_NONE; > unsigned int flags = 0, irq = desc->irq_data.irq; > @@ -175,8 +176,18 @@ handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action) > > add_interrupt_randomness(irq, flags); > > + return retval; > +} > + > +irqreturn_t > +handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action) > +{ > + irqreturn_t retval; > + > + retval = handle_irq_event_percpu_no_spurious_check(desc, action); > + > if (!noirqdebug) > - note_interrupt(irq, desc, retval); > + note_interrupt(desc->irq_data.irq, desc, retval); > return retval; > } > > @@ -195,3 +206,19 @@ irqreturn_t handle_irq_event(struct irq_desc *desc) > irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS); > return ret; > } > + > +irqreturn_t handle_irq_event_no_spurious_check(struct irq_desc *desc) > +{ > + struct irqaction *action = desc->action; > + irqreturn_t ret; > + > + desc->istate &= ~IRQS_PENDING; > + irqd_set(&desc->irq_data, IRQD_IRQ_INPROGRESS); > + raw_spin_unlock(&desc->lock); > + > + ret = handle_irq_event_percpu_no_spurious_check(desc, action); > + > + raw_spin_lock(&desc->lock); > + irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS); > + return ret; > +} > diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h > index df553b0..fe056fb 100644 > --- a/kernel/irq/internals.h > +++ b/kernel/irq/internals.h > @@ -90,6 +90,9 @@ extern void init_kstat_irqs(struct irq_desc *desc, int node, int nr); > > irqreturn_t handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action); > irqreturn_t handle_irq_event(struct irq_desc *desc); > +irqreturn_t handle_irq_event_percpu_no_spurious_check(struct irq_desc *desc, > + struct irqaction *action); > +irqreturn_t handle_irq_event_no_spurious_check(struct irq_desc *desc); > > /* Resending of interrupts :*/ > void check_irq_resend(struct irq_desc *desc, unsigned int irq); > -- > 1.9.1 >
Hi Rob, On Tue, 13 Jan 2015 21:26:42 -0600 Rob Herring <robherring2@gmail.com> wrote: > On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > Some interrupt controllers are multiplexing several peripheral IRQs on > > a single interrupt line. > > While this is not a problem for most IRQs (as long as all peripherals > > request the interrupt with IRQF_SHARED flag set), multiplexing timers and > > other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND > > and !IRQF_NO_SUSPEND is prohibited). > > > > Create a dumb irq demultiplexer which simply forwards interrupts to all > > peripherals (exactly what's happening with IRQ_SHARED) but keep a unique > > irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND > > and !IRQF_NO_SUSPEND mix on a given interrupt. > > This really seems like a work-around for how IRQF_SHARED works. It > seems like what is really desired is just per handler disabling. Like what I proposed here [1] ? > It is > fragile in that devices can deadlock the system if the drivers don't > disable the interrupt source before calling disable_irq. Not exactly deadlock since spurious interrupt detection is implemented, but yes, things won't work as expected. > But unlike > IRQF_SHARED, there is nothing explicit in the driver indicating it is > designed to work properly with a shared interrupt line. > > I see no reason to accept this into DT either. We already can support > shared lines and modeling an OR gate as an interrupt controller is > pointless. Okay, I guess I'll let DT and irq maintainers decide what is preferable here (I already spent much time than I first expected to remove this warning in a proper way). Best Regards, Boris [1]https://lkml.org/lkml/2014/12/15/552
Hi Thomas, On Tue, 13 Jan 2015 22:00:55 +0100 (CET) Thomas Gleixner <tglx@linutronix.de> wrote: > On Tue, 13 Jan 2015, Boris Brezillon wrote: > > + ret = irq_set_handler_data(irq, demux); > > + if (ret) { > > + pr_err("Failed to assign handler data\n"); > > + goto err_free_domain; > > + } > > + > > + irq_set_chained_handler(irq, irq_dumb_demux_handler); > > + > > + /* > > + * Disable the src irq (automatically enabled by > > + * irq_set_chained_handler) to prevent irqs from happening while > > + * nobody requested any of the demuxed irqs. > > + */ > > + disable_irq(irq); > > We rather prevent the startup of the irq line right away. Yep, the comment was here to draw the attention, since I wasn't sure modifying core code was the best solution. Anyway I agree that keeping the src_irq disabled when registering the chained handler is better than doing it afterwards. > > enum { > IRQ_CHAINED_NONE, > IRQ_CHAINED_STARTUP, > IRQ_CHAINED_NOSTARTUP, > }; > > -__irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained, > - const char *name); > +__irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int chained_mode, > + const char *name); > > { > .... > if (handle != handle_bad_irq && chained_mode) { > irq_settings_set_noprobe(desc); > irq_settings_set_norequest(desc); > irq_settings_set_nothread(desc); > - irq_startup(desc, true); > + if (chained_mode == IRQ_CHAINED_STARTUP) > + irq_startup(desc, true); > } > .... > } > > Hmm? I'm fine with this approach, with a static inline helper: static inline void irq_set_chained_handler_nostartup(unsigned int irq, irq_flow_handler_t handle) { __irq_set_handler(irq, handle, IRQ_CHAINED_NOSTARTUP, NULL); } Anyway, I'll wait a clear decision regarding which approach should be taken (see my answer to Rob) before sending a new version. Thanks for your reviews. Best Regards, Boris
On Tue, 13 Jan 2015, Rob Herring wrote: > On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > Some interrupt controllers are multiplexing several peripheral IRQs on > > a single interrupt line. > > While this is not a problem for most IRQs (as long as all peripherals > > request the interrupt with IRQF_SHARED flag set), multiplexing timers and > > other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND > > and !IRQF_NO_SUSPEND is prohibited). > > > > Create a dumb irq demultiplexer which simply forwards interrupts to all > > peripherals (exactly what's happening with IRQ_SHARED) but keep a unique > > irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND > > and !IRQF_NO_SUSPEND mix on a given interrupt. > > This really seems like a work-around for how IRQF_SHARED works. It It's a workaround for a short coming of IRQF_SHARED. IRQF_SHARED has a massive short coming versus suspend and wakeup interrupts. If one of the demultiplexed interrupts is a valid wakeup source then we have no sane way to express this with IRQF_SHARED simply because the drivers need to be aware whether they run on stupid or well designed hardware. > seems like what is really desired is just per handler disabling. It is So you want a magic API like disable/enable_irq_action()? Certainly not. You'd open just another can of worms which will bring us abuse and hard to debug problems because driver writers think it's a good idea to use it for random purposes. Aside of that it would add another conditional into the interrupt delivery hotpath which is not desired either. > fragile in that devices can deadlock the system if the drivers don't > disable the interrupt source before calling disable_irq. But unlike Any misdesigned driver can do that for you. > IRQF_SHARED, there is nothing explicit in the driver indicating it is > designed to work properly with a shared interrupt line. IRQF_SHARED is a pretty bad indicator. Look at all the drivers which slap this flag onto request_irq() and have no single line of code which makes sure that the driver would ever work on a shared line. If it's just for annotational purposes, we can add a new IRQF flag, which is required to request such a interrupt line. > I see no reason to accept this into DT either. We already can support > shared lines and modeling an OR gate as an interrupt controller is > pointless. It's absolutely not pointless. All attempts to work around that have resulted in horrible bandaids so far. That's why I guided Boris to implement this dummy demultiplexing mechanism. It solves the problem at hand nicely without adding nasty hackarounds into the suspend/resume code and inflicting platform knowledge on multi-platform device drivers. If you have a proper solution for the problem at hand which - avoids the demux dummy - works straight forward with suspend/resume/wakeup - does not add horrible new APIs - does not add conditionals to the interrupt hotpath - does not inflict platform knowledge about interrupt chip details on drivers then I'm happy to take it. But as long as you can't come up with anything sane, the demux dummy is the best solution for this problem we've seen so far. Thanks, tglx
Le 13/01/2015 19:46, Boris Brezillon a écrit : > Some interrupt controllers are multiplexing several peripheral IRQs on > a single interrupt line. > While this is not a problem for most IRQs (as long as all peripherals > request the interrupt with IRQF_SHARED flag set), multiplexing timers and > other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND > and !IRQF_NO_SUSPEND is prohibited). > > Create a dumb irq demultiplexer which simply forwards interrupts to all > peripherals (exactly what's happening with IRQ_SHARED) but keep a unique > irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND > and !IRQF_NO_SUSPEND mix on a given interrupt. > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > drivers/irqchip/Kconfig | 4 ++ > drivers/irqchip/Makefile | 1 + > drivers/irqchip/irq-dumb-demux.c | 70 ++++++++++++++++++++ > include/linux/irq.h | 49 ++++++++++++++ > include/linux/irqdomain.h | 1 + > kernel/irq/Kconfig | 5 ++ > kernel/irq/Makefile | 1 + > kernel/irq/chip.c | 41 ++++++++++++ > kernel/irq/dumb-demux-chip.c | 140 +++++++++++++++++++++++++++++++++++++++ > kernel/irq/handle.c | 31 ++++++++- > kernel/irq/internals.h | 3 + > 11 files changed, 344 insertions(+), 2 deletions(-) > create mode 100644 drivers/irqchip/irq-dumb-demux.c > create mode 100644 kernel/irq/dumb-demux-chip.c > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index cc79d2a..8a9df88 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -70,6 +70,10 @@ config BRCMSTB_L2_IRQ > select GENERIC_IRQ_CHIP > select IRQ_DOMAIN > > +config DUMB_DEMUX_IRQ > + bool > + select DUMB_IRQ_DEMUX_CHIP > + > config DW_APB_ICTL > bool > select GENERIC_IRQ_CHIP > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index 9516a32..77f3c51 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -8,6 +8,7 @@ obj-$(CONFIG_ARCH_MVEBU) += irq-armada-370-xp.o > obj-$(CONFIG_ARCH_MXS) += irq-mxs.o > obj-$(CONFIG_ARCH_S3C24XX) += irq-s3c24xx.o > obj-$(CONFIG_DW_APB_ICTL) += irq-dw-apb-ictl.o > +obj-$(CONFIG_DUMB_DEMUX_IRQ) += irq-dumb-demux.o > obj-$(CONFIG_METAG) += irq-metag-ext.o > obj-$(CONFIG_METAG_PERFCOUNTER_IRQS) += irq-metag.o > obj-$(CONFIG_ARCH_MOXART) += irq-moxart.o > diff --git a/drivers/irqchip/irq-dumb-demux.c b/drivers/irqchip/irq-dumb-demux.c > new file mode 100644 > index 0000000..dfa05ce > --- /dev/null > +++ b/drivers/irqchip/irq-dumb-demux.c > @@ -0,0 +1,70 @@ Maybe add a little file header here. It's always better. > +#include <linux/interrupt.h> > +#include <linux/irq.h> > +#include <linux/irqdomain.h> > +#include <linux/of_irq.h> > +#include <linux/slab.h> > + > +#include "irqchip.h" > + > +static int __init dumb_irq_demux_of_init(struct device_node *node, > + struct device_node *parent) > +{ > + struct irq_chip_dumb_demux *demux; > + unsigned int irq; > + u32 valid_irqs; > + int ret; > + > + irq = irq_of_parse_and_map(node, 0); > + if (!irq) { > + pr_err("Failed to retrieve dumb irq demuxer source\n"); > + return -EINVAL; > + } > + > + ret = of_property_read_u32(node, "irqs", &valid_irqs); > + if (ret) { > + pr_err("Invalid of missing 'irqs' property\n"); > + return ret; > + } > + > + demux = irq_alloc_dumb_demux_chip(irq, valid_irqs, > + IRQ_NOREQUEST | IRQ_NOPROBE | > + IRQ_NOAUTOEN, 0, 0); > + if (!demux) { > + pr_err("Failed to allocate dumb irq demuxer struct\n"); > + return -ENOMEM; > + } > + > + demux->domain = irq_domain_add_linear(node, BITS_PER_LONG, > + &irq_dumb_demux_domain_ops, > + demux); > + if (!demux->domain) { > + ret = -ENOMEM; > + goto err_free_demux; > + } > + > + ret = irq_set_handler_data(irq, demux); > + if (ret) { > + pr_err("Failed to assign handler data\n"); > + goto err_free_domain; > + } > + > + irq_set_chained_handler(irq, irq_dumb_demux_handler); > + > + /* > + * Disable the src irq (automatically enabled by > + * irq_set_chained_handler) to prevent irqs from happening while > + * nobody requested any of the demuxed irqs. > + */ > + disable_irq(irq); > + > + return 0; > + > +err_free_domain: > + irq_domain_remove(demux->domain); > + > +err_free_demux: > + kfree(demux); > + > + return ret; > +} > +IRQCHIP_DECLARE(dumb_irq_demux, "irqchip-dumb-demux", dumb_irq_demux_of_init); > diff --git a/include/linux/irq.h b/include/linux/irq.h > index d09ec7a..ae8fa21 100644 > --- a/include/linux/irq.h > +++ b/include/linux/irq.h > @@ -445,6 +445,10 @@ extern void handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc); > extern void handle_edge_irq(unsigned int irq, struct irq_desc *desc); > extern void handle_edge_eoi_irq(unsigned int irq, struct irq_desc *desc); > extern void handle_simple_irq(unsigned int irq, struct irq_desc *desc); > +#ifdef CONFIG_DUMB_IRQ_DEMUX_CHIP > +extern irqreturn_t handle_dumb_demux_irq(unsigned int irq, > + struct irq_desc *desc); > +#endif > extern void handle_percpu_irq(unsigned int irq, struct irq_desc *desc); > extern void handle_percpu_devid_irq(unsigned int irq, struct irq_desc *desc); > extern void handle_bad_irq(unsigned int irq, struct irq_desc *desc); > @@ -862,4 +866,49 @@ static inline u32 irq_reg_readl(struct irq_chip_generic *gc, > return readl(gc->reg_base + reg_offset); > } > > +/** > + * enum irq_dumb_demux_flags - Initialization flags for generic irq chips > + * @IRQ_DD_INIT_NESTED_LOCK: Set the lock class of the irqs to nested for > + * irq chips which need to call irq_set_wake() on > + * the parent irq. Usually GPIO implementations > + */ > +enum irq_dumb_demux_flags { > + IRQ_DD_INIT_NESTED_LOCK = 1 << 0, > +}; > + > +#ifdef CONFIG_DUMB_IRQ_DEMUX_CHIP > +/** > + * struct irq_chip_dumb_demux - Dumb demultiplexer irq chip data structure > + * @domain: irq domain pointer > + * @available: Bitfield of valid irqs > + * @unmasked: Bitfield containing irqs status > + * @flags: irq_dumb_demux_flags flags > + * @src_irq: irq feeding the dumb demux chip > + * > + * Note, that irq_chip_generic can have multiple irq_chip_type > + * implementations which can be associated to a particular irq line of > + * an irq_chip_generic instance. That allows to share and protect > + * state in an irq_chip_generic instance when we need to implement > + * different flow mechanisms (level/edge) for it. > + */ > +struct irq_chip_dumb_demux { > + struct irq_domain *domain; > + unsigned long available; > + unsigned long unmasked; > + unsigned int flags; > + unsigned int src_irq; > + unsigned int irq_flags_to_clear; > + unsigned int irq_flags_to_set; > +}; > + > +void irq_dumb_demux_handler(unsigned int irq, struct irq_desc *desc); > + > +struct irq_chip_dumb_demux * > +irq_alloc_dumb_demux_chip(unsigned int src_irq, > + unsigned long valid_irqs, > + unsigned int clr_flags, > + unsigned int set_flags, > + unsigned int dd_flags); > +#endif /* CONFIG_DUMB_IRQ_DEMUX_CHIP */ > + > #endif /* _LINUX_IRQ_H */ > diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h > index 676d730..1de3808 100644 > --- a/include/linux/irqdomain.h > +++ b/include/linux/irqdomain.h > @@ -80,6 +80,7 @@ struct irq_domain_ops { > }; > > extern struct irq_domain_ops irq_generic_chip_ops; > +extern struct irq_domain_ops irq_dumb_demux_domain_ops; > > struct irq_domain_chip_generic; > > diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig > index 9a76e3b..d01554a 100644 > --- a/kernel/irq/Kconfig > +++ b/kernel/irq/Kconfig > @@ -51,6 +51,11 @@ config GENERIC_IRQ_CHIP > bool > select IRQ_DOMAIN > > +# Dumb interrupt demuxer chip implementation > +config DUMB_IRQ_DEMUX_CHIP > + bool > + select IRQ_DOMAIN > + > # Generic irq_domain hw <--> linux irq number translation > config IRQ_DOMAIN > bool > diff --git a/kernel/irq/Makefile b/kernel/irq/Makefile > index d121235..1cd4e42 100644 > --- a/kernel/irq/Makefile > +++ b/kernel/irq/Makefile > @@ -7,3 +7,4 @@ obj-$(CONFIG_PROC_FS) += proc.o > obj-$(CONFIG_GENERIC_PENDING_IRQ) += migration.o > obj-$(CONFIG_PM_SLEEP) += pm.o > obj-$(CONFIG_GENERIC_MSI_IRQ) += msi.o > +obj-$(CONFIG_DUMB_IRQ_DEMUX_CHIP) += dumb-demux-chip.o > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c > index 6f1c7a5..d2a5c96 100644 > --- a/kernel/irq/chip.c > +++ b/kernel/irq/chip.c > @@ -405,6 +405,47 @@ out_unlock: > } > EXPORT_SYMBOL_GPL(handle_simple_irq); > > +#ifdef CONFIG_DUMB_IRQ_DEMUX_CHIP > +/** > + * handle_dumb_demux_irq - Dumb demuxer irq handle function. > + * @irq: the interrupt number > + * @desc: the interrupt description structure for this irq > + * > + * Dumb demux interrupts are sent from a demultiplexing interrupt handler > + * which is not able to decide which child interrupt interrupt handler typo: "interrupt interrupt" > + * should be called. > + * > + * Note: The caller is expected to handle the ack, clear, mask and > + * unmask issues if necessary. > + */ > +irqreturn_t > +handle_dumb_demux_irq(unsigned int irq, struct irq_desc *desc) > +{ > + irqreturn_t retval = IRQ_NONE; > + > + raw_spin_lock(&desc->lock); > + > + if (!irq_may_run(desc)) > + goto out_unlock; > + > + desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING); > + kstat_incr_irqs_this_cpu(irq, desc); > + > + if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) { > + desc->istate |= IRQS_PENDING; > + goto out_unlock; > + } > + > + retval = handle_irq_event_no_spurious_check(desc); > + > +out_unlock: > + raw_spin_unlock(&desc->lock); > + > + return retval; > +} > +EXPORT_SYMBOL_GPL(handle_dumb_demux_irq); > +#endif /* CONFIG_DUMB_IRQ_DEMUX_CHIP */ > + > /* > * Called unconditionally from handle_level_irq() and only for oneshot > * interrupts from handle_fasteoi_irq() > diff --git a/kernel/irq/dumb-demux-chip.c b/kernel/irq/dumb-demux-chip.c > new file mode 100644 > index 0000000..8e2de1d > --- /dev/null > +++ b/kernel/irq/dumb-demux-chip.c > @@ -0,0 +1,140 @@ > +/* > + * Library implementing common dumb irq demux chip functions > + * > + * Copyright (C) 2015, Boris Brezillon License here, please. > + */ > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/irq.h> > +#include <linux/slab.h> > +#include <linux/export.h> > +#include <linux/irq.h> > +#include <linux/irqdomain.h> > +#include <linux/irqchip/chained_irq.h> > +#include <linux/interrupt.h> > +#include <linux/kernel_stat.h> > +#include <linux/syscore_ops.h> > + > +#include "internals.h" > + > +static void irq_dumb_demux_mask(struct irq_data *d) > +{ > + struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d); > + > + clear_bit(d->hwirq, &demux->unmasked); > + > + if (!demux->unmasked) > + disable_irq_nosync(demux->src_irq); > +} > + > +static void irq_dumb_demux_unmask(struct irq_data *d) > +{ > + struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d); > + bool enable_src_irq = !demux->unmasked; Why this additional "bool" unlike the other function above? > + > + set_bit(d->hwirq, &demux->unmasked); > + > + if (enable_src_irq) > + enable_irq(demux->src_irq); > +} > + > +static struct irq_chip irq_dumb_demux_chip = { > + .name = "dumb-demux-irq", > + .irq_mask = irq_dumb_demux_mask, > + .irq_unmask = irq_dumb_demux_unmask, > +}; > + > +/* > + * Separate lockdep class for interrupt chip which can nest irq_desc > + * lock. > + */ > +static struct lock_class_key irq_nested_lock_class; > + > +/* > + * irq_map_dumb_demux_chip - Map a dumb demux chip for an irq domain > + */ > +static int irq_map_dumb_demux_chip(struct irq_domain *d, > + unsigned int virq, > + irq_hw_number_t hw_irq) > +{ > + struct irq_chip_dumb_demux *demux = d->host_data; > + > + if (!test_bit(hw_irq, &demux->available)) > + return -EINVAL; > + > + if (demux->flags & IRQ_DD_INIT_NESTED_LOCK) > + irq_set_lockdep_class(virq, &irq_nested_lock_class); > + > + irq_set_chip(virq, &irq_dumb_demux_chip); > + irq_set_chip_data(virq, demux); > + irq_modify_status(virq, demux->irq_flags_to_clear, > + demux->irq_flags_to_set); > + > + return 0; > +} > + > +struct irq_domain_ops irq_dumb_demux_domain_ops = { > + .map = irq_map_dumb_demux_chip, > + .xlate = irq_domain_xlate_onecell, > +}; > +EXPORT_SYMBOL_GPL(irq_dumb_demux_domain_ops); > + > +/** > + * irq_dumb_demux_handler - Dumb demux flow handler > + * @irq: Virtual irq number > + * @irq_desc: irq descriptor > + */ > +void irq_dumb_demux_handler(unsigned int irq, struct irq_desc *desc) > +{ > + struct irq_chip_dumb_demux *demux = irq_get_handler_data(irq); > + struct irq_chip *chip = irq_desc_get_chip(desc); > + irqreturn_t ret = IRQ_NONE; > + int i; > + > + chained_irq_enter(chip, desc); > + for_each_set_bit(i, &demux->unmasked, BITS_PER_LONG) { > + int demuxed_irq = irq_find_mapping(demux->domain, i); > + struct irq_desc *desc = irq_to_desc(demuxed_irq); > + > + ret |= handle_dumb_demux_irq(demuxed_irq, desc); > + } > + chained_irq_exit(chip, desc); > + > + if (!noirqdebug) > + note_interrupt(irq, desc, ret); > +} > +EXPORT_SYMBOL_GPL(irq_dumb_demux_handler); > + > +/** > + * irq_alloc_dumb_demux_chip - Allocate a dumb demux chip > + * @src_irq: irq feeding the dumb demux chip > + * @dd_flags: irq_dumb_demux_flags flags > + * @valid_irqs: Bitmask representing valid irqs > + * @clr_flags: irq_flags to clear when mapping an interrupt > + * @set_flags: irq_flags to set when mapping an interrupt Nit. not same order as the function parameters... > + */ > +struct irq_chip_dumb_demux * > +irq_alloc_dumb_demux_chip(unsigned int src_irq, > + unsigned long valid_irqs, > + unsigned int clr_flags, > + unsigned int set_flags, > + unsigned int dd_flags) > +{ > + struct irq_chip_dumb_demux *demux; > + > + if (!src_irq) > + return ERR_PTR(-EINVAL); > + > + demux = kzalloc(sizeof(*demux), GFP_KERNEL); > + if (!demux) > + return ERR_PTR(-ENOMEM); > + > + demux->available = valid_irqs; > + demux->flags = dd_flags; > + demux->src_irq = src_irq; > + demux->irq_flags_to_clear = clr_flags; > + demux->irq_flags_to_set = set_flags; > + > + return demux; > +} > +EXPORT_SYMBOL_GPL(irq_alloc_dumb_demux_chip); > diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c > index 6354802..f786850 100644 > --- a/kernel/irq/handle.c > +++ b/kernel/irq/handle.c > @@ -131,7 +131,8 @@ void __irq_wake_thread(struct irq_desc *desc, struct irqaction *action) > } > > irqreturn_t > -handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action) > +handle_irq_event_percpu_no_spurious_check(struct irq_desc *desc, > + struct irqaction *action) > { > irqreturn_t retval = IRQ_NONE; > unsigned int flags = 0, irq = desc->irq_data.irq; > @@ -175,8 +176,18 @@ handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action) > > add_interrupt_randomness(irq, flags); > > + return retval; > +} > + > +irqreturn_t > +handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action) > +{ > + irqreturn_t retval; > + > + retval = handle_irq_event_percpu_no_spurious_check(desc, action); > + > if (!noirqdebug) > - note_interrupt(irq, desc, retval); > + note_interrupt(desc->irq_data.irq, desc, retval); > return retval; > } > > @@ -195,3 +206,19 @@ irqreturn_t handle_irq_event(struct irq_desc *desc) > irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS); > return ret; > } > + > +irqreturn_t handle_irq_event_no_spurious_check(struct irq_desc *desc) > +{ > + struct irqaction *action = desc->action; > + irqreturn_t ret; > + > + desc->istate &= ~IRQS_PENDING; > + irqd_set(&desc->irq_data, IRQD_IRQ_INPROGRESS); > + raw_spin_unlock(&desc->lock); > + > + ret = handle_irq_event_percpu_no_spurious_check(desc, action); > + > + raw_spin_lock(&desc->lock); > + irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS); > + return ret; > +} > diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h > index df553b0..fe056fb 100644 > --- a/kernel/irq/internals.h > +++ b/kernel/irq/internals.h > @@ -90,6 +90,9 @@ extern void init_kstat_irqs(struct irq_desc *desc, int node, int nr); > > irqreturn_t handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action); > irqreturn_t handle_irq_event(struct irq_desc *desc); > +irqreturn_t handle_irq_event_percpu_no_spurious_check(struct irq_desc *desc, > + struct irqaction *action); > +irqreturn_t handle_irq_event_no_spurious_check(struct irq_desc *desc); > > /* Resending of interrupts :*/ > void check_irq_resend(struct irq_desc *desc, unsigned int irq); Otherwise, it look okay to me: Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> I'm still a little bit frighten about the additional latency added to the timer interrupt. But if it's a valid way to solve the problem for these older SoCs, I'm for it... Bye,
On Wed, 14 Jan 2015 14:36:42 +0100 Nicolas Ferre <nicolas.ferre@atmel.com> wrote: > Le 13/01/2015 19:46, Boris Brezillon a écrit : > > Some interrupt controllers are multiplexing several peripheral IRQs on > > a single interrupt line. > > While this is not a problem for most IRQs (as long as all peripherals > > request the interrupt with IRQF_SHARED flag set), multiplexing timers and > > other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND > > and !IRQF_NO_SUSPEND is prohibited). > > > > Create a dumb irq demultiplexer which simply forwards interrupts to all > > peripherals (exactly what's happening with IRQ_SHARED) but keep a unique > > irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND > > and !IRQF_NO_SUSPEND mix on a given interrupt. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > --- > > drivers/irqchip/Kconfig | 4 ++ > > drivers/irqchip/Makefile | 1 + > > drivers/irqchip/irq-dumb-demux.c | 70 ++++++++++++++++++++ > > include/linux/irq.h | 49 ++++++++++++++ > > include/linux/irqdomain.h | 1 + > > kernel/irq/Kconfig | 5 ++ > > kernel/irq/Makefile | 1 + > > kernel/irq/chip.c | 41 ++++++++++++ > > kernel/irq/dumb-demux-chip.c | 140 +++++++++++++++++++++++++++++++++++++++ > > kernel/irq/handle.c | 31 ++++++++- > > kernel/irq/internals.h | 3 + > > 11 files changed, 344 insertions(+), 2 deletions(-) > > create mode 100644 drivers/irqchip/irq-dumb-demux.c > > create mode 100644 kernel/irq/dumb-demux-chip.c > > > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > > index cc79d2a..8a9df88 100644 > > --- a/drivers/irqchip/Kconfig > > +++ b/drivers/irqchip/Kconfig > > @@ -70,6 +70,10 @@ config BRCMSTB_L2_IRQ > > select GENERIC_IRQ_CHIP > > select IRQ_DOMAIN > > > > +config DUMB_DEMUX_IRQ > > + bool > > + select DUMB_IRQ_DEMUX_CHIP > > + > > config DW_APB_ICTL > > bool > > select GENERIC_IRQ_CHIP > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > > index 9516a32..77f3c51 100644 > > --- a/drivers/irqchip/Makefile > > +++ b/drivers/irqchip/Makefile > > @@ -8,6 +8,7 @@ obj-$(CONFIG_ARCH_MVEBU) += irq-armada-370-xp.o > > obj-$(CONFIG_ARCH_MXS) += irq-mxs.o > > obj-$(CONFIG_ARCH_S3C24XX) += irq-s3c24xx.o > > obj-$(CONFIG_DW_APB_ICTL) += irq-dw-apb-ictl.o > > +obj-$(CONFIG_DUMB_DEMUX_IRQ) += irq-dumb-demux.o > > obj-$(CONFIG_METAG) += irq-metag-ext.o > > obj-$(CONFIG_METAG_PERFCOUNTER_IRQS) += irq-metag.o > > obj-$(CONFIG_ARCH_MOXART) += irq-moxart.o > > diff --git a/drivers/irqchip/irq-dumb-demux.c b/drivers/irqchip/irq-dumb-demux.c > > new file mode 100644 > > index 0000000..dfa05ce > > --- /dev/null > > +++ b/drivers/irqchip/irq-dumb-demux.c > > @@ -0,0 +1,70 @@ > > Maybe add a little file header here. It's always better. Sure, I just forgot it. > > +#ifdef CONFIG_DUMB_IRQ_DEMUX_CHIP > > +/** > > + * handle_dumb_demux_irq - Dumb demuxer irq handle function. > > + * @irq: the interrupt number > > + * @desc: the interrupt description structure for this irq > > + * > > + * Dumb demux interrupts are sent from a demultiplexing interrupt handler > > + * which is not able to decide which child interrupt interrupt handler > > typo: "interrupt interrupt" I'll fix that. > > > + * should be called. > > + * > > + * Note: The caller is expected to handle the ack, clear, mask and > > + * unmask issues if necessary. > > + */ [...] > > + > > /* > > * Called unconditionally from handle_level_irq() and only for oneshot > > * interrupts from handle_fasteoi_irq() > > diff --git a/kernel/irq/dumb-demux-chip.c b/kernel/irq/dumb-demux-chip.c > > new file mode 100644 > > index 0000000..8e2de1d > > --- /dev/null > > +++ b/kernel/irq/dumb-demux-chip.c > > @@ -0,0 +1,140 @@ > > +/* > > + * Library implementing common dumb irq demux chip functions > > + * > > + * Copyright (C) 2015, Boris Brezillon > > License here, please. Yep, I'll add it. > > > + */ > > +#include <linux/err.h> > > +#include <linux/io.h> > > +#include <linux/irq.h> > > +#include <linux/slab.h> > > +#include <linux/export.h> > > +#include <linux/irq.h> > > +#include <linux/irqdomain.h> > > +#include <linux/irqchip/chained_irq.h> > > +#include <linux/interrupt.h> > > +#include <linux/kernel_stat.h> > > +#include <linux/syscore_ops.h> > > + > > +#include "internals.h" > > + > > +static void irq_dumb_demux_mask(struct irq_data *d) > > +{ > > + struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d); > > + > > + clear_bit(d->hwirq, &demux->unmasked); > > + > > + if (!demux->unmasked) > > + disable_irq_nosync(demux->src_irq); > > +} > > + > > +static void irq_dumb_demux_unmask(struct irq_data *d) > > +{ > > + struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d); > > + bool enable_src_irq = !demux->unmasked; > > Why this additional "bool" unlike the other function above? Because set_bit will modify the unmasked status and we must check if it is equal to 0 (in other terms, all irqs are masked) before modifying it in order to know whether we should enable the src irq or not. > > > + > > + set_bit(d->hwirq, &demux->unmasked); > > + > > + if (enable_src_irq) > > + enable_irq(demux->src_irq); > > +} > > + [...] > > + > > +/** > > + * irq_alloc_dumb_demux_chip - Allocate a dumb demux chip > > + * @src_irq: irq feeding the dumb demux chip > > + * @dd_flags: irq_dumb_demux_flags flags > > + * @valid_irqs: Bitmask representing valid irqs > > + * @clr_flags: irq_flags to clear when mapping an interrupt > > + * @set_flags: irq_flags to set when mapping an interrupt > > Nit. not same order as the function parameters... I'll fix that. Thanks, Boris
Le 14/01/2015 15:03, Boris Brezillon a écrit : > On Wed, 14 Jan 2015 14:36:42 +0100 > Nicolas Ferre <nicolas.ferre@atmel.com> wrote: > >> Le 13/01/2015 19:46, Boris Brezillon a écrit : >>> Some interrupt controllers are multiplexing several peripheral IRQs on >>> a single interrupt line. >>> While this is not a problem for most IRQs (as long as all peripherals >>> request the interrupt with IRQF_SHARED flag set), multiplexing timers and >>> other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND >>> and !IRQF_NO_SUSPEND is prohibited). >>> >>> Create a dumb irq demultiplexer which simply forwards interrupts to all >>> peripherals (exactly what's happening with IRQ_SHARED) but keep a unique >>> irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND >>> and !IRQF_NO_SUSPEND mix on a given interrupt. >>> >>> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> >>> --- >>> drivers/irqchip/Kconfig | 4 ++ >>> drivers/irqchip/Makefile | 1 + >>> drivers/irqchip/irq-dumb-demux.c | 70 ++++++++++++++++++++ >>> include/linux/irq.h | 49 ++++++++++++++ >>> include/linux/irqdomain.h | 1 + >>> kernel/irq/Kconfig | 5 ++ >>> kernel/irq/Makefile | 1 + >>> kernel/irq/chip.c | 41 ++++++++++++ >>> kernel/irq/dumb-demux-chip.c | 140 +++++++++++++++++++++++++++++++++++++++ >>> kernel/irq/handle.c | 31 ++++++++- >>> kernel/irq/internals.h | 3 + >>> 11 files changed, 344 insertions(+), 2 deletions(-) >>> create mode 100644 drivers/irqchip/irq-dumb-demux.c >>> create mode 100644 kernel/irq/dumb-demux-chip.c [..] >>> +static void irq_dumb_demux_mask(struct irq_data *d) >>> +{ >>> + struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d); >>> + >>> + clear_bit(d->hwirq, &demux->unmasked); >>> + >>> + if (!demux->unmasked) >>> + disable_irq_nosync(demux->src_irq); >>> +} >>> + >>> +static void irq_dumb_demux_unmask(struct irq_data *d) >>> +{ >>> + struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d); >>> + bool enable_src_irq = !demux->unmasked; >> >> Why this additional "bool" unlike the other function above? > > Because set_bit will modify the unmasked status and we must check if it > is equal to 0 (in other terms, all irqs are masked) before modifying it > in order to know whether we should enable the src irq or not. pfffff! ok, sorry for the noise then ;-) >>> + >>> + set_bit(d->hwirq, &demux->unmasked); >>> + >>> + if (enable_src_irq) >>> + enable_irq(demux->src_irq); >>> +} >>> + [...] Bye,
On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner <tglx@linutronix.de> wrote: > On Tue, 13 Jan 2015, Rob Herring wrote: >> On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon >> <boris.brezillon@free-electrons.com> wrote: >> > Some interrupt controllers are multiplexing several peripheral IRQs on >> > a single interrupt line. >> > While this is not a problem for most IRQs (as long as all peripherals >> > request the interrupt with IRQF_SHARED flag set), multiplexing timers and >> > other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND >> > and !IRQF_NO_SUSPEND is prohibited). >> > >> > Create a dumb irq demultiplexer which simply forwards interrupts to all >> > peripherals (exactly what's happening with IRQ_SHARED) but keep a unique >> > irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND >> > and !IRQF_NO_SUSPEND mix on a given interrupt. >> >> This really seems like a work-around for how IRQF_SHARED works. It > > It's a workaround for a short coming of IRQF_SHARED. > > IRQF_SHARED has a massive short coming versus suspend and wakeup > interrupts. If one of the demultiplexed interrupts is a valid wakeup > source then we have no sane way to express this with IRQF_SHARED > simply because the drivers need to be aware whether they run on stupid > or well designed hardware. Unfortunately, the drivers will still have to know this. They cannot assume that they can call disable_irq and their device irq state does not matter. Perhaps we need a debug feature such that disable_irq/enable_irq are nops with IRQF_SHARED? >> seems like what is really desired is just per handler disabling. It is > > So you want a magic API like disable/enable_irq_action()? > > Certainly not. Agreed. > You'd open just another can of worms which will bring us abuse and > hard to debug problems because driver writers think it's a good idea > to use it for random purposes. > > Aside of that it would add another conditional into the interrupt > delivery hotpath which is not desired either. > >> fragile in that devices can deadlock the system if the drivers don't >> disable the interrupt source before calling disable_irq. But unlike > > Any misdesigned driver can do that for you. > >> IRQF_SHARED, there is nothing explicit in the driver indicating it is >> designed to work properly with a shared interrupt line. > > IRQF_SHARED is a pretty bad indicator. Look at all the drivers which > slap this flag onto request_irq() and have no single line of code > which makes sure that the driver would ever work on a shared line. > > If it's just for annotational purposes, we can add a new IRQF flag, > which is required to request such a interrupt line. > >> I see no reason to accept this into DT either. We already can support >> shared lines and modeling an OR gate as an interrupt controller is >> pointless. > > It's absolutely not pointless. > > All attempts to work around that have resulted in horrible bandaids so > far. That's why I guided Boris to implement this dummy demultiplexing > mechanism. It solves the problem at hand nicely without adding nasty > hackarounds into the suspend/resume code and inflicting platform > knowledge on multi-platform device drivers. This change will break on old kernels with a new dtb. Would you be happy if a BIOS update required a new kernel? Fixing this for any platform requires a dtb update which may not be possible on some platforms. I don't have a problem with this breakage for 1 platform and the at91 guys may not care, but we'd ultimately be changing how all shared irqs are specified for all DT. Maybe we decide that this is how we want to describe things, but that needs much wider discussion and agreement. > If you have a proper solution for the problem at hand which > > - avoids the demux dummy > > - works straight forward with suspend/resume/wakeup > > - does not add horrible new APIs > > - does not add conditionals to the interrupt hotpath > > - does not inflict platform knowledge about interrupt chip details > on drivers > > then I'm happy to take it. > > But as long as you can't come up with anything sane, the demux dummy > is the best solution for this problem we've seen so far. What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a suspended action list? This would leave only the actions with IRQF_NO_SUSPEND set in the active action list. The cost would be a pointer in irq_desc and moving the actions during suspend and resume. There are probably ways to do this demux irqchip without a DT change. Since we can't just move Linux irq numbers to different irq_chips during request_irq, we would have to parse the DT up front to find all shared interrupts and create a demux irqchip for them. That wouldn't be very efficient, but is straight-forward. Then we'd have to handle the translation into Linux irq numbers correctly which is probably the more difficult part. Rob
Hi Rob, On Wed, 14 Jan 2015 16:24:32 -0600 Rob Herring <robherring2@gmail.com> wrote: > On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner <tglx@linutronix.de> wrote: > > On Tue, 13 Jan 2015, Rob Herring wrote: > >> On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon > >> <boris.brezillon@free-electrons.com> wrote: > >> > Some interrupt controllers are multiplexing several peripheral IRQs on > >> > a single interrupt line. > >> > While this is not a problem for most IRQs (as long as all peripherals > >> > request the interrupt with IRQF_SHARED flag set), multiplexing timers and > >> > other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND > >> > and !IRQF_NO_SUSPEND is prohibited). > >> > > >> > Create a dumb irq demultiplexer which simply forwards interrupts to all > >> > peripherals (exactly what's happening with IRQ_SHARED) but keep a unique > >> > irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND > >> > and !IRQF_NO_SUSPEND mix on a given interrupt. > >> > >> This really seems like a work-around for how IRQF_SHARED works. It > > > > It's a workaround for a short coming of IRQF_SHARED. > > > > IRQF_SHARED has a massive short coming versus suspend and wakeup > > interrupts. If one of the demultiplexed interrupts is a valid wakeup > > source then we have no sane way to express this with IRQF_SHARED > > simply because the drivers need to be aware whether they run on stupid > > or well designed hardware. > > Unfortunately, the drivers will still have to know this. They cannot > assume that they can call disable_irq and their device irq state does > not matter. > > Perhaps we need a debug feature such that disable_irq/enable_irq are > nops with IRQF_SHARED? > > >> seems like what is really desired is just per handler disabling. It is > > > > So you want a magic API like disable/enable_irq_action()? > > > > Certainly not. > > Agreed. > > > You'd open just another can of worms which will bring us abuse and > > hard to debug problems because driver writers think it's a good idea > > to use it for random purposes. > > > > Aside of that it would add another conditional into the interrupt > > delivery hotpath which is not desired either. > > > >> fragile in that devices can deadlock the system if the drivers don't > >> disable the interrupt source before calling disable_irq. But unlike > > > > Any misdesigned driver can do that for you. > > > >> IRQF_SHARED, there is nothing explicit in the driver indicating it is > >> designed to work properly with a shared interrupt line. > > > > IRQF_SHARED is a pretty bad indicator. Look at all the drivers which > > slap this flag onto request_irq() and have no single line of code > > which makes sure that the driver would ever work on a shared line. > > > > If it's just for annotational purposes, we can add a new IRQF flag, > > which is required to request such a interrupt line. > > > >> I see no reason to accept this into DT either. We already can support > >> shared lines and modeling an OR gate as an interrupt controller is > >> pointless. > > > > It's absolutely not pointless. > > > > All attempts to work around that have resulted in horrible bandaids so > > far. That's why I guided Boris to implement this dummy demultiplexing > > mechanism. It solves the problem at hand nicely without adding nasty > > hackarounds into the suspend/resume code and inflicting platform > > knowledge on multi-platform device drivers. > > This change will break on old kernels with a new dtb. Would you be > happy if a BIOS update required a new kernel? Fixing this for any > platform requires a dtb update which may not be possible on some > platforms. I don't have a problem with this breakage for 1 platform > and the at91 guys may not care, but we'd ultimately be changing how > all shared irqs are specified for all DT. Maybe we decide that this is > how we want to describe things, but that needs much wider discussion > and agreement. I tried really hard on finding a DT representation that would not break the DT ABI, but didn't find any easy solution. How about keeping all platforms with the shared irq pattern, except for those that really have an irq line shared by a timer and several other devices (not sure yet, but at91 seems to be the only impacted platform so far). > > > If you have a proper solution for the problem at hand which > > > > - avoids the demux dummy > > > > - works straight forward with suspend/resume/wakeup > > > > - does not add horrible new APIs > > > > - does not add conditionals to the interrupt hotpath > > > > - does not inflict platform knowledge about interrupt chip details > > on drivers > > > > then I'm happy to take it. > > > > But as long as you can't come up with anything sane, the demux dummy > > is the best solution for this problem we've seen so far. > > What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a > suspended action list? This would leave only the actions with > IRQF_NO_SUSPEND set in the active action list. The cost would be a > pointer in irq_desc and moving the actions during suspend and resume. That really looks like what I suggested here [1]. > > There are probably ways to do this demux irqchip without a DT change. > Since we can't just move Linux irq numbers to different irq_chips > during request_irq, we would have to parse the DT up front to find all > shared interrupts and create a demux irqchip for them. That wouldn't > be very efficient, but is straight-forward. Then we'd have to handle > the translation into Linux irq numbers correctly which is probably the > more difficult part. I'm really interested in seeing how you would do this. Best Reagrds, Boris [1]https://lkml.org/lkml/2014/12/15/551
On Wed, 14 Jan 2015, Rob Herring wrote: > On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner <tglx@linutronix.de> wrote: > > All attempts to work around that have resulted in horrible bandaids so > > far. That's why I guided Boris to implement this dummy demultiplexing > > mechanism. It solves the problem at hand nicely without adding nasty > > hackarounds into the suspend/resume code and inflicting platform > > knowledge on multi-platform device drivers. > > This change will break on old kernels with a new dtb. Would you be > happy if a BIOS update required a new kernel? Fixing this for any > platform requires a dtb update which may not be possible on some > platforms. I don't have a problem with this breakage for 1 platform > and the at91 guys may not care, but we'd ultimately be changing how > all shared irqs are specified for all DT. Maybe we decide that this is > how we want to describe things, but that needs much wider discussion > and agreement. We do not change shared interrupts in any way. We provide an alternative mechanism for braindead hardware. And if the at91 folks are fine with the DT change, then it's their decision. Nothing forces this on everyone. > > If you have a proper solution for the problem at hand which > > > > - avoids the demux dummy > > > > - works straight forward with suspend/resume/wakeup > > > > - does not add horrible new APIs > > > > - does not add conditionals to the interrupt hotpath > > > > - does not inflict platform knowledge about interrupt chip details > > on drivers > > > > then I'm happy to take it. > > > > But as long as you can't come up with anything sane, the demux dummy > > is the best solution for this problem we've seen so far. > > What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a > suspended action list? This would leave only the actions with > IRQF_NO_SUSPEND set in the active action list. The cost would be a > pointer in irq_desc and moving the actions during suspend and resume. That's exactly what we want NOT. Because it prevents us to do proper sanity checks for IRQF_NO_SUSPEND. We've been there and I rejected it for that very reason. > There are probably ways to do this demux irqchip without a DT change. What's the problem with a DT change for a single platform, if the maintainers are willing to take it and deal with the fallout? And in case of AT91 the new kernel will simply work with the old DT and just emit the same warning vs. the IRQF_NO_SUSPEND mismatch. Older kernels won't work with a new DT, but that's about it. Aside of that, I'm quite amused about your DT update worries. DTs break with every kernel version on very popular platforms in very weird and subtle ways. Thanks, tglx
Le 15/01/2015 10:11, Thomas Gleixner a écrit : > On Wed, 14 Jan 2015, Rob Herring wrote: >> On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner <tglx@linutronix.de> wrote: >>> All attempts to work around that have resulted in horrible bandaids so >>> far. That's why I guided Boris to implement this dummy demultiplexing >>> mechanism. It solves the problem at hand nicely without adding nasty >>> hackarounds into the suspend/resume code and inflicting platform >>> knowledge on multi-platform device drivers. >> >> This change will break on old kernels with a new dtb. Would you be >> happy if a BIOS update required a new kernel? Fixing this for any >> platform requires a dtb update which may not be possible on some >> platforms. I don't have a problem with this breakage for 1 platform >> and the at91 guys may not care, but we'd ultimately be changing how >> all shared irqs are specified for all DT. Maybe we decide that this is >> how we want to describe things, but that needs much wider discussion >> and agreement. > > We do not change shared interrupts in any way. We provide an > alternative mechanism for braindead hardware. And if the at91 folks Let me add subtle details: "Old braindead hardware, with possibility to use alternative set of timers which doesn't have shared interrupt lines" ;-) > are fine with the DT change, then it's their decision. Nothing forces > this on everyone. Yes I do agree to change DT. >>> If you have a proper solution for the problem at hand which >>> >>> - avoids the demux dummy >>> >>> - works straight forward with suspend/resume/wakeup >>> >>> - does not add horrible new APIs >>> >>> - does not add conditionals to the interrupt hotpath >>> >>> - does not inflict platform knowledge about interrupt chip details >>> on drivers >>> >>> then I'm happy to take it. >>> >>> But as long as you can't come up with anything sane, the demux dummy >>> is the best solution for this problem we've seen so far. >> >> What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a >> suspended action list? This would leave only the actions with >> IRQF_NO_SUSPEND set in the active action list. The cost would be a >> pointer in irq_desc and moving the actions during suspend and resume. > > That's exactly what we want NOT. Because it prevents us to do proper > sanity checks for IRQF_NO_SUSPEND. We've been there and I rejected it > for that very reason. > >> There are probably ways to do this demux irqchip without a DT change. > > What's the problem with a DT change for a single platform, if the > maintainers are willing to take it and deal with the fallout? > > And in case of AT91 the new kernel will simply work with the old DT > and just emit the same warning vs. the IRQF_NO_SUSPEND mismatch. Older > kernels won't work with a new DT, but that's about it. > > Aside of that, I'm quite amused about your DT update worries. DTs > break with every kernel version on very popular platforms in very > weird and subtle ways. DT on AT91 is a Work In Progress (for 3.5 years) and the facts have told us that DT updates were absolutely necessary. So, for now, I agree to change DT as far as AT91 is concerned. Bye,
Le 14/01/2015 23:55, Boris Brezillon a écrit : > Hi Rob, > > On Wed, 14 Jan 2015 16:24:32 -0600 > Rob Herring <robherring2@gmail.com> wrote: > >> On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner <tglx@linutronix.de> wrote: >>> On Tue, 13 Jan 2015, Rob Herring wrote: >>>> On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon >>>> <boris.brezillon@free-electrons.com> wrote: >>>>> Some interrupt controllers are multiplexing several peripheral IRQs on >>>>> a single interrupt line. >>>>> While this is not a problem for most IRQs (as long as all peripherals >>>>> request the interrupt with IRQF_SHARED flag set), multiplexing timers and >>>>> other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND >>>>> and !IRQF_NO_SUSPEND is prohibited). >>>>> >>>>> Create a dumb irq demultiplexer which simply forwards interrupts to all >>>>> peripherals (exactly what's happening with IRQ_SHARED) but keep a unique >>>>> irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND >>>>> and !IRQF_NO_SUSPEND mix on a given interrupt. >>>> >>>> This really seems like a work-around for how IRQF_SHARED works. It >>> >>> It's a workaround for a short coming of IRQF_SHARED. >>> >>> IRQF_SHARED has a massive short coming versus suspend and wakeup >>> interrupts. If one of the demultiplexed interrupts is a valid wakeup >>> source then we have no sane way to express this with IRQF_SHARED >>> simply because the drivers need to be aware whether they run on stupid >>> or well designed hardware. >> >> Unfortunately, the drivers will still have to know this. They cannot >> assume that they can call disable_irq and their device irq state does >> not matter. >> >> Perhaps we need a debug feature such that disable_irq/enable_irq are >> nops with IRQF_SHARED? >> >>>> seems like what is really desired is just per handler disabling. It is >>> >>> So you want a magic API like disable/enable_irq_action()? >>> >>> Certainly not. >> >> Agreed. >> >>> You'd open just another can of worms which will bring us abuse and >>> hard to debug problems because driver writers think it's a good idea >>> to use it for random purposes. >>> >>> Aside of that it would add another conditional into the interrupt >>> delivery hotpath which is not desired either. >>> >>>> fragile in that devices can deadlock the system if the drivers don't >>>> disable the interrupt source before calling disable_irq. But unlike >>> >>> Any misdesigned driver can do that for you. >>> >>>> IRQF_SHARED, there is nothing explicit in the driver indicating it is >>>> designed to work properly with a shared interrupt line. >>> >>> IRQF_SHARED is a pretty bad indicator. Look at all the drivers which >>> slap this flag onto request_irq() and have no single line of code >>> which makes sure that the driver would ever work on a shared line. >>> >>> If it's just for annotational purposes, we can add a new IRQF flag, >>> which is required to request such a interrupt line. >>> >>>> I see no reason to accept this into DT either. We already can support >>>> shared lines and modeling an OR gate as an interrupt controller is >>>> pointless. >>> >>> It's absolutely not pointless. >>> >>> All attempts to work around that have resulted in horrible bandaids so >>> far. That's why I guided Boris to implement this dummy demultiplexing >>> mechanism. It solves the problem at hand nicely without adding nasty >>> hackarounds into the suspend/resume code and inflicting platform >>> knowledge on multi-platform device drivers. >> >> This change will break on old kernels with a new dtb. Would you be >> happy if a BIOS update required a new kernel? Fixing this for any >> platform requires a dtb update which may not be possible on some >> platforms. I don't have a problem with this breakage for 1 platform >> and the at91 guys may not care, but we'd ultimately be changing how >> all shared irqs are specified for all DT. Maybe we decide that this is >> how we want to describe things, but that needs much wider discussion >> and agreement. > > I tried really hard on finding a DT representation that would not break > the DT ABI, but didn't find any easy solution. > How about keeping all platforms with the shared irq pattern, except for > those that really have an irq line shared by a timer and several other > devices (not sure yet, but at91 seems to be the only impacted platform > so far). > >> >>> If you have a proper solution for the problem at hand which >>> >>> - avoids the demux dummy >>> >>> - works straight forward with suspend/resume/wakeup >>> >>> - does not add horrible new APIs >>> >>> - does not add conditionals to the interrupt hotpath >>> >>> - does not inflict platform knowledge about interrupt chip details >>> on drivers >>> >>> then I'm happy to take it. >>> >>> But as long as you can't come up with anything sane, the demux dummy >>> is the best solution for this problem we've seen so far. >> >> What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a >> suspended action list? This would leave only the actions with >> IRQF_NO_SUSPEND set in the active action list. The cost would be a >> pointer in irq_desc and moving the actions during suspend and resume. > > That really looks like what I suggested here [1]. > >> >> There are probably ways to do this demux irqchip without a DT change. >> Since we can't just move Linux irq numbers to different irq_chips >> during request_irq, we would have to parse the DT up front to find all >> shared interrupts and create a demux irqchip for them. That wouldn't >> be very efficient, but is straight-forward. Then we'd have to handle >> the translation into Linux irq numbers correctly which is probably the >> more difficult part. > > I'm really interested in seeing how you would do this. Well, it really looks like a ping-pong game and Boris tried hard to explore both options. I suspect that he already made his best to solve this problem. In the meantime, we begin to see people using our (old, badly designed, okay-okay) hardware and have the surprise of hitting this warning on Mainline kernel. As we have several set of timers, they may even not be affected by the core issue by selecting a timer which doesn't have a shared interrupt line. I mean, we discovered this when it was already written (in linux-next actually) and tried to address the issue quickly. But now, we urgently need a solution or a temporary workaround and Boris wrote two. I have the feeling that we have to move on by selecting one. Clearly, if the only issue remaining is that one solution may cause DT changes for AT91, well, we can cope with this and accept it. If we don't come to a conclusion quickly, maybe we would have to consider removing this warning while we are working on the issue: believe me, it's frightening and sometimes even inappropriate when people select a different set of timers for their clocksource/clockevent after boot time. Best regards, > [1]https://lkml.org/lkml/2014/12/15/551 >
On Thu, Jan 15, 2015 at 3:11 AM, Thomas Gleixner <tglx@linutronix.de> wrote: > On Wed, 14 Jan 2015, Rob Herring wrote: >> On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner <tglx@linutronix.de> wrote: >> > All attempts to work around that have resulted in horrible bandaids so >> > far. That's why I guided Boris to implement this dummy demultiplexing >> > mechanism. It solves the problem at hand nicely without adding nasty >> > hackarounds into the suspend/resume code and inflicting platform >> > knowledge on multi-platform device drivers. >> >> This change will break on old kernels with a new dtb. Would you be >> happy if a BIOS update required a new kernel? Fixing this for any >> platform requires a dtb update which may not be possible on some >> platforms. I don't have a problem with this breakage for 1 platform >> and the at91 guys may not care, but we'd ultimately be changing how >> all shared irqs are specified for all DT. Maybe we decide that this is >> how we want to describe things, but that needs much wider discussion >> and agreement. > > We do not change shared interrupts in any way. We provide an > alternative mechanism for braindead hardware. And if the at91 folks > are fine with the DT change, then it's their decision. Nothing forces > this on everyone. We are changing how shared interrupts are described in DT. We don't need 2 ways to describe them. We could say this is only for AT91 and continue to describe shared interrupts as has been done forever. Then the next platform that hits this problem will have to go thru the same ABI breakage. Or we change DT practices to describe all shared interrupts with a demux node. Given the way DTs are incrementally created, it is not something we can check with review or tools, so we will still have the same ABI breakage problem. >> > If you have a proper solution for the problem at hand which >> > >> > - avoids the demux dummy >> > >> > - works straight forward with suspend/resume/wakeup >> > >> > - does not add horrible new APIs >> > >> > - does not add conditionals to the interrupt hotpath >> > >> > - does not inflict platform knowledge about interrupt chip details >> > on drivers >> > >> > then I'm happy to take it. >> > >> > But as long as you can't come up with anything sane, the demux dummy >> > is the best solution for this problem we've seen so far. >> >> What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a >> suspended action list? This would leave only the actions with >> IRQF_NO_SUSPEND set in the active action list. The cost would be a >> pointer in irq_desc and moving the actions during suspend and resume. > > That's exactly what we want NOT. Because it prevents us to do proper > sanity checks for IRQF_NO_SUSPEND. We've been there and I rejected it > for that very reason. > >> There are probably ways to do this demux irqchip without a DT change. > > What's the problem with a DT change for a single platform, if the > maintainers are willing to take it and deal with the fallout? What's the solution for a platform that an ABI break is not okay and can't deal with the fallout? > And in case of AT91 the new kernel will simply work with the old DT > and just emit the same warning vs. the IRQF_NO_SUSPEND mismatch. Older > kernels won't work with a new DT, but that's about it. > > Aside of that, I'm quite amused about your DT update worries. DTs > break with every kernel version on very popular platforms in very > weird and subtle ways. Someone has to. It is a problem that we need to get better at preventing. It's an ABI. I don't think I need to explain what that means. Certainly there are folks who would just prefer DT to be a kernel data structure so we don't have to worry about this ABI nonsense. Rob
On Thu, 15 Jan 2015, Rob Herring wrote: > On Thu, Jan 15, 2015 at 3:11 AM, Thomas Gleixner <tglx@linutronix.de> wrote: > > On Wed, 14 Jan 2015, Rob Herring wrote: > > > We do not change shared interrupts in any way. We provide an > > alternative mechanism for braindead hardware. And if the at91 folks > > are fine with the DT change, then it's their decision. Nothing forces > > this on everyone. > > We are changing how shared interrupts are described in DT. We don't > need 2 ways to describe them. We could say this is only for AT91 and > continue to describe shared interrupts as has been done forever. Then > the next platform that hits this problem will have to go thru the same > ABI breakage. Or we change DT practices to describe all shared > interrupts with a demux node. Given the way DTs are incrementally > created, it is not something we can check with review or tools, so we > will still have the same ABI breakage problem. This is not describing the proper shared interrupts. This is a special case for a special case of braindamaged hardware. Whats wrong with doing that? We dont have to change that for all shared interrupts because 99% of them have a proper hardware implementation and are not affected by this. What's wrong with serving the AT91 with a proper solution, which does NOT inflict horrible hacks into the core code and does NOT weaken sanity checks and does NOT require irq chip specific knowledge in device drivers? > >> There are probably ways to do this demux irqchip without a DT change. So far you have not provided any useful hint how to do so. > > What's the problem with a DT change for a single platform, if the > > maintainers are willing to take it and deal with the fallout? > > What's the solution for a platform that an ABI break is not okay and > can't deal with the fallout? There is no other platform affected. This is a break for a specific set of devices and the 'fallout' is confined, well known and accepted. So what's your problem, really? Thanks, tglx
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index cc79d2a..8a9df88 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -70,6 +70,10 @@ config BRCMSTB_L2_IRQ select GENERIC_IRQ_CHIP select IRQ_DOMAIN +config DUMB_DEMUX_IRQ + bool + select DUMB_IRQ_DEMUX_CHIP + config DW_APB_ICTL bool select GENERIC_IRQ_CHIP diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index 9516a32..77f3c51 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -8,6 +8,7 @@ obj-$(CONFIG_ARCH_MVEBU) += irq-armada-370-xp.o obj-$(CONFIG_ARCH_MXS) += irq-mxs.o obj-$(CONFIG_ARCH_S3C24XX) += irq-s3c24xx.o obj-$(CONFIG_DW_APB_ICTL) += irq-dw-apb-ictl.o +obj-$(CONFIG_DUMB_DEMUX_IRQ) += irq-dumb-demux.o obj-$(CONFIG_METAG) += irq-metag-ext.o obj-$(CONFIG_METAG_PERFCOUNTER_IRQS) += irq-metag.o obj-$(CONFIG_ARCH_MOXART) += irq-moxart.o diff --git a/drivers/irqchip/irq-dumb-demux.c b/drivers/irqchip/irq-dumb-demux.c new file mode 100644 index 0000000..dfa05ce --- /dev/null +++ b/drivers/irqchip/irq-dumb-demux.c @@ -0,0 +1,70 @@ +#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/irqdomain.h> +#include <linux/of_irq.h> +#include <linux/slab.h> + +#include "irqchip.h" + +static int __init dumb_irq_demux_of_init(struct device_node *node, + struct device_node *parent) +{ + struct irq_chip_dumb_demux *demux; + unsigned int irq; + u32 valid_irqs; + int ret; + + irq = irq_of_parse_and_map(node, 0); + if (!irq) { + pr_err("Failed to retrieve dumb irq demuxer source\n"); + return -EINVAL; + } + + ret = of_property_read_u32(node, "irqs", &valid_irqs); + if (ret) { + pr_err("Invalid of missing 'irqs' property\n"); + return ret; + } + + demux = irq_alloc_dumb_demux_chip(irq, valid_irqs, + IRQ_NOREQUEST | IRQ_NOPROBE | + IRQ_NOAUTOEN, 0, 0); + if (!demux) { + pr_err("Failed to allocate dumb irq demuxer struct\n"); + return -ENOMEM; + } + + demux->domain = irq_domain_add_linear(node, BITS_PER_LONG, + &irq_dumb_demux_domain_ops, + demux); + if (!demux->domain) { + ret = -ENOMEM; + goto err_free_demux; + } + + ret = irq_set_handler_data(irq, demux); + if (ret) { + pr_err("Failed to assign handler data\n"); + goto err_free_domain; + } + + irq_set_chained_handler(irq, irq_dumb_demux_handler); + + /* + * Disable the src irq (automatically enabled by + * irq_set_chained_handler) to prevent irqs from happening while + * nobody requested any of the demuxed irqs. + */ + disable_irq(irq); + + return 0; + +err_free_domain: + irq_domain_remove(demux->domain); + +err_free_demux: + kfree(demux); + + return ret; +} +IRQCHIP_DECLARE(dumb_irq_demux, "irqchip-dumb-demux", dumb_irq_demux_of_init); diff --git a/include/linux/irq.h b/include/linux/irq.h index d09ec7a..ae8fa21 100644 --- a/include/linux/irq.h +++ b/include/linux/irq.h @@ -445,6 +445,10 @@ extern void handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc); extern void handle_edge_irq(unsigned int irq, struct irq_desc *desc); extern void handle_edge_eoi_irq(unsigned int irq, struct irq_desc *desc); extern void handle_simple_irq(unsigned int irq, struct irq_desc *desc); +#ifdef CONFIG_DUMB_IRQ_DEMUX_CHIP +extern irqreturn_t handle_dumb_demux_irq(unsigned int irq, + struct irq_desc *desc); +#endif extern void handle_percpu_irq(unsigned int irq, struct irq_desc *desc); extern void handle_percpu_devid_irq(unsigned int irq, struct irq_desc *desc); extern void handle_bad_irq(unsigned int irq, struct irq_desc *desc); @@ -862,4 +866,49 @@ static inline u32 irq_reg_readl(struct irq_chip_generic *gc, return readl(gc->reg_base + reg_offset); } +/** + * enum irq_dumb_demux_flags - Initialization flags for generic irq chips + * @IRQ_DD_INIT_NESTED_LOCK: Set the lock class of the irqs to nested for + * irq chips which need to call irq_set_wake() on + * the parent irq. Usually GPIO implementations + */ +enum irq_dumb_demux_flags { + IRQ_DD_INIT_NESTED_LOCK = 1 << 0, +}; + +#ifdef CONFIG_DUMB_IRQ_DEMUX_CHIP +/** + * struct irq_chip_dumb_demux - Dumb demultiplexer irq chip data structure + * @domain: irq domain pointer + * @available: Bitfield of valid irqs + * @unmasked: Bitfield containing irqs status + * @flags: irq_dumb_demux_flags flags + * @src_irq: irq feeding the dumb demux chip + * + * Note, that irq_chip_generic can have multiple irq_chip_type + * implementations which can be associated to a particular irq line of + * an irq_chip_generic instance. That allows to share and protect + * state in an irq_chip_generic instance when we need to implement + * different flow mechanisms (level/edge) for it. + */ +struct irq_chip_dumb_demux { + struct irq_domain *domain; + unsigned long available; + unsigned long unmasked; + unsigned int flags; + unsigned int src_irq; + unsigned int irq_flags_to_clear; + unsigned int irq_flags_to_set; +}; + +void irq_dumb_demux_handler(unsigned int irq, struct irq_desc *desc); + +struct irq_chip_dumb_demux * +irq_alloc_dumb_demux_chip(unsigned int src_irq, + unsigned long valid_irqs, + unsigned int clr_flags, + unsigned int set_flags, + unsigned int dd_flags); +#endif /* CONFIG_DUMB_IRQ_DEMUX_CHIP */ + #endif /* _LINUX_IRQ_H */ diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h index 676d730..1de3808 100644 --- a/include/linux/irqdomain.h +++ b/include/linux/irqdomain.h @@ -80,6 +80,7 @@ struct irq_domain_ops { }; extern struct irq_domain_ops irq_generic_chip_ops; +extern struct irq_domain_ops irq_dumb_demux_domain_ops; struct irq_domain_chip_generic; diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig index 9a76e3b..d01554a 100644 --- a/kernel/irq/Kconfig +++ b/kernel/irq/Kconfig @@ -51,6 +51,11 @@ config GENERIC_IRQ_CHIP bool select IRQ_DOMAIN +# Dumb interrupt demuxer chip implementation +config DUMB_IRQ_DEMUX_CHIP + bool + select IRQ_DOMAIN + # Generic irq_domain hw <--> linux irq number translation config IRQ_DOMAIN bool diff --git a/kernel/irq/Makefile b/kernel/irq/Makefile index d121235..1cd4e42 100644 --- a/kernel/irq/Makefile +++ b/kernel/irq/Makefile @@ -7,3 +7,4 @@ obj-$(CONFIG_PROC_FS) += proc.o obj-$(CONFIG_GENERIC_PENDING_IRQ) += migration.o obj-$(CONFIG_PM_SLEEP) += pm.o obj-$(CONFIG_GENERIC_MSI_IRQ) += msi.o +obj-$(CONFIG_DUMB_IRQ_DEMUX_CHIP) += dumb-demux-chip.o diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index 6f1c7a5..d2a5c96 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -405,6 +405,47 @@ out_unlock: } EXPORT_SYMBOL_GPL(handle_simple_irq); +#ifdef CONFIG_DUMB_IRQ_DEMUX_CHIP +/** + * handle_dumb_demux_irq - Dumb demuxer irq handle function. + * @irq: the interrupt number + * @desc: the interrupt description structure for this irq + * + * Dumb demux interrupts are sent from a demultiplexing interrupt handler + * which is not able to decide which child interrupt interrupt handler + * should be called. + * + * Note: The caller is expected to handle the ack, clear, mask and + * unmask issues if necessary. + */ +irqreturn_t +handle_dumb_demux_irq(unsigned int irq, struct irq_desc *desc) +{ + irqreturn_t retval = IRQ_NONE; + + raw_spin_lock(&desc->lock); + + if (!irq_may_run(desc)) + goto out_unlock; + + desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING); + kstat_incr_irqs_this_cpu(irq, desc); + + if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) { + desc->istate |= IRQS_PENDING; + goto out_unlock; + } + + retval = handle_irq_event_no_spurious_check(desc); + +out_unlock: + raw_spin_unlock(&desc->lock); + + return retval; +} +EXPORT_SYMBOL_GPL(handle_dumb_demux_irq); +#endif /* CONFIG_DUMB_IRQ_DEMUX_CHIP */ + /* * Called unconditionally from handle_level_irq() and only for oneshot * interrupts from handle_fasteoi_irq() diff --git a/kernel/irq/dumb-demux-chip.c b/kernel/irq/dumb-demux-chip.c new file mode 100644 index 0000000..8e2de1d --- /dev/null +++ b/kernel/irq/dumb-demux-chip.c @@ -0,0 +1,140 @@ +/* + * Library implementing common dumb irq demux chip functions + * + * Copyright (C) 2015, Boris Brezillon + */ +#include <linux/err.h> +#include <linux/io.h> +#include <linux/irq.h> +#include <linux/slab.h> +#include <linux/export.h> +#include <linux/irq.h> +#include <linux/irqdomain.h> +#include <linux/irqchip/chained_irq.h> +#include <linux/interrupt.h> +#include <linux/kernel_stat.h> +#include <linux/syscore_ops.h> + +#include "internals.h" + +static void irq_dumb_demux_mask(struct irq_data *d) +{ + struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d); + + clear_bit(d->hwirq, &demux->unmasked); + + if (!demux->unmasked) + disable_irq_nosync(demux->src_irq); +} + +static void irq_dumb_demux_unmask(struct irq_data *d) +{ + struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d); + bool enable_src_irq = !demux->unmasked; + + set_bit(d->hwirq, &demux->unmasked); + + if (enable_src_irq) + enable_irq(demux->src_irq); +} + +static struct irq_chip irq_dumb_demux_chip = { + .name = "dumb-demux-irq", + .irq_mask = irq_dumb_demux_mask, + .irq_unmask = irq_dumb_demux_unmask, +}; + +/* + * Separate lockdep class for interrupt chip which can nest irq_desc + * lock. + */ +static struct lock_class_key irq_nested_lock_class; + +/* + * irq_map_dumb_demux_chip - Map a dumb demux chip for an irq domain + */ +static int irq_map_dumb_demux_chip(struct irq_domain *d, + unsigned int virq, + irq_hw_number_t hw_irq) +{ + struct irq_chip_dumb_demux *demux = d->host_data; + + if (!test_bit(hw_irq, &demux->available)) + return -EINVAL; + + if (demux->flags & IRQ_DD_INIT_NESTED_LOCK) + irq_set_lockdep_class(virq, &irq_nested_lock_class); + + irq_set_chip(virq, &irq_dumb_demux_chip); + irq_set_chip_data(virq, demux); + irq_modify_status(virq, demux->irq_flags_to_clear, + demux->irq_flags_to_set); + + return 0; +} + +struct irq_domain_ops irq_dumb_demux_domain_ops = { + .map = irq_map_dumb_demux_chip, + .xlate = irq_domain_xlate_onecell, +}; +EXPORT_SYMBOL_GPL(irq_dumb_demux_domain_ops); + +/** + * irq_dumb_demux_handler - Dumb demux flow handler + * @irq: Virtual irq number + * @irq_desc: irq descriptor + */ +void irq_dumb_demux_handler(unsigned int irq, struct irq_desc *desc) +{ + struct irq_chip_dumb_demux *demux = irq_get_handler_data(irq); + struct irq_chip *chip = irq_desc_get_chip(desc); + irqreturn_t ret = IRQ_NONE; + int i; + + chained_irq_enter(chip, desc); + for_each_set_bit(i, &demux->unmasked, BITS_PER_LONG) { + int demuxed_irq = irq_find_mapping(demux->domain, i); + struct irq_desc *desc = irq_to_desc(demuxed_irq); + + ret |= handle_dumb_demux_irq(demuxed_irq, desc); + } + chained_irq_exit(chip, desc); + + if (!noirqdebug) + note_interrupt(irq, desc, ret); +} +EXPORT_SYMBOL_GPL(irq_dumb_demux_handler); + +/** + * irq_alloc_dumb_demux_chip - Allocate a dumb demux chip + * @src_irq: irq feeding the dumb demux chip + * @dd_flags: irq_dumb_demux_flags flags + * @valid_irqs: Bitmask representing valid irqs + * @clr_flags: irq_flags to clear when mapping an interrupt + * @set_flags: irq_flags to set when mapping an interrupt + */ +struct irq_chip_dumb_demux * +irq_alloc_dumb_demux_chip(unsigned int src_irq, + unsigned long valid_irqs, + unsigned int clr_flags, + unsigned int set_flags, + unsigned int dd_flags) +{ + struct irq_chip_dumb_demux *demux; + + if (!src_irq) + return ERR_PTR(-EINVAL); + + demux = kzalloc(sizeof(*demux), GFP_KERNEL); + if (!demux) + return ERR_PTR(-ENOMEM); + + demux->available = valid_irqs; + demux->flags = dd_flags; + demux->src_irq = src_irq; + demux->irq_flags_to_clear = clr_flags; + demux->irq_flags_to_set = set_flags; + + return demux; +} +EXPORT_SYMBOL_GPL(irq_alloc_dumb_demux_chip); diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c index 6354802..f786850 100644 --- a/kernel/irq/handle.c +++ b/kernel/irq/handle.c @@ -131,7 +131,8 @@ void __irq_wake_thread(struct irq_desc *desc, struct irqaction *action) } irqreturn_t -handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action) +handle_irq_event_percpu_no_spurious_check(struct irq_desc *desc, + struct irqaction *action) { irqreturn_t retval = IRQ_NONE; unsigned int flags = 0, irq = desc->irq_data.irq; @@ -175,8 +176,18 @@ handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action) add_interrupt_randomness(irq, flags); + return retval; +} + +irqreturn_t +handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action) +{ + irqreturn_t retval; + + retval = handle_irq_event_percpu_no_spurious_check(desc, action); + if (!noirqdebug) - note_interrupt(irq, desc, retval); + note_interrupt(desc->irq_data.irq, desc, retval); return retval; } @@ -195,3 +206,19 @@ irqreturn_t handle_irq_event(struct irq_desc *desc) irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS); return ret; } + +irqreturn_t handle_irq_event_no_spurious_check(struct irq_desc *desc) +{ + struct irqaction *action = desc->action; + irqreturn_t ret; + + desc->istate &= ~IRQS_PENDING; + irqd_set(&desc->irq_data, IRQD_IRQ_INPROGRESS); + raw_spin_unlock(&desc->lock); + + ret = handle_irq_event_percpu_no_spurious_check(desc, action); + + raw_spin_lock(&desc->lock); + irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS); + return ret; +} diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h index df553b0..fe056fb 100644 --- a/kernel/irq/internals.h +++ b/kernel/irq/internals.h @@ -90,6 +90,9 @@ extern void init_kstat_irqs(struct irq_desc *desc, int node, int nr); irqreturn_t handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action); irqreturn_t handle_irq_event(struct irq_desc *desc); +irqreturn_t handle_irq_event_percpu_no_spurious_check(struct irq_desc *desc, + struct irqaction *action); +irqreturn_t handle_irq_event_no_spurious_check(struct irq_desc *desc); /* Resending of interrupts :*/ void check_irq_resend(struct irq_desc *desc, unsigned int irq);
Some interrupt controllers are multiplexing several peripheral IRQs on a single interrupt line. While this is not a problem for most IRQs (as long as all peripherals request the interrupt with IRQF_SHARED flag set), multiplexing timers and other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND and !IRQF_NO_SUSPEND is prohibited). Create a dumb irq demultiplexer which simply forwards interrupts to all peripherals (exactly what's happening with IRQ_SHARED) but keep a unique irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND and !IRQF_NO_SUSPEND mix on a given interrupt. Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> --- drivers/irqchip/Kconfig | 4 ++ drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-dumb-demux.c | 70 ++++++++++++++++++++ include/linux/irq.h | 49 ++++++++++++++ include/linux/irqdomain.h | 1 + kernel/irq/Kconfig | 5 ++ kernel/irq/Makefile | 1 + kernel/irq/chip.c | 41 ++++++++++++ kernel/irq/dumb-demux-chip.c | 140 +++++++++++++++++++++++++++++++++++++++ kernel/irq/handle.c | 31 ++++++++- kernel/irq/internals.h | 3 + 11 files changed, 344 insertions(+), 2 deletions(-) create mode 100644 drivers/irqchip/irq-dumb-demux.c create mode 100644 kernel/irq/dumb-demux-chip.c