Using set_irq_handler in set_irq_type callback?

Using set_irq_handler in set_irq_type callback?

[Uwe Kleine-König]

Hallo,

I'm seeing an imx31 (ARCH=arm) based system failing to boot .31.2-rt13.

The reason is that the irq for the ethernet device is level triggered,
but the handler for that interrupt is handle_edge_irq.
As the handler is threaded with PREEMPT_RT=y, the irq is only acked but
neither masked (as it is believed to be an edge irq) nor handled.  This
stucks the machine as the irq is still active.

I think the fix for that is adding something like:

	if (type & IRQ_TYPE_EDGE_BOTH)
		set_irq_handler(irq, handle_edge_irq);
	else
		set_irq_handler(irq, handle_level_irq);

to the chip's .set_type callback.  (Doing this directly fails, as
.set_type holds desc->lock which set_irq_handler acquires, too.  So
maybe I need to code up a handler that checks how a given irq triggers
and then calls handle_edge_irq or handle_level_irq.)

The thing that let's me question that change is that I only found[1] a
single .set_type callback that does something like that (i.e.
arch/blackfin/mach-common/ints-priority.c).

Do I just miss something or are there more than one platforms that are
in the need of handling this issue, too?

Best regards
Uwe

[1] checking files found by 

	git grep -c set_irq_handler | awk -F: '$2 >= 2'

-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

Re: Using set_irq_handler in set_irq_type callback?

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 553 bytes --]

On Wed, 7 Oct 2009, Uwe Kleine-König wrote:
> I think the fix for that is adding something like:
> 
> 	if (type & IRQ_TYPE_EDGE_BOTH)
> 		set_irq_handler(irq, handle_edge_irq);
> 	else
> 		set_irq_handler(irq, handle_level_irq);
> 
> to the chip's .set_type callback.  (Doing this directly fails, as
> .set_type holds desc->lock which set_irq_handler acquires, too.  So
> maybe I need to code up a handler that checks how a given irq triggers
> and then calls handle_edge_irq or handle_level_irq.)

Nope. See __set_irq_handler_unlocked()

Thanks,

	tglx

Re: Using set_irq_handler in set_irq_type callback?

[Russell King]

On Wed, Oct 07, 2009 at 12:07:56PM +0200, Uwe Kleine-König wrote:
> Hallo,
> 
> I'm seeing an imx31 (ARCH=arm) based system failing to boot .31.2-rt13.
> 
> The reason is that the irq for the ethernet device is level triggered,
> but the handler for that interrupt is handle_edge_irq.

There's nothing wrong with that with the conventional interrupt handling
scheme.

> As the handler is threaded with PREEMPT_RT=y, the irq is only acked but
> neither masked (as it is believed to be an edge irq) nor handled.  This
> stucks the machine as the irq is still active.

Actually, the problem is that I misnamed the edge/level handlers.
What follows here is the non-RT case.

Really, what the two are about is that the level handler is there for
interrupt controllers which behave in a good way - in other words, for
level-based inputs and for edge-based inputs which "remember" new
transitions while the input is masked.

The edge handler is for edge-based inputs where the controller does not
remember transitions with the input masked.

Forcing people to use the edge handler all the time for edge based inputs
is highly sub-optimal - it means greater interrupt load since the input
is left unmasked.

So really, the choice of the handler should be determined by how the
hardware behaves, and not by the input type requested.


When it comes to RT and its thread-based interrupt model, the assumptions
which these handlers were designed around are no longer true.  What is
now required is a different handling philosophy - rather than leaving
the interrupt-time decision about what to do with a signalled interrupt
to the flow handler, it should be immediately ack'ed and disabled, and
the interrupt thread scheduled.

It is then up to the interrupt thread to determine how to handle the
interrupt - if it's really a level interrupt, then the interrupt thread
has to call the handlers before re-enabling the input.  If it's edge
based, the input has to be re-enabled before running the handlers (so
that new edges received during the running of those handlers are
recognised.)


So, the technical aspects of handling of interrupts between the RT and
non-RT cases are quite different, and I feel that we shouldn't be
re-using the same flow handlers between the two cases.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: Using set_irq_handler in set_irq_type callback?

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1362 bytes --]

On Wed, 7 Oct 2009, Russell King wrote:
> On Wed, Oct 07, 2009 at 12:07:56PM +0200, Uwe Kleine-König wrote:
> When it comes to RT and its thread-based interrupt model, the assumptions
> which these handlers were designed around are no longer true.  What is
> now required is a different handling philosophy - rather than leaving
> the interrupt-time decision about what to do with a signalled interrupt
> to the flow handler, it should be immediately ack'ed and disabled, and
> the interrupt thread scheduled.

That's what the code does at least for the level handler. When the
thread has run then the irq line is reenabled.
 
> It is then up to the interrupt thread to determine how to handle the
> interrupt - if it's really a level interrupt, then the interrupt thread
> has to call the handlers before re-enabling the input.  If it's edge
> based, the input has to be re-enabled before running the handlers (so
> that new edges received during the running of those handlers are
> recognised.)

That's exaclty how the RT code works :) 
 
> So, the technical aspects of handling of interrupts between the RT and
> non-RT cases are quite different, and I feel that we shouldn't be
> re-using the same flow handlers between the two cases.

Why not. It works perfectly fine except for the case where a level
type interrupt uses the edge handler :)

Thanks,

	tglx