[PATCH] genirq: irq_chip->startup() usage in setup_irq and set_irq_chained handler

[PATCH] genirq: irq_chip->startup() usage in setup_irq and set_irq_chained handler

[Pawel MOLL]

This patch clarifies a usage of irq_chip->startup() callback:

1. The "if (startup) startup(); else enabled();" code in setup_irq()
   is unnecessary, as startup() falls back to enabled() via
   default callbacks, set by irq_chip_set_defaults().

2. When using set_irq_chained_handler() the startup() was never called,
   which is not good at all... Fixed. And again - when startup() is not
   defined the call will fall back to enable() than to unmask() via
   default callbacks.

Signed-off-by: Pawel Moll <pawel.moll@st.com>
Cc: Ingo Molnar <mingo@elte.hu>
---
 kernel/irq/chip.c   |    2 +-
 kernel/irq/manage.c |    5 +----
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 7279484..a32c337 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -595,7 +595,7 @@ __set_irq_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained,
 		desc->status &= ~IRQ_DISABLED;
 		desc->status |= IRQ_NOREQUEST | IRQ_NOPROBE;
 		desc->depth = 0;
-		desc->chip->unmask(irq);
+		desc->chip->startup(irq);
 	}
 	spin_unlock_irqrestore(&desc->lock, flags);
 }
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index fdccfd5..d0af30c 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -369,10 +369,7 @@ int setup_irq(unsigned int irq, struct irqaction *new)
 		if (!(desc->status & IRQ_NOAUTOEN)) {
 			desc->depth = 0;
 			desc->status &= ~IRQ_DISABLED;
-			if (desc->chip->startup)
-				desc->chip->startup(irq);
-			else
-				desc->chip->enable(irq);
+			desc->chip->startup(irq);
 		} else
 			/* Undo nested disables: */
 			desc->depth = 1;
-- 
1.5.5.1

Re: [PATCH] genirq: irq_chip->startup() usage in setup_irq and set_irq_chained handler

[Pawel MOLL]

Hi, Ingo,

Apparently I have swapped To: and Cc: field in the original mail,
apologies.

> This patch clarifies a usage of irq_chip->startup() callback:
> 
> 1. The "if (startup) startup(); else enabled();" code in setup_irq()
>    is unnecessary, as startup() falls back to enabled() via
>    default callbacks, set by irq_chip_set_defaults().
> 
> 2. When using set_irq_chained_handler() the startup() was never called,
>    which is not good at all... Fixed. And again - when startup() is not
>    defined the call will fall back to enable() than to unmask() via
>    default callbacks.

Any comment on the patch?

Best regards

Paweł

Re: [PATCH] genirq: irq_chip->startup() usage in setup_irq and set_irq_chained handler

[Ingo Molnar]

* Pawel MOLL <pawel.moll@st.com> wrote:

> Hi, Ingo,
> 
> Apparently I have swapped To: and Cc: field in the original mail,
> apologies.
> 
> > This patch clarifies a usage of irq_chip->startup() callback:
> > 
> > 1. The "if (startup) startup(); else enabled();" code in setup_irq()
> >    is unnecessary, as startup() falls back to enabled() via
> >    default callbacks, set by irq_chip_set_defaults().
> > 
> > 2. When using set_irq_chained_handler() the startup() was never called,
> >    which is not good at all... Fixed. And again - when startup() is not
> >    defined the call will fall back to enable() than to unmask() via
> >    default callbacks.
> 
> Any comment on the patch?

looks good to me at first glance - but i Cc:-ed Thomas and Ben, maybe 
they have further comments.

	Ingo

Re: [PATCH] genirq: irq_chip->startup() usage in setup_irq and set_irq_chained handler

[Benjamin Herrenschmidt]

On Sat, 2008-08-23 at 18:08 +0200, Ingo Molnar wrote:
> > Apparently I have swapped To: and Cc: field in the original mail,
> > apologies.
> > 
> > > This patch clarifies a usage of irq_chip->startup() callback:
> > > 
> > > 1. The "if (startup) startup(); else enabled();" code in setup_irq()
> > >    is unnecessary, as startup() falls back to enabled() via
> > >    default callbacks, set by irq_chip_set_defaults().
> > > 
> > > 2. When using set_irq_chained_handler() the startup() was never called,
> > >    which is not good at all... Fixed. And again - when startup() is not
> > >    defined the call will fall back to enable() than to unmask() via
> > >    default callbacks.
> > 
> > Any comment on the patch?
> 
> looks good to me at first glance - but i Cc:-ed Thomas and Ben, maybe 
> they have further comments.

The second change is a significant semantic change. I wouldn't be
surprised if I have cases that rely (or work around) the lack of
startup() in set_irq_chained_handler(). I'll have to dbl check things
next week. Can you send me a copy of those patches ?

Cheers,
Ben.

Re: [PATCH] genirq: irq_chip->startup() usage in setup_irq and set_irq_chained handler

[Pawel MOLL]

> The second change is a significant semantic change. I wouldn't be
> surprised if I have cases that rely (or work around) the lack of
> startup() in set_irq_chained_handler(). I'll have to dbl check things
> next week. 

Let me briefly explain my situation. I have a main interrupt controller
which provides startup() and unmask/mask() functions. The first one is
rather expensive (as the controller itself is... hmmm...
complicated ;-), the second - very cheap. And that is how I understand
the different "levels" of interrupt access - startup() should be called
once, somewhere during request_irq(), (un)masking may be used
frequently.

And one of the interrupt is generated by hardware PIO controller. The
idea was obvious - register a chained handler, which decodes the PIO
controller state and generates a interrupt, which number may be obtained
by gpio_to_irq(). Sounds simple, doesn't it? :-)

And in that moment the problem raised its ugly head - the interrupt
controller's startup() was never called for the PIO interrupt (as there
was no request_irq()), so the hardware wasn't configured properly and...
well... bad things were happening ;-)

So unless I totally misunderstood the meaning of irq_chip callbacks, I
believe the startup() should be called in set_irq_chained_handler(). 

Regards

Paweł

Re: [PATCH] genirq: irq_chip->startup() usage in setup_irq and set_irq_chained handler

[Benjamin Herrenschmidt]

On Tue, 2008-08-26 at 11:14 +0100, Pawel MOLL wrote:

> Let me briefly explain my situation. I have a main interrupt controller
> which provides startup() and unmask/mask() functions. The first one is
> rather expensive (as the controller itself is... hmmm...
> complicated ;-), the second - very cheap. And that is how I understand
> the different "levels" of interrupt access - startup() should be called
> once, somewhere during request_irq(), (un)masking may be used
> frequently.

Oh, I don't disagree. It's probably a good idea. I'm just worried of
the potential impact on existing code written around the current
behaviour.

We have 23 calls to set_irq_chained_handler in arch/powerpc, and I need
to audit them all. Luckily, we mostly don't have startup() callbacks.

 (... some time later ...)

It looks good. Of course, we'll have to test at one point, but at this
stage, I think powerpc is happy with the change.

Interestingly enough, I can see a case where we would have a problem
-without- your change :-) Not with the current code, but in conjunction
with another change that's planned for .28.

So Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Cheers,
Ben.

Re: [PATCH] genirq: irq_chip->startup() usage in setup_irq and set_irq_chained handler

[Pawel MOLL]

> So Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Great, thanks ;-)

Ingo, I will send you an acked patch.

Cheers

Paweł

Re: [PATCH] genirq: irq_chip->startup() usage in setup_irq and set_irq_chained handler

[Ingo Molnar]

* Pawel MOLL <pawel.moll@st.com> wrote:

> This patch clarifies usage of irq_chip->startup() callback:
> 
> 1. The "if (startup) startup(); else enabled();" code in setup_irq()
>    is unnecessary, as startup() falls back to enabled() via
>    default callbacks, set by irq_chip_set_defaults().
> 
> 2. When using set_irq_chained_handler() the startup() was never called,
>    which is not good at all... Fixed. And again - when startup() is not
>    defined the call will fall back to enable() than to unmask() via
>    default callbacks.
> 
> Signed-off-by: Pawel Moll <pawel.moll@st.com>
> Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

applied the commit below to tip/genirq - thanks Pawel.

	Ingo

------------->
>From 7e6e178ab1548c8d894a77593e757acf4510b8ba Mon Sep 17 00:00:00 2001
From: Pawel MOLL <pawel.moll@st.com>
Date: Mon, 1 Sep 2008 10:12:11 +0100
Subject: [PATCH] genirq: irq_chip->startup() usage in setup_irq and set_irq_chained handler

This patch clarifies usage of irq_chip->startup() callback:

1. The "if (startup) startup(); else enabled();" code in setup_irq()
   is unnecessary, as startup() falls back to enabled() via
   default callbacks, set by irq_chip_set_defaults().

2. When using set_irq_chained_handler() the startup() was never called,
   which is not good at all... Fixed. And again - when startup() is not
   defined the call will fall back to enable() than to unmask() via
   default callbacks.

Signed-off-by: Pawel Moll <pawel.moll@st.com>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/irq/chip.c   |    2 +-
 kernel/irq/manage.c |    5 +----
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 964964b..240c64d 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -587,7 +587,7 @@ __set_irq_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained,
 		desc->status &= ~IRQ_DISABLED;
 		desc->status |= IRQ_NOREQUEST | IRQ_NOPROBE;
 		desc->depth = 0;
-		desc->chip->unmask(irq);
+		desc->chip->startup(irq);
 	}
 	spin_unlock_irqrestore(&desc->lock, flags);
 }
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index ae1b684..9aa3e7b 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -397,10 +397,7 @@ int setup_irq(unsigned int irq, struct irqaction *new)
 		if (!(desc->status & IRQ_NOAUTOEN)) {
 			desc->depth = 0;
 			desc->status &= ~IRQ_DISABLED;
-			if (desc->chip->startup)
-				desc->chip->startup(irq);
-			else
-				desc->chip->enable(irq);
+			desc->chip->startup(irq);
 		} else
 			/* Undo nested disables: */
 			desc->depth = 1;