[PATCH 0/5] ARM: at91: fix irq_pm_install_action WARNING

[PATCH 0/5] ARM: at91: fix irq_pm_install_action WARNING

[Boris Brezillon]

Commit cab303be91dc47942bc25de33dc1140123540800 [1] introduced a WARN_ON
test which triggers a WARNING backtrace on at91 platforms.
While this WARN_ON is absolutely necessary to warn users that they should
not mix request with and without IRQF_NO_SUSPEND flags on shared IRQs,
there is no easy way to solve this issue on at91 platforms.

The main reason is that the init timer is often using a shared irq line
and thus request this irq with IRQF_NO_SUSPEND flag set, while other
peripherals request the same irq line without this flag.

We could deal with that by identifying whether a given peripheral is
connected to the init timer shared irq line and add the IRQF_NO_SUSPEND
in this case, but this implies adding the logic in all peripheral drivers
that could be connected to this shared irq.

This series takes the reverse approach: force IRQ users to specify that
they take care of disabling peripheral interrupts and that IRQ core can
safely leave the handler in a suspended state without having to bother
about spurious interrupts.
This is done by mean of a new IRQF_SUSPEND_NOACTION flag which tells the
core to move the action handler to a suspended list, thus preventing its
execution when we are in suspend mode.
Of course, specifying the IRQF_SUSPEND_NOACTION flag implies taking care
of masking/unmasking the peripheral interrupts in the suspend/resume
implementation.

The WARN_ON is kept, but now it is only triggered when (no_suspend_depth +
suspend_noaction_depth) and nr_actions are unbalanced.

The first patch introduces the IRQF_SUSPEND_NOACTION logic, while other
patches modify existing at91 drivers to specify this flag (and implement
suspend/resume if needed).


Best Regards,

Boris

[1]https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/kernel/irq/pm.c?id=cab303be91dc47942bc25de33dc1140123540800

Boris Brezillon (5):
  genirq: Support mixing IRQF_NO_SUSPEND/IRQF_SUSPEND on shared irqs
  clk: at91: implement suspend/resume for the PMC irqchip
  watchdog: at91sam9: request the irq with IRQF_NO_SUSPEND
  rtc: at91: request IRQs with IRQF_SUSPEND_NOACTION
  tty: serial: atmel: request IRQ with IRQF_SUSPEND_NOACTION

 drivers/clk/at91/pmc.c            | 20 ++++++++++-
 drivers/clk/at91/pmc.h            |  1 +
 drivers/rtc/rtc-at91rm9200.c      |  2 +-
 drivers/rtc/rtc-at91sam9.c        |  3 +-
 drivers/tty/serial/atmel_serial.c |  3 +-
 drivers/watchdog/at91sam9_wdt.c   |  3 +-
 include/linux/interrupt.h         | 11 ++++++
 include/linux/irqdesc.h           |  3 ++
 kernel/irq/pm.c                   | 71 +++++++++++++++++++++++++++++++++++++--
 9 files changed, 109 insertions(+), 8 deletions(-)

-- 
1.9.1

[PATCH 1/5] genirq: Support mixing IRQF_NO_SUSPEND/IRQF_SUSPEND on shared irqs

[Boris Brezillon]

The current implementation forbid sharing an irq line on devices that do
not request the same behavior on suspend/resume (controlled via the
IRQF_NO_SUSPEND/IRQF_FORCE_RESUME flags).

Add a flag (IRQF_SUSPEND_NOACTION) to specify that you don't want to be
called in suspend mode, and that you already took care of disabling the
interrupt on the device side.

The suspend_device_irq will now move actions specifying the
IRQF_SUSPEND_NOACTION into a temporary list so that they won't be called
when the interrupt is triggered, and resume_irq_actions restores the
suspended actions into the active action list.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 include/linux/interrupt.h | 11 ++++++++
 include/linux/irqdesc.h   |  3 ++
 kernel/irq/pm.c           | 71 +++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 82 insertions(+), 3 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index d9b05b5..aba3f36 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -57,6 +57,16 @@
  * IRQF_NO_THREAD - Interrupt cannot be threaded
  * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device
  *                resume time.
+ * IRQF_SUSPEND_NOACTION - When sharing an irq, a specific handler can ask to
+ *                         be disabled when entering suspend. This is
+ *                         particularly useful on shared irqs where at least
+ *                         one user is requesting IRQF_NO_SUSPEND while the
+ *                         others don't want to be active on suspend.
+ *                         Setting this flag implies taking the appropriate
+ *                         action to disable device interrupts when entering
+ *                         suspend, otherwise you might experience spurious
+ *                         interrupts.
+ *
  */
 #define IRQF_DISABLED		0x00000020
 #define IRQF_SHARED		0x00000080
@@ -70,6 +80,7 @@
 #define IRQF_FORCE_RESUME	0x00008000
 #define IRQF_NO_THREAD		0x00010000
 #define IRQF_EARLY_RESUME	0x00020000
+#define IRQF_SUSPEND_NOACTION	0x00040000
 
 #define IRQF_TIMER		(__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD)
 
diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index faf433a..64a577f 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -22,6 +22,7 @@ struct pt_regs;
  * @handle_irq:		highlevel irq-events handler
  * @preflow_handler:	handler called before the flow handler (currently used by sparc)
  * @action:		the irq action chain
+ * @suspended_action:	the irq suspended action chain
  * @status:		status information
  * @core_internal_state__do_not_mess_with_it: core internal status information
  * @depth:		disable-depth, for nested irq_disable() calls
@@ -54,6 +55,7 @@ struct irq_desc {
 	irq_preflow_handler_t	preflow_handler;
 #endif
 	struct irqaction	*action;	/* IRQ action list */
+	struct irqaction	*suspended_action; /* IRQ suspended action list */
 	unsigned int		status_use_accessors;
 	unsigned int		core_internal_state__do_not_mess_with_it;
 	unsigned int		depth;		/* nested irq disables */
@@ -79,6 +81,7 @@ struct irq_desc {
 	unsigned int		nr_actions;
 	unsigned int		no_suspend_depth;
 	unsigned int		force_resume_depth;
+	unsigned int		suspend_noaction_depth;
 #endif
 #ifdef CONFIG_PROC_FS
 	struct proc_dir_entry	*dir;
diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index 3ca5325..45446e1 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -35,17 +35,24 @@ void irq_pm_install_action(struct irq_desc *desc, struct irqaction *action)
 {
 	desc->nr_actions++;
 
+	if (action->flags & IRQF_SUSPEND_NOACTION) {
+		desc->suspend_noaction_depth++;
+		return;
+	}
+
 	if (action->flags & IRQF_FORCE_RESUME)
 		desc->force_resume_depth++;
 
 	WARN_ON_ONCE(desc->force_resume_depth &&
-		     desc->force_resume_depth != desc->nr_actions);
+		     (desc->force_resume_depth +
+		      desc->suspend_noaction_depth) != desc->nr_actions);
 
 	if (action->flags & IRQF_NO_SUSPEND)
 		desc->no_suspend_depth++;
 
 	WARN_ON_ONCE(desc->no_suspend_depth &&
-		     desc->no_suspend_depth != desc->nr_actions);
+		     (desc->no_suspend_depth +
+		      desc->suspend_noaction_depth) != desc->nr_actions);
 }
 
 /*
@@ -56,6 +63,11 @@ void irq_pm_remove_action(struct irq_desc *desc, struct irqaction *action)
 {
 	desc->nr_actions--;
 
+	if (action->flags & IRQF_SUSPEND_NOACTION) {
+		desc->suspend_noaction_depth--;
+		return;
+	}
+
 	if (action->flags & IRQF_FORCE_RESUME)
 		desc->force_resume_depth--;
 
@@ -63,11 +75,63 @@ void irq_pm_remove_action(struct irq_desc *desc, struct irqaction *action)
 		desc->no_suspend_depth--;
 }
 
+/*
+ * Move all irq actions that specifically set IRQF_SUSPEND_NOACTION
+ * into the suspended action list so that they won't be called if an
+ * interrupt happens.
+ */
+static void suspend_irq_actions(struct irq_desc *desc)
+{
+	struct irqaction *action;
+	struct irqaction *suspended_action = NULL;
+
+	for (action = desc->action; action; action = action->next) {
+		if (!(action->flags & IRQF_SUSPEND_NOACTION))
+			continue;
+
+		if (!suspended_action) {
+			suspended_action = action->next;
+		} else {
+			suspended_action->next = action;
+			suspended_action = action;
+		}
+	}
+
+	BUG_ON(!suspended_action);
+	suspended_action->next = NULL;
+}
+
+/*
+ * Restore all irq actions pushed into the suspended action list.
+ */
+static void resume_irq_actions(struct irq_desc *desc)
+{
+	struct irqaction *action;
+
+	if (!desc->suspended_action)
+		return;
+
+	for (action = desc->action; action && action->next;
+	     action = action->next)
+		;
+
+	BUG_ON(!action);
+	action->next = desc->suspended_action;
+	desc->suspended_action = NULL;
+}
+
 static bool suspend_device_irq(struct irq_desc *desc, int irq)
 {
-	if (!desc->action || desc->no_suspend_depth)
+	if (!desc->action)
 		return false;
 
+	if (desc->no_suspend_depth) {
+		if (desc->no_suspend_depth != desc->nr_actions)
+			suspend_irq_actions(desc);
+
+		return false;
+	}
+
 	if (irqd_is_wakeup_set(&desc->irq_data)) {
 		irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED);
 		/*
@@ -131,6 +195,7 @@ EXPORT_SYMBOL_GPL(suspend_device_irqs);
 static void resume_irq(struct irq_desc *desc, int irq)
 {
 	irqd_clear(&desc->irq_data, IRQD_WAKEUP_ARMED);
+	resume_irq_actions(desc);
 
 	if (desc->istate & IRQS_SUSPENDED)
 		goto resume;
-- 
1.9.1

[PATCH 1/5] genirq: Support mixing IRQF_NO_SUSPEND/IRQF_SUSPEND on shared irqs

[Rafael J. Wysocki]

On Monday, December 15, 2014 05:15:48 PM Boris Brezillon wrote:
> The current implementation forbid sharing an irq line on devices that do
> not request the same behavior on suspend/resume (controlled via the
> IRQF_NO_SUSPEND/IRQF_FORCE_RESUME flags).

IRQF_NO_SUSPEND is practically only for timers and IPIs now.  Any other
usages are strongly discouraged.

> Add a flag (IRQF_SUSPEND_NOACTION) to specify that you don't want to be
> called in suspend mode, and that you already took care of disabling the
> interrupt on the device side.
> 
> The suspend_device_irq will now move actions specifying the
> IRQF_SUSPEND_NOACTION into a temporary list so that they won't be called
> when the interrupt is triggered, and resume_irq_actions restores the
> suspended actions into the active action list.

Why is the current way of handling wakeup interrupts not sufficient?


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH 2/5] clk: at91: implement suspend/resume for the PMC irqchip

[Boris Brezillon]

The irq line used by the PMC block is shared with several peripherals
including the init timer which is registering the irq with IRQF_NO_SUSPEND.

To prevent the PMC irq handler from being called while suspended request
the irq with the IRQF_SUSPEND_NOACTION flag and implement proper suspend
resume functions to save and restore the active PMC IRQs.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/clk/at91/pmc.c | 20 +++++++++++++++++++-
 drivers/clk/at91/pmc.h |  1 +
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c
index 386999b..59753e7 100644
--- a/drivers/clk/at91/pmc.c
+++ b/drivers/clk/at91/pmc.c
@@ -80,12 +80,29 @@ static int pmc_irq_set_type(struct irq_data *d, unsigned type)
 	return 0;
 }
 
+static void pmc_irq_suspend(struct irq_data *d)
+{
+	struct at91_pmc *pmc = irq_data_get_irq_chip_data(d);
+
+	pmc->imr = pmc_read(pmc, AT91_PMC_IMR);
+	pmc_write(pmc, AT91_PMC_IDR, pmc->imr);
+}
+
+static void pmc_irq_resume(struct irq_data *d)
+{
+	struct at91_pmc *pmc = irq_data_get_irq_chip_data(d);
+
+	pmc_write(pmc, AT91_PMC_IER, pmc->imr);
+}
+
 static struct irq_chip pmc_irq = {
 	.name = "PMC",
 	.irq_disable = pmc_irq_mask,
 	.irq_mask = pmc_irq_mask,
 	.irq_unmask = pmc_irq_unmask,
 	.irq_set_type = pmc_irq_set_type,
+	.irq_suspend = pmc_irq_suspend,
+	.irq_resume = pmc_irq_resume,
 };
 
 static struct lock_class_key pmc_lock_class;
@@ -215,7 +232,8 @@ static struct at91_pmc *__init at91_pmc_init(struct device_node *np,
 		goto out_free_pmc;
 
 	pmc_write(pmc, AT91_PMC_IDR, 0xffffffff);
-	if (request_irq(pmc->virq, pmc_irq_handler, IRQF_SHARED, "pmc", pmc))
+	if (request_irq(pmc->virq, pmc_irq_handler,
+			IRQF_SHARED | IRQF_SUSPEND_NOACTION, "pmc", pmc))
 		goto out_remove_irqdomain;
 
 	return pmc;
diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h
index 52d2041..69abb08 100644
--- a/drivers/clk/at91/pmc.h
+++ b/drivers/clk/at91/pmc.h
@@ -33,6 +33,7 @@ struct at91_pmc {
 	spinlock_t lock;
 	const struct at91_pmc_caps *caps;
 	struct irq_domain *irqdomain;
+	u32 imr;
 };
 
 static inline void pmc_lock(struct at91_pmc *pmc)
-- 
1.9.1

[PATCH 3/5] watchdog: at91sam9: request the irq with IRQF_NO_SUSPEND

[Boris Brezillon]

The watchdog interrupt (only used when activating software watchdog)
shouldn't be suspended when entering suspend mode, because it is shared
with a timer device (which request the line with IRQF_NO_SUSPEND) and once
the watchdog "Mode Register" has been written, it cannot be changed (which
means we cannot disable the watchdog interrupt when entering suspend).

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/watchdog/at91sam9_wdt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index 6df9405..1443b3c 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -208,7 +208,8 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
 
 	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
 		err = request_irq(wdt->irq, wdt_interrupt,
-				  IRQF_SHARED | IRQF_IRQPOLL,
+				  IRQF_SHARED | IRQF_IRQPOLL |
+				  IRQF_NO_SUSPEND,
 				  pdev->name, wdt);
 		if (err)
 			return err;
-- 
1.9.1

[PATCH 4/5] rtc: at91: request IRQs with IRQF_SUSPEND_NOACTION

[Boris Brezillon]

The IRQ line connected to RTC blocks are often shared with a timer device
which request the IRQ with IRQF_NO_SUSPEND.
Since the RTC drivers are correctly disabling IRQs when entering suspend
we can safely request the IRQ with IRQF_SUSPEND_NOACTION so that irq core
can suspend this specific handler.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/rtc/rtc-at91rm9200.c | 2 +-
 drivers/rtc/rtc-at91sam9.c   | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
index 70a5d94..0ed504f 100644
--- a/drivers/rtc/rtc-at91rm9200.c
+++ b/drivers/rtc/rtc-at91rm9200.c
@@ -401,7 +401,7 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
 					AT91_RTC_CALEV);
 
 	ret = devm_request_irq(&pdev->dev, irq, at91_rtc_interrupt,
-				IRQF_SHARED,
+				IRQF_SHARED | IRQF_SUSPEND_NOACTION,
 				"at91_rtc", pdev);
 	if (ret) {
 		dev_err(&pdev->dev, "IRQ %d already in use.\n", irq);
diff --git a/drivers/rtc/rtc-at91sam9.c b/drivers/rtc/rtc-at91sam9.c
index 6b9aaf1..e7f8eed 100644
--- a/drivers/rtc/rtc-at91sam9.c
+++ b/drivers/rtc/rtc-at91sam9.c
@@ -421,7 +421,8 @@ static int at91_rtc_probe(struct platform_device *pdev)
 
 	/* register irq handler after we know what name we'll use */
 	ret = devm_request_irq(&pdev->dev, rtc->irq, at91_rtc_interrupt,
-				IRQF_SHARED, dev_name(&rtc->rtcdev->dev), rtc);
+				IRQF_SHARED | IRQF_SUSPEND_NOACTION,
+				dev_name(&rtc->rtcdev->dev), rtc);
 	if (ret) {
 		dev_dbg(&pdev->dev, "can't share IRQ %d?\n", rtc->irq);
 		return ret;
-- 
1.9.1

[PATCH 5/5] tty: serial: atmel: request IRQ with IRQF_SUSPEND_NOACTION

[Boris Brezillon]

The IRQ line connected to the DBGU UART is often shared with a timer device
which request the IRQ with IRQF_NO_SUSPEND.
Since the UART driver is correctly disabling IRQs when entering suspend
we can safely request the IRQ with IRQF_SUSPEND_NOACTION so that irq core
can suspend this specific handler.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/tty/serial/atmel_serial.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 4d848a2..0c252ff 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1725,7 +1725,8 @@ static int atmel_startup(struct uart_port *port)
 	/*
 	 * Allocate the IRQ
 	 */
-	retval = request_irq(port->irq, atmel_interrupt, IRQF_SHARED,
+	retval = request_irq(port->irq, atmel_interrupt,
+			IRQF_SHARED | IRQF_SUSPEND_NOACTION,
 			tty ? tty->name : "atmel_serial", port);
 	if (retval) {
 		dev_err(port->dev, "atmel_startup - Can't get irq\n");
-- 
1.9.1

[PATCH 0/5] ARM: at91: fix irq_pm_install_action WARNING

[Rafael J. Wysocki]

On Monday, December 15, 2014 05:15:47 PM Boris Brezillon wrote:
> Commit cab303be91dc47942bc25de33dc1140123540800 [1] introduced a WARN_ON
> test which triggers a WARNING backtrace on at91 platforms.

Pretty much as intended.

> While this WARN_ON is absolutely necessary to warn users that they should
> not mix request with and without IRQF_NO_SUSPEND flags on shared IRQs,
> there is no easy way to solve this issue on at91 platforms.
> 
> The main reason is that the init timer is often using a shared irq line
> and thus request this irq with IRQF_NO_SUSPEND flag set, while other
> peripherals request the same irq line without this flag.
> 
> We could deal with that by identifying whether a given peripheral is
> connected to the init timer shared irq line and add the IRQF_NO_SUSPEND
> in this case, but this implies adding the logic in all peripheral drivers
> that could be connected to this shared irq.
> 
> This series takes the reverse approach: force IRQ users to specify that
> they take care of disabling peripheral interrupts and that IRQ core can
> safely leave the handler in a suspended state without having to bother
> about spurious interrupts.
> This is done by mean of a new IRQF_SUSPEND_NOACTION flag which tells the
> core to move the action handler to a suspended list, thus preventing its
> execution when we are in suspend mode.
> Of course, specifying the IRQF_SUSPEND_NOACTION flag implies taking care
> of masking/unmasking the peripheral interrupts in the suspend/resume
> implementation.

Well, I'm not sure how much value is in all that to be honest.  The only
thing it helps with is to make the WARN_ON go away in some cases, while
the drivers in question need to make sure that they disable their interrupts
properly anyway, so what exactly is the purpose of the new irqaction
shuffling?

It might just be simpler to add a flag to suppress the WARN_ON that would be
set by the user of IRQF_NO_SUSPEND that is broken enough to have to share the
interrupt with others ...


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH 0/5] ARM: at91: fix irq_pm_install_action WARNING

[Rafael J. Wysocki]

On Monday, December 15, 2014 11:20:17 PM Rafael J. Wysocki wrote:
> On Monday, December 15, 2014 05:15:47 PM Boris Brezillon wrote:
> > Commit cab303be91dc47942bc25de33dc1140123540800 [1] introduced a WARN_ON
> > test which triggers a WARNING backtrace on at91 platforms.
> 
> Pretty much as intended.
> 
> > While this WARN_ON is absolutely necessary to warn users that they should
> > not mix request with and without IRQF_NO_SUSPEND flags on shared IRQs,
> > there is no easy way to solve this issue on at91 platforms.
> > 
> > The main reason is that the init timer is often using a shared irq line
> > and thus request this irq with IRQF_NO_SUSPEND flag set, while other
> > peripherals request the same irq line without this flag.
> > 
> > We could deal with that by identifying whether a given peripheral is
> > connected to the init timer shared irq line and add the IRQF_NO_SUSPEND
> > in this case, but this implies adding the logic in all peripheral drivers
> > that could be connected to this shared irq.
> > 
> > This series takes the reverse approach: force IRQ users to specify that
> > they take care of disabling peripheral interrupts and that IRQ core can
> > safely leave the handler in a suspended state without having to bother
> > about spurious interrupts.
> > This is done by mean of a new IRQF_SUSPEND_NOACTION flag which tells the
> > core to move the action handler to a suspended list, thus preventing its
> > execution when we are in suspend mode.
> > Of course, specifying the IRQF_SUSPEND_NOACTION flag implies taking care
> > of masking/unmasking the peripheral interrupts in the suspend/resume
> > implementation.
> 
> Well, I'm not sure how much value is in all that to be honest.  The only
> thing it helps with is to make the WARN_ON go away in some cases, while
> the drivers in question need to make sure that they disable their interrupts
> properly anyway, so what exactly is the purpose of the new irqaction
> shuffling?
> 
> It might just be simpler to add a flag to suppress the WARN_ON that would be
> set by the user of IRQF_NO_SUSPEND that is broken enough to have to share the
> interrupt with others ...

Or even set IRFQ_NO_SUSPEND for all of the users of this interrupt and add
comments to them explaining why it is set.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH 0/5] ARM: at91: fix irq_pm_install_action WARNING

[Boris Brezillon]

Hi Rafael,

On Mon, 15 Dec 2014 23:48:14 +0100
"Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:

> On Monday, December 15, 2014 11:20:17 PM Rafael J. Wysocki wrote:
> > On Monday, December 15, 2014 05:15:47 PM Boris Brezillon wrote:
> > > Commit cab303be91dc47942bc25de33dc1140123540800 [1] introduced a WARN_ON
> > > test which triggers a WARNING backtrace on at91 platforms.
> > 
> > Pretty much as intended.

Yep. BTW did you have other platforms/drivers in mind when proposing
this patch, or were you just trying to identify the offending ones ?

> > 
> > > While this WARN_ON is absolutely necessary to warn users that they should
> > > not mix request with and without IRQF_NO_SUSPEND flags on shared IRQs,
> > > there is no easy way to solve this issue on at91 platforms.
> > > 
> > > The main reason is that the init timer is often using a shared irq line
> > > and thus request this irq with IRQF_NO_SUSPEND flag set, while other
> > > peripherals request the same irq line without this flag.
> > > 
> > > We could deal with that by identifying whether a given peripheral is
> > > connected to the init timer shared irq line and add the IRQF_NO_SUSPEND
> > > in this case, but this implies adding the logic in all peripheral drivers
> > > that could be connected to this shared irq.
> > > 
> > > This series takes the reverse approach: force IRQ users to specify that
> > > they take care of disabling peripheral interrupts and that IRQ core can
> > > safely leave the handler in a suspended state without having to bother
> > > about spurious interrupts.
> > > This is done by mean of a new IRQF_SUSPEND_NOACTION flag which tells the
> > > core to move the action handler to a suspended list, thus preventing its
> > > execution when we are in suspend mode.
> > > Of course, specifying the IRQF_SUSPEND_NOACTION flag implies taking care
> > > of masking/unmasking the peripheral interrupts in the suspend/resume
> > > implementation.
> > 
> > Well, I'm not sure how much value is in all that to be honest.  The only
> > thing it helps with is to make the WARN_ON go away in some cases, while
> > the drivers in question need to make sure that they disable their interrupts
> > properly anyway, so what exactly is the purpose of the new irqaction
> > shuffling?
> > 
> > It might just be simpler to add a flag to suppress the WARN_ON that would be
> > set by the user of IRQF_NO_SUSPEND that is broken enough to have to share the
> > interrupt with others ...
> 
> Or even set IRFQ_NO_SUSPEND for all of the users of this interrupt and add
> comments to them explaining why it is set.
> 
> 

Maybe I should have sent this series as a RFC, anyway, I'm not trying
to make existing code more complicated just for fun, and I'm open to
any suggestion.

Actually I thought about adding a new flag (let's call it
IRQF_DONT_COMPLAIN for now ;-)) to remove those warnings (or specifying
IRFQ_NO_SUSPEND in all peripherals sharing the IRQ with the init
timer), but after discussing the problem with Thomas I decided to go
for the approach described in my cover letter.

Thomas, correct me if I'm wrong, but your concern about the
IRQF_DONT_COMPLAIN approach was that it was leaving interrupt handlers
of suspended devices in an active state (meaning that they could be
called in "suspend" or "early resume" state), and such devices might
not properly handle interrupts while being in a suspended state (clocks
and regulators disabled).
In at91 specific case this should not be an issue thought.

We have the same problem when setting IRFQ_NO_SUSPEND on all peripherals
sharing the IRQ with the init timer.
Moreover, I'd like to keep the core automatically disabling the IRQ when
the PMC, RTC, watchdog or DBGU (UART) peripherals have their own
dedicated IRQ (which is the case on Atmel sama5 SoCs).
This implies testing for the SoC version in each of these drivers and
adapting the request_irq call accordingly.

Thomas, Rafael, if both of you think I should either introduce a new
flag or specify IRFQ_NO_SUSPEND in all shared IRQ users, then I can go
for one of this solution.

BTW, have you heard about other platforms/drivers impacted by this
WARN_ON addition, and if so how did they solve the problem ?

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 0/5] ARM: at91: fix irq_pm_install_action WARNING

[Thomas Gleixner]

On Tue, 16 Dec 2014, Boris Brezillon wrote:
> On Mon, 15 Dec 2014 23:48:14 +0100
> "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> > Or even set IRFQ_NO_SUSPEND for all of the users of this interrupt and add
> > comments to them explaining why it is set.
> Actually I thought about adding a new flag (let's call it
> IRQF_DONT_COMPLAIN for now ;-)) to remove those warnings (or specifying
> IRFQ_NO_SUSPEND in all peripherals sharing the IRQ with the init
> timer), but after discussing the problem with Thomas I decided to go
> for the approach described in my cover letter.
> 
> Thomas, correct me if I'm wrong, but your concern about the
> IRQF_DONT_COMPLAIN approach was that it was leaving interrupt handlers
> of suspended devices in an active state (meaning that they could be
> called in "suspend" or "early resume" state), and such devices might
> not properly handle interrupts while being in a suspended state (clocks
> and regulators disabled).
> In at91 specific case this should not be an issue thought.
> 
> We have the same problem when setting IRFQ_NO_SUSPEND on all peripherals
> sharing the IRQ with the init timer.
> Moreover, I'd like to keep the core automatically disabling the IRQ when
> the PMC, RTC, watchdog or DBGU (UART) peripherals have their own
> dedicated IRQ (which is the case on Atmel sama5 SoCs).
> This implies testing for the SoC version in each of these drivers and
> adapting the request_irq call accordingly.

But still all those drivers must disable the interrupts at the device
level on suspend, right?

> Thomas, Rafael, if both of you think I should either introduce a new
> flag or specify IRFQ_NO_SUSPEND in all shared IRQ users, then I can go
> for one of this solution.

All of this really sucks. What about the following?

Install the timer interrupt as a demultiplexing interrupt.

Create a pseudo interrupt chip, which essentially does nothing, but
keeps track of the disabled state. Install handle_simple_irq as
handler for those "demux" interrupts. Then have:

struct data {
       u32 unmasked;
       u32 demuxavail;
};

static struct data demuxdata;

At init time you know how many of these demux interrupts are
available. So you set the demuxdata up, e.g. for 3 interrupts
connected:

	demuxdata.demuxavail = 0x07;

You install a pointer to demuxdata for all demux interrupts as irq
chip data and the simple handler.

And in the mask/unmask handlers you do:

mask(irqdata) {
       struct data *d = irq_data_get_irq_chip_data(irqdata);

       d->unmasked &= ~irqdata->mask;
       if (!d->unmasked)
       	  mask_demux_irq();
}

unmask(irqdata) {
       struct data *d = irq_data_get_irq_chip_data(irqdata);

       if (!d->unmasked)
       	  unmask_demux_irq();
       d->mask |= irqdata->mask;
}

Now the demuxhandler does:

    mask = demuxdata.demuxavail & demuxdata.unmasked;

    for_each_bit(bit, mask)
    	generic_handle_irq(demuxirq_start + bit);

So the handler wont be invoked for masked bits and handle_simple_irq()
will not call the device handler if the interrupt is marked disabled.

So in the suspend case all "demux" interrupts except those which are
marked NOSUSPEND are marked disabled and the handlers wont be invoked.

Locking and other details omitted.

That avoids the whole flag, action, whatever business for the price of
a really trivial demux mechanism. Everything just works. Even the irq
storm detector will just disable the parent interrupt once all
handlers return NONE often enough.

Your device drivers just work fine, as long as they disable the
interrupt at the device level on suspend, but they have to do that in
any case. Aside of that detail they are completely oblivous whether
they are connected to this "demux" chip or to a seperate irq line on
other devices.

> BTW, have you heard about other platforms/drivers impacted by this
> WARN_ON addition, and if so how did they solve the problem ?

Nothing so far. Seems not many hardware folks are that crazy.

But I have no objections to make this generic infrastructure as long
as it can be compiled out.

Thanks,

	tglx

[PATCH 0/5] ARM: at91: fix irq_pm_install_action WARNING

[Boris Brezillon]

Hi Thomas,

On Tue, 16 Dec 2014 11:03:55 +0100 (CET)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Tue, 16 Dec 2014, Boris Brezillon wrote:
> > On Mon, 15 Dec 2014 23:48:14 +0100
> > "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> > > Or even set IRFQ_NO_SUSPEND for all of the users of this interrupt and add
> > > comments to them explaining why it is set.
> > Actually I thought about adding a new flag (let's call it
> > IRQF_DONT_COMPLAIN for now ;-)) to remove those warnings (or specifying
> > IRFQ_NO_SUSPEND in all peripherals sharing the IRQ with the init
> > timer), but after discussing the problem with Thomas I decided to go
> > for the approach described in my cover letter.
> > 
> > Thomas, correct me if I'm wrong, but your concern about the
> > IRQF_DONT_COMPLAIN approach was that it was leaving interrupt handlers
> > of suspended devices in an active state (meaning that they could be
> > called in "suspend" or "early resume" state), and such devices might
> > not properly handle interrupts while being in a suspended state (clocks
> > and regulators disabled).
> > In at91 specific case this should not be an issue thought.
> > 
> > We have the same problem when setting IRFQ_NO_SUSPEND on all peripherals
> > sharing the IRQ with the init timer.
> > Moreover, I'd like to keep the core automatically disabling the IRQ when
> > the PMC, RTC, watchdog or DBGU (UART) peripherals have their own
> > dedicated IRQ (which is the case on Atmel sama5 SoCs).
> > This implies testing for the SoC version in each of these drivers and
> > adapting the request_irq call accordingly.
> 
> But still all those drivers must disable the interrupts at the device
> level on suspend, right?

Absolutely.

> 
> > Thomas, Rafael, if both of you think I should either introduce a new
> > flag or specify IRFQ_NO_SUSPEND in all shared IRQ users, then I can go
> > for one of this solution.
> 
> All of this really sucks. What about the following?
> 
> Install the timer interrupt as a demultiplexing interrupt.

I can try to hack the AIC irqchip driver to implement this demux logic,
but this logic can't be placed in the PIT (Periodic Interval Timer)
driver itself, because the shared IRQ line is used by the at91 clock
controller (PMC) which is providing the clock to the PIT device.
This gives the following dependency graph:

PIT =depends-on=> Master Clock =provided-by=> PMC =needs=> PMC IRQ.

Anyway, I'll give this solution another try.

Thanks for your detailed answer.

Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 0/5] ARM: at91: fix irq_pm_install_action WARNING

[Thomas Gleixner]

On Tue, 16 Dec 2014, Boris Brezillon wrote:
> Thomas Gleixner <tglx@linutronix.de> wrote:
> > Install the timer interrupt as a demultiplexing interrupt.
> 
> I can try to hack the AIC irqchip driver to implement this demux logic,
> but this logic can't be placed in the PIT (Periodic Interval Timer)
> driver itself, because the shared IRQ line is used by the at91 clock
> controller (PMC) which is providing the clock to the PIT device.
> This gives the following dependency graph:
> 
> PIT =depends-on=> Master Clock =provided-by=> PMC =needs=> PMC IRQ.

It never can be a part of a device driver. This is a property of the
interrupt controller which fails to provide a proper demux for this
irq line in the first place.

So you fake the demux in the irq chip driver, where you provide the
pseudo chip and the demux logic. None of the device drivers (PIT, PMC,
UART ...) even knows about that.

|--------|             |--------|
| AIC    |-------------| Pseudo |---- PIT
|        |             | chip   |---- PMC
|        |             |        |---- UART
|        |             |--------|
|        |--- devX
|        |--- devY
|        |--- devZ
|--------|

So PIT, PMC and UART get interrupt numbers handed out which are
outside of the interrupt space of AIC.

Thanks,

	tglx

[PATCH 0/5] ARM: at91: fix irq_pm_install_action WARNING

[Boris Brezillon]

On Tue, 16 Dec 2014 13:56:26 +0100 (CET)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Tue, 16 Dec 2014, Boris Brezillon wrote:
> > Thomas Gleixner <tglx@linutronix.de> wrote:
> > > Install the timer interrupt as a demultiplexing interrupt.
> > 
> > I can try to hack the AIC irqchip driver to implement this demux logic,
> > but this logic can't be placed in the PIT (Periodic Interval Timer)
> > driver itself, because the shared IRQ line is used by the at91 clock
> > controller (PMC) which is providing the clock to the PIT device.
> > This gives the following dependency graph:
> > 
> > PIT =depends-on=> Master Clock =provided-by=> PMC =needs=> PMC IRQ.
> 
> It never can be a part of a device driver. This is a property of the
> interrupt controller which fails to provide a proper demux for this
> irq line in the first place.
> 
> So you fake the demux in the irq chip driver, where you provide the
> pseudo chip and the demux logic. None of the device drivers (PIT, PMC,
> UART ...) even knows about that.
> 
> |--------|             |--------|
> | AIC    |-------------| Pseudo |---- PIT
> |        |             | chip   |---- PMC
> |        |             |        |---- UART
> |        |             |--------|
> |        |--- devX
> |        |--- devY
> |        |--- devZ
> |--------|
> 
> So PIT, PMC and UART get interrupt numbers handed out which are
> outside of the interrupt space of AIC.

Okay, that's what I had in mind, thanks for clarifying this aspect.

Regarding the DT representation, it should look like this, right ?

aic: interrupt-controller at fffff000 {
	#interrupt-cells = <3>;
	compatible = "atmel,at91rm9200-aic";
	interrupt-controller;
	reg = <0xfffff000 0x200>;
	atmel,external-irqs = <29 30 31>;

	irq1_demux: irq1-demux {
		#interrupt-cells = <1>;
		interrupt-controller;
	}
};

pit: timer at fffffd30 {
	compatible = "atmel,at91sam9260-pit";
	reg = <0xfffffd30 0xf>;
	/* 0 is the id reserved for the PIT */
	interrupts-extended = <&irq1_demux 0>;
	clocks = <&mck>;
};

pmc: pmc at fffffc00 {
	compatible = "atmel,at91sam9260-pmc";
	reg = <0xfffffc00 0x100>;
	/* 1 is the id reserved for the PMC */
	interrupts-extended = <&irq1_demux 1>;
[...]
};

Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 0/5] ARM: at91: fix irq_pm_install_action WARNING

[Thomas Gleixner]

On Tue, 16 Dec 2014, Boris Brezillon wrote:
> On Tue, 16 Dec 2014 13:56:26 +0100 (CET)
> Okay, that's what I had in mind, thanks for clarifying this aspect.
> 
> Regarding the DT representation, it should look like this, right ?
> 
> aic: interrupt-controller at fffff000 {
> 	#interrupt-cells = <3>;
> 	compatible = "atmel,at91rm9200-aic";
> 	interrupt-controller;
> 	reg = <0xfffff000 0x200>;
> 	atmel,external-irqs = <29 30 31>;
> 
> 	irq1_demux: irq1-demux {
> 		#interrupt-cells = <1>;
> 		interrupt-controller;
> 	}
> };
> 
> pit: timer at fffffd30 {
> 	compatible = "atmel,at91sam9260-pit";
> 	reg = <0xfffffd30 0xf>;
> 	/* 0 is the id reserved for the PIT */
> 	interrupts-extended = <&irq1_demux 0>;
> 	clocks = <&mck>;
> };
> 
> pmc: pmc at fffffc00 {
> 	compatible = "atmel,at91sam9260-pmc";
> 	reg = <0xfffffc00 0x100>;
> 	/* 1 is the id reserved for the PMC */
> 	interrupts-extended = <&irq1_demux 1>;

Not that I'm a DT expert, but that looks about right.

Thanks,

	tglx

[PATCH 0/5] ARM: at91: fix irq_pm_install_action WARNING

[Boris Brezillon]

Hi Thomas,

On Tue, 16 Dec 2014 11:03:55 +0100 (CET)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Tue, 16 Dec 2014, Boris Brezillon wrote:
> > On Mon, 15 Dec 2014 23:48:14 +0100
> > "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> > > Or even set IRFQ_NO_SUSPEND for all of the users of this interrupt and add
> > > comments to them explaining why it is set.
> > Actually I thought about adding a new flag (let's call it
> > IRQF_DONT_COMPLAIN for now ;-)) to remove those warnings (or specifying
> > IRFQ_NO_SUSPEND in all peripherals sharing the IRQ with the init
> > timer), but after discussing the problem with Thomas I decided to go
> > for the approach described in my cover letter.
> > 
> > Thomas, correct me if I'm wrong, but your concern about the
> > IRQF_DONT_COMPLAIN approach was that it was leaving interrupt handlers
> > of suspended devices in an active state (meaning that they could be
> > called in "suspend" or "early resume" state), and such devices might
> > not properly handle interrupts while being in a suspended state (clocks
> > and regulators disabled).
> > In at91 specific case this should not be an issue thought.
> > 
> > We have the same problem when setting IRFQ_NO_SUSPEND on all peripherals
> > sharing the IRQ with the init timer.
> > Moreover, I'd like to keep the core automatically disabling the IRQ when
> > the PMC, RTC, watchdog or DBGU (UART) peripherals have their own
> > dedicated IRQ (which is the case on Atmel sama5 SoCs).
> > This implies testing for the SoC version in each of these drivers and
> > adapting the request_irq call accordingly.
> 
> But still all those drivers must disable the interrupts at the device
> level on suspend, right?
> 
> > Thomas, Rafael, if both of you think I should either introduce a new
> > flag or specify IRFQ_NO_SUSPEND in all shared IRQ users, then I can go
> > for one of this solution.
> 
> All of this really sucks. What about the following?
> 
> Install the timer interrupt as a demultiplexing interrupt.
> 
> Create a pseudo interrupt chip, which essentially does nothing, but
> keeps track of the disabled state. Install handle_simple_irq as
> handler for those "demux" interrupts. Then have:
> 
> struct data {
>        u32 unmasked;
>        u32 demuxavail;
> };
> 
> static struct data demuxdata;
> 
> At init time you know how many of these demux interrupts are
> available. So you set the demuxdata up, e.g. for 3 interrupts
> connected:
> 
> 	demuxdata.demuxavail = 0x07;
> 
> You install a pointer to demuxdata for all demux interrupts as irq
> chip data and the simple handler.
> 
> And in the mask/unmask handlers you do:
> 
> mask(irqdata) {
>        struct data *d = irq_data_get_irq_chip_data(irqdata);
> 
>        d->unmasked &= ~irqdata->mask;
>        if (!d->unmasked)
>        	  mask_demux_irq();
> }
> 
> unmask(irqdata) {
>        struct data *d = irq_data_get_irq_chip_data(irqdata);
> 
>        if (!d->unmasked)
>        	  unmask_demux_irq();
>        d->mask |= irqdata->mask;
> }
> 
> Now the demuxhandler does:
> 
>     mask = demuxdata.demuxavail & demuxdata.unmasked;
> 
>     for_each_bit(bit, mask)
>     	generic_handle_irq(demuxirq_start + bit);
> 
> So the handler wont be invoked for masked bits and handle_simple_irq()
> will not call the device handler if the interrupt is marked disabled.
> 
> So in the suspend case all "demux" interrupts except those which are
> marked NOSUSPEND are marked disabled and the handlers wont be invoked.
> 
> Locking and other details omitted.
> 
> That avoids the whole flag, action, whatever business for the price of
> a really trivial demux mechanism. Everything just works. Even the irq
> storm detector will just disable the parent interrupt once all
> handlers return NONE often enough.

Still have one question regarding the spurious interrupt detection code.
AFAIU, it disables a specific IRQ handler if it triggers to often with
an IRQ_NONE return, right ?

In this dumb demuxer I can't tell which child interrupt should be
handled (there is no interrupt cause register), thus I call
generic_handle_irq on all unmasked IRQs.
Isn't there a risk to get some of my child interrupts disabled because
they always return IRQ_NONE (which is a normal use case for
IRQF_SHARED: we're only expecting at least one handler to return
IRQ_HANDLED or IRQ_WAKE_THREAD, not all of them) ?


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 0/5] ARM: at91: fix irq_pm_install_action WARNING

[Thomas Gleixner]

On Tue, 16 Dec 2014, Boris Brezillon wrote:
> On Tue, 16 Dec 2014 11:03:55 +0100 (CET)
> Thomas Gleixner <tglx@linutronix.de> wrote:
> > That avoids the whole flag, action, whatever business for the price of
> > a really trivial demux mechanism. Everything just works. Even the irq
> > storm detector will just disable the parent interrupt once all
> > handlers return NONE often enough.
> 
> Still have one question regarding the spurious interrupt detection code.
> AFAIU, it disables a specific IRQ handler if it triggers to often with
> an IRQ_NONE return, right ?
> 
> In this dumb demuxer I can't tell which child interrupt should be
> handled (there is no interrupt cause register), thus I call
> generic_handle_irq on all unmasked IRQs.
> Isn't there a risk to get some of my child interrupts disabled because
> they always return IRQ_NONE (which is a normal use case for
> IRQF_SHARED: we're only expecting at least one handler to return
> IRQ_HANDLED or IRQ_WAKE_THREAD, not all of them) ?

Good point. I missed that aspect.

Another good reason to make that core infrastructure.

So what you need is the following:

void dumb_demux_handler(....)
{
	irq_return ret = IRQ_NONE:
	int i;

	for_each_demux_irq(i)
		ret |= handle_dumb_demux_irq(base + i);

	note_interrupt(demuxirq, ret);
}   

So that disables the demux irq if all sub handlers return NONE 100k
times in a row. Otherwise if one of them handles it, you're fine.

That more or less resembles the shared action magic.

So yes, that wants to be core functionality which can be instantiated
from an affected irq chip for this particular case of HW insanity.

Thanks,

	tglx

[RFC PATCH] irqchip: add dumb demultiplexer implementation

[Boris Brezillon]

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 forward interrupts to all
peripherals (exactly what's happening with IRQ_SHARED) but keeps 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>
---
Hi Thomas,

Here is a proof of concept of the dumb demux irqchip implementation
(tested on an at91 board).
This is definitely not the final implementation, but before going further,
and sending a proper patch series, I'd like to have your opinion on
several aspects.

1) Is it close to what you had in mind ?
2) I'm not sure which part of the code should go where, so don't hesitate
   to point out misplaced sections.
3) Do I need to disable the source irq (the one feeding the irqchip) when
   entering suspend (and enable it on resume) ?
4) I'm not sure of what flags should be set/cleared when mapping an
   interrupt. Should I let the caller decide of this config (something
   similar to what is done in generic-chip) ?

Best Regards,

Boris

 drivers/irqchip/Kconfig          |   4 ++
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-dumb-demux.c |  71 ++++++++++++++++++++++
 include/linux/irq.h              |  40 ++++++++++++
 include/linux/irqdomain.h        |   1 +
 kernel/irq/Makefile              |   1 +
 kernel/irq/chip.c                |  39 ++++++++++++
 kernel/irq/dumb-demux-chip.c     | 128 +++++++++++++++++++++++++++++++++++++++
 kernel/irq/handle.c              |  31 +++++++++-
 kernel/irq/internals.h           |   3 +
 10 files changed, 317 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..e315c63 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_IRQ_DEMUX
+	bool
+	select IRQ_DOMAIN
+
 config DW_APB_ICTL
 	bool
 	select GENERIC_IRQ_CHIP
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 9516a32..6ad7441 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_IRQ_DEMUX)		+= 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..b941b8b
--- /dev/null
+++ b/drivers/irqchip/irq-dumb-demux.c
@@ -0,0 +1,71 @@
+#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;
+	struct property *prop;
+	const __be32 *propiter;
+	unsigned int irq;
+	int max_irq = -1;
+	u32 tmpval;
+	int ret;
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (!irq) {
+		pr_err("Failed to retrieve dumb irq demuxer source\n");
+		return -EINVAL;
+	}
+
+	of_property_for_each_u32(node, "irqs", prop, propiter, tmpval) {
+		if (max_irq < (int)tmpval)
+			max_irq = tmpval;
+	}
+
+	if (max_irq < 0) {
+		pr_err("Missing 'irqs' property\n");
+		return -EINVAL;
+	}
+
+	demux = irq_alloc_dumb_demux_chip(0, max_irq);
+	if (!demux) {
+		pr_err("Failed to allocate dumb irq demuxer struct\n");
+		return -ENOMEM;
+	}
+
+	of_property_for_each_u32(node, "irqs", prop, propiter, tmpval)
+		set_bit(tmpval, demux->available);
+
+	demux->domain = irq_domain_add_linear(node, max_irq + 1,
+					      &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);
+
+	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..6217284 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -445,6 +445,8 @@ 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);
+extern irqreturn_t handle_dumb_demux_irq(unsigned int irq,
+					 struct irq_desc *desc);
 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 +864,42 @@ 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,
+};
+
+/**
+ * struct irq_chip_dumb_demux - Dumb demultiplexer irq chip data structure
+ * @domain:		irq domain pointer
+ * @max_irq:		Last valid irq
+ * @available:		Bitfield of valid irqs
+ * @unmasked:		Bitfield containing irqs status
+ * @flags:		irq_dumb_demux_flags flags
+ *
+ * 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;
+	int max_irq;
+	unsigned long *available;
+	unsigned long *unmasked;
+	unsigned int flags;
+};
+
+void irq_dumb_demux_handler(unsigned int irq, struct irq_desc *desc);
+
+struct irq_chip_dumb_demux *
+irq_alloc_dumb_demux_chip(unsigned int dd_flags,
+			  unsigned int max_out_irq);
+
 #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/Makefile b/kernel/irq/Makefile
index d121235..5aad99d 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) += dumb-demux-chip.o
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 6f1c7a5..3318972 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -405,6 +405,45 @@ out_unlock:
 }
 EXPORT_SYMBOL_GPL(handle_simple_irq);
 
+/**
+ *	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);
+
 /*
  * 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..049e537
--- /dev/null
+++ b/kernel/irq/dumb-demux-chip.c
@@ -0,0 +1,128 @@
+/*
+ * 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);
+}
+
+static void irq_dumb_demux_unmask(struct irq_data *d)
+{
+	struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
+
+	set_bit(d->hwirq, demux->unmasked);
+}
+
+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 (hw_irq > demux->max_irq ||
+	    !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);
+	set_irq_flags(virq, IRQF_VALID);
+	irq_set_chip_data(virq, demux);
+
+	return 0;
+}
+
+struct irq_domain_ops irq_dumb_demux_domain_ops = {
+	.map	= irq_map_dumb_demux_chip,
+	.xlate	= irq_domain_xlate_onecell,
+};
+EXPORT_SYMBOL(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, demux->max_irq) {
+		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(irq_dumb_demux_handler);
+
+/**
+ * irq_alloc_dumb_demux_chip - Allocate a dumb demux chip
+ * @dd_flags:		irq_dumb_demux_flags flags
+ * @max_out_irq:	Last valid irq number
+ */
+struct irq_chip_dumb_demux *
+irq_alloc_dumb_demux_chip(unsigned int dd_flags,
+			  unsigned int max_out_irq)
+{
+	struct irq_chip_dumb_demux *demux;
+	int max_irq = -1;
+
+	demux = kzalloc(sizeof(*demux) +
+			(DIV_ROUND_UP(max_out_irq + 1, BITS_PER_LONG) * 2),
+			GFP_KERNEL);
+	if (!demux) {
+		pr_err("Failed to allocate dumb irq demuxer struct\n");
+		return ERR_PTR(-ENOMEM);
+	}
+
+	demux->available = (void *)demux + sizeof(*demux);
+	demux->unmasked = demux->available +
+			  DIV_ROUND_UP(max_irq + 1, BITS_PER_LONG);
+	demux->max_irq = max_out_irq;
+
+	return demux;
+}
+EXPORT_SYMBOL(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

[RFC PATCH] irqchip: add dumb demultiplexer implementation

[Thomas Gleixner]

On Thu, 8 Jan 2015, Boris Brezillon wrote:
> 1) Is it close to what you had in mind ?

Yes.

> 2) I'm not sure which part of the code should go where, so don't hesitate
>    to point out misplaced sections.

Looks sane. Nits below.

> 3) Do I need to disable the source irq (the one feeding the irqchip) when
>    entering suspend (and enable it on resume) ?

That probably needs to be part of the dumb mask/unmask functions.,
i.e. if no demux interrupt is enabled, the source irq should be
masked.

> 4) I'm not sure of what flags should be set/cleared when mapping an
>    interrupt. Should I let the caller decide of this config (something
>    similar to what is done in generic-chip) ?

I don't think you need to set/clear anything. Lets look at that dumb
demux chip as a real electronic circuit

     	     	  |----------------|
		  |                |
     	     	  |  --|Unmasked|--|---- Demux0
		  |  |             |
     SRC irq -----|-- -|Unmasked|--|---- Demux1
		  |  |             |
     	     	  |  --|Unmasked|--|---- Demux2
		  |                |
     	     	  |----------------|

Whether a demultiplexed interrupt is mapped or not is not really
important. The only relevant information is whether its masked or
not. So the default state is masked until a demultiplexed interrupt
gets requested.

> +/**
> + * 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,
> +};
> +
> +/**
> + * struct irq_chip_dumb_demux - Dumb demultiplexer irq chip data structure
> + * @domain:		irq domain pointer
> + * @max_irq:		Last valid irq
> + * @available:		Bitfield of valid irqs
> + * @unmasked:		Bitfield containing irqs status
> + * @flags:		irq_dumb_demux_flags flags
> + *
> + * 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;
> +	int max_irq;
> +	unsigned long *available;
> +	unsigned long *unmasked;

Why pointers? A single ulong is certainly enough.

> +/**
> + *	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);

This should go into the new file as well, so it gets compiled out when
not enabled.

> +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);
> +}
> +
> +static void irq_dumb_demux_unmask(struct irq_data *d)
> +{
> +	struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
> +
> +	set_bit(d->hwirq, demux->unmasked);
> +}

So this needs the handling of enabling/disabling the source 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,

+	.name		= "dumb-demux-irq",
+	.irq_mask	= irq_dumb_demux_mask,
+	.irq_unmask	= irq_dumb_demux_unmask,

Makes it simpler to read.

> +struct irq_domain_ops irq_dumb_demux_domain_ops = {
> +	.map	= irq_map_dumb_demux_chip,
> +	.xlate	= irq_domain_xlate_onecell,
> +};
> +EXPORT_SYMBOL(irq_dumb_demux_domain_ops);

SYMBOL_GPL please

Rest looks good.

Thanks,

	tglx

[RFC PATCH] irqchip: add dumb demultiplexer implementation

[Boris Brezillon]

Hi Thomas,

On Tue, 13 Jan 2015 11:38:14 +0100 (CET)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Thu, 8 Jan 2015, Boris Brezillon wrote:
> > 1) Is it close to what you had in mind ?
> 
> Yes.
> 
> > 2) I'm not sure which part of the code should go where, so don't hesitate
> >    to point out misplaced sections.
> 
> Looks sane. Nits below.
> 
> > 3) Do I need to disable the source irq (the one feeding the irqchip) when
> >    entering suspend (and enable it on resume) ?
> 
> That probably needs to be part of the dumb mask/unmask functions.,
> i.e. if no demux interrupt is enabled, the source irq should be
> masked.

Ok, I'll add that part.

> 
> > 4) I'm not sure of what flags should be set/cleared when mapping an
> >    interrupt. Should I let the caller decide of this config (something
> >    similar to what is done in generic-chip) ?
> 
> I don't think you need to set/clear anything. Lets look at that dumb
> demux chip as a real electronic circuit
> 
>      	     	  |----------------|
> 		  |                |
>      	     	  |  --|Unmasked|--|---- Demux0
> 		  |  |             |
>      SRC irq -----|-- -|Unmasked|--|---- Demux1
> 		  |  |             |
>      	     	  |  --|Unmasked|--|---- Demux2
> 		  |                |
>      	     	  |----------------|
> 
> Whether a demultiplexed interrupt is mapped or not is not really
> important. The only relevant information is whether its masked or
> not. So the default state is masked until a demultiplexed interrupt
> gets requested.

Hmm, my question was not really clear: I was talking about irq flags [1]
(those that are set with irq_modify_status in the generic irq chip [2]).

> 
> > +/**
> > + * 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,
> > +};
> > +
> > +/**
> > + * struct irq_chip_dumb_demux - Dumb demultiplexer irq chip data structure
> > + * @domain:		irq domain pointer
> > + * @max_irq:		Last valid irq
> > + * @available:		Bitfield of valid irqs
> > + * @unmasked:		Bitfield containing irqs status
> > + * @flags:		irq_dumb_demux_flags flags
> > + *
> > + * 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;
> > +	int max_irq;
> > +	unsigned long *available;
> > +	unsigned long *unmasked;
> 
> Why pointers? A single ulong is certainly enough.

Okay, just thought one might need a dumb demuxer with more than 32 (or
64) outputs, but I guess we can limit it to an unsigned long for now.

> 
> > +/**
> > + *	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);
> 
> This should go into the new file as well, so it gets compiled out when
> not enabled.

Sure.

> 
> > +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);
> > +}
> > +
> > +static void irq_dumb_demux_unmask(struct irq_data *d)
> > +{
> > +	struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
> > +
> > +	set_bit(d->hwirq, demux->unmasked);
> > +}
> 
> So this needs the handling of enabling/disabling the source irq.

Yep.

> 
> > +static struct irq_chip irq_dumb_demux_chip = {
> > +	.name = "dumb-demux-irq",
> > +	.irq_mask = irq_dumb_demux_mask,
> > +	.irq_unmask = irq_dumb_demux_unmask,
> 
> +	.name		= "dumb-demux-irq",
> +	.irq_mask	= irq_dumb_demux_mask,
> +	.irq_unmask	= irq_dumb_demux_unmask,
> 
> Makes it simpler to read.

I'll fix that

> 
> > +struct irq_domain_ops irq_dumb_demux_domain_ops = {
> > +	.map	= irq_map_dumb_demux_chip,
> > +	.xlate	= irq_domain_xlate_onecell,
> > +};
> > +EXPORT_SYMBOL(irq_dumb_demux_domain_ops);
> 
> SYMBOL_GPL please

and that too.

Thanks,

Boris


[1]http://lxr.free-electrons.com/source/kernel/irq/settings.h#L21
[2]http://lxr.free-electrons.com/source/kernel/irq/generic-chip.c#L394

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[RFC PATCH] irqchip: add dumb demultiplexer implementation

[Thomas Gleixner]

On Tue, 13 Jan 2015, Boris Brezillon wrote:
> Hmm, my question was not really clear: I was talking about irq flags [1]
> (those that are set with irq_modify_status in the generic irq chip [2]).

Ah, ok. Taking the same approach should be ok.

Thanks,

	tglx

[RFC PATCH] irqchip: add dumb demultiplexer implementation

[Boris Brezillon]

On Tue, 13 Jan 2015 11:38:14 +0100 (CET)
Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > +/**
> > + *	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);
> 
> This should go into the new file as well, so it gets compiled out when
> not enabled.

Actually, I can't move that function out of kernel/irq/chip.c because
it calls irq_may_run which is statically defined in this file.
Should I export this function or just enclose it in a
#ifdef DUMB_IRQ_DEMUX section ?


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[RFC PATCH] irqchip: add dumb demultiplexer implementation

[Thomas Gleixner]

On Tue, 13 Jan 2015, Boris Brezillon wrote:
> On Tue, 13 Jan 2015 11:38:14 +0100 (CET)
> Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > > +/**
> > > + *	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);
> > 
> > This should go into the new file as well, so it gets compiled out when
> > not enabled.
> 
> Actually, I can't move that function out of kernel/irq/chip.c because
> it calls irq_may_run which is statically defined in this file.
> Should I export this function or just enclose it in a
> #ifdef DUMB_IRQ_DEMUX section ?

Missed that. Make it conditional then.