[PATCH] Support IRQ_NOAUTOEN flag in set_irq_chained_handler()

[PATCH] Support IRQ_NOAUTOEN flag in set_irq_chained_handler()

[eha]

From: Esben Haabendal <eha@doredevelopment.dk>

Handle IRQ_NOAUTOEN in __set_irq_handler() (ie. for
set_irq_chained_handler()) instead of just silently ignoring it, and in
the same way as is done in __setup_irq() (ie. request_irq()).

This give a more consistent interface, and also adheres better to
the rule of least surprise.

Signed-off-by: Esben Haabendal <eha@doredevelopment.dk>
---
 kernel/irq/chip.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index baa5c4a..5594b17 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -731,10 +731,13 @@ __set_irq_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained,
 	desc->name = name;
 
 	if (handle != handle_bad_irq && is_chained) {
-		desc->status &= ~IRQ_DISABLED;
 		desc->status |= IRQ_NOREQUEST | IRQ_NOPROBE;
-		desc->depth = 0;
-		desc->irq_data.chip->irq_startup(&desc->irq_data);
+		if (!(desc->status & IRQ_NOAUTOEN)) {
+			desc->depth = 0;
+			desc->status &= ~IRQ_DISABLED;
+			desc->irq_data.chip->irq_startup(&desc->irq_data);
+		} else
+			desc->depth = 1;
 	}
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
 	chip_bus_sync_unlock(desc);
-- 
1.7.1

Re: [PATCH] Support IRQ_NOAUTOEN flag in set_irq_chained_handler()

[Thomas Gleixner]

On Fri, 18 Mar 2011, eha@doredevelopment.dk wrote:

> From: Esben Haabendal <eha@doredevelopment.dk>
> 
> Handle IRQ_NOAUTOEN in __set_irq_handler() (ie. for
> set_irq_chained_handler()) instead of just silently ignoring it, and in
> the same way as is done in __setup_irq() (ie. request_irq()).
> 
> This give a more consistent interface, and also adheres better to
> the rule of least surprise.

Well, that might be less surprising for you, but you will be surprised
that such a change would be a real big surprise for all users of
chained handlers in arch/arm. They simply would not work anymore.

So we _cannot_ change the semantics here. All we can do is document
it.

Thanks,

	tglx

Re: [PATCH] Support IRQ_NOAUTOEN flag in set_irq_chained_handler()

[Esben Haabendal]

Thomas Gleixner <tglx@linutronix.de> writes:

> On Fri, 18 Mar 2011, eha@doredevelopment.dk wrote:
>
>> From: Esben Haabendal <eha@doredevelopment.dk>
>> 
>> Handle IRQ_NOAUTOEN in __set_irq_handler() (ie. for
>> set_irq_chained_handler()) instead of just silently ignoring it, and in
>> the same way as is done in __setup_irq() (ie. request_irq()).
>> 
>> This give a more consistent interface, and also adheres better to
>> the rule of least surprise.
>
> Well, that might be less surprising for you, but you will be surprised
> that such a change would be a real big surprise for all users of
> chained handlers in arch/arm. They simply would not work anymore.

How is that?  I don't see any use of IRQ_NOAUTOEN flag in arch/arm at
all.  Is there some other way that IRQ_NOAUTOEN get's enabled in
arch/arm?  Or is my patch broken in some way that it does change irq
handler setup when IRQ_NOAUTOEN is not set?

The idea of the patch is that it will do exactly the same as
before, unless you specifically set IRQ_NOAUTOEN before calling
set_irq_chained_handler...

> So we _cannot_ change the semantics here. All we can do is document
> it.

With the current semantics, how are one supposed to be able use
set_irq_chained_handler without having the handler enabled immediately?

/Esben

Re: [PATCH] Support IRQ_NOAUTOEN flag in set_irq_chained_handler()

[Thomas Gleixner]

On Tue, 22 Mar 2011, Esben Haabendal wrote:

> Thomas Gleixner <tglx@linutronix.de> writes:
> 
> > On Fri, 18 Mar 2011, eha@doredevelopment.dk wrote:
> >
> >> From: Esben Haabendal <eha@doredevelopment.dk>
> >> 
> >> Handle IRQ_NOAUTOEN in __set_irq_handler() (ie. for
> >> set_irq_chained_handler()) instead of just silently ignoring it, and in
> >> the same way as is done in __setup_irq() (ie. request_irq()).
> >> 
> >> This give a more consistent interface, and also adheres better to
> >> the rule of least surprise.
> >
> > Well, that might be less surprising for you, but you will be surprised
> > that such a change would be a real big surprise for all users of
> > chained handlers in arch/arm. They simply would not work anymore.
> 
> How is that?  I don't see any use of IRQ_NOAUTOEN flag in arch/arm at
> all.  Is there some other way that IRQ_NOAUTOEN get's enabled in
> arch/arm?  Or is my patch broken in some way that it does change irq
> handler setup when IRQ_NOAUTOEN is not set?

Ooops, sorry. I had it somewhere in the back of my memory that ARM
marked all interrupts IRQ_NOAUTOEN by default. Confused that with
NOPROBE.
 
> The idea of the patch is that it will do exactly the same as
> before, unless you specifically set IRQ_NOAUTOEN before calling
> set_irq_chained_handler...

I understand the patch :)
 
> > So we _cannot_ change the semantics here. All we can do is document
> > it.
> 
> With the current semantics, how are one supposed to be able use
> set_irq_chained_handler without having the handler enabled immediately?

Not at all. Why do you want to do that ?

Thanks,

	tglx

Re: [PATCH] Support IRQ_NOAUTOEN flag in set_irq_chained_handler()

[Esben Haabendal]

Thomas Gleixner <tglx@linutronix.de> writes:

> On Tue, 22 Mar 2011, Esben Haabendal wrote:
>
>> Thomas Gleixner <tglx@linutronix.de> writes:
>> 
>> > On Fri, 18 Mar 2011, eha@doredevelopment.dk wrote:
>> >
>> >> From: Esben Haabendal <eha@doredevelopment.dk>
>> >> 
>> >> Handle IRQ_NOAUTOEN in __set_irq_handler() (ie. for
>> >> set_irq_chained_handler()) instead of just silently ignoring it, and in
>> >> the same way as is done in __setup_irq() (ie. request_irq()).
>> >> 
>> >> This give a more consistent interface, and also adheres better to
>> >> the rule of least surprise.
>> >
>> > Well, that might be less surprising for you, but you will be surprised
>> > that such a change would be a real big surprise for all users of
>> > chained handlers in arch/arm. They simply would not work anymore.
>> 
>> How is that?  I don't see any use of IRQ_NOAUTOEN flag in arch/arm at
>> all.  Is there some other way that IRQ_NOAUTOEN get's enabled in
>> arch/arm?  Or is my patch broken in some way that it does change irq
>> handler setup when IRQ_NOAUTOEN is not set?
>
> Ooops, sorry. I had it somewhere in the back of my memory that ARM
> marked all interrupts IRQ_NOAUTOEN by default. Confused that with
> NOPROBE.
>  
>> The idea of the patch is that it will do exactly the same as
>> before, unless you specifically set IRQ_NOAUTOEN before calling
>> set_irq_chained_handler...
>
> I understand the patch :)
>  
>> > So we _cannot_ change the semantics here. All we can do is document
>> > it.
>> 
>> With the current semantics, how are one supposed to be able use
>> set_irq_chained_handler without having the handler enabled immediately?
>
> Not at all. Why do you want to do that ?

I have a system where 

I setup the chained interrupt handler (together with a lot of other
stuff related to the CPLD firmware the interrupt controller lives in) in
of_platform_driver.probe().  The CPLD may be (re)programmed from
user-space, so all driver functionality is disabled until user-space
either programs the CPLD or gives a signal that this will not happen.

I thought it would be the cleanest solution to keep driver
initialization in the probe() function, and just enable it later on.

And I cannot just set the mask early, as I am not guaranteed how the irq
line is behaving and if there actually is a mask register before it is
programmed.

/Esben