2011-03-18 08:13:13

by Esben Haabendal

[permalink] [raw]
Subject: [PATCH] Support IRQ_NOAUTOEN flag in set_irq_chained_handler()

From: Esben Haabendal <[email protected]>

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 <[email protected]>
---
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


2011-03-18 20:39:53

by Thomas Gleixner

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

On Fri, 18 Mar 2011, [email protected] wrote:

> From: Esben Haabendal <[email protected]>
>
> 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

2011-03-22 09:05:21

by Esben Haabendal

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

Thomas Gleixner <[email protected]> writes:

> On Fri, 18 Mar 2011, [email protected] wrote:
>
>> From: Esben Haabendal <[email protected]>
>>
>> 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

2011-03-22 11:17:25

by Thomas Gleixner

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

On Tue, 22 Mar 2011, Esben Haabendal wrote:

> Thomas Gleixner <[email protected]> writes:
>
> > On Fri, 18 Mar 2011, [email protected] wrote:
> >
> >> From: Esben Haabendal <[email protected]>
> >>
> >> 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

2011-03-22 12:38:12

by Esben Haabendal

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

Thomas Gleixner <[email protected]> writes:

> On Tue, 22 Mar 2011, Esben Haabendal wrote:
>
>> Thomas Gleixner <[email protected]> writes:
>>
>> > On Fri, 18 Mar 2011, [email protected] wrote:
>> >
>> >> From: Esben Haabendal <[email protected]>
>> >>
>> >> 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