[PATCH 1/2] genirq: Introduce parent and children concept for IRQ

[PATCH 1/2] genirq: Introduce parent and children concept for IRQ

[Ning Jiang]

For chained and nested irq, there exists parent and children relation
in nature. Especially for nested irq, its handler function is executed
in the parent thread context.  If we need to resend a nested interrupt,
we have to trace all the way back to its ancestor and trigger ancestor's
irq flow handler.  If we retrigger the nested interrupt directly, we'll
get warning message in irq_nested_primary_handler(). Introduce parent
and children concept so that we can get the parent irq number in such
condition.

Signed-off-by: Ning Jiang <ning.n.jiang@gmail.com>
---
 include/linux/irq.h  |   14 ++++++++++++++
 kernel/irq/chip.c    |   16 ++++++++++++++++
 kernel/irq/irqdesc.c |    2 ++
 3 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 45593d0..e043d1a 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -146,6 +146,8 @@ struct irq_domain;
 struct irq_data {
 	unsigned int		irq;
 	unsigned long		hwirq;
+	unsigned int		parent_irq;
+	unsigned int		has_children;
 	unsigned int		node;
 	unsigned int		state_use_accessors;
 	struct irq_chip		*chip;
@@ -523,6 +525,7 @@ static inline void dynamic_irq_init(unsigned int irq)
 }
 
 /* Set/get chip/data for an IRQ: */
+extern int irq_set_parent_irq(unsigned int irq, unsigned int parent_irq);
 extern int irq_set_chip(unsigned int irq, struct irq_chip *chip);
 extern int irq_set_handler_data(unsigned int irq, void *data);
 extern int irq_set_chip_data(unsigned int irq, void *data);
@@ -530,6 +533,17 @@ extern int irq_set_irq_type(unsigned int irq, unsigned int type);
 extern int irq_set_msi_desc(unsigned int irq, struct msi_desc *entry);
 extern struct irq_data *irq_get_irq_data(unsigned int irq);
 
+static inline unsigned int irq_get_parent_irq(unsigned int irq)
+{
+	struct irq_data *d = irq_get_irq_data(irq);
+	return d ? d->parent_irq : NO_IRQ;
+}
+
+static inline unsigned int irq_data_get_parent_irq(struct irq_data *d)
+{
+	return d->parent_irq;
+}
+
 static inline struct irq_chip *irq_get_chip(unsigned int irq)
 {
 	struct irq_data *d = irq_get_irq_data(irq);
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 6cec1a2..a382fef 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -20,6 +20,22 @@
 
 #include "internals.h"
 
+int irq_set_parent_irq(unsigned int irq, unsigned int parent_irq)
+{
+	unsigned long flags, flags_parent;
+	struct irq_desc *desc = irq_get_desc_lock(irq, &flags, 0);
+	struct irq_desc *desc_parent = irq_get_desc_lock(parent_irq, &flags_parent, 0);
+
+	if (!desc || !desc_parent || (parent_irq == NO_IRQ))
+		return -EINVAL;
+	desc->irq_data.parent_irq = parent_irq;
+	desc_parent->irq_data.has_children = true;
+	irq_put_desc_unlock(desc_parent, flags_parent);
+	irq_put_desc_unlock(desc, flags);
+	return 0;
+}
+EXPORT_SYMBOL(irq_set_parent_irq);
+
 /**
  *	irq_set_chip - set the irq chip for an irq
  *	@irq:	irq number
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 192a302..bf60ba9 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -76,6 +76,8 @@ static void desc_set_defaults(unsigned int irq, struct irq_desc *desc, int node,
 	int cpu;
 
 	desc->irq_data.irq = irq;
+	desc->irq_data.parent_irq = NO_IRQ;
+	desc->irq_data.has_children = false;
 	desc->irq_data.chip = &no_irq_chip;
 	desc->irq_data.chip_data = NULL;
 	desc->irq_data.handler_data = NULL;
-- 
1.7.1

[PATCH 2/2] genirq: Resend nested irq's ancestor irq

[Ning Jiang]

We'll set IRQS_PENDING for handle_nested_irq if it's disabled. When
it's re-enabled later on, check_irq_resend() will detect this flag
and trigger the software resend mechanism. resend_irqs() will call
desc->handle_irq() directly to process this interrupt, hence the
irq_nested_primary_handler() will be called for the nested irq which
gives us a warning.

If we need to resend a nested interrupt, we have to trace all the
way back to its ancestor and trigger ancestor's irq flow handler.

Signed-off-by: Ning Jiang <ning.n.jiang@gmail.com>
---
 kernel/irq/resend.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/kernel/irq/resend.c b/kernel/irq/resend.c
index 6454db7..dcfc1b5 100644
--- a/kernel/irq/resend.c
+++ b/kernel/irq/resend.c
@@ -74,6 +74,11 @@ void check_irq_resend(struct irq_desc *desc, unsigned int irq)
 		if (!desc->irq_data.chip->irq_retrigger ||
 		    !desc->irq_data.chip->irq_retrigger(&desc->irq_data)) {
 #ifdef CONFIG_HARDIRQS_SW_RESEND
+			while (irq_settings_is_nested_thread(desc) &&
+				(desc->irq_data.parent_irq != NO_IRQ)) {
+				irq = desc->irq_data.parent_irq;
+				desc = irq_to_desc(irq);
+			}
 			/* Set it pending and activate the softirq: */
 			set_bit(irq, irqs_resend);
 			tasklet_schedule(&resend_tasklet);
-- 
1.7.1

Re: [PATCH 2/2] genirq: Resend nested irq's ancestor irq

[Thomas Gleixner]

On Thu, 14 Jun 2012, Ning Jiang wrote:
> We'll set IRQS_PENDING for handle_nested_irq if it's disabled. When
> it's re-enabled later on, check_irq_resend() will detect this flag
> and trigger the software resend mechanism. resend_irqs() will call
> desc->handle_irq() directly to process this interrupt, hence the
> irq_nested_primary_handler() will be called for the nested irq which
> gives us a warning.
> 
> If we need to resend a nested interrupt, we have to trace all the
> way back to its ancestor and trigger ancestor's irq flow handler.

And what makes you believe that the ancestors demux handler will find
the irq bit of the nested interrupt still pending? There is NO
guarantee for that.

The correct solution for this is to replace the tasklet with a kernel
thread and check whether the interrupt is marked nested or not and
then invoke the correct function.

That requires a check for irq in progress in the nested handler as
well, but that's trivial to add.

Thanks,

	tglx

Re: [PATCH 2/2] genirq: Resend nested irq's ancestor irq

[Ning Jiang]

2012/6/14 Thomas Gleixner <tglx@linutronix.de>:
> On Thu, 14 Jun 2012, Ning Jiang wrote:
>> We'll set IRQS_PENDING for handle_nested_irq if it's disabled. When
>> it's re-enabled later on, check_irq_resend() will detect this flag
>> and trigger the software resend mechanism. resend_irqs() will call
>> desc->handle_irq() directly to process this interrupt, hence the
>> irq_nested_primary_handler() will be called for the nested irq which
>> gives us a warning.
>>
>> If we need to resend a nested interrupt, we have to trace all the
>> way back to its ancestor and trigger ancestor's irq flow handler.
>
> And what makes you believe that the ancestors demux handler will find
> the irq bit of the nested interrupt still pending? There is NO
> guarantee for that.

You are right.

> The correct solution for this is to replace the tasklet with a kernel
> thread and check whether the interrupt is marked nested or not and
> then invoke the correct function.
>

Do you want something like this? This just gives a rough idea. Please
help to review.

>From 1ce5f392446a06631d190468b314b7060f5c34ae Mon Sep 17 00:00:00 2001
From: Ning Jiang <ning.n.jiang@gmail.com>
Date: Fri, 15 Jun 2012 09:13:24 +0800
Subject: [PATCH] genirq: Create temporary kernel thread when resending
nested irq

---
 kernel/irq/chip.c      |    1 +
 kernel/irq/handle.c    |    2 +-
 kernel/irq/internals.h |    4 ++++
 kernel/irq/manage.c    |    8 +++++++-
 kernel/irq/resend.c    |   20 +++++++++++++++++---
 5 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 6cec1a2..ec48d4e 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -272,6 +272,7 @@ void handle_nested_irq(unsigned int irq)

 	raw_spin_lock_irq(&desc->lock);

+	desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
 	kstat_incr_irqs_this_cpu(irq, desc);

 	action = desc->action;
diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index bdb1803..556b469 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -51,7 +51,7 @@ static void warn_no_thread(unsigned int irq, struct
irqaction *action)
 	       "but no thread function available.", irq, action->name);
 }

-static void irq_wake_thread(struct irq_desc *desc, struct irqaction *action)
+void irq_wake_thread(struct irq_desc *desc, struct irqaction *action)
 {
 	/*
 	 * In case the thread crashed and was killed we just pretend that
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index 001fa5b..fd9dabe 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -104,6 +104,10 @@ extern void irq_set_thread_affinity(struct irq_desc *desc);
 extern int irq_do_set_affinity(struct irq_data *data,
 			       const struct cpumask *dest, bool force);

+extern int irq_thread(void *data);
+
+extern void irq_wake_thread(struct irq_desc *desc, struct irqaction *action);
+
 /* Inline functions for support of irq chips on slow busses */
 static inline void chip_bus_lock(struct irq_desc *desc)
 {
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 8c54823..2bd4a39 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -811,7 +811,7 @@ static void irq_thread_dtor(struct task_work *unused)
 /*
  * Interrupt handler thread
  */
-static int irq_thread(void *data)
+int irq_thread(void *data)
 {
 	struct task_work on_exit_work;
 	static const struct sched_param param = {
@@ -843,6 +843,12 @@ static int irq_thread(void *data)
 			note_interrupt(action->irq, desc, action_ret);

 		wake_threads_waitq(desc);
+
+		if (irq_settings_is_nested_thread(desc) && desc->action->thread) {
+			put_task_struct(desc->action->thread);
+			desc->action->thread = NULL;
+			do_exit(0);
+		}
 	}

 	/*
diff --git a/kernel/irq/resend.c b/kernel/irq/resend.c
index 6454db7..5bb9227 100644
--- a/kernel/irq/resend.c
+++ b/kernel/irq/resend.c
@@ -14,6 +14,7 @@
  */

 #include <linux/irq.h>
+#include <linux/kthread.h>
 #include <linux/module.h>
 #include <linux/random.h>
 #include <linux/interrupt.h>
@@ -74,9 +75,22 @@ void check_irq_resend(struct irq_desc *desc,
unsigned int irq)
 		if (!desc->irq_data.chip->irq_retrigger ||
 		    !desc->irq_data.chip->irq_retrigger(&desc->irq_data)) {
 #ifdef CONFIG_HARDIRQS_SW_RESEND
-			/* Set it pending and activate the softirq: */
-			set_bit(irq, irqs_resend);
-			tasklet_schedule(&resend_tasklet);
+			struct task_struct *t;
+			if (irq_settings_is_nested_thread(desc)) {
+				raw_spin_unlock(&desc->lock);
+				t = kthread_create(irq_thread, desc->action,
+					"irq/%d-%s", irq, desc->action->name);
+				raw_spin_lock(&desc->lock);
+				if (IS_ERR(t))
+					return;
+				get_task_struct(t);
+				desc->action->thread = t;
+				irq_wake_thread(desc, desc->action);
+			} else {
+				/* Set it pending and activate the softirq: */
+				set_bit(irq, irqs_resend);
+				tasklet_schedule(&resend_tasklet);
+			}
 #endif
 		}
 	}
-- 
1.7.1


> That requires a check for irq in progress in the nested handler as
> well, but that's trivial to add.

Will check it later.

> Thanks,
>
>        tglx

Re: [PATCH 2/2] genirq: Resend nested irq's ancestor irq

[Thomas Gleixner]

On Fri, 15 Jun 2012, Ning Jiang wrote:
> 2012/6/14 Thomas Gleixner <tglx@linutronix.de>:
> > The correct solution for this is to replace the tasklet with a kernel
> > thread and check whether the interrupt is marked nested or not and
> > then invoke the correct function.
> >
> 
> Do you want something like this?

Definitely not.

> This just gives a rough idea. Please help to review.

It's a very bad idea.

>  #ifdef CONFIG_HARDIRQS_SW_RESEND
> -			/* Set it pending and activate the softirq: */
> -			set_bit(irq, irqs_resend);
> -			tasklet_schedule(&resend_tasklet);
> +			struct task_struct *t;
> +			if (irq_settings_is_nested_thread(desc)) {
> +				raw_spin_unlock(&desc->lock);
> +				t = kthread_create(irq_thread, desc->action,
> +					"irq/%d-%s", irq, desc->action->name);

Did you even try to run that code?

You CANNOT call kthread_create() with interrupts disabled and you
CANNOT expect that the state of the irq desc is unchanged when you
drop the lock.

And even if this would work it would be fricking insane to create a
kernel thread every time we resend an interrupt. Not to talk about the
exit madness you decided to stick into the existing thread function.

Either we have a kthread set up explicitely for that and just have to
wake it up or if we can deal with the possible latency just hand it
off to workqueue context.

Thanks,

	tglx