Re: [PATCH 4/5] Centralise NO_IRQ definition

[David Howells <dhowells@redhat.com>]

Linus Torvalds <torvalds@osdl.org> wrote:

> The fact is, 0 _is_ "no interrupt". Always has been.

The fact that it always has been doesn't make it correct; and the fact that
the C particularly likes zeros is only optimisable on some hardware. On RISC
processors where you can't test memory directly, it seems to be the case that
any small positive or negative integer is as good as zero; for instance, in
FRV:

	ldi	@(gr8,#0),gr4		# doesn't set the condition codes
	subicc	gr4,#-1,gr0,icc0	# cmp to -1, result to immutable gr0
	beq	icc0,#0,it_was_unset

I suspect a lot of RISC archs will be like this. It's only certain ones like
M68K and x86 where you can test memory directly:

	testl	(%eax)
	je	it_was_unset

that it's usefully optimisable, and in those cases it might be possible to do
this sort of thing:

	cmpl	$-1,(%eax)
	je	it_was_unset

though the instruction will be longer.

> In short: NO_IRQ _is_ 0. Always has been.

So what? That hasn't stopped you imposing a blanket change before.

> It's the only sane value.

Has anyone ever accused you of being sane? :-)

> Anybody who does anything else is a bug waiting to happen.

My three main concerns are this:

 (1) Changing the no-irq value away from zero is going to cause problems in
     certain drivers that assume they can do !dev->irq. I'd like my drivers to
     work without me having to do anything to them, but there's a lot of
     rubbish drivers out there, even allowing for this. I suspect this is a
     tiny part of the problem, and easily fixed in the drivers in the kernel.

 (2) 0 is a valid IRQ in lots of places, including x86. IIRC it is permissible
     for ISAPNP and PNP-BIOS (and presumably ACPI) to indicate something is
     attached to IRQ 0 (usually only the timer is there though, but it can be
     possible to reconfigure that).
     
     Fortunately, for the FRV arch IRQ 0 is not used - level 0 in the interrupt
     mask register permits all interrupts; and for the AM33 arch, whilst there
     is an IRQ 0, that's the NMI interrupt and so has to be handled specially
     anyway.

     The only reason NO_IRQ on FRV is -1 is that I copied the code from
     elsewhere. It could easily be changed to 0.

 (3) Having to translate a cookie for a specific IRQ means that the IRQ
     handling code will be slower and more complex, or is going to avoid the
     issue and be naughty and not deal with irq == NO_IRQ properly:

     The x86 PIC reports it as IRQ 0 having happened. In which case, by your
     argument, you _have_ to translate it: you're not allowed to pass NO_IRQ to
     setup_irq(), and you're not allowed to pass it to the interrupt handler -
     in this case timer_interrupt(). Doing otherwise is wrong, insane, etc...

     It may even be possible to simplify the x86/x86_64 arch code by making IRQ
     0 a normal IRQ instead of something special.

     The only argument for not doing so is that it's hidden inside the arch
     where it can't be seen... apart from by those looking for examples of good
     code to copy (the i386 arch is used as a model for a lot of things).


I'd like to see dev->irq as a pointer to a structure. As you say, the number is
a cookie, but it's also very much dependent on the bus, so why shouldn't it be
associated with the bus in the same way I/O ports and memory ranges are? I'd
also like to see it arranged as a tree: with FRV, I can add extra PIC's into
the tree, thus expanding the IRQ space available dynamically.

However, as far as the current issue goes, I've no concerns for FRV or AM33
should NO_IRQ become 0.

Whatever you decide to do *please* document this in Documentation/ somewhere!
Then you have a "standard" at which to point and say "so it is written". At the
moment it's documented in the code, and that is inconsistent, perhaps
reasonably.

David
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/