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



--

[ message continues ]

Hi, Ingo,

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

Any comment on the patch?

Best regards

Paweł

--

[ message continues ]

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

	Ingo
--

[ message continues ]

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.


--

[ message continues ]

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ł



--

[ message continues ]

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.




--

[ message continues ]

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

Great, thanks ;-)

Ingo, I will send you an acked patch.

Cheers

Paweł

--

[ message continues ]