* [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
2015-01-13 18:46 ` [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation Boris Brezillon
@ 2015-01-13 21:00 ` Thomas Gleixner
2015-01-14 8:31 ` Boris Brezillon
2015-01-14 3:26 ` Rob Herring
2015-01-14 13:36 ` Nicolas Ferre
2 siblings, 1 reply; 32+ messages in thread
From: Thomas Gleixner @ 2015-01-13 21:00 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
2015-01-13 21:00 ` Thomas Gleixner
@ 2015-01-14 8:31 ` Boris Brezillon
0 siblings, 0 replies; 32+ messages in thread
From: Boris Brezillon @ 2015-01-14 8:31 UTC (permalink / raw)
To: linux-arm-kernel
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
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
2015-01-13 18:46 ` [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation Boris Brezillon
2015-01-13 21:00 ` Thomas Gleixner
@ 2015-01-14 3:26 ` Rob Herring
2015-01-14 8:22 ` Boris Brezillon
2015-01-14 10:36 ` Thomas Gleixner
2015-01-14 13:36 ` Nicolas Ferre
2 siblings, 2 replies; 32+ messages in thread
From: Rob Herring @ 2015-01-14 3:26 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
2015-01-14 3:26 ` Rob Herring
@ 2015-01-14 8:22 ` Boris Brezillon
2015-01-14 10:36 ` Thomas Gleixner
1 sibling, 0 replies; 32+ messages in thread
From: Boris Brezillon @ 2015-01-14 8:22 UTC (permalink / raw)
To: linux-arm-kernel
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
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
2015-01-14 3:26 ` Rob Herring
2015-01-14 8:22 ` Boris Brezillon
@ 2015-01-14 10:36 ` Thomas Gleixner
2015-01-14 22:24 ` Rob Herring
1 sibling, 1 reply; 32+ messages in thread
From: Thomas Gleixner @ 2015-01-14 10:36 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
2015-01-14 10:36 ` Thomas Gleixner
@ 2015-01-14 22:24 ` Rob Herring
2015-01-14 22:55 ` Boris Brezillon
2015-01-15 9:11 ` Thomas Gleixner
0 siblings, 2 replies; 32+ messages in thread
From: Rob Herring @ 2015-01-14 22:24 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
2015-01-14 22:24 ` Rob Herring
@ 2015-01-14 22:55 ` Boris Brezillon
2015-01-15 9:44 ` Nicolas Ferre
2015-01-15 9:11 ` Thomas Gleixner
1 sibling, 1 reply; 32+ messages in thread
From: Boris Brezillon @ 2015-01-14 22:55 UTC (permalink / raw)
To: linux-arm-kernel
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
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
2015-01-14 22:55 ` Boris Brezillon
@ 2015-01-15 9:44 ` Nicolas Ferre
0 siblings, 0 replies; 32+ messages in thread
From: Nicolas Ferre @ 2015-01-15 9:44 UTC (permalink / raw)
To: linux-arm-kernel
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
>
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
2015-01-14 22:24 ` Rob Herring
2015-01-14 22:55 ` Boris Brezillon
@ 2015-01-15 9:11 ` Thomas Gleixner
2015-01-15 9:26 ` Nicolas Ferre
2015-01-15 15:40 ` Rob Herring
1 sibling, 2 replies; 32+ messages in thread
From: Thomas Gleixner @ 2015-01-15 9:11 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
2015-01-15 9:11 ` Thomas Gleixner
@ 2015-01-15 9:26 ` Nicolas Ferre
2015-01-15 15:40 ` Rob Herring
1 sibling, 0 replies; 32+ messages in thread
From: Nicolas Ferre @ 2015-01-15 9:26 UTC (permalink / raw)
To: linux-arm-kernel
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,
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
2015-01-15 9:11 ` Thomas Gleixner
2015-01-15 9:26 ` Nicolas Ferre
@ 2015-01-15 15:40 ` Rob Herring
2015-01-20 13:08 ` Thomas Gleixner
1 sibling, 1 reply; 32+ messages in thread
From: Rob Herring @ 2015-01-15 15:40 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
2015-01-15 15:40 ` Rob Herring
@ 2015-01-20 13:08 ` Thomas Gleixner
0 siblings, 0 replies; 32+ messages in thread
From: Thomas Gleixner @ 2015-01-20 13:08 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
2015-01-13 18:46 ` [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation Boris Brezillon
2015-01-13 21:00 ` Thomas Gleixner
2015-01-14 3:26 ` Rob Herring
@ 2015-01-14 13:36 ` Nicolas Ferre
2015-01-14 14:03 ` Boris Brezillon
2 siblings, 1 reply; 32+ messages in thread
From: Nicolas Ferre @ 2015-01-14 13:36 UTC (permalink / raw)
To: linux-arm-kernel
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,
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
2015-01-14 13:36 ` Nicolas Ferre
@ 2015-01-14 14:03 ` Boris Brezillon
2015-01-14 14:43 ` Nicolas Ferre
0 siblings, 1 reply; 32+ messages in thread
From: Boris Brezillon @ 2015-01-14 14:03 UTC (permalink / raw)
To: linux-arm-kernel
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
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
2015-01-14 14:03 ` Boris Brezillon
@ 2015-01-14 14:43 ` Nicolas Ferre
0 siblings, 0 replies; 32+ messages in thread
From: Nicolas Ferre @ 2015-01-14 14:43 UTC (permalink / raw)
To: linux-arm-kernel
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,
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 32+ messages in thread